[NAE-2213] Delete global role #362
Merged
Conversation
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
Introduced a new process for deleting global roles, including updates to `ProcessRoleService` and a new `ProcessRoleController`. Added exceptions for role handling and enhanced `PetriNetRepository` to support role-specific queries. This allows streamlined management and cleanup of global roles across users, cases, and tasks.
- added check for non global role
- replaced warn to exception
renczesstefan
added
improvement
WalkthroughAdds cascade deletion for global roles with a new DELETE /api/roles/{id} endpoint; adds PetriNet role-based query/service method; wires workflow/task services into ProcessRoleService; introduces role exceptions; alters Importer.initRole to skip explicit Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as ProcessRoleController
participant PRS as ProcessRoleService
participant PRRepo as ProcessRoleRepository
participant Realm as RealmService
participant UserSvc as UserService
participant PNService as PetriNetService
participant PNRepo as PetriNetRepository
participant WF as IWorkflowService
participant TS as ITaskService
Client->>Controller: DELETE /api/roles/{id}
Controller->>PRS: deleteGlobalRole(id, loggedUser)
PRS->>PRRepo: findById(id)
alt role not found
PRS-->>Controller: throw RoleNotFoundException
Controller-->>Client: 404
else role exists but not global
PRS-->>Controller: throw RoleNotGlobalException
Controller-->>Client: 409
else global role
PRS->>Realm: iterate realms
loop per realm
PRS->>UserSvc: list users with role
loop per user
PRS->>UserSvc: remove role, save user
end
end
PRS->>PNService: findAllByRoleId(id, pageable)
PNService->>PNRepo: query roles.?0 exists
PNRepo-->>PNService: Page<PetriNet>
loop per PetriNet
PRS->>PNService: remove role from net & transitions, save net
PRS->>WF: find cases by petriNet
loop per case
PRS->>WF: remove role from case collections, save case
PRS->>TS: remove role from tasks, save tasks
end
end
PRS->>PRRepo: delete(role)
PRS-->>Controller: success
Controller-->>Client: 200
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (2)
49-63: Limit surface area: avoid exposing repositories/services via getters unless required.
@Getteron internal deps widens API unnecessarily; reduce to package-private or remove if only used internally/tests.
160-166: Replace LoggedUser#getId() equality checks with getStringId()ProcessRoleService change is correct — ripgrep shows remaining LoggedUser#getId() comparisons that must be updated to avoid internal/external ID mismatches:
- application-engine/src/main/java/com/netgrif/application/engine/security/service/SecurityContextService.java:112 — ((LoggedUser) ...).getId().equals(loggedUser.getId()) → use getStringId() on both sides.
- application-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.java:161 — Objects.equals(loggedUser.getId(), userId) → use loggedUser.getStringId().
Optional: consider updating logging/message uses for consistency:
- application-engine/src/main/java/com/netgrif/application/engine/auth/web/AuthenticationController.java:136
- application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java:138
- application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:114
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/config/ProcessBeansConfiguration.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/domain/repositories/PetriNetRepository.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java(9 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotFoundException.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotGlobalException.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/service/ProcessRoleService.java(1 hunks)
💤 Files with no reviewable changes (1)
- application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
PR: netgrif/application-engine#331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/config/ProcessBeansConfiguration.java
🧬 Code graph analysis (2)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java (3)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotFoundException.java (1)
RoleNotFoundException(3-7)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotGlobalException.java (1)
RoleNotGlobalException(3-7)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (1)
ProcessRoleService(44-558)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (1)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotGlobalException.java (1)
RoleNotGlobalException(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (6)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java (1)
349-356: API addition LGTM; ensure consistent behavior with other gettersMethod is fine. Align behavior with other list fetchers (e.g., initializeArcs per item) in the concrete service to avoid surprises.
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/service/ProcessRoleService.java (1)
40-40: Deletion should be authorized, idempotent, and transactionally boundedEnsure the implementation:
- checks caller privileges (admin/global-role manager),
- is idempotent (no error on repeated deletes),
- uses @transactional where applicable for repository ops, with safe pagination for long runs.
Would you like a quick scan script to verify the implementation throws RoleNotGlobalException/RoleNotFoundException and is annotated with @transactional?
application-engine/src/main/java/com/netgrif/application/engine/petrinet/config/ProcessBeansConfiguration.java (1)
36-53: Constructor matches bean wiring; @lazy present — verify runtime circulars
- application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java: constructor (lines ~68–76) matches the bean parameters and includes @lazy on the relevant dependencies.
- deleteGlobalRole(String, LoggedUser) is implemented (starts ~line 463).
- Verify no circular runtime dependency by searching for reciprocal references to ProcessRoleService (e.g. run rg -n 'ProcessRoleService') and ensure none of the injected beans (UserService, IPetriNetService, GroupService, RealmService, PaginationProperties, IWorkflowService, ITaskService) inject ProcessRoleService; if any do, keep @lazy or refactor.
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (3)
528-536: Avoid unpaged task traversal — use paging or batched updates.findAllByCase(caseId) may load all Task entities for a case and cause OOM/slow writes.
- If possible, add Page findAllByCase(String caseId, Pageable p) and iterate page-by-page, updating/saving per page.
- Otherwise, stream only task IDs and perform batched read/update/save (e.g., batches of 100–1000) to reduce memory and write amplification.
Location: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java:528-536. Automated repo search produced no matches for TaskService paging methods (script returned no output); verify whether TaskService exposes a paged API or implement batching.
495-511: Process cascade looks correct; verify all net-level role holders are covered.You remove role IDs from transitions and the net.roles map. Automated repo search produced no results (couldn't locate source files), so I couldn't confirm other net-scoped collections — manually verify PetriNet, Transition, and any View/NegativeView/Permission/ACL/UI role-holder collections for roleId references and update/remove them as needed.
72-85: Constructor wiring: OK; confirm @lazy is necessary and doesn’t mask circular deps.If cycles persist, prefer breaking them with clearer boundaries over
@Lazy.
...ain/java/com/netgrif/application/engine/petrinet/domain/repositories/PetriNetRepository.java
Show resolved
Hide resolved
...on-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
Show resolved
Hide resolved
...m/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotFoundException.java
Show resolved
Hide resolved
.../netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotGlobalException.java
Show resolved
Hide resolved
Updated the `findAllByRoleId` method to ensure PetriNet arcs are initialized after retrieval. This change prevents potential uninitialized arc issues and ensures consistency when accessing PetriNet objects.
Replaced logging and silent return with a `RoleNotFoundException` to ensure proper error handling during global role deletion. This change improves robustness and clarity by enforcing explicit feedback when a role is missing.
Added a validation to block attempts to delete core roles (DEFAULT and ANONYMOUS) in the ProcessRoleService. This change ensures critical roles remain intact and avoids potential application issues caused by their removal.
Introduced @PreAuthorize annotation to restrict access to the deleteGlobalRole endpoint to administrators only. Added IllegalArgumentException to the exception handling to improve error coverage during role deletion. These changes enhance security and robustness.
Ensure the `pageable` object advances in each iteration of the role removal loop to correctly process paginated cases. This fixes potential issues with infinite loops or incomplete processing of data.
Simplify the removal of process roles from transitions and refine the filtering of user roles by eliminating redundant conditions. These changes enhance code readability and maintainability.
Replaced the schemaLocation URL to point to the updated GitHub-hosted location. This ensures compatibility with the latest schema version and resolves potential issues with the old URL.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (1)
31-65: Avoid exposing internal service dependencies via public getters.@getterS on repositories/services widen surface area and couple tests/callers to internals. Prefer package-private fields or targeted accessor methods only where truly needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java(9 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java(1 hunks)nae-object-library/src/main/resources/petriflow_schema.xsd(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-29T10:31:57.307Z
Learnt from: renczesstefan
PR: netgrif/application-engine#362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java:513-529
Timestamp: 2025-09-29T10:31:57.307Z
Learning: PetriNet.getStringId() returns a simple ObjectId string representation (_id.toString()), not a composite Netgrif ID format, so new ObjectId(petriNetId) works correctly when petriNetId comes from PetriNet.getStringId().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java
📚 Learning: 2025-09-29T10:31:31.458Z
Learnt from: renczesstefan
PR: netgrif/application-engine#362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java:43-45
Timestamp: 2025-09-29T10:31:31.458Z
Learning: In the Netgrif application engine, when Authentication objects reach controller endpoints, auth.getPrincipal() always returns a LoggedUser instance and is never null, so defensive casting checks are not necessary in controller methods.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
🧬 Code graph analysis (2)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (3)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotFoundException.java (1)
RoleNotFoundException(3-7)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotGlobalException.java (1)
RoleNotGlobalException(3-7)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java (1)
Getter(21-311)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java (3)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotFoundException.java (1)
RoleNotFoundException(3-7)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotGlobalException.java (1)
RoleNotGlobalException(3-7)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (1)
ProcessRoleService(45-560)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (8)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (1)
333-336: Arc initialization stays consistentThanks for keeping the new role-based finder aligned with the service invariant—initializing arcs before returning the page matches the other accessors and avoids surprises downstream.
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java (2)
32-34: Verify security expression aligns with your authority model.Confirm @authorizationService.hasAuthority('ADMIN') matches your standard (e.g., role prefixing, case), and that authorizationService is exposed as a SpEL bean.
41-52: Fix content-type and map exceptions to proper HTTP statuses (JSON response).Currently returns a plain String with produces=application/json and collapses errors to 400. Return JSON and map:
- RoleNotFoundException → 404
- RoleNotGlobalException → 409
- IllegalArgumentException → 400
Also log client errors at WARN.Apply within this method:
- @DeleteMapping(value = "/{id}",produces = MediaType.APPLICATION_JSON_VALUE) - public ResponseEntity<String> deleteGlobalRole(@PathVariable("id") String id, Authentication auth) { + @DeleteMapping(value = "/{id}", produces = MediaType.APPLICATION_JSON_VALUE) + public ResponseEntity<java.util.Map<String, String>> deleteGlobalRole(@PathVariable("id") String id, Authentication auth) { try { LoggedUser user = (LoggedUser) auth.getPrincipal(); processRoleService.deleteGlobalRole(id, user); - } catch (RoleNotGlobalException | RoleNotFoundException | IllegalArgumentException e) { - String message = "Error when deleting global role [%s]".formatted(id); - log.error(message, e); - return ResponseEntity.badRequest().body(e.getMessage()); + } catch (RoleNotFoundException e) { + log.warn("Global role [{}] not found", id, e); + return ResponseEntity.status(404).body(java.util.Map.of("message", e.getMessage())); + } catch (RoleNotGlobalException e) { + log.warn("Role [{}] is not global", id, e); + return ResponseEntity.status(409).body(java.util.Map.of("message", e.getMessage())); + } catch (IllegalArgumentException e) { + log.warn("Invalid role id [{}]", id, e); + return ResponseEntity.badRequest().body(java.util.Map.of("message", e.getMessage())); } - return ResponseEntity.ok("Global role was deleted successfully"); + return ResponseEntity.ok(java.util.Map.of("message", "Global role was deleted successfully")); }Add/update OpenAPI responses to reflect 404/409 and JSON bodies:
- @ApiResponses(value = { - @ApiResponse(responseCode = "200", description = "Global role was deleted successfully"), - @ApiResponse(responseCode = "400", description = "Invalid role id"), - @ApiResponse(responseCode = "403", description = "Caller doesn't fulfill the authorisation requirements"), - @ApiResponse(responseCode = "500", description = "Internal server error") - }) + @ApiResponses(value = { + @ApiResponse(responseCode = "200", description = "Global role was deleted successfully"), + @ApiResponse(responseCode = "400", description = "Invalid role id"), + @ApiResponse(responseCode = "403", description = "Caller doesn't fulfill the authorisation requirements"), + @ApiResponse(responseCode = "404", description = "Role not found"), + @ApiResponse(responseCode = "409", description = "Role exists but is not global"), + @ApiResponse(responseCode = "500", description = "Internal server error") + })And ensure imports (outside this hunk):
import org.springframework.http.HttpStatus; // if you choose to use status constants import java.util.Map;application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (4)
163-168: LGTM: correct context refresh check.Comparing userId to loggedUser.getStringId() prevents unnecessary context reloads.
498-511: Net updates OK; verify map key type.Removing from petriNet.getRoles() by role string ID assumes the map is keyed by role string IDs. If keyed differently (e.g., importId), adjust the removal.
513-529: LGTM: paging advanced; ObjectId conversion is valid for PetriNet IDs.Loop advances pageable and uses new ObjectId(petriNetId), which matches PetriNet.getStringId() format. Based on learnings.
539-549: LGTM: preserves unrelated roles while removing only the target.The filter now excludes just the deleted role and reassigns the remainder correctly.
nae-object-library/src/main/resources/petriflow_schema.xsd (1)
6-6: Include is correct—no changes needed
Verified that the remote schema’s<xs:schema>root has notargetNamespace, soxs:includeis appropriate.
...engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java
Outdated
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
Show resolved
Hide resolved
The @parameter annotation provides additional details for the "id" parameter in the delete global role endpoint. This enhancement improves API documentation by specifying the parameter's purpose, requirement status, and an example value.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-29T10:31:31.458Z
Learnt from: renczesstefan
PR: netgrif/application-engine#362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java:43-45
Timestamp: 2025-09-29T10:31:31.458Z
Learning: In the Netgrif application engine, when Authentication objects reach controller endpoints, auth.getPrincipal() always returns a LoggedUser instance and is never null, so defensive casting checks are not necessary in controller methods.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java (3)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotFoundException.java (1)
RoleNotFoundException(3-7)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotGlobalException.java (1)
RoleNotGlobalException(3-7)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (1)
ProcessRoleService(45-560)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
...-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
Show resolved
Hide resolved
Updated the deleteGlobalRole endpoint to use ResponseMessage for consistent error handling. Added specific responses for RoleNotFoundException (404) and RoleNotGlobalException (409) with detailed logging.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-29T10:31:31.469Z
Learnt from: renczesstefan
PR: netgrif/application-engine#362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java:43-45
Timestamp: 2025-09-29T10:31:31.469Z
Learning: In the Netgrif application engine, when Authentication objects reach controller endpoints, auth.getPrincipal() always returns a LoggedUser instance and is never null, so defensive casting checks are not necessary in controller methods.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java (4)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/common/web/responsebodies/ResponseMessage.java (1)
ResponseMessage(4-60)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotFoundException.java (1)
RoleNotFoundException(3-7)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotGlobalException.java (1)
RoleNotGlobalException(3-7)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (1)
ProcessRoleService(45-560)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (3)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java (3)
1-34: LGTM! Controller structure and dependencies are well-organized.The imports, class annotations, and dependency injection are correctly configured. The use of Lombok annotations reduces boilerplate, and the REST controller is properly mapped to
/api/roles.
36-47: LGTM! Authorization and API documentation are comprehensive.The endpoint is properly secured with
@PreAuthorizerestricting access to ADMIN authority, and the OpenAPI annotations thoroughly document all possible response codes (200, 400, 403, 404, 409, 500) with clear descriptions and parameter documentation.
48-65: LGTM! Exception handling correctly maps to HTTP status codes.The try-catch structure properly handles all exceptions thrown by the service layer:
RoleNotFoundException→ 404 Not FoundRoleNotGlobalException→ 409 ConflictIllegalArgumentException→ 400 Bad RequestAll error paths use
ResponseMessage.createErrorMessage()correctly and log appropriately.
...-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
Outdated
Show resolved
Hide resolved
machacjozef
previously approved these changes
Sep 30, 2025
Introduced a new `roles` field to the `PetriNetSearch` class to store a list of role identifiers. This change enhances the class by enabling role-specific functionality and broadening its applicability in access control scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNetSearch.java(1 hunks)
...ary/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNetSearch.java
Show resolved
Hide resolved
Introduced a check to prevent deletion of roles referenced by other processes. Added a `RoleReferencedException` to handle this scenario, ensuring proper error responses and maintaining process integrity. Modified related methods and controller to support the new exception.
machacjozef
previously approved these changes
Sep 30, 2025
- Replaced the schema location URL in `petriflow_schema.xsd` for consistency with the new domain. - Corrected the message type in `ProcessRoleController` to indicate a successful operation.
tuplle
approved these changes
Sep 30, 2025
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java(1 hunks)nae-object-library/src/main/resources/petriflow_schema.xsd(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-29T10:31:31.469Z
Learnt from: renczesstefan
PR: netgrif/application-engine#362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java:43-45
Timestamp: 2025-09-29T10:31:31.469Z
Learning: In the Netgrif application engine, when Authentication objects reach controller endpoints, auth.getPrincipal() always returns a LoggedUser instance and is never null, so defensive casting checks are not necessary in controller methods.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java (5)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/common/web/responsebodies/ResponseMessage.java (1)
ResponseMessage(4-60)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotFoundException.java (1)
RoleNotFoundException(3-7)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleNotGlobalException.java (1)
RoleNotGlobalException(3-7)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/roles/RoleReferencedException.java (1)
RoleReferencedException(3-7)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java (1)
ProcessRoleService(46-528)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (3)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java (3)
28-35: LGTM!Class structure, annotations, and dependency injection setup are correct and follow Spring best practices.
37-48: LGTM!Authorization guard, OpenAPI documentation, and API response codes are comprehensive and correctly specified. All previous review feedback has been addressed.
49-72: LGTM!Exception handling correctly maps to appropriate HTTP status codes (404 for not found, 409 for conflict, 400 for invalid requests), and the success response properly uses
createSuccessMessage(). All responses return JSON viaResponseMessage, maintaining content-type consistency.
...-engine/src/main/java/com/netgrif/application/engine/petrinet/web/ProcessRoleController.java
Show resolved
Hide resolved
machacjozef
approved these changes
Sep 30, 2025
This was referenced Oct 16, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Added new controller method and service methods to remove global roles.
Implements NAE-2213
Dependencies
No new dependencies were introduced.
Third party dependencies
No new dependencies were introduced.
Blocking Pull requests
There are no dependencies on other PR.
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores
Other