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

DRIVERS-2573: Reference test for $$matchAsDocument and $$matchAsRoot #1706

Merged
merged 4 commits into from
Nov 6, 2024

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Oct 30, 2024

https://jira.mongodb.org/browse/DRIVERS-2573

Please complete the following before merging:

Note: I implemented $$matchAsDocument and $$matchAsRoot in PHPLIB in order to POC this, so I'd appreciate it if another language driver could also test this.

jmikola added a commit to jmikola/mongo-php-library that referenced this pull request Oct 30, 2024
jmikola added a commit to jmikola/mongo-php-library that referenced this pull request Oct 30, 2024
@jmikola jmikola marked this pull request as ready for review October 30, 2024 17:58
@jmikola jmikola requested a review from a team as a code owner October 30, 2024 17:58
@jmikola jmikola requested review from alcaeus and removed request for a team October 30, 2024 17:58
@W-A-James W-A-James requested review from ShaneHarvey and removed request for W-A-James October 30, 2024 22:37

# Note: $$lte is not technically in the 1.8 schema but was introduced at the same time.
# Note: $$lte was introduced alongside schema changes for CSOT
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed this file for consistency, as I didn't like the matches-<name>-operator.yml convention. Our other test files are generally prefixed with the logical type of the thing being tested, so I went with operator-<name>.

"1.8" was also never the correct schema version for CSOT.

jmikola added a commit to jmikola/mongo-php-library that referenced this pull request Oct 31, 2024
jmikola added a commit to jmikola/mongo-php-library that referenced this pull request Oct 31, 2024
databaseName: *database0Name
documents:
- { _id: 1, json: '{ "x": 1, "y": 2 }' }
- { _id: 2, json: '{ "x" }' }
Copy link
Member

Choose a reason for hiding this comment

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

You can add an error case where json is a json array.

Suggested change
- { _id: 2, json: '{ "x" }' }
- { _id: 2, json: '{ "x" }' }
- { _id: 3, json: '[{"x":1}]' }

It seems to be in your implementation: https://github.com/mongodb/mongo-php-library/pull/1508/files#diff-5911ac585872dc4a55ff2fe77af552d46e160b4d5636699c9cf2c1211217e25eR278

Copy link
Member Author

Choose a reason for hiding this comment

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

Added some additional valid-fail tests with unexpected JSON strings.

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.

Tests pass locally in Python, running a patch here: mongodb/mongo-python-driver#1988

This uncovered a bunch of issues with Python's implementation of $$matchAsDocument and $$matchAsRoot. Nice work!

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.

LGTM!

jmikola added a commit to jmikola/mongo-php-library that referenced this pull request Nov 1, 2024
@jmikola
Copy link
Member Author

jmikola commented Nov 1, 2024

@ShaneHarvey: I made some test revisions based on @GromNaN's feedback. Also removed a useless test for $$matchAsRoot. Please queue up another CI run and let me know if all is well.

@jmikola jmikola merged commit daf9e07 into mongodb:master Nov 6, 2024
5 checks passed
@jmikola jmikola deleted the drivers-2573 branch November 6, 2024 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants