Fix client-v2: quote String/temporal elements in container query parameters#2898
Merged
Merged
Conversation
…meters
addStatementParams formatted every HTTP param_<name> value with a blanket
String.valueOf(v). For a container value (List/array/Map) this left inner
elements unquoted (e.g. param_l=[2026-05-13] for an Array(Date) parameter),
which the server rejects with HTTP 400 CANNOT_PARSE_INPUT_ASSERTION_FAILED.
The server's array/map text parser requires String/temporal leaves to be
single-quoted (['2026-05-13']) while numeric leaves must stay unquoted.
Format container parameters type-awarely: frame Collection/array as [..] and
Map as {..}, and render scalar leaves via ClickHouseValues.convertToSqlExpression
(String/temporal single-quoted, numeric/boolean unquoted, null -> NULL). Scalar
parameters are unchanged (still emitted unquoted, as the server requires). Both
transport paths (URL query params and multipart body) funnel through this method.
Fixes: #2897
|
Repository collaborators can run the JMH benchmark suite against this PR by commenting: Optional regression threshold override (Δ% on Time or Alloc/op; defaults to 10%): Only one benchmark run per PR is active at a time — issuing a new |
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes client-v2 HTTP param_<name> formatting for container-valued query parameters so that inner String/temporal leaves are single-quoted (and escaped) while numeric/boolean leaves remain unquoted, enabling ClickHouse to parse Array(...) and Map(...) parameters correctly via the HTTP parameter interface.
Changes:
- Updated
HttpAPIClientHelper.addStatementParamsto use a new type-aware formatter for container parameters. - Added unit tests covering container quoting/escaping and contrast cases where scalars and numeric arrays remain unquoted.
- Added an integration regression test that round-trips
Array(Date),Array(String),Array(Int32), andMap(String,Date)parameters end-to-end.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java | Adds recursive container formatting so nested elements use type-aware SQL literal rendering. |
| client-v2/src/test/java/com/clickhouse/client/api/internal/HttpAPIClientHelperTest.java | Adds unit coverage for the new formatter across scalar/array/map/nested/escaping/null cases. |
| client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java | Adds integration regression ensuring container params parse correctly on a live server. |
chernser
reviewed
Jun 26, 2026
chernser
reviewed
Jun 26, 2026
chernser
reviewed
Jun 26, 2026
chernser
reviewed
Jun 26, 2026
chernser
requested changes
Jun 26, 2026
…rter
Address review feedback on the Array/Map query-parameter quoting fix:
- Move the param formatting out of the HttpAPIClientHelper transport class
into a reusable DataTypeConverter.convertParameterToString instance method,
so the transport class no longer knows about ClickHouse parameter formatting
and there are no static formatting helpers on it.
- Format parameters upstream in Client.query so the transport layer receives
already-formatted text; addStatementParams reverts to a plain passthrough.
- Detect containers via getClass().isArray() and iterate via reflection so
primitive arrays (int[], long[], ...) and nested primitive arrays are
formatted correctly instead of being mis-rendered as a scalar ("[I@..").
- Parametrize the unit tests (DataTypeConverterTest) and extend the
integration test with object array, primitive array and nested array cases.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
Collaborator
Author
|
@chernser thanks for the review — pushed
I left the Tuple thread open: a raw |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Address review feedback on PR #2898: the comment on the parameter pre-formatting block read 'rendered with the quoting the server's param_<name> parser needs', which stacked two noun phrases and read as a garden-path sentence. Reword to 'quoted the way the server's param_<name> parser expects'. Comment-only change; no behavior change.
chernser
reviewed
Jul 8, 2026
chernser
requested changes
Jul 8, 2026
Convert testContainerQueryParamsQuoteInnerValues to a TestNG @dataProvider so each container/type case is a {type, value, expected} row and the query is constructed from the row's type. New cases extend by adding a row. Also broadens coverage with Array(DateTime) and Array(Float64).
chernser
reviewed
Jul 8, 2026
chernser
requested changes
Jul 8, 2026
chernser
left a comment
chernser
left a comment
Contributor
There was a problem hiding this comment.
- use stack implementation to convert parameters.
- update changelog with short description how to use DataTypeConverter.
Replace the recursive convertParameterContainer with an explicit ArrayDeque work stack so deeply nested container parameters cannot overflow the call stack (review feedback on #2898). Output is unchanged and stays pinned by the exact-string DataTypeConverterTest data provider; adds a deep-nesting regression test and a CHANGELOG entry describing container parameter formatting via DataTypeConverter.
Collaborator
Author
|
@chernser addressed your review in c17591f:
One note on the failing SonarCloud gate, in case it's a concern for merging: the "C Security Rating on New Code" comes entirely from 4 findings in |
chernser
approved these changes
Jul 8, 2026
Contributor
|
@cursor review |
chernser
approved these changes
Jul 8, 2026
|
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c17591f. Configure here.
This was referenced Jul 8, 2026
Contributor
|
Fixes #2407 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2897.
HttpAPIClientHelper.addStatementParamsformatted every HTTPparam_<name>value with a blanketString.valueOf(v). For a container value (aList/array/Map) that callsAbstractCollection.toString(), which renders each element via its owntoString()and leaves innerString/temporal values unquoted — e.g. aList<LocalDate>bound to{l:Array(Date)}is sent asparam_l=[2026-05-13]. ClickHouse's array text parser requires single-quoted dates (['2026-05-13']), so the server rejects the request with HTTP 400CANNOT_PARSE_INPUT_ASSERTION_FAILED. The same affectedArray(String),Array(DateTime),Map(..., Date), and nested containers.The fix formats container parameters type-awarely:
Collection/object-array are framed as[..],Mapas{..}, and scalar leaves are rendered with the existing type-awareClickHouseValues.convertToSqlExpression(single-quotesString/temporal/UUID/Inet, leaves numeric/boolean unquoted,null→NULL). Scalar parameters are untouched — they are still emitted in their bare, unquoted form, which is what the server requires for a scalar placeholder (a quoted scalar'2026-05-13'is itself rejected). Both transport paths (URL query params and the multipart body) funnel through this single method, so both are fixed.Note: the formatter intentionally frames a
Collectionas anArray([..]), not a tuple ((..)) — the server rejects('2026-05-13')forArray(Date)("Array does not start with '['"), andString.valueOfalready used[..]for lists.Changes
client-v2/.../internal/HttpAPIClientHelper.java: replaceString.valueOf(v)inaddStatementParamswith a newformatStatementParam(v); add private recursive helpersformatStatementParamContainer/formatStatementParamElement. Importcom.clickhouse.data.ClickHouseValues.Test
HttpAPIClientHelperTestasserts the exact emitted strings for the full matrix: scalarDate/String/number stay unquoted (contrast);Array(Date)/Array(String)/Array(DateTime), object-array, nestedArray(Array(Date)), andMap(String,Date)single-quote their leaves; embedded-quote escaping (['a\'b']);nullleaf →NULL; and numeric/Decimalarrays stay unquoted ([1,2,3],[1.50]) — contrast cases proving the fix doesn't over-quote. (12 cases, all pass.)QueryTests#testContainerQueryParamsQuoteInnerValuesround-tripsArray(Date),Array(String),Array(Int32)andMap(String,Date)parameters end-to-end viatoString({...})and asserts the parsed values. This fails onmain(server 400) and passes with the fix.['2026-05-13']accepted,[2026-05-13]rejected (Code: 27),['1','2']rejected forArray(Int32)(Code: 130).changes_checklist.md — "String, SQL, or serialized output changed"
String/temporal leaves single-quoted (with backslash-escaping viaconvertToSqlExpression), numeric/Decimalleaves unquoted,null→NULL— each matched against the server's text parser.param_<name>rendering of container values changes; scalar rendering is byte-identical (String.valueOf), so no scalar regression. Existing param tests pass scalar / pre-formatted-string values and are unaffected.Pre-PR validation gate
[2026-05-13])String.valueOfon containers)AGENTS.md/docs/changes_checklist.mdCross-repo context
Same bug class as
clickhouse-connectissue #188 (Python): a server-side parameter formatter not recursively quoting inner temporal/Stringvalues. Root cause is client-side value formatting for the HTTPparam_*path (not the wire protocol), so it is specific to each client's formatter.