-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix TypeError when parameters=None in query_items #43681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Fixes Azure#43662 Ensure parameters=None is treated the same as omitting parameters, preventing TypeError in HTTP request layer.
|
Thank you for your contribution @Nikhil172913832! We will review the pull request and get back to you soon. |
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the query parameter handling logic in the query_items method for both synchronous and asynchronous container implementations. The change simplifies the condition for determining when to construct a parameterized query object by checking for parameter existence directly rather than using a utility function.
Key Changes:
- Replaced
utils.valid_key_value_exist(kwargs, "parameters")with a directparameters is not Nonecheck - Extracted
queryandparametersvalues into intermediate variables before the conditional logic
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sdk/cosmos/azure-cosmos/azure/cosmos/container.py | Updated query parameter handling in the synchronous query_items method |
| sdk/cosmos/azure-cosmos/azure/cosmos/aio/_container.py | Updated query parameter handling in the async query_items method |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
The failing test (test_read_items_concurrency_internals) seems unrelated to the changes in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Nikhil172913832 for making these changes, really appreciate it. However, we cannot accept the PR without any test coverage, can you please add tests if possible or if you would like, we can take this change ourselves and add appropriate tests to it, let us know!
|
@kushagraThapar I will add the necessary test. |
|
@kushagraThapar I’ve added the tests for the fix as requested. Let me know if everything looks good. |
Fixes #43662
Ensure parameters=None is treated the same as omitting parameters, preventing TypeError in HTTP request layer.
Description
This PR fixes a TypeError that occurs when
parameters=Noneis explicitly passed tocontainer.query_items()in the Cosmos DB SDK.Problem:
When users explicitly pass
parameters=Nonetoquery_items(), the SDK would create a query dictionary with aNoneparameters value, which would then be passed down to the HTTP request layer causing aTypeError: Session.request() got an unexpected keyword argument 'parameters'.Solution:
Modified the query building logic in both sync and async container implementations to check if
parametershas a non-None value before including it in the query structure. Nowparameters=Noneis treated identically to omitting the parameter entirely, returning a simple query string instead of a dictionary with None values.Changes:
sdk/cosmos/azure-cosmos/azure/cosmos/container.py: Updated query building logic (lines 936-941)sdk/cosmos/azure-cosmos/azure/cosmos/aio/_container.py: Updated async query building logic (lines 824-829)Impact:
parameters=Noneis explicitly passedAll SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines