[Hardik K (hardikkaurani)]

Summary

In ParametersNameDecodingHandler (microsoft-kiota-http), there were four issues in parameter decoding, URL manipulation, and telemetry:

1. Parameter Value Corruption: decode_uri_encoded_string decoded parameters by calling original.replace(parameter, new_parameter) on the whole query string. If a query parameter's value happened to contain an encoded parameter name (e.g., query=%24select&$select=1), the value was erroneously decoded (query=$select), corrupting query parameter values.
2. Case Sensitivity: Percent-encoding was generated using uppercase hex only (f"%{ord(x):X}"), failing to decode standard lowercase percent-encoding (such as %2d, %2e, %7e).
3. URL Path Corruption Risk: original_url.replace(query_params, decoded_query_parameters_string) replaced the first occurrence in the entire URL, corrupting paths that contain identical substrings. Replacing query via request.url.copy_with(query=...) safely updates only the query component.
4. Span Lifecycle & Null Check:
   • Validates if request is None: raise TypeError("request cannot be null") matching other middleware.
   • Ends observability span in finally over the super().send call instead of ending prematurely before the request is sent.

Changes

• Updated decode_uri_encoded_string to split query parameters and decode exclusively parameter names (supporting both uppercase and lowercase hex), leaving parameter values untouched.
• Replaced URL string replacement with request.url.copy_with(query=...).
• Wrapped super().send() in try ... finally: span.end() and added request is None guard check.
• Added comprehensive unit tests in test_parameters_name_decoding_handler.py.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Hardik K (hardikkaurani)]

Hi Vincent Biret (@baywet)! This PR refines ParametersNameDecodingHandler by decoding parameter names on split query segments instead of doing a global string replace (which inadvertently decoded parameter values containing encoded names), supports lowercase percent-encoding, uses request.url.copy_with to protect paths from string replacement collisions, adds a null check, and keeps the observability span active across the send execution. Unit tests covering all scenarios are included. Whenever convenient, please take a look. Thank you!

[Copilot]

🟡 Changes recommended

The span-lifecycle change needs regression coverage for successful and failing transport execution.

1 open issue

• Test span lifecycle and exception propagation around transport execution · New

_{Review effort: Balanced}

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Hardik K (hardikkaurani)]

Hi Vincent Biret (@baywet),

I have updated the PR to address the feedback regarding span lifecycle coverage:

• Added est_span_lifecycle_active_during_send_and_ends_on_success: verifies that span.end() has not been called while the downstream transport call is in flight, and is called exactly once after send() successfully completes.
• Added est_span_lifecycle_ends_and_exception_propagates_on_transport_failure: verifies that span.end() has not been called while transport is in flight, verifies that the transport exception propagates, and confirms that span.end() is invoked in the inally: block.
• Positioned the ry: block immediately after _create_observability_span(...) in ParametersNameDecodingHandler.send() to guarantee span closure across all paths.
• Added explicit typing for query decoding collections to pass mypy without warnings or type mismatches.
• Verified all code formatting (yapf), import ordering (isort), linting (pylint), type checking (mypy), and the full pytest suite (all 186 tests passing).

The PR is ready for review. Thank you!

[Copilot]

🟢 Approval recommended

No blocking defects remain; the suggested path-regression test improvement is nonblocking.

2 open issues

• Test encoded query substring in path to catch copy_with corruption · New
• Test span lifecycle and exception propagation around transport execution

_{Review effort: Balanced}

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

🟢 Approval recommended

The focused changes address the reported defects, with targeted regression coverage and no unresolved findings.

1 open issue

• Test span lifecycle and exception propagation around transport execution

1 resolved since last review

• Test encoded query substring in path to catch copy_with corruption

_{Review effort: Balanced}

[Hardik K (hardikkaurani)]

Hi Vincent Biret (@baywet),

I have addressed the review comment in commit 6d506cb.

test_decoding_preserves_url_path has been updated so that the URL path contains the full query parameter substring (%24select=name in https://example.com/api/%24select=name/items?%24select=name). The test verifies that the path remains fully encoded (%24select=name) in both str(resp.request.url) and resp.request.url.raw_path, while the query parameter is properly decoded to $select=name.

All formatting, lint checks, static type checks (mypy), unit tests, and CI workflows have passed. Ready for review!

[Vincent Biret (baywet)]

Thanks for the contribution!

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud