Skip to content
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

MOTOR-969 Support for Range Indexes #195

Merged
merged 12 commits into from Mar 15, 2023
Merged

Conversation

juliusgeo
Copy link
Contributor

No description provided.

@@ -149,6 +149,8 @@ def __init__(self, attr_name, doc=None):

def create_attribute(self, cls, attr_name):
name = self.attr_name or attr_name
if name == "encrypt_expression":
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could instead add None as the last arg to getattr() and return if it is None.

Copy link
Contributor Author

@juliusgeo juliusgeo Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern with doing it that way is that we might forget to get rid of this workaround later. In general I think we do want to raise an error in this case (for example method_that_is_mispelled = AsyncCommand()), so I want it to be as specific as possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unconditionally gets rid of the attribute though right? I think we need to first check if it exists, and then check if it is in an expected list of ones that may not exist yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that is a very good point. Done.

motor/core.py Outdated
@@ -1979,6 +1979,7 @@ class AgnosticClientEncryption(AgnosticBase):
add_key_alt_name = AsyncCommand()
get_key_by_alt_name = AsyncCommand()
remove_key_alt_name = AsyncCommand()
encrypt_expression = AsyncCommand()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we remove the create_attribute changes and instead do:

if hasattr(ClientEncryption, "encrypt_expression"):
    encrypt_expression = AsyncCommand()

Otherwise (with the current solution) AgnosticClientEncryption will end up with encrypt_expression = None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Done.

kms_provider="local",
)
with self.assertRaises(pymongo.errors.WriteError) as exc:
coll.insert_one({"ssn": "123-45-6789"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this working without the await? What is the type of coll here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How was it passing before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before there were no awaits, so the function calls were returning immediately and not raising any exceptions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently this was failing before.

@juliusgeo juliusgeo marked this pull request as ready for review March 13, 2023 17:19
@blink1073
Copy link
Member

Can you pull from master to pick up the tox fix from #194?

@juliusgeo
Copy link
Contributor Author

@blink1073 I appear to already have that commit:

(motor3.9) ➜  motor git:(MOTOR-969) git log | grep 194
   MOTOR-1095 Make Rust Compiler available for Cryptography build (#194)

@blink1073
Copy link
Member

Hmm, then we might need to use ignore_basepython_conflict = true, https://tox.wiki/en/latest/config.html#python-language-core-options

@juliusgeo
Copy link
Contributor Author

@blink1073 I opened https://jira.mongodb.org/browse/MOTOR-1104 to track that work.

@juliusgeo juliusgeo merged commit f4422dd into mongodb:master Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants