[NAE-2382] Elastic bulk index fails if option fields have empty string#416
Merged
machacjozef merged 8 commits intorelease/7.0.0-rev10from Mar 2, 2026
Merged
[NAE-2382] Elastic bulk index fails if option fields have empty string#416machacjozef merged 8 commits intorelease/7.0.0-rev10from
machacjozef merged 8 commits intorelease/7.0.0-rev10from
Conversation
- updated null or empty string key handling for elastic translation objects - added additional logging to RestResponseExceptionHandler
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds detailed logging and path-inspection helpers to RestResponseExceptionHandler for JsonMappingException traces, and introduces NONE_OPTION_KEY plus key-normalization and deterministic LinkedHashMap usage in MapField constructors. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/web/RestResponseExceptionHandler.java`:
- Around line 42-45: The debug logging calls use fieldFrom.getClass() and
caseFrom.getClass() which will NPE if path.getLast().getFrom() or
path.get(path.size() - 3).getFrom() returns null; update the logging in
RestResponseExceptionHandler to be null-safe by deriving class names defensively
(e.g., use a conditional or Objects.toString/getClassName fallback) when
referencing fieldFrom and caseFrom from their getFrom() results, and log a clear
placeholder like "null" or "unknown" for the class when getFrom() is null so the
debug statements never call .getClass() on a null reference.
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java`:
- Around line 40-44: In the loop over valueTranslationPairs, avoid calling
resolveTranslationPairKey twice for the same entry: compute String resolvedKey =
resolveTranslationPairKey(valueTranslationPair.getKey()) once, use resolvedKey
when adding to this.keyValue and when putting into this.keyValueTranslations,
and retain
values.addAll(I18nStringUtils.collectTranslations(valueTranslationPair.getValue()));
so resolveTranslationPairKey, valueTranslationPair, this.keyValue and
this.keyValueTranslations are referenced but only invoked once per iteration.
- Around line 24-26: The copy constructor for MapField uses Collectors.toMap on
field.keyValueTranslations which can throw IllegalStateException when multiple
source keys map to the same resolved key (via resolveTranslationPairKey); modify
that stream collect call to pass a merge function that keeps the last value
(e.g. (oldVal, newVal) -> newVal) so duplicate resolved keys are handled like
HashMap.put; update the expression that constructs I18nString(entry.getValue())
accordingly and keep resolveTranslationPairKey and I18nString references
unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/RestResponseExceptionHandler.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
.../src/main/java/com/netgrif/application/engine/workflow/web/RestResponseExceptionHandler.java
Outdated
Show resolved
Hide resolved
...ct-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
Outdated
Show resolved
Hide resolved
...ct-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
Show resolved
Hide resolved
Retoocs
previously approved these changes
Feb 23, 2026
Updated the collector in MapField to ensure that keyValueTranslations maintains insertion order by using LinkedHashMap. Additionally, resolved potential duplicate key conflicts by explicitly handling them in the collector function.
Previously, debug logs for 'fieldFrom' and 'caseFrom' assumed non-null values, which could lead to a null pointer exception. This update adds checks to handle null values gracefully in log statements.
tuplle
previously approved these changes
Feb 23, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java (1)
44-46: 🧹 Nitpick | 🔵 Trivial
resolveTranslationPairKeyinvoked twice for the same key — extract to a local variable.♻️ Proposed refactor
for (Map.Entry<String, I18nString> valueTranslationPair : valueTranslationPairs) { - this.keyValue.add(resolveTranslationPairKey(valueTranslationPair.getKey())); + String resolvedKey = resolveTranslationPairKey(valueTranslationPair.getKey()); + this.keyValue.add(resolvedKey); values.addAll(I18nStringUtils.collectTranslations(valueTranslationPair.getValue())); - this.keyValueTranslations.put(resolveTranslationPairKey(valueTranslationPair.getKey()), valueTranslationPair.getValue()); + this.keyValueTranslations.put(resolvedKey, valueTranslationPair.getValue()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java` around lines 44 - 46, In MapField, avoid calling resolveTranslationPairKey twice for the same valueTranslationPair: compute String resolvedKey = resolveTranslationPairKey(valueTranslationPair.getKey()) once and reuse it for keyValue.add(...) and keyValueTranslations.put(...), leaving values.addAll(I18nStringUtils.collectTranslations(...)) unchanged; update the code inside the block that currently calls resolveTranslationPairKey in both keyValue.add and keyValueTranslations.put to use the local resolvedKey variable instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java`:
- Line 42: The list-based constructor of MapField initializes
keyValueTranslations with new HashMap<>() which breaks the insertion-order
guarantee preserved by the copy constructor (which uses LinkedHashMap::new);
update the list-based constructor to initialize keyValueTranslations with a
LinkedHashMap (consistent with the copy constructor) so key iteration order is
deterministic for Elastic indexing—look for the MapField constructor that sets
this.keyValueTranslations and replace its HashMap instantiation with
LinkedHashMap.
---
Duplicate comments:
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java`:
- Around line 44-46: In MapField, avoid calling resolveTranslationPairKey twice
for the same valueTranslationPair: compute String resolvedKey =
resolveTranslationPairKey(valueTranslationPair.getKey()) once and reuse it for
keyValue.add(...) and keyValueTranslations.put(...), leaving
values.addAll(I18nStringUtils.collectTranslations(...)) unchanged; update the
code inside the block that currently calls resolveTranslationPairKey in both
keyValue.add and keyValueTranslations.put to use the local resolvedKey variable
instead.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/RestResponseExceptionHandler.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
...ct-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
Outdated
Show resolved
Hide resolved
Retoocs
requested changes
Feb 23, 2026
...ct-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
Outdated
Show resolved
Hide resolved
...ct-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
Show resolved
Hide resolved
Switched from HashMap to LinkedHashMap in keyValueTranslations to maintain the insertion order of entries. This change ensures predictable ordering when iterating over the map, which may be crucial for consistent processing or display.
Switched from HashMap to LinkedHashMap in keyValueTranslations to maintain the insertion order of entries. This change ensures predictable ordering when iterating over the map, which may be crucial for consistent processing or display.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java`:
- Around line 44-47: The constructor code in MapField is storing mutable
I18nString objects by reference into keyValueTranslations (via
this.keyValueTranslations.put(key, valueTranslationPair.getValue())), which can
leak external mutations; change the constructor to defensively copy the
I18nString before putting it into keyValueTranslations (create a new I18nString
copy from valueTranslationPair.getValue()) so MapField holds its own instance;
locate the MapField(List<...>) constructor and replace the direct put with
putting a cloned/constructed copy of the I18nString (use the same copy semantics
used elsewhere in the codebase, e.g., how ElasticCase deep-copies nested
objects) while keeping resolveTranslationPairKey(valueTranslationPair.getKey())
and existing collection logic unchanged.
ℹ️ Review info
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/MapField.java
...ct-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
Outdated
Show resolved
Hide resolved
Previously, the code directly assigned a nullable value, which could lead to potential issues. This change ensures that the value is wrapped in an I18nString instance if not null, improving safety and consistency when managing translations.
tuplle
previously approved these changes
Mar 2, 2026
There was a problem hiding this comment.
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/MapField.java (1)
21-30:⚠️ Potential issue | 🟠 Major
keyValueis not normalized in the copy constructor, butkeyValueTranslationskeys are — potential data inconsistency.The copy constructor normalizes keys in
keyValueTranslations(line 27), but copieskeyValueverbatim (line 23). If the source field contains an empty-string key:
this.keyValuewill still hold""this.keyValueTranslationswill have key"none"This breaks the invariant established by the list constructor (lines 44-50), where both collections use the same normalized key. The
getValue()method (line 56-63) returns elements fromkeyValue, potentially returning unnormalized keys.🛡️ Proposed fix — normalize keyValue during copy
public MapField(MapField field) { super(field); - this.keyValue = field.keyValue == null ? null : new ArrayList<>(field.keyValue); + this.keyValue = field.keyValue == null ? null + : field.keyValue.stream() + .map(this::resolveTranslationPairKey) + .collect(Collectors.toCollection(ArrayList::new)); this.keyValueTranslations = field.keyValueTranslations == null ? null : field.keyValueTranslations.entrySet().stream() .collect(Collectors.toMap(entry ->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java` around lines 21 - 30, The copy constructor MapField(MapField field) copies keyValue verbatim while normalizing keys for keyValueTranslations causing mismatch; update the constructor to normalize each key in field.keyValue using the same resolveTranslationPairKey(...) logic (preserve null handling and ordering) when creating this.keyValue (e.g., map each element through resolveTranslationPairKey before collecting into the new ArrayList) so keyValue and keyValueTranslations remain consistent with getValue() expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java`:
- Around line 21-30: The copy constructor MapField(MapField field) copies
keyValue verbatim while normalizing keys for keyValueTranslations causing
mismatch; update the constructor to normalize each key in field.keyValue using
the same resolveTranslationPairKey(...) logic (preserve null handling and
ordering) when creating this.keyValue (e.g., map each element through
resolveTranslationPairKey before collecting into the new ArrayList) so keyValue
and keyValueTranslations remain consistent with getValue() expectations.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/RestResponseExceptionHandler.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
Updated the log message in RestResponseExceptionHandler to specify that the JSON write failure is due to the path being smaller than 3. This improves clarity and debugging efficiency when analyzing error logs.
Retoocs
approved these changes
Mar 2, 2026
tuplle
approved these changes
Mar 2, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
Fix elastic bulk fail, when translation with empty key fails + added additional logging for
RestResponseErrorHandlerFixes NAE-2382
Dependencies
No new dependencies where introduced.
Third party dependencies
No new dependencies were introduced.
Blocking Pull requests
There are no dependencies on other PR.
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
Bug Fixes
Chores