[NAE-2194] Global role creation fix #355
Conversation
Replaced single role fetch with a paginated query to ensure proper handling of multiple records and avoid potential conflicts. Adjusted logic to check for empty content in the paginated result.
Updated role ID generation logic in Importer to ensure proper assignment based on global or process-specific scope. Added getter for pagination properties in UserServiceImpl for better access. Removed unused `countAll` method from UserRepository to clean up unused code.
Refine Javadoc to clarify the behavior of assigning process roles to admin users. Explicitly state that roles are added as a union, duplicates are ignored, and no roles are removed. Simplify parameter description for better readability.
# Conflicts: # nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java
WalkthroughUpdates role import logic to use pageable lookups and adjust ID initialization for global vs. non-global roles. Removes redundant ID setting in create path. Adds a new UserService API to update admin users with roles. Minor Lombok annotation added in UserServiceImpl. ProcessRoleService API changed from single-result to pageable. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Importer
participant PRS as ProcessRoleService
participant Net as PetriNet
participant Role as ProcessRole
Importer->>PRS: findAllByImportId(GLOBAL + roleId, Pageable.ofSize(1))
PRS-->>Importer: Page<ProcessRole> (0 or 1)
alt no existing role
Importer->>Importer: shouldInitializeRole() via Page.isEmpty()
Importer->>Role: initRole(importRole)
note right of Role: _id assignment<br/>- Global: new ObjectId()<br/>- Non-global: new ProcessResourceId(ObjectId(net.stringId))
Importer->>Net: attach role to net (create path)
else role exists
Importer->>Role: use existing role from Page.getContent().getFirst()
note right of Importer: No external _id set outside init
end
sequenceDiagram
autonumber
actor Caller
participant US as UserService
participant Repo as UserRepository
participant Admins as AdminUsers
Caller->>US: updateAdminWithRoles(roles)
US->>Repo: findAllAdmins()
Repo-->>US: AdminUsers
loop for each admin
US->>Admins: addAll(roles) // union semantics
US->>Repo: save(admin)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
⏰ 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). (2)
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 |
|
Blocked by #350 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java (1)
33-34: Remove duplicate/unused SUPER_ADMIN_EMAIL; rely on UserConstantsThis field duplicates UserConstants.ADMIN_USER_EMAIL and appears unused here. Drop it to avoid drift.
Apply:
- public static final String SUPER_ADMIN_EMAIL = "super@netgrif.com";nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java (1)
52-63: Null-safe builder assignmentsIf metadata/entryPoints are passed as null, keep non-null defaults.
- this.entryPoints = entryPoints; + this.entryPoints = entryPoints != null ? entryPoints : new HashMap<>(); this.active = active; - this.metadata = metadata; + this.metadata = metadata != null ? metadata : new LinkedHashMap<>();application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileListFieldTest.groovy (1)
111-115: Use centralized admin email constantKeep a single source of truth by using UserConstants.ADMIN_USER_EMAIL instead of a local USER_EMAIL.
Apply this diff:
- def adminUser = importHelper.createUser(new User(firstName: "Admin", lastName: "User", username: UserConstants.ADMIN_USER_USERNAME, email: USER_EMAIL, password: "password", state: UserState.ACTIVE), + def adminUser = importHelper.createUser(new User(firstName: "Admin", lastName: "User", username: UserConstants.ADMIN_USER_USERNAME, email: UserConstants.ADMIN_USER_EMAIL, password: "password", state: UserState.ACTIVE), [auths.get("admin")] as Authority[], // [] as Group[], [] as ProcessRole[])application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy (1)
97-105: Reduce duplication: cache admin onceMinor: fetch the admin once (in @beforeeach) and reuse to cut repeated lookups.
If you want, I can provide a concise refactor snippet moving admin retrieval to a field set in before().
Also applies to: 111-120, 128-136, 145-145, 151-156
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileFieldTest.groovy (2)
114-118: Use configured admin password instead of hard-coded literalYou already inject userPassword (Line 71). Use it to avoid drift with configuration.
Apply this diff:
- def adminUser = importHelper.createUser(new User(firstName: "Admin", lastName: "User", username: UserConstants.ADMIN_USER_USERNAME, email: UserConstants.ADMIN_USER_EMAIL, password: "password", state: UserState.ACTIVE), + def adminUser = importHelper.createUser(new User(firstName: "Admin", lastName: "User", username: UserConstants.ADMIN_USER_USERNAME, email: UserConstants.ADMIN_USER_EMAIL, password: userPassword, state: UserState.ACTIVE), [auths.get("admin")] as Authority[], // [] as Group[], [] as ProcessRole[])
66-67: Remove unused USER_EMAIL constantIt’s now superseded by UserConstants and appears unused.
Apply this diff:
- public static final String USER_EMAIL = "super@netgrif.com"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
application-engine/pom.xml(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/groovy/GroovyShellConfiguration.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java(2 hunks)application-engine/src/main/resources/application.yaml(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/action/RemoveActionTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy(6 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileFieldTest.groovy(5 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileListFieldTest.groovy(4 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/CaseSearchTest.groovy(2 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/workflow/UserRefsTest.groovy(1 hunks)nae-object-library/pom.xml(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/constants/UserConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java(3 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Serializer.java(2 hunks)nae-spring-core-adapter/pom.xml(1 hunks)nae-user-ce/pom.xml(1 hunks)nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java(3 hunks)nae-user-common/pom.xml(1 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java(1 hunks)pom.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-05T10:21:54.857Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy:135-135
Timestamp: 2025-09-05T10:21:54.857Z
Learning: In ExportServiceTest.groovy, writing to src/test/resources is intentional to simulate production behavior where the working tree is mutated during file exports. This mirrors how the system works in production.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In the SuperCreatorRunner.java file, setPassword() sets the same credential as setCredential("password", passwordCredential), so there's no need to call both methods when creating the super user.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser(), and both setPassword() and setCredential() ultimately reference the same credential system.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java
🧬 Code graph analysis (2)
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/constants/UserConstants.java (1)
UserConstants(3-11)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/constants/UserConstants.java (1)
UserConstants(3-11)
⏰ 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). (4)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
🔇 Additional comments (18)
nae-object-library/pom.xml (1)
10-10: Parent bump and Spring Boot consistency verified
- All modules now target application-engine-parent 7.0.0-RC6.1
- No lingering -SNAPSHOT references
- spring.boot.version defined as 3.4.4 in the parent POM
- spring-boot-maven-plugin consistently at 3.4.4 (explicit in one module, inherited in others)
nae-user-common/pom.xml (1)
9-9: Parent version updated to RC6.1: looks good.application-engine/pom.xml (1)
9-9: Application module aligned to parent RC6.1: OK. Ensure dependencyManagement resolves identically post-bump.Use the script shared in nae-object-library/pom.xml to confirm effective Spring Boot and plugin versions remain consistent after the parent change.
nae-user-ce/pom.xml (1)
9-9: Parent RC6.1 alignment confirmed.application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java (1)
56-63: Correct: using UserConstants and single password credential pathThis aligns with prior guidance to avoid redundant setPassword() and centralizes admin identity.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Serializer.java (2)
3-5: Logging addition is appropriateSLF4J logger and category look good.
Also applies to: 10-10
20-22: Document null return in Serializer.deserialize Javadoc and verify callers
- Method now returns null for null/empty input or deserialization failure; update Javadoc:
/** * @return Deserialized object, or null if bytes are null/empty or deserialization fails. */- No internal calls found; manually verify external and integration consumers handle null results.
- Consider adding an ObjectInputFilter to mitigate deserialization gadget risks.
application-engine/src/main/java/com/netgrif/application/engine/configuration/groovy/GroovyShellConfiguration.java (1)
23-26: Default star imports helper: LGTMAdding getDefaultEngineImports() and wiring via addStarImports improves DX for Groovy actions.
Also applies to: 39-45
application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java (2)
1094-1101: Id assignment semantics diverge between global and process rolesGlobal roles get a fresh ObjectId, non-global use net id; with the unconditional set_id (Line 1072) removed, this looks consistent. Please confirm this matches downstream persistence/lookup expectations (e.g., queries by importId and id).
1070-1071: Omit the additional null-guard refactor: shouldInitializeRole’s empty check already guarantees getFirst() is safe in this branch.Likely an incorrect or invalid review comment.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java (1)
9-9: LinkedHashMap import LGTMPreserving insertion order for metadata is reasonable.
application-engine/src/test/groovy/com/netgrif/application/engine/workflow/CaseSearchTest.groovy (1)
6-6: Centralized constant import LGTMUsing UserConstants improves maintainability across tests.
application-engine/src/test/groovy/com/netgrif/application/engine/action/RemoveActionTest.groovy (1)
6-6: Centralized constant import LGTMAligns with other tests.
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (2)
93-94: Expose Nullable in DSL — verify null semanticsImport + helper look good. Please confirm Nullable.of(value) accepts null and does not throw. If it mirrors Optional.of, calls like nullable(null) from actions would fail; in that case, provide Nullable.ofNullable or guard here.
Also applies to: 911-913
2769-2776: Clearer errors in resolveStoragePath — LGTMString.formatted usage and specific messages are good and JDK 21–safe.
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileListFieldTest.groovy (1)
197-199: Same here: remove Optional.get()Mirror the safer pattern for consistency.
Apply this diff:
- AbstractUser user = userService.findUserByUsername(UserConstants.ADMIN_USER_USERNAME, null).get() + AbstractUser user = userService.findByEmail(UserConstants.ADMIN_USER_EMAIL, null) + assert user != null : "Missing admin user: ${UserConstants.ADMIN_USER_EMAIL}"⛔ Skipped due to learnings
Learnt from: renczesstefan PR: netgrif/application-engine#350 File: application-engine/src/test/groovy/com/netgrif/application/engine/export/service/ExportServiceTest.groovy:82-82 Timestamp: 2025-09-05T10:17:28.557Z Learning: In the application-engine test suite, testHelper.truncateDbs() follows a two-phase approach: first it clears the databases, then it executes superCreator to create the admin user. This means Optional.get() is safe when retrieving the admin user after truncateDbs() because the admin user is guaranteed to exist.application-engine/src/main/resources/application.yaml (1)
162-163: Add Nullable to default action imports — LGTMMatches the new ActionDelegate.nullable helper and enables use from actions without explicit import.
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileFieldTest.groovy (1)
9-9: LGTM: Centralized admin constantsUsing UserConstants avoids hard-coded values across tests.
.../main/java/com/netgrif/application/engine/configuration/groovy/GroovyShellConfiguration.java
Show resolved
Hide resolved
application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java
Show resolved
Hide resolved
application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java
Outdated
Show resolved
Hide resolved
application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java
Show resolved
Hide resolved
...ication-engine/src/test/groovy/com/netgrif/application/engine/action/RemoveActionTest.groovy
Show resolved
Hide resolved
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
Show resolved
Hide resolved
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
Show resolved
Hide resolved
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
Show resolved
Hide resolved
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java
Show resolved
Hide resolved
- Remove redundant `_id` assignment in Importer class
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java (4)
46-46: Prefer PageRequest over Pageable.ofSize for clarity and broader SD compatibilityUse
PageRequest.ofSize(1)and importPageRequest. This also aligns with prior feedback.import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.data.domain.Pageable; +import org.springframework.data.domain.Pageable; +import org.springframework.data.domain.PageRequest;
1071-1071: Replace Pageable.ofSize with PageRequest.ofSizeKeep paging style consistent with the import.
- role = processRoleService.findAllByImportId(ProcessRole.GLOBAL + importRole.getId(), Pageable.ofSize(1)).getContent().getFirst(); + role = processRoleService.findAllByImportId(ProcessRole.GLOBAL + importRole.getId(), PageRequest.ofSize(1)).getContent().getFirst();
1080-1081: Use PageRequest.ofSize in existence check (or remove check per above refactor)If you keep
shouldInitializeRole, align withPageRequest.- (importRole.isGlobal() && processRoleService.findAllByImportId(ProcessRole.GLOBAL + importRole.getId(), Pageable.ofSize(1)).getContent().isEmpty()); + (importRole.isGlobal() && processRoleService.findAllByImportId(ProcessRole.GLOBAL + importRole.getId(), PageRequest.ofSize(1)).getContent().isEmpty());Also consider deleting this method if you adopt the single-retrieval flow above.
1071-1071: Enforce uniqueness of global role importId at the repository/index levelRelying on “first page item” is brittle and order-dependent. Add a unique index on
importIdfor global roles (or a repository method returning a single optional) to prevent duplicates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java(3 hunks)
⏰ 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). (3)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (1)
application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java (1)
1-1: Incorrect — Importer.java doesn't depend on that API; method already presentImporter.java contains no ProcessRoleService references; Page findAllByImportId(String, Pageable) is implemented at nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/service/ProcessRoleService.java.
Likely an incorrect or invalid review comment.
application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java
Show resolved
Hide resolved
application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java
Show resolved
Hide resolved
renczesstefan
dismissed stale reviews from machacjozef and tuplle
The merge-base changed after approval.
Description
Implements NAE-2194
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