-
Notifications
You must be signed in to change notification settings - Fork 624
Fix client-v2: quote String/temporal elements in container query parameters #2898
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
base: main
Are you sure you want to change the base?
Changes from all commits
f0d4820
0972134
52d3aea
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 |
|---|---|---|
|
|
@@ -59,6 +59,7 @@ | |
| import java.net.InetAddress; | ||
| import java.nio.ByteBuffer; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.time.LocalDate; | ||
| import java.time.LocalDateTime; | ||
| import java.time.ZoneId; | ||
| import java.time.ZonedDateTime; | ||
|
|
@@ -1660,6 +1661,45 @@ | |
| Assert.assertEquals(records.get(1).getString("name"), "ENGINES"); | ||
| } | ||
|
|
||
| @Test(groups = {"integration"}) | ||
| public void testContainerQueryParamsQuoteInnerValues() { | ||
| // Regression for Array/Map parameters whose elements were emitted unquoted (e.g. | ||
| // param_dates=[2026-05-13]) and rejected by the server with HTTP 400. Raw List/array/Map | ||
| // values must now round-trip without the manual DataTypeConverter pre-formatting workaround. | ||
| final Map<String, Object> params = new HashMap<>(); | ||
|
Contributor
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. there are many other containers.
Collaborator
Author
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. Extended the integration test with an object array ( On Tuple specifically: a raw So rendering a |
||
| params.put("dates", Arrays.asList(LocalDate.of(2026, 5, 13), LocalDate.of(2026, 5, 14))); | ||
|
Check warning on line 1670 in client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java
|
||
| params.put("names", Arrays.asList("a", "b")); | ||
| params.put("ints", Arrays.asList(1, 2, 3)); | ||
| params.put("dateMap", Collections.singletonMap("k", LocalDate.of(2026, 5, 13))); | ||
|
Check warning on line 1673 in client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java
|
||
| // Object array, primitive array and a nested array must all be supported, not just List. | ||
| params.put("dateArr", new LocalDate[]{LocalDate.of(2026, 5, 13)}); | ||
|
Check warning on line 1675 in client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java
|
||
| params.put("intArr", new int[]{4, 5, 6}); | ||
| params.put("nested", Arrays.asList(Arrays.asList(1, 2), Arrays.asList(3, 4))); | ||
|
|
||
| List<GenericRecord> records = client.queryAll( | ||
| "SELECT toString({dates:Array(Date)}) AS d, " + | ||
| "toString({names:Array(String)}) AS n, " + | ||
| "toString({ints:Array(Int32)}) AS i, " + | ||
| "toString({dateMap:Map(String, Date)}) AS m, " + | ||
| "toString({dateArr:Array(Date)}) AS da, " + | ||
| "toString({intArr:Array(Int32)}) AS ia, " + | ||
| "toString({nested:Array(Array(Int32))}) AS ne", | ||
| params); | ||
|
|
||
| Assert.assertEquals(records.size(), 1); | ||
| GenericRecord record = records.get(0); | ||
|
Check warning on line 1690 in client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java
|
||
| // Array(Date)/Array(String) elements are single-quoted so the server parses them. | ||
| Assert.assertEquals(record.getString("d"), "['2026-05-13','2026-05-14']"); | ||
| Assert.assertEquals(record.getString("n"), "['a','b']"); | ||
| // Contrast: numeric arrays must stay unquoted (quoting causes CANNOT_READ_ARRAY_FROM_TEXT). | ||
| Assert.assertEquals(record.getString("i"), "[1,2,3]"); | ||
| Assert.assertEquals(record.getString("m"), "{'k':'2026-05-13'}"); | ||
| // Object array, primitive array and nested array round-trip too. | ||
| Assert.assertEquals(record.getString("da"), "['2026-05-13']"); | ||
| Assert.assertEquals(record.getString("ia"), "[4,5,6]"); | ||
| Assert.assertEquals(record.getString("ne"), "[[1,2],[3,4]]"); | ||
| } | ||
|
|
||
| @Test(groups = {"integration"}) | ||
| public void testExecuteQueryParam() throws ExecutionException, InterruptedException, TimeoutException { | ||
|
|
||
|
|
||
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.
Top-level char array mishandled
Medium Severity
convertParameterToStringtreats every Java array as anArraycontainer viagetClass().isArray(), butString.valueOf(char[])is defined to return the character sequence as plain text, not an array literal. A top-levelchar[]bound to a scalar placeholder (e.g.{s:String}) is now emitted as numeric codepoints like[104,105]instead of the prior string value, which can break queries that previously worked.Additional Locations (1)
client-v2/src/main/java/com/clickhouse/client/api/internal/DataTypeConverter.java#L242-L250Reviewed by Cursor Bugbot for commit 0972134. Configure here.
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.
Thanks — I dug into this and it's not a regression; the
[104,105]rendering is consistent with this library's existingchar[]convention, so I'm leaving it as-is (happy to revisit if you'd preferchar[]treated as text — see end).The "previously worked / prior string value" premise doesn't hold. Before this PR, params were emitted by
HttpAPIClientHelper.addStatementParamsviaString.valueOf(v), wherevis theMap<String,Object>value — statically typedObject. Overload resolution therefore picksString.valueOf(Object), not theString.valueOf(char[])overload (that one requires achar[]static type). So a top-levelchar[]was rendered aschar[].toString()→[C@<hash>identity junk, never the character sequence:Post-PR,
Client.querypre-formats viaDataTypeConverter.convertParameterToString(Object)— sameObjectstatic type — so thechar[]overload was never in play on either side. There was no workingchar[]→'hi'path to break.[104,105]matches the library's deliberatechar[]convention.ClickHouseValues.convertToString(char[])(ClickHouseValues.java:630-643) renders a char array as its numeric code points ([104,105]), andconvertToSqlExpressionmaps aCharacterleaf to its code point (:577-578). The new container formatter delegates leaves toconvertToSqlExpression, so it now produces the same[104,105]as the rest of the converter — this makes the param path consistent, not divergent. (Net: for{s:String}both the old[C@hash]and the new[104,105]just store a literal string — neither errors, neither ishi; for anArray(UInt16)-style placeholder the new[104,105]actually parses, where[C@hash]did not.)This is also outside the reported bug (
Array(Date)of boxed temporals) —char[]isn't part of it.If you'd prefer a
char[]parameter be treated as text ('hi') — a deliberate divergence from the existingconvertToString(char[])convention — I'm glad to add that as a small explicit special-case. Leaving this thread open for your call.