-
Notifications
You must be signed in to change notification settings - Fork 3.2k
None Options for Container APIs #44098
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
Conversation
…into tvaron3/noneOptions
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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 fixes a bug where passing None values for optional parameters in Azure Cosmos SDK container operations would cause unexpected errors. The fix updates the valid_key_value_exist utility function to remove keys with None values from kwargs before processing, and adds comprehensive test coverage for both sync and async clients.
Key Changes:
- Modified
valid_key_value_existfunction to handle and removeNonevalues from kwargs - Added comprehensive test suites for sync and async clients to verify all container methods handle
Noneoptions correctly - Updated CHANGELOG to document the bug fix
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/cosmos/azure-cosmos/azure/cosmos/_utils.py | Updated valid_key_value_exist to remove keys with None values, preventing downstream errors |
| sdk/cosmos/azure-cosmos/tests/test_none_options.py | Added comprehensive sync test suite validating container operations with None parameters |
| sdk/cosmos/azure-cosmos/tests/test_none_options_async.py | Added comprehensive async test suite validating container operations with None parameters |
| sdk/cosmos/azure-cosmos/CHANGELOG.md | Documented the bug fix for None option handling |
| sdk/cosmos/azure-cosmos/tests/workloads/run_workloads.sh | Removed wait statement to allow script to exit after starting workloads |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
* change workloads based on feedback * add staging yml file * add staging yml file * test_none_options * test_none_options * update changelog * revert unrelated file * react to copilot comments
Description
This pull request addresses a bug in the Azure Cosmos SDK where passing
Nonefor certain options in methods likequery_itemscould cause unexpected errors. The main fix involves updating utility logic to handleNonevalues gracefully, and comprehensive new tests have been added for both sync and async clients to ensure all container operations work correctly when optional parameters are set toNone.Bug Fixes and Logic Improvements
valid_key_value_existfunction in_utils.pyto remove keys withNonevalues fromkwargs, preventing them from causing errors in downstream logic.CHANGELOG.mdto inform users about the resolution of issues when passingNonefor options inquery_items.Testing Enhancements
test_none_options.pyfor sync clients to verify that all container methods handleNoneoptions without error. This covers item creation, reading, updating, deleting, batch operations, and change feed queries.test_none_options_async.pyfor async clients, mirroring the sync tests to ensure consistent behavior withNoneoptions in all relevant container methods.Miscellaneous
run_workloads.shby removing an unnecessarywaitstatement making it more convenient to run.All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines