linq2db v6: shadow-property workaround for jsonb collection rewrites#2274
Draft
myieye wants to merge 36 commits into
Draft
linq2db v6: shadow-property workaround for jsonb collection rewrites#2274myieye wants to merge 36 commits into
myieye wants to merge 36 commits into
Conversation
This does not compile, due to several breaking changes (mostly various functions being moved to different namespaces). We may back out some of these version bumps later to minimize said breaking changes.
`dotnet package upgrade` upgraded most SQLitePCLRaw packages to version 3.0.3, but missed one component that would have caused inconsistencies.
Too many breaking changes in System.CommandLine version 3, and not worth solving at this time. Going back to version 2 for now.
SetDbStatementForText option has been removed, so we need to remove the line setting it. It's the default behavior now, so that's the only change needed.
Version 11 of DataAnnotatedModelValidations uses HotChocolate 16, which has some breaking changes we're trying to avoid. We need to stick to version 10 as long as we're on HotChocolate 15.
New versions of MongoDB driver can't handle chaining .Select and .Where on a IMongoQueryable, and it ends up as a System.Linq.IQueryable after the method chain. Which means MongoExtensions.ToAsyncEnumerable can no longer handle it. Replaced the LINQ query with a manually-constructed Mongo query that does the same thing. (Except that since Mongo doesn't provide .ToHashSetAsync, we have to convert it to a list first to get the data back, then convert the list to a HashSet locally to get O(1) lookup).
On JsonObjects, Should().HaveProperty() is the correct call now, rather than Should().ContainKey().
Otherwise EF tells us a migration is needed, when in fact the DB schema didn't change — but the version of the EF tools is kept in the model snapshot, and that's what's causing "migration needed" test failures.
This one is in the MorphType tests that were just merged into develop.
The version of Quartz.AspNetCore that we migrated to has some minor DB schema changes, which need a migration.
Workaround isn't ideal in all cases, we'll want to come back and revisit this once all tests are passing.
- WithOpenApi(...) -> AddOpenApiOperationTransformer(...) on route groups - ForwardedHeadersOptions.KnownNetworks -> KnownIPNetworks (drop the legacy Microsoft.AspNetCore.HttpOverrides.IPNetwork alias) - SelectAwait / ToDictionaryAwaitAsync -> Select / ToDictionaryAsync with the (T, CancellationToken) overload now in System.Linq.AsyncEnumerable - Drop package refs that are now part of the .NET 10 BCL / Microsoft.AspNetCore.App framework reference
Linq2Db v6 changed two behaviors that broke us:
1) ExpressionMethodAttribute on a collection property is now applied
during entity materialization for LoadWith(...) eager loads, not
only at filter/query translation. That blew up every entry load
with "only supported server side" from Json.QueryInternal because
the [Sql.TableFunction("json_each")] rewrite needs a FROM-clause
context. Dropping the FluentMapping registrations on
Sense.SemanticDomains and Entry.PublishIn fixes loads. The Gridify
filter map provider now wraps content-walking projections in
Json.Query(...) explicitly.
2) Linq2Db v6 + linq2db.EntityFrameworkCore 10.3 now wraps the EF
value-converter (JsonSerializer.Serialize) around any expression in
a Merge.InsertWhenNotMatched(...) projection lambda, including the
raw Sql.Expr<...> we used to cast to jsonb. Switching to the
parameterless InsertWhenNotMatched() avoids the projection entirely
and does a plain column-to-column copy from the temp table (which
is already jsonb thanks to the EF column type). ProjectId is
stamped on each commit during the bulk-copy stream.
Also: EntryQueryHelpers.SearchHeadwords uses Sql.Expr directly for
the cross-scope path access — v6 emits `[kv].*` instead of
`[kv].[key]` when Json.Value's path builder tries to convert
`kv.Key` from a captured outer-scope json_each row.
- RichMultiString.IDictionary.Add now deserializes JsonElement (which SystemTextJsonPatch v5 hands us via PocoAdapter) instead of throwing - Update VerifyDbModel snapshot for EF Core 10's removed DiscriminatorProperty lines and dropped Optional/Required annotations on nav properties
Linq2Db v6 can't translate `new CommitMetadata()` inside an InsertAsync
projection lambda (the EF JSON value converter requires a runtime serialization
that Linq2Db can't synthesize). Use the same Sql.Expr trick we already use for
ChangeEntities to give it a raw `'{}'::jsonb`.
Linq2Db v6 unconditionally wraps any column assignment in an InsertAsync projection lambda (even raw Sql.Expr) with the EF JSON value converter for the affected columns, so the Sql.Expr workaround used by the previous commit still fails. This test specifically inserts pre-serialized JSON to simulate the old commit format, so route around Linq2Db entirely with a parameterized raw INSERT via DataContextExtensions.ExecuteAsync.
The actual PG columns are HybridDateTime_DateTime and HybridDateTime_Counter (from EF ComplexProperty mapping); Linq2Db's fluent column-alias mapping flattens these for typed queries but the raw SQL has to use the physical names.
Removing the registration entirely broke Gridify-based filters that walk into the collection content (CanFilterSemanticDomainCodeContains, CanFilterByPublicationId, etc.) because Gridify only knows how to walk bare-property projections. Bring the registration back so the filter projections can stay bare (e.Senses.SelectMany(s => s.SemanticDomains)...). Two changes to make v6 happy: 1) Json.QueryInternal no longer throws — v6's eager-load preamble may inline the rewrite client-side after materialization, so the bodies return a real iterator over the deserialized list / multistring. 2) The rewrite expression returns IList<T> (via .ToList()) so v6's materializer can assign the result back into the IList<T> property without an InvalidCastException. Trade-off: filter-only tests that compare the collection projection to null (Senses.SemanticDomains=, PublishIn=, etc.) still fail because linq2db v6 can't translate `(IList<T>)json_each(...).ToList() == null` to SQL. Same handful of tests that were failing before this commit; the broader BasicApi/CRDT load path is fixed in exchange.
The .NET 10 / Linq2Db v6 fixes turned many fast-failing tests into slow-passing tests (they now actually run to completion). On Windows CI the LcmCrdt + FwLiteProjectSync test step needs more than 40 min.
Adds LINQ2DB-V6-NOTES.md next to the affected code with: the two v6 behavior changes that triggered the issues, the matrix of workarounds we tried, the trade-off the current code picks, links to the migration wiki / related issues / in-flight PR, and a clear revert checklist for when upstream lands a fix. Drops TODO pointers to that doc in the two source locations holding the workaround (Json.QueryInternal bodies and the SemanticDomains / PublishIn ExpressionMethodAttribute expressions in LcmCrdtKernel).
Replaces the .ToList()-on-property workaround with non-column shadow properties (Entry.PublishInRows / Sense.SemanticDomainRows) carrying the [ExpressionMethod] rewrite. v6's materializer doesn't walk unmapped properties, so the substitution no longer fires client-side at entity load — eager-load Regression 1 stays fixed without the .ToList() that defeated SQL translation of Any/null filters (Regression 2). The 9 filter tests that were red on the .ToList() branch now pass, and QueryPerformanceTesting is back under its v5-era 0.5 µs/entry threshold. See LINQ2DB-V6-NOTES.md for full root-cause + attempt history. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
💻 FW Lite
Contributor
|
The latest updates on your projects. Learn more about Argos notifications ↗︎ Awaiting the start of a new Argos build… |
Contributor
PublishInRows / SemanticDomainRows aren't domain state — they're server-side query-rewrite targets that happen to return the underlying collection in client context. BeEquivalentTo was walking them and failing in shapes that exclude the real collection (UpdateDiffTests, random-entry round-trip). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The properties have to stay public because LcmCrdt's filter map provider is in another assembly, but they're query-rewrite plumbing — not domain state. Mark them as: [MiniLcmInternal] — strips from MiniLcm's external JSON resolver [JsonIgnore] — and from any default System.Text.Json path [NotMapped] — and from EF column mapping [EditorBrowsable.Never] — and from IntelliSense Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8.x has a serious BeEquivalentTo perf regression that pushes FwLite CI past its 60-minute timeout. Revert the 8.x-only API call in EntityCopyMethodTests (Subject/Expectation.Description -> Description) and AssertionConfiguration.Current -> AssertionOptions.AssertEquivalencyUsing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HaveProperty on dictionary assertions was added in FA 8.x; with the pin to 7.0.0-alpha.5 we have to keep using ContainKey. Co-Authored-By: Claude Opus 4.7 (1M context) <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
2 participants
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.
Draft PR sharing progress on the linq2db v6 / .NET 10 upgrade. The shared piece is fully working but this branch also still carries the broader .NET 10 / package-bump churn that's being worked through on another branch — not intended as the merge candidate yet.
What this changes
Replaces the
.ToList()-on-property workaround forSense.SemanticDomains/Entry.PublishInwith non-column shadow properties (SemanticDomainRows/PublishInRows) that carry the[ExpressionMethod]rewrite. v6's materializer doesn't walk unmapped properties, so the substitution no longer fires client-side at entity load. Result: eager-load Regression 1 stays fixed and SQL translation ofAny/ null filters works — the 9 filter tests that were red on the.ToList()branch now pass, andQueryPerformanceTestingis back under its v5-era 0.5 µs/entry threshold.Full root-cause + attempt history + upstream-plan in
backend/FwLite/LcmCrdt/LINQ2DB-V6-NOTES.md.Test plan
dotnet test backend/FwLite/LcmCrdt.Tests/LcmCrdt.Tests.csproj— 459 passed / 2 skipped / 0 failedQueryPerformanceTesting(50k + 100k rows) passes < 0.5 µs/entryLcmCrdt.Tests(FwLiteProjectSync, FwDataMiniLcmBridge) — not run locally yet🤖 Generated with Claude Code