Repository navigation
fix: final v9.0.0 review — silent defects, final-SQL guard, strict literal grammar - #33
Merged
Merged
Conversation
A grouped test-dependency update edited the shipping satellite's reference, raising every consumer's minimum without a release decision. Dependabot now ignores NodaTime, which the test projects only reach through the satellite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LocalDateRange and LocalDateTimeRange passed no normalizeValue, so an element operand in another calendar bound its own year/month/day as if ISO: a constant silently missed, a LocalDate parameter threw inside Npgsql. The sets already normalized. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The core value-set table listed six existing wrapper arities as missing; the NodaTime README still promised JSON setup and carried a sample that does not compile (CS1023). architecture.md described MakeGenericType where GenericConverters is used, CONTRIBUTING's skip command ran one of seven projects, and the release-notes migration link was relative, which does not resolve in a GitHub release body. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
.NET reads "" as the general format, exactly like null, but the literal
entry points checked only `format is null`. ToString("", de-DE) on
DecimalRange [1.5,2) wrote [1,5,2) and RangeSet {[1,5,2)}; DateRange
with ("", th-TH) wrote Buddhist-era short dates, since `format ??
"yyyy-MM-dd"` does not trigger on ""; Int32Range ("", sv-SE) took U+2212
as its minus; DecimalSet ("", de-DE) wrote {"1,5"}.
IRangeFactory's IFormattable.ToString and SetFormat.Format now normalize
an empty format to null before choosing the provider, and every
FormatValue that defaults a null format (the temporal ranges and sets,
their wrapper arities, and the NodaTime satellite) reads "" as that
default too, since FormatValue is public and reachable directly.
LiteralProviderContractTests and LiteralCultureContractTests sweep ""
exactly like null for every discovered type, and a new sweep asserts
FormatValue(value, "") == FormatValue(value, null) for every range and
set type.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DecimalSet<TElement> and the integer arities handed the raw element
text to TElement.Parse, so the element decided the grammar. A generated
wrapper (`new(decimal.Parse(s, provider))`, NumberStyles.Number) accepts
group separators: DecimalSet.Parse("{\"1,5\"}") threw while
DecimalSet<Price>.Parse("{\"1,5\"}") was {15}, and {"1,000.5"} was
1000.5 - the same through TryParse, the TypeConverter and a JSON string.
In the other direction {1e3} and a JSON 1.5e1, which DecimalSet reads,
were refused by the wrapper.
ParseValue now parses with the primitive family's own ParseValue
(NumberStyles.Float / Integer, invariant) and hands the element the
parsed value's invariant text, so its validation still runs and decimal
scale is kept - the pattern the temporal arities already used. The
decimal JSON writer checks the element's text with the same grammar
(NumberStyles.Number read a culture-leaking "1,5" as the number 15), and
the EF bridge entries for the numeric arities parse through the family's
ParseValue for the same reason, so such an element is refused rather
than bound as fifteen.
WrapperGrammarContractTests sweeps every discovered wrapper family,
closed over a deliberately lenient element and over its representative
one, and asserts each probe text is accepted or refused exactly as the
primitive sibling does, with the same value, through Parse, TryParse,
the span overload, the TypeConverter and JSON (string and number).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…onfigureForNodaTime AddNodaTimeRangeConverters registered plain element converters, which System.Text.Json resolves first-match: called before ConfigureForNodaTime they shadowed NodaTime's converters for all five types, so the XML doc's "either registration order" claim was false. They also wrote a Julian LocalDate(2024, 1, 1) - ISO 2024-01-14 - as "2024-01-01", which read back thirteen days off with no error; LocalDateTime and YearMonth likewise. The element converters now sit behind a factory that is resolved only once the options are in use, looks past itself and hands over any other converter registered for the type. The calendar-bearing converters refuse a non-ISO value on write with ArgumentException, as NodaTime's own converter does; sets and ranges normalize or reject at construction, so they never reach it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A DateTime element with an offset was converted into the parsing machine's zone, so one literal stored a different wall clock — what tsrange holds — on every server, and a local value inside a DST gap came back an hour off. The offset is now accepted and dropped, as PostgreSQL's timestamp input does, with the kind Local; the EF wrapper bridge reads its "O" text the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IsoTemporal accepts a time of day without seconds, and CLAUDE.md lists
HH:mm as part of the strict grammar, but NodaPatterns did not:
TimeRange.Parse("[09:00,17:00)") parsed while LocalTimeSet.Parse("{09:00}")
and LocalDateTimeRange.Parse("[2024-01-01T09:00,2024-01-01T17:00)")
threw.
NodaPatterns now reads HH:mm wherever IsoTemporal does: a LocalTime, the
time part of a LocalDateTime (T or space), and an Instant written with
its offset or Z. Formatting is unchanged, the hour is still two digits,
and an Instant without an offset is still refused.
TemporalLiteralStrictnessContractTests asserts HH:mm is accepted
uniformly, and reads as the full form, for every time-of-day range and
set type through Parse, TryParse, the span overload, JSON, the
TypeConverter and RangeSet; its refusal table gains a one-digit hour for
LocalTime and LocalDateTime and an offset-less HH:mm Instant.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
System.Text.Json resolves a converter by the exact type it is handed. A range reached through object, an object collection or dictionary, or a variant-typed declaration is handed over as its variant, whose own [JsonConverter] wrote the literal - so with a custom Int32Range converter on the options, Serialize(range) gave the custom form and Serialize<object>(range) gave "[1,2]", and one payload mixed both. RangeVariantJsonConverter now looks up what System.Text.Json would choose for the union from Converters (first match, factories expanded through their own CreateConverter) and, unless that is this package's converter, delegates values and dictionary keys to it on write and read, narrowing reads as before. Cached per read-only options instance; nothing is built by reflection, so AOT and source generation are unaffected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RangeFormat unquoted each bound and then tested it for emptiness, so a
quoted empty bound read as an absent one: Int32Range.Parse("[\"\",5)")
was (,4] and ("","") was (,), silently, where PostgreSQL answers
`invalid input syntax for type integer: ""`. A bound of only whitespace
was trimmed to nothing the same way ([ ,5), [1, )).
Bounds are now scanned as range_in scans them: only an absent bound (the
next character is the comma or the end) is infinite; a present bound,
quoted-empty or whitespace-only, goes to ParseValue, which refuses it for
every element type. A double quote opens or closes a quoted section
anywhere in a bound, a doubled quote inside one is a literal quote, and
a backslash escapes the next character inside quotes or out ([\1,5) is
[1,5), as in PostgreSQL). A second comma or an unquoted closing bracket
inside a bound is refused. Whitespace around the unquoted part of a bound
is still dropped - every element input function behind these types
ignores it. The multirange splitter skips quoted and escaped characters.
RangeLiteralGrammarContractTests sweeps every discovered range type,
core and NodaTime, through Parse, TryParse, the span overload, JSON, the
TypeConverter and the multirange literal. The expectations mirror probes
run against PostgreSQL 17.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…time type
System.Text.Json resolves the declared type, and the range and set
interfaces carried no converter: a property declared as IRange<int> wrote
{} and one declared as IValueSet<string> wrote {"Values":["a"]}, silently,
and neither read back.
IRange<T>, its five shape interfaces and IValueSet<T> now carry
[JsonConverter(typeof(InterfaceDeclaredJsonConverter))], a non-generic
JsonConverter<object> that writes through options.GetTypeInfo of the
runtime type - the same literal or array, honouring any converter
registered for that type, reflection-based or source-generated - and
refuses a read with a NotSupportedException naming the remedy: declare
the concrete type. No generic converter is closed at runtime.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two refusals recognized their subject by how the LINQ was written and were bypassed by every shape the pattern did not list. Union translates to array_cat, a concatenation. The 8.0.0 guard refused ==/Equals only inside a fixed list of Queryable operators, and Count only over a bare union. A projected comparison, a comparison on a projected union, a ternary or ?? around a union, GroupBy, Distinct, Count through COALESCE, and ExecuteUpdate to a union all translated and answered (or stored) something different from memory. Its comment claimed EF client-evaluates a comparison in a projection; it translates it. ExecuteUpdate of a value set inside a JSON column (new in 9.0.0) was refused only for an entity-rooted member chain; a join or EF.Property reached the same jsonb_set(..., to_jsonb(@p)) and corrupted the row. Both are now checked by ValueSetSqlGuard on the tree the provider's parameter-based SQL processor returns, through a decorator around IRelationalParameterBasedSqlProcessorFactory: the last stage before SQL generation, and the one queries, ExecuteUpdate and ExecuteDelete all pass through. It follows values derived from array_cat (through array_remove, CASE, COALESCE and subquery/set-operation columns), allows them only under @>, <@, &&, cardinality = 0, a null test or the outermost projection, and refuses any to_jsonb/to_json of a value set in an UPDATE setter, naming the column and JSON path. The LINQ-level guards are removed; the Count translator keeps declining a visible union, now also through CASE and COALESCE, so a projection still falls back to the client. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
UnionCanonicalityTests runs each shape that used to translate and answer wrongly (projection, projected union, ternary, coalesce, removed union, membership, GroupBy, Distinct, OrderBy, set operation, Count through COALESCE and through a derived table, ExecuteUpdate) and asserts the refusal by executing the query. The legal compositions are executed and compared with memory, and a projected union is shown to materialize canonical bare, in an anonymous type, a DTO, a conditional, a derived table, a nested collection and a UNION ALL. Shapes over a UNION ALL are what prove the guard follows a column back to its array_cat. ExecuteUpdateJsonTests adds the join and EF.Property bypasses, the nested and computed setters (refused, row still loads), and the allowed cases: a whole complex property holding a set, and the same complex CLR type mapped outside the JSON column. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ve AOT Two cases in the NativeAOT consumer: a record with IRange<DateOnly> and IValueSet<string> properties written through a source-generated context, and a custom DateRange converter on a context's options reaching a value serialized through object and read into a variant. Seeding both defects fails both assertions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The EF guide, architecture notes, CLAUDE.md and the package READMEs described the 8.0.0 LINQ-level Union refusal, including the false claim that a comparison in a projection is client-evaluated and correct. They now describe what is refused on the translated SQL and why, the migration guide lists the shapes that start throwing in 9.0.0 (and corrects the 8.0 section), and the testing guide says what the new live tests prove. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The unquoted element ran to the next comma or quote with no escapes and
no structural characters, so the array grammar departed from array_in
with silently wrong values: StringSet.Parse(@"{a\,b}") was two elements,
a\ and b, where PostgreSQL reads one, a,b; {{a,b}} was the
one-dimensional set {"{a","b}"} where PostgreSQL reads a 2-D array; and
{a}b} was {"a}b"} where PostgreSQL raises an error.
Unquoted elements now follow array_in: a backslash escapes the next
character; an unescaped {, } or " inside one is refused, and a leading {
is refused as a multi-dimensional array; whitespace around the element
is dropped and whitespace inside it kept; only a bare, unescaped NULL is
the null marker (N\ULL is the string). The literal is no longer trimmed
as a whole, so a trailing escaped space belongs to the last element.
Both literal parsers skip only the whitespace PostgreSQL skips (ASCII
space, tab, newline, CR, VT, FF - LiteralWhiteSpace): a no-break space
is element text in an array, as on the server, and makes a range bound
invalid rather than being trimmed away.
SetParseFormatTests pins each case and sweeps Parse(Format(x)) == x over
strings built from backslash, braces, quotes, commas, whitespace, NBSP
and NULL. LiteralGrammarParityTests (integration) hands the same text to
PostgreSQL's array_in and range_in and requires the same answer, for
text[], int4range, daterange and int4multirange.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # docs/architecture.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # docs/serialization.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…igration guide Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…E.md Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… sets
NodaTime's ISO patterns format a value in its own calendar's fields. A
LocalDateSet<T> over a Julian LocalDate(2024, 1, 1) - ISO 2024-01-14 -
wrote {2024-01-01} and ["2024-01-01"], and the EF bridge bound, inlined
and stored ISO 2024-01-01: thirteen days off, silently, with an unequal
round trip. LocalDateTimeSet<T> and YearMonthSet<T> likewise; measured
live, a Julian January parameter matched a row holding ISO January.
A wrapper arity cannot convert its element to ISO - it can only build one
by parsing - so it refuses it: From, collection expressions and reads
check that the element's ISO text reads back as that element, and
NormalizeElement applies the same check, so Contains/Add/Remove refuse a
non-ISO probe (against the empty set nothing was compared, so Contains
answered false and Add stored it). The EF bridge gains requireRoundTrip,
set for the three families, which refuses such an element as a bare
operand.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The intro named only the JSON and culture changes. It now lists, in order of likelihood, every change a consumer can observe: payload shape, input that now throws, queries that now throw, the DateTime offset and NodaTime calendar rules, and the microsecond store text, each linking to its section. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The final-SQL guard runs from a decorator around the provider's IRelationalParameterBasedSqlProcessorFactory. A later ReplaceService, or another extension replacing rather than decorating that service, dropped it without a trace, and every refused Union shape answered as in 8.x again: array_cat equality returned no rows where memory finds both. The translator now emits Union as valueranges_union_without_sql_guard, a function PostgreSQL does not have, and only the guard renames it to array_cat. Without the guard, any query with a server-side Union fails on the server, naming the placeholder. A decorator around the guard still reaches it, so wrapping keeps working; checking for the decorator's type instead would have refused that legitimate setup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
1 participant
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.
The last review round before tagging v9.0.0. Four parallel reviews (literals, JSON/AOT, live EF, release wiring) found more silent defects. They are all fixed here, following the release rule: a silent wrong value, or a fix that tightens or changes output, goes in the major. Only items that are loud and fixable later without a major are deferred to 9.1.
Fixes
EF Core
Unionguard runs on the final SQL tree (fcd28d7), through a decorator around the provider'sIRelationalParameterBasedSqlProcessorFactory. The 8.0 LINQ pattern missed projections,?:/??,GroupBy,Distinct,OrderBy, set operations andExecuteUpdate; all of these translated and answered wrongly. The JSONExecuteUpdateguard reads the translatedUPDATE, which closes the join/EF.Propertybypass.2f15a8d). The translator emitsUnionasvalueranges_union_without_sql_guard, and only the guard renames it toarray_cat. A laterReplaceServicethat dropped the decorator used to switch every refusal off silently: the refused equality query returned no rows where memory finds both. Now that query fails with PostgreSQL 42883. A decorator around ours keeps working.Literals
DateTimeliteral keeps its wall clock, asLocal, instead of applying its offset. This is PostgreSQL'stimestampinput rule.range_in/array_ingrammar: escapes, a refused quoted-empty bound, structural braces, ASCII-only whitespace.""format behaves like no format.HH:mm.JSON
AddNodaTimeRangeConverters()no longer shadowsConfigureForNodaTime.Build
src;dependabot.ymlnow ignores NodaTime.Docs
96f62ec).architecture.md/efcore.mdupdated.Verification
CI=true: 1,579 of 1,579 passed, including both live PostgreSQL integration projects.35613f9. The two commits after it are the guard fix and docs; CI re-runs the gate on this head.Deferred to 9.1 (loud, and fixable without a major)
--nativeaotcompiled models.List<StringSet>forlist.Contains(r.Tags).IComparablefor ranges.After merge: tag the merged commit
v9.0.0. The release workflow re-runs the gate and waits for approval before the NuGet push.🤖 Generated with Claude Code