TCP N5a: Add the per-type half of the POCO mapping (attributes, descriptor, registry) - #551
Draft
alex-clickhouse wants to merge 2 commits into
Draft
TCP N5a: Add the per-type half of the POCO mapping (attributes, descriptor, registry)#551alex-clickhouse wants to merge 2 commits into
alex-clickhouse wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the CLR-type metadata layer for future TCP POCO query and insert mapping.
Changes:
- Adds TCP-specific mapping attributes.
- Discovers, matches, and caches POCO member descriptors.
- Adds comprehensive unit coverage for mapping and concurrency behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ClickHouse.Driver.Tcp/Poco/PocoTypeRegistry.cs |
Caches descriptors per CLR type. |
ClickHouse.Driver.Tcp/Poco/PocoTypeDescriptor.cs |
Implements discovery, matching, and activation. |
ClickHouse.Driver.Tcp/Poco/PocoMember.cs |
Records member capabilities and nullability. |
ClickHouse.Driver.Tcp/Poco/ClickHouseTcpNotMappedAttribute.cs |
Adds property exclusion metadata. |
ClickHouse.Driver.Tcp/Poco/ClickHouseTcpColumnAttribute.cs |
Adds column-name mapping metadata. |
ClickHouse.Driver.Tcp.Tests/Poco/PocoTypeRegistryTests.cs |
Tests caching and concurrency. |
ClickHouse.Driver.Tcp.Tests/Poco/PocoTypeDescriptorTests.cs |
Tests discovery, matching, activation, and errors. |
Suppressed comments (1)
ClickHouse.Driver.Tcp.Tests/Poco/PocoTypeDescriptorTests.cs:586
- This fixture comment repeats the same incorrect timing: without the
IsAbstractguard, the activator does not compile successfully and then fail on first use; compilation itself fails. Please align it with the production comment.
// the activator from compiling and then failing on first use.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public void Activator_AbstractClass_IsBlockedForBeingAbstractRatherThanForLackingAConstructor() | ||
| { | ||
| // The declared public constructor makes GetConstructor succeed, so only the IsAbstract check catches this; | ||
| // otherwise it would compile and fail when the delegate first ran. |
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco-skeleton
branch
from
August 14, 2026 16:44
8630f9b to
a39dcc1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco-skeleton
branch
from
August 15, 2026 11:48
a39dcc1 to
a5a1458
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco-skeleton
branch
from
August 16, 2026 10:31
a5a1458 to
0ffb958
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco-skeleton
branch
from
August 16, 2026 17:50
0ffb958 to
61123c6
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco-skeleton
branch
from
August 17, 2026 09:49
61123c6 to
97bd8d1
Compare
…gistry The shape of a POCO mapping splits in two. What depends only on the CLR type -- which properties map, to which column names, and how a name off the wire finds its property -- is resolved once per type and shared by the query and insert paths. What depends on the column types is per (type, wire shape) and comes later. This lands the first half. Matching is tiered, most specific first: the exact column name, then case-insensitively, then ignoring underscores. So a `user_id` column reaches a `UserId` property with no attribute. A column that reaches several properties at the tier it lands on is an error, not a silent pick; a column that reaches none is simply unmapped. The attributes are `[ClickHouseTcpColumn]` and `[ClickHouseTcpNotMapped]`, not duplicates of the main driver's names. Both assemblies ship in the one package, so a same-named attribute in `ClickHouse.Driver.Tcp` would be an ambiguous reference for any file importing both namespaces. The surfaces also differ: the HTTP attribute's `Type` exists only to skip that client's schema probe, and the native protocol sends the target types with the insert. One descriptor serves both directions rather than the HTTP client's two mappings. `PocoMember` carries `CanGet` and `CanSet`, and each plan filters on the one it needs, so a type is never refused for lacking the direction it is not being used in -- an immutable getter-only POCO builds and stays insertable, and the query path checks `CanActivate` instead. Only a type with nothing mappable at all fails the build. Ambiguity is likewise reported per column rather than up front: an exact duplicate fails the build, since no arriving name could disambiguate it, but a case- or underscore-level collision fails only when a column lands on it. Coverage: all four files at 100% line coverage, 42 unit tests. Nothing consumes the registry yet; the client wires it up with `QueryAsync<T>`.
A POCO interface lost every property it inherited. `Type.GetProperties` walks a class's base types, but an interface has no base type -- what it inherits sits on the interfaces it extends. So `InsertAsync<IDerived>` would have written only the properties `IDerived` itself declares and let the server default the rest, with no diagnostic. Discovery now also enumerates `GetInterfaces()`, whose closure covers any depth and reports each interface once. That opens a case a class hierarchy cannot reach: one name declared by two unrelated interfaces. A class chain always has a most-derived declaration, so `KeepMostDerived` could assume one; two sibling interfaces have none, and C# itself needs a cast to read such a property. It now throws instead of keeping whichever arrived first. Three comments claimed things that are not true, verified against the BCL: - `Expression.New` over an abstract class does not fail at first invocation. `Compile` refuses it outright, so without the `IsAbstract` check the failure would land at descriptor build with an opaque message. The check is still right, for a different reason. - `Members` was documented as declaration order. Reflection contracts no order and reports an inherited property derived-first, which also contradicted a comment further down the same file. - A compiled `Func<T>` is not as fast as `new T()` -- it costs a delegate call. It beats `Activator.CreateInstance`, which is the real claim. `Members` now wraps its array so a shared descriptor cannot be mutated through it, and the "all N properties are excluded" message counts only mappable ones, since an indexer inflated N. Tests: 42 -> 50. Added the interface cases, attribute inheritance through an override (pinned because `GetCustomAttribute<T>` and `GetCustomAttributes(t, inherit: true)` disagree on an override), a private getter, exact-beats-underscore precedence, and the indexer count. Fixed one assertion that a substring made trivially true, and one test whose name promised more than it asserted. All four files stay at 100% line coverage; 1444/1444 TCP tests pass.
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco-skeleton
branch
from
August 17, 2026 14:00
97bd8d1 to
5a642f2
Compare
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.
Stacked on #548 (base branch
tcp/epic-n5-poco). Steps 1–2 of the POCO epic (N5a). No behavior is reachable from the public API yet: nothing consumes the registry untilQueryAsync<T>lands.What this is
A POCO mapping splits in two. What depends only on the CLR type — which properties map, to which column names, and how a name off the wire finds its property — is resolved once per type and shared by the query and insert paths. What depends on the column types is per
(type, wire shape)and comes later. This is the first half.Name matching
Tiered, most specific first: the exact column name, then ignoring case, then ignoring case and underscores. So a
user_idcolumn reaches aUserIdproperty with no attribute. A column that reaches several properties at the tier it lands on is an error, not a silent pick; a column that reaches none is simply unmapped.Ambiguity is reported per column, at match time, not eagerly at build. An exact duplicate column name fails the build, since no arriving name could disambiguate it, but a case- or underscore-level collision fails only when a column actually lands on it — so a POCO holding both
Valueand avalue-renamed member stays usable as long as the server sends exact names.Decisions worth review
The attributes are
[ClickHouseTcpColumn]/[ClickHouseTcpNotMapped], not duplicates of the main driver's names. Three reasons:ClickHouse.Driverpackage, so a same-namedClickHouse.Driver.Tcp.ClickHouseColumnAttributewould be a CS0104 ambiguous reference for any file importing both namespaces — which a user migrating client by client necessarily does.ClickHouseTcp*-prefixed, so this is the convention rather than an exception to it.Typeproperty exists only to skip that client's schema-probe query, and the native protocol sends the target types with the insert itself — so the TCP attribute carriesNamealone, and a same-named duplicate would have had to grow a dead property or silently differ.Cost: a POCO shared between both clients needs both attributes. Honoring the HTTP attribute by full-name reflection stays available later.
One descriptor serves both directions, rather than the HTTP client's separate insert and read mappings.
PocoMembercarriesCanGet(public getter → insert source) andCanSet(public non-init setter → query target), and each future plan filters on the one it needs. So a type is never refused for lacking the direction it is not being used in: an immutable getter-only POCO with no parameterless constructor builds fine and stays insertable, and the query path checksCanActivate. Only a type with nothing mappable at all fails the build.Review findings already fixed in the second commit
Tlost every base-interface property.GetPropertieswalks a class's base types, but an interface has no base type.InsertAsync<IDerived>would have written only whatIDeriveditself declares and let the server default the rest, silently. Discovery now also enumeratesGetInterfaces().Expression.Newover an abstract class fails atCompile(), not at first invocation;Membersis not in declaration order (reflection contracts none and reports inherited properties derived-first); a compiledFunc<T>is not as fast asnew T(), it beatsActivator.CreateInstance.Verification
CHANGELOG and RELEASENOTES are deliberately untouched — there is no user-visible behavior until the query and insert paths land. They come with the epic's final step.
🤖 Generated with Claude Code