Refactor case search to replace stringId with id.
#341
Conversation
Replaced the deprecated `stringId` field with the new `id` field throughout the codebase, maintaining backward compatibility by merging both fields during queries. Updated `ImmediateField` and its references to simplify type handling, and moved it to a more appropriate package. These changes improve clarity, support future deprecation, and ensure smooth transitions.
WalkthroughAdds CaseSearchRequest.id (deprecates stringId); ElasticCaseService now combines request.id and request.stringId to build a terms query on Elasticsearch document _id; ImmediateField moved to elastic.domain and its type changed from FieldType to String; removed an unused import in ElasticCase. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Request as CaseSearchRequest
participant Service as ElasticCaseService
participant ES as Elasticsearch
Client->>Request: submit search payload (id[] and/or stringId[] )
Client->>Service: call search(request)
Service->>Service: validIds = concat(request.id, request.stringId) // Optional-safe
alt validIds empty
Service-->>Client: continue without ID terms filter / or return no filter
else validIds present
Service->>ES: terms query on _id with validIds (FieldValue.of(...))
ES-->>Service: hits
Service-->>Client: results
end
Note over Request,Service: `stringId` retained for backward compatibility (deprecated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
added
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/CaseSearchRequest.java (1)
98-104: Map-based constructor ignores id (functional gap vs. JSON binding)When CaseSearchRequest is constructed via the Map constructor, id is never populated (only stringId is). This breaks parity with the Jackson path and can make id-based queries no-op for callers that use the Map route.
Apply this diff to support both id and stringId (and to accept a single string as well as a list, mirroring ACCEPT_SINGLE_VALUE_AS_ARRAY):
- if (request.containsKey("stringId") && request.get("stringId") instanceof List) { - this.stringId = (List<String>) request.get("stringId"); - } + if (request.containsKey("stringId")) { + Object rawStringId = request.get("stringId"); + if (rawStringId instanceof List) { + this.stringId = (List<String>) rawStringId; + } else if (rawStringId instanceof String) { + this.stringId = List.of((String) rawStringId); + } + } + if (request.containsKey("id")) { + Object rawId = request.get("id"); + if (rawId instanceof List) { + this.id = (List<String>) rawId; + } else if (rawId instanceof String) { + this.id = List.of((String) rawId); + } + }If desired, we can also add a DEBUG log when stringId is present to help track deprecation usage.
Would you like me to add a small unit test to verify the Map constructor populates id for both String and List inputs?
🧹 Nitpick comments (5)
application-engine/src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/CaseSearchRequest.java (1)
50-55: Deprecation metadata looks right; consider communicating removal timelineThe @deprecated(since = "7.0.0") and Javadoc are clear. As a minor improvement, you may want to add a brief note on the planned removal version to set expectations for clients.
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java (1)
426-444: Fix JSON examples (missing colon) in JavadocMinor docs nit: the array examples are missing a colon after the field name.
Apply this doc-only diff:
- * "id" [ + * "id": [nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ImmediateField.java (3)
4-4: Avoid coupling ES projection to domain Field<?>; prefer a factory/mapperImporting and accepting
Field<?>here creates a dependency from the ES DTO to the domain model. That coupling makes reindexing/migrations harder.Move the conversion to a mapper and keep this class as a plain DTO:
// Keep the POJO constructor public ImmediateField(String stringId, I18nString name, String type) { this.stringId = stringId; this.name = name; this.type = type; } // Mapper (elsewhere, not in this DTO) public static ImmediateField from(Field<?> field) { return new ImmediateField(field.getStringId(), field.getName(), field.getType() != null ? field.getType().getName() : null); }This keeps elastic.domain free of higher-level dependencies, easing maintenance and future index schema changes.
18-18: “type” as a free-form String: ensure ES mapping is keyword and consider a clearer property nameSwitching to
Stringis fine, but:
- Make sure the ES mapping for this field is
keywordto avoid analyzer bloat and accidental full-text behavior.- Optional: rename to
fieldTypeto avoid ambiguity with ES “type” terminology and improve readability.Apply if you want the clearer name:
- private String type; + private String fieldType; @@ - this.type = field.getType().getName(); + this.fieldType = field.getType().getName();Please confirm the index template/mapping defines this field as
keyword(nottext). If not, you’ll want to adjust the template and reindex.Also applies to: 26-26
23-27: Defensive null-safety in constructor
fieldandfield.getType()may be null depending on upstream data. A small guard prevents an NPE during indexing.Apply:
- public ImmediateField(Field<?> field) { - this.stringId = field.getStringId(); - this.name = field.getName(); - this.type = field.getType().getName(); - } + public ImmediateField(Field<?> field) { + java.util.Objects.requireNonNull(field, "field"); + this.stringId = field.getStringId(); + this.name = field.getName(); + var ft = field.getType(); + this.type = (ft != null) ? ft.getName() : null; + }Given the PR’s broader “stringId → id” migration for cases/tasks (per prior learnings), confirm that keeping
stringIdhere is intentional for fields. If unifying naming is desired, we can rename toidand adapt upstream mappers accordingly. I can prep that change if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/CaseSearchRequest.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java(0 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ImmediateField.java(2 hunks)
💤 Files with no reviewable changes (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
PR: netgrif/application-engine#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
PR: netgrif/application-engine#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
PR: netgrif/application-engine#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: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
PR: netgrif/application-engine#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/ElasticCaseService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/CaseSearchRequest.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). (3)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/CaseSearchRequest.java (1)
47-49: Good addition of id with tolerant JSON deserializationAccepting single values as arrays on id aligns well with existing request patterns and keeps the API ergonomic.
...-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
Show resolved
Hide resolved
...rary/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ImmediateField.java
Show resolved
Hide resolved
Replaced the deprecated `stringId` field with the new `id` field throughout the codebase, maintaining backward compatibility by merging both fields during queries. Updated `ImmediateField` and its references to simplify type handling, and moved it to a more appropriate package. These changes improve clarity, support future deprecation, and ensure smooth transitions.
The `@JsonSerialize` and `@JsonDeserialize` annotations were removed from the `startDate` field. This change promotes default serialization and deserialization behavior, simplifying the code and improving maintainability.
Description
Replaced the deprecated
stringIdfield with the newidfield throughout the codebase, maintaining backward compatibility by merging both fields during queries. UpdatedImmediateFieldand its references to simplify type handling, and moved it to a more appropriate package. These changes improve clarity, support future deprecation, and ensure smooth transitions.Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
This was tested manually.
Test Configuration
Checklist:
Summary by CodeRabbit