Support composite store for resource service#1530
Open
rajithacharith wants to merge 1 commit intoasgardeo:mainfrom
Open
Support composite store for resource service#1530rajithacharith wants to merge 1 commit intoasgardeo:mainfrom
rajithacharith wants to merge 1 commit intoasgardeo:mainfrom
Conversation
rajithacharith
requested review from
ThaminduDilshan,
darshanasbg and
senthalan
as code owners
|
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:
📝 WalkthroughWalkthroughAdds declarative resource support: new file-based and composite stores, declarative exporter and YAML parsing/validation, per-service store-mode resolution, immutability enforcement for declarative entities, Initialize now returns an exporter, and extensive tests/mocks updated across resource subsystem. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Init as Init
participant Config as Config
participant StoreFactory as StoreFactory
participant FileStore as FileStore
participant DBStore as DBStore
participant Service as Service
participant Exporter as Exporter
Caller->>Init: Initialize(mux, ouService)
Init->>Config: getResourceStoreMode()
Config-->>Init: mode
alt declarative/composite
Init->>StoreFactory: construct file-based store
StoreFactory-->>FileStore: file store ready
Init->>StoreFactory: construct DB store (if composite)
StoreFactory-->>DBStore: db store ready
Init->>FileStore: loadDeclarativeResources()
FileStore-->>Init: parsed declarative DTOs
else mutable
Init->>StoreFactory: construct DB store
StoreFactory-->>DBStore: db store ready
end
Init->>Service: create ResourceService(store)
Service-->>Init: service
Init->>Exporter: create ResourceServerExporter (if applicable)
Exporter-->>Init: exporter
Init-->>Caller: (service, exporter, nil)
sequenceDiagram
participant Client as Client
participant CompositeStore as CompositeStore
participant DBStore as DBStore
participant FileStore as FileStore
Client->>CompositeStore: GetResourceServer(id)
CompositeStore->>DBStore: GetResourceServer(id)
DBStore-->>CompositeStore: dbResult?
CompositeStore->>FileStore: GetResourceServer(id)
FileStore-->>CompositeStore: fileResult?
CompositeStore->>CompositeStore: merge & dedupe (DB priority)
CompositeStore-->>Client: merged result
Client->>CompositeStore: UpdateResourceServer(rs)
CompositeStore->>CompositeStore: IsResourceServerDeclarative(rs.id)?
alt declarative
CompositeStore-->>Client: ErrorImmutableResourceServer
else mutable
CompositeStore->>DBStore: UpdateResourceServer(rs)
DBStore-->>CompositeStore: success
CompositeStore-->>Client: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 16
🧹 Nitpick comments (3)
backend/internal/resource/config.go (1)
38-56: Log a warning when an invalidResource.Storevalue is silently ignored.An invalid configured store mode (e.g., a typo like
"composit") falls through theswitchand silently reverts to the globalDeclarativeResources.Enabledsetting. Without a warning, operators have no signal that their configuration is being ignored. Per project guidelines, use thelogpackage frominternal/system/logfor this.♻️ Proposed fix
if cfg.Resource.Store != "" { mode := serverconst.StoreMode(strings.ToLower(strings.TrimSpace(cfg.Resource.Store))) switch mode { case serverconst.StoreModeMutable, serverconst.StoreModeDeclarative, serverconst.StoreModeComposite: return mode } + log.GetLogger().Warn("Invalid resource store mode configured, falling back to global setting", + log.String("configured_value", cfg.Resource.Store)) }As per coding guidelines: "Add minimal info logs and ensure server errors are logged for debugging."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/config.go` around lines 38 - 56, The getResourceStoreMode function silently ignores invalid cfg.Resource.Store values; update it to detect when cfg.Resource.Store is non-empty but not one of serverconst.StoreModeMutable/Declarative/Composite and log a warning via the internal/system/log package (include the provided invalid value and that it will fall back to Declarative or Mutable based on declarativeresource.IsDeclarativeModeEnabled()). Keep the existing return behavior but ensure you call log.Warnf (or similar) before returning to give operators visibility into the ignored config.backend/internal/resource/model.go (1)
223-251: Unexport the processed DTO types.
ResourceServerProcessedDTO,ResourceProcessedDTO, andActionProcessedDTOare used exclusively within the resource package for internal processing of the declarative resource pipeline. Since they are not part ofResourceServiceInterfaceand carry no JSON/YAML struct tags, they should be unexported to comply with the guideline: "Only export the service interface and models that are used in the service interface from a package."Proposed refactor — make processed DTOs unexported
-type ResourceServerProcessedDTO struct { +type resourceServerProcessedDTO struct { ... -type ResourceProcessedDTO struct { +type resourceProcessedDTO struct { ... -type ActionProcessedDTO struct { +type actionProcessedDTO struct {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/model.go` around lines 223 - 251, The three processed DTO types (ResourceServerProcessedDTO, ResourceProcessedDTO, ActionProcessedDTO) are internal-only and should be unexported; rename them to lowercase identifiers (e.g., resourceServerProcessedDTO, resourceProcessedDTO, actionProcessedDTO) and update every usage within the resource package (functions, method signatures, variables, returns, and tests) to the new names so package-internal processing compiles; keep the field names and types unchanged and do not add JSON/YAML tags.backend/internal/resource/config_test.go (1)
41-55: Reset ThunderRuntime after the suite to avoid cross‑test leakage.
ATearDownSuitethat callsconfig.ResetThunderRuntime()will keep global config state from bleeding into other tests.♻️ Suggested addition
func (s *ResourceConfigTestSuite) SetupSuite() { // Initialize runtime once for all tests in the suite testConfig := &config.Config{} err := config.InitializeThunderRuntime("/tmp/test", testConfig) if err != nil { s.Fail("Failed to initialize runtime", err) } } + +func (s *ResourceConfigTestSuite) TearDownSuite() { + config.ResetThunderRuntime() +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/config_test.go` around lines 41 - 55, The test suite leaves a shared ThunderRuntime initialized in SetupSuite which can leak into other tests; add a TearDownSuite method on ResourceConfigTestSuite that calls config.ResetThunderRuntime() to clean up the global runtime after the suite runs. Place the new TearDownSuite alongside existing SetupSuite/SetupTest methods and reference config.ResetThunderRuntime() so the runtime initialized by config.InitializeThunderRuntime(...) in SetupSuite and accessed via config.GetThunderRuntime() is properly cleared.
🤖 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/resource/composite_store_test.go`:
- Around line 201-211: The count methods (GetResourceServerListCount and
GetResourceListCount) currently return raw sums causing pagination mismatch with
the deduplicating list methods; update the composite store implementations to
compute unique counts by determining overlapping IDs (e.g., fetch IDs or lists
from dbStore and fileStore, compute the set union with DB winning duplicates)
and return the deduplicated count, then update
TestGetResourceServerListCount_SumsBothStores (and the analogous test at lines
386-396) to mock overlapping IDs and assert the deduplicated total rather than a
raw sum; alternatively, if you choose to keep the raw-sum behavior, change the
test to explicitly document this limitation instead of asserting a summed value.
- Around line 530-540: The test named TestValidatePermissions_ChecksBothStores
is misleading because it only sets expectations on dbStoreMock and does not
assert fileStoreMock interaction; rename the test to
TestValidatePermissions_DelegatesToDB and update the test body to explicitly
assert that fileStoreMock was not called (e.g., use
fileStoreMock.AssertNotCalled or equivalent) or otherwise mirror other
delegation tests; locate the test function
TestValidatePermissions_ChecksBothStores and change its name and add an
assertion referencing dbStoreMock, fileStoreMock, and
compositeStore.ValidatePermissions to make the intent clear and consistent.
In `@backend/internal/resource/composite_store.go`:
- Around line 66-90: GetResourceServerList currently calls
dbStore.GetResourceServerList(ctx, 0, 0) (and fileStore likewise) to mean “fetch
all”, but many SQL engines treat LIMIT 0 as zero rows so DB-backed records are
dropped; change the call to fetch all rows properly by either adding/using a
ListAll helper on dbStore (e.g., dbStore.ListAllResourceServers(ctx)) or first
calling a count method and then calling GetResourceServerList with limit=count,
and apply the same pattern to GetResourceList, GetResourceListByParent, and
GetActionList so merged results are correct before pagination in
compositeResourceStore.GetResourceServerList.
- Around line 93-105: The GetResourceServerListCount method currently sums
dbStore and fileStore counts and overcounts when the same ID exists in both
stores; change compositeResourceStore.GetResourceServerListCount to dedupe by
retrieving the ID lists (using the same merge/list logic you use elsewhere),
build a set of unique IDs and return its size (instead of dbCount+fileCount),
and apply the same pattern to the analogous resource/action count methods so
counts match the deduped list/merge behavior; use
c.dbStore.GetResourceServerList (or the list method that returns IDs) and
c.fileStore.GetResourceServerList to collect IDs, merge into a
map[string]struct{} (or equivalent) and return len(map).
In `@backend/internal/resource/declarative_resource.go`:
- Around line 308-312: While building resourceHandleMap from rsDTO.Resources,
detect and fail on duplicate ResourceDTO.Handle values instead of silently
overwriting: in the loop that populates resourceHandleMap (iterating
rsDTO.Resources) check if resourceHandleMap[res.Handle] already exists and
return a clear error (or validation failure) naming the duplicate handle and the
conflicting ResourceDTO identifiers; update the function that surrounds this
code to propagate that error so parent resolution cannot proceed with ambiguous
handles.
- Around line 105-127: The export currently calls e.service.GetResourceList and
e.service.GetActionList once with a fixed limit (1000) which can truncate
results; modify the logic around GetResourceList and GetActionList so you page
through all results (using limit+offset or the service's paging cursor) and
accumulate all resources and all actions before building ResourceDTOs (i.e.,
replace the single-call uses of GetResourceList and GetActionList in
declarative_resource.go with a loop or helper that repeatedly calls
GetResourceList/GetActionList until no more items are returned, then proceed to
construct the ResourceDTO map and attach complete Actions slices).
- Around line 333-353: processResourceDTO currently accepts empty/invalid
resource and action handles; before calling buildPermissionString and before
appending ActionProcessedDTO, validate the resource and action name/handle using
the existing validateHandle and required-field checks: call
validateHandle(resDTO.Handle) and ensure resDTO.Name is non-empty (return an
error) before computing permission, and inside the loop call
validateHandle(actionDTO.Handle) and ensure actionDTO.Name is non-empty (return
an error) before constructing actionPermission and appending to
processedRes.Actions; use the existing symbols ResourceProcessedDTO,
ActionProcessedDTO, buildPermissionString and validateHandle so invalid inputs
are rejected early.
- Around line 150-171: Root-level actions returned by GetActionList
(rootActions) are currently ignored; update the assembly logic in
declarative_resource.go to include those server-level actions in the DTO by
creating or reusing a top-level ResourceDTO and assigning rootActions.Actions to
its Actions (use resourceMap/respective ResourceDTO or create a synthetic root
entry when dto.Resources is empty), so the exported dto contains server-level
actions instead of silently dropping them; adjust the branch that checks
len(rootActions.Actions) and len(dto.Resources) to merge rootActions into
dto.Resources (or return a clear error) accordingly.
- Around line 111-147: The current resourceMap keyed by res.Handle can overwrite
siblings with identical handles; change it to key by the resource unique ID
(e.g., use map[int64]*ResourceDTO or map[string]*ResourceDTO keyed with res.ID)
so each ResourceDTO stored under resourceMap corresponds uniquely to res.ID,
populate resourceMap using res.ID immediately after creating ResourceDTO, and
when setting parent (res.Parent) look up the parent handle via
resourceMap[parentID] (or maintain a separate id→handle map) instead of scanning
resources.Resources by handle; keep all other logic that fills Actions
(GetActionList, ActionDTO) the same.
- Around line 39-55: Make the exporter unexported: rename the exported type
ResourceServerExporter to resourceServerExporter and make
NewResourceServerExporterForTest unexported (e.g.,
newResourceServerExporterForTest) so the exporter remains an internal
implementation detail; keep the existing newResourceServerExporter factory and
ResourceServiceInterface reference, ensure tests live in the same package (so
they can call the unexported factory), and remove any external dependencies on
the exported names (update callers and imports accordingly, keeping the
testing.Testing() guard in the test-only factory).
- Around line 293-296: Replace the hard-coded default delimiter ":" used when
rsDTO.Delimiter is empty with the system runtime config: call the runtime config
accessor (use ThunderRuntime in internal/system/config) to get the default
delimiter (e.g., getDefaultDelimiter() or equivalent) and use that value when
delimiter == ""; update the code around rsDTO.Delimiter and delimiter variable
initialization to read from ThunderRuntime instead of using ":" so it matches
service configuration.
- Around line 69-82: GetAllResourceIDs in ResourceServerExporter currently calls
e.service.GetResourceServerList with a hardcoded limit of 1000 which can
truncate results; change the function to page through results (e.g., set a
variable limit, call GetResourceServerList repeatedly with increasing offset or
a nextPageToken until the returned servers.ResourceServers length is less than
limit or there is no next token), accumulate IDs across pages while still
filtering with e.service.IsResourceServerDeclarative(server.ID), and preserve
existing error handling by returning early on any GetResourceServerList error.
In `@backend/internal/resource/file_based_store_test.go`:
- Around line 47-55: TearDownTest currently does nothing; update the
type-assertion block in FileBasedResourceStoreTestSuite.TearDownTest to clear
the singleton by accessing the underlying GenericFileBasedStore on the
fileBasedResourceStore (e.g., fileStore.<fieldContainingGenericStore>) and
calling its Clear/Reset/test-only ResetSingleton method (or add such a test-only
helper on GenericFileBasedStore if missing) so the file-based singleton state is
explicitly cleared between tests.
In `@backend/internal/resource/file_based_store.go`:
- Around line 154-156: The IsResourceServerDeclarative method currently always
returns true; change it to check whether the resource actually exists in the
file store and return true only when present. In
fileBasedResourceStore.IsResourceServerDeclarative call the underlying presence
check (e.g., use the store’s exists/get/lookup function for resource id —
reference fileBasedResourceStore and IsResourceServerDeclarative) and return
true if the resource is found, false if not found (treat lookup errors as not
found or propagate as appropriate for your code path) so declarative-only
behavior is only applied to existing resources.
- Around line 360-363: The IsActionExist implementation swallows all GetAction
errors and always returns (false, nil); change it to call GetAction and: if err
== nil return (true, nil); if errors.Is(err, errActionNotFound) return (false,
nil); otherwise return (false, err) so unexpected/storage errors are propagated;
update the function IsActionExist to use GetAction and check err against
errActionNotFound.
- Around line 194-205: GetResourceList/GetResourceListByParent (and their
count/action counterparts) currently return empty results; update them to
actually return file-based data by mapping the incoming resServerInternalID ->
serverID and then calling the existing helpers (getResourcesByServerID,
getActionsByServerAndResource). Implement or reuse a resolver (e.g., add a
helper like getServerIDForInternalID or use an existing server metadata map on
fileBasedResourceStore) to translate resServerInternalID to the string serverID,
validate existence, then call
getResourcesByServerID/getActionsByServerAndResource and apply limit/offset or
counts; if such mapping is impossible, add clear documentation/error return in
these methods explaining file-based entries are excluded from composite list
operations instead of returning empty slices silently.
---
Nitpick comments:
In `@backend/internal/resource/config_test.go`:
- Around line 41-55: The test suite leaves a shared ThunderRuntime initialized
in SetupSuite which can leak into other tests; add a TearDownSuite method on
ResourceConfigTestSuite that calls config.ResetThunderRuntime() to clean up the
global runtime after the suite runs. Place the new TearDownSuite alongside
existing SetupSuite/SetupTest methods and reference config.ResetThunderRuntime()
so the runtime initialized by config.InitializeThunderRuntime(...) in SetupSuite
and accessed via config.GetThunderRuntime() is properly cleared.
In `@backend/internal/resource/config.go`:
- Around line 38-56: The getResourceStoreMode function silently ignores invalid
cfg.Resource.Store values; update it to detect when cfg.Resource.Store is
non-empty but not one of serverconst.StoreModeMutable/Declarative/Composite and
log a warning via the internal/system/log package (include the provided invalid
value and that it will fall back to Declarative or Mutable based on
declarativeresource.IsDeclarativeModeEnabled()). Keep the existing return
behavior but ensure you call log.Warnf (or similar) before returning to give
operators visibility into the ignored config.
In `@backend/internal/resource/model.go`:
- Around line 223-251: The three processed DTO types
(ResourceServerProcessedDTO, ResourceProcessedDTO, ActionProcessedDTO) are
internal-only and should be unexported; rename them to lowercase identifiers
(e.g., resourceServerProcessedDTO, resourceProcessedDTO, actionProcessedDTO) and
update every usage within the resource package (functions, method signatures,
variables, returns, and tests) to the new names so package-internal processing
compiles; keep the field names and types unchanged and do not add JSON/YAML
tags.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
backend/cmd/server/servicemanager.gobackend/internal/resource/ResourceServiceInterface_mock_test.gobackend/internal/resource/composite_store.gobackend/internal/resource/composite_store_test.gobackend/internal/resource/config.gobackend/internal/resource/config_test.gobackend/internal/resource/declarative_resource.gobackend/internal/resource/declarative_resource_test.gobackend/internal/resource/error_constants.gobackend/internal/resource/file_based_store.gobackend/internal/resource/file_based_store_test.gobackend/internal/resource/init.gobackend/internal/resource/init_test.gobackend/internal/resource/model.gobackend/internal/resource/resourceStoreInterface_mock_test.gobackend/internal/resource/service.gobackend/internal/resource/service_test.gobackend/internal/resource/store.gobackend/internal/system/config/config.gobackend/internal/system/declarative_resource/entity/entity_store.gobackend/tests/mocks/resourcemock/ResourceServiceInterface_mock.gobackend/tests/mocks/resourcemock/resourceStoreInterface_mock.go
rajithacharith
force-pushed
the
composite-resource-server
branch
from
b5a2704 to
14a2a2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (17)
backend/internal/resource/composite_store_test.go (2)
201-211:⚠️ Potential issue | 🟠 MajorCount methods diverge from deduplicated list semantics.
List APIs dedupe (DB wins), but counts sum both stores, so pagination can report more results than actually available. Consider computing unique counts (or explicitly documenting the limitation in tests).Also applies to: 386-396
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/composite_store_test.go` around lines 201 - 211, The test TestGetResourceServerListCount_SumsBothStores exposes a mismatch: list APIs deduplicate (DB wins) but GetResourceServerListCount simply sums both stores, causing incorrect pagination; update the composite implementation of GetResourceServerListCount (and corresponding tests such as the one around lines 386-396) to compute a deduplicated count by merging identifiers from DB and file stores and counting unique server IDs (ensuring DB entries take precedence when resolving duplicates), or alternatively update the test to explicitly assert and document the current non-deduplicating behavior—locate the compositeStore.GetResourceServerListCount implementation and the tests TestGetResourceServerListCount_SumsBothStores to apply the chosen fix.
530-540:⚠️ Potential issue | 🟡 MinorTest name suggests cross-store validation but only DB is used.
Rename to reflect DB-only delegation and assert the file store isn’t called.Suggested rename + assertion
-func (s *CompositeResourceStoreTestSuite) TestValidatePermissions_ChecksBothStores() { +func (s *CompositeResourceStoreTestSuite) TestValidatePermissions_DelegatesToDB() { permissions := []string{"perm1", "perm2", "perm3"} s.dbStoreMock.On("ValidatePermissions", s.ctx, 1, permissions).Return([]string{}, nil) invalid, err := s.compositeStore.ValidatePermissions(s.ctx, 1, permissions) assert.NoError(s.T(), err) assert.Len(s.T(), invalid, 0) s.dbStoreMock.AssertExpectations(s.T()) + s.fileStoreMock.AssertNotCalled(s.T(), "ValidatePermissions") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/composite_store_test.go` around lines 530 - 540, Rename the test TestValidatePermissions_ChecksBothStores to reflect DB-only delegation (e.g., TestValidatePermissions_DelegatesToDBOnly) and add an assertion that the file store mock's ValidatePermissions method was not invoked (use s.fileStoreMock.AssertNotCalled/AssertNotCalled with the "ValidatePermissions" method and appropriate args) in addition to the existing s.dbStoreMock expectations and result assertions so the test documents and enforces that only the DB store is used.backend/internal/resource/file_based_store_test.go (1)
47-54:⚠️ Potential issue | 🟡 MinorTearDownTest doesn’t reset the singleton store.
The comment says state is cleared, but no reset happens, so tests can leak data between runs. Please invoke a clear/reset on the underlying singleton (or add a test-only reset helper on the generic store).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store_test.go` around lines 47 - 54, TearDownTest is supposed to clear the singleton EntityStore but currently does nothing; update TearDownTest in FileBasedResourceStoreTestSuite to call the test-only reset/clear method on the underlying singleton (accessed via s.store.(*fileBasedResourceStore) -> its GenericFileBasedStore/EntityStore) so state is wiped between tests, or add a new package-private Reset/Clear method on GenericFileBasedStore/EntityStore and invoke that from TearDownTest to ensure no data leaks between tests.backend/internal/resource/declarative_resource.go (8)
39-55: Keep exporter unexported inside internal package.
ResourceServerExporterandNewResourceServerExporterForTestare exported but are internal implementation details; prefer lowercase types/factories and keep tests in‑package.As per coding guidelines, "Only export the service interface (e.g.,
XServiceInterface) and models that are used in the service interface from a package".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource.go` around lines 39 - 55, ResourceServerExporter and NewResourceServerExporterForTest are exported but should be unexported internal details; rename ResourceServerExporter to resourceServerExporter and NewResourceServerExporterForTest to newResourceServerExporterForTest (keep existing newResourceServerExporter as the internal constructor), update references and tests to use the unexported names, and move any tests that need access into the same package (non-*_test external package) so they can call the unexported symbols; ensure testing.Testing() guard remains intact in the test-only factory.
300-304:⚠️ Potential issue | 🟠 MajorDetect duplicate handles when building
resourceHandleMap.Currently duplicates silently overwrite entries, which breaks parent resolution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource.go` around lines 300 - 304, When building resourceHandleMap from rsDTO.Resources (ResourceDTO.Handle), detect duplicate handles instead of silently overwriting: before assigning resourceHandleMap[rsDTO.Resources[i].Handle] = &rsDTO.Resources[i], check whether that key already exists and if so surface an error (or collect and return all duplicate handles) so parent resolution fails fast; update the function that constructs resourceHandleMap to return an error when duplicates are found and include the conflicting Handle and the involved ResourceDTO identifiers in the error message.
150-155:⚠️ Potential issue | 🟠 MajorRoot‑level actions are fetched but ignored.
Server‑level actions (
resourceID == nil) are retrieved and then dropped, so exports are incomplete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource.go` around lines 150 - 155, The code calls e.service.GetActionList(ctx, id, nil, 1000, 0) to fetch root‑level (resourceID == nil) actions but discards the returned list; update the code to capture the returned actions (e.g., assign the first return to a variable like rootActions, check for err) and merge/append those rootActions into the export/output data structure used by the surrounding function (the same place where per‑resource actions are collected), ensuring root‑level actions are included in the final export; reference the GetActionList call and the variable that accumulates actions in this function (e.g., rootActions and the export actions slice/map).
67-81:⚠️ Potential issue | 🟠 MajorFixed limit can truncate server exports.
GetAllResourceIDscallsGetResourceServerListonce withlimit=1000; exports will be incomplete when more servers exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource.go` around lines 67 - 81, GetAllResourceIDs currently calls e.service.GetResourceServerList once with a fixed limit (1000) which can miss servers; update GetAllResourceIDs to page through results until all are returned by repeatedly calling e.service.GetResourceServerList (using limit and advancing offset/page token as supported) and accumulating server IDs from servers.ResourceServers while still skipping declarative servers via e.service.IsResourceServerDeclarative(server.ID); ensure loop exits when fewer results than limit (or when next-page token is empty) and return the full ids slice.
105-127:⚠️ Potential issue | 🟠 MajorResources/actions export can truncate at 1000.
GetResourceListandGetActionListare called once withlimit=1000; exports will be incomplete above that size.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource.go` around lines 105 - 127, The export currently fetches resources and actions with a fixed limit of 1000 which truncates large exports; update the logic around e.service.GetResourceList and e.service.GetActionList in declarative_resource.go to page through results until all items are retrieved (e.g., loop incrementing offset or using a next-token) and merge each page into the resource list/map (resourceMap) and into each resource's actions collection (when calling GetActionList with &res.ID) rather than relying on a single call with limit=1000; ensure you stop when a page returns fewer than the requested limit or when the service indicates no more pages.
283-288:⚠️ Potential issue | 🟠 MajorDefault delimiter should follow runtime config.
Hard‑coding
":"diverges from configured defaults; reusegetDefaultDelimiter()(or equivalent) so exports match runtime behavior.As per coding guidelines, "Use
ThunderRuntimeininternal/system/configto read system configs".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource.go` around lines 283 - 288, In processResourceServerDTO, avoid hard-coding the delimiter ":"; instead obtain the default from the runtime config (use getDefaultDelimiter() or ThunderRuntime in internal/system/config) and fall back only if that call returns empty; update the delimiter assignment (variable delimiter in processResourceServerDTO) to call the runtime config helper and use its value so exports match runtime behavior.
111-147:⚠️ Potential issue | 🟠 MajorHandle‑keyed map can overwrite resources.
Handles are only unique per parent, so a
map[handle]can drop siblings in other branches. Consider keying by resource ID and mapping parent IDs → handles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource.go` around lines 111 - 147, resourceMap is keyed by handle which is only unique per parent and can overwrite siblings; change the map to be keyed by resource ID (e.g., map[<IDType>]*ResourceDTO) and store the handle on the DTO for lookups. Update the declaration of resourceMap, the assignment currently using resourceMap[res.Handle] = &resourceDTO, and the parent resolution logic (the loop that compares parentRes.ID == *res.Parent) to look up parent by ID (e.g., parentDTO := resourceMap[*res.Parent] or maintain a separate parentID→handle map) so parent relationships use IDs instead of handles; ensure GetActionList usage and ActionDTO population remain unchanged.
315-342:⚠️ Potential issue | 🟠 MajorValidate resource/action names and handles during processing.
processResourceDTOaccepts empty/invalid handles and action handles; reuse existing validators to fail fast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource.go` around lines 315 - 342, processResourceDTO currently accepts empty/invalid resource and action handles; update it to validate resDTO.Name and resDTO.Handle and each actionDTO.Name and actionDTO.Handle using the existing shared validators (e.g., the package's ValidateHandle/ValidateName or equivalent used elsewhere) before building permission or appending ActionProcessedDTO; if any validation fails, fail fast by logging the error and returning nil (or propagate an error) so invalid resources/actions are not processed; touch symbols: processResourceDTO, resDTO, actionDTO, buildPermissionString, ResourceProcessedDTO, ActionProcessedDTO.backend/internal/resource/composite_store.go (2)
94-105:⚠️ Potential issue | 🟠 MajorCounts can overstate when IDs overlap across stores.
Summing DB + file counts will overcount if the same IDs exist in both stores, which will make pagination metadata incorrect. Consider deduping using the same merge logic as list results.
Also applies to: 222-248, 321-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/composite_store.go` around lines 94 - 105, The current GetResourceServerListCount implementation simply adds dbStore.GetResourceServerListCount and fileStore.GetResourceServerListCount which double-counts IDs present in both stores; change it to use the same merge/dedup logic as the list path: retrieve the underlying resource lists (or ID lists) from dbStore and fileStore and compute the unique count by deduping on the resource ID (i.e. use the same merge function used by ListResourceServers or the composite merge helper) instead of summing counts; apply the same fix to the other analogous count functions referenced in the comment (the other composite count methods that mirror lines 222-248 and 321-333).
67-91:⚠️ Potential issue | 🟠 MajorLimit=0 “fetch all” can drop DB/file rows.
If underlying stores treat
limit=0asLIMIT 0, composite lists will be empty for that store and merged results will be incomplete. This affectsGetResourceServerListand similarlyGetResourceList,GetResourceListByParent, andGetActionList.Please verify how list methods interpret
limit=0and, if needed, switch to a real fetch‑all strategy (e.g., count then list, or a ListAll helper).#!/bin/bash # Verify list implementations interpret limit=0 as fetch-all. rg -n --type=go -C3 'GetResource(Server|)List\(' rg -n --type=go -C3 'GetResourceListByParent\(' rg -n --type=go -C3 'GetActionList\('🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/composite_store.go` around lines 67 - 91, The composite GetResourceServerList passes limit=0 to underlying stores which may be interpreted as SQL LIMIT 0 and return no rows; update compositeResourceStore.GetResourceServerList to treat limit==0 as "fetch all" by first determining the total rows from each store (e.g., call a CountResourceServers/CountResources/CountActions helper or use a ListAll helper) and then call dbStore.GetResourceServerList(ctx, totalDB, 0) and fileStore.GetResourceServerList(ctx, totalFile, 0) (or otherwise call their ListAll variants) before merging via mergeAndDeduplicateResourceServers; apply the same pattern to GetResourceList, GetResourceListByParent, and GetActionList so they never forward limit=0 directly to underlying stores.backend/internal/resource/file_based_store.go (4)
331-334:IsActionExistswallows all errors fromGetAction, masking potential storage failures.Should differentiate between
errActionNotFound(→false, nil) and unexpected errors (→false, err).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 331 - 334, The IsActionExist method currently ignores errors from f.GetAction; change it so it calls f.GetAction and returns (true, nil) when err == nil, returns (false, nil) when errors.Is(err, errActionNotFound), and returns (false, err) for any other unexpected error; reference the IsActionExist method on fileBasedResourceStore, call f.GetAction as before, and use errors.Is with errActionNotFound to distinguish the not-found case.
154-156:IsResourceServerDeclarativeunconditionally returnstruewithout checking existence.This means queries for non-existent IDs will get "immutable" errors instead of "not found" errors in composite mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 154 - 156, The IsResourceServerDeclarative method currently always returns true; change it to first check whether the resource server with the given id exists and only return true when it exists and is declarative, otherwise return false (or propagate a not-found result) so missing IDs produce "not found" instead of "immutable" errors; update fileBasedResourceStore.IsResourceServerDeclarative to perform an existence/lookup call (use the existing fileBasedResourceStore lookup/get method for resource servers) and base the boolean on that lookup result.
275-278:GetActionListandGetActionListCountare also stubs that will make declarative actions invisible in list queries.Same concern as the resource list stubs — these return empty results even though
getActionsByServerAndResourceexists and can retrieve the data.Also applies to: 319-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 275 - 278, GetActionList and GetActionListCount are stubs that return empty results; replace them with implementations that call the existing helper getActionsByServerAndResource to fetch declarative actions. In GetActionList use resServerInternalID and optional resInternalID with limit and offset to retrieve and return the slice of Action and error from getActionsByServerAndResource (or apply paging after retrieving results if the helper doesn't accept limit/offset). Implement GetActionListCount to return the total count for the same server/resource pair (call getActionsByServerAndResource and return len(results) or call a count variant if available). Ensure both functions propagate errors from the helper instead of swallowing them.
194-214: Resource list/count methods are stubs returning empty results, making file-based resources invisible in list queries.These stub implementations will cause declarative resources to be omitted from composite store list operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 194 - 214, The four stub methods GetResourceList, GetResourceListByParent, GetResourceListCount, and GetResourceListCountByParent currently return empty results; implement them to read and filter resources from the file-based store backing f (use the store's existing loading/parsing helpers or directory scanning logic), filter by the provided resServerInternalID and optional parentInternalID, apply pagination (limit/offset) in GetResourceList and GetResourceListByParent, and compute counts in GetResourceListCount and GetResourceListCountByParent; ensure you propagate context and errors (return non-nil errors on I/O/parse failures) and reuse existing helpers on fileBasedResourceStore to avoid duplicating parsing logic.
🧹 Nitpick comments (2)
backend/internal/resource/file_based_store.go (2)
128-146: Unchecked type assertions inGetByFieldcallbacks can panic on corrupted data.The callbacks at lines 130 and 140 use
d.(*ResourceServerProcessedDTO)without the comma-ok pattern. If the stored data is not the expected type, this panics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 128 - 146, The callbacks passed to GenericFileBasedStore.GetByField in CheckResourceServerNameExists and CheckResourceServerIdentifierExists perform unchecked type assertions (d.(*ResourceServerProcessedDTO)) and can panic on corrupted data; update both callbacks to use the comma-ok form (v, ok := d.(*ResourceServerProcessedDTO)) and handle the false case safely (e.g., return "" or a safe sentinel so GetByField won't panic), optionally logging the mismatch or returning an error from the outer function if you want to surface corrupted entries.
58-68:GetResourceServermasks all store errors as "not found".Line 61 replaces the original error from
GenericFileBasedStore.GetwitherrResourceServerNotFound. If the store returns a data-corruption or I/O error, it gets silently swallowed, making debugging harder.Proposed improvement
func (f *fileBasedResourceStore) GetResourceServer(ctx context.Context, id string) (int, ResourceServer, error) { data, err := f.GenericFileBasedStore.Get(id) if err != nil { - return 0, ResourceServer{}, errResourceServerNotFound + return 0, ResourceServer{}, fmt.Errorf("%w: %v", errResourceServerNotFound, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 58 - 68, The GetResourceServer function currently replaces any error from GenericFileBasedStore.Get with errResourceServerNotFound; change it to preserve and return the original error (or wrap it with context) so non-not-found errors (I/O, corruption) aren't swallowed: in fileBasedResourceStore.GetResourceServer, when GenericFileBasedStore.Get returns a non-nil err, return that err (e.g. return 0, ResourceServer{}, err or return 0, ResourceServer{}, fmt.Errorf("getting resource_server %s: %w", id, err)) instead of errResourceServerNotFound; keep the existing type-assertion handling (declarativeresource.LogTypeAssertionError and returning a data-corrupted error) as-is or wrap it for context.
🤖 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/resource/file_based_store.go`:
- Around line 103-113: The slice bounds panic happens when offset is negative
before the pagination slice servers[start:end]; guard against negative and
out-of-range pagination by clamping offset to a minimum of 0 and ensuring limit
is non-negative, e.g. compute start := max(0, offset) and end := start + max(0,
limit) then cap end to len(servers) and return an empty slice if start >=
len(servers); update the pagination logic around the start, end, servers, offset
and limit variables in file_based_store.go accordingly.
- Around line 47-50: The Create method in fileBasedResourceStore uses an
unchecked type assertion data.(*ResourceServerProcessedDTO) which will panic on
wrong input; change Create (fileBasedResourceStore.Create) to safely assert the
type (use the comma-ok form or a type switch) and return a descriptive error if
the assertion fails, and only call f.GenericFileBasedStore.Create(id,
rsProcessed) when the type is valid so callers receive an error instead of a
panic.
In `@backend/internal/resource/model.go`:
- Around line 196-221: The DTO types are exported but only used internally;
rename ActionDTO, ResourceDTO, ResourceServerDTO, ResourceServerProcessedDTO,
ResourceProcessedDTO, and ActionProcessedDTO to unexported versions (actionDTO,
resourceDTO, resourceServerDTO, resourceServerProcessedDTO,
resourceProcessedDTO, actionProcessedDTO) and update all internal
references/usages within the package (struct literals, function params/returns,
type assertions, unmarshalling tags remain unchanged) so the package API only
exposes intended exported types and service interfaces.
---
Duplicate comments:
In `@backend/internal/resource/composite_store_test.go`:
- Around line 201-211: The test TestGetResourceServerListCount_SumsBothStores
exposes a mismatch: list APIs deduplicate (DB wins) but
GetResourceServerListCount simply sums both stores, causing incorrect
pagination; update the composite implementation of GetResourceServerListCount
(and corresponding tests such as the one around lines 386-396) to compute a
deduplicated count by merging identifiers from DB and file stores and counting
unique server IDs (ensuring DB entries take precedence when resolving
duplicates), or alternatively update the test to explicitly assert and document
the current non-deduplicating behavior—locate the
compositeStore.GetResourceServerListCount implementation and the tests
TestGetResourceServerListCount_SumsBothStores to apply the chosen fix.
- Around line 530-540: Rename the test TestValidatePermissions_ChecksBothStores
to reflect DB-only delegation (e.g., TestValidatePermissions_DelegatesToDBOnly)
and add an assertion that the file store mock's ValidatePermissions method was
not invoked (use s.fileStoreMock.AssertNotCalled/AssertNotCalled with the
"ValidatePermissions" method and appropriate args) in addition to the existing
s.dbStoreMock expectations and result assertions so the test documents and
enforces that only the DB store is used.
In `@backend/internal/resource/composite_store.go`:
- Around line 94-105: The current GetResourceServerListCount implementation
simply adds dbStore.GetResourceServerListCount and
fileStore.GetResourceServerListCount which double-counts IDs present in both
stores; change it to use the same merge/dedup logic as the list path: retrieve
the underlying resource lists (or ID lists) from dbStore and fileStore and
compute the unique count by deduping on the resource ID (i.e. use the same merge
function used by ListResourceServers or the composite merge helper) instead of
summing counts; apply the same fix to the other analogous count functions
referenced in the comment (the other composite count methods that mirror lines
222-248 and 321-333).
- Around line 67-91: The composite GetResourceServerList passes limit=0 to
underlying stores which may be interpreted as SQL LIMIT 0 and return no rows;
update compositeResourceStore.GetResourceServerList to treat limit==0 as "fetch
all" by first determining the total rows from each store (e.g., call a
CountResourceServers/CountResources/CountActions helper or use a ListAll helper)
and then call dbStore.GetResourceServerList(ctx, totalDB, 0) and
fileStore.GetResourceServerList(ctx, totalFile, 0) (or otherwise call their
ListAll variants) before merging via mergeAndDeduplicateResourceServers; apply
the same pattern to GetResourceList, GetResourceListByParent, and GetActionList
so they never forward limit=0 directly to underlying stores.
In `@backend/internal/resource/declarative_resource.go`:
- Around line 39-55: ResourceServerExporter and NewResourceServerExporterForTest
are exported but should be unexported internal details; rename
ResourceServerExporter to resourceServerExporter and
NewResourceServerExporterForTest to newResourceServerExporterForTest (keep
existing newResourceServerExporter as the internal constructor), update
references and tests to use the unexported names, and move any tests that need
access into the same package (non-*_test external package) so they can call the
unexported symbols; ensure testing.Testing() guard remains intact in the
test-only factory.
- Around line 300-304: When building resourceHandleMap from rsDTO.Resources
(ResourceDTO.Handle), detect duplicate handles instead of silently overwriting:
before assigning resourceHandleMap[rsDTO.Resources[i].Handle] =
&rsDTO.Resources[i], check whether that key already exists and if so surface an
error (or collect and return all duplicate handles) so parent resolution fails
fast; update the function that constructs resourceHandleMap to return an error
when duplicates are found and include the conflicting Handle and the involved
ResourceDTO identifiers in the error message.
- Around line 150-155: The code calls e.service.GetActionList(ctx, id, nil,
1000, 0) to fetch root‑level (resourceID == nil) actions but discards the
returned list; update the code to capture the returned actions (e.g., assign the
first return to a variable like rootActions, check for err) and merge/append
those rootActions into the export/output data structure used by the surrounding
function (the same place where per‑resource actions are collected), ensuring
root‑level actions are included in the final export; reference the GetActionList
call and the variable that accumulates actions in this function (e.g.,
rootActions and the export actions slice/map).
- Around line 67-81: GetAllResourceIDs currently calls
e.service.GetResourceServerList once with a fixed limit (1000) which can miss
servers; update GetAllResourceIDs to page through results until all are returned
by repeatedly calling e.service.GetResourceServerList (using limit and advancing
offset/page token as supported) and accumulating server IDs from
servers.ResourceServers while still skipping declarative servers via
e.service.IsResourceServerDeclarative(server.ID); ensure loop exits when fewer
results than limit (or when next-page token is empty) and return the full ids
slice.
- Around line 105-127: The export currently fetches resources and actions with a
fixed limit of 1000 which truncates large exports; update the logic around
e.service.GetResourceList and e.service.GetActionList in declarative_resource.go
to page through results until all items are retrieved (e.g., loop incrementing
offset or using a next-token) and merge each page into the resource list/map
(resourceMap) and into each resource's actions collection (when calling
GetActionList with &res.ID) rather than relying on a single call with
limit=1000; ensure you stop when a page returns fewer than the requested limit
or when the service indicates no more pages.
- Around line 283-288: In processResourceServerDTO, avoid hard-coding the
delimiter ":"; instead obtain the default from the runtime config (use
getDefaultDelimiter() or ThunderRuntime in internal/system/config) and fall back
only if that call returns empty; update the delimiter assignment (variable
delimiter in processResourceServerDTO) to call the runtime config helper and use
its value so exports match runtime behavior.
- Around line 111-147: resourceMap is keyed by handle which is only unique per
parent and can overwrite siblings; change the map to be keyed by resource ID
(e.g., map[<IDType>]*ResourceDTO) and store the handle on the DTO for lookups.
Update the declaration of resourceMap, the assignment currently using
resourceMap[res.Handle] = &resourceDTO, and the parent resolution logic (the
loop that compares parentRes.ID == *res.Parent) to look up parent by ID (e.g.,
parentDTO := resourceMap[*res.Parent] or maintain a separate parentID→handle
map) so parent relationships use IDs instead of handles; ensure GetActionList
usage and ActionDTO population remain unchanged.
- Around line 315-342: processResourceDTO currently accepts empty/invalid
resource and action handles; update it to validate resDTO.Name and resDTO.Handle
and each actionDTO.Name and actionDTO.Handle using the existing shared
validators (e.g., the package's ValidateHandle/ValidateName or equivalent used
elsewhere) before building permission or appending ActionProcessedDTO; if any
validation fails, fail fast by logging the error and returning nil (or propagate
an error) so invalid resources/actions are not processed; touch symbols:
processResourceDTO, resDTO, actionDTO, buildPermissionString,
ResourceProcessedDTO, ActionProcessedDTO.
In `@backend/internal/resource/file_based_store_test.go`:
- Around line 47-54: TearDownTest is supposed to clear the singleton EntityStore
but currently does nothing; update TearDownTest in
FileBasedResourceStoreTestSuite to call the test-only reset/clear method on the
underlying singleton (accessed via s.store.(*fileBasedResourceStore) -> its
GenericFileBasedStore/EntityStore) so state is wiped between tests, or add a new
package-private Reset/Clear method on GenericFileBasedStore/EntityStore and
invoke that from TearDownTest to ensure no data leaks between tests.
In `@backend/internal/resource/file_based_store.go`:
- Around line 331-334: The IsActionExist method currently ignores errors from
f.GetAction; change it so it calls f.GetAction and returns (true, nil) when err
== nil, returns (false, nil) when errors.Is(err, errActionNotFound), and returns
(false, err) for any other unexpected error; reference the IsActionExist method
on fileBasedResourceStore, call f.GetAction as before, and use errors.Is with
errActionNotFound to distinguish the not-found case.
- Around line 154-156: The IsResourceServerDeclarative method currently always
returns true; change it to first check whether the resource server with the
given id exists and only return true when it exists and is declarative,
otherwise return false (or propagate a not-found result) so missing IDs produce
"not found" instead of "immutable" errors; update
fileBasedResourceStore.IsResourceServerDeclarative to perform an
existence/lookup call (use the existing fileBasedResourceStore lookup/get method
for resource servers) and base the boolean on that lookup result.
- Around line 275-278: GetActionList and GetActionListCount are stubs that
return empty results; replace them with implementations that call the existing
helper getActionsByServerAndResource to fetch declarative actions. In
GetActionList use resServerInternalID and optional resInternalID with limit and
offset to retrieve and return the slice of Action and error from
getActionsByServerAndResource (or apply paging after retrieving results if the
helper doesn't accept limit/offset). Implement GetActionListCount to return the
total count for the same server/resource pair (call
getActionsByServerAndResource and return len(results) or call a count variant if
available). Ensure both functions propagate errors from the helper instead of
swallowing them.
- Around line 194-214: The four stub methods GetResourceList,
GetResourceListByParent, GetResourceListCount, and GetResourceListCountByParent
currently return empty results; implement them to read and filter resources from
the file-based store backing f (use the store's existing loading/parsing helpers
or directory scanning logic), filter by the provided resServerInternalID and
optional parentInternalID, apply pagination (limit/offset) in GetResourceList
and GetResourceListByParent, and compute counts in GetResourceListCount and
GetResourceListCountByParent; ensure you propagate context and errors (return
non-nil errors on I/O/parse failures) and reuse existing helpers on
fileBasedResourceStore to avoid duplicating parsing logic.
---
Nitpick comments:
In `@backend/internal/resource/file_based_store.go`:
- Around line 128-146: The callbacks passed to GenericFileBasedStore.GetByField
in CheckResourceServerNameExists and CheckResourceServerIdentifierExists perform
unchecked type assertions (d.(*ResourceServerProcessedDTO)) and can panic on
corrupted data; update both callbacks to use the comma-ok form (v, ok :=
d.(*ResourceServerProcessedDTO)) and handle the false case safely (e.g., return
"" or a safe sentinel so GetByField won't panic), optionally logging the
mismatch or returning an error from the outer function if you want to surface
corrupted entries.
- Around line 58-68: The GetResourceServer function currently replaces any error
from GenericFileBasedStore.Get with errResourceServerNotFound; change it to
preserve and return the original error (or wrap it with context) so
non-not-found errors (I/O, corruption) aren't swallowed: in
fileBasedResourceStore.GetResourceServer, when GenericFileBasedStore.Get returns
a non-nil err, return that err (e.g. return 0, ResourceServer{}, err or return
0, ResourceServer{}, fmt.Errorf("getting resource_server %s: %w", id, err))
instead of errResourceServerNotFound; keep the existing type-assertion handling
(declarativeresource.LogTypeAssertionError and returning a data-corrupted error)
as-is or wrap it for context.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
backend/cmd/server/servicemanager.gobackend/internal/resource/ResourceServiceInterface_mock_test.gobackend/internal/resource/composite_store.gobackend/internal/resource/composite_store_test.gobackend/internal/resource/config.gobackend/internal/resource/config_test.gobackend/internal/resource/declarative_resource.gobackend/internal/resource/declarative_resource_test.gobackend/internal/resource/error_constants.gobackend/internal/resource/file_based_store.gobackend/internal/resource/file_based_store_test.gobackend/internal/resource/init.gobackend/internal/resource/init_test.gobackend/internal/resource/model.gobackend/internal/resource/resourceStoreInterface_mock_test.gobackend/internal/resource/service.gobackend/internal/resource/service_test.gobackend/internal/resource/store.gobackend/internal/system/config/config.gobackend/internal/system/declarative_resource/entity/entity_store.gobackend/tests/mocks/resourcemock/ResourceServiceInterface_mock.gobackend/tests/mocks/resourcemock/resourceStoreInterface_mock.go
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/internal/resource/config.go
- backend/internal/system/declarative_resource/entity/entity_store.go
- backend/internal/resource/store.go
- backend/internal/resource/config_test.go
rajithacharith
force-pushed
the
composite-resource-server
branch
from
14a2a2a to
5cf1141
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
backend/internal/resource/composite_store_test.go (2)
530-540:⚠️ Potential issue | 🟡 MinorTest name says “ChecksBothStores” but only DB is exercised.
Rename to reflect delegation and assert the file store isn’t called.🔧 Suggested rename and explicit non-call assertion
-func (s *CompositeResourceStoreTestSuite) TestValidatePermissions_ChecksBothStores() { +func (s *CompositeResourceStoreTestSuite) TestValidatePermissions_DelegatesToDB() { permissions := []string{"perm1", "perm2", "perm3"} s.dbStoreMock.On("ValidatePermissions", s.ctx, 1, permissions).Return([]string{}, nil) invalid, err := s.compositeStore.ValidatePermissions(s.ctx, 1, permissions) assert.NoError(s.T(), err) assert.Len(s.T(), invalid, 0) s.dbStoreMock.AssertExpectations(s.T()) + s.fileStoreMock.AssertNotCalled(s.T(), "ValidatePermissions") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/composite_store_test.go` around lines 530 - 540, Rename the test TestValidatePermissions_ChecksBothStores to reflect that only the DB store is used (e.g., TestValidatePermissions_DelegatesToDBOnly) and add an explicit assertion that the file store was not called: after calling compositeStore.ValidatePermissions(s.ctx, 1, permissions) assert no error and zero invalids as before, then call fileStoreMock.AssertNotCalled(s.T(), "ValidatePermissions", mock.Anything, mock.Anything, mock.Anything) (or the equivalent AssertNotCalled pattern) to prove fileStoreMock was not invoked; keep dbStoreMock expectation on ValidatePermissions as the delegated call.
201-211:⚠️ Potential issue | 🟠 MajorCounts are summed while lists are de-duplicated.
These tests encode a mismatch that breaks pagination when IDs overlap across stores. Consider deduping counts (or explicitly documenting this limitation) to align totals with list behavior.Also applies to: 386-396
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/composite_store_test.go` around lines 201 - 211, The test assumes counts from db and file stores can simply be summed, but lists are de-duplicated which breaks pagination; update the compositeStore behavior instead of the test: change compositeStore.GetResourceServerListCount to deduplicate by collecting IDs from both underlying calls (use the same internal call that returns resource IDs / GetResourceServerList from dbStore and fileStore, or alter GetResourceServerListCount to call those list methods), merge into a set to remove duplicates, and return the deduped length (ensure tests like TestGetResourceServerListCount_SumsBothStores and the similar test at lines 386-396 are updated to mock underlying list/ID returns rather than raw counts or adjust their expectations to the deduped total).backend/internal/resource/composite_store.go (2)
94-106:⚠️ Potential issue | 🟠 MajorCounts should dedupe IDs to match merged lists.
Lists are de-duplicated with DB precedence, but counts are summed, which can overstate totals when IDs overlap and break pagination. Consider deduping counts using the same merge logic.💡 Example fix for ResourceServer count (apply similarly elsewhere)
- return dbCount + fileCount, nil + dbServers, err := c.dbStore.GetResourceServerList(ctx, dbCount, 0) + if err != nil { + return 0, err + } + fileServers, err := c.fileStore.GetResourceServerList(ctx, fileCount, 0) + if err != nil { + return 0, err + } + return len(mergeAndDeduplicateResourceServers(dbServers, fileServers)), nilAlso applies to: 222-234, 236-248, 321-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/composite_store.go` around lines 94 - 106, The function compositeResourceStore.GetResourceServerListCount simply sums dbStore.GetResourceServerListCount and fileStore.GetResourceServerListCount which double-counts overlapping IDs; instead, fetch the actual ID lists (using the same merge/dedupe logic you use for list results with DB precedence), build a deduplicated set of IDs honoring DB precedence, and return its size; apply the same change pattern to the other count functions that currently sum counts (referenced methods: dbStore.GetResourceServerListCount, fileStore.GetResourceServerListCount and the compositeResourceStore counterparts mentioned in the review).
67-75:⚠️ Potential issue | 🟠 MajorAvoid
limit=0fetch-all calls—DB-backed rows can disappear.
These list methods fetch “all” withlimit=0, which many SQL-backed implementations treat as zero rows. If the DB store passes that through to SQL, composite lists will omit DB data. Prefer a count+list-all approach or a dedicated list-all helper, and apply the same pattern across all list methods.💡 Example fix for one list (apply similarly elsewhere)
- dbServers, err := c.dbStore.GetResourceServerList(ctx, 0, 0) + dbCount, err := c.dbStore.GetResourceServerListCount(ctx) + if err != nil { + return nil, err + } + dbServers, err := c.dbStore.GetResourceServerList(ctx, dbCount, 0) if err != nil { return nil, err } - fileServers, err := c.fileStore.GetResourceServerList(ctx, 0, 0) + fileCount, err := c.fileStore.GetResourceServerListCount(ctx) + if err != nil { + return nil, err + } + fileServers, err := c.fileStore.GetResourceServerList(ctx, fileCount, 0) if err != nil { return nil, err }#!/bin/bash # Inspect list implementations for limit handling in resource stores. rg -n --type=go 'GetResourceServerList|GetResourceListByParent|GetResourceList\(|GetActionList\(' backend/internal/resource -g '*store*.go' rg -n --type=go 'LIMIT' backend/internal/resource -g '*store*.go'Also applies to: 166-174, 193-204, 294-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/composite_store.go` around lines 67 - 75, The compositeResourceStore methods (e.g., GetResourceServerList) currently call c.dbStore.GetResourceServerList(ctx, 0, 0) and c.fileStore.GetResourceServerList(ctx, 0, 0), which relies on callers interpreting limit=0 as “all” and can result in zero rows from SQL-backed stores; change these calls to use a safe “list-all” pattern: either add and call a dedicated ListAll helper on the underlying stores (e.g., ResourceStore.ListAllResourceServers(ctx)), or first call a Count method (e.g., dbStore.GetResourceServerCount) and then call GetResourceServerList with that count as the limit; apply the same fix to the other composite methods mentioned (the calls at the blocks around lines ~166-174, ~193-204, ~294-303 that use limit=0) so DB-backed implementations no longer receive limit=0.backend/internal/resource/file_based_store_test.go (1)
47-54:⚠️ Potential issue | 🟡 MinorTearDownTest still doesn’t clear the singleton store.
The teardown only type-asserts and leaves the store untouched, so state can leak between tests. Consider a test-only reset/clear on the underlying file store and call it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store_test.go` around lines 47 - 54, TearDownTest currently only type-asserts s.store and doesn't clear the shared singleton, so tests leak state; add or use a test-only clear/reset method on the underlying singleton GenericFileBasedStore (e.g., Reset or Clear on fileBasedResourceStore/GenericFileBasedStore) and invoke it from FileBasedResourceStoreTestSuite.TearDownTest when s.store is a *fileBasedResourceStore so the file-backed singleton is emptied between tests.backend/internal/resource/file_based_store.go (5)
47-50:⚠️ Potential issue | 🔴 CriticalUnchecked type assertion in
Createstill panics on wrong input.This was flagged in a previous review — the fix using comma-ok safe assertion has not yet been applied.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 47 - 50, The Create method on fileBasedResourceStore currently uses a direct type assertion on data to *ResourceServerProcessedDTO which can panic; change it to a safe comma-ok assertion (e.g., v, ok := data.(*ResourceServerProcessedDTO)), return a clear error if !ok (or if v == nil), and then call f.GenericFileBasedStore.Create(id, v) only when the assertion succeeds; update function Create and reference ResourceServerProcessedDTO and GenericFileBasedStore.Create accordingly.
331-334:⚠️ Potential issue | 🟠 Major
IsActionExiststill swallows all errors — storage failures masked as "not found".Past review flagged this; the fix (distinguishing
errActionNotFoundfrom other errors) has not been applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 331 - 334, The IsActionExist function masks all errors by returning (err == nil, nil); change it to distinguish not-found vs other failures: call GetAction as currently done, if err == nil return (true, nil); if errors.Is(err, errActionNotFound) (or err == errActionNotFound if that constant is used) return (false, nil); otherwise return (false, err) so storage/IO errors propagate. Update imports to include "errors" if using errors.Is. Keep function name IsActionExist and the GetAction call unchanged.
103-113:⚠️ Potential issue | 🟡 MinorNegative
offsetstill causes a slice bounds panic.Past review flagged this; the guard (
start = max(0, offset)) has not been applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 103 - 113, The slice panic happens because negative offset isn't clamped before slicing; in the pagination logic inside file_based_store.go set start to a non-negative value (e.g., replace start := offset with start := offset; if start < 0 { start = 0 }) so start is max(0, offset) before computing end and slicing servers[start:end]; also ensure end is computed from the clamped start (end := start + limit) and keep the existing bounds checks for end > len(servers).
194-213:⚠️ Potential issue | 🟠 MajorResource/action list and count stubs still return empty — declarative resources are invisible in composite list queries.
This was raised in a prior review. The stubs return
[]Resource{}, nil/0, nil, making file-based resources invisible whenever the composite store merges list results. If this is an intentional design limitation, it should be documented with a// TODOor a clear comment explaining why declarative resources are excluded from list operations. Otherwise, consider adding anerrNotSupportedsentinel so callers can distinguish "no items" from "not implemented."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 194 - 213, The current fileBasedResourceStore list/count methods (GetResourceList, GetResourceListByParent, GetResourceListCount, GetResourceListCountByParent) return empty results which hides declarative resources from composite merges; either document this limitation with a clear TODO/comment explaining why file-based declarative resources are excluded, or (recommended) introduce and return a sentinel error (e.g., errNotSupported) from these four methods so callers can distinguish “not supported” from “no items” — add the errNotSupported declaration near other store errors and update these methods to return that error instead of nil, and update any callers/tests that expect empty results to handle errNotSupported appropriately.
154-156:⚠️ Potential issue | 🟠 Major
IsResourceServerDeclarativestill always returnstrue.This was flagged in a previous review — non-existent IDs are treated as declarative, causing delete/update to return "immutable" rather than "not-found". Fix: check existence in the generic store first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 154 - 156, IsResourceServerDeclarative currently always returns true; change it to first check existence in the underlying generic store (use f.genericStore.Exists(id) or f.genericStore.Get(id) and treat absence as not-found) and return false for missing IDs, and only apply the existing declarative-detection logic for IDs that actually exist. Update the fileBasedResourceStore.IsResourceServerDeclarative implementation to call the generic store existence check (f.genericStore) before returning the declarative result.
🧹 Nitpick comments (2)
backend/internal/resource/config_test.go (1)
41-55: Reset ThunderRuntime in the suite lifecycle to avoid cross-test leakage.Because InitializeThunderRuntime uses a singleton, initializing it once in SetupSuite without a TearDownSuite can leak state to other tests in the same package. Resetting before/after the suite keeps isolation predictable.
♻️ Suggested change
func (s *ResourceConfigTestSuite) SetupSuite() { + // Ensure a clean runtime for this suite + config.ResetThunderRuntime() // Initialize runtime once for all tests in the suite testConfig := &config.Config{} err := config.InitializeThunderRuntime("/tmp/test", testConfig) if err != nil { s.Fail("Failed to initialize runtime", err) } } + +func (s *ResourceConfigTestSuite) TearDownSuite() { + config.ResetThunderRuntime() +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/config_test.go` around lines 41 - 55, The suite currently calls config.InitializeThunderRuntime once in SetupSuite which leaves the singleton runtime shared across tests; add a TearDownSuite method to reset the singleton after the suite (or add a ResetThunderRuntime function in the config package and call it from TearDownSuite) so state won't leak; alternatively, move initialization into SetupTest or have SetupTest call config.ResetThunderRuntime() before calling config.InitializeThunderRuntime to ensure a fresh runtime for each test; reference SetupSuite, TearDownSuite, SetupTest, config.InitializeThunderRuntime and config.GetThunderRuntime when making the change.backend/internal/resource/file_based_store.go (1)
275-321:getActionsByServerAndResourceis defined but never called —GetActionList/GetActionListCountremain empty stubs.The helper correctly accepts
serverID string(the right abstraction for a file-based store) and can fetch actions for a specific server/resource combination, but neitherGetActionListnorGetActionListCountcalls it. Until a mapping fromresServerInternalID→serverIDis available (e.g., via a lookup in the composite store), this helper is unreachable dead code. Either wire it in once the ID mapping is resolved, or remove it and add a// TODOcomment documenting what is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 275 - 321, getActionsByServerAndResource is implemented but never used, leaving GetActionList and GetActionListCount as empty stubs; either wire the helper into those methods once you resolve the mapping from resServerInternalID→serverID or remove the helper and document the missing dependency. Update GetActionList and GetActionListCount to call getActionsByServerAndResource (or a wrapper that converts resServerInternalID/resInternalID to the serverID/resourceHandle expected) and return the results/count, or if the mapping is not yet available, delete getActionsByServerAndResource and add a clear TODO in its place explaining that a lookup from resServerInternalID to serverID is required by the file-based store before wiring. Ensure references to getActionsByServerAndResource, GetActionList, and GetActionListCount remain consistent.
🤖 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/resource/declarative_resource_test.go`:
- Around line 238-352: The tests TestParseToResourceServerDTO,
TestParseToResourceServerDTO_MissingID,
TestParseToResourceServerDTO_MissingName, and TestBuildPermissionString should
be converted from standalone functions into methods on the existing test suite
(e.g., DeclarativeResourceTestSuite) to follow the suite pattern; locate uses of
parseToResourceServerDTO and buildPermissionString and move the test bodies into
suite methods like func (s *DeclarativeResourceTestSuite)
TestParseToResourceServerDTO() and adjust assertions from assert.* to
s.Require.* or s.Assert.* as appropriate, ensuring shared fixtures
(resourceHandleMap, ResourceDTO) are either moved into the suite struct or
recreated in the method bodies so tests compile and run under the suite runner.
In `@backend/internal/resource/file_based_store.go`:
- Around line 246-273: GetAction currently ignores resServerInternalID and
resInternalID and returns on handle-only matches; change it to scope searches
and only accept the composite ID form. Specifically, in GetAction filter
rsProcessed by resServerInternalID (compare to the resource server's internal ID
field) and filter resProcessed by resInternalID (compare to the resource's
internal ID field) before iterating actions, remove/disable the branch that
returns on actionProcessed.Handle == id, and only return when the constructed
composite actionID (fmt.Sprintf("%s_%s_%s", rsProcessed.ID, resProcessed.Handle,
actionProcessed.Handle)) equals id; if no match is found return
errActionNotFound as before.
- Around line 164-192: GetResource currently ignores resServerInternalID and
matches raw handles which can return the wrong resource when multiple servers
define the same handle; update GetResource (in fileBasedResourceStore) to
restrict matching to the unambiguous composite form or to respect
resServerInternalID: if id contains the composite pattern "serverID_handle" only
accept matches where fmt.Sprintf("%s_%s", rsProcessed.ID, resProcessed.Handle)
== id, otherwise if resServerInternalID is provided use resServerInternalID to
find the specific ResourceServerProcessedDTO (compare against rsProcessed.ID or
its internal numeric ID mapping) and then match resProcessed.Handle == id;
remove the loose raw-handle-first branch so raw handles cannot silently collide.
In `@backend/internal/resource/service.go`:
- Around line 908-911: The branch that checks
rs.IsResourceServerDeclarative(resourceServerID) returns the
ErrorImmutableAction constant directly, leaving its %s placeholder unformatted
and skipping the usual debug logging; instead, construct and return a
ServiceError with the description formatted (e.g.
fmt.Sprintf(ErrorImmutableAction.Description, resourceServerID)) and log the
formatted message via the same debug logging used in other error paths so the
message is consistent with other returns (refer to ErrorImmutableAction,
rs.IsResourceServerDeclarative, and the UpdateAction/UpdateResourceServer
error-return pattern to mirror).
---
Duplicate comments:
In `@backend/internal/resource/composite_store_test.go`:
- Around line 530-540: Rename the test TestValidatePermissions_ChecksBothStores
to reflect that only the DB store is used (e.g.,
TestValidatePermissions_DelegatesToDBOnly) and add an explicit assertion that
the file store was not called: after calling
compositeStore.ValidatePermissions(s.ctx, 1, permissions) assert no error and
zero invalids as before, then call fileStoreMock.AssertNotCalled(s.T(),
"ValidatePermissions", mock.Anything, mock.Anything, mock.Anything) (or the
equivalent AssertNotCalled pattern) to prove fileStoreMock was not invoked; keep
dbStoreMock expectation on ValidatePermissions as the delegated call.
- Around line 201-211: The test assumes counts from db and file stores can
simply be summed, but lists are de-duplicated which breaks pagination; update
the compositeStore behavior instead of the test: change
compositeStore.GetResourceServerListCount to deduplicate by collecting IDs from
both underlying calls (use the same internal call that returns resource IDs /
GetResourceServerList from dbStore and fileStore, or alter
GetResourceServerListCount to call those list methods), merge into a set to
remove duplicates, and return the deduped length (ensure tests like
TestGetResourceServerListCount_SumsBothStores and the similar test at lines
386-396 are updated to mock underlying list/ID returns rather than raw counts or
adjust their expectations to the deduped total).
In `@backend/internal/resource/composite_store.go`:
- Around line 94-106: The function
compositeResourceStore.GetResourceServerListCount simply sums
dbStore.GetResourceServerListCount and fileStore.GetResourceServerListCount
which double-counts overlapping IDs; instead, fetch the actual ID lists (using
the same merge/dedupe logic you use for list results with DB precedence), build
a deduplicated set of IDs honoring DB precedence, and return its size; apply the
same change pattern to the other count functions that currently sum counts
(referenced methods: dbStore.GetResourceServerListCount,
fileStore.GetResourceServerListCount and the compositeResourceStore counterparts
mentioned in the review).
- Around line 67-75: The compositeResourceStore methods (e.g.,
GetResourceServerList) currently call c.dbStore.GetResourceServerList(ctx, 0, 0)
and c.fileStore.GetResourceServerList(ctx, 0, 0), which relies on callers
interpreting limit=0 as “all” and can result in zero rows from SQL-backed
stores; change these calls to use a safe “list-all” pattern: either add and call
a dedicated ListAll helper on the underlying stores (e.g.,
ResourceStore.ListAllResourceServers(ctx)), or first call a Count method (e.g.,
dbStore.GetResourceServerCount) and then call GetResourceServerList with that
count as the limit; apply the same fix to the other composite methods mentioned
(the calls at the blocks around lines ~166-174, ~193-204, ~294-303 that use
limit=0) so DB-backed implementations no longer receive limit=0.
In `@backend/internal/resource/file_based_store_test.go`:
- Around line 47-54: TearDownTest currently only type-asserts s.store and
doesn't clear the shared singleton, so tests leak state; add or use a test-only
clear/reset method on the underlying singleton GenericFileBasedStore (e.g.,
Reset or Clear on fileBasedResourceStore/GenericFileBasedStore) and invoke it
from FileBasedResourceStoreTestSuite.TearDownTest when s.store is a
*fileBasedResourceStore so the file-backed singleton is emptied between tests.
In `@backend/internal/resource/file_based_store.go`:
- Around line 47-50: The Create method on fileBasedResourceStore currently uses
a direct type assertion on data to *ResourceServerProcessedDTO which can panic;
change it to a safe comma-ok assertion (e.g., v, ok :=
data.(*ResourceServerProcessedDTO)), return a clear error if !ok (or if v ==
nil), and then call f.GenericFileBasedStore.Create(id, v) only when the
assertion succeeds; update function Create and reference
ResourceServerProcessedDTO and GenericFileBasedStore.Create accordingly.
- Around line 331-334: The IsActionExist function masks all errors by returning
(err == nil, nil); change it to distinguish not-found vs other failures: call
GetAction as currently done, if err == nil return (true, nil); if errors.Is(err,
errActionNotFound) (or err == errActionNotFound if that constant is used) return
(false, nil); otherwise return (false, err) so storage/IO errors propagate.
Update imports to include "errors" if using errors.Is. Keep function name
IsActionExist and the GetAction call unchanged.
- Around line 103-113: The slice panic happens because negative offset isn't
clamped before slicing; in the pagination logic inside file_based_store.go set
start to a non-negative value (e.g., replace start := offset with start :=
offset; if start < 0 { start = 0 }) so start is max(0, offset) before computing
end and slicing servers[start:end]; also ensure end is computed from the clamped
start (end := start + limit) and keep the existing bounds checks for end >
len(servers).
- Around line 194-213: The current fileBasedResourceStore list/count methods
(GetResourceList, GetResourceListByParent, GetResourceListCount,
GetResourceListCountByParent) return empty results which hides declarative
resources from composite merges; either document this limitation with a clear
TODO/comment explaining why file-based declarative resources are excluded, or
(recommended) introduce and return a sentinel error (e.g., errNotSupported) from
these four methods so callers can distinguish “not supported” from “no items” —
add the errNotSupported declaration near other store errors and update these
methods to return that error instead of nil, and update any callers/tests that
expect empty results to handle errNotSupported appropriately.
- Around line 154-156: IsResourceServerDeclarative currently always returns
true; change it to first check existence in the underlying generic store (use
f.genericStore.Exists(id) or f.genericStore.Get(id) and treat absence as
not-found) and return false for missing IDs, and only apply the existing
declarative-detection logic for IDs that actually exist. Update the
fileBasedResourceStore.IsResourceServerDeclarative implementation to call the
generic store existence check (f.genericStore) before returning the declarative
result.
---
Nitpick comments:
In `@backend/internal/resource/config_test.go`:
- Around line 41-55: The suite currently calls config.InitializeThunderRuntime
once in SetupSuite which leaves the singleton runtime shared across tests; add a
TearDownSuite method to reset the singleton after the suite (or add a
ResetThunderRuntime function in the config package and call it from
TearDownSuite) so state won't leak; alternatively, move initialization into
SetupTest or have SetupTest call config.ResetThunderRuntime() before calling
config.InitializeThunderRuntime to ensure a fresh runtime for each test;
reference SetupSuite, TearDownSuite, SetupTest, config.InitializeThunderRuntime
and config.GetThunderRuntime when making the change.
In `@backend/internal/resource/file_based_store.go`:
- Around line 275-321: getActionsByServerAndResource is implemented but never
used, leaving GetActionList and GetActionListCount as empty stubs; either wire
the helper into those methods once you resolve the mapping from
resServerInternalID→serverID or remove the helper and document the missing
dependency. Update GetActionList and GetActionListCount to call
getActionsByServerAndResource (or a wrapper that converts
resServerInternalID/resInternalID to the serverID/resourceHandle expected) and
return the results/count, or if the mapping is not yet available, delete
getActionsByServerAndResource and add a clear TODO in its place explaining that
a lookup from resServerInternalID to serverID is required by the file-based
store before wiring. Ensure references to getActionsByServerAndResource,
GetActionList, and GetActionListCount remain consistent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
backend/cmd/server/servicemanager.gobackend/internal/resource/ResourceServiceInterface_mock_test.gobackend/internal/resource/composite_store.gobackend/internal/resource/composite_store_test.gobackend/internal/resource/config.gobackend/internal/resource/config_test.gobackend/internal/resource/declarative_resource.gobackend/internal/resource/declarative_resource_test.gobackend/internal/resource/error_constants.gobackend/internal/resource/file_based_store.gobackend/internal/resource/file_based_store_test.gobackend/internal/resource/init.gobackend/internal/resource/init_test.gobackend/internal/resource/model.gobackend/internal/resource/resourceStoreInterface_mock_test.gobackend/internal/resource/service.gobackend/internal/resource/service_test.gobackend/internal/resource/store.gobackend/internal/system/config/config.gobackend/internal/system/declarative_resource/entity/entity_store.gobackend/tests/mocks/resourcemock/ResourceServiceInterface_mock.gobackend/tests/mocks/resourcemock/resourceStoreInterface_mock.go
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/internal/system/config/config.go
- backend/internal/resource/store.go
- backend/internal/resource/error_constants.go
- backend/internal/resource/declarative_resource.go
- backend/internal/resource/model.go
- backend/internal/system/declarative_resource/entity/entity_store.go
rajithacharith
force-pushed
the
composite-resource-server
branch
from
5cf1141 to
47fab61
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
backend/internal/resource/declarative_resource.go (1)
144-149:⚠️ Potential issue | 🟠 MajorServer-level actions are fetched but discarded.
Root actions (
resourceID == nil) are retrieved and then ignored, so the export misses server-level actions entirely. Please include them in the export payload (or explicitly document why they are excluded).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource.go` around lines 144 - 149, The code calls e.service.GetActionList(ctx, id, nil, 1000, 0) for root-level actions but discards the returned list; capture that return value (e.g., rootActions, err := e.service.GetActionList(...)) and merge/append those entries into the export payload's actions collection (the same slice you use for resource-scoped actions) so server-level actions (resourceID == nil) are included; keep the existing error handling and ensure duplicates are handled/filtered if necessary.backend/internal/resource/service.go (1)
908-911:⚠️ Potential issue | 🟠 Major
UpdateActionreturns the raw error constant with an unformatted%splaceholder.Unlike all other declarative checks in this file (e.g.,
DeleteActionat lines 985-998,UpdateResourceat 582-595),UpdateActionreturns&ErrorImmutableActiondirectly. SinceErrorImmutableAction.ErrorDescriptioncontains a%splaceholder, the client will see a literal%sin the response. Addfmt.Sprintfformatting and a debug log to match the pattern used elsewhere.🐛 Proposed fix
// Check if resource server is declarative (immutable) if rs.IsResourceServerDeclarative(resourceServerID) { - return nil, &ErrorImmutableAction + rs.logger.Debug( + "Cannot modify action in declarative resource server", + log.String("resource_server_id", resourceServerID), + ) + errorMsg := fmt.Sprintf(ErrorImmutableAction.ErrorDescription, id) + return nil, &serviceerror.ServiceError{ + Type: ErrorImmutableAction.Type, + Code: ErrorImmutableAction.Code, + Error: ErrorImmutableAction.Error, + ErrorDescription: errorMsg, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/service.go` around lines 908 - 911, UpdateAction currently returns &ErrorImmutableAction directly which leaves the %s placeholder unformatted; change it to format ErrorImmutableAction.ErrorDescription with fmt.Sprintf (passing the resourceServerID or appropriate name) and return a copy like &ErrorImmutableAction with the formatted ErrorDescription, and add a debug log (using the same logger used elsewhere) that records the immutable-action attempt and resourceServerID; locate the check around IsResourceServerDeclarative in UpdateAction and mirror the pattern used in DeleteAction and UpdateResource for formatting and logging.backend/internal/resource/declarative_resource_test.go (1)
240-359: Standalone test functions should be part of the suite.
TestParseToResourceServer,TestParseToResourceServer_MissingID,TestParseToResourceServer_MissingName, andTestBuildPermissionStringare standalone functions that bypass the suite pattern. Consider converting them to suite methods for consistency. As per coding guidelines, "Usestretchr/testifyfor tests and follow the test suite pattern".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource_test.go` around lines 240 - 359, The four standalone tests (TestParseToResourceServer, TestParseToResourceServer_MissingID, TestParseToResourceServer_MissingName, TestBuildPermissionString) must be converted into methods on the test suite (e.g., func (s *DeclarativeResourceTestSuite) TestParseToResourceServer(...) ) so they run under the stretchr/testify suite pattern; update each function signature to be a suite method, replace t.* assertions with the suite equivalents (s.Require(), s.Assert(), or s.Equal()/s.NoError() on the suite), and ensure the suite's TestMain or Test... entry registers/runs the suite; keep references to parseToResourceServer and buildPermissionString unchanged but invoke them from the new suite methods.
🧹 Nitpick comments (1)
backend/internal/resource/service.go (1)
281-291: Declarative check placement differs betweenUpdateResourceServerand other mutating methods.In
UpdateResourceServer, the declarative check (line 282) occurs after fetching the existing resource server (line 271), whereas inDeleteResourceServer,UpdateResource,DeleteResource,UpdateAction, andDeleteAction, the declarative check is the first guard. This meansUpdateResourceServerreturns a 404 for a nonexistent declarative ID, whileDeleteResourceServerreturns the immutable error. Consider moving the declarative check before the existence fetch for consistency, or document the intentional difference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/service.go` around lines 281 - 291, UpdateResourceServer performs the declarative check (rs.IsResourceServerDeclarative(id)) after fetching the existing resource server which causes a 404 for non-existent declarative IDs; to match DeleteResourceServer, UpdateResource, DeleteResource, UpdateAction, and DeleteAction, move the rs.IsResourceServerDeclarative(id) guard to the top of UpdateResourceServer (before fetching existing data) so it returns the immutable ServiceError when the ID is declarative; update the error-return path in UpdateResourceServer to construct and return the same serviceerror.ServiceError (Type/Code/Error/ErrorDescription) as used elsewhere when the check fails.
🤖 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/resource/declarative_resource.go`:
- Around line 105-157: The export currently never sets Resource.ParentHandle so
child resources lose parent links; when iterating resources from GetResourceList
populate each exported resource's ParentHandle from the source resource's parent
ID using the id->handle map (idToHandleMap[res.Parent] or res.ParentID) and
store it on the Resource before inserting into resourceIDMap, then when building
the hierarchy use the source res.Parent (or res.ParentID) to detect roots
(nil/empty means root) and to attach children to their parent by looking up the
parent's handle via idToHandleMap and resourceIDMap (use resourceIDMap[parentID]
or the mapped parent handle) so children are preserved under their parents
instead of being flattened.
In `@backend/internal/resource/model.go`:
- Around line 162-167: Update the header comment to accurately describe the
struct tag strategy: state that the models primarily use yaml tags for parsing,
but many fields also include json tags (e.g., in Action, Resource,
ResourceServer) for service/API use, and computed/internal fields are marked
with json:"-" and yaml:"-" as appropriate; replace the incorrect "No json tags"
sentence with this corrected description so the comment matches the actual tags
on Action, Resource, and ResourceServer.
In `@backend/internal/resource/service.go`:
- Around line 410-412: Remove the stale concatenated comment fragment next to
the CreateResource documentation so the comment reads cleanly; locate the
comment block that documents CreateResource (the line containing "CreateResource
creates a new resource under a resource server") and delete the trailing "//
Resource Methods" text that was accidentally appended, leaving only the proper
doc comment for the CreateResource function.
---
Duplicate comments:
In `@backend/internal/resource/declarative_resource_test.go`:
- Around line 240-359: The four standalone tests (TestParseToResourceServer,
TestParseToResourceServer_MissingID, TestParseToResourceServer_MissingName,
TestBuildPermissionString) must be converted into methods on the test suite
(e.g., func (s *DeclarativeResourceTestSuite) TestParseToResourceServer(...) )
so they run under the stretchr/testify suite pattern; update each function
signature to be a suite method, replace t.* assertions with the suite
equivalents (s.Require(), s.Assert(), or s.Equal()/s.NoError() on the suite),
and ensure the suite's TestMain or Test... entry registers/runs the suite; keep
references to parseToResourceServer and buildPermissionString unchanged but
invoke them from the new suite methods.
In `@backend/internal/resource/declarative_resource.go`:
- Around line 144-149: The code calls e.service.GetActionList(ctx, id, nil,
1000, 0) for root-level actions but discards the returned list; capture that
return value (e.g., rootActions, err := e.service.GetActionList(...)) and
merge/append those entries into the export payload's actions collection (the
same slice you use for resource-scoped actions) so server-level actions
(resourceID == nil) are included; keep the existing error handling and ensure
duplicates are handled/filtered if necessary.
In `@backend/internal/resource/service.go`:
- Around line 908-911: UpdateAction currently returns &ErrorImmutableAction
directly which leaves the %s placeholder unformatted; change it to format
ErrorImmutableAction.ErrorDescription with fmt.Sprintf (passing the
resourceServerID or appropriate name) and return a copy like
&ErrorImmutableAction with the formatted ErrorDescription, and add a debug log
(using the same logger used elsewhere) that records the immutable-action attempt
and resourceServerID; locate the check around IsResourceServerDeclarative in
UpdateAction and mirror the pattern used in DeleteAction and UpdateResource for
formatting and logging.
---
Nitpick comments:
In `@backend/internal/resource/service.go`:
- Around line 281-291: UpdateResourceServer performs the declarative check
(rs.IsResourceServerDeclarative(id)) after fetching the existing resource server
which causes a 404 for non-existent declarative IDs; to match
DeleteResourceServer, UpdateResource, DeleteResource, UpdateAction, and
DeleteAction, move the rs.IsResourceServerDeclarative(id) guard to the top of
UpdateResourceServer (before fetching existing data) so it returns the immutable
ServiceError when the ID is declarative; update the error-return path in
UpdateResourceServer to construct and return the same serviceerror.ServiceError
(Type/Code/Error/ErrorDescription) as used elsewhere when the check fails.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
backend/cmd/server/servicemanager.gobackend/internal/resource/ResourceServiceInterface_mock_test.gobackend/internal/resource/composite_store.gobackend/internal/resource/composite_store_test.gobackend/internal/resource/config.gobackend/internal/resource/config_test.gobackend/internal/resource/declarative_resource.gobackend/internal/resource/declarative_resource_test.gobackend/internal/resource/error_constants.gobackend/internal/resource/file_based_store.gobackend/internal/resource/file_based_store_test.gobackend/internal/resource/handler.gobackend/internal/resource/init.gobackend/internal/resource/init_test.gobackend/internal/resource/model.gobackend/internal/resource/resourceStoreInterface_mock_test.gobackend/internal/resource/service.gobackend/internal/resource/service_test.gobackend/internal/resource/store.gobackend/internal/resource/store_test.gobackend/internal/system/config/config.gobackend/internal/system/declarative_resource/entity/entity_store.gobackend/internal/system/declarative_resource/store.gobackend/tests/mocks/resourcemock/ResourceServiceInterface_mock.gobackend/tests/mocks/resourcemock/resourceStoreInterface_mock.go
✅ Files skipped from review due to trivial changes (1)
- backend/internal/resource/store_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/cmd/server/servicemanager.go
- backend/internal/resource/composite_store_test.go
- backend/internal/system/config/config.go
- backend/internal/resource/config.go
- backend/internal/resource/store.go
- backend/tests/mocks/resourcemock/ResourceServiceInterface_mock.go
- backend/internal/resource/file_based_store.go
rajithacharith
force-pushed
the
composite-resource-server
branch
from
47fab61 to
cbd9670
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
backend/internal/resource/service.go (2)
410-412:⚠️ Potential issue | 🟡 MinorStale concatenated comment on line 412.
// CreateResource creates a new resource under a resource server.// Resource Methodshas a leftover fragment from a merge/edit. Clean it up.Proposed fix
-// Resource operations - -// CreateResource creates a new resource under a resource server.// Resource Methods +// Resource operations // CreateResource creates a new resource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/service.go` around lines 410 - 412, Remove the stale concatenated comment fragment on the CreateResource comment: locate the comment attached to the CreateResource declaration (the line containing "// CreateResource creates a new resource under a resource server.// Resource Methods") and edit it to leave a single clean comment (e.g., "// CreateResource creates a new resource under a resource server.") so the leftover "// Resource Methods" fragment is removed.
908-911:⚠️ Potential issue | 🟠 MajorUnformatted
%splaceholder inErrorImmutableActionreturn.Unlike all other declarative-check paths in this file (e.g.,
UpdateResourceServerat Line 284,DeleteActionat Line 991),UpdateActionreturns the rawErrorImmutableActionconstant without formatting the%splaceholder or logging a debug message. This sends a literal%sto the client.Proposed fix
// Check if resource server is declarative (immutable) if rs.IsResourceServerDeclarative(resourceServerID) { - return nil, &ErrorImmutableAction + rs.logger.Debug( + "Cannot modify action in declarative resource server", + log.String("resource_server_id", resourceServerID), + ) + errorMsg := fmt.Sprintf(ErrorImmutableAction.ErrorDescription, id) + return nil, &serviceerror.ServiceError{ + Type: ErrorImmutableAction.Type, + Code: ErrorImmutableAction.Code, + Error: ErrorImmutableAction.Error, + ErrorDescription: errorMsg, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/service.go` around lines 908 - 911, UpdateAction currently returns the ErrorImmutableAction constant raw, leaving the `%s` placeholder unformatted; change the return to format ErrorImmutableAction with the resourceServerID (or the appropriate identifier used elsewhere) and add a matching debug/process log like other declarative-check paths; locate the check that calls rs.IsResourceServerDeclarative(resourceServerID) inside UpdateAction and replace the plain return of &ErrorImmutableAction with a formatted error instance and an accompanying debug log message that includes resourceServerID.backend/internal/resource/model.go (1)
162-166:⚠️ Potential issue | 🟡 MinorComment at line 165 contradicts the actual struct tags.
The comment says "No json tags (service layer, converted to Response structs for API)" but the structs below all include
jsontags on most fields. Update the comment to accurately reflect that bothyamlandjsontags are present, with computed/internal fields usingjson:"-".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/model.go` around lines 162 - 166, The top comment block starting "Consolidated resource models for YAML parsing, processing, and service layer" is inaccurate: update it to state that struct fields include both yaml and json tags (yaml tags for parsing/serialization and json tags for service/API conversion) and that computed/internal fields use yaml:"-" and json:"-"; keep the rest of the guidance (e.g. converting to Response structs for the API) intact. Ensure the revised comment mentions both tag types and the json:"-" convention for computed/internal fields so it matches the actual struct tags in model.go.backend/internal/resource/declarative_resource.go (1)
144-157:⚠️ Potential issue | 🟠 MajorRoot-level actions are fetched but silently discarded; parent detection uses wrong field.
Two issues in this block:
Line 145: Root-level actions are fetched (
GetActionListwithnilresourceID) but assigned to_, so server-level actions are lost from the export.Line 152:
res.ParentHandle == ""is used to detect root resources, butParentHandleis only populated during YAML parsing. For DB-sourced resources (which this exporter handles),ParentHandlewill always be empty — meaning all resources appear as roots and parent relationships are lost. Useres.Parent == nilinstead.Proposed fix for root detection and parent handle population
+ // Attach parent handles for YAML export + for _, res := range resources.Resources { + if res.Parent != nil { + if parentHandle, ok := idToHandleMap[*res.Parent]; ok { + resourceIDMap[res.ID].ParentHandle = parentHandle + } + } + } + // Build the hierarchical structure - add root-level resources for _, res := range resources.Resources { - if res.ParentHandle == "" { + if res.Parent == nil { if resource, ok := resourceIDMap[res.ID]; ok { rs.Resources = append(rs.Resources, *resource) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource.go` around lines 144 - 157, The code calls GetActionList(ctx, id, nil, 1000, 0) but discards the returned server-level actions and also determines root resources using res.ParentHandle == "" which is only set during YAML parsing; update the block to capture the returned actions (assign the first return value to a variable and attach them to the export result) and change the root-detection to use res.Parent == nil when building the hierarchical structure; ensure you still use resourceIDMap and resources.Resources to map children into rs.Resources and populate any missing ParentHandle only if needed for downstream YAML output.backend/internal/resource/file_based_store.go (3)
192-215:⚠️ Potential issue | 🟠 MajorResource list/count stubs hide declarative data in composite mode.
These methods always return empty/zero, so composite list endpoints never surface file-based resources. Either implement proper server-scoped listing (e.g., by mapping
resServerInternalIDto a file server ID) or return an explicit error to make the limitation visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 192 - 215, The four stub methods on fileBasedResourceStore (GetResourceList, GetResourceListByParent, GetResourceListCount, GetResourceListCountByParent) currently hide file-backed resources by returning empty/zero; replace the stubs with one of two explicit fixes: (A) implement server-scoped listing by mapping the resServerInternalID to the underlying file server identifier (use whatever mapping helper exists on fileBasedResourceStore or add a helper like mapResServerInternalID) and then enumerate/filter the file index by that server ID, parentInternalID, limit and offset and return the matching []Resource or count; or (B) if composite listing truly cannot include file-based resources, return a descriptive error (e.g., fmt.Errorf("file-based resources not supported for composite listing: resServerInternalID=%d") ) from each of GetResourceList, GetResourceListByParent, GetResourceListCount, and GetResourceListCountByParent so callers see the limitation instead of getting silent empty results.
250-330:⚠️ Potential issue | 🟠 MajorAction lookups/lists ignore scoping, causing cross-resource collisions.
GetActionmatches by raw handle and ignoresresServerInternalID/resInternalID, so it can return an action from the wrong server/resource.GetActionListalso cannot correctly scope to a specific resource and can return actions from unrelated resources. Tighten to composite IDs and add resource/server scoping (or fail fast when scoping cannot be honored).💡 Minimal fix for the handle-only match
- if actionID == id || action.Handle == id { + if actionID == id { return Action{ ID: actionID, Name: action.Name, Handle: action.Handle, Description: action.Description, Permission: action.Permission, }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 250 - 330, GetAction and GetActionList are ignoring scoping: stop matching actions by raw action.Handle and instead require the composite ID or enforce resServerInternalID/resInternalID scoping. In GetAction (function GetAction) only return an Action when the composite actionID (fmt.Sprintf("%s_%s_%s", rs.ID, res.Handle, action.Handle)) equals id OR when resServerInternalID matches the current ResourceServer's internal ID and resInternalID (when non-nil and non-zero) matches the current Resource's internal ID and the action.Handle matches; otherwise continue searching or return errActionNotFound. In GetActionList (function GetActionList) fix the resource-scoping logic so that when resInternalID is provided you only collect actions from the Resource whose internal ID equals *resInternalID (remove the current inverted continue), and when resServerInternalID is provided only iterate the matching ResourceServer; if the provided scoping cannot be honored, fail fast or return an empty list per callers' contract. Use the existing rs and res identifiers to compare their internal IDs and build composite IDs consistently.
163-184:⚠️ Potential issue | 🟠 MajorAvoid ambiguous resource matches by enforcing composite IDs or server scoping.
GetResourceignoresresServerInternalIDand allows raw-handle matches, so two declarative servers sharing a handle can return the wrong resource. Prefer matching only the unambiguous composite ID (or otherwise scope by server).💡 Suggested fix (require composite ID)
- for _, res := range rs.Resources { - if res.Handle == id || fmt.Sprintf("%s_%s", rs.ID, res.Handle) == id { + for _, res := range rs.Resources { + compositeID := fmt.Sprintf("%s_%s", rs.ID, res.Handle) + if compositeID == id { resource := Resource{ - ID: id, + ID: compositeID, Name: res.Name, Handle: res.Handle, Description: res.Description, Parent: res.Parent, ParentHandle: res.ParentHandle, Permission: res.Permission, } return 0, resource, nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/file_based_store.go` around lines 163 - 184, GetResource currently ignores resServerInternalID and treats raw handles as valid matches causing ambiguous results; update the matching logic in GetResource (iterating GenericFileBasedStore.List over ResourceServer rs and rs.Resources) to only accept the composite ID fmt.Sprintf("%s_%s", rs.ID, res.Handle) OR, if resServerInternalID is provided (non-zero), require rs.InternalID == resServerInternalID and then allow matching by res.Handle; also return the matched server's internal ID (rs.InternalID) instead of the hardcoded 0 when a resource is found so callers receive the correct server scoping information.backend/internal/resource/declarative_resource_test.go (1)
240-359: Move standalone tests into the suite to follow the test pattern.The standalone tests bypass the suite pattern and should be converted into suite methods for consistency. This matches a prior review note.
As per coding guidelines, "Usestretchr/testifyfor tests and follow the test suite pattern".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource_test.go` around lines 240 - 359, Several tests (TestParseToResourceServer, TestParseToResourceServer_MissingID, TestParseToResourceServer_MissingName, TestBuildPermissionString) are standalone but should be moved into the test suite; convert each to methods on the existing suite (e.g., func (s *DeclarativeResourceTestSuite) TestParseToResourceServer() { ... }) and replace t.* asserts with s.Require.* or s.Assert.* (using s.T() if necessary), update any helper calls unchanged (parseToResourceServer, buildPermissionString), remove or comment out the old top-level Test* functions, and ensure the suite registration (suite.Run(t, new(DeclarativeResourceTestSuite))) remains in the file so the new methods are executed.
🧹 Nitpick comments (6)
backend/internal/resource/handler.go (1)
189-197: Redundant nil-init + conditional assignment forParent.
sanitized.Parentis already*string— assigning it directly would have the same effect without the nil-init and conditional block.Suggested simplification
serviceReq := Resource{ Name: sanitized.Name, Handle: sanitized.Handle, Description: sanitized.Description, - Parent: nil, - } - if sanitized.Parent != nil { - serviceReq.Parent = sanitized.Parent + Parent: sanitized.Parent, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/handler.go` around lines 189 - 197, The code redundantly sets serviceReq.Parent to nil then conditionally assigns sanitized.Parent; instead set Parent directly from sanitized.Parent when constructing Resource. Update the Resource literal in the handler where serviceReq is created (the Resource struct initializer) to use Parent: sanitized.Parent and remove the separate if block that assigns serviceReq.Parent, referencing the Resource type and the sanitized.Parent field.backend/internal/resource/declarative_resource.go (1)
21-32:import "testing"in production code is a code smell.Importing the
testingpackage in non-test source files is generally discouraged. While thetesting.Testing()guard at line 51 prevents misuse at runtime, this import links the testing infrastructure into the production binary. Consider movingnewResourceServerExporterForTestto a_test.gofile or a separate test-helper file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/declarative_resource.go` around lines 21 - 32, The production file imports the testing package and uses testing.Testing() inside newResourceServerExporterForTest, which pulls test code into the binary; move the helper newResourceServerExporterForTest (and any usage of testing.Testing()) into a _test.go file or a dedicated test helper package so the production file no longer imports "testing" and the production build is free of test-only dependencies; update references to declarativeresource.newResourceServerExporterForTest accordingly (or export a test-only constructor in the _test.go helper) to preserve test behavior.backend/internal/resource/init.go (1)
54-69:getResourceStoreMode()is called twice — once insideinitializeStore()(line 74) and again here (line 56).Since the config doesn't change between calls, this is functionally correct but redundant. Consider computing the mode once and passing it into
initializeStore, or returning it alongside the store.Suggested refactor
-func Initialize( - mux *http.ServeMux, - ouService oupkg.OrganizationUnitServiceInterface, -) (ResourceServiceInterface, declarativeresource.ResourceExporter, error) { - // Initialize store based on configuration - resourceStore, err := initializeStore() +func Initialize( + mux *http.ServeMux, + ouService oupkg.OrganizationUnitServiceInterface, +) (ResourceServiceInterface, declarativeresource.ResourceExporter, error) { + storeMode := getResourceStoreMode() + + // Initialize store based on configuration + resourceStore, err := initializeStore(storeMode) ... - storeMode := getResourceStoreMode() if storeMode == serverconst.StoreModeDeclarative || storeMode == serverconst.StoreModeComposite {And update
initializeStoreto accept astoreModeparameter:-func initializeStore() (resourceStoreInterface, error) { - storeMode := getResourceStoreMode() +func initializeStore(storeMode serverconst.StoreMode) (resourceStoreInterface, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/init.go` around lines 54 - 69, getResourceStoreMode() is invoked twice (here and inside initializeStore); compute the mode once and thread it through to avoid redundant calls by either (A) calling getResourceStoreMode() once in this function and passing the resulting storeMode into initializeStore (update initializeStore signature to accept storeMode) or (B) change initializeStore to return the storeMode alongside the store and use that returned value here; update references to the storeMode variable in this file and the initializeStore function signature/return accordingly (look for getResourceStoreMode, initializeStore, loadDeclarativeResources, and serverconst.StoreMode* usages).backend/internal/resource/config.go (1)
38-56: Consider logging a warning when an invalid service-level store mode is ignored.If
cfg.Resource.Storeis set to a non-empty but invalid value, the function silently falls through to the global/default path. A debug/warn log would help operators diagnose misconfiguration.Suggested addition
if cfg.Resource.Store != "" { mode := serverconst.StoreMode(strings.ToLower(strings.TrimSpace(cfg.Resource.Store))) // Validate and normalize switch mode { case serverconst.StoreModeMutable, serverconst.StoreModeDeclarative, serverconst.StoreModeComposite: return mode + default: + // Invalid value configured — fall through to global/default } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/config.go` around lines 38 - 56, When cfg.Resource.Store is non-empty but not one of the valid values in getResourceStoreMode, add a warning log that includes the invalid cfg.Resource.Store value and the fallback decision; update the branch inside getResourceStoreMode (after evaluating mode := serverconst.StoreMode(... ) and before falling through) to call your project’s logger (e.g., processLogger or package logger) with a message like "invalid resource store '%s' - falling back to %s" where the second %s is the resolved fallback (use declarativeresource.IsDeclarativeModeEnabled() ? serverconst.StoreModeDeclarative : serverconst.StoreModeMutable) so operators can see the bad value and which mode will be used.backend/internal/resource/composite_store_test.go (1)
202-220: Rename count tests to reflect deduplication behavior.These tests now assert deduplicated counts but their names still say “SumsBothStores”, which is misleading.
✏️ Suggested renames
-func (s *CompositeResourceStoreTestSuite) TestGetResourceServerListCount_SumsBothStores() { +func (s *CompositeResourceStoreTestSuite) TestGetResourceServerListCount_Deduplicates() { -func (s *CompositeResourceStoreTestSuite) TestGetResourceListCount_SumsBothStores() { +func (s *CompositeResourceStoreTestSuite) TestGetResourceListCount_Deduplicates() { -func (s *CompositeResourceStoreTestSuite) TestGetActionListCount_SumsBothStores() { +func (s *CompositeResourceStoreTestSuite) TestGetActionListCount_Deduplicates() {Also applies to: 398-416, 516-534
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/composite_store_test.go` around lines 202 - 220, The test TestGetResourceServerListCount_SumsBothStores is misnamed because it asserts deduplicated results; rename it (and other similar tests at the other locations) to reflect deduplication (e.g., TestGetResourceServerListCount_DeduplicatesAcrossStores) and update any related test names in the suite; search for occurrences of TestGetResourceServerListCount_SumsBothStores and other mismatching names and change them to names that reference deduplication so they match the behavior of compositeStore.GetResourceServerListCount and the asserted deduplicated count.backend/internal/resource/config_test.go (1)
32-39: Keep the test suite type unexported.The suite type doesn’t need to be exported; keeping it unexported aligns with internal export rules and reduces package surface area.
Proposed change
-type ResourceConfigTestSuite struct { +type resourceConfigTestSuite struct { suite.Suite } func TestResourceConfigTestSuite(t *testing.T) { - suite.Run(t, new(ResourceConfigTestSuite)) + suite.Run(t, new(resourceConfigTestSuite)) }As per coding guidelines, "Only export the service interface (e.g.,
XServiceInterface) and models that are used in the service interface from a package."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/resource/config_test.go` around lines 32 - 39, The test suite type ResourceConfigTestSuite should be unexported; rename it to resourceConfigTestSuite and update the test runner to instantiate the new unexported type (change suite.Run(t, new(ResourceConfigTestSuite)) to use new(resourceConfigTestSuite)). Ensure the type declaration "type resourceConfigTestSuite struct { suite.Suite }" and the TestResourceConfigTestSuite function reference the new name so tests continue to run.
🤖 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/resource/composite_store.go`:
- Around line 399-402: The current compositeResourceStore.ValidatePermissions
delegates solely to c.dbStore.ValidatePermissions which treats permissions
present only in the declarative store as invalid; change ValidatePermissions to
call both c.dbStore.ValidatePermissions and
c.declarativeStore.ValidatePermissions (or equivalent declarative store method),
collect each call's invalid-permissions slice, and return only permissions that
are invalid in both stores (i.e., the intersection) so a permission is valid if
present in either store; propagate any call errors immediately. Ensure you
reference compositeResourceStore.ValidatePermissions,
c.dbStore.ValidatePermissions and the declarative store's ValidatePermissions
method when making the change.
In `@backend/internal/resource/config_test.go`:
- Around line 50-55: SetupTest in ResourceConfigTestSuite mutates the shared
ThunderRuntime config (via config.GetThunderRuntime()) by setting
runtime.Config.Resource.Store and runtime.Config.DeclarativeResources.Enabled
without restoring them; capture the previous values at the start of SetupTest
(read and save runtime.Config.Resource.Store and
runtime.Config.DeclarativeResources.Enabled) and restore them in a teardown step
(implement TearDownTest or defer/cleanup called from SetupTest) so that the
global runtime state is returned to its original values after each test.
In `@backend/internal/resource/declarative_resource.go`:
- Around line 328-370: The buildPermissionString function currently breaks
silently when a parent handle is missing; change it to surface that problem by
either returning an error (change signature of buildPermissionString to (string,
error) and return a descriptive error when resourceHandleMap lacks
current.ParentHandle) or by emitting a warning via the package logger before
breaking; update callers (places calling buildPermissionString) to handle the
new error return or tolerate the warning, and ensure you reference
current.ParentHandle, resourceHandleMap, and the buildPermissionString callers
so the chain-missing situation is not silently ignored.
- Around line 372-403: validateResourceServerWrapper currently treats any error
from fileStore.GetResourceServer and dbStore.GetResourceServer as "not a
duplicate", so unexpected errors (I/O, permission, etc.) are ignored; update the
checks around fileStore.GetResourceServer and dbStore.GetResourceServer to
distinguish a true "not found" result from other errors (e.g., using
errors.Is(err, ErrNotFound) or the store's sentinel error) and only treat
not-found as non-duplicate, while propagating/logging any other error returned;
keep references to ResourceServer, fileStore.GetResourceServer and
dbStore.GetResourceServer when changing the logic.
In `@backend/internal/resource/file_based_store.go`:
- Around line 379-407: The method ValidatePermissions currently builds a set
validPermissions from ResourceServer entries but returns that set; instead, keep
building validPermissions (from list and ResourceServer/rs.Resources and
actions) and then iterate over the input parameter permissions to collect those
not present in validPermissions into an invalidList (e.g., change validList to
invalidList), and return invalidList, nil; preserve the existing
resServerInternalID filtering and error handling around
f.GenericFileBasedStore.List() and use the same symbols: ValidatePermissions,
validPermissions, permissions, list, ResourceServer, rs.Resources, and actions
to locate and implement this change.
---
Duplicate comments:
In `@backend/internal/resource/declarative_resource_test.go`:
- Around line 240-359: Several tests (TestParseToResourceServer,
TestParseToResourceServer_MissingID, TestParseToResourceServer_MissingName,
TestBuildPermissionString) are standalone but should be moved into the test
suite; convert each to methods on the existing suite (e.g., func (s
*DeclarativeResourceTestSuite) TestParseToResourceServer() { ... }) and replace
t.* asserts with s.Require.* or s.Assert.* (using s.T() if necessary), update
any helper calls unchanged (parseToResourceServer, buildPermissionString),
remove or comment out the old top-level Test* functions, and ensure the suite
registration (suite.Run(t, new(DeclarativeResourceTestSuite))) remains in the
file so the new methods are executed.
In `@backend/internal/resource/declarative_resource.go`:
- Around line 144-157: The code calls GetActionList(ctx, id, nil, 1000, 0) but
discards the returned server-level actions and also determines root resources
using res.ParentHandle == "" which is only set during YAML parsing; update the
block to capture the returned actions (assign the first return value to a
variable and attach them to the export result) and change the root-detection to
use res.Parent == nil when building the hierarchical structure; ensure you still
use resourceIDMap and resources.Resources to map children into rs.Resources and
populate any missing ParentHandle only if needed for downstream YAML output.
In `@backend/internal/resource/file_based_store.go`:
- Around line 192-215: The four stub methods on fileBasedResourceStore
(GetResourceList, GetResourceListByParent, GetResourceListCount,
GetResourceListCountByParent) currently hide file-backed resources by returning
empty/zero; replace the stubs with one of two explicit fixes: (A) implement
server-scoped listing by mapping the resServerInternalID to the underlying file
server identifier (use whatever mapping helper exists on fileBasedResourceStore
or add a helper like mapResServerInternalID) and then enumerate/filter the file
index by that server ID, parentInternalID, limit and offset and return the
matching []Resource or count; or (B) if composite listing truly cannot include
file-based resources, return a descriptive error (e.g., fmt.Errorf("file-based
resources not supported for composite listing: resServerInternalID=%d") ) from
each of GetResourceList, GetResourceListByParent, GetResourceListCount, and
GetResourceListCountByParent so callers see the limitation instead of getting
silent empty results.
- Around line 250-330: GetAction and GetActionList are ignoring scoping: stop
matching actions by raw action.Handle and instead require the composite ID or
enforce resServerInternalID/resInternalID scoping. In GetAction (function
GetAction) only return an Action when the composite actionID
(fmt.Sprintf("%s_%s_%s", rs.ID, res.Handle, action.Handle)) equals id OR when
resServerInternalID matches the current ResourceServer's internal ID and
resInternalID (when non-nil and non-zero) matches the current Resource's
internal ID and the action.Handle matches; otherwise continue searching or
return errActionNotFound. In GetActionList (function GetActionList) fix the
resource-scoping logic so that when resInternalID is provided you only collect
actions from the Resource whose internal ID equals *resInternalID (remove the
current inverted continue), and when resServerInternalID is provided only
iterate the matching ResourceServer; if the provided scoping cannot be honored,
fail fast or return an empty list per callers' contract. Use the existing rs and
res identifiers to compare their internal IDs and build composite IDs
consistently.
- Around line 163-184: GetResource currently ignores resServerInternalID and
treats raw handles as valid matches causing ambiguous results; update the
matching logic in GetResource (iterating GenericFileBasedStore.List over
ResourceServer rs and rs.Resources) to only accept the composite ID
fmt.Sprintf("%s_%s", rs.ID, res.Handle) OR, if resServerInternalID is provided
(non-zero), require rs.InternalID == resServerInternalID and then allow matching
by res.Handle; also return the matched server's internal ID (rs.InternalID)
instead of the hardcoded 0 when a resource is found so callers receive the
correct server scoping information.
In `@backend/internal/resource/model.go`:
- Around line 162-166: The top comment block starting "Consolidated resource
models for YAML parsing, processing, and service layer" is inaccurate: update it
to state that struct fields include both yaml and json tags (yaml tags for
parsing/serialization and json tags for service/API conversion) and that
computed/internal fields use yaml:"-" and json:"-"; keep the rest of the
guidance (e.g. converting to Response structs for the API) intact. Ensure the
revised comment mentions both tag types and the json:"-" convention for
computed/internal fields so it matches the actual struct tags in model.go.
In `@backend/internal/resource/service.go`:
- Around line 410-412: Remove the stale concatenated comment fragment on the
CreateResource comment: locate the comment attached to the CreateResource
declaration (the line containing "// CreateResource creates a new resource under
a resource server.// Resource Methods") and edit it to leave a single clean
comment (e.g., "// CreateResource creates a new resource under a resource
server.") so the leftover "// Resource Methods" fragment is removed.
- Around line 908-911: UpdateAction currently returns the ErrorImmutableAction
constant raw, leaving the `%s` placeholder unformatted; change the return to
format ErrorImmutableAction with the resourceServerID (or the appropriate
identifier used elsewhere) and add a matching debug/process log like other
declarative-check paths; locate the check that calls
rs.IsResourceServerDeclarative(resourceServerID) inside UpdateAction and replace
the plain return of &ErrorImmutableAction with a formatted error instance and an
accompanying debug log message that includes resourceServerID.
---
Nitpick comments:
In `@backend/internal/resource/composite_store_test.go`:
- Around line 202-220: The test TestGetResourceServerListCount_SumsBothStores is
misnamed because it asserts deduplicated results; rename it (and other similar
tests at the other locations) to reflect deduplication (e.g.,
TestGetResourceServerListCount_DeduplicatesAcrossStores) and update any related
test names in the suite; search for occurrences of
TestGetResourceServerListCount_SumsBothStores and other mismatching names and
change them to names that reference deduplication so they match the behavior of
compositeStore.GetResourceServerListCount and the asserted deduplicated count.
In `@backend/internal/resource/config_test.go`:
- Around line 32-39: The test suite type ResourceConfigTestSuite should be
unexported; rename it to resourceConfigTestSuite and update the test runner to
instantiate the new unexported type (change suite.Run(t,
new(ResourceConfigTestSuite)) to use new(resourceConfigTestSuite)). Ensure the
type declaration "type resourceConfigTestSuite struct { suite.Suite }" and the
TestResourceConfigTestSuite function reference the new name so tests continue to
run.
In `@backend/internal/resource/config.go`:
- Around line 38-56: When cfg.Resource.Store is non-empty but not one of the
valid values in getResourceStoreMode, add a warning log that includes the
invalid cfg.Resource.Store value and the fallback decision; update the branch
inside getResourceStoreMode (after evaluating mode := serverconst.StoreMode(...
) and before falling through) to call your project’s logger (e.g., processLogger
or package logger) with a message like "invalid resource store '%s' - falling
back to %s" where the second %s is the resolved fallback (use
declarativeresource.IsDeclarativeModeEnabled() ?
serverconst.StoreModeDeclarative : serverconst.StoreModeMutable) so operators
can see the bad value and which mode will be used.
In `@backend/internal/resource/declarative_resource.go`:
- Around line 21-32: The production file imports the testing package and uses
testing.Testing() inside newResourceServerExporterForTest, which pulls test code
into the binary; move the helper newResourceServerExporterForTest (and any usage
of testing.Testing()) into a _test.go file or a dedicated test helper package so
the production file no longer imports "testing" and the production build is free
of test-only dependencies; update references to
declarativeresource.newResourceServerExporterForTest accordingly (or export a
test-only constructor in the _test.go helper) to preserve test behavior.
In `@backend/internal/resource/handler.go`:
- Around line 189-197: The code redundantly sets serviceReq.Parent to nil then
conditionally assigns sanitized.Parent; instead set Parent directly from
sanitized.Parent when constructing Resource. Update the Resource literal in the
handler where serviceReq is created (the Resource struct initializer) to use
Parent: sanitized.Parent and remove the separate if block that assigns
serviceReq.Parent, referencing the Resource type and the sanitized.Parent field.
In `@backend/internal/resource/init.go`:
- Around line 54-69: getResourceStoreMode() is invoked twice (here and inside
initializeStore); compute the mode once and thread it through to avoid redundant
calls by either (A) calling getResourceStoreMode() once in this function and
passing the resulting storeMode into initializeStore (update initializeStore
signature to accept storeMode) or (B) change initializeStore to return the
storeMode alongside the store and use that returned value here; update
references to the storeMode variable in this file and the initializeStore
function signature/return accordingly (look for getResourceStoreMode,
initializeStore, loadDeclarativeResources, and serverconst.StoreMode* usages).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
backend/cmd/server/servicemanager.gobackend/internal/resource/ResourceServiceInterface_mock_test.gobackend/internal/resource/composite_store.gobackend/internal/resource/composite_store_test.gobackend/internal/resource/config.gobackend/internal/resource/config_test.gobackend/internal/resource/declarative_resource.gobackend/internal/resource/declarative_resource_test.gobackend/internal/resource/error_constants.gobackend/internal/resource/file_based_store.gobackend/internal/resource/file_based_store_test.gobackend/internal/resource/handler.gobackend/internal/resource/init.gobackend/internal/resource/init_test.gobackend/internal/resource/model.gobackend/internal/resource/resourceStoreInterface_mock_test.gobackend/internal/resource/service.gobackend/internal/resource/service_test.gobackend/internal/resource/store.gobackend/internal/resource/store_test.gobackend/internal/system/config/config.gobackend/internal/system/declarative_resource/entity/entity_store.gobackend/internal/system/declarative_resource/store.gobackend/tests/mocks/resourcemock/ResourceServiceInterface_mock.gobackend/tests/mocks/resourcemock/resourceStoreInterface_mock.go
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/tests/mocks/resourcemock/ResourceServiceInterface_mock.go
- backend/internal/system/config/config.go
- backend/internal/system/declarative_resource/entity/entity_store.go
- backend/internal/resource/store.go
- backend/internal/resource/store_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1530 +/- ##
==========================================
- Coverage 91.35% 91.19% -0.16%
==========================================
Files 693 697 +4
Lines 46293 47131 +838
==========================================
+ Hits 42289 42981 +692
- Misses 2685 2772 +87
- Partials 1319 1378 +59
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:
|
rajithacharith
force-pushed
the
composite-resource-server
branch
12 times, most recently
from
be0888e to
468362f
Compare
rajithacharith
force-pushed
the
composite-resource-server
branch
2 times, most recently
from
5833cbc to
d2a7478
Compare
senthalan
reviewed
Feb 26, 2026
| // - If disabled: return "mutable" | ||
| // | ||
| // Returns normalized store mode: "mutable", "declarative", or "composite" | ||
| func getResourceStoreMode() serverconst.StoreMode { |
Contributor
There was a problem hiding this comment.
shall we have move to a common place rather than having it in each package?
We can do this refactoring in separate PR
Comment on lines
41
to
43
| resourceServerIDMap map[string]int // maps resource server UUID to internal ID | ||
| resourceIDMap map[string]int // maps resource UUID to internal ID | ||
| actionIDMap map[string]int // maps action UUID to internal ID |
Contributor
There was a problem hiding this comment.
I think we have removed the internalID in the DB store level. So do we need this?
rajithacharith
force-pushed
the
composite-resource-server
branch
5 times, most recently
from
58cec2f to
7f5dac4
Compare
rajithacharith
force-pushed
the
composite-resource-server
branch
from
7f5dac4 to
97a1849
Compare
senthalan
reviewed
Feb 26, 2026
| return Resource{}, err | ||
| } | ||
|
|
||
| for _, item := range list { |
Contributor
There was a problem hiding this comment.
can we improve the GenericFileBasedStore to have a getByID func and keep a map in the if Entity data against the ID we can quickly get by ID without this loop.
Can we track it as a separate issue to improve this all over the place?
senthalan
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
Support composite store for resource service
Approach
This pull request introduces significant enhancements to the resource server management subsystem, focusing on supporting declarative and composite resource server modes, improving configuration flexibility, and increasing test coverage. The changes include the addition of configuration logic to determine the resource store mode, new error constants for immutable (declarative) resources, expanded mock/test support for declarative checks, and comprehensive unit tests for both configuration and declarative resource export functionality.
Resource server store mode configuration and validation:
config.goinresourceto determine the resource server store mode (mutable,declarative, orcomposite) based on service-level or global configuration, including normalization and fallback logic.config_test.gowith extensive test coverage for all configuration scenarios, ensuring correct precedence and case-insensitivity for store mode values.Declarative resource server support and error handling:
error_constants.gofor attempts to modify declarative (immutable) resource servers, resources, or actions, improving error clarity and client feedback.Declarative resource server exporter and test coverage:
declarative_resource_test.gowith a full test suite for theResourceServerExporter, covering resource export, filtering of declarative servers, validation, and YAML parsing logic.Mock/test infrastructure improvements:
ResourceServiceInterfaceMockto supportIsResourceServerDeclarative, enabling accurate mocking of declarative resource server checks in tests.Integration with service initialization:
servicemanager.goto append the newresourceExporterto the exporters list during service registration, ensuring declarative resource export is available at runtime.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
Bug Fixes
Tests