fix(NODE-4069): remove 'default' from options for fullDocument field in change stream options #3169
Conversation
|
|
||
| expect(changeStream.cursor).to.haveOwnProperty('pipeline'); | ||
| const pipelineOptions = changeStream.cursor.pipeline[0].$changeStream; | ||
| expect(pipelineOptions).to.haveOwnProperty('fullDocument').to.be.undefined; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); // passesso 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
nbbeeken
added
the
Primary Review
src/change_stream.ts
Outdated
| @@ -568,25 +576,37 @@ function setIsIterator<TSchema>(changeStream: ChangeStream<TSchema>): void { | |||
| } | |||
| changeStream[kMode] = 'iterator'; | |||
| } | |||
|
|
|||
| function applyKnownOptions(source: Document, options: ReadonlyArray<string>) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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); // passesso 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
nbbeeken
added
Team Review
Description
What is changing?
This PR removes the default value of
'default'for thefullDocumentfield in the ChangeStreamOptions.This PR also adds tests to options that are set in the
createChangeStreamCursormethod. 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
npm run check:lintscript<type>(NODE-xxxx)<!>: <description>