- 
                Notifications
    
You must be signed in to change notification settings  - Fork 338
 
fix(checkpoint-mongodb): fix filter support, store state deltas, populate pendingWrites #625
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
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
  
    | 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.migratemethod.Non-breaking changes:
listmethod to support theoptions.filterargument, fixesMongoDBSaver.*listyields nothing whenoptions.filteris defined, and fixing it requires a breaking change #581checkpoint-sqliteandcheckpointlibraries to centralize an array that I'm using in multiple packages to sanitize filter inputs.channel_valuesby version on change, respecting thenewVersionsargument toput, fixesMongoDbSaverignoresnewVersionsargument onput- not storing deltas #595pendingWritesandpending_sendscorrectly, fixesMongoDBSaver.listreturnsCheckpointTupleswithout thependingWritesfield populated. #589checkpoint-validationto no longer skip tests that exercise the above fixes