[NAE-2251] NAE - Map field options are not translated #379
Conversation
- add keyValue translations to elastic case mapping
WalkthroughThese changes refactor map-like fields across multiple layers to use I18nString (translation-aware strings) instead of Collection values. The service layer, core domain, and adapter layer are updated to consistently handle translations for map entries through a new keyValueTranslations structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Pre-merge checks✅ 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java (1)
144-144: Same null handling consideration astransformMultichoiceMapField.Similar to the multichoice map field transformation,
options.get(selectedKey)may return null if the selected key is not present in the options map. While this is handled gracefully by the MapField constructor, consider whether this scenario warrants logging for data consistency tracking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
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/elastic/domain/MapField.java(3 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.java(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 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/MapField.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/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:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.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-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.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:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.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: Build
🔇 Additional comments (5)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java (2)
20-29: LGTM with minor observation.The constructor correctly initializes the map field with a single key-value translation pair. The
collectTranslationsmethod handles null I18nString values gracefully by returning an empty list, which results in emptytextValueandfulltextValuearrays.Note: The
keyValueTranslationsmap will contain the key mapped to null if the entry value is null, which appears intentional for preserving the key even when translations are absent.
31-43: LGTM!The list-based constructor correctly processes multiple key-value translation pairs, appropriately delegating translation collection to the helper method and maintaining consistency with the single-entry constructor.
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.java (2)
16-22: LGTM!The constructors correctly delegate to the parent class with the updated I18nString-based signatures, maintaining consistency with the base MapField implementation.
35-38: LGTM!The
getKeyValueTranslations()method appropriately exposes translation metadata with@Field(type = Flattened, index = false), which stores the translations in Elasticsearch without making them searchable. This is correct for metadata that needs to be retrieved but not queried.application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java (1)
104-106: Null handling is already in place—no fixes required.The concern about
options.get(key)returning null is valid, but verification confirms the code already handles this safely. TheMapField.collectTranslations()method explicitly checks for null I18nString values (line 56) and returns an empty translations list, ensuring graceful degradation.While the code is correct as-is, logging missing option keys could aid debugging if schema or options drift occurs. This remains an optional optimization for observability rather than a required fix.
| protected List<String> collectTranslations(I18nString i18nString) { | ||
| List<String> translations = new ArrayList<>(); | ||
| if (i18nString == null) { | ||
| return translations; | ||
| } | ||
| translations.add(i18nString.getDefaultValue()); | ||
| translations.addAll(i18nString.getTranslations().values()); | ||
| return translations; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the duplicated collectTranslations method.
This method is duplicated in ElasticCaseMappingService.java (lines 207-215) with an identical implementation. Consider extracting it to a utility class or a common base class to maintain DRY principles.
Example location: Create a utility class such as I18nStringUtils:
public class I18nStringUtils {
public static List<String> collectTranslations(I18nString i18nString) {
List<String> translations = new ArrayList<>();
if (i18nString == null) {
return translations;
}
translations.add(i18nString.getDefaultValue());
translations.addAll(i18nString.getTranslations().values());
return translations;
}
}🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
around lines 54 to 62, the collectTranslations(I18nString) method is duplicated
in ElasticCaseMappingService.java (around lines 207-215); extract this logic to
a shared utility (e.g., com.netgrif.application.engine.util.I18nStringUtils with
a public static List<String> collectTranslations(I18nString)) and replace both
implementations with calls to that utility; add the necessary import, remove the
duplicated method from both classes, and run a quick compile to ensure no
visibility/import issues remain.
| import com.netgrif.application.engine.objects.petrinet.domain.I18nString; | ||
| import lombok.NoArgsConstructor; | ||
| import org.springframework.data.elasticsearch.annotations.Field; | ||
|
|
||
| import java.util.Collection; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import static org.springframework.data.elasticsearch.annotations.FieldType.Keyword; | ||
| import static org.springframework.data.elasticsearch.annotations.FieldType.Text; | ||
| import static org.springframework.data.elasticsearch.annotations.FieldType.*; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Verify unused imports.
The Collection import (line 7) may no longer be needed since the constructor signatures were updated from Collection<String> to I18nString. Consider removing it if it's unused.
Also note that the static wildcard import (line 11) is less explicit than named imports, though this is a minor style preference.
🏁 Script executed:
#!/bin/bash
# Check if Collection is used anywhere in this file
rg -n '\bCollection\b' nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.javaLength of output: 30
Remove unused Collection import.
The Collection import on line 7 is unused and should be removed. The static wildcard import on line 11 is acceptable, though explicit imports are generally preferred.
🤖 Prompt for AI Agents
In
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.java
around lines 3 to 11, the import for java.util.Collection is unused; remove that
import line so only required imports remain (keep I18nString, NoArgsConstructor,
Field, List, Map and the static FieldType wildcard as-is) and run a quick
compile/organize-imports to ensure there are no lingering unused imports.
Description
Fixes NAE-2251
Dependencies
none
Third party dependencies
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
manually
Test Configuration
Checklist:
Summary by CodeRabbit