Skip to content

[NAE-2418] Verify @Transient handling across persistence layers in na…#436

Open
renczesstefan wants to merge 9 commits intorelease/7.0.0-rev10from
NAE-2418
Open

[NAE-2418] Verify @Transient handling across persistence layers in na…#436
renczesstefan wants to merge 9 commits intorelease/7.0.0-rev10from
NAE-2418

Conversation

@renczesstefan
Copy link
Copy Markdown
Member

@renczesstefan renczesstefan commented Apr 27, 2026

Description

Implements NAE-2418

Dependencies

No new dependencies were introduced.

Third party dependencies

There are no dependencies on other PR.

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

Name Tested on
OS macOS Tahoe 26.3
Runtime Java 21
Dependency Manager Maven 3.9.9n
Framework version Spring Boot 3.4.4
Run parameters
Other configuration

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes have been checked, personally or remotely, with @machacjozef
  • I have commented my code, particularly in hard-to-understand areas
  • I have resolved all conflicts with the target branch of the PR
  • I have updated and synced my code with the target branch
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes:
    • Lint test
    • Unit tests
    • Integration tests
  • I have checked my contribution with code analysis tools:
  • I have made corresponding changes to the documentation:
    • Developer documentation
    • User Guides
    • Migration Guides

Summary by CodeRabbit

Release Notes

  • Refactor
    • Refactored core domain model architecture to adopt adapter-based implementations, improving Spring Data persistence compatibility and long-term maintainability
    • Optimized role and authority data retrieval through denormalization of related identifiers
    • Enhanced user lifecycle management with event-driven role and authority resolution mechanisms

…e-spring-core-adapter

Replaced the incorrect `java.beans.Transient` import with `org.springframework.data.annotation.Transient`. This ensures proper handling of transiency in the context of Spring Data.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 27, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR refactors domain classes to be abstract with Spring Data adapter implementations, introduces denormalized ID sets for authorities and process roles with lazy-loading via a MongoDB event listener, and updates all instantiation points throughout the codebase to use the adapter implementations.

Changes

Core Domain Abstraction & Spring Adapter Implementation

Layer / File(s) Summary
Core Domain Abstractions
nae-object-library/src/main/java/.../domain/arcs/{Arc,InhibitorArc,ReadArc,ResetArc}.java
nae-object-library/src/main/java/.../auth/domain/{User,Group}.java
Arc, InhibitorArc, ReadArc, and ResetArc are declared abstract. User and Group are declared abstract. All method bodies and constructors remain unchanged; only class declarations are modified.
Denormalization Fields
nae-object-library/src/main/java/.../auth/domain/AbstractActor.java
New authorityIds and processRoleIds fields added as denormalized Set<String> collections. Authority and process-role lifecycle methods (setters, add, remove, clear) now synchronize both the ID sets and object sets to maintain consistency.
Spring Arc Adapters
nae-spring-core-adapter/src/main/.../adapter/spring/petrinet/domain/arcs/{Arc,InhibitorArc,ReadArc,ResetArc}.java
New adapter classes extend their core counterparts. Each provides constructors (no-arg and copy) delegating to superclass, and overrides getSource() and getDestination() with @Transient annotations to mark them as non-persistent in Spring Data.
Spring Auth Adapters
nae-spring-core-adapter/src/main/.../adapter/spring/auth/domain/{User,Group}.java
New adapter classes extend core User and Group. Each provides constructors delegating to superclass and overrides getProcessRoles() and getAuthoritySet() with @Transient to exclude them from Spring Data persistence.
Arc Factory Wiring
application-engine/src/main/.../petrinet/service/ArcFactory.java
Arc instantiation logic updated to use Spring adapter arc classes (com.netgrif.application.engine.adapter.spring.petrinet.domain.arcs.*) for all arc types (REGULAR, VARIABLE, RESET, INHIBITOR, READ).
Service Layer Refactoring
application-engine/src/main/.../petrinet/service/ProcessRoleService.java
nae-user-ce/src/main/.../auth/service/UserServiceImpl.java
nae-user-ce/src/main/.../auth/service/GroupServiceImpl.java
ProcessRoleService: assignRolesToActor refactored to accept AbstractActor parameter instead of raw Collection<ProcessRole>, computing role diffs from actor state. UserServiceImpl/GroupServiceImpl: instantiations changed to use Spring adapter User/Group classes.
Lazy-Loading Mechanism
nae-user-ce/src/main/.../auth/service/UserMongoEventListener.java
New @Component event listener extending AbstractMongoEventListener<AbstractActor>. On onAfterConvert, it resolves processRoleIds and authorityIds back to object sets by fetching from ProcessRoleService and AuthorityService, enabling lazy population of complex collections from denormalized IDs.
Repository Query Update
nae-user-common/src/main/.../auth/repository/UserRepository.java
findAllByProcessRoles__idIn(...) query updated to filter by denormalized processRoleIds field instead of nested processRoles._id, improving query efficiency.
Production Instantiation Updates
application-engine/src/main/.../auth/service/RegistrationService.java
application-engine/src/main/.../configuration/security/PublicAuthenticationFilter.java
application-engine/src/main/.../startup/runner/SuperCreatorRunner.java
User instantiations changed from new User() to new com.netgrif.application.engine.adapter.spring.auth.domain.User().
Test Instantiation Updates
application-engine/src/test/groovy/com/netgrif/application/engine/action/...
application-engine/src/test/groovy/com/netgrif/application/engine/auth/...
application-engine/src/test/groovy/com/netgrif/application/engine/.../...
application-engine/src/test/java/com/netgrif/application/engine/...
application-engine/src/test/resources/petriNets/...
All test classes and XML test resources updated to instantiate User and Group using fully-qualified Spring adapter class names instead of core domain classes. Affects 30+ test files and XML resources.
Annotation Fix
nae-spring-core-adapter/src/main/.../workflow/domain/DataGroup.java
Import changed from java.beans.Transient to org.springframework.data.annotation.Transient so @Transient annotations resolve from Spring Data instead of the Java beans package.

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly Related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.71% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'NAE-2418 Verify @Transient handling across persistence layers in na…' is directly related to the main objective of the changeset, which is implementing NAE-2418 to verify @Transient handling across persistence layers.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Apr 27, 2026
Retoocs
Retoocs previously approved these changes Apr 27, 2026
…e-spring-core-adapter

- created new arc classes in spring core adapter
- refactored existing arc class to be abstract
- added transient to arcs
@renczesstefan renczesstefan dismissed stale reviews from Retoocs and coderabbitai[bot] via 3f362f9 April 28, 2026 10:51
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetEventHandler.java`:
- Around line 21-24: Remove the no-op Spring Data lifecycle hook by deleting the
onAfterLoad(AfterLoadEvent<PetriNet> event) method from the PetriNetEventHandler
class and also remove the unused AfterLoadEvent import; ensure no other code
references onAfterLoad so compilation remains clean.

In
`@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/arcs/Arc.java`:
- Around line 12-14: The copy constructor public Arc(Arc arc) is too narrowly
typed; change its parameter to the core/base arc type (the base Arc
interface/class used by the superclass) so it can accept core arcs, e.g. public
Arc(BaseArcType arc) { super(arc); }, and update the import for that core/base
Arc type; keep the body calling super(arc) unchanged to delegate copying to the
superclass.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d5fe2af9-b04f-42a0-b2dd-3da8dca5d1d3

📥 Commits

Reviewing files that changed from the base of the PR and between 17e7be8 and 3f362f9.

📒 Files selected for processing (10)
  • application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ArcFactory.java
  • application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetEventHandler.java
  • nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/arcs/Arc.java
  • nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/arcs/InhibitorArc.java
  • nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/arcs/ReadArc.java
  • nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/arcs/ResetArc.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/arcs/Arc.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/arcs/InhibitorArc.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/arcs/ReadArc.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/arcs/ResetArc.java

…e-spring-core-adapter

- added annotation to user objects, so process list is transient
…e-spring-core-adapter

Refactored `AbstractActor` to manage roles and authorities with additional `processRoleIds` and `authorityIds` fields for better flexibility. Updated related services, repositories, and XML test resources to align with the modified structure. Introduced `UserMongoEventListener` to resolve roles and authorities from IDs after data loading.
@coderabbitai coderabbitai Bot added improvement A change that improves on an existing feature breaking change Fix or feature that would cause existing functionality doesn't work as expected Large Medium labels May 4, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (2)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/repository/UserRepository.java (1)

187-191: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Migrate this query to processRoleIds as well to avoid inconsistent results.

This method still filters by processRoles._id, while the new persistence path uses denormalized processRoleIds. With transient role objects, this query path can miss valid users.

Suggested fix
 default Page<User> findDistinctByStateAndProcessRoles__idIn(UserState state, Collection<ProcessResourceId> roleId, Pageable pageable, MongoTemplate mongoTemplate, String collection) {
     Query query = Query.query(
             Criteria.where("state").is(state)
-                    .and("processRoles._id").in(roleId));
+                    .and("processRoleIds").in(roleId.stream().map(ProcessResourceId::toString).toList()));
     return resolveUserPage(pageable, mongoTemplate, collection, query);
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@nae-user-common/src/main/java/com/netgrif/application/engine/auth/repository/UserRepository.java`
around lines 187 - 191, The query in findDistinctByStateAndProcessRoles__idIn
still filters on "processRoles._id" which misses users when roles are
denormalized to "processRoleIds"; update the Criteria in that method to query
"processRoleIds" instead (or use an OR Criteria that checks both
"processRoleIds" and "processRoles._id" for backward compatibility) so
resolveUserPage receives a query that finds users by the denormalized role IDs;
ensure the change uses the existing roleId parameter and preserves return via
resolveUserPage.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java (1)

223-233: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix authority ID/object desynchronization in fallback removal path.

If object removal falls back to name-based matching, the removed authority object may not match the originally removed ID, leaving stale entries in authorityIds.

Suggested fix
 public void removeAuthority(Authority authority) {
+    if (authority == null) {
+        return;
+    }
     if (this.authorityIds == null) {
         this.authorityIds = new HashSet<>();
-    } else {
-        this.authorityIds.remove(authority.getStringId());
     }
     if (this.authoritySet == null) {
         this.authoritySet = new HashSet<>();
     } else if (!this.authoritySet.remove(authority)) {
-        this.authoritySet.removeIf(it -> it.getName().equals(authority.getName()));
+        Authority matched = this.authoritySet.stream()
+                .filter(it -> it.getName().equals(authority.getName()))
+                .findFirst()
+                .orElse(null);
+        if (matched != null) {
+            this.authoritySet.remove(matched);
+            this.authorityIds.remove(matched.getStringId());
+        }
+        return;
     }
+    this.authorityIds.remove(authority.getStringId());
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java`
around lines 223 - 233, The removeAuthority(Authority authority) method can
leave stale IDs in authorityIds when it falls back to name-based removal from
authoritySet; update the fallback path so that when you remove by name (in the
removeIf on authoritySet) you also remove the corresponding authority IDs from
authorityIds (e.g., find entries in authorityIds whose Authority.getStringId()
corresponds to the removed objects or remove IDs whose names match
authority.getName()), ensuring null-safe handling of authorityIds and
authoritySet; modify removeAuthority to synchronize both collections in both the
direct remove(authority) branch and the removeIf(name) fallback so IDs and
objects stay 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
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java`:
- Around line 281-289: In addAllProcessRoles, guard against a null roles
argument: check if the incoming Set<ProcessRole> roles is null (or empty) at the
top of the method and return early to avoid calling roles.stream() or
this.processRoles.addAll(roles); ensure existing initialization of
this.processRoleIds and this.processRoles (as already present) remains, then
only perform
this.processRoleIds.addAll(roles.stream().map(ProcessRole::getStringId).toList())
and this.processRoles.addAll(roles) when roles is non-null; reference the method
addAllProcessRoles and the fields processRoleIds/processRoles and class
ProcessRole to locate the code.

In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Group.java`:
- Line 19: The Group class was made abstract but GroupController still
instantiates it with new Group(...), causing a compile error; either revert
Group to a concrete class until the constructor call is migrated, or update the
controller to use the concrete Spring adapter class (the adapter that implements
Group) instead of new Group(...). Locate the Group class declaration (public
abstract class Group extends AbstractActor) and either remove the abstract
modifier or change the instantiation in GroupController (the code that does new
Group(...)) to call the concrete adapter/implementation class that wraps Spring
functionality; ensure constructors/signatures match so compilation succeeds.

In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserMongoEventListener.java`:
- Around line 23-24: The trace logs in UserMongoEventListener currently print
the entire loaded user document via event.getSource(), which can expose
sensitive fields; change both log.trace calls to avoid logging the full payload
and instead log a minimal identifier (e.g., user id or username) and/or omit the
payload entirely — locate the two calls in UserMongoEventListener where
log.trace("User loaded: {}", event.getSource()) and log.trace("Resolving process
roles of user with id: {}", event.getSource()) are used and replace
event.getSource() with a safe accessor like event.getSource().getId() or
event.getSource().getUsername(), or remove the payload argument so only
non-sensitive context is logged.
- Around line 29-31: In UserMongoEventListener, avoid adding null role/authority
objects into actor collections: when iterating actor.getProcessRoleIds() call
processRoleService.findById(processRoleId) and only add the result to
actor.getProcessRoles() if it is non-null (skip otherwise), and similarly when
iterating actor.getAuthorityIds() call authorityService.getOne(authorityId) and
only add to actor.getAuthoritySet() if non-null; update the loops in the block
that uses AbstractActor actor to perform null checks (or filter nulls) before
calling add to prevent polluted collections and NPEs.

---

Outside diff comments:
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java`:
- Around line 223-233: The removeAuthority(Authority authority) method can leave
stale IDs in authorityIds when it falls back to name-based removal from
authoritySet; update the fallback path so that when you remove by name (in the
removeIf on authoritySet) you also remove the corresponding authority IDs from
authorityIds (e.g., find entries in authorityIds whose Authority.getStringId()
corresponds to the removed objects or remove IDs whose names match
authority.getName()), ensuring null-safe handling of authorityIds and
authoritySet; modify removeAuthority to synchronize both collections in both the
direct remove(authority) branch and the removeIf(name) fallback so IDs and
objects stay consistent.

In
`@nae-user-common/src/main/java/com/netgrif/application/engine/auth/repository/UserRepository.java`:
- Around line 187-191: The query in findDistinctByStateAndProcessRoles__idIn
still filters on "processRoles._id" which misses users when roles are
denormalized to "processRoleIds"; update the Criteria in that method to query
"processRoleIds" instead (or use an OR Criteria that checks both
"processRoleIds" and "processRoles._id" for backward compatibility) so
resolveUserPage receives a query that finds users by the denormalized role IDs;
ensure the change uses the existing roleId parameter and preserves return via
resolveUserPage.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7bdab57e-4c74-48c2-86db-e1e6bfd916b9

📥 Commits

Reviewing files that changed from the base of the PR and between 3f362f9 and e3f0867.

📒 Files selected for processing (44)
  • application-engine/src/main/java/com/netgrif/application/engine/auth/service/RegistrationService.java
  • application-engine/src/main/java/com/netgrif/application/engine/configuration/security/PublicAuthenticationFilter.java
  • application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java
  • application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java
  • application-engine/src/test/groovy/com/netgrif/application/engine/action/AssignActionTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/action/RemoveActionTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/auth/LoginAttemptsTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/auth/SecurityContextTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/auth/TaskAuthorizationServiceTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/auth/UserServiceTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticSearchTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/impersonation/ImpersonationServiceTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/insurance/mvc/InsuranceTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/menu/MenuImportExportTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/orgstructure/groups/GroupServiceTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/permissions/ElasticSearchViewPermissionTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/permissions/QueryDSLViewPermissionTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileFieldTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FileListFieldTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/roles/ProcessRoleTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/CachePetriNetServiceTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/web/PetriNetControllerTest.groovy
  • application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy
  • application-engine/src/test/java/com/netgrif/application/engine/MailSenderServiceTest.java
  • application-engine/src/test/java/com/netgrif/application/engine/auth/service/TokenServiceTest.java
  • application-engine/src/test/java/com/netgrif/application/engine/workflow/WorkflowMvcPerformanceTest.java
  • application-engine/src/test/java/com/netgrif/application/engine/workflow/service/AbstractAuthorizationServiceTest.java
  • application-engine/src/test/java/com/netgrif/application/engine/workflow/service/TaskServiceTest.java
  • application-engine/src/test/java/com/netgrif/application/engine/workflow/web/VariableArcsTest.java
  • application-engine/src/test/resources/petriNets/function_test.xml
  • application-engine/src/test/resources/petriNets/function_test_v2.xml
  • nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java
  • nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/Group.java
  • nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/AnonymousUser.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/Group.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/User.java
  • nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/GroupServiceImpl.java
  • nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserMongoEventListener.java
  • nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
  • nae-user-common/src/main/java/com/netgrif/application/engine/auth/repository/UserRepository.java

…e-spring-core-adapter

Replaced explicit imports with wildcard in `AbstractActor` for conciseness. Also adjusted the `addAllProcessRoles` method to use a more general `Collection` type for improved flexibility in handling process roles.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java`:
- Around line 188-190: When replacing denormalized ID sets in setAuthorityIds
and setProcessRoleIds, also clear or reset the corresponding hydrated object
collections so in-memory state doesn't stay stale: in setAuthorityIds(String)
assign this.authorityIds as before and also clear/reset authoritySet (e.g.,
this.authoritySet = new HashSet<>() or this.authoritySet.clear()) and in
setProcessRoleIds(String) do the same for processRoles; preserve the
null-to-empty behavior for IDs and ensure you reference setAuthorityIds,
authorityIds, authoritySet, setProcessRoleIds, processRoleIds and processRoles
when making the changes so isAdmin and other in-memory checks reflect the new
IDs.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 24f773a3-fb6e-42af-b287-66d2ab1d09d3

📥 Commits

Reviewing files that changed from the base of the PR and between e3f0867 and ae51eb2.

📒 Files selected for processing (1)
  • nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java

@renczesstefan renczesstefan requested a review from Retoocs May 4, 2026 13:21
Adjusted the constructor to reference DataGroup with its fully qualified name. This change resolves ambiguity and ensures clarity when dealing with similarly named classes in different packages.
Add null checks to safeguard against potential NullPointerExceptions in methods handling authorities and process roles. This ensures better stability and avoids unexpected crashes when null values are passed as arguments.
@renczesstefan renczesstefan requested a review from Retoocs May 5, 2026 07:20
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java (1)

19-43: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Enforce encapsulation for role/authority collections to preserve ID mirrors.

Because @Getter exposes mutable authoritySet and processRoles, callers can mutate them directly and bypass sync logic for authorityIds/processRoleIds. This is already happening (e.g., UserServiceImpl.java Line 676 and ProcessRoleService.java Lines 322-327), which can leave denormalized IDs stale and break persistence/query behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java`
around lines 19 - 43, AbstractActor currently exposes mutable collections
(authoritySet, processRoles) via Lombok `@Getter` which allows callers to mutate
them and bypass the mirror ID sets (authorityIds, processRoleIds); change the
getters for authoritySet and processRoles to return unmodifiable/defensive
copies and add explicit mutator methods (e.g., addAuthority(Authority),
removeAuthority(Authority), addProcessRole(ProcessRole),
removeProcessRole(ProcessRole)) that update both the object set and the
corresponding ID set (authorityIds/processRoleIds) in a single place; remove or
avoid exposing the raw mutable collections and ensure any existing callers
(e.g., UserServiceImpl and ProcessRoleService usages) use the new mutators or
work with the defensive copies so the denormalized ID mirrors remain consistent.
♻️ Duplicate comments (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java (1)

280-289: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Guard addAllProcessRoles against null input.

At Line 287 and Line 288, roles.stream() / addAll(roles) will throw if roles is null.

Suggested patch
 public void addAllProcessRoles(Collection<ProcessRole> roles) {
+    if (roles == null || roles.isEmpty()) {
+        return;
+    }
     if (this.processRoleIds == null) {
         this.processRoleIds = new HashSet<>();
     }
     if (this.processRoles == null) {
         this.processRoles = new HashSet<>();
     }
-    this.processRoleIds.addAll(roles.stream().map(ProcessRole::getStringId).toList());
-    this.processRoles.addAll(roles);
+    this.processRoleIds.addAll(roles.stream().filter(Objects::nonNull).map(ProcessRole::getStringId).toList());
+    this.processRoles.addAll(roles.stream().filter(Objects::nonNull).toList());
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java`
around lines 280 - 289, The addAllProcessRoles method can NPE when the input
collection roles is null; update addAllProcessRoles to first check if roles is
null (or empty) and return immediately (or treat as empty) before accessing
roles.stream() or calling addAll, keeping initialization of processRoleIds and
processRoles as-is; reference the method name addAllProcessRoles and the fields
processRoleIds and processRoles when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java`:
- Around line 19-43: AbstractActor currently exposes mutable collections
(authoritySet, processRoles) via Lombok `@Getter` which allows callers to mutate
them and bypass the mirror ID sets (authorityIds, processRoleIds); change the
getters for authoritySet and processRoles to return unmodifiable/defensive
copies and add explicit mutator methods (e.g., addAuthority(Authority),
removeAuthority(Authority), addProcessRole(ProcessRole),
removeProcessRole(ProcessRole)) that update both the object set and the
corresponding ID set (authorityIds/processRoleIds) in a single place; remove or
avoid exposing the raw mutable collections and ensure any existing callers
(e.g., UserServiceImpl and ProcessRoleService usages) use the new mutators or
work with the defensive copies so the denormalized ID mirrors remain consistent.

---

Duplicate comments:
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java`:
- Around line 280-289: The addAllProcessRoles method can NPE when the input
collection roles is null; update addAllProcessRoles to first check if roles is
null (or empty) and return immediately (or treat as empty) before accessing
roles.stream() or calling addAll, keeping initialization of processRoleIds and
processRoles as-is; reference the method name addAllProcessRoles and the fields
processRoleIds and processRoles when making the change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: db263d5d-2f35-4736-a039-f75c1090de66

📥 Commits

Reviewing files that changed from the base of the PR and between ae51eb2 and 5699473.

📒 Files selected for processing (8)
  • application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetEventHandler.java
  • nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractActor.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/arcs/Arc.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/arcs/InhibitorArc.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/arcs/ReadArc.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/petrinet/domain/arcs/ResetArc.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/workflow/domain/DataGroup.java
  • nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserMongoEventListener.java
💤 Files with no reviewable changes (1)
  • application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetEventHandler.java

Introduced copy constructors for `Arc`, `ReadArc`, `ResetArc`, and `InhibitorArc` classes to allow object duplication. This enhances flexibility and simplifies operations involving the creation of identical arc instances.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Fix or feature that would cause existing functionality doesn't work as expected improvement A change that improves on an existing feature Large Medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants