fix(rest-api): clean stale group refs from page accessControls on group deletion (APIM-13541)#16331
fix(rest-api): clean stale group refs from page accessControls on group deletion (APIM-13541)#16331
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| return objectMapper.writerWithDefaultPrettyPrinter().writeValueAsString(apiEntity); | ||
| } catch (final Exception e) { | ||
| LOGGER.error("An error occurs while trying to JSON serialize the API {}", apiEntity, e); | ||
| throw new TechnicalManagementException("An error occurs while trying to JSON serialize the API " + apiId, e); |
There was a problem hiding this comment.
exportAsJson catch block logs the exception then throws TechnicalManagementException; avoid logging and rethrowing the same exception (choose one).
Details
✨ AI Reasoning
The change to exportAsJson's exception handling now both logs the caught exception and immediately throws a new TechnicalManagementException. Logging and rethrowing the same exception (or wrapping it) creates duplicate reporting and can lead to noisy logs and confusing error handling. The rest of the diff does not introduce other logging+rethrow patterns. This finding is focused on the modified catch block which previously swallowed the exception and returned an empty string; the new behavior adds a throw while retaining the log call.
🔧 How do I fix it?
Choose to either log the exception and handle it locally, or rethrow it for handling at a higher level. If rethrowing, consider wrapping the original exception to add context. Avoid logging in catch blocks that rethrow, as the caller will likely log the exception.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| } | ||
|
|
||
| // Replace group id by group name in access control list | ||
| // Replace group id by group name in access control list, skipping stale refs to deleted groups |
There was a problem hiding this comment.
Pages access-control mapping duplicates the group-id-to-name resolution logic already used for plans; extract a shared helper to avoid repeating computeIfAbsent(...) with the same try/catch and logging.
Details
✨ AI Reasoning
The serializer now contains two very similar code blocks that map group IDs to names using groupIdNameMap.computeIfAbsent(...) with identical exception handling and logging. Both blocks perform the same business operation (resolve group id to name while skipping missing groups), which is substantial logic repeated in the same class and would benefit from consolidation to a helper method to avoid duplicated behavior and logging differences in future changes.
🔧 How do I fix it?
Delete extra code. Extract repeated code sequences into reusable functions or methods. Use loops or data structures to eliminate repetitive patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| ) | ||
| ) | ||
| ) | ||
| .filter(accessControlEntity -> { |
There was a problem hiding this comment.
Anonymous lambda in the stream.filter is lengthy and contains try/catch, logging, mutation, and conditional logic; extract to a well-named method to improve readability and testability.
Details
✨ AI Reasoning
A newly added anonymous function (lambda) inside the pages.forEach stream pipeline encapsulates several steps: computing a group name with a try/catch, logging on exception, returning null to indicate skip, checking the computed value, mutating the accessControlEntity by setting referenceId, and returning a boolean. This is more than 5 lines and performs non-trivial business logic and side-effects, making it harder to read and test. Extracting it to a named private method would improve readability and testability. The issue is introduced by this PR's change to the access control transformation logic.
🔧 How do I fix it?
Extract complex anonymous functions into named functions with descriptive names, or add explanatory comments for their purpose.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the handling of group deletions and API exports by ensuring stale group references are cleaned up or ignored. Specifically, it adds a global cleanup step during group deletion to remove access control references from all API pages and updates the API serializer to skip and log warnings for missing groups. Review feedback suggests optimizing the global page query in GroupServiceImpl to avoid performance issues and refactoring the stream logic in ApiSerializer to avoid performing side effects within a filter predicate.
| PageCriteria allApiPagesCriteria = new PageCriteria.Builder().referenceType(PageReferenceType.API.name()).build(); | ||
| removeGroupFromPages(groupId, updatedDate, allApiPagesCriteria); |
There was a problem hiding this comment.
Querying all API pages globally (PageReferenceType.API) without any further filtering can lead to significant performance issues in environments with a large number of APIs and pages. This operation performs a full scan of the pages associated with all APIs, which could lead to high memory usage or timeouts during group deletion.
Additionally, if removeGroupFromPages is still called within the preceding loop for APIs explicitly belonging to the group, this results in redundant processing for those pages. Consider removing the redundant call from the loop and, if possible, optimizing the repository search to filter by group reference within the access controls.
References
- Prioritize code simplicity and readability over micro-optimizations that do not address a real performance bottleneck.
- When propagating attributes in a tree structure, if a child item does not require an update, propagation to its descendants can be skipped if there's a constraint that prevents descendants from having a more permissive state than their parent. This is a performance optimization.
| .filter(accessControlEntity -> { | ||
| String groupName = groupIdNameMap.computeIfAbsent(accessControlEntity.getReferenceId(), key -> { | ||
| try { | ||
| return groupService.findById(GraviteeContext.getExecutionContext(), key).getName(); | ||
| } catch (GroupNotFoundException e) { | ||
| log.warn( | ||
| "Group [{}] referenced in page access control no longer exists, skipping from export", | ||
| key | ||
| ); | ||
| return null; | ||
| } | ||
| }); | ||
| if (groupName == null) { | ||
| return false; | ||
| } | ||
| accessControlEntity.setReferenceId(groupName); | ||
| return true; | ||
| }) |
There was a problem hiding this comment.
Using .filter() to perform side effects, such as mutating the accessControlEntity via setReferenceId, is discouraged in functional programming and violates the expected behavior of the Stream API. Predicates in a filter should be side-effect free. A cleaner approach would be to use .map() for the transformation and then filter out the null results.
| .filter(accessControlEntity -> { | |
| String groupName = groupIdNameMap.computeIfAbsent(accessControlEntity.getReferenceId(), key -> { | |
| try { | |
| return groupService.findById(GraviteeContext.getExecutionContext(), key).getName(); | |
| } catch (GroupNotFoundException e) { | |
| log.warn( | |
| "Group [{}] referenced in page access control no longer exists, skipping from export", | |
| key | |
| ); | |
| return null; | |
| } | |
| }); | |
| if (groupName == null) { | |
| return false; | |
| } | |
| accessControlEntity.setReferenceId(groupName); | |
| return true; | |
| }) | |
| .map(accessControlEntity -> { | |
| String groupName = groupIdNameMap.computeIfAbsent(accessControlEntity.getReferenceId(), key -> { | |
| try { | |
| return groupService.findById(GraviteeContext.getExecutionContext(), key).getName(); | |
| } catch (GroupNotFoundException e) { | |
| log.warn( | |
| "Group [{}] referenced in page access control no longer exists, skipping from export", | |
| key | |
| ); | |
| return null; | |
| } | |
| }); | |
| if (groupName != null) { | |
| accessControlEntity.setReferenceId(groupName); | |
| return accessControlEntity; | |
| } | |
| return null; | |
| }) | |
| .filter(Objects::nonNull) |
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
resolveGroupName returns null on missing group and is used with computeIfAbsent; computeIfAbsent won’t cache null, causing repeated groupService.findById DB calls for the same missing group. Cache negative lookups or store a sentinel to avoid repeated calls.
Details
✨ AI Reasoning
The new resolveGroupName(...) mapping is used inside a stream to replace group ids with names for page access controls. resolveGroupName calls groupService.findById(...) inside Map.computeIfAbsent and returns null when the group is missing. In Java, computeIfAbsent does not add an entry when the mapping function returns null, so repeated missing groupIds will cause repeated calls to groupService.findById for the same id during the same export, turning what should be a cached lookup into many redundant DB calls. This is a performance regression compared to the previous computeIfAbsent usage that always returned a non-null name (or threw). The changed lines are where the mapping and the helper method were added/used.
🔧 How do I fix it?
Move constant work outside loops. Use StringBuilder instead of string concatenation in loops. Cache compiled regex patterns. Use hash-based lookups instead of nested loops. Batch database operations instead of N+1 queries.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| new Criteria().andOperator( | ||
| where("accessControls.referenceId").is(criteria.getAccessControlGroupId()), | ||
| where("accessControls.referenceType").is("GROUP") |
There was a problem hiding this comment.
The accessControls.referenceId and accessControls.referenceType checks can match different array elements, causing false-positive pages. This logic may return pages without a single GROUP access control matching the requested group id.
Show fix
| new Criteria().andOperator( | |
| where("accessControls.referenceId").is(criteria.getAccessControlGroupId()), | |
| where("accessControls.referenceType").is("GROUP") | |
| where("accessControls").elemMatch( | |
| new Criteria().andOperator( | |
| where("referenceId").is(criteria.getAccessControlGroupId()), | |
| where("referenceType").is("GROUP") | |
| ) |
Details
✨ AI Reasoning
The new filtering logic is trying to return only pages where a single access-control entry matches both the target group id and type GROUP. However, the two predicates are applied independently, so one array element can satisfy the id check while another satisfies the type check. That makes the assumption of a single matching access-control entry invalid and can return pages that do not actually contain the intended GROUP access control.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| } | ||
|
|
||
| // Replace group id by group name in access control list | ||
| // Replace group id by group name in access control list, skipping stale refs to deleted groups |
There was a problem hiding this comment.
ApiSerializer.serialize is very large and handles many unrelated responsibilities (serialization, page filtering, group resolution, plan transformations); consider decomposing.
Details
✨ AI Reasoning
1. The code is trying to serialize an ApiEntity to JSON for export, including embedding and transforming pages, plans and group references.
2. The serialize method mixes many responsibilities: writing top-level API fields, handling flows/paths, resources, properties, membership export, resolving group IDs to names for pages and plans, filtering pages by version, and writing media/metadata — which increases cognitive load.
3. This harms maintainability because a developer must understand multiple unrelated concerns in one large method to make changes (serialization format, group resolution, filtering rules, page transformations).
4. The PR modified this long method (replacing inline group id resolution and adding resolveGroupName), which increases the total code associated with serialization and adds more branching and caching behavior within the serialization flow, thus worsening the method's cognitive complexity.
5. This is not a small helper or lifecycle method where large size is expected; it's a serializer that would benefit from decomposition.
🔧 How do I fix it?
Break down long functions into smaller helper functions. Aim for functions under 60 lines with fewer than 10 local variables.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
…up deletion (APIM-13541)
The PR description states "Changed |
|
Ran tests and fix works. See https://gravitee.atlassian.net/browse/APIM-13541?focusedCommentId=82420 for details |
2 participants
Summary
accessControlsfor APIs not in the group'sgroups[]field. v2 export then crashed silently (HTTP 200 with empty body).GroupServiceImpl.delete()that queries ALL API-type pages and removes stale access controlsexportAsJson()to throw instead of silently returning empty stringFixes https://gravitee.atlassian.net/browse/APIM-13541
Test plan