-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-7537): add QE String GA - rename TextPreview to String, sync spec tests
#4972
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
c9d4c68
92680fa
53a7ca6
c58dbc6
87f84be
2ef9ea3
6f12668
9b68494
63e6dc4
49f0fac
80c0316
c70d9e7
f5f7b0e
4a2c552
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -746,8 +746,16 @@ export class ClientEncryption { | |
| expressionMode: boolean, | ||
| options: ClientEncryptionEncryptOptions | ||
| ): Promise<Binary> { | ||
| const { algorithm, keyId, keyAltName, contentionFactor, queryType, rangeOptions, textOptions } = | ||
| options; | ||
| const { | ||
| algorithm, | ||
| keyId, | ||
| keyAltName, | ||
| contentionFactor, | ||
| queryType, | ||
| rangeOptions, | ||
| stringOptions, | ||
| textOptions | ||
| } = options; | ||
| const contextOptions: ExplicitEncryptionContextOptions = { | ||
| expressionMode, | ||
| algorithm | ||
|
|
@@ -780,8 +788,9 @@ export class ClientEncryption { | |
| contextOptions.rangeOptions = serialize(rangeOptions); | ||
| } | ||
|
|
||
| if (typeof textOptions === 'object') { | ||
| contextOptions.textOptions = serialize(textOptions); | ||
| const resolvedStringOptions = stringOptions ?? textOptions; | ||
|
PavelSafronov marked this conversation as resolved.
|
||
| if (typeof resolvedStringOptions === 'object') { | ||
| contextOptions.textOptions = serialize(resolvedStringOptions); | ||
| } | ||
|
Comment on lines
+791
to
794
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disagree here, we already preferred new
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to accept both (I'm not sure we do), then we should flip the order of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't remove it, that's why I'm ok to add this suggested condition and disallow passing both options at the same time, is that what you suggest?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. When both settings are specified, it seems appropriate here to coalesce between the old and the new. I was thinking of throwing, but it looks like in the driver we typically only do that for conflicting options, not for renamed ones. Is there a plan to drop the old property? Should we add a deprecation notice, or invoke |
||
|
|
||
| const valueBuffer = serialize({ v: value }); | ||
|
|
@@ -815,6 +824,8 @@ export interface ClientEncryptionEncryptOptions { | |
| | 'Indexed' | ||
| | 'Unindexed' | ||
| | 'Range' | ||
| | 'String' | ||
| /** @deprecated Use `'String'` instead. */ | ||
|
PavelSafronov marked this conversation as resolved.
addaleax marked this conversation as resolved.
|
||
| | 'TextPreview'; | ||
|
|
||
| /** | ||
|
|
@@ -833,26 +844,38 @@ export interface ClientEncryptionEncryptOptions { | |
| /** | ||
| * The query type. | ||
| */ | ||
| queryType?: 'equality' | 'range' | 'prefixPreview' | 'suffixPreview' | 'substringPreview'; | ||
| queryType?: | ||
| | 'equality' | ||
| | 'range' | ||
| | 'prefix' | ||
| | 'suffix' | ||
| /** @deprecated Use `'prefix'` instead. */ | ||
| | 'prefixPreview' | ||
| /** @deprecated Use `'suffix'` instead. */ | ||
| | 'suffixPreview' | ||
| /** @experimental Public Technical Preview: `substringPreview` is an experimental feature and may break at any time. */ | ||
| | 'substringPreview'; | ||
|
PavelSafronov marked this conversation as resolved.
|
||
|
|
||
| /** The index options for a Queryable Encryption field supporting "range" queries.*/ | ||
| rangeOptions?: RangeOptions; | ||
|
|
||
| /** Options for a Queryable Encryption field supporting string queries. Only valid when `algorithm` is `'String'`. */ | ||
| stringOptions?: StringQueryOptions; | ||
|
|
||
| /** | ||
| * Options for a Queryable Encryption field supporting text queries. Only valid when `algorithm` is `TextPreview`. | ||
| * Options for a Queryable Encryption field supporting text queries. Only valid when `algorithm` is `'String'`. | ||
|
PavelSafronov marked this conversation as resolved.
|
||
| * | ||
| * @experimental Public Technical Preview: `textPreview` is an experimental feature and may break at any time. | ||
| * @deprecated Use `stringOptions` instead. | ||
| */ | ||
| textOptions?: TextQueryOptions; | ||
| textOptions?: StringQueryOptions; | ||
| } | ||
|
|
||
| /** | ||
| * Options for a Queryable Encryption field supporting text queries. | ||
| * Options for a Queryable Encryption field supporting string queries. | ||
| * | ||
| * @public | ||
| * @experimental Public Technical Preview: `textPreview` is an experimental feature and may break at any time. | ||
| */ | ||
| export interface TextQueryOptions { | ||
| export interface StringQueryOptions { | ||
| /** Indicates that text indexes for this field are case sensitive */ | ||
| caseSensitive: boolean; | ||
| /** Indicates that text indexes for this field are diacritic sensitive. */ | ||
|
|
@@ -872,6 +895,9 @@ export interface TextQueryOptions { | |
| strMinQueryLength: Int32 | number; | ||
| }; | ||
|
|
||
| /** | ||
| * @experimental Public Technical Preview: `substring` is an experimental feature and may break at any time. | ||
| */ | ||
| substring?: { | ||
| /** The maximum allowed length to insert. */ | ||
| strMaxLength: Int32 | number; | ||
|
|
@@ -882,6 +908,12 @@ export interface TextQueryOptions { | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * @public | ||
| * @deprecated Use {@link StringQueryOptions} instead. | ||
| */ | ||
| export type TextQueryOptions = StringQueryOptions; | ||
|
|
||
| /** | ||
| * @public | ||
| * @experimental | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,7 @@ describe('$lookup support', defaultMetadata, function () { | |
| * db.qe2 with options: { "encryptedFields": "<schema-qe2.json>"}. | ||
| * db.no_schema with no options. | ||
| * db.no_schema2 with no options. | ||
| * db.non_csfle_schema with options: { "validator": { "$jsonSchema": "<schema-non-csfle.json>"}}. | ||
| * ``` | ||
| */ | ||
| const collections = [ | ||
|
|
@@ -115,6 +116,11 @@ describe('$lookup support', defaultMetadata, function () { | |
| name: 'no_schema2', | ||
| options: {}, | ||
| document: { no_schema2: 'no_schema2' } | ||
| }, | ||
| { | ||
| name: 'non_csfle_schema', | ||
| options: { validator: { $jsonSchema: await readFixture('schema-non-csfle.json') } }, | ||
| document: { non_csfle_schema: 'non_csfle_schema' } | ||
| } | ||
| ]; | ||
|
|
||
|
|
@@ -126,6 +132,7 @@ describe('$lookup support', defaultMetadata, function () { | |
| unencryptedClient = this.configuration.newClient({}, { writeConcern: { w: 'majority' } }); | ||
|
|
||
| /** | ||
| * Insert documents with encryptedClient: | ||
| * ``` | ||
| * {"csfle": "csfle"} into db.csfle | ||
| * Use the unencrypted client to retrieve it. Assert the csfle field is BSON binary. | ||
|
|
@@ -137,12 +144,13 @@ describe('$lookup support', defaultMetadata, function () { | |
| * Use the unencrypted client to retrieve it. Assert the qe2 field is BSON binary. | ||
| * {"no_schema": "no_schema"} into db.no_schema | ||
| * {"no_schema2": "no_schema2"} into db.no_schema2 | ||
| * {"non_csfle_schema": "non_csfle_schema"} into db.non_csfle_schema | ||
| * ``` | ||
| */ | ||
| for (const { name, document } of collections) { | ||
| const { insertedId } = await encryptedClient.db('db').collection(name).insertOne(document); | ||
|
|
||
| if (name.startsWith('no_')) continue; | ||
| if (name.startsWith('no_') || name === 'non_csfle_schema') continue; | ||
|
|
||
| expect(await unencryptedClient.db('db').collection(name).findOne(insertedId)) | ||
| .to.have.property(Object.keys(document)[0]) | ||
|
|
@@ -325,6 +333,11 @@ describe('$lookup support', defaultMetadata, function () { | |
| { requires: { ...defaultMetadata.requires, mongodb: '>=8.1.0' } } | ||
| ); | ||
|
|
||
| // Case 8 expects one of two error messages depending on mongocryptd/crypt_shared and libmongocrypt versions: | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Case 8 accepts two error messages because the server changed the error text in 8.2 via MONGOCRYPT-793. See spec prose test 25 Case 8. |
||
| // - mongocryptd/crypt_shared <8.2 or libmongocrypt <1.17.0: "not supported" | ||
| // - mongocryptd/crypt_shared 8.2+ and libmongocrypt 1.17.0+: | ||
| // "Cannot specify both encryptionInformation and csfleEncryptionSchemas unless | ||
| // csfleEncryptionSchemas only contains non-encryption JSON schema validators" | ||
| test( | ||
| 'Case 8: db.csfle joins db.qe', | ||
| 'csfle', | ||
|
|
@@ -339,7 +352,7 @@ describe('$lookup support', defaultMetadata, function () { | |
| }, | ||
| { $project: { _id: 0 } } | ||
| ], | ||
| /not supported/i, | ||
| /not supported|Cannot specify both encryptionInformation and csfleEncryptionSchemas/i, | ||
| { requires: { ...defaultMetadata.requires, mongodb: '>=8.1.0' } } | ||
| ); | ||
|
|
||
|
|
@@ -360,4 +373,26 @@ describe('$lookup support', defaultMetadata, function () { | |
| /Upgrade/i, | ||
| { requires: { ...defaultMetadata.requires, mongodb: '>=7.0.0 <8.1.0' } } | ||
| ); | ||
|
|
||
| // Case 10 requires server 8.2+, mongocryptd/crypt_shared 8.2+, and libmongocrypt 1.17.0+. | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Case 10 is new in NODE-7221: verifies that a QE collection can |
||
| test( | ||
| 'Case 10: db.qe joins db.non_csfle_schema', | ||
| 'qe', | ||
| [ | ||
| { $match: { qe: 'qe' } }, | ||
| { | ||
| $lookup: { | ||
| from: 'non_csfle_schema', | ||
| as: 'matched', | ||
| pipeline: [ | ||
| { $match: { non_csfle_schema: 'non_csfle_schema' } }, | ||
| { $project: { _id: 0, __safeContent__: 0 } } | ||
| ] | ||
| } | ||
| }, | ||
| { $project: { _id: 0, __safeContent__: 0 } } | ||
| ], | ||
| { qe: 'qe', matched: [{ non_csfle_schema: 'non_csfle_schema' }] }, | ||
| { requires: { ...defaultMetadata.requires, mongodb: '>=8.2.0' } } | ||
| ); | ||
| }); | ||
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.
Should this be
^7.1.0so the latest libomngocrypt always gets picked up?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.
Yes! It should, great catch