Refactor flow/mgt package to remove int based FK reference#1584
Open
KaveeshaPiumini wants to merge 1 commit intoasgardeo:mainfrom
Open
Refactor flow/mgt package to remove int based FK reference#1584KaveeshaPiumini wants to merge 1 commit intoasgardeo:mainfrom
KaveeshaPiumini wants to merge 1 commit intoasgardeo:mainfrom
Conversation
KaveeshaPiumini
requested review from
ThaminduDilshan and
darshanasbg
as code owners
KaveeshaPiumini
added
Type/Improvement
breaking change
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors FLOW and FLOW_VERSION to remove surrogate integer IDs and make FLOW_ID the primary key; updates schema, foreign keys, store queries, Go store implementation, and tests to operate on FLOW_ID and (FLOW_ID, DEPLOYMENT_ID) composite relations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant FlowSvc as FlowService
participant DB as Database
rect rgba(100,149,237,0.5)
Client->>FlowSvc: Create/Update Flow (FLOW_ID, meta)
FlowSvc->>DB: INSERT/UPDATE FLOW (FLOW_ID, DEPLOYMENT_ID, ...)
FlowSvc->>DB: INSERT FLOW_VERSION (FLOW_ID, VERSION, NODES, DEPLOYMENT_ID)
DB-->>FlowSvc: OK / Constraint error
FlowSvc-->>Client: Success / Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/internal/flow/mgt/store_test.go (1)
156-162: Use canonical flow-type constants in type-filter tests.Line 156 and Line 161 hardcode
"authentication". Preferstring(common.FlowTypeAuthentication)to keep tests aligned with canonical enum values and avoid case drift.♻️ Suggested test tweak
+ flowType := string(common.FlowTypeAuthentication) s.mockDBProvider.EXPECT().GetConfigDBClient().Return(s.mockDBClient, nil) - s.mockDBClient.EXPECT().Query(queryCountFlowsWithType, "authentication", "test-deployment"). + s.mockDBClient.EXPECT().Query(queryCountFlowsWithType, flowType, "test-deployment"). Return([]map[string]interface{}{{colCount: int64(1)}}, nil).Once() - s.mockDBClient.EXPECT().Query(queryListFlowsWithType, "authentication", "test-deployment", 10, 0). + s.mockDBClient.EXPECT().Query(queryListFlowsWithType, flowType, "test-deployment", 10, 0). Return(flowsData, nil).Once() - flows, count, err := s.store.ListFlows(10, 0, "authentication") + flows, count, err := s.store.ListFlows(10, 0, flowType)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/flow/mgt/store_test.go` around lines 156 - 162, Replace the hardcoded "authentication" flow-type string in the test with the canonical enum value so the test uses the real constant; update the two places where s.mockDBClient.EXPECT().Query is called (the calls using queryCountFlowsWithType and queryListFlowsWithType) and the ListFlows invocation to pass string(common.FlowTypeAuthentication) instead of "authentication", and add/ensure the test imports the package that defines FlowTypeAuthentication so the symbol resolves; update only the test assertions/expectations to use this canonical value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/dbscripts/thunderdb/postgres.sql`:
- Around line 309-317: Change the table to use a composite primary key of
(FLOW_ID, DEPLOYMENT_ID) instead of making FLOW_ID the standalone primary key:
remove the "PRIMARY KEY" annotation from FLOW_ID, remove the separate "UNIQUE
(FLOW_ID, DEPLOYMENT_ID)" constraint, and add a single "PRIMARY KEY (FLOW_ID,
DEPLOYMENT_ID)" definition; keep the other unique constraint "UNIQUE (HANDLE,
FLOW_TYPE, DEPLOYMENT_ID)" as-is and ensure DEPLOYMENT_ID column is present and
NOT NULL to support the new composite primary key.
In `@backend/dbscripts/thunderdb/sqlite.sql`:
- Around line 306-314: The FLOW table currently declares FLOW_ID as the
standalone PRIMARY KEY which enforces global uniqueness; change the primary key
to the composite (FLOW_ID, DEPLOYMENT_ID) to scope rows per deployment, remove
the redundant UNIQUE (FLOW_ID, DEPLOYMENT_ID) constraint, and keep or adjust
other UNIQUE constraints (e.g., UNIQUE (HANDLE, FLOW_TYPE, DEPLOYMENT_ID)) so
they still include DEPLOYMENT_ID; update any code or FK definitions that assume
a single-column primary key (references to FLOW_ID) to use the composite key
where appropriate.
---
Nitpick comments:
In `@backend/internal/flow/mgt/store_test.go`:
- Around line 156-162: Replace the hardcoded "authentication" flow-type string
in the test with the canonical enum value so the test uses the real constant;
update the two places where s.mockDBClient.EXPECT().Query is called (the calls
using queryCountFlowsWithType and queryListFlowsWithType) and the ListFlows
invocation to pass string(common.FlowTypeAuthentication) instead of
"authentication", and add/ensure the test imports the package that defines
FlowTypeAuthentication so the symbol resolves; update only the test
assertions/expectations to use this canonical value.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/dbscripts/thunderdb/postgres.sqlbackend/dbscripts/thunderdb/sqlite.sqlbackend/internal/flow/mgt/store.gobackend/internal/flow/mgt/store_constants.gobackend/internal/flow/mgt/store_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1584 +/- ##
==========================================
+ Coverage 91.35% 91.41% +0.05%
==========================================
Files 692 692
Lines 46194 46155 -39
==========================================
- Hits 42202 42194 -8
+ Misses 2673 2659 -14
+ Partials 1319 1302 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
KaveeshaPiumini
force-pushed
the
refactor
branch
from
7cb9f0e to
fdbc343
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/flow/mgt/store_test.go (1)
716-785:⚠️ Potential issue | 🟠 MajorAdd success path tests for transactional methods to meet coverage requirements.
The error path tests are comprehensive, but success paths for
CreateFlow(57.1%),UpdateFlow(22.7%), andRestoreFlowVersion(11.1%) must be added to meet the 80% coverage guideline for each method. The current overall package coverage is 86.0%, but these critical transactional methods require success path testing to reach the required threshold.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/flow/mgt/store_test.go` around lines 716 - 785, Add unit tests covering the success paths for CreateFlow, UpdateFlow, and RestoreFlowVersion: for each test obtain a mock transaction from GetConfigDBClient().BeginTx(), set Exec expectations on the mockTx for the relevant queries (e.g., the insert used in CreateFlow, the update used in UpdateFlow, and the restore/version-update query used in RestoreFlowVersion) to return expected results (no error), expect mockTx.Commit() to be called and no Rollback, call the store method (CreateFlow/UpdateFlow/RestoreFlowVersion) and assert no error and that the returned result matches the created/updated/restored flow/version; mirror existing error-path tests’ structure (use mockTx from modelmock.NewTxInterfaceMock, s.mockDBProvider.EXPECT(), s.mockDBClient.EXPECT().BeginTx().Return(mockTx, nil), then Exec expectations and Commit().Return(nil)) so coverage for those transactional methods reaches the success-case paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/internal/flow/mgt/store_test.go`:
- Around line 716-785: Add unit tests covering the success paths for CreateFlow,
UpdateFlow, and RestoreFlowVersion: for each test obtain a mock transaction from
GetConfigDBClient().BeginTx(), set Exec expectations on the mockTx for the
relevant queries (e.g., the insert used in CreateFlow, the update used in
UpdateFlow, and the restore/version-update query used in RestoreFlowVersion) to
return expected results (no error), expect mockTx.Commit() to be called and no
Rollback, call the store method (CreateFlow/UpdateFlow/RestoreFlowVersion) and
assert no error and that the returned result matches the
created/updated/restored flow/version; mirror existing error-path tests’
structure (use mockTx from modelmock.NewTxInterfaceMock,
s.mockDBProvider.EXPECT(), s.mockDBClient.EXPECT().BeginTx().Return(mockTx,
nil), then Exec expectations and Commit().Return(nil)) so coverage for those
transactional methods reaches the success-case paths.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/dbscripts/thunderdb/postgres.sqlbackend/dbscripts/thunderdb/sqlite.sqlbackend/internal/flow/mgt/store.gobackend/internal/flow/mgt/store_constants.gobackend/internal/flow/mgt/store_test.go
KaveeshaPiumini
force-pushed
the
refactor
branch
2 times, most recently
from
746df2f to
1e6ebbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/flow/mgt/store_test.go`:
- Around line 156-162: Replace the hard-coded lowercase type string
"authentication" in the test expectations and the call to s.store.ListFlows with
the canonical FlowType constant (use string(common.FlowTypeAuthentication)) so
the query expectations on mockDBClient (queryCountFlowsWithType and
queryListFlowsWithType) and the ListFlows invocation use the canonical value;
update the two places where "authentication" is passed to use
string(common.FlowTypeAuthentication) to avoid case-sensitivity regressions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/dbscripts/thunderdb/postgres.sqlbackend/dbscripts/thunderdb/sqlite.sqlbackend/internal/flow/mgt/store.gobackend/internal/flow/mgt/store_constants.gobackend/internal/flow/mgt/store_test.go
KaveeshaPiumini
force-pushed
the
refactor
branch
4 times, most recently
from
3efa7fe to
437bc2e
Compare
KaveeshaPiumini
force-pushed
the
refactor
branch
from
437bc2e to
81d3750
Compare
rajithacharith
approved these changes
Feb 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This pull request refactors the flow storage schema and logic to simplify how flows and their versions are managed. The main change is the removal of the internal numeric ID (
ID/FLOW_INTERNAL_ID) in favor of using the externalFLOW_ID(UUID) as the primary and foreign key throughout the schema and code. This streamlines queries, reduces indirection, and simplifies both the database structure and the application logic. The test suite is also updated to cover the new logic and ensure correctness.Database schema and query refactoring:
IDandFLOW_INTERNAL_IDcolumns from theFLOWandFLOW_VERSIONtables in both Postgres and SQLite schemas, makingFLOW_IDthe primary key and reference for all operations. [1] [2] [3] [4]FLOW_IDinstead of the internal ID, including joins, inserts, deletes, and version management. [1] [2] [3] [4]Application logic simplification:
store.go) to remove all logic related to fetching or using internal IDs, updating all data access to useFLOW_IDdirectly. This includes creation, updating, listing, restoring, and deleting flow versions. [1] [2] [3] [4] [5] [6] [7]Test coverage improvements:
store_test.goto validate the new logic, including successful flow and version listing, retrieval, and type filtering. These tests ensure the refactored storage logic works as intended with the new schema. [1] [2] [3] [4]Overall, this PR makes the flow storage more robust, easier to maintain, and less error-prone by eliminating unnecessary indirection and aligning the schema and code with the external flow identifier.
🔧 Summary of Breaking Changes
The resource package has been refactored to remove all INT-based foreign key references and standardize relationships using UUID-based identifiers.
Previously, some tables referenced related entities using internal auto-incremented INT IDs. With this change, all such references have been updated to use the UUID-based identifiers of the related entities. As a result, INT-based FK columns and their associated queries have been removed or replaced.
💥 Impact
Direct database integrations, custom scripts, or downstream services that depend on the previous schema will require modification.
🔄 Migration Guide
Update database scripts to remove references to ID columns.
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit