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(checkpoint-mongodb): fix filter support, store state deltas, populate pendingWrites #625

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

benjamincburns
Copy link
Contributor

@benjamincburns benjamincburns commented Oct 23, 2024

Breaking changes:

  • Changes the format of stored documents in mongo, requiring existing databases to be migrated by calling the MongoDBSaver.migrate method.

Non-breaking changes:

@benjamincburns benjamincburns changed the title fix(checkpoint-mongodb): apply filters correctly in list method fix(checkpoint-mongodb): fix filter support, store state deltas, populate pending_sends Oct 24, 2024
@benjamincburns benjamincburns changed the title fix(checkpoint-mongodb): fix filter support, store state deltas, populate pending_sends fix(checkpoint-mongodb): fixes for #581, #589, #595 Oct 24, 2024
@benjamincburns benjamincburns changed the title fix(checkpoint-mongodb): fixes for #581, #589, #595 fix(checkpoint-mongodb): fix filter support, store state deltas, populate pendingWrites Oct 24, 2024
@@ -36,3 +36,26 @@ export interface CheckpointMetadata {
*/
parents: Record<string, string>;
}

const checkpointMetadataKeys = ["source", "step", "writes", "parents"] as const;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@benjamincburns
Copy link
Contributor Author

benjamincburns commented Oct 28, 2024

The ESLint failure is a false positive and it should be fixed by #633. It seems transient, and sometimes occurs in main.

@benjamincburns
Copy link
Contributor Author

The ESLint failure is a false positive and it should be fixed by #633. It seems transient, and sometimes occurs in main.

Just rebased on current main, which should remove the ESLint violation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants