Skip to content

Conversation

blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 2, 2022

@blink1073
Copy link
Member Author

Test failures are being addressed in PYTHON-3286 and PYTHON-3319.

@blink1073 blink1073 requested a review from ShaneHarvey June 29, 2022 15:24
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

A few final comments.



binary_types = (Binary, bytes)
binary_types = (Binary, bytes, uuid.UUID)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little concerned this might lead to masking bugs where we expect to be sending (or returning) Binary but actually end up with UUID. Can we remove this now that we use uuid_representation=unspecified?

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying it out

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, the issue is actually coming from the expected event. We enforce that _ids given to create_data_key are of the type UUID, but the expected event wants a Binary _id. I don't see a precedent for altering the events, but it seems like that might be needed here.

Copy link
Member

@ShaneHarvey ShaneHarvey Jun 29, 2022

Choose a reason for hiding this comment

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

 [2022/06/29 21:48:53.819] FAIL: test_create_data_key_with_AWS_KMS_provider (test.test_encryption.TestUnifiedCreateDataKey) [2022/06/29 21:48:53.819] ---------------------------------------------------------------------- [2022/06/29 21:48:53.819] Traceback (most recent call last): ... [2022/06/29 21:48:53.819] self.test.assertIsInstance(value, permissible_types) [2022/06/29 21:48:53.819] AssertionError: UUID('55c9396c-0d87-4128-93b0-095b806818cb') is not an instance of (<class 'bson.binary.Binary'>, <class 'bytes'>)

We should fix these errors by making _KEY_VAULT_OPTS use unspecified UUID representation (the default) instead of STANDARD. The "data_key _id must be a UUID" check will need to be updated to check for Binary with UUID_SUBTYPE too.

Note that _DATA_KEY_OPTS should still keep using uuid_representation=STANDARD otherwise we may break b/c for encoding a schema_map that contains UUIDs. _KEY_VAULT_OPTS is only used internally so we are free to change it as we please.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@blink1073
Copy link
Member Author

I updated the pymongocrypt dependency to point to the commit merging mongodb/libmongocrypt#375

if isinstance(data_key_id, Binary):
if data_key_id.subtype != UUID_SUBTYPE:
raise TypeError("data_key _id must have a UUID subtype")
elif not isinstance(data_key_id, uuid.UUID):
Copy link
Member

Choose a reason for hiding this comment

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

We should only accept Binary, never UUID here. Let's also update the return below:

- return Binary(data_key_id.bytes, subtype=UUID_SUBTYPE) + return data_key_id 
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@blink1073 blink1073 requested a review from ShaneHarvey June 30, 2022 14:12
@blink1073 blink1073 merged commit b37b146 into mongodb:master Jun 30, 2022
@blink1073 blink1073 deleted the PYTHON-3053 branch June 30, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants