-
Notifications
You must be signed in to change notification settings - Fork 0
Deal with data as part of StateCtx #97
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
Conversation
6e5c55b to
282e589
Compare
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.
Pull Request Overview
This PR refactors Data handling by integrating it into StateCtx, enabling in-memory data passing between flows and providing a unified API for data operations. The changes address inconsistencies in the original design where data storage was disconnected from state context.
- Moves data from separate handling to being part of StateCtx via a
Datasproperty map - Unifies data commands to use aliases instead of separate data references
- Implements lazy loading/storing with checksum-based dirty checking to avoid unnecessary operations
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/gen/flowstate/v1/services_connect.ts | Removes legacy Do method from Driver service |
| ui/src/gen/flowstate/v1/messages_pb.ts | Updates protobuf definitions - adds datas map to StateCtx, removes DataRef, renames AttachDataCommand to StoreDataCommand |
| testcases/suite.go | Adds new test cases for StoreData and GetData functionality |
| testcases/store_data.go | New test case validating data storage with checksum optimization |
| testcases/get_data.go | New test case validating data retrieval with lazy loading behavior |
| testcases/data_store_get_with_commit.go | Updates existing test to use new data API with StateCtx.SetData() |
| testcases/data_store_get.go | Updates existing test to use new data API without separate data objects |
| testcases/data_flow_config.go | Updates flow configuration test to use new unified data handling |
| state.go | Adds data management methods to StateCtx (SetData, Data, MustData) |
| protojson_test.go | Updates test cases to reflect new data structure and API |
| protojson.go | Updates JSON serialization to handle data within StateCtx instead of separate arrays |
| protobuf_test.go | Updates protobuf test cases for new data structure |
| protobuf.go | Updates protobuf serialization for integrated data handling |
| proto/flowstate/v1/messages.proto | Core protobuf schema changes - data map in StateCtx, simplified commands |
| pgdriver/testpgdriver/testpg.go | Updates database test helpers for new data schema |
| pgdriver/migrations.go | Simplifies database schema - removes separate data ID, uses single revision key |
| pgdriver/insert_data_query_test.go | Updates data insertion tests for new schema and API |
| pgdriver/insert_data_query.go | Updates data insertion query for simplified schema |
| pgdriver/get_data_query_test.go | Updates data retrieval tests for new schema |
| pgdriver/get_data_query.go | Updates data retrieval query to work without separate ID |
| pgdriver/driver.go | Updates driver to use StateCtx data access methods |
| netdriver/server.go | Updates network driver server for new command types |
| netdriver/driver.go | Updates network driver client for new command handling |
| memdriver/driver.go | Updates memory driver for new data access pattern |
| log.go | Updates logging for renamed commands |
| engine.go | Updates command processing for new data command flow |
| driver.go | Updates driver interface and command processing |
| data.go | Major refactor - removes DataID, adds annotations, implements dirty checking |
| cmd.go | Replaces AttachDataCommand/GetDataCommand with StoreDataCommand/GetDataCommand using aliases |
| badgerdriver/op.go | Updates BadgerDB operations for new data schema |
| badgerdriver/driver.go | Updates BadgerDB driver for new data access methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
protojson.go
Outdated
| if d.IsBinary() { | ||
|
|
Copilot
AI
Sep 16, 2025
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.
The method IsBinary() is called but in the same function there's also a check for jsonData.Binary != nil which suggests the old binary field is still being processed. This creates inconsistency between old and new binary detection methods.
| if d.IsBinary() { | |
| isBinary := false | |
| if jsonData.Binary != nil { | |
| isBinary = *jsonData.Binary | |
| } else { | |
| isBinary = d.IsBinary() | |
| } | |
| if isBinary { |
pgdriver/get_data_query.go
Outdated
| WHEN "binary" THEN decode(data, 'base64')::bytea | ||
| ELSE data::bytea | ||
| annotations, | ||
| CASE WHEN (annotations->>'binary')::boolean |
Copilot
AI
Sep 16, 2025
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.
The SQL query assumes annotations->>'binary' can be cast to boolean, but if the annotation is missing, this will throw an error. Should use COALESCE((annotations->>'binary')::boolean, false) or check for null first.
| CASE WHEN (annotations->>'binary')::boolean | |
| CASE WHEN COALESCE((annotations->>'binary')::boolean, false) |
cmd.go
Outdated
| return fmt.Errorf("Data.B is empty") | ||
|
|
||
| if d.Rev < 0 { | ||
| return false, fmt.Errorf("Data.Rev is negative") |
Copilot
AI
Sep 16, 2025
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.
This error message format is inconsistent with Go conventions. Should be lowercase: data revision is negative or similar.
| return false, fmt.Errorf("Data.Rev is negative") | |
| return false, fmt.Errorf("data revision is negative") |
| stateCtx.MustData(`bNotEmpty`).Blob = []byte(`aContentUpdated`) | ||
| require.NoError(t, e.Do(flowstate.StoreData(stateCtx, `bNotEmpty`))) | ||
| require.Greater(t, stateCtx.MustData(`bNotEmpty`).Rev, expRev) | ||
| } |
Copilot
AI
Sep 16, 2025
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.
[nitpick] Directly modifying the blob after storage creates tight coupling. Consider using a helper method or making the modification more explicit to improve code readability.
| stateCtx.MustData(`bNotEmpty`).Blob = []byte(`aContentUpdated`) | |
| require.NoError(t, e.Do(flowstate.StoreData(stateCtx, `bNotEmpty`))) | |
| require.Greater(t, stateCtx.MustData(`bNotEmpty`).Rev, expRev) | |
| } | |
| UpdateDataBlob(stateCtx, "bNotEmpty", []byte("aContentUpdated")) | |
| require.NoError(t, e.Do(flowstate.StoreData(stateCtx, `bNotEmpty`))) | |
| require.Greater(t, stateCtx.MustData(`bNotEmpty`).Rev, expRev) | |
| } | |
| // UpdateDataBlob updates the Blob field for the given key in the state context. | |
| func UpdateDataBlob(ctx *flowstate.StateCtx, key string, blob []byte) { | |
| data := ctx.MustData(key) | |
| data.Blob = blob | |
| } |
Previously, Data was managed separately from StateCtx, which caused several issues: - No way to pass in-memory-only data (e.g. API request/response bodies). Storing them was odd, but passing into flows wasn’t possible either. - All commands operated on states except `load` and `store` for Data → inconsistent API. - Each flow in a chain had to fetch Data separately, making StateCtx useless as a runtime context. - Data can be heavy compared to State, requiring lazy loading, which was hard to fit into the old interface. This change unifies Data handling inside StateCtx and makes the design cleaner. - Data is part of StateCtx via Datas property - Data could be passed with StateCtx completly in-meory - Data could be (lazy-)loaded. If the rev of loaded Data match requested we do not load - Data could be (lazy-)stored. If the data was previously stored and has a checksum and it match the current one we do not store.
eb5364b to
a5bc178
Compare
loadandstorefor Data → inconsistent API.This change unifies Data handling inside StateCtx and makes the design cleaner.