feat(bulk)!: add collation to FindOperators #2679
Conversation
…tion-to-bulk-find-operators
emadum
changed the title
emadum
changed the title
src/bulk/common.ts
Outdated
| /** Add a multiple update operation to the bulk operation */ | ||
| update(updateDocument: Document): BulkOperationBase { | ||
| /** @internal */ | ||
| makeUpdateDocument(u: Document, multi: boolean): Document { |
There was a problem hiding this comment.
A few nits/questions:
- is this making an
UpdateStatement? Can we reuse this internal helper I introduced last week? - at very least I don't think this should be mounted on
FindOperatorssince you're now effectively making it public API (despite the loose contract of@internal)
There was a problem hiding this comment.
Can we reuse this internal helper I introduced last week?
Nice, I refactored to use the makeUpdateStatement and makeDeleteStatement helpers. Note that there were already helpers by that name in this file, which do a very similar thing but with slightly different behavior.
It would be nice to consolidate them, but for now I've renamed them to makeBulkUpdateStatement and makeBulkDeleteStatement. There are a few TODOs in there so I want to be careful about replacing them:
// NOTE: legacy support for a raw statement, consider removing
// TODO: this check should be done at command construction against a connection, not a topology
Regarding removing the legacy support for raw statements, would you have any concerns about that? 4.0 would probably be a good time for it.
There was a problem hiding this comment.
I went ahead with that consolidation, removed the legacy support for raw statements and fixed the tests we had using the legacy format, and updated the validation checks marked with TODOs to happen against a server rather than a topology.
There was a problem hiding this comment.
Heck'n YES, I love it! I'd say make sure you take another trip through the API docs and tests to ensure that you're properly documenting the shape of what's accepted in bulk. Also, now that the "raw" form of the operations are no longer accepted I think you will want to add some validation to let users know that things have changed. Right now I think you'll be silently breaking their applications potentially, so good to add some tests that prove their apps will not break. Otherwise, I think this is a huge improvement for bulk, great job.
| if (hasAtomicOperators(replacement)) { | ||
| throw new TypeError('Replacement document must not use atomic operators'); | ||
| } | ||
| removeOne(): BulkOperationBase { |
There was a problem hiding this comment.
remind me, is there a ticket to remove these duplicated methods for v4?
There was a problem hiding this comment.
I wasn't able to find one, it's not in the main ticket tracking deprecation (NODE-2317). Do you think we should get rid of them?
There was a problem hiding this comment.
Yeah, I think now's the time to do it!
There was a problem hiding this comment.
I added NODE-2978 so as to not scope creep more on this PR 👍
emadum
changed the title
emadum
force-pushed
the
NODE-2757/add-collation-to-bulk-find-operators
branch
from
55df13e
to
e0e6869
Compare
emadum
force-pushed
the
NODE-2757/add-collation-to-bulk-find-operators
branch
from
e0e6869
to
85b8911
Compare
| if (hasAtomicOperators(replacement)) { | ||
| throw new TypeError('Replacement document must not use atomic operators'); | ||
| } | ||
| removeOne(): BulkOperationBase { |
There was a problem hiding this comment.
Yeah, I think now's the time to do it!
|
|
||
| remove(): BulkOperationBase { | ||
| return this.delete(); | ||
| this.bulkOperation.s.currentOp.collation = collation; |
There was a problem hiding this comment.
I don't think this should expand the scope of this PR, but I just wanted to comment on something so you can keep it in mind. We have "builders" for a number of things in the driver, specifically for bulk and cursors. I think it would be really great for readability/maintainability if you were doing them all the same way. The bulk builders need a lot of work, and are confusingly coupled, but I think you could use a [kBuiltOptions] approach here (or better yet, wrap [kBuiltOptions] in some well-typed methods like addToCurrentOperation that you can reuse as a toolkit across all "builders" in the codebase).
…tion-to-bulk-find-operators
Adds a fluent builder method for the
collationof filtered bulk writes.Removes legacy support for raw statements from
collection.bulkWrite.NODE-2757