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(createCollection): Db.createCollection should pass readConcern to new collection #2026

Merged
merged 13 commits into from Jun 21, 2019

Conversation

rosemaryy
Copy link
Contributor

@rosemaryy rosemaryy commented Jun 13, 2019

Fixes Node-1790

Description

Adhere to the spec by having the db.createCollection create a collection with the same readConcern of db if the readConcern isn't otherwise specified.

What changed?
In Db.createCollection, inherit the readConcern of the database db if none are specified.

@rosemaryy
Copy link
Contributor Author

Failing the below test because the tests doesn't expect for db.createCollection to always create a new field for ReadConcern. Not sure if I should change the test to always expect a readConcern or if I should modify the code so that the new collection doesn't inherit db's readConcern value if db's readConcern value is empty.

expected
{ create: 'test', viewOn: 'users', pipeline: [ { '$match': {} } ], readConcern: {} }
to deeply equal
{ create: 'test', viewOn: 'users', pipeline: [ { '$match': {} } ] }

Copy link
Contributor

@kvwalker kvwalker left a comment

Choose a reason for hiding this comment

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

Looks good overall! Can you write a test to show that a collection created with db.createCollection does inherit the db's readConcern?

Also, I think this commit should be fix(createCollection) instead of perf(readConcern)!

@rosemaryy rosemaryy changed the title perf(readConcern): Db.createCollection should pass readConcern to new collection fix(createCollection): Db.createCollection should pass readConcern to new collection Jun 13, 2019
lib/core/sessions.js Outdated Show resolved Hide resolved
test/functional/readconcern_tests.js Outdated Show resolved Hide resolved
@kvwalker
Copy link
Contributor

Okay so we're seeing the test failures because of the error The field 'readConcern' is not a valid collection option, which makes sense because readConcern is not listed in the options for createCollection in the docs. We can get around this by modifying create_collection.js's illegalCommandFields to include readConcern.

test/functional/readconcern_tests.js Outdated Show resolved Hide resolved
test/functional/readconcern_tests.js Outdated Show resolved Hide resolved
lib/operations/create_collection.js Outdated Show resolved Hide resolved
test/functional/readconcern_tests.js Outdated Show resolved Hide resolved
test/functional/readconcern_tests.js Outdated Show resolved Hide resolved
test/functional/readconcern_tests.js Outdated Show resolved Hide resolved
test/functional/readconcern_tests.js Outdated Show resolved Hide resolved
test/functional/readconcern_tests.js Outdated Show resolved Hide resolved
Copy link
Contributor

@kvwalker kvwalker left a comment

Choose a reason for hiding this comment

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

looks good to me! 🎉

@rosemaryy rosemaryy merged commit 9da0395 into mongodb:next Jun 21, 2019
@rosemaryy rosemaryy deleted the NODE-1790 branch June 21, 2019 20:39
daprahamian pushed a commit that referenced this pull request Aug 13, 2019
… new collection (#2026)

* add tests to verify correct readConcern inheritance

* create new test for createCollection

* modernized test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants