Skip to content

Commit 34806ac

Browse files
thiagohorathiagohora
authored
[OPIK-5023] [BE] refactor(filter): introduce ENUM_LEGACY field type for source legacy fallback (#5846)
* [OPIK-5023] [BE] refactor(filter): introduce ENUM_LEGACY field type for source legacy fallback Centralise the legacy-fallback operator handling inside ANALYTICS_DB_OPERATOR_MAP by introducing FieldType.ENUM_LEGACY. The EQUAL and NOT_EQUAL templates for the new type encode the OR/AND fallback SQL directly in the map, removing the need for the separate LEGACY_FALLBACK_OPERATOR_MAP and buildWithLegacyFallback method. TraceField.SOURCE and SpanField.SOURCE are re-typed to ENUM_LEGACY so the behaviour is driven by the field type, consistent with every other operator/type pair. * fix(test): add ENUM_LEGACY to exhaustive FieldType switches in filter resource tests * refactor(tests): delegate filter helper methods to shared FilterTestUtils Replace duplicate getValidValue/getKey/getInvalidValue implementations in FindTraceThreadsResourceTest and GetTracesByProjectResourceTest with delegation to the shared FilterTestUtils utility class. * fix(filter): fix MissingFormatArgumentException and dedup legacyFallbackDbValue Fix MissingFormatArgumentException in toAnalyticsDbFilter when an ENUM_LEGACY field has no legacy fallback for a given value (e.g. source = "ui"): fall back to the plain ENUM template for that operator instead of calling template.formatted with too few arguments. Replace identical legacyFallbackDbValue anonymous overrides on TraceField.SOURCE and SpanField.SOURCE with a constructor-injected Function, eliminating the duplicate enum constant body. * test: add FilterTestUtils shared helper for filter test methods * refactor(filter): use @requiredargsconstructor and inline FilterTestUtils calls * fix: add missing FilterTestUtils import in FindSpansResourceTest * [OPIK-5023] [BE] refactor: apply ENUM_LEGACY function-based pattern to TraceThreadField * [OPIK-5023] [BE] refactor(filter): move legacy fallback logic to FieldType Per review feedback: - ENUM_LEGACY.buildFilter overrides the FieldType default, encoding the OR/AND legacy-clause logic directly on the type. Source::legacyFallbackDbValue is called once, in one place. - Field enums (SpanField, TraceField, TraceThreadField) no longer carry a Function<String,Optional<String>> lambda; SOURCE just uses ENUM_LEGACY. - FilterQueryBuilder.toAnalyticsDbFilter delegates to the type's buildFilter, removing the if(ENUM_LEGACY) discriminator. Adding a new type with similar behaviour only requires overriding buildFilter in FieldType. - legacyFallbackDbValue removed from Field interface; it lives on FieldType. - FilterTestUtils uses @UtilityClass instead of manual private constructor.
1 parent 1e5ac78 commit 34806ac

File tree

11 files changed

+113
-140
lines changed

11 files changed

+113
-140
lines changed

‎apps/opik-backend/src/main/java/com/comet/opik/api/filter/Field.java‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@
33
import com.comet.opik.domain.filter.FilterStrategy;
44
import com.fasterxml.jackson.annotation.JsonValue;
55

6-
import java.util.Optional;
7-
86
public interface Field {
97

108
String ID_QUERY_PARAM = "id";
@@ -77,15 +75,4 @@ default boolean isDynamic(FilterStrategy filterStrategy) {
7775
return false;
7876
}
7977

80-
/**
81-
* Returns an additional DB value to OR/AND-include alongside the primary filter value, to capture
82-
* rows that predate a field's introduction (e.g. legacy {@code unknown} rows treated as {@code sdk}).
83-
* <p>
84-
* Fields that have no legacy fallback return {@link Optional#empty()} (the default).
85-
* </p>
86-
*/
87-
default Optional<String> legacyFallbackDbValue(String filterValue) {
88-
return Optional.empty();
89-
}
90-
9178
}

‎apps/opik-backend/src/main/java/com/comet/opik/api/filter/FieldType.java‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
11
package com.comet.opik.api.filter;
22

3+
import com.comet.opik.api.Source;
34
import com.fasterxml.jackson.annotation.JsonValue;
45
import lombok.Getter;
56
import lombok.RequiredArgsConstructor;
67

8+
import java.util.Optional;
9+
710
@RequiredArgsConstructor
811
@Getter
912
public enum FieldType {
@@ -20,10 +23,45 @@ public enum FieldType {
2023
MAP("map"),
2124
LIST("list"),
2225
ENUM("enum"),
26+
ENUM_LEGACY("enum_legacy") {
27+
@Override
28+
public String buildFilter(String template, String dbField, int i, String filterValue,
29+
String enumFallbackTemplate) {
30+
return Source.legacyFallbackDbValue(filterValue)
31+
.map(fallback -> "(%s)".formatted(template.formatted(dbField, i, fallback)))
32+
.orElseGet(() -> "(%s)".formatted(enumFallbackTemplate.formatted(dbField, i)));
33+
}
34+
},
2335
ERROR_CONTAINER("error_container"),
2436
CUSTOM("custom"),
2537
;
2638

2739
@JsonValue
2840
private final String queryParamType;
41+
42+
/**
43+
* Formats the ClickHouse filter expression for this field type.
44+
* <p>
45+
* Types with legacy fallback behaviour (e.g. {@link #ENUM_LEGACY}) override this method to
46+
* include the additional OR/AND clause for rows that predate the field's introduction.
47+
* All other types apply the two-argument template directly.
48+
* </p>
49+
*
50+
* @param template the operator template for this field type (may contain {@code %3$s} for legacy types)
51+
* @param dbField the ClickHouse column name
52+
* @param i the filter index used as the bind-parameter suffix
53+
* @param filterValue the raw filter value supplied by the caller
54+
* @param enumFallbackTemplate the two-argument {@link #ENUM} template for the same operator,
55+
* used when a legacy type has no fallback mapping for {@code filterValue}
56+
* @return the fully-formatted {@code (…)} filter clause
57+
*/
58+
public String buildFilter(String template, String dbField, int i, String filterValue,
59+
String enumFallbackTemplate) {
60+
return "(%s)".formatted(template.formatted(dbField, i));
61+
}
62+
63+
/** Returns the legacy DB value to OR/AND alongside the primary filter value, if one exists. */
64+
public Optional<String> legacyFallbackDbValue(String filterValue) {
65+
return Optional.empty();
66+
}
2967
}

‎apps/opik-backend/src/main/java/com/comet/opik/api/filter/FiltersFactory.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ public class FiltersFactory {
4242
.put(FieldType.STRING_EXACT, filter -> StringUtils.isNotBlank(filter.value()))
4343
.put(FieldType.STRING_STATE_DB, filter -> StringUtils.isNotBlank(filter.value()))
4444
.put(FieldType.ENUM, filter -> StringUtils.isNotBlank(filter.value()))
45+
.put(FieldType.ENUM_LEGACY, filter -> StringUtils.isNotBlank(filter.value()))
4546
.put(FieldType.DATE_TIME, filter -> {
4647
try {
4748
Instant.parse(filter.value());

‎apps/opik-backend/src/main/java/com/comet/opik/api/filter/SpanField.java‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,10 @@
11
package com.comet.opik.api.filter;
22

3-
import com.comet.opik.api.Source;
43
import lombok.Getter;
54
import lombok.RequiredArgsConstructor;
65

7-
import java.util.Optional;
8-
9-
@RequiredArgsConstructor
106
@Getter
7+
@RequiredArgsConstructor
118
public enum SpanField implements Field {
129
ID(ID_QUERY_PARAM, FieldType.STRING_EXACT),
1310
NAME(NAME_QUERY_PARAM, FieldType.STRING),
@@ -32,12 +29,7 @@ public enum SpanField implements Field {
3229
ERROR_TYPE(ERROR_TYPE_QUERY_PARAM, FieldType.STRING),
3330
TYPE(TYPE_QUERY_PARAM, FieldType.ENUM),
3431
TRACE_ID(TRACE_ID_QUERY_PARAM, FieldType.STRING_EXACT),
35-
SOURCE(SOURCE_QUERY_PARAM, FieldType.ENUM) {
36-
@Override
37-
public Optional<String> legacyFallbackDbValue(String filterValue) {
38-
return Source.legacyFallbackDbValue(filterValue);
39-
}
40-
},
32+
SOURCE(SOURCE_QUERY_PARAM, FieldType.ENUM_LEGACY),
4133
CUSTOM(CUSTOM_QUERY_PARAM, FieldType.CUSTOM),
4234
;
4335

‎apps/opik-backend/src/main/java/com/comet/opik/api/filter/TraceField.java‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,10 @@
11
package com.comet.opik.api.filter;
22

3-
import com.comet.opik.api.Source;
43
import lombok.Getter;
54
import lombok.RequiredArgsConstructor;
65

7-
import java.util.Optional;
8-
9-
@RequiredArgsConstructor
106
@Getter
7+
@RequiredArgsConstructor
118
public enum TraceField implements Field {
129
ID(ID_QUERY_PARAM, FieldType.STRING_EXACT),
1310
NAME(NAME_QUERY_PARAM, FieldType.STRING),
@@ -38,12 +35,7 @@ public enum TraceField implements Field {
3835
CUSTOM(CUSTOM_QUERY_PARAM, FieldType.CUSTOM),
3936
ANNOTATION_QUEUE_IDS(ANNOTATION_QUEUE_IDS_QUERY_PARAM, FieldType.LIST),
4037
EXPERIMENT_ID(EXPERIMENT_ID_QUERY_PARAM, FieldType.STRING_EXACT),
41-
SOURCE(SOURCE_QUERY_PARAM, FieldType.ENUM) {
42-
@Override
43-
public Optional<String> legacyFallbackDbValue(String filterValue) {
44-
return Source.legacyFallbackDbValue(filterValue);
45-
}
46-
},
38+
SOURCE(SOURCE_QUERY_PARAM, FieldType.ENUM_LEGACY),
4739
;
4840

4941
private final String queryParamField;

‎apps/opik-backend/src/main/java/com/comet/opik/api/filter/TraceThreadField.java‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,10 @@
11
package com.comet.opik.api.filter;
22

3-
import com.comet.opik.api.Source;
43
import lombok.Getter;
54
import lombok.RequiredArgsConstructor;
65

7-
import java.util.Optional;
8-
9-
@RequiredArgsConstructor
106
@Getter
7+
@RequiredArgsConstructor
118
public enum TraceThreadField implements Field {
129
ID(ID_QUERY_PARAM, FieldType.STRING_EXACT),
1310
FIRST_MESSAGE(FIRST_MESSAGE_QUERY_PARAM, FieldType.STRING),
@@ -22,12 +19,7 @@ public enum TraceThreadField implements Field {
2219
STATUS(STATUS_QUERY_PARAM, FieldType.ENUM),
2320
TAGS(TAGS_QUERY_PARAM, FieldType.LIST),
2421
ANNOTATION_QUEUE_IDS(ANNOTATION_QUEUE_IDS_QUERY_PARAM, FieldType.LIST),
25-
SOURCE(SOURCE_QUERY_PARAM, FieldType.ENUM) {
26-
@Override
27-
public Optional<String> legacyFallbackDbValue(String filterValue) {
28-
return Source.legacyFallbackDbValue(filterValue);
29-
}
30-
},
22+
SOURCE(SOURCE_QUERY_PARAM, FieldType.ENUM_LEGACY),
3123
;
3224

3325
private final String queryParamField;

‎apps/opik-backend/src/main/java/com/comet/opik/domain/filter/FilterQueryBuilder.java‎

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,8 @@ public class FilterQueryBuilder {
207207
// First remove escaped quotes with replaceAll, then trim remaining quotes with trimBoth
208208
Map.entry(FieldType.MAP,
209209
"lower(trimBoth(replaceAll(arrayElement(mapValues(%1$s),indexOf(mapKeys(%1$s), :filterKey%2$d)), '\\\\\"', ''), '\"')) = lower(:filter%2$d)"),
210-
Map.entry(FieldType.ENUM, "%1$s = :filter%2$d"))))
210+
Map.entry(FieldType.ENUM, "%1$s = :filter%2$d"),
211+
Map.entry(FieldType.ENUM_LEGACY, "(%1$s = :filter%2$d OR %1$s = '%3$s')"))))
211212
.put(Operator.NOT_EQUAL, new EnumMap<>(Map.ofEntries(
212213
Map.entry(FieldType.STRING, "lower(%1$s) != lower(:filter%2$d)"),
213214
Map.entry(FieldType.STRING_EXACT, "%1$s != :filter%2$d"),
@@ -227,7 +228,8 @@ public class FilterQueryBuilder {
227228
// First remove escaped quotes with replaceAll, then trim remaining quotes with trimBoth
228229
Map.entry(FieldType.MAP,
229230
"lower(trimBoth(replaceAll(arrayElement(mapValues(%1$s),indexOf(mapKeys(%1$s), :filterKey%2$d)), '\\\\\"', ''), '\"')) != lower(:filter%2$d)"),
230-
Map.entry(FieldType.ENUM, "%1$s != :filter%2$d"))))
231+
Map.entry(FieldType.ENUM, "%1$s != :filter%2$d"),
232+
Map.entry(FieldType.ENUM_LEGACY, "(%1$s != :filter%2$d AND %1$s != '%3$s')"))))
231233
.put(Operator.GREATER_THAN, new EnumMap<>(Map.ofEntries(
232234
Map.entry(FieldType.STRING, "lower(%1$s) > lower(:filter%2$d)"),
233235
Map.entry(FieldType.STRING_EXACT, "%1$s > :filter%2$d"),
@@ -925,20 +927,8 @@ private static boolean isFeedbackScore(Filter filter) {
925927
private static String toAnalyticsDbFilter(Filter filter, int i, FilterStrategy filterStrategy) {
926928
var template = toAnalyticsDbOperator(filter, filterStrategy);
927929
var dbField = getAnalyticsDbField(filter.field(), filterStrategy, i);
928-
var formattedTemplate = template.formatted(dbField, i);
929-
930-
return filter.field().legacyFallbackDbValue(filter.value())
931-
.map(fallback -> buildWithLegacyFallback(filter.operator(), dbField, i, fallback, formattedTemplate))
932-
.orElse("(%s)".formatted(formattedTemplate));
933-
}
934-
935-
private static String buildWithLegacyFallback(
936-
Operator operator, String dbField, int i, String fallback, String formattedTemplate) {
937-
return switch (operator) {
938-
case EQUAL -> "(%s = :filter%d OR %s = '%s')".formatted(dbField, i, dbField, fallback);
939-
case NOT_EQUAL -> "(%s != :filter%d AND %s != '%s')".formatted(dbField, i, dbField, fallback);
940-
default -> "(%s)".formatted(formattedTemplate);
941-
};
930+
var enumFallbackTemplate = ANALYTICS_DB_OPERATOR_MAP.get(filter.operator()).get(FieldType.ENUM);
931+
return filter.field().getType().buildFilter(template, dbField, i, filter.value(), enumFallbackTemplate);
942932
}
943933

944934
private static String getAnalyticsDbField(Field field, FilterStrategy filterStrategy, int i) {

‎apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/FilterTestUtils.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
package com.comet.opik.api.resources.utils;
2+
3+
import com.comet.opik.api.filter.Field;
4+
import lombok.experimental.UtilityClass;
5+
import org.apache.commons.lang3.RandomStringUtils;
6+
import org.apache.commons.lang3.RandomUtils;
7+
8+
import java.time.Instant;
9+
10+
@UtilityClass
11+
public class FilterTestUtils {
12+
13+
public static String getValidValue(Field field) {
14+
return switch (field.getType()) {
15+
case STRING, STRING_EXACT, LIST, DICTIONARY, DICTIONARY_STATE_DB, MAP, ENUM, ENUM_LEGACY,
16+
ERROR_CONTAINER, STRING_STATE_DB, CUSTOM ->
17+
RandomStringUtils.secure().nextAlphanumeric(10);
18+
case NUMBER, DURATION, FEEDBACK_SCORES_NUMBER -> String.valueOf(RandomUtils.secure().randomInt(1, 10));
19+
case DATE_TIME, DATE_TIME_STATE_DB -> Instant.now().toString();
20+
};
21+
}
22+
23+
public static String getKey(Field field) {
24+
return switch (field.getType()) {
25+
case STRING, STRING_EXACT, NUMBER, DURATION, DATE_TIME, LIST, ENUM, ENUM_LEGACY,
26+
ERROR_CONTAINER, STRING_STATE_DB, DATE_TIME_STATE_DB ->
27+
null;
28+
case FEEDBACK_SCORES_NUMBER, CUSTOM -> RandomStringUtils.secure().nextAlphanumeric(10);
29+
case DICTIONARY, DICTIONARY_STATE_DB, MAP -> "";
30+
};
31+
}
32+
33+
public static String getInvalidValue(Field field) {
34+
return switch (field.getType()) {
35+
case STRING, STRING_EXACT, DICTIONARY, DICTIONARY_STATE_DB, MAP, CUSTOM, LIST, ENUM, ENUM_LEGACY,
36+
ERROR_CONTAINER, STRING_STATE_DB, DATE_TIME_STATE_DB ->
37+
" ";
38+
case NUMBER, DURATION, DATE_TIME, FEEDBACK_SCORES_NUMBER ->
39+
RandomStringUtils.secure().nextAlphanumeric(10);
40+
};
41+
}
42+
}

‎apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/FindSpansResourceTest.java‎

Lines changed: 12 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import com.comet.opik.api.resources.utils.ClickHouseContainerUtils;
1717
import com.comet.opik.api.resources.utils.ClientSupportUtils;
1818
import com.comet.opik.api.resources.utils.DurationUtils;
19+
import com.comet.opik.api.resources.utils.FilterTestUtils;
1920
import com.comet.opik.api.resources.utils.MigrationUtils;
2021
import com.comet.opik.api.resources.utils.MinIOContainerUtils;
2122
import com.comet.opik.api.resources.utils.MySQLContainerUtils;
@@ -465,36 +466,6 @@ private Stream<Arguments> getTtftArgs() {
465466
arguments("/spans/search", spanStreamTestAssertion, arg.get()[0], arg.get()[1])));
466467
}
467468

468-
private String getValidValue(Field field) {
469-
return switch (field.getType()) {
470-
case STRING, STRING_EXACT, LIST, DICTIONARY, DICTIONARY_STATE_DB, MAP, ENUM, ERROR_CONTAINER,
471-
STRING_STATE_DB, CUSTOM ->
472-
RandomStringUtils.secure().nextAlphanumeric(10);
473-
case NUMBER, DURATION, FEEDBACK_SCORES_NUMBER -> String.valueOf(randomNumber(1, 10));
474-
case DATE_TIME, DATE_TIME_STATE_DB -> Instant.now().toString();
475-
};
476-
}
477-
478-
private String getKey(Field field) {
479-
return switch (field.getType()) {
480-
case STRING, STRING_EXACT, NUMBER, DURATION, DATE_TIME, LIST, ENUM, ERROR_CONTAINER,
481-
STRING_STATE_DB, DATE_TIME_STATE_DB ->
482-
null;
483-
case FEEDBACK_SCORES_NUMBER, CUSTOM -> RandomStringUtils.secure().nextAlphanumeric(10);
484-
case DICTIONARY, DICTIONARY_STATE_DB, MAP -> "";
485-
};
486-
}
487-
488-
private String getInvalidValue(Field field) {
489-
return switch (field.getType()) {
490-
case STRING, STRING_EXACT, DICTIONARY, DICTIONARY_STATE_DB, MAP, CUSTOM, LIST, ENUM, ERROR_CONTAINER,
491-
STRING_STATE_DB, DATE_TIME_STATE_DB ->
492-
" ";
493-
case NUMBER, DURATION, DATE_TIME, FEEDBACK_SCORES_NUMBER ->
494-
RandomStringUtils.secure().nextAlphanumeric(10);
495-
};
496-
}
497-
498469
private Stream<Arguments> getFilterInvalidOperatorForFieldTypeArgs() {
499470
return filterQueryBuilder.getUnSupportedOperators(SpanField.values())
500471
.entrySet()
@@ -505,20 +476,20 @@ private Stream<Arguments> getFilterInvalidOperatorForFieldTypeArgs() {
505476
Arguments.of("/stats", SpanFilter.builder()
506477
.field(filter.getKey())
507478
.operator(operator)
508-
.key(getKey(filter.getKey()))
509-
.value(getValidValue(filter.getKey()))
479+
.key(FilterTestUtils.getKey(filter.getKey()))
480+
.value(FilterTestUtils.getValidValue(filter.getKey()))
510481
.build()),
511482
Arguments.of("", SpanFilter.builder()
512483
.field(filter.getKey())
513484
.operator(operator)
514-
.key(getKey(filter.getKey()))
515-
.value(getValidValue(filter.getKey()))
485+
.key(FilterTestUtils.getKey(filter.getKey()))
486+
.value(FilterTestUtils.getValidValue(filter.getKey()))
516487
.build()),
517488
Arguments.of("/search", SpanFilter.builder()
518489
.field(filter.getKey())
519490
.operator(operator)
520-
.key(getKey(filter.getKey()))
521-
.value(getValidValue(filter.getKey()))
491+
.key(FilterTestUtils.getKey(filter.getKey()))
492+
.value(FilterTestUtils.getValidValue(filter.getKey()))
522493
.build()))));
523494
}
524495

@@ -536,16 +507,16 @@ private Stream<Arguments> getFilterInvalidValueOrKeyForFieldTypeArgs() {
536507
.field(filter.getKey())
537508
.operator(operator)
538509
.key(null)
539-
.value(getValidValue(filter.getKey()))
510+
.value(FilterTestUtils.getValidValue(filter.getKey()))
540511
.build(),
541512
SpanFilter.builder()
542513
.field(filter.getKey())
543514
.operator(operator)
544515
// if no value is expected, create an invalid filter by an empty key
545516
.key(Operator.NO_VALUE_OPERATORS.contains(operator)
546517
? ""
547-
: getKey(filter.getKey()))
548-
.value(getInvalidValue(filter.getKey()))
518+
: FilterTestUtils.getKey(filter.getKey()))
519+
.value(FilterTestUtils.getInvalidValue(filter.getKey()))
549520
.build());
550521
case ERROR_CONTAINER -> Stream.of();
551522
case LIST -> {
@@ -557,13 +528,13 @@ private Stream<Arguments> getFilterInvalidValueOrKeyForFieldTypeArgs() {
557528
yield Stream.of(SpanFilter.builder()
558529
.field(filter.getKey())
559530
.operator(operator)
560-
.value(getInvalidValue(filter.getKey()))
531+
.value(FilterTestUtils.getInvalidValue(filter.getKey()))
561532
.build());
562533
}
563534
default -> Stream.of(SpanFilter.builder()
564535
.field(filter.getKey())
565536
.operator(operator)
566-
.value(getInvalidValue(filter.getKey()))
537+
.value(FilterTestUtils.getInvalidValue(filter.getKey()))
567538
.build());
568539
}));
569540

0 commit comments

Comments
 (0)