Skip to content

Commit 177ec02

Browse files
thiagohoraclaude
thiagohora
and
claude
committed
[OPIK-7499] [BE] fix: build parseable JSONPaths for dictionary filter keys
Filtering a dictionary field on a key containing characters outside [A-Za-z0-9_] built an invalid ClickHouse JSONPath. ClickHouse parses that argument while analysing the query, so the statement aborted with BAD_ARGUMENTS rather than returning no rows, surfacing as a 500. JSON_VALUE requires the path to be a constant, so it has to be correct by construction. Such a key is now quoted segment by segment into bracket notation, which can express any character and therefore always parses. A key whose every segment is expressible in dot notation still produces the exact path it produced before, and an expression the caller authored is passed through untouched so that shapes such as wildcard indices keep working. Brackets only count as path syntax when their content is an index, a wildcard or a quoted key, so a literal key like feature[beta] is quoted rather than handed over as a subscript. A leading dollar only roots an expression when what follows continues the path, so ordinary keys such as $schema and $ref are quoted instead of aborting the query. An authored expression too damaged to parse under any grammar is quoted into a literal key no document carries, so the query runs and matches nothing instead of aborting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 6ae37c4 commit 177ec02

5 files changed

Lines changed: 414 additions & 24 deletions

File tree

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

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.comet.opik.domain;
22

33
import com.comet.opik.api.grouping.GroupBy;
4+
import com.comet.opik.domain.filter.JsonPathUtils;
45
import jakarta.inject.Singleton;
56
import jakarta.ws.rs.BadRequestException;
67
import lombok.NonNull;
@@ -11,8 +12,6 @@
1112
import java.util.stream.Collectors;
1213
import java.util.stream.IntStream;
1314

14-
import static com.comet.opik.domain.filter.FilterQueryBuilder.JSONPATH_ROOT;
15-
1615
@Singleton
1716
@Slf4j
1817
public class GroupingQueryBuilder {
@@ -60,23 +59,10 @@ public void addGroupingTemplateParams(@NonNull List<GroupBy> groups, @NonNull ST
6059

6160
private String getKeyAndValidate(GroupBy group) {
6261

63-
String key = getKey(group);
62+
String key = JsonPathUtils.toRootedJsonPath(group.key());
6463
return isValidJsonPath(key) ? key : DUMMY_JSON_KEY;
6564
}
6665

67-
private String getKey(GroupBy group) {
68-
69-
if (group.key().startsWith(JSONPATH_ROOT)) {
70-
return group.key();
71-
}
72-
73-
if (group.key().startsWith("[") || group.key().startsWith(".")) {
74-
return "%s%s".formatted(JSONPATH_ROOT, group.key());
75-
}
76-
77-
return "%s.%s".formatted(JSONPATH_ROOT, group.key());
78-
}
79-
8066
static boolean isValidJsonPath(String path) {
8167
// must start with "$" and match allowed patterns
8268
return path.matches(VALID_JSON_KEY_REGEXP);

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

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1363,23 +1363,33 @@ private static String getSQLJsonPath(String jsonKey) {
13631363
return "%s.\"%s\"".formatted(JSONPATH_ROOT, jsonKey);
13641364
}
13651365

1366+
/**
1367+
* Resolves the key bound as {@code :filterKey} for a filter.
1368+
* <p>
1369+
* The analytics DB path is delegated to {@link JsonPathUtils#toAnalyticsDbJsonPath(String)} so that
1370+
* a key holding characters unquoted dot notation cannot express resolves normally instead of
1371+
* aborting the query. The state DB path is unchanged.
1372+
*/
13661373
private static String getKey(Filter filter) {
13671374

1368-
if (filter.key().startsWith(JSONPATH_ROOT)
1369-
|| (filter.field().getType() != FieldType.DICTIONARY
1370-
&& filter.field().getType() != FieldType.DICTIONARY_STATE_DB)) {
1375+
if (filter.field().getType() != FieldType.DICTIONARY
1376+
&& filter.field().getType() != FieldType.DICTIONARY_STATE_DB) {
13711377
return filter.key();
13721378
}
13731379

1374-
if (filter.key().startsWith("[") || filter.key().startsWith(".")) {
1375-
return "%s%s".formatted(JSONPATH_ROOT, filter.key());
1380+
if (filter.field().getType() == FieldType.DICTIONARY) {
1381+
return JsonPathUtils.toAnalyticsDbJsonPath(filter.key());
13761382
}
13771383

1378-
if (filter.field().getType() == FieldType.DICTIONARY_STATE_DB) {
1379-
return getSQLJsonPath(filter.key());
1384+
if (filter.key().startsWith(JSONPATH_ROOT)) {
1385+
return filter.key();
1386+
}
1387+
1388+
if (filter.key().startsWith("[") || filter.key().startsWith(".")) {
1389+
return "%s%s".formatted(JSONPATH_ROOT, filter.key());
13801390
}
13811391

1382-
return "%s.%s".formatted(JSONPATH_ROOT, filter.key());
1392+
return getSQLJsonPath(filter.key());
13831393
}
13841394

13851395
/**

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

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
package com.comet.opik.domain.filter;
2+
3+
import lombok.NonNull;
4+
import lombok.experimental.UtilityClass;
5+
6+
import java.util.Arrays;
7+
import java.util.regex.Pattern;
8+
import java.util.stream.Collectors;
9+
10+
import static com.comet.opik.domain.filter.FilterQueryBuilder.JSONPATH_ROOT;
11+
12+
/**
13+
* Builds the JSONPath expressions used to address dynamic dictionary fields (typically
14+
* {@code metadata}) when querying the analytics DB.
15+
* <p>
16+
* ClickHouse parses the JSONPath argument of {@code JSON_VALUE} while it analyses the query, before
17+
* execution starts. An expression it cannot parse therefore aborts the whole statement with
18+
* {@code BAD_ARGUMENTS: Unable to parse JSONPath} rather than yielding no rows, and there is no
19+
* runtime option that can soften it. The {@code RETURNING ... NULL ON ERROR} clause used elsewhere in
20+
* {@link FilterQueryBuilder} for the state DB is MySQL-only syntax and ClickHouse rejects it outright.
21+
* The path therefore has to be settled while it is being built.
22+
* <p>
23+
* Unquoted dot notation only accepts {@code [A-Za-z0-9_]} in a key, so a key holding any other
24+
* character — a hyphen being by far the most common — cannot be expressed that way. Bracket notation
25+
* accepts arbitrary characters once quoted and is used for those keys.
26+
*/
27+
@UtilityClass
28+
public class JsonPathUtils {
29+
30+
private static final Pattern DOT_NOTATION_SEGMENT = Pattern.compile("[A-Za-z0-9_]+");
31+
32+
/**
33+
* A subscript that carries path meaning rather than being part of a key: an array index, a
34+
* wildcard, or a quoted key.
35+
*/
36+
private static final Pattern PATH_SUBSCRIPT = Pattern.compile("\\[(?:\\d+|\\*|'(?:[^'\\\\]|\\\\.)*')]");
37+
38+
private static final String PATH_SEPARATOR = ".";
39+
40+
/**
41+
* Resolves a dictionary filter key into a JSONPath for the analytics DB.
42+
* <p>
43+
* A key that already carries JSONPath syntax was authored by the caller, so it is assembled
44+
* exactly as before and only screened for damage. It is deliberately not matched against an
45+
* allowlist of accepted shapes: ClickHouse accepts constructs such as wildcard indices
46+
* ({@code version[*]}) that no such list here has enumerated, and rejecting them would silently
47+
* break filters that work today.
48+
* <p>
49+
* Anything else is quoted segment by segment into bracket notation, which can express any
50+
* character and therefore always parses. That covers both a plain key holding characters dot
51+
* notation cannot express, which then resolves normally, and an authored expression too damaged to
52+
* parse, which becomes a literal key no document carries and so simply matches nothing. Either way
53+
* the query runs instead of aborting.
54+
* <p>
55+
* A plain key whose every segment is expressible in dot notation keeps producing the exact path it
56+
* produced before, and the dot keeps its meaning as the segment separator throughout.
57+
*
58+
* @param key dictionary key, e.g. {@code environment} or {@code hidden_params.retry-count}
59+
* @return the JSONPath, e.g. {@code $.environment} or {@code $['hidden_params']['retry-count']}
60+
*/
61+
public static String toAnalyticsDbJsonPath(@NonNull String key) {
62+
var segments = key.split("\\.", -1);
63+
64+
if (isPathExpression(key)) {
65+
var path = toRootedJsonPath(key);
66+
67+
return isStructurallySound(path) ? path : toBracketNotation(segments);
68+
}
69+
70+
return isExpressibleInDotNotation(segments)
71+
? toRootedJsonPath(key)
72+
: toBracketNotation(segments);
73+
}
74+
75+
private static boolean isPathExpression(String key) {
76+
if (isRooted(key) || key.startsWith("[") || key.startsWith(PATH_SEPARATOR)) {
77+
return true;
78+
}
79+
80+
return hasSubscript(key) && everySubscriptCarriesPathMeaning(key);
81+
}
82+
83+
/**
84+
* A leading {@code $} only roots an expression when what follows continues the path: nothing at
85+
* all, a separator, or a subscript. A key such as {@code $schema} or {@code $ref} merely begins
86+
* with the character and is an ordinary key, so it is quoted rather than handed to ClickHouse as
87+
* an expression it cannot parse.
88+
*/
89+
private static boolean isRooted(String key) {
90+
if (!key.startsWith(JSONPATH_ROOT)) {
91+
return false;
92+
}
93+
94+
var afterRoot = key.substring(JSONPATH_ROOT.length());
95+
96+
return afterRoot.isEmpty() || afterRoot.startsWith(PATH_SEPARATOR) || afterRoot.startsWith("[");
97+
}
98+
99+
private static boolean hasSubscript(String key) {
100+
return key.indexOf('[') >= 0 || key.indexOf(']') >= 0;
101+
}
102+
103+
/**
104+
* Distinguishes {@code version[*]}, where the brackets subscript the key, from
105+
* {@code feature[beta]}, where they are part of the key itself. A bracket only means subscript if
106+
* its content is an index, a wildcard or a quoted key; anything else leaves the whole thing a
107+
* literal key, which is then quoted rather than handed to ClickHouse as path syntax.
108+
*/
109+
private static boolean everySubscriptCarriesPathMeaning(String key) {
110+
return !hasSubscript(PATH_SUBSCRIPT.matcher(key).replaceAll(""));
111+
}
112+
113+
/**
114+
* Screens an authored expression for damage that no JSONPath can survive: unbalanced brackets, an
115+
* unterminated quote, or a trailing separator with nothing after it.
116+
* <p>
117+
* This only ever rejects expressions that cannot parse under any grammar, so a working filter
118+
* cannot be turned into an empty one. It is not a completeness check — an expression that is
119+
* well-formed here may still be rejected by ClickHouse.
120+
*/
121+
private static boolean isStructurallySound(String path) {
122+
if (path.endsWith(PATH_SEPARATOR)) {
123+
return false;
124+
}
125+
126+
var depth = 0;
127+
var quoted = false;
128+
129+
for (var i = 0; i < path.length(); i++) {
130+
var current = path.charAt(i);
131+
132+
if (quoted) {
133+
if (current == '\\') {
134+
i++;
135+
} else if (current == '\'') {
136+
quoted = false;
137+
}
138+
continue;
139+
}
140+
141+
switch (current) {
142+
case '\'' -> quoted = true;
143+
case '[' -> depth++;
144+
case ']' -> depth--;
145+
default -> {
146+
}
147+
}
148+
149+
if (depth < 0) {
150+
return false;
151+
}
152+
}
153+
154+
return depth == 0 && !quoted;
155+
}
156+
157+
private static boolean isExpressibleInDotNotation(String[] segments) {
158+
return Arrays.stream(segments).allMatch(segment -> DOT_NOTATION_SEGMENT.matcher(segment).matches());
159+
}
160+
161+
/**
162+
* Prefixes a key with the root so it reads as a path, e.g. {@code .a} and {@code a} both become
163+
* {@code $.a}. Whatever syntax the key already carries — subscripts, wildcards, quoted segments —
164+
* is left exactly as written; only the root is supplied. Shared with
165+
* {@link com.comet.opik.domain.GroupingQueryBuilder} so the two agree on what a rooted path is.
166+
*/
167+
public static String toRootedJsonPath(String key) {
168+
if (key.startsWith(JSONPATH_ROOT)) {
169+
return key;
170+
}
171+
172+
if (key.startsWith("[") || key.startsWith(PATH_SEPARATOR)) {
173+
return "%s%s".formatted(JSONPATH_ROOT, key);
174+
}
175+
176+
return "%s%s%s".formatted(JSONPATH_ROOT, PATH_SEPARATOR, key);
177+
}
178+
179+
/**
180+
* Renders {@code a.b-c} as {@code $['a']['b-c']}.
181+
*/
182+
private static String toBracketNotation(String[] segments) {
183+
return Arrays.stream(segments)
184+
.map(JsonPathUtils::quoteSegment)
185+
.collect(Collectors.joining("", JSONPATH_ROOT, ""));
186+
}
187+
188+
private static String quoteSegment(String segment) {
189+
return "['%s']".formatted(segment.replace("\\", "\\\\").replace("'", "\\'"));
190+
}
191+
}

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2217,6 +2217,57 @@ void whenFilterMetadataEqualString__thenReturnSpansFiltered(String endpoint,
22172217
values.all(), filters, Map.of());
22182218
}
22192219

2220+
@ParameterizedTest
2221+
@MethodSource("getFilterTestArguments")
2222+
void whenFilterMetadataKeyHasSpecialCharacters__thenReturnSpansFiltered(String endpoint,
2223+
SpanPageTestAssertion testAssertion) {
2224+
2225+
String workspaceName = UUID.randomUUID().toString();
2226+
String workspaceId = UUID.randomUUID().toString();
2227+
String apiKey = UUID.randomUUID().toString();
2228+
2229+
mockTargetWorkspace(apiKey, workspaceName, workspaceId);
2230+
2231+
var projectName = generator.generate().toString();
2232+
var spans = PodamFactoryUtils.manufacturePojoList(podamFactory, Span.class)
2233+
.stream()
2234+
.map(span -> span.toBuilder()
2235+
.projectId(null)
2236+
.projectName(projectName)
2237+
.metadata(JsonUtils.getJsonNodeFromString(
2238+
"{\"hidden_params\":{\"additional_headers\":{\"x-litellm-attempted-retries\":\"0\"}}}"))
2239+
.feedbackScores(null)
2240+
.totalEstimatedCost(null)
2241+
.build())
2242+
.collect(toCollection(ArrayList::new));
2243+
spans.set(0, spans.getFirst().toBuilder()
2244+
.metadata(JsonUtils.getJsonNodeFromString(
2245+
"{\"hidden_params\":{\"additional_headers\":{\"x-litellm-attempted-retries\":\"3\"}}}"))
2246+
.build());
2247+
2248+
spanResourceClient.batchCreateSpans(spans, apiKey, workspaceName);
2249+
2250+
var expectedSpans = List.of(spans.getFirst());
2251+
var unexpectedSpans = List.of(podamFactory.manufacturePojo(Span.class).toBuilder()
2252+
.projectId(null)
2253+
.build());
2254+
2255+
spanResourceClient.batchCreateSpans(unexpectedSpans, apiKey, workspaceName);
2256+
2257+
var filters = List.of(SpanFilter.builder()
2258+
.field(SpanField.METADATA)
2259+
.operator(Operator.EQUAL)
2260+
.key("hidden_params.additional_headers.x-litellm-attempted-retries")
2261+
.value("3")
2262+
.build());
2263+
2264+
var values = testAssertion.transformTestParams(spans, expectedSpans, unexpectedSpans);
2265+
2266+
testAssertion.runTestAndAssert(projectName, null, apiKey, workspaceName, values.expected(),
2267+
values.unexpected(),
2268+
values.all(), filters, Map.of());
2269+
}
2270+
22202271
@ParameterizedTest
22212272
@MethodSource("getFilterTestArguments")
22222273
void whenFilterMetadataEqualNumber__thenReturnSpansFiltered(String endpoint,

0 commit comments

Comments
 (0)