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-4069): remove 'default' from options for fullDocument field in change stream options #3169

Merged
merged 9 commits into from
Mar 16, 2022

Conversation

baileympearson
Copy link
Contributor

Description

What is changing?

This PR removes the default value of 'default' for the fullDocument field in the ChangeStreamOptions.

This PR also adds tests to options that are set in the createChangeStreamCursor method. It seemed too out of scope to write tests to fully test the constructor, but at the very least I could add tests for options that were set in the ChangeStreamCursor constructor from this method.

Is there new documentation needed for these changes?

No.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

src/change_stream.ts Outdated Show resolved Hide resolved
src/change_stream.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
test/integration/change-streams/change_stream.test.js Outdated Show resolved Hide resolved

expect(changeStream.cursor).to.haveOwnProperty('pipeline');
const pipelineOptions = changeStream.cursor.pipeline[0].$changeStream;
expect(pipelineOptions).to.haveOwnProperty('fullDocument').to.be.undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests look good to me as is, so no need to change, but I've tried to get in the habbit of using the nested feature when splunking deep into an object, at least when there are questions of nullishness

expect(changeStream).to.have.nested.property('cursor.pipeline[0].$changeStream.fullDocument', undefined);

If anything in the chain is nullish we crash without a useful assertion printout, YMMV depending, maybe this case is overkill.

Copy link
Contributor Author

@baileympearson baileympearson Mar 11, 2022

Choose a reason for hiding this comment

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

I went with this approach. The only concern I have is that when checking for undefined, any typo in the string will cause a false negative in the test

Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't be the case, I tried the following:

expect({a: {}}).to.have.nested.property('a.b', undefined); // throws
expect({a: {b: undefined}}).to.have.nested.property('a.b', undefined); // passes

so actually I think two of your tests need to be changed to say "expect to NOT have nested property" distinct from a defined property that is undefined

package.json Outdated Show resolved Hide resolved
test/integration/change-streams/change_stream.test.js Outdated Show resolved Hide resolved
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Mar 11, 2022
src/change_stream.ts Outdated Show resolved Hide resolved
src/change_stream.ts Outdated Show resolved Hide resolved
@@ -568,25 +576,37 @@ function setIsIterator<TSchema>(changeStream: ChangeStream<TSchema>): void {
}
changeStream[kMode] = 'iterator';
}

function applyKnownOptions(source: Document, options: ReadonlyArray<string>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind moving this function back down below createChangeStreamCursor, I'm happy with modernizing the loop I think leaving it in the same spot will make it clearer that it was the only change. Sorry a little pedantic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at all, done!


expect(changeStream.cursor).to.haveOwnProperty('pipeline');
const pipelineOptions = changeStream.cursor.pipeline[0].$changeStream;
expect(pipelineOptions).to.haveOwnProperty('fullDocument').to.be.undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't be the case, I tried the following:

expect({a: {}}).to.have.nested.property('a.b', undefined); // throws
expect({a: {b: undefined}}).to.have.nested.property('a.b', undefined); // passes

so actually I think two of your tests need to be changed to say "expect to NOT have nested property" distinct from a defined property that is undefined

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.

small function move, and assertion fixes

nbbeeken
nbbeeken previously approved these changes Mar 14, 2022
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.

LGTM!

@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 Mar 14, 2022
@nbbeeken nbbeeken requested review from dariakp and durran March 14, 2022 20:05
@nbbeeken nbbeeken merged commit 799689e into main Mar 16, 2022
@nbbeeken nbbeeken deleted the NODE-4069-fix-changestream-fulldocument-default branch March 16, 2022 20:01
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.

3 participants