[NAE-2288] Delete of process retains global role from user #390
Conversation
- added filter for global role
|
Warning Rate limit exceeded@renczesstefan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughUser and role deletion flows were changed: UserServiceImpl was made realm-aware (adds RealmService, iterates per-realm and pages users, excludes global roles), PetriNetService no longer calls user-side role removal during net deletion, and ProcessRoleService relaxed an early-return and narrowed deleted-role propagation to non-global roles. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
Outdated
Show resolved
Hide resolved
Added `@JsonIgnore` to prevent `sessionId` serialization. Enhanced null-checking and parameter handling in `NaeReflectionUtils`. Optimized `ActionApiMethods` lookup with a prebuilt map for better performance.
- removed unnecessary method call for removing roles from user - the processRoleService.deleteRolesOfNet does the same thing - added filter for deletable process roles, so global roles will be ignored
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 (3)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java(2 hunks)nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java(3 hunks)
💤 Files with no reviewable changes (1)
- application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 329
File: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/RealmServiceImpl.java:39-39
Timestamp: 2025-07-31T08:05:21.587Z
Learning: In the Netgrif Application Engine, the domain Realm class (com.netgrif.application.engine.objects.auth.domain.Realm) is abstract and cannot be instantiated. The concrete implementation is in the adapter package (com.netgrif.application.engine.adapter.spring.auth.domain.Realm) which extends the abstract domain class. Services must instantiate the concrete adapter class and cast it when saving to repositories that accept the abstract domain type.
📚 Learning: 2025-07-31T08:05:21.587Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 329
File: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/RealmServiceImpl.java:39-39
Timestamp: 2025-07-31T08:05:21.587Z
Learning: In the Netgrif Application Engine, the domain Realm class (com.netgrif.application.engine.objects.auth.domain.Realm) is abstract and cannot be instantiated. The concrete implementation is in the adapter package (com.netgrif.application.engine.adapter.spring.auth.domain.Realm) which extends the abstract domain class. Services must instantiate the concrete adapter class and cast it when saving to repositories that accept the abstract domain type.
Applied to files:
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java
⏰ 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: Test
- 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/service/ProcessRoleService.java (1)
417-456: LGTM! Correctly preserves global roles during PetriNet deletion.The filter
.filter(processRole -> !processRole.isGlobal())ensures that only non-global roles are removed when deleting a PetriNet. This correctly addresses the bug where global roles were inadvertently removed from users during process deletion.nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (2)
59-59: LGTM! Dependency injection follows established pattern.The addition of
RealmServicevia setter injection is consistent with the existing dependency injection pattern used throughout this class.Also applies to: 117-120
553-554: LGTM! Global role filtering correctly addresses the reported bug.Excluding global roles before removal ensures they persist when the PetriNet is deleted, which is the intended behavior described in the PR objectives (NAE-2288).
...engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java
Outdated
Show resolved
Hide resolved
| Pageable realmPageable = PageRequest.of(0, paginationProperties.getBackendPageSize()); | ||
| Page<Realm> realms; | ||
| do { | ||
| users = searchUsersByRoleIds(roleIds, defaultRealmCollection, pageable); | ||
| users.getContent().forEach(u -> removeRoles(u, petriNetRoles)); | ||
| pageable = pageable.next(); | ||
| } while (users.hasNext()); | ||
| realms = realmService.getSmallRealm(realmPageable); | ||
| Page<AbstractUser> users; | ||
| for (Realm realm : realms.getContent()) { | ||
| do { | ||
| users = searchUsersByRoleIds(roleIds, collectionNameProvider.getCollectionNameForRealm(realm.getName()), pageable); | ||
| users.getContent().forEach(u -> removeRoles(u, nonGlobalPetriNetRoles)); | ||
| pageable = pageable.next(); | ||
| } while (users.hasNext()); | ||
| } | ||
| } while (realms.hasNext()); | ||
| } |
There was a problem hiding this comment.
Critical pagination bugs cause infinite loop and data corruption.
The realm-aware pagination implementation has two critical issues:
-
Infinite loop:
realmPageableis never incremented, so line 558 fetches the same realm page indefinitely whenrealms.hasNext()is true. -
User pagination not reset:
pageableis declared at line 552 and incremented at line 564, but never reset between realms. This causes users in the second and subsequent realms to be skipped. For example, if Realm 1 has 3 pages of users, Realm 2 will start at page 3, missing pages 0-2.
Apply this diff to fix both issues:
public void removeRoleOfDeletedPetriNet(Set<ProcessRole> petriNetRoles) {
- Pageable pageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
Set<ProcessRole> nonGlobalPetriNetRoles = petriNetRoles.stream().filter(r -> !r.isGlobal()).collect(Collectors.toSet());
Collection<ProcessResourceId> roleIds = nonGlobalPetriNetRoles.stream().map(ProcessRole::get_id).collect(Collectors.toSet());
Pageable realmPageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
Page<Realm> realms;
do {
realms = realmService.getSmallRealm(realmPageable);
Page<AbstractUser> users;
for (Realm realm : realms.getContent()) {
+ Pageable pageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
do {
users = searchUsersByRoleIds(roleIds, collectionNameProvider.getCollectionNameForRealm(realm.getName()), pageable);
users.getContent().forEach(u -> removeRoles(u, nonGlobalPetriNetRoles));
pageable = pageable.next();
} while (users.hasNext());
}
+ realmPageable = realmPageable.next();
} while (realms.hasNext());
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
around lines 555 to 568, the realm-aware pagination loop never advances
realmPageable and reuses the same pageable across realms; update the outer loop
to advance realmPageable (e.g., realmPageable = realmPageable.next() after
processing each realms page) and reset the user-pageable before iterating each
realm (e.g., pageable = PageRequest.of(0, pageable.getPageSize()) or capture the
initial pageable and reassign it) so each realm starts from page 0 and realm
pages progress correctly.
There was a problem hiding this comment.
Very good comment. I find both issues relevant. Please fix it and I think this refactor needs better attention to testing
There was a problem hiding this comment.
@Retoocs, thank you for confirming! Here's the complete fixed method:
@Override
public void removeRoleOfDeletedPetriNet(Set<ProcessRole> petriNetRoles) {
Set<ProcessRole> nonGlobalPetriNetRoles = petriNetRoles.stream().filter(r -> !r.isGlobal()).collect(Collectors.toSet());
Collection<ProcessResourceId> roleIds = nonGlobalPetriNetRoles.stream().map(ProcessRole::get_id).collect(Collectors.toSet());
Pageable realmPageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
Page<Realm> realms;
do {
realms = realmService.getSmallRealm(realmPageable);
for (Realm realm : realms.getContent()) {
Pageable pageable = PageRequest.of(0, paginationProperties.getBackendPageSize());
Page<AbstractUser> users;
do {
users = searchUsersByRoleIds(roleIds, collectionNameProvider.getCollectionNameForRealm(realm.getName()), pageable);
users.getContent().forEach(u -> removeRoles(u, nonGlobalPetriNetRoles));
pageable = pageable.next();
} while (users.hasNext());
}
realmPageable = realmPageable.next();
} while (realms.hasNext());
}Key changes:
- Line 560:
pageablenow initialized inside the realm loop, ensuring each realm starts from page 0 - Line 568:
realmPageablenow incremented after processing each realm page
Testing recommendations:
Given the complexity of multi-realm pagination, I strongly recommend adding integration tests that cover:
- Multiple realms with multiple pages of users - Verify all users across all realms have roles removed
- Edge cases:
- Realm with zero users
- Realm with exactly one page of users
- Realm with users spanning multiple pages
- Global vs non-global roles - Verify global roles are preserved while non-global roles are removed
- Pagination boundaries - Test with varying page sizes (e.g., 1, 10, 100)
- Data integrity - Verify no users are skipped or processed twice
Would you like me to help generate unit test scaffolding for these scenarios?
🧠 Learnings used
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 329
File: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/RealmServiceImpl.java:39-39
Timestamp: 2025-07-31T08:05:21.587Z
Learning: In the Netgrif Application Engine, the domain Realm class (com.netgrif.application.engine.objects.auth.domain.Realm) is abstract and cannot be instantiated. The concrete implementation is in the adapter package (com.netgrif.application.engine.adapter.spring.auth.domain.Realm) which extends the abstract domain class. Services must instantiate the concrete adapter class and cast it when saving to repositories that accept the abstract domain type.
| Pageable realmPageable = PageRequest.of(0, paginationProperties.getBackendPageSize()); | ||
| Page<Realm> realms; | ||
| do { | ||
| users = searchUsersByRoleIds(roleIds, defaultRealmCollection, pageable); | ||
| users.getContent().forEach(u -> removeRoles(u, petriNetRoles)); | ||
| pageable = pageable.next(); | ||
| } while (users.hasNext()); | ||
| realms = realmService.getSmallRealm(realmPageable); | ||
| Page<AbstractUser> users; | ||
| for (Realm realm : realms.getContent()) { | ||
| do { | ||
| users = searchUsersByRoleIds(roleIds, collectionNameProvider.getCollectionNameForRealm(realm.getName()), pageable); | ||
| users.getContent().forEach(u -> removeRoles(u, nonGlobalPetriNetRoles)); | ||
| pageable = pageable.next(); | ||
| } while (users.hasNext()); | ||
| } | ||
| } while (realms.hasNext()); | ||
| } |
There was a problem hiding this comment.
Very good comment. I find both issues relevant. Please fix it and I think this refactor needs better attention to testing
Description
Fixes NAE-2288
Dependencies
No new dependencies were introduced
Third party dependencies
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
Bug Fixes
Behavior Change
✏️ Tip: You can customize this high-level summary in your review settings.