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

fix(NODE-3150): allow retrieving PCRE-style RegExp #2840

Merged
merged 6 commits into from
Jun 25, 2021
Merged

Conversation

andymina
Copy link
Contributor

@andymina andymina commented Jun 14, 2021

Description

  • Allowed retrieving PCRE-style RegExp by adding bsonRegExp option to commands, collections, and dbs
  • Added unit tests for the bsonRegExp option

@andymina andymina marked this pull request as draft June 14, 2021 14:54
@durran
Copy link
Member

durran commented Jun 15, 2021

I noticed our BSON library has this option to deserialize to the native RegExp or our BSONRegExp type, which to me is unexpected. We ran into this type of issue years ago in the Ruby driver, and the way we handled it there was to always deserialize to the BSON type, and if the user wanted the native type we added a compile method to it for them to get the native one. This seems a bit more intuitive to me than having to supply the option everywhere. @nbbeeken Maybe you have some context on why the BSON library is doing this? For reference https://github.com/mongodb/bson-ruby/blob/master/lib/bson/regexp.rb

Also to note is that the Java BSON library also behaves in the same way as Ruby, so that may be common across more drivers as well.

@nbbeeken
Copy link
Contributor

I don't have further background on this, we have a precedent of other values being auto converted with the promoteValues and like settings. However, I know it is a strict requirement that BSON libraries have an API that explicitly converts UUIDs to native types, so that is a precedence for the opposite (as well as the regex examples from other libs you mention). There is convenience in handing the user the native type, up until it fails because of the contents.

Maybe as a separate effort we should take a look at the BSON library catching a Regexp decoding error and using BSONRegex automatically instead of failing, although I imagine that would also need a flag.

@durran
Copy link
Member

durran commented Jun 15, 2021

Yeah it's a weird one since it is backwards breaking anyways so I guess we are stuck with a flag. Thanks for the clarification.

@andymina andymina requested a review from durran June 15, 2021 17:40
@andymina andymina added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 15, 2021
@andymina andymina marked this pull request as ready for review June 15, 2021 17:40
src/connection_string.ts Show resolved Hide resolved
@durran
Copy link
Member

durran commented Jun 15, 2021

I would also change the description not to mention findOne specifically, as the option is set on the command response before deserialization, which will actually take care of all commands (not just findOne).

@durran durran self-assigned this Jun 15, 2021
@nbbeeken nbbeeken requested review from emadum and dariakp June 17, 2021 18:26
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jun 17, 2021
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Some test clean up requests, driver changes LGTM

Comment on lines 15 to 16
const db = client.db('a', { bsonRegExp: true });
const collection = db.collection('b');
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use a better db/collection name here

test/unit/bson_regex.test.js Outdated Show resolved Hide resolved
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

I'm LGTM once @nbbeeken 's test change requests are in.

src/sdam/server.ts Outdated Show resolved Hide resolved
@andymina andymina requested a review from nbbeeken June 23, 2021 21:24
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

LGTM

@durran durran merged commit ca9e2dc into 4.0 Jun 25, 2021
@durran durran deleted the NODE-3150/bson-regex branch June 25, 2021 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants