Fix AdvancedSql/raw-SQL scalar queries for reference-typed columns (byte[], IPAddress, etc.)#4960
Conversation
…yte[], IPAddress, etc.) ScalarSelectClause<T> is `where T : struct` because it also implements ISelector<T?> via Nullable<T>, so any Npgsql-mapped reference type (byte[] bytea, IPAddress inet, BitArray, JsonDocument, ...) blew up at query construction with an ArgumentException/TypeLoadException instead of ever reaching Npgsql's own type mapping. Only string had a dedicated workaround (ScalarStringSelectClause); byte[] and other reference types did not. Add ScalarClassSelectClause<T> where T : class as the reference-type counterpart (natural nullability, no Nullable<T>), and branch on typeof(T).IsValueType in both AdvancedSqlQueryHandlerBase.GetSelectClause<T> and UserSuppliedQueryHandler.GetSelectClause, which had the identical bug. Fixes JasperFx#4959. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mysticmind
left a comment
There was a problem hiding this comment.
The AdvancedSql coverage is solid. Could you please also add a unit test for the UserSuppliedQueryHandler path, i.e. a raw-SQL scalar case like QueryAsync<byte[]>("select …")? Right now all the new tests go through AdvancedSql, so that second fixed path has no coverage.
Addresses PR review feedback: all prior new tests went through AdvancedSql, leaving the UserSuppliedQueryHandler.GetSelectClause path (session.QueryAsync<T> raw-SQL scalar queries) — which had the identical struct-constraint bug — without its own regression test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Good catch — added `can_query_single_bytea_scalar_via_raw_sql` which exercises `session.QueryAsync<byte[]>("select ...")`, i.e. the `UserSuppliedQueryHandler.GetSelectClause` path directly (pushed in 586b34f). Note: the tuple/`row(...)` form (`QueryAsync<T1, T2, T3>`) only exists on `AdvancedSql`, not on the base `QueryAsync` API, so that variant is already fully covered by the existing `AdvancedSql` row-tuple test — there's no equivalent to add on the raw-SQL side. |
mysticmind
left a comment
There was a problem hiding this comment.
Thanks @mdissel, looks good to me!
Summary
Fixes #4959.
ScalarSelectClause<T>iswhere T : structbecause it also implementsISelector<T?>viaNullable<T>. Any Npgsql-mapped reference type (byte[]/bytea,IPAddress/inet,BitArray,JsonDocument, ...) blew up at query-construction time withArgumentException/TypeLoadExceptionbefore Npgsql's own type mapping ever got a chance to run. Onlystringhad a dedicated workaround (ScalarStringSelectClause) —byte[]and other reference types did not.ScalarClassSelectClause<T> where T : classas the reference-type counterpart — relies on natural reference-type nullability instead ofNullable<T>, mirroring the existingScalarStringSelectClausepattern.typeof(T).IsValueTypein bothAdvancedSqlQueryHandlerBase.GetSelectClause<T>(session.AdvancedSql.QueryAsync/StreamAsync) andUserSuppliedQueryHandler.GetSelectClause(session.QueryAsync<T>("select ...")), which had the identical latent bug.byte[]-specific but is specific to types with a genuine Npgsql scalar type mapping — CLR arrays likeint[]/Guid[]are composed from their element type by Npgsql rather than registered as scalar mappings, so they don't hit this code path at all (a separate, unrelated limitation, out of scope here).Test plan
src/DocumentDbTests/Bugs/advanced_sql_query_scalar_reference_types.cs: single-scalar, row-tuple (row(...)), andStreamAsynccases for abyte[]/byteacolumn, plus a single-scalar case for anIPAddress/inetcolumn, against a plain (non-document) table added viaExtendedSchemaObjects.CoreTestssuite: 479 passed, 0 failed.DocumentDbTestssuite: 1055 passed, 0 failed.LinqTestsscalar-select suites (Scalar/select_scalarfilters): 36 passed, 0 failed — confirms the existing LINQ scalar-projection paths (which never route arbitrary reference types throughScalarSelectClause<T>in the first place) are unaffected.ArgumentException/TypeLoadExceptionbefore the fix, and pass after.🤖 Generated with Claude Code