-
Notifications
You must be signed in to change notification settings - Fork 6
[NAE-2197] Elastic Mapping Fixes #352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
82cf7bd
208021d
6b563c1
ff3a995
5a7c88b
4a3527e
ebd9c5c
c44dde6
7fc0546
184e670
9b5dc59
3195287
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package com.netgrif.application.engine.elastic.service; | ||
|
|
||
| import co.elastic.clients.elasticsearch._types.FieldValue; | ||
| import co.elastic.clients.elasticsearch._types.mapping.FieldType; | ||
| import co.elastic.clients.elasticsearch._types.query_dsl.BoolQuery; | ||
| import co.elastic.clients.elasticsearch._types.query_dsl.QueryStringQuery; | ||
| import co.elastic.clients.elasticsearch._types.query_dsl.TermsQueryField; | ||
|
|
@@ -138,7 +139,7 @@ public Page<Case> search(List<CaseSearchRequest> requests, LoggedUser user, Page | |
| // TODO: impersonation | ||
| // LoggedUser loggedOrImpersonated = user.getSelfOrImpersonated(); | ||
| LoggedUser loggedOrImpersonated = user; | ||
| pageable = resolveUnmappedSortAttributes(pageable); | ||
| // pageable = resolveUnmappedSortAttributes(pageable); | ||
machacjozef marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| NativeQuery query = buildQuery(requests, loggedOrImpersonated, pageable, locale, isIntersection); | ||
| List<Case> casePage; | ||
| long total; | ||
|
|
@@ -190,11 +191,22 @@ protected NativeQuery buildQuery(List<CaseSearchRequest> requests, LoggedUser us | |
| BinaryOperator<BoolQuery.Builder> reductionOperation = isIntersection ? (a, b) -> a.must(b.build()._toQuery()) : (a, b) -> a.should(b.build()._toQuery()); | ||
| BoolQuery.Builder query = singleQueries.stream().reduce(new BoolQuery.Builder(), reductionOperation); | ||
|
|
||
| NativeQueryBuilder builder = new NativeQueryBuilder(); | ||
| return builder | ||
| NativeQueryBuilder builder = new NativeQueryBuilder() | ||
| .withQuery(query.build()._toQuery()) | ||
| .withPageable(pageable) | ||
| .build(); | ||
| .withPageable(PageRequest.of(pageable.getPageNumber(), pageable.getPageSize())); | ||
|
|
||
| for (org.springframework.data.domain.Sort.Order o : pageable.getSort()) { | ||
| builder.withSort(s -> s.field(f -> f | ||
| .field(o.getProperty()) | ||
| .order(o.isAscending() | ||
| ? co.elastic.clients.elasticsearch._types.SortOrder.Asc | ||
| : co.elastic.clients.elasticsearch._types.SortOrder.Desc) | ||
| .unmappedType(FieldType.Keyword) | ||
| .missing("_last") | ||
| )); | ||
| } | ||
|
Comment on lines
+198
to
+207
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Hard-coding unmappedType=Keyword can break date/numeric sorts (e.g., creationDate). Elastic expects Apply this diff to select - for (org.springframework.data.domain.Sort.Order o : pageable.getSort()) {
- builder.withSort(s -> s.field(f -> f
- .field(o.getProperty())
- .order(o.isAscending()
- ? co.elastic.clients.elasticsearch._types.SortOrder.Asc
- : co.elastic.clients.elasticsearch._types.SortOrder.Desc)
- .unmappedType(FieldType.Keyword)
- .missing("_last")
- ));
- }
+ for (org.springframework.data.domain.Sort.Order o : pageable.getSort()) {
+ String prop = o.getProperty();
+ if ("_score".equals(prop)) {
+ builder.withSort(s -> s.score(sc -> sc.order(o.isAscending()
+ ? co.elastic.clients.elasticsearch._types.SortOrder.Asc
+ : co.elastic.clients.elasticsearch._types.SortOrder.Desc)));
+ continue;
+ }
+ builder.withSort(s -> s.field(f -> f
+ .field(prop)
+ .order(o.isAscending()
+ ? co.elastic.clients.elasticsearch._types.SortOrder.Asc
+ : co.elastic.clients.elasticsearch._types.SortOrder.Desc)
+ .unmappedType(resolveSortUnmappedType(prop))
+ .missing("_last")));
+ }Then add this helper in the class: // Heuristic mapping; extend as needed to cover your mapped fields
private FieldType resolveSortUnmappedType(String property) {
if (property.endsWith(".keyword")) return FieldType.Keyword;
if ("creationDate".equals(property) || property.endsWith("Date")) return FieldType.Date;
if ("priority".equals(property) || property.endsWith(".priority")) return FieldType.Long;
if (property.equals("_id") || property.endsWith("Id") || property.endsWith("Name")) return FieldType.Keyword;
return FieldType.Keyword; // safe default for string-ish fields
}🤖 Prompt for AI Agents |
||
|
|
||
| return builder.build(); | ||
| } | ||
|
|
||
| protected BoolQuery.Builder buildSingleQuery(CaseSearchRequest request, LoggedUser user, Locale locale) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,19 @@ | ||||||||||||||||||||||||||
| package com.netgrif.application.engine.objects.elastic.domain; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import lombok.Data; | ||||||||||||||||||||||||||
| import lombok.EqualsAndHashCode; | ||||||||||||||||||||||||||
| import lombok.NoArgsConstructor; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Data | ||||||||||||||||||||||||||
| @NoArgsConstructor | ||||||||||||||||||||||||||
| @EqualsAndHashCode(callSuper = true) | ||||||||||||||||||||||||||
| public abstract class StringCollectionField extends TextField { | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public String[] collectionValue; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public StringCollectionField(String[] values) { | ||||||||||||||||||||||||||
| super(values); | ||||||||||||||||||||||||||
| this.collectionValue = values; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+13
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Encapsulation of collectionValue Public mutable array invites accidental external mutation. Prefer at least protected to limit exposure (the adapter uses an accessor anyway). Apply this minimal change: - public String[] collectionValue;
+ protected String[] collectionValue;If feasible, consider returning an unmodifiable copy in the getter to prevent aliasing. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,17 +4,13 @@ | |||||||||||||||||||||||
| import lombok.NoArgsConstructor; | ||||||||||||||||||||||||
| import org.springframework.data.annotation.Id; | ||||||||||||||||||||||||
| import org.springframework.data.annotation.Version; | ||||||||||||||||||||||||
| import org.springframework.data.elasticsearch.annotations.DateFormat; | ||||||||||||||||||||||||
| import org.springframework.data.elasticsearch.annotations.Document; | ||||||||||||||||||||||||
| import org.springframework.data.elasticsearch.annotations.Field; | ||||||||||||||||||||||||
| import org.springframework.data.elasticsearch.annotations.FieldType; | ||||||||||||||||||||||||
| import org.springframework.data.elasticsearch.annotations.*; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import java.time.LocalDateTime; | ||||||||||||||||||||||||
| import java.util.Map; | ||||||||||||||||||||||||
| import java.util.Set; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import static org.springframework.data.elasticsearch.annotations.FieldType.Flattened; | ||||||||||||||||||||||||
| import static org.springframework.data.elasticsearch.annotations.FieldType.Keyword; | ||||||||||||||||||||||||
| import static org.springframework.data.elasticsearch.annotations.FieldType.*; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @NoArgsConstructor | ||||||||||||||||||||||||
| @Document(indexName = "#{@elasticCaseIndex}") | ||||||||||||||||||||||||
|
|
@@ -29,10 +25,30 @@ public void update(ElasticCase useCase) { | |||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @Id | ||||||||||||||||||||||||
| @Field(type = Keyword) | ||||||||||||||||||||||||
| public String getId() { | ||||||||||||||||||||||||
| return super.getId(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+28
to
31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Mapping changes: plan safe rollout (templates, reindex, aliases)
Also applies to: 33-40, 42-45, 47-50, 67-70, 82-89 🤖 Prompt for AI Agents |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @MultiField( | ||||||||||||||||||||||||
| mainField = @Field(type = Text), | ||||||||||||||||||||||||
| otherFields = { | ||||||||||||||||||||||||
| @InnerField(suffix = "keyword", type = Keyword) | ||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||
| public String getTitle() { | ||||||||||||||||||||||||
| return super.getTitle(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @Field(type = Keyword) | ||||||||||||||||||||||||
| public String getVisualId() { | ||||||||||||||||||||||||
| return super.getVisualId(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @Field(type = Keyword) | ||||||||||||||||||||||||
| public String getCaseId() { | ||||||||||||||||||||||||
| return super.getId(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @Version | ||||||||||||||||||||||||
| public Long getVersion() { | ||||||||||||||||||||||||
| return super.getVersion(); | ||||||||||||||||||||||||
|
|
@@ -48,7 +64,7 @@ public String getProcessId() { | |||||||||||||||||||||||
| return super.getProcessId(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @Field(type = FieldType.Date, format = DateFormat.date_hour_minute_second_millis) | ||||||||||||||||||||||||
| @Field(type = Date, format = DateFormat.date_hour_minute_second_millis) | ||||||||||||||||||||||||
| public LocalDateTime getCreationDate() { | ||||||||||||||||||||||||
| return super.getCreationDate(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+67
to
70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Date format change: ensure backward compatibility Switching to Suggested change: - @Field(type = Date, format = DateFormat.date_hour_minute_second_millis)
+ @Field(type = Date, format = {
+ DateFormat.date_hour_minute_second_millis,
+ DateFormat.date_optional_time
+ })
public LocalDateTime getCreationDate() {📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
|
|
@@ -63,7 +79,11 @@ public String getAuthorRealm() { | |||||||||||||||||||||||
| return super.getAuthorRealm(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @Field(type = Keyword) | ||||||||||||||||||||||||
| @MultiField( | ||||||||||||||||||||||||
| mainField = @Field(type = Text), | ||||||||||||||||||||||||
| otherFields = { | ||||||||||||||||||||||||
| @InnerField(suffix = "keyword", type = Keyword) | ||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||
| public String getAuthorName() { | ||||||||||||||||||||||||
| return super.getAuthorName(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+82
to
89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) AuthorName multi-field mapping — LGTM Covers full-text search and exact filters/sorts. If you need case-insensitive sorting, consider a normalizer-backed keyword subfield in the index settings. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| package com.netgrif.application.engine.adapter.spring.elastic.domain; | ||
|
|
||
| import lombok.Data; | ||
| import lombok.EqualsAndHashCode; | ||
| import lombok.NoArgsConstructor; | ||
| import org.springframework.data.elasticsearch.annotations.Field; | ||
|
|
||
| import static org.springframework.data.elasticsearch.annotations.FieldType.Keyword; | ||
| import static org.springframework.data.elasticsearch.annotations.FieldType.Text; | ||
|
|
||
| @Data | ||
| @NoArgsConstructor | ||
| @EqualsAndHashCode(callSuper = true) | ||
| public class StringCollectionField extends com.netgrif.application.engine.objects.elastic.domain.StringCollectionField { | ||
|
|
||
| public StringCollectionField(String[] values) { | ||
| super(values); | ||
| } | ||
|
|
||
| @Override | ||
| @Field(type = Text) | ||
| public String[] getFulltextValue() { | ||
| return super.getFulltextValue(); | ||
| } | ||
|
|
||
| @Override | ||
| @Field(type = Keyword) | ||
| public String[] getCollectionValue() { | ||
| return super.getCollectionValue(); | ||
| } | ||
|
Comment on lines
+26
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Plan reindex if changing existing mappings. |
||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Unsafe cast and null-element handling in transformStringCollectionField
Direct toArray(new String[0]) will throw if the collection contains non-String elements; nulls are not filtered. Make it type-safe and robust.
Apply this diff:
I can add unit tests covering: null/empty collections, mixed-type collections (ensure filtering), and successful mapping.
📝 Committable suggestion
🤖 Prompt for AI Agents