[NAE-2185] Align plugin property handling and metadata registration with architecture standards #347
Conversation
…ith architecture standards - Added a `metadata` property to the `Plugin` class, represented as a `LinkedHashMap` to maintain insertion order. - Updated the `Plugin` constructor to include the new `metadata` property. - Ensured proper initialization of the `metadata` map in the default `Plugin` constructor. - Improved Javadoc formatting for consistency and readability.
WalkthroughParent POM versions updated from 7.0.0-RC6 to 7.0.0-SNAPSHOT across modules. The Plugin domain class gained a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
added
the
improvement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java (2)
3-5: Prevent leaking metadata in logs and avoid equality/identity drift@DaTa includes metadata in toString/equals/hashCode. If metadata may contain sensitive values or is not part of Plugin identity, exclude it.
Apply:
import lombok.Builder; import lombok.Data; +import lombok.EqualsAndHashCode; +import lombok.ToString; @@ - private Map<String, String> metadata; + @ToString.Exclude + @EqualsAndHashCode.Exclude + private Map<String, String> metadata;Also applies to: 44-45
51-63: Ensure builder defaults maps to empty collections to prevent NPE
The Lombok-generated @builder constructor assignsentryPointsandmetadatadirectly, so callers that omit those values get nulls—unlike the no-arg constructor, which initializes both to non-null collections. To align behaviors and eliminate NPE risk, default them as follows:--- a/nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java +++ b/nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java @@ public Plugin(String identifier, String name, String version, String description, String url, int restPort, int grpcPort, Map<String, EntryPoint> entryPoints, boolean active, Map<String, String> metadata) { - this.entryPoints = entryPoints; + this.entryPoints = entryPoints != null + ? entryPoints + : new HashMap<>(); this.active = active; - this.metadata = metadata; + this.metadata = metadata != null + ? metadata + : new LinkedHashMap<>();• File:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java
• Replace lines 60–63 in the @builder constructor.
♻️ Duplicate comments (4)
nae-user-common/pom.xml (1)
9-9: Same SNAPSHOT concern as root module.
Mirrors the parent version change; ensure consistency with release strategy and resolvability of the parent SNAPSHOT.nae-spring-core-adapter/pom.xml (1)
10-10: Same SNAPSHOT concern as root module.
Parent set to 7.0.0-SNAPSHOT on a release branch; please confirm intent and availability.nae-object-library/pom.xml (1)
10-10: Same SNAPSHOT concern as root module.
Confirm that 7.0.0-SNAPSHOT parent is available during CI and local builds; consider pinning if this branch is for a cut.nae-user-ce/pom.xml (1)
9-9: Same SNAPSHOT concern as root module.
Ensure parent 7.0.0-SNAPSHOT is intentionally used on release/7.0.0-rev6.1 and won’t break reproducibility.
🧹 Nitpick comments (2)
application-engine/pom.xml (1)
9-9: Confirm SNAPSHOT on a release branch.
Parent bumped to 7.0.0-SNAPSHOT while targeting release/7.0.0-rev6.1. Verify this is intentional, the parent SNAPSHOT is resolvable (in reactor or OSSRH snapshots), and that reproducibility expectations are met; otherwise pin a released tag for the parent.nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java (1)
8-8: Consistent iteration order: prefer LinkedHashMap for entryPoints as wellDefault ctor uses HashMap for entryPoints and LinkedHashMap for metadata. For deterministic iteration/serialization, align both to LinkedHashMap.
-import java.util.HashMap; @@ - entryPoints = new HashMap<>(); + entryPoints = new LinkedHashMap<>();(Note: remove the now-unused HashMap import.)
Also applies to: 46-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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 (7)
application-engine/pom.xml(1 hunks)nae-object-library/pom.xml(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java(3 hunks)nae-spring-core-adapter/pom.xml(1 hunks)nae-user-ce/pom.xml(1 hunks)nae-user-common/pom.xml(1 hunks)pom.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementBody.java (1)
Builder(13-88)
⏰ 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 (1)
pom.xml (1)
9-9: Confirm SNAPSHOT usage on this branchAll consistency checks passed successfully:
- No remaining references to
7.0.0-RC6were found in anypom.xml.- Every module’s
<parent>now uniformly specifies version7.0.0-SNAPSHOT.Next steps:
- Verify that using a SNAPSHOT on this branch is intentional. SNAPSHOT versions:
- Are not eligible for publishing to Maven Central via the
ossrh-publishprofile.- Can break reproducible builds that depend on immutable, numbered releases.
- If this is meant to be a final release branch, update to a non-SNAPSHOT version (e.g.
<version>7.0.0</version>).- Otherwise, document that this branch is for ongoing pre-release iterations and adjust your CI/CD/publishing pipelines accordingly.
…ith architecture standards - Replaced `HashSet` with `LinkedHashSet` in `changeType` method to preserve insertion order when transforming user collections. - Ensured consistent behavior by maintaining predictable iteration order in the returned `PageImpl` object.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (1)
201-210: Fix SLF4J placeholder/argument mismatch.Second placeholder logs third-party auth but
authMethodisn’t passed.- log.info("User [{}] from third-party [{}] successfully created in realm [{}]", username, realmId); + log.info("User [{}] from third-party [{}] successfully created in realm [{}]", username, authMethod, realmId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java(1 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). (2)
- GitHub Check: Test
- GitHub Check: task-list-completed
🔇 Additional comments (1)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (1)
625-627: Verify Group and AbstractUser APIs before refactoringI wasn’t able to locate the
GrouporAbstractUserdomain types in the local module, so please confirm the following before applying this change:
- That
com.netgrif.application.engine.auth.domain.Group(or equivalent) exposes a method
Set<SomeRoleType> getProcessRoles()
rather than justgetAuthoritySet().- That
com.netgrif.application.engine.auth.domain.AbstractUser(or equivalent) has a mutable
processRolescollection accessible viagetProcessRoles()(and that it’s initialized).Once you’ve verified those APIs, the implementation in
UserServiceImpl.resolveRelatedProcessRolesshould indeed populate the user’s process roles, not authorities:--- a/nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java +++ b/nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java @@ -625,3 +625,7 @@ private void resolveRelatedProcessRoles(AbstractUser user) { - user.getAuthoritySet().addAll( - getUserGroups(user).stream() - .map(Group::getAuthoritySet) - .flatMap(Set::stream) - .collect(Collectors.toSet()) - ); + user.getProcessRoles().addAll( + getUserGroups(user).stream() + .map(Group::getProcessRoles) + .flatMap(Set::stream) + .collect(Collectors.toSet()) + ); }Please verify these APIs and update accordingly.
| private <T> Page<AbstractUser> changeType(Page<T> users, Pageable pageable) { | ||
| return new PageImpl<>(changeType(new HashSet<>(users.getContent())), pageable, users.getTotalElements()); | ||
| return new PageImpl<>(changeType(new LinkedHashSet<>(users.getContent())), pageable, users.getTotalElements()); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Order-preserving de-dupe is good, but totalElements now lies.
De-duplicating the page with LinkedHashSet can shrink page size while leaving users.getTotalElements() unchanged, yielding inconsistent pagination and wrong last/last-but-one pages. Prefer pushing distinctness into the repository/count query; as a stopgap, at least return the de-duped count here.
Apply quick fix:
- return new PageImpl<>(changeType(new LinkedHashSet<>(users.getContent())), pageable, users.getTotalElements());
+ List<AbstractUser> unique = changeType(new LinkedHashSet<>(users.getContent()));
+ return new PageImpl<>(unique, pageable, unique.size());Follow-up (preferred): update repository methods to return distinct users and an aligned distinct-count, so totals reflect the real result set.
🏁 Script executed:
#!/bin/bash
# Display lines 580–620 around the changeType(Page) method in UserServiceImpl
sed -n '580,620p' nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.javaLength of output: 1773
Fix pagination total after de-duplication
De-duplicating the page content with LinkedHashSet can reduce the number of items returned on a page while still passing the original users.getTotalElements() into PageImpl, causing inconsistent pagination (incorrect total pages, wrong last page, etc.). At minimum, update this method to return the de-duplicated count; ideally, push distinctness and correct counting down into the repository layer.
• File: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
Method: private <T> Page<AbstractUser> changeType(Page<T> users, Pageable pageable)
Suggested quick fix:
- return new PageImpl<>(changeType(new LinkedHashSet<>(users.getContent())), pageable, users.getTotalElements());
+ List<AbstractUser> unique = changeType(new LinkedHashSet<>(users.getContent()));
+ return new PageImpl<>(unique, pageable, unique.size());Follow-up (preferred): update the repository queries to return distinct users and an aligned count, so totalElements accurately reflects the actual result set.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private <T> Page<AbstractUser> changeType(Page<T> users, Pageable pageable) { | |
| return new PageImpl<>(changeType(new HashSet<>(users.getContent())), pageable, users.getTotalElements()); | |
| return new PageImpl<>(changeType(new LinkedHashSet<>(users.getContent())), pageable, users.getTotalElements()); | |
| } | |
| private <T> Page<AbstractUser> changeType(Page<T> users, Pageable pageable) { | |
| List<AbstractUser> unique = changeType(new LinkedHashSet<>(users.getContent())); | |
| return new PageImpl<>(unique, pageable, unique.size()); | |
| } |
🤖 Prompt for AI Agents
In
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
around lines 595-597, changeType currently builds a de-duplicated content page
but still passes users.getTotalElements() into PageImpl which yields incorrect
pagination; fix it by computing the de-duplicated content list/set first, derive
the correct totalElements from that de-duplicated collection size and pass that
value into the PageImpl constructor; ideally, as a follow-up, modify the
repository queries to return distinct users and an accurate count so pagination
is correct at the query level.
There was a problem hiding this comment.
Total elements count expresses total counts of the same entities that satisfied the previously provided filter and so unique.size() is to the correct value to use there.
There was a problem hiding this comment.
@tuplle Thank you for confirming! You're absolutely right - after deduplication, unique.size() correctly represents the actual count of entities that satisfied the filter criteria, ensuring pagination consistency. Using the original users.getTotalElements() would indeed lead to incorrect pagination behavior since it wouldn't reflect the deduplicated result set.
The suggested fix ensures that the PageImpl constructor receives accurate information about both the content and the total count, maintaining proper pagination semantics.
🐰💭
╭─────╮
│ ✓ │ Confirmation appreciated!
╰─────╯
Description
Implements NAE-2185
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
Improvements
Chores