-
Notifications
You must be signed in to change notification settings - Fork 111
[CLIENT-2383] Fix bug where Query.where() and client.index_cdt_create() do not accept a list of contexts #550
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
CLIENT-2383 Added support for context lists in Client.index_cdt_create() and Client.Query.where() Fixed the check for TestBaseClass.version not working corerctly upon initialization
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #550 +/- ##
==========================================
+ Coverage 83.30% 83.31% +0.01%
==========================================
Files 99 99
Lines 14402 14422 +20
==========================================
+ Hits 11997 12016 +19
- Misses 2405 2406 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
juliannguyen4
left a comment
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.
query.where() is missing a test case.
src/main/conversions.c
Outdated
| PyObject *op_dict, bool *ctx_in_use, | ||
| as_static_pool *static_pool, int serializer_type) | ||
| { | ||
| PyObject *py_ctx = PyDict_GetItemString(op_dict, CTX_KEY); |
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.
I think it might be better to modify Client.index_cdt_create() and Query.where() directly, instead of this helper function. Many other functions use this helper function, so this change might introduce a bug somewhere else in the client.
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.
You are right. I added these changes in the newest commit
Added Documentation Added test cases for ctx dictionary legacy support
Ran precommit lint
Fixed memory leak in get_cdt_ctx
Fixed misuse of Py_Decref
* [CLIENT-3793] Remove macOS 13 support (#846) * Auto-bump version to 18.1.0rc3.dev1 [skip ci] * [CLIENT-3106] Remove dead code in conversions.c (#817) - record_to_resultpyobject() was a helper function for client.batch_get_ops(), which is now removed. - record_to_pyobject_cnvt_list_to_map() and as_list_of_map_to_py_tuple_list(): these were helper functions that were used before Python client version 2.1.3 to return the result of certain map operations. They are no longer used starting from Python client 2.1.3 and higher, so it is safe to remove - bin_strict_type_checking() isn't used anywhere. But it would be good to consolidate the bin checking code into one place, since it is currently spread out all over the codebase - as_batch_read_results_to_pyobject() was used by get_many() which has been removed - batch_read_records_to_pyobject() was used by select_many() which has been removed Extra Changes Merge do_*_to_pyobject() methods into their calling methods, since they have the same function signature * Auto-bump version to 18.1.0rc3.dev2 [skip ci] --------- Co-authored-by: Julian Nguyen <109386615+juliannguyen4@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…longer take in a dictionary to match documentation...
…rieving it from a dictionary entry
…eaking changes to the api
…then index_cdt_create should not raise an exception with its own as_error because it may be initialized to AEROSPIKE_OK
… and reduce repetition
justinlee-aerospike
left a comment
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.
Code looks fine. Can you please add negative tests around parse errors in context?
|
Both query.where and client.index_cdt_create() have test cases where an invalid type object is passed to the ctx parameter
|
|
Bump |
justinlee-aerospike
left a comment
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.
changes look good
Justin already approved and I worked on the requested changes
This approach does not change the helper function get_cdt_ctx() which currently only accepts an optional Python dictionary. We're avoiding changes to get_cdt_ctx() since it is used in many places in cdt_list_operate.c and cdt_map_operate.c. In the future we will refactor cdt_{list,map}_operate.c to make it easier to refactor these type of helper functions
Extra changes:
TestBaseClass.get_new_connection()and not theas_connectionfixture to initialize the Python clientKnown issues
There's some missing code coverage in index_cdt_create() because ctx is a required parameter (non-NULL) and will always be passed to get_cdt_ctx in a Python dictionary. So when get_cdt_ctx succeeds, ctx_in_use will always be true.Manual testing