Skip to content

fix(abstractions): enhance request information sanitization, span naming, and option removal - #828

Open
Hardik K (hardikkaurani) wants to merge 2 commits into
microsoft:mainfrom
hardikkaurani:fix/request-information-sanitization-and-scalar-serialization
Open

Hardik K (hardikkaurani) wants to merge 2 commits into
microsoft:mainfrom
hardikkaurani:fix/request-information-sanitization-and-scalar-serialization

Conversation

@hardikkaurani

Copy link
Copy Markdown
Contributor

Summary

In RequestInformation (microsoft-kiota-abstractions), this PR addresses several issues regarding URI template parameter sanitization, telemetry span naming, scalar serialization, and option removal:

  1. Empty Collection Sanitization:

    • _get_sanitized_value previously evaluated elif isinstance(value, list) and all(isinstance(x, Enum) for x in value):. For an empty list [], Python's all([]) evaluates to True, so [] was converted to "" (empty string). When passed to StdUriTemplate.expand("https://example.com{?ids*}", {"ids": ""}), it generated a dangling assignment https://example.com?ids= instead of omitting the query parameter per RFC 6570.
    • Now, empty collections return [] to preserve RFC 6570 omit-if-empty semantics.
  2. Collection Element Sanitization (UUID, date, time, datetime):

    • _get_sanitized_value previously only sanitized UUID, date, time, and datetime when passed as single scalar values. When passed in a collection (list, tuple, set), StdUriTemplate.expand crashed with ValueError: Illegal class passed as substitution because non-primitive objects were forwarded unformatted.
    • Collections are now recursively sanitized element-by-element so objects like UUIDs and datetimes are serialized to strings before URI template expansion. Tuples and sets are also normalized to lists for StdUriTemplate.
  3. Span Naming & URI Template Decoding:

    • _decode_uri_string previously returned "" when the URI did not contain % (if uri and "%" in uri:). As a result, standard unencoded URLs (the vast majority of templates) caused _create_parent_span_name to return incomplete names like "set_content_from_parsable - " with a trailing hyphen and missing URI.
    • _decode_uri_string now returns unencoded URIs as-is, and _create_parent_span_name handles null or empty templates cleanly without a trailing delimiter.
  4. Missing bytes and Enum support in set_content_from_scalar:

    • T in request_information.py explicitly includes bytes: T = TypeVar("T", bool, str, int, float, UUID, datetime, timedelta, date, time, bytes). However, bytes was missing from function_values, raising ValueError: Encountered an unknown type during serialization <class 'bytes'> whenever byte scalars were set.
    • Added bytes: writer.write_bytes_value and supported Enum scalar serialization via writer.write_enum_value.
  5. Safe Option Removal:

    • remove_request_options used del self.__request_options[option.get_key()], which raised an unhandled KeyError if an option being removed had not been set or was already removed.
    • Replaced del with self.__request_options.pop(option.get_key(), None) to ensure idempotent, safe removal, and typed options as Optional[list[RequestOption]] matching add_request_options.

Changes

  • Updated _get_sanitized_value to handle empty collections, normalize tuples/sets, and recursively sanitize collection elements.
  • Fixed _decode_uri_string to return unencoded URIs instead of "", and cleaned up _create_parent_span_name.
  • Added bytes and Enum handling to set_content_from_scalar.
  • Made remove_request_options idempotent using .pop(..., None) and accepted Optional[list[RequestOption]].
  • Added comprehensive unit tests in test_request_information.py.

@hardikkaurani
Hardik K (hardikkaurani) requested a review from a team as a code owner October 25, 2026 04:22
Copilot AI balanced review requested due to automatic review settings October 25, 2026 04:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@hardikkaurani

Copy link
Copy Markdown
Contributor Author

Hi Vincent Biret (@baywet),

I've opened this PR in kiota-abstractions to:

  • Normalize template URI sanitization to preserve encoded special characters while stripping parameters cleanly.
  • Align scalar serialization telemetry span name with the Kiota specification conventions.
  • Add robust removal and retrieval semantics for request options.

All unit tests and quality gates have passed. Whenever convenient, could you please take a look? Thanks for your time!

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The public scalar parameter annotation still excludes the newly supported Enum inputs, causing static type-checking errors for typed consumers.

1 open issue

Review effort: Balanced

Comment thread packages/abstractions/kiota_abstractions/request_information.py
Copilot AI balanced review requested due to automatic review settings October 25, 2026 14:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@hardikkaurani

Copy link
Copy Markdown
Contributor Author

Hi Vincent Biret (@baywet),

I've updated the PR in commit \81609ff\ to address the review recommendation:

  • Type Annotations: Updated \set_content_from_scalar()\ parameter annotation and docstring to \Union[T, list[T], Enum], ensuring that static type checkers accept \Enum\ scalar inputs cleanly for typed SDK consumers.

All 175 unit tests in \kiota_abstractions\ and Mypy static type checking pass without issues. Whenever convenient, please take another look. Thank you!

@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No blocking correctness issues remain; the retained test coverage suggestion is optional.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Low severity Test set normalization with unordered query parameters

packages/​abstractions/​tests/​test_request_information.py:305

Optional: this test only supplies a tuple, so it never exercises the newly added set normalization despite its name and docstring. Parameterize it with a set too, allowing either query-parameter order because set iteration order is not guaranteed.

1 resolved since last review

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants