-
-
Notifications
You must be signed in to change notification settings - Fork 513
[Encryption] Add Int64 type #2805
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
96265de
to
ef61ca3
Compare
@@ -145,6 +145,7 @@ Here is a quick overview of the built-in mapping types: | |||
- ``hash`` | |||
- ``id`` | |||
- ``int`` | |||
- ``int64`` |
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.
Contrary to the PR description, int64
might have uses beyond encryption such as satisfying a general schema or improving compatibility in cross-language apps (other drivers might not play nice with interchangeable integer types).
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.
Ok, the new type can be used independently from encryption.
Note that the int
type will encode to Int64 when the value exceeds 32 bits. That would be a programmer issue. I could add an Int32 type to throw an error when the value does not fit in 32 bits.
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.
Note that the int type will encode to Int64 when the value exceeds 32 bits.
That's legacy behavior for ODM and the expected behavior for the server (e.g. incrementing an int32 BSON type server-side could yield an int64), so I'm not worried about it. And so long as ODM continues to accept int64 BSON types for int
mappings I think we're OK.
I could add an Int32 type to throw an error when the value does not fit in 32 bits.
This could certainly be added down the line. I'd probably wait until it was requested, though.
@@ -10,6 +10,9 @@ | |||
<field name="intField" type="int"> | |||
<encrypt queryType="range" min="5" max="10"/> | |||
</field> | |||
<field name="int64Field" type="int64"> | |||
<encrypt queryType="range" min="5" max="9223372036854775802"/> |
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.
Is it even necessary to use PHP_INT_MAX
for this example? Since Int64 types allow comparisons with integers, the assertion in AbstractDriverTestCase doesn't actually check that the value is converted to an Int64 object. It may make more sense to use relaxed numeric comparisons for the assertEquals
and then also assert that the value is an instance of Int64. In that case you can do without the large values here.
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 added a type assertion in TypeTest
.
My idea for using a low and a high value for the min and max was to test that they are both converted to Int64
. I could add a functional test for QE.
b7a7409
into
doctrine:feature/queryable-encryption
Summary
Queryable Encryption is more strict. While the PHP driver automatically switch the type of the BSON value between
int
(32 bits) orlong
(64 bits) depending on the actual value, the encrypted fields configuration requires a unique type.This new
Int64
type is only useful for very large int encrypted fields.