-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[BUG]: upsert()
always uses default tenant/database
#3387
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This reverts commit 74419e1.
3d05519
to
dc878ab
Compare
dc878ab
to
cd614d3
Compare
@@ -91,7 +91,6 @@ def __init__(self, client: ClientAPI): | |||
|
|||
@initialize(collection=collection_st) # type: ignore | |||
def initialize(self, collection: strategies.Collection): | |||
reset(self.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.
this should be fine since the created collection is torn down after each state machine run
otherwise, the reset removes the non-default tenant and database that we created
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 @codetheweb for fixing my bug, and adding tests. I think this approach is fine, except one possible concern around how heavyweight running the entire property test state machine is just to test non-default tenant / database ids.
Approved, with further changes at your discretion. Should we choose not to write them now, let's open an issue where we can discuss the need for some intermediate 'run each endpoint once' test suite.
Description of changes
Bug found by a user. Before this fix, the resource object passed to auth providers would always have the default tenant and default database for calls made via the chromadb Python HTTP client.
Added test was failing before I updated
Collection.py
.Test plan
How are these changes tested?
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?
n/a