Add composite store support for applications#1396
Open
rajithacharith wants to merge 1 commit intoasgardeo:mainfrom
Open
Add composite store support for applications#1396rajithacharith wants to merge 1 commit intoasgardeo:mainfrom
rajithacharith wants to merge 1 commit intoasgardeo:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds multi-mode application storage (mutable DB, declarative file, composite), three new store interface methods for existence/declarative checks, implementations across stores, composite store merging logic, config-driven store selection, service guards for read-only apps, new errors, and updated mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant CompositeStore
participant DBStore
participant FileStore
rect rgba(200, 150, 255, 0.5)
Note over Client,CompositeStore: GetApplicationList (composite mode)
end
Client->>Service: GetApplicationList()
Service->>CompositeStore: GetApplicationList()
CompositeStore->>DBStore: GetApplicationList()
DBStore-->>CompositeStore: dbApps
CompositeStore->>FileStore: GetApplicationList()
FileStore-->>CompositeStore: fileApps
CompositeStore->>CompositeStore: mergeAndDeduplicateApplications(dbApps, fileApps)
CompositeStore->>CompositeStore: enforceResultLimit(merged)
alt Limit exceeded
CompositeStore-->>Service: ErrResultLimitExceededInCompositeMode
Service-->>Client: ErrorResultLimitExceeded
else Within limit
CompositeStore-->>Service: merged []BasicApplicationDTO (IsReadOnly set)
Service-->>Client: response
end
sequenceDiagram
participant Client
participant Service
participant CompositeStore
participant DBStore
participant FileStore
rect rgba(200, 150, 255, 0.5)
Note over Client,CompositeStore: UpdateApplication (composite mode)
end
Client->>Service: UpdateApplication(app)
Service->>CompositeStore: GetApplicationByID(app.ID)
CompositeStore->>DBStore: GetApplicationByID(id)
alt Found in DB
DBStore-->>CompositeStore: app (IsReadOnly=false)
else Not in DB
DBStore-->>CompositeStore: ApplicationNotFoundError
CompositeStore->>FileStore: GetApplicationByID(id)
FileStore-->>CompositeStore: app (IsReadOnly=true)
end
CompositeStore-->>Service: appDTO
alt app.IsReadOnly == true
Service-->>Client: ErrorCannotModifyDeclarativeResource
else
Service->>CompositeStore: UpdateApplication(updated)
CompositeStore->>DBStore: UpdateApplication(updated)
DBStore-->>CompositeStore: success/error
CompositeStore-->>Service: result
Service-->>Client: UpdateApplicationResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 7
🤖 Fix all issues with AI agents
In `@backend/internal/application/composite_store.go`:
- Around line 27-42: Move the composite store implementation from
composite_store.go into the module's store.go to follow repo conventions:
relocate the compositeApplicationStore type and its constructor
newCompositeApplicationStore (including fileStore and dbStore fields) into
store.go alongside the applicationStoreInterface and other store
implementations; ensure imports and package declarations are adjusted, and
remove the redundant composite_store.go file or export no duplicate symbols so
only the single implementation in store.go remains.
- Around line 52-67: The GetApplicationList in compositeApplicationStore passes
a hardcoded limit of 100 to
declarativeresource.CompositeMergeListHelperWithLimit which can truncate results
for a non-paginated API; change that 100 to use the composite cap (e.g.
serverconst.MaxCompositeStoreRecords or MaxInt) so the helper can return all
records up to the enforced max, keeping mergeAndDeduplicateApplications and the
existing maxRecords parameter unchanged.
In `@backend/internal/application/declarative_resource.go`:
- Around line 109-115: The call to newApplicationService in
loadDeclarativeResources uses 5 arguments but the function requires 6 (causing a
compile error); update this invocation in loadDeclarativeResources to pass the
same sixth parameter used at the other call site (or add a nil placeholder if
the service doesn’t need a real value in this context) so the argument count
matches newApplicationService’s signature.
- Around line 224-236: The code currently swallows errors from
fileStore.IsApplicationExists and dbStore.IsApplicationExists; update the checks
in the declarative validation (around the IsApplicationExists calls) to return
the underlying error instead of ignoring it—i.e., if err != nil return
fmt.Errorf("failed to check application existence: %w", err) (or similar) for
both fileStore.IsApplicationExists(app.ID) and
dbStore.IsApplicationExists(app.ID), while still returning the duplicate-ID
fmt.Errorf when exists == true; ensure you wrap the original error to preserve
context.
In `@backend/internal/application/error_constants.go`:
- Around line 236-241: The ErrorCannotModifyDeclarativeResource constant
currently reuses the code "APP-1028" already assigned to
ErrorWhileRetrievingFlowDefinition; update the ServiceError.Code value for
ErrorCannotModifyDeclarativeResource to a unique code (for example "APP-1029")
by editing the Code field in the serviceerror.ServiceError literal for
ErrorCannotModifyDeclarativeResource so it no longer conflicts with
ErrorWhileRetrievingFlowDefinition.
- Around line 28-32: Rename the exported sentinel error
ErrResultLimitExceededInCompositeMode to a package-private name (e.g.,
errResultLimitExceededInCompositeMode) and update all references to this symbol
throughout the package to use the new lowercase identifier; ensure the variable
still uses errors.New("result limit exceeded in composite mode") and that any
callers within the package import no changes beyond the identifier rename so the
error remains internal to the package.
In `@backend/internal/application/service.go`:
- Around line 89-92: Update the Update and Delete paths to use the same
application-specific declarative-mode resolution as Create: replace calls to
declarativeresource.CheckDeclarativeUpdate() and
declarativeresource.CheckDeclarativeDelete() with the per-application check used
by Create (isDeclarativeModeEnabled() or an equivalent helper that takes the
Application/Store context), so that Update and Delete honor the resource's Store
setting (mutable/composite) rather than only the global
DeclarativeResources.Enabled flag; ensure you preserve the existing error
returned (&ErrorCannotModifyDeclarativeResource) when the per-app check denies
the operation.
🧹 Nitpick comments (1)
backend/internal/application/init.go (1)
47-49: Consider caching the DB store in composite mode for parity with mutable mode.Mutable mode wraps the DB store with the cached variant, while composite mode uses the raw DB store. If caching is still desired, wrap the DB store before composing to avoid a performance regression.
♻️ Suggested adjustment
case serverconst.StoreModeComposite: fileStore := newFileBasedStore() - dbStore := newApplicationStore() + dbStore := newCachedBackedApplicationStore(newApplicationStore()) appStore = newCompositeApplicationStore(fileStore, dbStore)Also applies to: 68-122
Comment on lines
+27
to
+42
| // compositeApplicationStore implements a composite store that combines file-based (immutable) and database (mutable) stores. | ||
| // - Read operations query both stores and merge results | ||
| // - Write operations (Create/Update/Delete) only affect the database store | ||
| // - Declarative applications (from YAML files) cannot be modified or deleted | ||
| type compositeApplicationStore struct { | ||
| fileStore applicationStoreInterface | ||
| dbStore applicationStoreInterface | ||
| } | ||
|
|
||
| // newCompositeApplicationStore creates a new composite store with both file-based and database stores. | ||
| func newCompositeApplicationStore(fileStore, dbStore applicationStoreInterface) *compositeApplicationStore { | ||
| return &compositeApplicationStore{ | ||
| fileStore: fileStore, | ||
| dbStore: dbStore, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move composite store implementation into store.go to match repo conventions.
This introduces a store implementation in composite_store.go, but the repo guideline places store interfaces/implementations in store.go. Please relocate the composite store (or consolidate into store.go) to stay consistent.
As per coding guidelines backend/internal/**/store.go: Define store interfaces (e.g., xStoreInterface) and implementations (e.g., xStore struct) in store.go.
🤖 Prompt for AI Agents
In `@backend/internal/application/composite_store.go` around lines 27 - 42, Move
the composite store implementation from composite_store.go into the module's
store.go to follow repo conventions: relocate the compositeApplicationStore type
and its constructor newCompositeApplicationStore (including fileStore and
dbStore fields) into store.go alongside the applicationStoreInterface and other
store implementations; ensure imports and package declarations are adjusted, and
remove the redundant composite_store.go file or export no duplicate symbols so
only the single implementation in store.go remains.
Comment on lines
+52
to
+67
| // GetApplicationList retrieves applications from both stores. | ||
| // Note: Application list does not support pagination at the API level, so we don't apply pagination here. | ||
| // However, we still apply the 1000-record limit in composite mode to prevent memory exhaustion. | ||
| func (c *compositeApplicationStore) GetApplicationList() ([]model.BasicApplicationDTO, error) { | ||
| // Use the helper to fetch, merge, and check limits | ||
| // Since application list doesn't support pagination, we use limit=MaxInt and offset=0 | ||
| apps, limitExceeded, err := declarativeresource.CompositeMergeListHelperWithLimit( | ||
| func() (int, error) { return c.dbStore.GetTotalApplicationCount() }, | ||
| func() (int, error) { return c.fileStore.GetTotalApplicationCount() }, | ||
| func(limit int) ([]model.BasicApplicationDTO, error) { return c.dbStore.GetApplicationList() }, | ||
| func(limit int) ([]model.BasicApplicationDTO, error) { return c.fileStore.GetApplicationList() }, | ||
| mergeAndDeduplicateApplications, | ||
| 100, // Setting limit to 100 as pagination is not supported at API level. | ||
| 0, // offset 0 - start from beginning | ||
| serverconst.MaxCompositeStoreRecords, | ||
| ) |
There was a problem hiding this comment.
List helper limit set to 100 can truncate results in a non-paginated API.
The API doesn’t paginate, but passing limit=100 risks returning only 100 items even when the total is below the composite cap. Use the composite cap (or MaxInt) as the limit and rely on maxRecords to protect memory.
🐛 Proposed fix
- 100, // Setting limit to 100 as pagination is not supported at API level.
+ serverconst.MaxCompositeStoreRecords, // return full list up to composite cap📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // GetApplicationList retrieves applications from both stores. | |
| // Note: Application list does not support pagination at the API level, so we don't apply pagination here. | |
| // However, we still apply the 1000-record limit in composite mode to prevent memory exhaustion. | |
| func (c *compositeApplicationStore) GetApplicationList() ([]model.BasicApplicationDTO, error) { | |
| // Use the helper to fetch, merge, and check limits | |
| // Since application list doesn't support pagination, we use limit=MaxInt and offset=0 | |
| apps, limitExceeded, err := declarativeresource.CompositeMergeListHelperWithLimit( | |
| func() (int, error) { return c.dbStore.GetTotalApplicationCount() }, | |
| func() (int, error) { return c.fileStore.GetTotalApplicationCount() }, | |
| func(limit int) ([]model.BasicApplicationDTO, error) { return c.dbStore.GetApplicationList() }, | |
| func(limit int) ([]model.BasicApplicationDTO, error) { return c.fileStore.GetApplicationList() }, | |
| mergeAndDeduplicateApplications, | |
| 100, // Setting limit to 100 as pagination is not supported at API level. | |
| 0, // offset 0 - start from beginning | |
| serverconst.MaxCompositeStoreRecords, | |
| ) | |
| // GetApplicationList retrieves applications from both stores. | |
| // Note: Application list does not support pagination at the API level, so we don't apply pagination here. | |
| // However, we still apply the 1000-record limit in composite mode to prevent memory exhaustion. | |
| func (c *compositeApplicationStore) GetApplicationList() ([]model.BasicApplicationDTO, error) { | |
| // Use the helper to fetch, merge, and check limits | |
| // Since application list doesn't support pagination, we use limit=MaxInt and offset=0 | |
| apps, limitExceeded, err := declarativeresource.CompositeMergeListHelperWithLimit( | |
| func() (int, error) { return c.dbStore.GetTotalApplicationCount() }, | |
| func() (int, error) { return c.fileStore.GetTotalApplicationCount() }, | |
| func(limit int) ([]model.BasicApplicationDTO, error) { return c.dbStore.GetApplicationList() }, | |
| func(limit int) ([]model.BasicApplicationDTO, error) { return c.fileStore.GetApplicationList() }, | |
| mergeAndDeduplicateApplications, | |
| serverconst.MaxCompositeStoreRecords, // return full list up to composite cap | |
| 0, // offset 0 - start from beginning | |
| serverconst.MaxCompositeStoreRecords, | |
| ) |
🤖 Prompt for AI Agents
In `@backend/internal/application/composite_store.go` around lines 52 - 67, The
GetApplicationList in compositeApplicationStore passes a hardcoded limit of 100
to declarativeresource.CompositeMergeListHelperWithLimit which can truncate
results for a non-paginated API; change that 100 to use the composite cap (e.g.
serverconst.MaxCompositeStoreRecords or MaxInt) so the helper can return all
records up to the enforced max, keeping mergeAndDeduplicateApplications and the
existing maxRecords parameter unchanged.
Comment on lines
109
to
115
| // loadDeclarativeResources loads application resources from declarative files. | ||
| // dbStore parameter is used in composite mode to check for duplicate IDs across both stores. | ||
| // Pass nil for dbStore when in pure declarative mode. | ||
| func loadDeclarativeResources(fileStore applicationStoreInterface, dbStore applicationStoreInterface) error { | ||
| // Create a temporary service for validation | ||
| appService := newApplicationService(fileStore, nil, nil, nil, nil) | ||
|
|
There was a problem hiding this comment.
Fix newApplicationService argument count to avoid a compile failure.
This call passes 5 args, but the other call site uses 6. Go doesn’t allow overloading, so this will not compile unless the signature changed everywhere.
🐛 Proposed fix
- appService := newApplicationService(fileStore, nil, nil, nil, nil)
+ appService := newApplicationService(fileStore, nil, nil, nil, nil, nil)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // loadDeclarativeResources loads application resources from declarative files. | |
| // dbStore parameter is used in composite mode to check for duplicate IDs across both stores. | |
| // Pass nil for dbStore when in pure declarative mode. | |
| func loadDeclarativeResources(fileStore applicationStoreInterface, dbStore applicationStoreInterface) error { | |
| // Create a temporary service for validation | |
| appService := newApplicationService(fileStore, nil, nil, nil, nil) | |
| // loadDeclarativeResources loads application resources from declarative files. | |
| // dbStore parameter is used in composite mode to check for duplicate IDs across both stores. | |
| // Pass nil for dbStore when in pure declarative mode. | |
| func loadDeclarativeResources(fileStore applicationStoreInterface, dbStore applicationStoreInterface) error { | |
| // Create a temporary service for validation | |
| appService := newApplicationService(fileStore, nil, nil, nil, nil, nil) |
🤖 Prompt for AI Agents
In `@backend/internal/application/declarative_resource.go` around lines 109 - 115,
The call to newApplicationService in loadDeclarativeResources uses 5 arguments
but the function requires 6 (causing a compile error); update this invocation in
loadDeclarativeResources to pass the same sixth parameter used at the other call
site (or add a nil placeholder if the service doesn’t need a real value in this
context) so the argument count matches newApplicationService’s signature.
Comment on lines
+224
to
+236
| // Check for duplicate ID in the file store | ||
| exists, err := fileStore.IsApplicationExists(app.ID) | ||
| if err == nil && exists { | ||
| return fmt.Errorf("duplicate application ID '%s': "+ | ||
| "an application with this ID already exists in declarative resources", app.ID) | ||
| } | ||
|
|
||
| // COMPOSITE MODE: Check for duplicate ID in the database store | ||
| if dbStore != nil { | ||
| if exists, err := dbStore.IsApplicationExists(app.ID); err == nil && exists { | ||
| return fmt.Errorf("duplicate application ID '%s': "+ | ||
| "an application with this ID already exists in the database store", app.ID) | ||
| } |
There was a problem hiding this comment.
Propagate store lookup errors during declarative validation.
Errors from IsApplicationExists are ignored, which can silently skip duplicate detection or mask IO failures. Return the error instead.
🛡️ Suggested fix
- exists, err := fileStore.IsApplicationExists(app.ID)
- if err == nil && exists {
+ exists, err := fileStore.IsApplicationExists(app.ID)
+ if err != nil {
+ return fmt.Errorf("failed to check declarative store for ID '%s': %w", app.ID, err)
+ }
+ if exists {
return fmt.Errorf("duplicate application ID '%s': "+
"an application with this ID already exists in declarative resources", app.ID)
}
...
if dbStore != nil {
- if exists, err := dbStore.IsApplicationExists(app.ID); err == nil && exists {
+ exists, err := dbStore.IsApplicationExists(app.ID)
+ if err != nil {
+ return fmt.Errorf("failed to check database store for ID '%s': %w", app.ID, err)
+ }
+ if exists {
return fmt.Errorf("duplicate application ID '%s': "+
"an application with this ID already exists in the database store", app.ID)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Check for duplicate ID in the file store | |
| exists, err := fileStore.IsApplicationExists(app.ID) | |
| if err == nil && exists { | |
| return fmt.Errorf("duplicate application ID '%s': "+ | |
| "an application with this ID already exists in declarative resources", app.ID) | |
| } | |
| // COMPOSITE MODE: Check for duplicate ID in the database store | |
| if dbStore != nil { | |
| if exists, err := dbStore.IsApplicationExists(app.ID); err == nil && exists { | |
| return fmt.Errorf("duplicate application ID '%s': "+ | |
| "an application with this ID already exists in the database store", app.ID) | |
| } | |
| // Check for duplicate ID in the file store | |
| exists, err := fileStore.IsApplicationExists(app.ID) | |
| if err != nil { | |
| return fmt.Errorf("failed to check declarative store for ID '%s': %w", app.ID, err) | |
| } | |
| if exists { | |
| return fmt.Errorf("duplicate application ID '%s': "+ | |
| "an application with this ID already exists in declarative resources", app.ID) | |
| } | |
| // COMPOSITE MODE: Check for duplicate ID in the database store | |
| if dbStore != nil { | |
| exists, err := dbStore.IsApplicationExists(app.ID) | |
| if err != nil { | |
| return fmt.Errorf("failed to check database store for ID '%s': %w", app.ID, err) | |
| } | |
| if exists { | |
| return fmt.Errorf("duplicate application ID '%s': "+ | |
| "an application with this ID already exists in the database store", app.ID) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@backend/internal/application/declarative_resource.go` around lines 224 - 236,
The code currently swallows errors from fileStore.IsApplicationExists and
dbStore.IsApplicationExists; update the checks in the declarative validation
(around the IsApplicationExists calls) to return the underlying error instead of
ignoring it—i.e., if err != nil return fmt.Errorf("failed to check application
existence: %w", err) (or similar) for both fileStore.IsApplicationExists(app.ID)
and dbStore.IsApplicationExists(app.ID), while still returning the duplicate-ID
fmt.Errorf when exists == true; ensure you wrap the original error to preserve
context.
Comment on lines
+28
to
+32
| // Store-level errors for application operations. | ||
| var ( | ||
| // ErrResultLimitExceededInCompositeMode is returned when the result limit is exceeded in composite mode. | ||
| ErrResultLimitExceededInCompositeMode = errors.New("result limit exceeded in composite mode") | ||
| ) |
There was a problem hiding this comment.
Make the composite-mode sentinel error unexported.
This is a store-internal error and shouldn’t be exported from a constants file; it can stay package-private with a lowercase name and the references updated accordingly.
🔧 Suggested change
- ErrResultLimitExceededInCompositeMode = errors.New("result limit exceeded in composite mode")
+ errResultLimitExceededInCompositeMode = errors.New("result limit exceeded in composite mode")As per coding guidelines: "backend/internal/**/*constants.go: Keep internal constants such as database queries, error codes, and other implementation details unexported (private)".
🤖 Prompt for AI Agents
In `@backend/internal/application/error_constants.go` around lines 28 - 32, Rename
the exported sentinel error ErrResultLimitExceededInCompositeMode to a
package-private name (e.g., errResultLimitExceededInCompositeMode) and update
all references to this symbol throughout the package to use the new lowercase
identifier; ensure the variable still uses errors.New("result limit exceeded in
composite mode") and that any callers within the package import no changes
beyond the identifier rename so the error remains internal to the package.
Comment on lines
+236
to
+241
| // ErrorCannotModifyDeclarativeResource is the error returned when trying to modify a declarative resource. | ||
| ErrorCannotModifyDeclarativeResource = serviceerror.ServiceError{ | ||
| Type: serviceerror.ClientErrorType, | ||
| Code: "APP-1028", | ||
| Error: "Cannot modify declarative resource", | ||
| ErrorDescription: "The application is declarative and cannot be modified or deleted", |
There was a problem hiding this comment.
Fix duplicate error code (APP-1028).
ErrorWhileRetrievingFlowDefinition already uses APP-1028, so this new error should have a unique code to avoid client-side ambiguity.
🔧 Suggested change
- Code: "APP-1028",
+ Code: "APP-1030",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // ErrorCannotModifyDeclarativeResource is the error returned when trying to modify a declarative resource. | |
| ErrorCannotModifyDeclarativeResource = serviceerror.ServiceError{ | |
| Type: serviceerror.ClientErrorType, | |
| Code: "APP-1028", | |
| Error: "Cannot modify declarative resource", | |
| ErrorDescription: "The application is declarative and cannot be modified or deleted", | |
| // ErrorCannotModifyDeclarativeResource is the error returned when trying to modify a declarative resource. | |
| ErrorCannotModifyDeclarativeResource = serviceerror.ServiceError{ | |
| Type: serviceerror.ClientErrorType, | |
| Code: "APP-1030", | |
| Error: "Cannot modify declarative resource", | |
| ErrorDescription: "The application is declarative and cannot be modified or deleted", |
🤖 Prompt for AI Agents
In `@backend/internal/application/error_constants.go` around lines 236 - 241, The
ErrorCannotModifyDeclarativeResource constant currently reuses the code
"APP-1028" already assigned to ErrorWhileRetrievingFlowDefinition; update the
ServiceError.Code value for ErrorCannotModifyDeclarativeResource to a unique
code (for example "APP-1029") by editing the Code field in the
serviceerror.ServiceError literal for ErrorCannotModifyDeclarativeResource so it
no longer conflicts with ErrorWhileRetrievingFlowDefinition.
rajithacharith
force-pushed
the
composite-pagination
branch
from
e90ea1b to
50c81d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/internal/application/composite_store.go`:
- Around line 163-178: The code mutates input slice elements by setting
dbApps[i].IsReadOnly and fileApps[i].IsReadOnly directly; instead, create a
local copy of each element before changing IsReadOnly and appending to result
(e.g., copy := dbApps[i]; copy.IsReadOnly = false; result = append(result,
copy)), and do the same for fileApps with IsReadOnly = true; apply this change
where dbApps, fileApps, seen, and result are handled so the original caller
slices remain unchanged.
🧹 Nitpick comments (1)
backend/internal/application/composite_store.go (1)
61-62: Fetcher functions ignore thelimitparameter.The fetchers wrap
GetApplicationList()which takes no parameters, so thelimit intpassed by the helper is unused. This is unavoidable given the current interface, but the code could be clearer by adding a comment explaining this design constraint.📝 Suggested clarifying comment
- func(limit int) ([]model.BasicApplicationDTO, error) { return c.dbStore.GetApplicationList() }, - func(limit int) ([]model.BasicApplicationDTO, error) { return c.fileStore.GetApplicationList() }, + // Note: limit param is unused since GetApplicationList() doesn't support pagination. + // The helper's limit only affects final result slicing, not fetch optimization. + func(limit int) ([]model.BasicApplicationDTO, error) { return c.dbStore.GetApplicationList() }, + func(limit int) ([]model.BasicApplicationDTO, error) { return c.fileStore.GetApplicationList() },
Comment on lines
+163
to
+178
| // Add DB apps first (they take precedence) - mark as mutable (isReadOnly=false) | ||
| for i := range dbApps { | ||
| if !seen[dbApps[i].ID] { | ||
| seen[dbApps[i].ID] = true | ||
| dbApps[i].IsReadOnly = false | ||
| result = append(result, dbApps[i]) | ||
| } | ||
| } | ||
|
|
||
| // Add file apps if not already present - mark as immutable (isReadOnly=true) | ||
| for i := range fileApps { | ||
| if !seen[fileApps[i].ID] { | ||
| seen[fileApps[i].ID] = true | ||
| fileApps[i].IsReadOnly = true | ||
| result = append(result, fileApps[i]) | ||
| } |
There was a problem hiding this comment.
Input slice elements are mutated.
The function modifies dbApps[i].IsReadOnly and fileApps[i].IsReadOnly directly, which mutates the caller's slices. This could cause unexpected side effects if the caller reuses the input slices.
🛡️ Proposed fix to avoid mutation
// Add DB apps first (they take precedence) - mark as mutable (isReadOnly=false)
for i := range dbApps {
if !seen[dbApps[i].ID] {
seen[dbApps[i].ID] = true
- dbApps[i].IsReadOnly = false
- result = append(result, dbApps[i])
+ app := dbApps[i]
+ app.IsReadOnly = false
+ result = append(result, app)
}
}
// Add file apps if not already present - mark as immutable (isReadOnly=true)
for i := range fileApps {
if !seen[fileApps[i].ID] {
seen[fileApps[i].ID] = true
- fileApps[i].IsReadOnly = true
- result = append(result, fileApps[i])
+ app := fileApps[i]
+ app.IsReadOnly = true
+ result = append(result, app)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Add DB apps first (they take precedence) - mark as mutable (isReadOnly=false) | |
| for i := range dbApps { | |
| if !seen[dbApps[i].ID] { | |
| seen[dbApps[i].ID] = true | |
| dbApps[i].IsReadOnly = false | |
| result = append(result, dbApps[i]) | |
| } | |
| } | |
| // Add file apps if not already present - mark as immutable (isReadOnly=true) | |
| for i := range fileApps { | |
| if !seen[fileApps[i].ID] { | |
| seen[fileApps[i].ID] = true | |
| fileApps[i].IsReadOnly = true | |
| result = append(result, fileApps[i]) | |
| } | |
| // Add DB apps first (they take precedence) - mark as mutable (isReadOnly=false) | |
| for i := range dbApps { | |
| if !seen[dbApps[i].ID] { | |
| seen[dbApps[i].ID] = true | |
| app := dbApps[i] | |
| app.IsReadOnly = false | |
| result = append(result, app) | |
| } | |
| } | |
| // Add file apps if not already present - mark as immutable (isReadOnly=true) | |
| for i := range fileApps { | |
| if !seen[fileApps[i].ID] { | |
| seen[fileApps[i].ID] = true | |
| app := fileApps[i] | |
| app.IsReadOnly = true | |
| result = append(result, app) | |
| } |
🤖 Prompt for AI Agents
In `@backend/internal/application/composite_store.go` around lines 163 - 178, The
code mutates input slice elements by setting dbApps[i].IsReadOnly and
fileApps[i].IsReadOnly directly; instead, create a local copy of each element
before changing IsReadOnly and appending to result (e.g., copy := dbApps[i];
copy.IsReadOnly = false; result = append(result, copy)), and do the same for
fileApps with IsReadOnly = true; apply this change where dbApps, fileApps, seen,
and result are handled so the original caller slices remain unchanged.
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
1 participant
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
Add composite store support for Applications
Approach
This pull request introduces support for a composite application store that combines both file-based (declarative/immutable) and database (mutable) sources. It also adds new error handling for composite mode and improves validation logic to prevent duplicate application IDs across stores. Additionally, it updates mocks and interfaces to support the new composite functionality and clarifies how application store mode is determined.
Key changes include:
Composite Store Implementation:
compositeApplicationStoreincomposite_store.go, which merges file-based and database-backed application stores. Read operations merge results from both stores, while write operations only affect the database store. Includes logic to prevent modification of declarative (YAML-based) applications and deduplicate results.Store Mode Configuration:
config.goto determine the application store mode (mutable,declarative, orcomposite) based on service-level or global configuration, with clear resolution order.Interface & Mock Updates:
applicationStoreInterfaceand its mock to includeIsApplicationExists,IsApplicationExistsByName, andIsApplicationDeclarativemethods for composite store compatibility. Also implemented these incachedBackedApplicationStore. [1] [2]Declarative Resource Loading & Validation:
loadDeclarativeResourcesto accept both file and database stores, enabling duplicate ID checks across both in composite mode. AddedvalidateApplicationWrapperto enforce unique IDs and basic validation for declarative applications. [1] [2]GetAllResourceIDsto exclude declarative (read-only) applications in composite mode.Error Handling Enhancements:
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
Error Handling
Tests