Fix GetSchema("Columns") silently ignoring restrictions beyond database and table - #497
Conversation
DescribeColumns only reads restrictions[0] (database) and restrictions[1]
(table). Any further restriction value was ignored, so a call such as
GetSchema("Columns", ["system", "functions", "name"]) ran cleanly and
returned every column of the table instead of the requested one - a silent
wrong result. Unsupported restriction values are now rejected with an
ArgumentException, matching how ADO.NET providers report more restrictions
than a metadata collection supports. Positions left null stay unspecified.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness issue in the ADO.NET metadata surface (ClickHouseConnection.GetSchema("Columns", ...)) by making unsupported restriction positions fail loudly instead of being silently ignored, and documents/tests the behavior as an observable bug fix.
Changes:
- Add an
ArgumentExceptionguard inSchemaDescriber.DescribeColumnswhen non-nullrestrictions are supplied beyond the supporteddatabase/tablepositions. - Document supported
GetSchema("Columns")restrictions and possible exceptions onClickHouseConnection.GetSchema(string, string[]). - Add regression tests plus changelog/release-notes entries for the behavior change.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ClickHouse.Driver/Utility/SchemaDescriber.cs | Rejects extra restriction values for the Columns collection (throws ArgumentException). |
| ClickHouse.Driver/ADO/ClickHouseConnection.cs | Adds XML docs clarifying the only supported collection and restriction semantics. |
| ClickHouse.Driver.Tests/ADO/ConnectionTests.cs | Adds tests covering unsupported restriction positions and contrast cases. |
| CHANGELOG.md | Documents the behavioral bug fix under Unreleased. |
| RELEASENOTES.md | Documents the behavioral bug fix under Unreleased. |
| if (restrictions != null && restrictions.Skip(ColumnsRestrictionNames.Length).Any(restriction => restriction != null)) | ||
| { | ||
| throw new ArgumentException( | ||
| $"More restrictions were provided than the requested schema ('{collectionName}') supports. " + | ||
| $"Supported restrictions are: {string.Join(", ", ColumnsRestrictionNames)}."); |
There was a problem hiding this comment.
Good catch, and the concern is real — but it is deliberately out of scope for this PR, because it is already fixed in a separate, earlier PR: #496 — "Fix GetSchema(\"Columns\") building invalid SQL for a table-only restriction".
That PR replaces exactly the inline " WHERE" / " AND" appends flagged here with collected predicates joined at the end:
var predicates = new List<string>();
if (database != null) { predicates.Add("database={database:String}"); ... }
if (table != null) { predicates.Add("table={table:String}"); ... }
if (predicates.Count > 0) query.Append(" WHERE ").Append(string.Join(" AND ", predicates));so GetSchema("Columns", [null, "functions"]) composes valid SQL and filters by table across all databases. #496 carries its own regression tests for the table-only and no-restriction cases.
I am keeping the two changes separate on purpose: #496 fixes the SQL-composition defect, this PR (#497) fixes a distinct defect — extra positional restriction values (a column restriction, per the canonical ADO.NET Columns collection) being silently dropped and returning a wrong result instead of raising ArgumentException. Folding the WHERE-composition rewrite in here would bundle two unrelated fixes into one PR and duplicate #496's diff on the same lines.
No code change here, so I am leaving this thread unresolved for the reviewer to decide. Whichever of #496/#497 merges second will need a trivial rebase on this method; happy to reorder them if you prefer one first.
|
@polyglotAI-bot please fix the merge conflict |
…lumns-reject-extra-restrictions # Conflicts: # ClickHouse.Driver/Utility/SchemaDescriber.cs
|
@alex-clickhouse conflict resolved and pushed ( What conflicted and how it was resolved:
Net diff versus current Verification in a devbox against a live ClickHouse server: Side note: since #496 is now in Heads-up: the merge may have dismissed any prior approval, so a re-approve might be needed. |
main added 22 new Unreleased entries since this branch was cut. Each is now its own changelog.d/ fragment, extracted verbatim by line number rather than retyped, so the assembled Unreleased section reproduces main's exactly (as a set of lines; sorting by PR number reorders entries within their sections). New fragments, one per (PR, category): #390 improvements multidim blittable inserts #472 improvements per-scalar Span<byte> reads #484 fixes byte[]/TimeOnly HTTP parameters #485 fixes JSON strings under ReadStringsAsByteArrays #490 breaking raw results return compressed bytes #490 features AcceptEncoding response compression #490 improvements lz4 by default, HttpClient, errors, deflate #492 fixes HTTP response disposal #493 fixes Enum type declarations #494 fixes raw-stream double dispose #497 fixes GetSchema("Columns") restrictions #498 fixes JSON paths starting with setting names #503 fixes quoted JSON typed paths #504 fixes quoted Tuple/Nested element names #509 fixes {name:Type} scanner vs server lexer #511 fixes {name:Type} hints after a non-hint brace #513 fixes @name placeholders, heredocs, $ in names #390's entry was appended to the *released* v1.3.0 section on main (v1.3.0 shipped 2026-06-29), so it would have documented an unreleased change under a shipped version and never appeared in 1.4.0's notes. It moves to Unreleased as a fragment; the rest of v1.3.0 is byte-identical. RELEASENOTES.md regenerated with --sync-notes. `--check` passes, the solution builds, and the packed .nupkg's releaseNotes open on v1.3.0 with no Unreleased stub and no #390 bullet.
Description
SchemaDescriber.DescribeColumnsreads onlyrestrictions[0](database) andrestrictions[1](table). Any further restriction value is neither used nor reported, so a call likeruns cleanly and returns all 16 columns of
system.functions— the caller's column filter is silently discarded. Verified against a live server (26.x) before the fix: 16 rows,name, is_aggregate, case_insensitive, alias_to, …. A metadata API quietly returning a wider result than was asked for is worse than an error, because callers have no way to tell that their filter was dropped.The fix rejects restriction values in positions the
Columnscollection does not support, with anArgumentExceptionwhose message mirrors .NET's own provider behaviour (DbMetaDataFactorythrowsArgumentException"More restrictions were provided than the requested schema … supports."). Positions leftnullremain "unspecified" — exactly how the two supported positions already readnull— so a canonically-sized array such as["system", "functions", null, null]keeps working unchanged.This deliberately does not add support for a column restriction: that would be a new capability, and it would have to invent positional semantics (this provider puts database at index 0 and table at index 1, whereas the canonical ADO.NET
Columnslayout is catalog/schema/table/column). Making the drop loud is the bug fix; adding a column filter can follow as a feature if wanted.Found while working on #496 (independent of it, and it does not touch the
WHERE-composition lines that PR changes).Changes
ClickHouse.Driver/Utility/SchemaDescriber.cs—DescribeColumnsthrowsArgumentExceptionwhen a restriction value is supplied past the supporteddatabase/tablepositions; the supported restriction names are declared once and used in the message. The collection name is threaded through so the message reports what the caller asked for.ClickHouse.Driver/ADO/ClickHouseConnection.cs— XML doc onGetSchema(string, string[])stating the supported collection, its two restrictions, and both exceptions.ClickHouse.Driver.Tests/ADO/ConnectionTests.cs— new tests (see below).CHANGELOG.md/RELEASENOTES.md— Bug Fixes entry (this is an observable behaviour change for callers that were passing extra restrictions).Test
GetSchema_ColumnsWithRestrictionBeyondDatabaseAndTable_ThrowsArgumentExceptionis aTestCaseSource-parametrized test over the shapes that used to be silently dropped:["system", "functions", "name"]— third position set["system", "functions", ""]— empty string counts as supplied (consistent with how the supported positions treat""as a real filter and onlynullas unspecified)[null, null, "name"]— an unsupported restriction with no supported ones["system", "functions", null, "name"]— fourth position set, third unspecifiedContrast cases pin the behaviour that must not change:
GetSchema_ColumnsWithUnspecifiedRestrictionsBeyondTable_AppliesSupportedRestrictions—["system", "functions", null, null]still returns rows, all withDatabase == "system"andTable == "functions".GetSchema_ColumnsWithEmptySupportedRestriction_FiltersOnTheEmptyValue—["system", ""]still filters on the empty table name (empty result), unchanged.ShouldFetchSchemaDatabaseColumns/ShouldFetchSchemaTableColumnscover the 1- and 2-restriction paths.Verified: the four parametrized cases fail on unpatched
main(Expected: <System.ArgumentException> But was: null) and pass with the fix; both contrast cases pass before and after. FullADOsuite green onnet10.0: 916 passed, 0 failed.Pre-PR validation gate
AGENTS.md(integration-style tests,Method_Scenario_ExpectedBehaviornaming,TestCaseSourceparametrization, ADO.NET compliance)PublicAPI/*.txtuntouched — the guard is inside an internal helper)