feat(NODE-4059): ChangeStreamDocument not fully typed to specification #3191

nbbeeken · 2022-04-06T19:46:46Z

Description

What is changing?

ChangeStreamDocument had some errors and friction in type info. I've restructured it to be a descriminated union of the possible change stream events which lends itself well to TS.

This does introduce compilation changes since properties that used to be optional now do not exist depending on the change stream event type. We could consider mitigating this by bringing back the properties but typing them as something that indicates they shouldn't have been used with a particular event (for example delete has no fullDocument field)

What is the motivation for this change?

Type hinting on change stream documents helps sort through the variety of events that can occur.

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

baileympearson

Some questions and suggestions

baileympearson · 2022-04-07T17:20:43Z

src/change_stream.ts

@@ -84,16 +85,16 @@ export interface ResumeOptions {
 }

 /**
- * Represents the logical starting point for a new or resuming {@link https://docs.mongodb.com/manual/changeStreams/#std-label-change-stream-resume| Change Stream} on the server.
+ * Represents the logical starting point for a new or resuming on the server.


Suggested change

* Represents the logical starting point for a new or resuming on the server.

* Represents the logical starting point for a new ChangeStream or resuming a ChangeStream on the server.

baileympearson · 2022-04-07T17:23:12Z

src/change_stream.ts

-export interface ChangeStreamDocument<TSchema extends Document = Document> {
+export interface ChangeStreamFullNameSpace {
+  /** The namespace */
+  ns: { db: string; coll: string };


I notice further down in the rename interface, you reuse { db: string; coll: string }. Ultimately, { db: string; coll: string } is the namespace. Could we create the type as

interface ChangeStreamFullNamespace { db: string; coll: string }

so we can reuse it where needed? Then we can add ns: ChangeStreamFullNamespace to the ChangeStreamDocumentCommon object.

Changed this, we can't add it to common because it's missing from a number of the events

baileympearson · 2022-04-07T17:24:04Z

src/change_stream.ts

-   * this will contain all the components of the shard key in order,
-   * followed by the _id if the _id isn’t part of the shard key.
+   * The transaction number.
+   * Only present if the operation is part of a multi-document transaction.


Can we note here that the type of transaction number depends on the promoteLongs flag?

I'm wondering if it's more friction that it's worth to include this in the type annotation and instead document it as a possiblity based on the option.

So include this as part of the documentation for the promoteLongs flag? I'm okay with that

baileympearson · 2022-04-07T17:26:57Z

src/change_stream.ts

+ * @public
+ * @see https://www.mongodb.com/docs/manual/reference/change-events/#insert-event
+ */
+export interface InsertChangeStreamDocument<TSchema extends Document = Document>


nit: I think ChangeStream<Command>Result might make more sense than <Command>ChangeStreamDocument as a general format for these interfaces but I don't feel too strongly. Thoughts?

Renamed them all, I'm still using Document at the end because I don't want to change the name of the ChangeStreamDocument type and we should keep these consistent, but reordering the indicator makes sense.

test/tools/unified-spec-runner/entities.ts

baileympearson · 2022-04-07T17:32:28Z

test/tools/unified-spec-runner/entities.ts

-          entity.collection.collectionName,
-          patchCollectionOptions(entity.collection.collectionOptions)
-        );
+        const collection = await db


question: is this logic added to handle the scenario where a user has two entities with the same collection name in a single test suite? If so, would it make more sense to flip the logic - list collections while querying for the collection, and then create it if it doesn't exist?

Until this PR we haven't used the unified runner to run a drop db / rename collection. They both require the collection to exist in advance of the operations stage of the test. I could instead adjust the test to perform an insert to create the collection, but this seems like a reasonable adjustment to the runner.

Oh that makes sense. I was mainly wondering if the logic would be easier to follow if we flipped it around to look for collections first. But I don't feel super strongly about this, feel free to ignore this thought

let collection = await db.listCollections({ collectionName: entitiy.collection.collectionName }) .map(({ name }) => db.collection(name)) .toArray()[0] if (!collection) { collection = db.createCollection( .... ) }

baileympearson · 2022-04-07T17:39:55Z

test/integration/enumerate_databases.test.ts

@@ -233,6 +233,6 @@ const testSuite = new UnifiedTestSuiteBuilder('listDatabases with comment option
  )
  .toJSON();

-describe('listDatabases w/ comment option', () => {
+describe('listDatabases with comment option', () => {


non-blocking: Feel free to use the new toMocha method you added on enumerate_databases and enumerate_indexes if you'd like, but not necessary for the review!

PaulEndri · 2022-04-21T16:15:26Z

Hi, me again, hope it's okay. I'm actually really excited for this particular change as I've been using changestreams for a while, though looking at it, I do have a question: I think this change would break typescript validation if someone is trying to access to .fullDocument without first explicitly checking by type. Pretty sure the break is intended because this is technically more correct, but could it be done so that somehow the .watch can automatically limit the returned changeStream types? Maybe through an additional generic? I don't know if it's possible to dynamically pull based on the parameters, but I doubt it. It's just a weird developer experience if I specify to only listen to say chances and replacements and I have to additionally, in the execution logic, additionally specify it

nbbeeken · 2022-04-21T17:21:46Z

Hi @PaulEndri always appreciate the feedback, something I hadn't originally considered when starting this work was the impact that the pipeline has on the change stream document as well, since currently the generic only controls the schema there isn't a way to account for a possible change like $addFields in the types. I've put this on the PR for the back burner for the moment while we consider how we could better support that possibility. It may come down to offering a new API that can capture the potential variability of the type. If we had something that better accounted for the pipeline then something like:

client.watchFor([{ $match: { operationType: 'insert' } }])

might be able to filter the actual change stream document type returned. Any ideas are welcome additions to the conversation 🙂

nbbeeken · 2022-04-21T21:48:30Z

I'm proposing we add a second optional generic argument that can override the type of the whole change stream document. Unfortunately you can't skip arguments so the schema will need to be passed in first, but the second argument can be extended from our existing types making adding/omitting properties somewhat ergonomic.

The impact on users should still be the same as the change proposed so far where certain properties need narrowing to access. Those who want a solution that doesn't require runtime narrowing could use this second argument to strictly type their changes or pass in any removing all strictness.

type InsertChangeWithComment = ChangeStreamInsertDocument<Schema> & { comment: string };

const pipelineChangeStream = collection.watch<Schema, InsertChangeWithComment>([
  { $addFields: { comment: 'big changes' } }, 
  { $match: { operationType: 'insert' } },
]);

pipelineChangeStream.addListener('change', change => {
  expectType<string>(change.comment);
  // No need to narrow in code because the generics did that for us!
  expectType<Schema>(change.fullDocument);
});

nbbeeken · 2022-04-25T21:04:09Z

Waiting on #3215 to slim this PR down

test/integration/change-streams/change_stream.test.ts

baileympearson

LGTM

nbbeeken force-pushed the NODE-4059/changeStream-document-type branch from ae8f97c to 7aaf33d Compare April 6, 2022 21:08

baileympearson requested changes Apr 7, 2022

View reviewed changes

dariakp changed the title ~~fix(NODE-4059): ChangeStreamDocument not fully typed to specification~~ feat(NODE-4059): ChangeStreamDocument not fully typed to specification Apr 7, 2022

nbbeeken marked this pull request as ready for review April 11, 2022 20:13

nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Apr 11, 2022

nbbeeken requested a review from baileympearson April 15, 2022 15:37

nbbeeken force-pushed the NODE-4059/changeStream-document-type branch 2 times, most recently from 86bb7f2 to b3610e5 Compare April 19, 2022 19:43

nbbeeken force-pushed the NODE-4059/changeStream-document-type branch from 981f105 to 68cbcac Compare April 22, 2022 15:44

nbbeeken added the Blocked Blocked on other work label Apr 25, 2022

nbbeeken force-pushed the NODE-4059/changeStream-document-type branch from 853f968 to b022793 Compare April 27, 2022 21:44

nbbeeken removed the Blocked Blocked on other work label Apr 27, 2022

nbbeeken mentioned this pull request Apr 28, 2022

test(NODE-4059): Change Stream test typescripting and unified builder fixes #3224

Merged

5 tasks

nbbeeken force-pushed the NODE-4059/changeStream-document-type branch 2 times, most recently from c10b98e to f2bfbaf Compare April 29, 2022 20:44

feat(NODE-4059): ChangeStreamDocument not fully typed to specification

acebbfe

nbbeeken force-pushed the NODE-4059/changeStream-document-type branch from f2bfbaf to acebbfe Compare April 29, 2022 20:50

nbbeeken commented Apr 29, 2022

View reviewed changes

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

nbbeeken added 4 commits May 2, 2022 13:50

fix: custom change stream document shape assertions

43d13ef

test: create collection before use

0a0f859

test: drop then create

7b94b15

test: limit system

176d053

baileympearson previously approved these changes May 2, 2022

View reviewed changes

baileympearson added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels May 2, 2022

comment for topology reasoning

898d373

nbbeeken dismissed baileympearson’s stale review via 898d373 May 2, 2022 20:40

Reduce constraints for $replaceRoot

2e9cbf1

nbbeeken requested a review from dariakp May 3, 2022 21:21

nbbeeken added 5 commits May 4, 2022 10:47

test: any object type accepted in change stream generic

d21e36d

test: first and second arg are unrelated

4d2457b

docs: document the generic arguments to watch methods

b159c44

test: fix tests

d4de050

fix: english

e4f5130

dariakp approved these changes May 4, 2022

View reviewed changes

baileympearson approved these changes May 4, 2022

View reviewed changes

baileympearson merged commit 8b24212 into main May 4, 2022

baileympearson deleted the NODE-4059/changeStream-document-type branch May 4, 2022 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-4059): ChangeStreamDocument not fully typed to specification #3191

feat(NODE-4059): ChangeStreamDocument not fully typed to specification #3191

nbbeeken commented Apr 6, 2022

baileympearson left a comment

baileympearson Apr 7, 2022

nbbeeken Apr 15, 2022

baileympearson Apr 7, 2022

nbbeeken Apr 15, 2022

baileympearson Apr 7, 2022

nbbeeken Apr 15, 2022

baileympearson Apr 15, 2022

baileympearson Apr 7, 2022

nbbeeken Apr 15, 2022

baileympearson Apr 7, 2022

nbbeeken Apr 15, 2022

baileympearson Apr 15, 2022

baileympearson Apr 7, 2022

nbbeeken Apr 15, 2022

PaulEndri commented Apr 21, 2022

nbbeeken commented Apr 21, 2022

nbbeeken commented Apr 21, 2022

nbbeeken commented Apr 25, 2022

baileympearson left a comment

	* Represents the logical starting point for a new or resuming on the server.
	* Represents the logical starting point for a new ChangeStream or resuming a ChangeStream on the server.

feat(NODE-4059): ChangeStreamDocument not fully typed to specification #3191

feat(NODE-4059): ChangeStreamDocument not fully typed to specification #3191

Conversation

nbbeeken commented Apr 6, 2022

Description

What is changing?

What is the motivation for this change?

Double check the following

baileympearson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulEndri commented Apr 21, 2022

nbbeeken commented Apr 21, 2022

nbbeeken commented Apr 21, 2022

nbbeeken commented Apr 25, 2022

baileympearson left a comment

Choose a reason for hiding this comment