[NAE-2261] SEARCH Node - fetch cases from mongo #385
Conversation
- add AuthPrincipalDto to object library - remove immediate data from elasticCase and add NodeInfo - rework of elastic Datafields to remove dependency to TextField
- remove code from 2251 - when the search node fetches cases from mongo, its not nessessery anymore to store translations of value into elastic
WalkthroughThe PR restructures Elasticsearch mapping for collection and map fields by transitioning map field values from single I18nString translations to Collection. Multiple field classes (MapField, I18nField, StringCollectionField) now inherit from DataField instead of TextField. New value objects (NodeInfo, AuthPrincipalDto) are introduced for data model enhancements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
improvement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/StringCollectionField.java (1)
10-17: Consider using private access modifier forcollectionValuefield.The field
collectionValueon line 12 is declared aspublic, which breaks encapsulation. Since the class uses Lombok@Data, getters and setters are already generated. Consider making itprivatefor consistency with typical Java bean conventions.Also note: the constructor stores
valuesboth in the parent class viasuper(values)(which setsfulltextValue) and incollectionValue. If this dual storage is intentional for different Elasticsearch query patterns (similar to thecaseValue/fulltextValuepattern in CaseField), this is fine. Otherwise, consider whether both are necessary.- public String[] collectionValue; + private String[] collectionValue;
♻️ Duplicate comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java (1)
136-136: Same null handling concern applies here.Similar to the multichoice map field transformation, if
selectedKeyis not present in theoptionsmap, the result will be an entry with an empty collection of translations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java(2 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/dto/AuthPrincipalDto.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nField.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/NodeInfo.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/StringCollectionField.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.java(1 hunks)
🧰 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-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:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.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:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.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/StringCollectionField.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/I18nField.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:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.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/MapField.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/I18nField.java
📚 Learning: 2025-06-23T13:30:13.096Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field `attributeId` will be renamed to `value` to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nField.java
📚 Learning: 2025-12-02T14:35:05.305Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 373
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/actions/ActionApi.java:98-98
Timestamp: 2025-12-02T14:35:05.305Z
Learning: In ActionApi, the deleteCase method intentionally does not include an AuthPrincipalDto parameter because it does not require author information, unlike createCase which needs the author via authPrincipalDto.
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/dto/AuthPrincipalDto.java
🧬 Code graph analysis (2)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nStringUtils.java (1)
I18nStringUtils(8-18)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/NodeInfo.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (1)
Data(18-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (10)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/dto/AuthPrincipalDto.java (1)
11-24: LGTM!The DTO structure is well-designed. Excluding
sessionIdfromtoStringandequals/hashCodeis a good practice—it prevents accidental logging of sensitive session data and ensures object equality is based on identity attributes rather than transient session state.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nField.java (2)
14-24: LGTM on the DataField migration.The inheritance change to
DataFieldand constructor update align with the broader refactoring in this PR.
26-32: Verify single-value behavior is intentional.The
getValue()method only usesfulltextValue[0]when constructing theI18nString. IffulltextValuecan contain multiple elements (sincevaluesis aSetpassed to the constructor), confirm this single-value extraction is the intended behavior.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (3)
82-107: LGTM on constructor logic.The constructor properly initializes all case-related fields from the source
Caseobject, with appropriate null-safety checks for tasks and proper use of defensive copies for collections.
109-125: LGTM on update method.The update method correctly synchronizes relevant mutable fields while incrementing the version counter.
80-81: Remove the unusednodeInfofield or integrate it properly.The
nodeInfofield is declared at line 80 but is not initialized in the constructor, not updated in theupdate()method, and is never accessed anywhere in the codebase. Either remove this unused field or integrate it fully by initializing it during construction and updating it in theupdate()method.Likely an incorrect or invalid review comment.
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.java (1)
14-31: LGTM!The constructor signatures correctly align with the base class refactoring from I18nString to Collection. The Elasticsearch field annotations are properly applied: Text type for full-text search and Keyword type for exact matching and aggregations.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java (3)
12-12: Architectural change aligns with PR objectives.The change from extending
TextFieldtoDataFieldremoves the dependency on TextField as stated in the PR objectives. This is part of the broader refactoring to decouple Elasticsearch datafields from TextField.
16-29: Constructor implementation is correct.The refactored constructors properly handle the transition from I18nString to Collection:
- Single-entry constructor delegates to the list constructor
- List constructor correctly extracts keys into
keyValuearray and flattens all translation collections intofulltextValue
31-38: getValue() method provides convenient API.The new
getValue()method returns:
- A single key as String for enumeration fields
- A LinkedHashSet of keys for multichoice fields
- null for empty/null keyValue
The LinkedHashSet preserves insertion order while ensuring uniqueness, which is appropriate for multichoice selections.
| List<Map.Entry<String, Collection<String>>> values = new ArrayList<>(); | ||
| for (String key : (Set<String>) mapValues) { | ||
| I18nString selectedValue = options.get(key); | ||
| values.add(new AbstractMap.SimpleEntry<>(key, selectedValue != null ? selectedValue : new I18nString(""))); | ||
| values.add(new AbstractMap.SimpleEntry<>(key, I18nStringUtils.collectTranslations(options.get(key)))); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file and the complete method context
cat -n application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java | sed -n '80,110p'Repository: netgrif/application-engine
Length of output: 2089
🏁 Script executed:
# Search for I18nStringUtils.collectTranslations implementation
find . -type f -name "*.java" | xargs grep -l "I18nStringUtils" | head -5Repository: netgrif/application-engine
Length of output: 292
🏁 Script executed:
# Look for the I18nStringUtils class definition
fd -type f -name "I18nStringUtils.java"Repository: netgrif/application-engine
Length of output: 240
🏁 Script executed:
# Read the I18nStringUtils file to see the collectTranslations implementation
cat -n nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nStringUtils.javaRepository: netgrif/application-engine
Length of output: 786
🏁 Script executed:
# Search for getFieldOptions implementation
rg -nA10 "getFieldOptions" application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.javaRepository: netgrif/application-engine
Length of output: 1830
🏁 Script executed:
# Search for tests related to transformMultichoiceMapField
fd -type f -name "*.java" | xargs grep -l "transformMultichoiceMapField\|ElasticCaseMappingService" | grep -i testRepository: netgrif/application-engine
Length of output: 240
🏁 Script executed:
# Search for tests of ElasticCaseMappingService
find . -type f -name "*.java" -path "*/test/*" | xargs grep -l "ElasticCaseMappingService" 2>/dev/null | head -3Repository: netgrif/application-engine
Length of output: 52
🏁 Script executed:
# Search for any validation or warnings about missing keys
rg -n "mapValues|options\.get" application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java -B2 -A2Repository: netgrif/application-engine
Length of output: 1025
🏁 Script executed:
# Check if there's any logging or warning in the surrounding code
rg -n "logger|log\.|warn|error" application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java | head -20Repository: netgrif/application-engine
Length of output: 1771
🏁 Script executed:
# Check the git history of this specific method to understand its evolution
git log --oneline -n 20 -- application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.javaRepository: netgrif/application-engine
Length of output: 150
Inconsistent error handling: missing options keys are silently converted to empty translations.
The code at line 98 does not validate that all keys from mapValues exist in the options map. While I18nStringUtils.collectTranslations(null) handles null gracefully by returning an empty list, this pattern is inconsistent with the error handling established elsewhere in this class. Other methods (e.g., lines 163, 180, 194) log errors when encountering data inconsistencies. Consider either adding validation to ensure all keys are present in the options map, or logging a warning when options.get(key) returns null to make data issues visible during debugging.
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java
around lines 96-99, the code blindly calls options.get(key) and passes the
result to I18nStringUtils.collectTranslations, silently producing empty
translations for missing keys; change this to first check
options.containsKey(key) and if the key is missing log a warning (including the
missing key and identifying context such as the parent map or document id), then
either continue with an explicit empty translation list or handle it
consistently with the class’s error strategy (throw or collect an error),
ensuring missing keys are visible in logs rather than silently ignored.
| import lombok.Data; | ||
| import lombok.EqualsAndHashCode; | ||
| import lombok.NonNull; | ||
| import lombok.ToString; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused import.
lombok.NonNull is imported but not used in this class.
import lombok.Data;
import lombok.EqualsAndHashCode;
-import lombok.NonNull;
import lombok.ToString;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import lombok.Data; | |
| import lombok.EqualsAndHashCode; | |
| import lombok.NonNull; | |
| import lombok.ToString; | |
| import lombok.Data; | |
| import lombok.EqualsAndHashCode; | |
| import lombok.ToString; |
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/dto/AuthPrincipalDto.java
around lines 3 to 6, the import lombok.NonNull is unused; remove that import
line and keep only the required lombok imports (Data, EqualsAndHashCode,
ToString) so there are no unused imports.
| @Data | ||
| @AllArgsConstructor | ||
| public class NodeInfo implements Serializable { | ||
|
|
||
| @Serial | ||
| private static final long serialVersionUID = 7454569786379865L; | ||
|
|
||
| private String nodeHost; | ||
| private int nodePort; | ||
| } |
There was a problem hiding this comment.
Add @NoArgsConstructor for deserialization compatibility.
This class is used as a field in ElasticCase, which is serialized to Elasticsearch. Most serialization frameworks (including Jackson, commonly used with Elasticsearch) require a no-argument constructor for deserialization. Without it, you may encounter runtime errors when reading ElasticCase documents from Elasticsearch.
@Data
@AllArgsConstructor
+@NoArgsConstructor
public class NodeInfo implements Serializable {Also add the import:
import lombok.NoArgsConstructor;🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/NodeInfo.java
around lines 9 to 18, the class lacks a no-argument constructor required by
deserializers (e.g., Jackson) — add Lombok's @NoArgsConstructor annotation to
the class and import lombok.NoArgsConstructor so the class has an implicit
no-arg constructor for deserialization while keeping the existing @Data and
@AllArgsConstructor.
machacjozef
changed the base branch from
release/7.0.0-rev9
to
release/7.0.0-rev10
Description
Implements NAE-2261
Dependencies
none
Third party dependencies
Blocking Pull requests
Depends on #(373) and merging rev8 into rev9
How Has Been This Tested?
manually
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.