Sanitize IDictionary query parameters to skip null values by bkoelman · Pull Request #755 · microsoft/kiota-dotnet

Bart Koelman (bkoelman) · 2026-07-23T00:09:26Z

Copilot

Pull request overview

This PR updates RequestInformation URL template expansion to properly handle IDictionary query parameter values by dropping entries with null values, enabling “free-form” query parameters (e.g., JSON:API-style dictionaries) and aligning behavior with RFC 6570 undefined-value semantics.

Changes:

Add IDictionary handling to RequestInformation sanitization so dictionary entries with null values are omitted during Std.UriTemplate expansion.
Add unit tests covering exploded dictionary query parameters, null skipping, empty-string inclusion, and AddQueryParameters integration.
Extend the test query-parameter model to include a Query dictionary property.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/abstractions/RequestInformation.cs	Adds dictionary sanitization to drop null entries before URI template expansion.
tests/abstractions/RequestInformationTests.cs	Adds coverage for dictionary-based query parameter expansion and null-handling behavior.

Bart Koelman (bkoelman) · 2026-07-23T00:35:37Z

+            foreach(DictionaryEntry entry in dict)
+            {
+                if(entry.Value is not null)
+                {
+                    result[(string)entry.Key] = GetSanitizedValue(entry.Value).ToString()!;
+                }
+            }


According to the plan (section PR 2), this is beyond the scope of this change. The dictionary signature of the client code generated by kiota should be Dictionary<string, string>, which can't contain null or non-string keys, so taking this change would basically introduce dead code.

Bart Koelman (bkoelman) · 2026-07-23T00:28:41Z

+
+            // Assert
+            var url = requestInfo.URI.OriginalString;
+            Assert.DoesNotContain("include", url);


This is intentional; the include parameter must not appear, whether or not it ends with an = sign.

Bart Koelman (bkoelman) · 2026-07-23T00:27:43Z

+            // page[size] key is percent-encoded by RFC 6570 {?} operator
+            Assert.Contains("page", url);
+            Assert.Contains("10", url);


The goal of these tests is not to verify encoding/formatting, so if that part of the project ever changes, they should not break these tests.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/abstractions/RequestInformation.cs:134

SanitizeDictionary casts entry.Key to string, which will throw an InvalidCastException (and with a fairly opaque message) if a consumer passes an IDictionary with non-string keys. Also, forcing .ToString() here is inconsistent with ExpandArray/scalar sanitization (which keep non-string primitives as objects) and can subtly change formatting (e.g., culture-sensitive decimal/double). Consider validating keys and keeping sanitized values as object so Std.UriTemplate can handle formatting consistently.

        private static Dictionary<string, string> SanitizeDictionary(IDictionary dict)
        {
            var result = new Dictionary<string, string>(dict.Count);
            foreach(DictionaryEntry entry in dict)
            {
                if(entry.Value is not null)
                {
                    result[(string)entry.Key] = GetSanitizedValue(entry.Value).ToString()!;
                }

tests/abstractions/RequestInformationTests.cs:951

The assertions for the page[size] query parameter are very permissive (Contains("page") / Contains("10")) and don't actually verify the percent-encoding behavior described in the comment. This test can pass even if the key is not encoded correctly or if 10 appears elsewhere in the URL.

            var url = requestInfo.URI.OriginalString;
            Assert.Contains("include=author", url);
            // page[size] key is percent-encoded by RFC 6570 {?} operator
            Assert.Contains("page", url);
            Assert.Contains("10", url);

Vincent Biret (baywet)

Thanks for the contribution!

Vincent Biret (baywet) · 2026-07-23T12:47:10Z

+        /// </summary>
+        private static Dictionary<string, string> SanitizeDictionary(IDictionary dict)
+        {
+            var result = new Dictionary<string, string>(dict.Count);


Can't we use Dictionary<string, object> so we can conform with the rest of the sanitization approach and we don't have to call ToString() below (which could potentially cause issues with number formatting etc... down the road)

Pushed, but that doesn't make sense to me at all. It's something entirely different from what's in the plan:

I've always assumed that dictionary values must be constrained to strings, which is what they are in an HTTP query string. Are you suggesting we deviate from the plan and switch to object values in the generated APIs?

sorry if I wasn't clear. I was only suggesting that we update the return type of the private method to align with the rest.

In terms of usage, the users should able to use dictionary<string, string | int | float |.... > (any primitive type) on their query parameters class if that makes sense?

… values Co-authored-by: Cursor <cursoragent@cursor.com>

…nary query parameters Update SanitizeDictionary return type from Dictionary<string, string> to Dictionary<string, object> and route dictionary entry values through GetSanitizedValues instead of calling .ToString()!. This avoids forcing string conversions on numbers, booleans, and dates in dictionary query parameters, allowing Std.UriTemplate to handle formatting natively according to RFC 6570. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Bart Koelman (bkoelman) · 2026-07-24T01:55:17Z

+            foreach(DictionaryEntry entry in dict)
+            {
+                if(entry.Value is not null)
+                {
+                    result[entry.Key.ToString()!] = GetSanitizedValues(entry.Value);
+                }
+            }


This matches the local formatting style of this specific file. Adding spaces introduces inconsistency.

Copilot AI review requested due to automatic review settings July 23, 2026 00:09

github-project-automation Bot added this to Kiota Jul 23, 2026

Copilot started reviewing on behalf of Bart Koelman (bkoelman) July 23, 2026 00:10 View session

Bart Koelman (bkoelman) force-pushed the feat/sanitize-dictionary-null-entries branch from 01bb318 to 2ba7a12 Compare July 23, 2026 00:12

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 23, 2026 00:13

Copilot started reviewing on behalf of Bart Koelman (bkoelman) July 23, 2026 00:13 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Bart Koelman (bkoelman) marked this pull request as ready for review July 23, 2026 00:39

Bart Koelman (bkoelman) requested a review from a team as a code owner July 23, 2026 00:39

Bart Koelman (bkoelman) mentioned this pull request Jul 23, 2026

How to pass a free-format query string? microsoft/kiota#3800

Open

Vincent Biret (baywet) requested changes Jul 23, 2026

View reviewed changes

github-project-automation Bot moved this to In Progress 🚧 in Kiota Jul 23, 2026

Bart Koelman (bkoelman) and others added 2 commits July 24, 2026 03:24

fix(abstractions): sanitize IDictionary query parameters to skip null…

c70d48c

… values Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI review requested due to automatic review settings July 24, 2026 01:39

Bart Koelman (bkoelman) force-pushed the feat/sanitize-dictionary-null-entries branch from 2ba7a12 to e147dd9 Compare July 24, 2026 01:39

Copilot started reviewing on behalf of Bart Koelman (bkoelman) July 24, 2026 01:39 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Navigation Menu

Uh oh!

Sanitize IDictionary query parameters to skip null values - #755

Sanitize IDictionary query parameters to skip null values#755
Bart Koelman (bkoelman) wants to merge 2 commits into
microsoft:mainfrom
bkoelman:feat/sanitize-dictionary-null-entries

Bart Koelman (bkoelman) commented Jul 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Bart Koelman (bkoelman) Jul 23, 2026

Uh oh!

Bart Koelman (bkoelman) Jul 23, 2026

Uh oh!

Bart Koelman (bkoelman) Jul 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Vincent Biret (baywet) left a comment

Uh oh!

Vincent Biret (baywet) Jul 23, 2026

Uh oh!

Bart Koelman (bkoelman) Jul 24, 2026

Uh oh!

Vincent Biret (baywet) Jul 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Bart Koelman (bkoelman) Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Bart Koelman (bkoelman) commented Jul 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Bart Koelman (bkoelman) Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Bart Koelman (bkoelman) Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Bart Koelman (bkoelman) Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Vincent Biret (baywet) left a comment

Choose a reason for hiding this comment

Uh oh!

Vincent Biret (baywet) Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Bart Koelman (bkoelman) Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Vincent Biret (baywet) Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Bart Koelman (bkoelman) Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants