-
Notifications
You must be signed in to change notification settings - Fork 105
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(checkpoint-mongodb): fix filter support, store state deltas, populate pendingWrites #625
base: main
Are you sure you want to change the base?
fix(checkpoint-mongodb): fix filter support, store state deltas, populate pendingWrites #625
Conversation
2bdf1fc
to
541ece1
Compare
8642d79
to
874977b
Compare
@@ -36,3 +36,26 @@ export interface CheckpointMetadata { | |||
*/ | |||
parents: Record<string, string>; | |||
} | |||
|
|||
const checkpointMetadataKeys = ["source", "step", "writes", "parents"] as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might accept arbitrary metadata keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just taking cues from PostgresSaver
on this. Is there some mechanism for user code to modify the checkpoint metadata? It doesn't have an index signature, so I (perhaps incorrectly) assumed it was a complete type that originated internally. The typing cues are contradictory though, as options.filter
is Record<string, unknown>
. A type of Partial<CheckpointMetadata>
would've been a clearer expression of intent there if it was indeed meant to be restricted.
Regardless, I added it in the RDBMS implementations because the list of filter keys is being directly concatenated into the query text (SQL injection potential), and as a result I also added it here for consistency.
Assuming arbitrary filter keys aren't desired, perhaps it would be better to throw on unknown keys. Silently dropping them could be confusing behavior, otherwise.
If they are desired, we should think carefully about how to sanitize the inputs for the RDBMS implementations.
The ESLint failure is a false positive and it should be fixed by #633. It seems transient, and sometimes occurs in |
874977b
to
1889c31
Compare
1889c31
to
aae0e51
Compare
Just rebased on current |
Breaking changes:
MongoDBSaver.migrate
method.Non-breaking changes:
list
method to support theoptions.filter
argument, fixesMongoDBSaver.*list
yields nothing whenoptions.filter
is defined, and fixing it requires a breaking change #581checkpoint-sqlite
andcheckpoint
libraries to centralize an array that I'm using in multiple packages to sanitize filter inputs.channel_values
by version on change, respecting thenewVersions
argument toput
, fixesMongoDbSaver
ignoresnewVersions
argument onput
- not storing deltas #595pendingWrites
andpending_sends
correctly, fixesMongoDBSaver.list
returnsCheckpointTuples
without thependingWrites
field populated. #589checkpoint-validation
to no longer skip tests that exercise the above fixes