[NAE-2316] Case permissions are not propagated correctly in cloud deployment #402
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
…loyment - added permissions, users and userRefs attributes of case and ElasticCase to copy constructor of ElasticCase
SamuelPalaj
requested review from
Retoocs,
machacjozef,
renczesstefan and
tuplle
WalkthroughElasticCase now initializes and updates nested collections/maps using null‑safe new instances and deep copies; many elastic domain field classes received copy constructors; ImmediateField gained a constructor accepting explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 0
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/elastic/domain/ElasticCase.java (1)
112-129: Critical: update() method must also refresh permissions, users, and userRefs.The constructor now initializes
permissions,users, anduserRefs(lines 105-107), but theupdate()method does not refresh these maps when synchronizing state. This creates an inconsistency where:
- Initial ElasticCase creation copies permissions correctly
- Subsequent updates via
update()leave permissions stale- Permission changes in the source system won't propagate to Elasticsearch
Since this PR specifically addresses permission propagation issues (NAE-2316), omitting these fields from
update()means the bug is only partially fixed.🔒 Proposed fix to synchronize permission maps in update()
tags = useCase.getTags(); + permissions = useCase.getPermissions(); + users = useCase.getUsers(); + userRefs = useCase.getUserRefs(); dataSet = useCase.getDataSet();
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.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). (8)
- 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
- GitHub Check: task-list-completed
🔇 Additional comments (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (1)
105-107: LGTM! Constructor now properly initializes permission maps.The addition of permissions, users, and userRefs initialization in the constructor correctly addresses the permission propagation issue described in the PR objective. The defensive copy pattern is appropriate and consistent with other map initializations in this constructor.
…loyment - added permissions, users and userRefs to update method of ElasticCase
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:
- Around line 126-128: The fields permissions, users and userRefs in ElasticCase
are not defensively initialized and can be set to null by the update logic;
change their declarations to initialize them to empty HashMap instances (i.e.,
initialize permissions, users and userRefs at declaration to new HashMap<>()) so
they are never null and match the constructor's defensive pattern used
elsewhere.
- Around line 105-107: The constructor in ElasticCase sets permissions, users,
and userRefs without null checks which can cause NPEs (unlike tasks which uses a
null-safe pattern); update the assignments in the ElasticCase constructor to use
null checks and initialize to empty HashMap when
useCase.getPermissions()/getUsers()/getUserRefs() is null (e.g., ternary
checking each getter and creating new HashMap<>(...) when non-null or new
HashMap<>() when null), keeping in mind this remains a shallow copy consistent
with tags handling.
- Around line 105-107: In update(), avoid assigning permissions, users, and
userRefs by reference; mirror the constructor’s defensive-copy behavior by
creating new HashMap instances from useCase.getPermissions(),
useCase.getUsers(), and useCase.getUserRefs() (i.e., set permissions = new
HashMap<>(useCase.getPermissions()), users = new HashMap<>(useCase.getUsers()),
userRefs = new HashMap<>(useCase.getUserRefs())); also add unit tests for
ElasticCase (or relevant behavior) into the application-engine test suite to
verify defensive copying prevents external mutation.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java:336-343
Timestamp: 2025-12-19T07:44:48.019Z
Learning: In the netgrif/application-engine repository, when a Case is created, the `Case.populateDataSetBehaviorAndComponents()` method automatically copies all transition-level behavior and components to the case-level data fields, keyed by transition ID. This means methods like `isDataFieldEditable` only need to check case-level behavior (dataField.getBehavior().get(transId)), without fallback to transition.getDataSet(), since the behavior has already been copied during case initialization.
📚 Learning: 2025-12-19T07:44:48.019Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java:336-343
Timestamp: 2025-12-19T07:44:48.019Z
Learning: In the netgrif/application-engine repository, when a Case is created, the `Case.populateDataSetBehaviorAndComponents()` method automatically copies all transition-level behavior and components to the case-level data fields, keyed by transition ID. This means methods like `isDataFieldEditable` only need to check case-level behavior (dataField.getBehavior().get(transId)), without fallback to transition.getDataSet(), since the behavior has already been copied during case initialization.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.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). (4)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
...library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Outdated
Show resolved
Hide resolved
...library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Outdated
Show resolved
Hide resolved
tuplle
changed the base branch from
release/7.0.0-rev10
to
release/7.0.0-rev9
…loyment - added defensive copying to update method for better consistency with constructor
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/elastic/domain/ElasticCase.java (2)
112-131:update()still risks aliasing (nested maps + elements) and can NPE onversion++/ null collections
version++will throw ifversionis null.new HashSet<>(useCase.getX())/new HashMap<>(useCase.getX())will throw if any getter returns null.permissions/users/userRefsare still shallow-copied (inner maps shared).immediateData = new ArrayList<>(...)copies the list, but not necessarily theImmediateFieldelements (constructor does element-copy).Proposed fix (null-safe increment + reuse deep-copy helper + consistent immediateData copy)
@@ public void update(ElasticCase useCase) { - version++; + version = (version == null) ? 1L : (version + 1); lastModified = useCase.getLastModified(); title = useCase.getTitle(); - taskIds = new HashSet<>(useCase.getTaskIds()); + taskIds = useCase.getTaskIds() == null ? new HashSet<>() : new HashSet<>(useCase.getTaskIds()); @@ - tags = new HashMap<>(useCase.getTags()); - permissions = new HashMap<>(useCase.getPermissions()); - users = new HashMap<>(useCase.getUsers()); - userRefs = new HashMap<>(useCase.getUserRefs()); + tags = useCase.getTags() == null ? new HashMap<>() : new HashMap<>(useCase.getTags()); + permissions = copyNestedBoolMap(useCase.getPermissions()); + users = copyNestedBoolMap(useCase.getUsers()); + userRefs = copyNestedBoolMap(useCase.getUserRefs()); dataSet = new HashMap<>(useCase.getDataSet()); - immediateData = new ArrayList<>(useCase.getImmediateData()); + immediateData = useCase.getImmediateData() == null + ? new ArrayList<>() + : useCase.getImmediateData().stream().map(ImmediateField::new).collect(Collectors.toList()); }
82-110: Shallow copy of nested maps in constructor + NPE risk from uninitializedversionfieldLines 105–107 create shallow copies of the nested permission maps: the outer
HashMapis copied, but the innerMap<String, Boolean>instances are shared with the source, allowing mutations to leak across copies.Additionally, the
versionfield (line 28, declared asLong) is never initialized in the constructor, so callingversion++at line 113 in theupdate()method will throw aNullPointerException.Suggested fix for nested map shallow copy
- permissions = new HashMap<>(useCase.getPermissions()); - users = new HashMap<>(useCase.getUsers()); - userRefs = new HashMap<>(useCase.getUserRefs()); + permissions = copyNestedBoolMap(useCase.getPermissions()); + users = copyNestedBoolMap(useCase.getUsers()); + userRefs = copyNestedBoolMap(useCase.getUserRefs()); + + private static Map<String, Map<String, Boolean>> copyNestedBoolMap(Map<String, Map<String, Boolean>> source) { + if (source == null || source.isEmpty()) { + return new HashMap<>(); + } + Map<String, Map<String, Boolean>> copy = new HashMap<>(source.size()); + source.forEach((k, v) -> copy.put(k, v == null ? null : new HashMap<>(v))); + return copy; + }For the
versionfield, initialize it to0Lin the constructor (or use@Datawith@Builderto ensure proper initialization).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-12-19T07:44:48.019Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java:336-343
Timestamp: 2025-12-19T07:44:48.019Z
Learning: In the netgrif/application-engine repository, when a Case is created, the `Case.populateDataSetBehaviorAndComponents()` method automatically copies all transition-level behavior and components to the case-level data fields, keyed by transition ID. This means methods like `isDataFieldEditable` only need to check case-level behavior (dataField.getBehavior().get(transId)), without fallback to transition.getDataSet(), since the behavior has already been copied during case initialization.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
…loyment - pom version revert
tuplle
previously approved these changes
Jan 12, 2026
…loyment - added creation of deep copy to permissions, users and userRefs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:
- Around line 133-138: The deepCopy method can NPE when an entry's inner map
value is null; update deepCopy (the private static Map<String, Map<String,
Boolean>> deepCopy(Map<String, Map<String, Boolean>> map) method) to guard
against null inner maps by replacing null with an empty map when copying (e.g.,
for each Map.Entry use e.getValue() == null ? new HashMap<>() : new
HashMap<>(e.getValue())); keep the existing null/empty check for the outer map
and preserve keys while ensuring no new HashMap is constructed from a null
value.
- Line 118: The current assignment "tasks = new HashSet<>(useCase.getTasks())"
makes only a shallow copy of the Set and can still share mutable ElasticTaskPair
instances; change it to perform a deep copy of the elements (create new
ElasticTaskPair instances from each element of useCase.getTasks()) so
ElasticCase's tasks contains independent copies—use the same cloning/constructor
pattern used earlier (e.g., map each task to new ElasticTaskPair(task) and
collect into a new Set) to keep behavior consistent with the constructor's
deep-copy approach.
- Around line 116-130: The update() method (and the corresponding constructor
that mirrors it) wraps many collection fields like taskIds, taskMongoIds, tasks,
enabledRoles, viewRoles, viewUserRefs, negativeViewRoles, viewUsers,
negativeViewUsers, tags, dataSet, immediateData with new HashSet/HashMap without
null checks, which can NPE after deserialization; change each assignment to
defensively handle null (e.g., if useCase.getX() != null then new
HashSet<>(useCase.getX()) else new HashSet<>() or new HashMap<>()), and mirror
the same null-safe pattern used by deepCopy() for permissions, users, and
userRefs so all collections are consistently non-null after construction/update.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-12-19T07:44:48.019Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java:336-343
Timestamp: 2025-12-19T07:44:48.019Z
Learning: In the netgrif/application-engine repository, when a Case is created, the `Case.populateDataSetBehaviorAndComponents()` method automatically copies all transition-level behavior and components to the case-level data fields, keyed by transition ID. This means methods like `isDataFieldEditable` only need to check case-level behavior (dataField.getBehavior().get(transId)), without fallback to transition.getDataSet(), since the behavior has already been copied during case initialization.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.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). (1)
- GitHub Check: Test
🔇 Additional comments (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (2)
105-107: Core fix looks correct.Deep copying
permissions,users, anduserRefsproperly isolates the nested map structures, preventing unintended shared state that was causing the permission propagation bug in cloud deployment.
126-128: Consistent application of the fix in update method.The same deep copy pattern is correctly applied here, ensuring updates also maintain isolated state for permissions.
...library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Outdated
Show resolved
Hide resolved
...library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Show resolved
Hide resolved
…loyment - null check added
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:
- Around line 129-130: The copies for dataSet and immediateData can NPE; guard
them like the other collection copies by checking useCase.getDataSet() and
useCase.getImmediateData() and initializing dataSet to an empty HashMap and
immediateData to an empty ArrayList when the getters return null—update the
ElasticCase constructor (or the method copying from UseCase) to use conditional
initialization for the dataSet and immediateData fields.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-12-19T07:44:48.019Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java:336-343
Timestamp: 2025-12-19T07:44:48.019Z
Learning: In the netgrif/application-engine repository, when a Case is created, the `Case.populateDataSetBehaviorAndComponents()` method automatically copies all transition-level behavior and components to the case-level data fields, keyed by transition ID. This means methods like `isDataFieldEditable` only need to check case-level behavior (dataField.getBehavior().get(transId)), without fallback to transition.getDataSet(), since the behavior has already been copied during case initialization.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-12-19T07:55:13.427Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java:239-247
Timestamp: 2025-12-19T07:55:13.427Z
Learning: In the netgrif/application-engine project, the `taskService.findOne()` method (from ITaskService) never returns null. When a task is not found, it handles the error through other means (such as throwing an exception) rather than returning null, so null checks after `findOne()` calls are unnecessary.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
🔇 Additional comments (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (2)
105-107: LGTM — Deep copy ensures permission isolation.Using
deepCopy()for the nested permission maps correctly prevents shared mutable state between the sourceCaseand the constructedElasticCase. This addresses the root cause of NAE-2316.
133-138: LGTM — Clean null-safe deep copy implementation.The helper correctly handles null/empty outer maps and null inner map values, returning fresh mutable copies at both levels.
tuplle
previously approved these changes
Jan 12, 2026
Retoocs
previously approved these changes
Jan 12, 2026
renczesstefan
requested changes
Jan 13, 2026
...library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Outdated
Show resolved
Hide resolved
…loyment - deep copying of tasks in update method added
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:
- Around line 116-117: The update() method assumes source collections are
non-null and can NPE; change each assignment in update() (e.g., taskIds,
taskMongoIds and the other collection fields around lines 121-127 and 131) to
defensively handle nulls by using a null-coalescing pattern: wrap useCase.getX()
with (useCase.getX() == null ? Collections.emptySet() : useCase.getX()) (or
Collections.emptyList() as appropriate) before constructing new
HashSet/ArrayList, so new HashSet<>(useCase.getTaskIds() == null ?
Collections.emptySet() : useCase.getTaskIds()) etc.; apply the same pattern for
all collection copies in update().
- Line 132: The update() method in ElasticCase performs a shallow copy of
immediateData causing shared ImmediateField/I18nString instances; change the
update(ElasticCase) implementation to deep-copy immediateData the same way the
constructor does (i.e., replace new ArrayList<>(useCase.getImmediateData()) with
a stream/map that constructs new ImmediateField(...) for each entry, ensuring
each ImmediateField's I18nString name is copied via its copy constructor or
factory if available) so updated ElasticCase gets independent ImmediateField and
I18nString instances.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 402
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:118-118
Timestamp: 2026-01-13T06:40:13.919Z
Learning: In ElasticCase (nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java), the tasks field containing ElasticTaskPair instances should be deep copied (creating new ElasticTaskPair instances) rather than shallow copied, both in the constructor and the update() method, to prevent sharing mutable references.
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2026-01-13T06:40:13.919Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 402
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:118-118
Timestamp: 2026-01-13T06:40:13.919Z
Learning: In ElasticCase (nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java), the tasks field containing ElasticTaskPair instances should be deep copied (creating new ElasticTaskPair instances) rather than shallow copied, both in the constructor and the update() method, to prevent sharing mutable references.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-12-19T07:44:48.019Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java:336-343
Timestamp: 2025-12-19T07:44:48.019Z
Learning: In the netgrif/application-engine repository, when a Case is created, the `Case.populateDataSetBehaviorAndComponents()` method automatically copies all transition-level behavior and components to the case-level data fields, keyed by transition ID. This means methods like `isDataFieldEditable` only need to check case-level behavior (dataField.getBehavior().get(transId)), without fallback to transition.getDataSet(), since the behavior has already been copied during case initialization.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-12-19T07:55:13.427Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java:239-247
Timestamp: 2025-12-19T07:55:13.427Z
Learning: In the netgrif/application-engine project, the `taskService.findOne()` method (from ITaskService) never returns null. When a task is not found, it handles the error through other means (such as throwing an exception) rather than returning null, so null checks after `findOne()` calls are unnecessary.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.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 (4)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (4)
105-107: LGTM! Deep copy of permission maps correctly addresses NAE-2316.Using
deepCopy()for the nestedpermissions,users, anduserRefsmaps ensures that both the outer and inner maps are cloned, preventing shared mutable state between the sourceCaseand the constructedElasticCase.
118-120: LGTM! Tasks are now correctly deep copied.Creating new
ElasticTaskPairinstances ensures that mutable task references are not shared between the source and targetElasticCaseobjects. This aligns with the learning that tasks should be deep copied in both the constructor and update method.
135-140: LGTM! Well-implemented deep copy utility.The method correctly handles null/empty outer maps and null inner maps, ensuring a fully independent copy of the nested map structure is created.
128-130: Consistent deep copy usage in update method.Using
deepCopy()forpermissions,users, anduserRefsin the update path ensures the permission propagation fix is complete across all mutation scenarios.
...library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Outdated
Show resolved
Hide resolved
...library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Outdated
Show resolved
Hide resolved
…loyment - deep copying of ImmediateField in update method added
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:
- Around line 117-128: The code assigns many collections from useCase directly
which can NPE; make these null-safe like the existing tasks handling by
replacing direct new HashSet<>(useCase.getX()) / new
HashMap<>(useCase.getTags()) with null checks or conditional expressions that
default to empty collections when getters return null (e.g., taskIds =
useCase.getTaskIds() == null ? new HashSet<>() : new
HashSet<>(useCase.getTaskIds()); same for taskMongoIds, enabledRoles, viewRoles,
viewUserRefs, negativeViewRoles, viewUsers, negativeViewUsers, and tags using
new HashMap<>()), and apply the same null-safe pattern used for
immediateData/getDataSet() where relevant.
- Line 132: The shallow copy of dataSet in ElasticCase (dataSet = new
HashMap<>(useCase.getDataSet())) leaves mutable DataField instances shared;
replace this with a deep copy by iterating useCase.getDataSet() and mapping each
key to a cloned DataField (use DataField.deepCopy() or construct new DataField
from the original) so ElasticCase.dataSet contains independent DataField
instances similar to how tasks/permissions/users/userRefs were handled.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ImmediateField.java
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 402
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:118-118
Timestamp: 2026-01-13T06:40:13.919Z
Learning: In ElasticCase (nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java), the tasks field containing ElasticTaskPair instances should be deep copied (creating new ElasticTaskPair instances) rather than shallow copied, both in the constructor and the update() method, to prevent sharing mutable references.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
📚 Learning: 2026-01-13T06:40:13.919Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 402
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:118-118
Timestamp: 2026-01-13T06:40:13.919Z
Learning: In ElasticCase (nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java), the tasks field containing ElasticTaskPair instances should be deep copied (creating new ElasticTaskPair instances) rather than shallow copied, both in the constructor and the update() method, to prevent sharing mutable references.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-12-19T07:44:48.019Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java:336-343
Timestamp: 2025-12-19T07:44:48.019Z
Learning: In the netgrif/application-engine repository, when a Case is created, the `Case.populateDataSetBehaviorAndComponents()` method automatically copies all transition-level behavior and components to the case-level data fields, keyed by transition ID. This means methods like `isDataFieldEditable` only need to check case-level behavior (dataField.getBehavior().get(transId)), without fallback to transition.getDataSet(), since the behavior has already been copied during case initialization.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:13:40.087Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
🔇 Additional comments (4)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ImmediateField.java (1)
29-33: LGTM!The new constructor provides a clean way to create
ImmediateFieldinstances with explicit values, supporting the deep copy requirements inElasticCase.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (3)
106-108: LGTM!Deep copying
permissions,users, anduserRefsprevents shared mutable references between the sourceCaseand the newElasticCaseinstance. This correctly addresses the permission propagation issue.
138-143: LGTM!The
deepCopyhelper correctly handles null/empty maps and creates new instances for both outer and inner maps, ensuring complete isolation of the nested permission structures.
133-135: No action needed. TheI18nStringclass has a proper copy constructor (lines 40-44) that accepts anotherI18nStringinstance and performs a deep copy of all fields including the translations map. The code at lines 133-135 correctly creates a copy of theI18nStringobject and preserves all localization data.
...library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Outdated
Show resolved
Hide resolved
...library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Outdated
Show resolved
Hide resolved
…loyment - deep copying of DataSet through reflection in update method of ElasticCase added - copy constructors added do DataField and its subclasses
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 (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/TextField.java (1)
25-28: Consider defensive copying for consistency.The existing constructor assigns the array reference directly without cloning, while the new copy constructor defensively copies. If a caller retains and modifies the passed array,
textValuewould unexpectedly change.♻️ Suggested defensive copy for consistency
public TextField(String[] values) { super(values); - this.textValue = values; + this.textValue = values == null ? null : values.clone(); }nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/UserListField.java (1)
16-30: Remove duplicate field declarations that shadow parent class fields.
UserListFieldshadows theusernameValue,fullNameValue,userIdValue, anduserRealmIdValuefields inherited fromUserField. This creates two separate copies of each field in the class hierarchy, leading to data inconsistency. The copy constructor correctly implements the duplicated pattern, but the pattern itself is the problem.Remove the four field declarations from
UserListField(lines 16-22) and rely solely on the inherited fields fromUserField. The copy constructor logic will continue to work correctly through the parent class fields.
🤖 Fix all issues with AI agents
In
@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:
- Around line 110-111: In the ElasticCase constructor, guard against null from
useCase.getImmediateData() like update() does, and ensure ImmediateField is
constructed the same way in update(): when mapping useCase.getImmediateData()
pass field.getType().getName() (string) instead of field.getType() to match the
ImmediateField(copy) constructor that expects a type name; update both the
constructor mapping (use null-check and map to new ImmediateField(...) or use
the copy constructor consistently) and the update() mapping to use
field.getType().getName() for consistency with ImmediateField.
- Around line 133-134: When building dataSet from useCase.getDataSet(), guard
against null entry values to avoid NPE from entry.getValue().getClass(); update
the stream that populates dataSet (the expression assigning dataSet) to filter
out entries with entry.getValue() == null or map them to null copies: e.g. add
.filter(entry -> entry.getValue() != null) before the collect, or in the
value-mapping use a ternary that returns null when entry.getValue() is null and
otherwise calls CopyConstructorUtil.copy(entry.getValue().getClass(),
entry.getValue())); keep the rest of the logic and null fallback for
useCase.getDataSet() unchanged.
In
@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java:
- Around line 22-24: In MapField assignation for keyValueTranslations, remove
the redundant new HashMap<>() wrapper around the stream+Collectors.toMap(...)
result; instead set this.keyValueTranslations to null when
field.keyValueTranslations is null or directly to the Map produced by
field.keyValueTranslations.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey,
entry -> new I18nString(entry.getValue())))); keep the existing null-check and
I18nString mapping but drop the extra HashMap copy to avoid the unnecessary
intermediate allocation.
In
@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/UserListField.java:
- Line 6: Remove the unused import of java.util.Arrays from the UserListField
class header; locate the import statement in the UserListField.java file and
delete the line "import java.util.Arrays;" since array copying is handled by the
parent class and no Arrays methods are referenced in UserListField.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (29)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/BooleanField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ButtonField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/DataField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/DateField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/FieldWithAllowedNetsField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/FileField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/FilterField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/NumberField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/StringCollectionField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/TextField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/UserField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/UserListField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/BooleanField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ButtonField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/CaseField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/DateField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/FileField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/FilterField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/I18nField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/NumberField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/StringCollectionField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/TextField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/UserField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/UserListField.java
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 402
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:118-118
Timestamp: 2026-01-13T06:40:13.919Z
Learning: In ElasticCase (nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java), the tasks field containing ElasticTaskPair instances should be deep copied (creating new ElasticTaskPair instances) rather than shallow copied, both in the constructor and the update() method, to prevent sharing mutable references.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
📚 Learning: 2026-01-13T06:40:13.919Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 402
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:118-118
Timestamp: 2026-01-13T06:40:13.919Z
Learning: In ElasticCase (nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java), the tasks field containing ElasticTaskPair instances should be deep copied (creating new ElasticTaskPair instances) rather than shallow copied, both in the constructor and the update() method, to prevent sharing mutable references.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/UserField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/FilterField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/FileField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/I18nField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/DateField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/UserField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/UserListField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/CaseField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/FilterField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/FieldWithAllowedNetsField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/StringCollectionField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/TextField.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/CaseField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/DataField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:13:40.087Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/CaseField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/DataField.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-12-19T07:44:48.019Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java:336-343
Timestamp: 2025-12-19T07:44:48.019Z
Learning: In the netgrif/application-engine repository, when a Case is created, the `Case.populateDataSetBehaviorAndComponents()` method automatically copies all transition-level behavior and components to the case-level data fields, keyed by transition ID. This means methods like `isDataFieldEditable` only need to check case-level behavior (dataField.getBehavior().get(transId)), without fallback to transition.getDataSet(), since the behavior has already been copied during case initialization.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-12-19T07:55:13.427Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java:239-247
Timestamp: 2025-12-19T07:55:13.427Z
Learning: In the netgrif/application-engine project, the `taskService.findOne()` method (from ITaskService) never returns null. When a task is not found, it handles the error through other means (such as throwing an exception) rather than returning null, so null checks after `findOne()` calls are unnecessary.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
🧬 Code graph analysis (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/StringCollectionField.java (3)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/DataField.java (1)
Data(11-34)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/FieldWithAllowedNetsField.java (1)
Data(11-32)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/TextField.java (1)
Data(7-29)
⏰ 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 (31)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/FieldWithAllowedNetsField.java (1)
18-21: Copy constructor implementation looks correct.The use of
Arrays.copyOffor theString[]is appropriate sinceStringis immutable, ensuring a proper shallow copy of the array is sufficient. The null-safe handling ofallowedNetsis good.One minor consideration: the constructor does not guard against
fielditself beingnull. If a null argument is passed, bothsuper(field)andfield.allowedNetswill throwNullPointerException. This aligns with typical copy constructor semantics (caller responsibility), but adding a null check orObjects.requireNonNull(field)would make the failure explicit. Based on learnings, this pattern is consistent with the deep-copy approach used across ElasticCase and related fields.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/NumberField.java (1)
15-18: LGTM!The copy constructor correctly delegates to the superclass and copies the
numberValuefield. SinceDoubleis immutable, direct assignment is safe and appropriate for deep-copy semantics.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/NumberField.java (1)
12-14: LGTM!The copy constructor correctly delegates to the superclass. Since this adapter class only adds Elasticsearch
@Fieldannotations without introducing new fields, no additional copying is needed.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/StringCollectionField.java (1)
17-20: LGTM! Copy constructor correctly implements defensive copying.The implementation properly calls the parent constructor and creates an independent copy of
collectionValuewith appropriate null-safety. This follows the established pattern used inDataFieldandFieldWithAllowedNetsField, ensuring thatElasticCasefield data is properly deep-copied to fix the permission propagation issue.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/FileField.java (1)
24-29: LGTM!The copy constructor correctly deep copies all array fields using
Arrays.copyOf()with proper null-safety checks. SinceStringis immutable, shallow-copying the array elements is sufficient for a true defensive copy. The delegation tosuper(fileField)ensures parent state is also copied.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/FileField.java (1)
15-17: LGTM!The copy constructor correctly delegates to the parent class's copy constructor. Since this subclass only adds Elasticsearch annotations to inherited getters and defines no additional fields, delegating to
super(field)is sufficient for complete state copying.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/TextField.java (1)
14-17: Copy constructor implementation is correct.The defensive copy using
clone()is appropriate forString[]since strings are immutable. The null-safety check is properly handled.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/UserField.java (1)
11-13: Copy constructor implementation is correct.The delegation to
super(field)is appropriate since this subclass adds no new fields—only Elasticsearch annotation overrides on getters. The parent class (com.netgrif.application.engine.objects.elastic.domain.UserField) properly deep copies all mutable array fields usingArrays.copyOf()forusernameValue,fullNameValue,userIdValue, anduserRealmIdValue, preventing shared mutable state.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/TextField.java (1)
15-17: Copy constructor delegation is correct.The copy constructor properly delegates to the superclass. The parent
TextFieldclass defensively copies theString[]field usingclone()(line 16 of parent), ensuring array references are not shared between the original and copied instances. Since this subclass has no additional instance fields, delegation to the parent is complete and sufficient.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/FilterField.java (1)
13-15: LGTM!The copy constructor correctly delegates to the superclass, which is appropriate since this adapter class doesn't introduce additional fields beyond annotations.
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/CaseField.java (1)
13-15: LGTM!The copy constructor correctly delegates to the parent class, which handles copying the
caseValuelist. Since this adapter class doesn't introduce additional mutable fields, this implementation is sufficient.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/BooleanField.java (1)
15-18: LGTM!The copy constructor properly delegates to the superclass and copies the
booleanValuefield. SinceBooleanis immutable, direct assignment is appropriate.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/I18nField.java (1)
15-17: LGTM!The copy constructor correctly delegates to the parent class, which handles deep copying of
keyValueandtranslationsmaps per the AI summary. Since this adapter class doesn't introduce additional mutable fields, delegation is sufficient.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/DataField.java (1)
22-24: > Likely an incorrect or invalid review comment.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/DateField.java (1)
16-18: LGTM!The copy constructor correctly delegates to the superclass, which handles copying the
dateValueandtimestampValuefields. Since this subclass doesn't introduce additional fields and the parent's fields are immutable types (LocalDateTime,Long), the implementation is sound.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ButtonField.java (1)
15-18: LGTM!The copy constructor properly delegates to the superclass and copies the
buttonValuefield. SinceIntegeris immutable, direct assignment is appropriate.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/BooleanField.java (1)
15-18: LGTM!The copy constructor correctly delegates to the superclass and copies the local
booleanValuefield. SinceBooleanis immutable, direct assignment is safe.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java (1)
18-21: LGTM!The copy constructor properly handles the mutable
caseValuelist with null-safety and creates a defensive copy vianew ArrayList<>(). SinceStringelements are immutable, this shallow copy of the list is sufficient to prevent shared mutable state. Based on learnings, the separatecaseValuefield is intentionally maintained for specific Elasticsearch query requirements.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/DateField.java (1)
21-25: LGTM!The copy constructor correctly delegates to the superclass and copies both fields. Since
LocalDateTimeandLongare immutable types, direct assignment is appropriate without needing defensive copies.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nField.java (1)
22-26: LGTM! Copy constructor properly deep-copies mutable state.The implementation correctly:
- Delegates to the parent
TextFieldcopy constructor- Deep copies the
keyValuearray usingArrays.copyOf- Creates an independent copy of
translationsmap (shallow copy is sufficient sinceStringvalues are immutable)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.java (1)
16-18: LGTM! Copy constructor correctly delegates to parent.The implementation properly leverages the parent class's copy constructor, which handles all deep copying of
keyValuearray andkeyValueTranslationsmap. Since this adapter class extends the domainMapField, polymorphic dispatch ensures correct behavior.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/StringCollectionField.java (1)
16-18: Copy constructor correctly delegates to superclass.The addition of the copy constructor is consistent with the PR pattern of adding defensive-copy support across field classes. The delegation to
super(field)is valid and the parent class hierarchy properly handles deep copying through the chain:
DataFielddeep copiesfulltextValueusingArrays.copyOf()TextFielddeep copiestextValueusing.clone()StringCollectionFielddeep copiescollectionValueusingArrays.copyOf()No changes needed.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/UserField.java (1)
27-33: LGTM!The copy constructor correctly performs null-safe defensive copies of all array fields using
Arrays.copyOf. SinceStringis immutable, shallow array copies are sufficient here.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/UserListField.java (1)
13-15: LGTM!The copy constructor correctly delegates to the parent class which handles the deep copying of all array fields.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (7)
3-4: Imports look correct.Both imports are used appropriately in the file:
I18nStringfor wrapping field names inImmediateFieldconstruction (line 136), andCopyConstructorUtilfor deep copyingDataFieldinstances (line 134).
107-109: Correctly deep copies permission maps to fix propagation issue.Using
deepCopy()forpermissions,users, anduserRefsensures these nestedMap<String, Map<String, Boolean>>structures are fully cloned, preventing shared mutable references between the sourceCaseand thisElasticCase. This directly addresses NAE-2316.
120-122: Tasks are correctly deep copied.Creating new
ElasticTaskPairinstances rather than reusing references from the source ensures mutation isolation. This aligns with the requirement to prevent sharing mutable references for thetasksfield. Based on learnings, this is the correct approach.
130-132: Permission maps are correctly deep copied inupdate().Consistent with the constructor, using
deepCopy()for these nested maps ensures theElasticCaseis fully independent of the source's permission structures.
140-145: Well-implemented deep copy utility for nested permission maps.The method correctly handles:
- Null outer map → returns empty
HashMap- Empty outer map → returns empty
HashMap(short-circuit)- Null inner maps → replaced with empty
HashMap- Non-null inner maps → copied via
new HashMap<>()This ensures complete isolation from the source maps.
114-138: Good defensive copying pattern inupdate()method overall.The refactored
update()method consistently creates new collection instances rather than sharing references, which is essential for Elasticsearch document updates. The null-safety guards are thorough for most fields.
100-106: No NPE risk in the constructor—Case guarantees non-null values.The Case constructor (lines 125-141) initializes
enabledRoles,viewRoles,viewUserRefs,negativeViewRoles,viewUsers,negativeViewUsers, andtagsto non-null empty collections. Since Case is abstract and all concrete subclasses invoke this initialization, the ElasticCase constructor safely assumes these getters return non-null. The update() method's null checks are defensive because it receives an ElasticCase parameter (which may have null fields), not a Case parameter.Likely an incorrect or invalid review comment.
...library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Outdated
Show resolved
Hide resolved
...library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Show resolved
Hide resolved
...ct-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/UserListField.java
Outdated
Show resolved
Hide resolved
…loyment - null checks added in ElasticCase
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 (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (1)
100-106: Potential NPE: Missing null checks for collection fields.The constructor lacks null checks for
enabledRoles,viewRoles,viewUserRefs,negativeViewRoles,viewUsers,negativeViewUsers, andtags— unlike theupdate()method (lines 123-129) which handles nulls defensively. If any getter returnsnull,new HashSet<>(null)ornew HashMap<>(null)will throwNullPointerException.Proposed fix for consistency with update()
- enabledRoles = new HashSet<>(useCase.getEnabledRoles()); - viewRoles = new HashSet<>(useCase.getViewRoles()); - viewUserRefs = new HashSet<>(useCase.getViewUserRefs()); - negativeViewRoles = new HashSet<>(useCase.getNegativeViewRoles()); - viewUsers = new HashSet<>(useCase.getViewUsers()); - negativeViewUsers = new HashSet<>(useCase.getNegativeViewUsers()); - tags = new HashMap<>(useCase.getTags()); + enabledRoles = useCase.getEnabledRoles() == null ? new HashSet<>() : new HashSet<>(useCase.getEnabledRoles()); + viewRoles = useCase.getViewRoles() == null ? new HashSet<>() : new HashSet<>(useCase.getViewRoles()); + viewUserRefs = useCase.getViewUserRefs() == null ? new HashSet<>() : new HashSet<>(useCase.getViewUserRefs()); + negativeViewRoles = useCase.getNegativeViewRoles() == null ? new HashSet<>() : new HashSet<>(useCase.getNegativeViewRoles()); + viewUsers = useCase.getViewUsers() == null ? new HashSet<>() : new HashSet<>(useCase.getViewUsers()); + negativeViewUsers = useCase.getNegativeViewUsers() == null ? new HashSet<>() : new HashSet<>(useCase.getNegativeViewUsers()); + tags = useCase.getTags() == null ? new HashMap<>() : new HashMap<>(useCase.getTags());
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/UserListField.java
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 402
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:118-118
Timestamp: 2026-01-13T06:40:13.919Z
Learning: In ElasticCase (nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java), the tasks field containing ElasticTaskPair instances should be deep copied (creating new ElasticTaskPair instances) rather than shallow copied, both in the constructor and the update() method, to prevent sharing mutable references.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2026-01-13T06:40:13.919Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 402
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:118-118
Timestamp: 2026-01-13T06:40:13.919Z
Learning: In ElasticCase (nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java), the tasks field containing ElasticTaskPair instances should be deep copied (creating new ElasticTaskPair instances) rather than shallow copied, both in the constructor and the update() method, to prevent sharing mutable references.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-12-19T07:44:48.019Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/DataService.java:336-343
Timestamp: 2025-12-19T07:44:48.019Z
Learning: In the netgrif/application-engine repository, when a Case is created, the `Case.populateDataSetBehaviorAndComponents()` method automatically copies all transition-level behavior and components to the case-level data fields, keyed by transition ID. This means methods like `isDataFieldEditable` only need to check case-level behavior (dataField.getBehavior().get(transId)), without fallback to transition.getDataSet(), since the behavior has already been copied during case initialization.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-12-19T07:55:13.427Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 400
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java:239-247
Timestamp: 2025-12-19T07:55:13.427Z
Learning: In the netgrif/application-engine project, the `taskService.findOne()` method (from ITaskService) never returns null. When a task is not found, it handles the error through other means (such as throwing an exception) rather than returning null, so null checks after `findOne()` calls are unnecessary.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-12-19T09:06:38.316Z
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 398
File: application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java:57-58
Timestamp: 2025-12-19T09:06:38.316Z
Learning: In the file `application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java`, the `definition` attribute of the `Action` class (from `com.netgrif.application.engine.objects.petrinet.domain.dataset.logic.action.Action`) is never null, so null checks for `action.getDefinition()` are not needed.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
🔇 Additional comments (6)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/UserListField.java (1)
11-13: LGTM!The copy constructor correctly delegates to the superclass copy constructor, which (per the AI summary) performs deep copying of the array fields using
Arrays.copyOf. Since this Spring adapter class doesn't introduce any additional fields beyond the parent, the delegation tosuper(field)is sufficient for proper cloning.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java (1)
19-25: Copy constructor correctly implements deep-copy pattern.The implementation properly:
- Delegates to parent
TextFieldcopy constructor viasuper(field), which deep-copiestextValue- Deep-copies
keyValuearray usingArrays.copyOf()(safe for immutableStringelements)- Deep-copies
keyValueTranslationsby creating newI18nStringinstances for each map value viastream().collect(Collectors.toMap(...))- Includes null checks for both mutable fields
This follows the required pattern of deep-copying mutable references to prevent shared state issues across instances.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (4)
107-111: LGTM! Deep copy for permissions, users, and userRefs.These changes correctly address NAE-2316 by ensuring nested maps are deep-copied rather than sharing references with the source
Case. TheimmediateDatahandling with the copy constructor is also properly null-safe.
118-135: Good defensive copying with null-safe handling.The
update()method correctly implements:
- Null-safe collection initialization with mutable
HashSet/HashMapinstances- Deep copy of
tasksvia newElasticTaskPairinstances (per established pattern)- Deep copy of
dataSetvalues usingCopyConstructorUtil.copyfor proper polymorphic copying- Filtering out null entries in
dataSetto prevent downstream issues
141-146: LGTM! Proper nested map deep copy.The
deepCopyhelper correctly creates independent copies of nestedMap<String, Map<String, Boolean>>structures, handling null outer maps and null inner maps appropriately. This ensures permission maps are fully decoupled from the source object.
136-138: Remove unnecessaryI18nStringwrapping forImmediateField.getName().
field.getName()returnsI18nString(fromFieldclass), so wrapping it withnew I18nString(field.getName())creates an unnecessary copy. Pass theI18nStringdirectly to theImmediateFieldconstructor:new ImmediateField(field.getStringId(), field.getName(), field.getType())⛔ Skipped due to learnings
Learnt from: renczesstefan Repo: netgrif/application-engine PR: 402 File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java:118-118 Timestamp: 2026-01-13T06:40:13.919Z Learning: In ElasticCase (nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java), the tasks field containing ElasticTaskPair instances should be deep copied (creating new ElasticTaskPair instances) rather than shallow copied, both in the constructor and the update() method, to prevent sharing mutable references.Learnt from: renczesstefan Repo: netgrif/application-engine PR: 339 File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45 Timestamp: 2025-08-20T07:27:02.660Z Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).Learnt from: renczesstefan Repo: netgrif/application-engine PR: 339 File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341 Timestamp: 2025-08-19T20:07:15.621Z Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Retoocs
requested changes
Jan 13, 2026
...rary/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ImmediateField.java
Outdated
Show resolved
Hide resolved
…loyment - constructor call fix in ImmediateField
Retoocs
previously approved these changes
Jan 13, 2026
SamuelPalaj
dismissed
Retoocs’s stale review
The merge-base changed after approval.
renczesstefan
previously approved these changes
Jan 13, 2026
SamuelPalaj
dismissed
renczesstefan’s stale review
The merge-base changed after approval.
tuplle
previously approved these changes
Jan 13, 2026
SamuelPalaj
dismissed
tuplle’s stale review
The merge-base changed after approval.
machacjozef
previously approved these changes
Jan 13, 2026
SamuelPalaj
dismissed
machacjozef’s stale review
The merge-base changed after approval.
machacjozef
requested review from
Retoocs,
machacjozef,
renczesstefan and
tuplle
machacjozef
approved these changes
Jan 13, 2026
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
Fixes NAE-2316
Test Configuration
Checklist:
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.