diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 000000000..b8928da15 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,55 @@ +# Squidex + +Headless CMS. Angular frontend in `frontend/`, ASP.NET Core backend in `backend/`. + +## Frontend + +- Angular app in `frontend/`, source under `src/app`: + - `framework/` — generic, reusable UI components and utilities (no domain knowledge). + - `shared/` — Squidex-specific services, state stores and components. + - `features/` — the actual screens (apps, assets, content, rules, schemas, settings, teams, ...). + - `shell/` — app frame, navigation, layout. +- State is handled with the state store pattern from `framework/state.ts` (immutable value objects + `State` subclasses), not with a third-party store library. +- Commands: + +```bash +npm start +``` + +```bash +npm test +``` + +```bash +npm run lint +``` + +### Best Practices + +- i18n texts live in `backend/i18n`, translations are generated into the frontend — do not edit generated translation files by hand. +- Do not write JsDoc comments. + +## Backend + +- .NET solution `backend/Squidex.slnx`. Projects under `backend/src`, tests under `backend/tests`, optional integrations under `backend/extensions`. +- Layering: `Squidex.Infrastructure` (generic building blocks) → `Squidex.Domain.Apps.*` (core model, operations, events, entities) → `Squidex.Web` / `Squidex` (API host). +- Event-sourced domain: aggregates emit events from `Squidex.Domain.Apps.Events`, state is projected into MongoDB or EF Core (`Squidex.Data.MongoDb`, `Squidex.Data.EntityFramework`). +- Run tests with the filter below — some tests need external setup (real databases, Docker/Testcontainers) and will fail without it: + +### Tests + +Some tests need test setup or test containers which are slow. Run the tests like this to skip these tests. + +```bash +dotnet test --filter "Category!=Dependencies & Category!=TestContainer" +``` + +### Best Practices + +- Code style is enforced by StyleCop (`backend/stylecop.json`) and `.editorconfig` — follow the surrounding file's conventions. +- Do not write XML comments. + +## Shared best practices + +- Do write precise short comments and only when needed. +- Do not comment a class or a method, only put comments inside functions or above variables. \ No newline at end of file diff --git a/backend/src/Squidex.Domain.Apps.Entities/Contents/GraphQL/GraphQLExecutionContext.cs b/backend/src/Squidex.Domain.Apps.Entities/Contents/GraphQL/GraphQLExecutionContext.cs index c8a502a80..dc241e449 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Contents/GraphQL/GraphQLExecutionContext.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Contents/GraphQL/GraphQLExecutionContext.cs @@ -23,9 +23,33 @@ public sealed class GraphQLExecutionContext : QueryExecutionContext private const int MinBatchSize = 1; private static readonly EmptyDataLoaderResult EmptyAssets = new EmptyDataLoaderResult(); private static readonly EmptyDataLoaderResult EmptyContents = new EmptyDataLoaderResult(); + + // Field names are resolved to a new set for every field, therefore the sets have to be compared + // by their content. Otherwise the results are never matched to the keys they were loaded for. + private static readonly IEqualityComparer> FieldsComparer = HashSet.CreateSetComparer(); + private readonly IDataLoaderContextAccessor dataLoaders; private readonly int batchSize; + private sealed class ContentWithFieldsComparer : IEqualityComparer<(DomainId Id, HashSet Fields)> + { + public static readonly ContentWithFieldsComparer Instance = new ContentWithFieldsComparer(); + + private ContentWithFieldsComparer() + { + } + + public bool Equals((DomainId Id, HashSet Fields) x, (DomainId Id, HashSet Fields) y) + { + return x.Id.Equals(y.Id) && FieldsComparer.Equals(x.Fields, y.Fields); + } + + public int GetHashCode((DomainId Id, HashSet Fields) obj) + { + return HashCode.Combine(obj.Id, FieldsComparer.GetHashCode(obj.Fields)); + } + } + public override Context Context { get; } public bool CanExposePII { get; } @@ -159,11 +183,21 @@ public sealed class GraphQLExecutionContext : QueryExecutionContext return dataLoaders.Context!.GetOrAddNonCachingBatchLoader<(DomainId Id, HashSet Fields), EnrichedContent>(nameof(GetContentsLoaderWithFields), async (batch, ct) => { - var fields = batch.SelectMany(x => x.Fields).ToHashSet(); + var result = new Dictionary<(DomainId Id, HashSet Fields), EnrichedContent>(ContentWithFieldsComparer.Instance); - var result = await QueryContentsByIdsAsync(batch.Select(x => x.Id), fields, ct); + // A batch can contain several field selections. They cannot be merged into a single + // query, because every content must only contain the fields it was requested with. + foreach (var byFields in batch.GroupBy(x => x.Fields, FieldsComparer)) + { + var contents = await QueryContentsByIdsAsync(byFields.Select(x => x.Id), byFields.Key, ct); - return result.ToDictionary(x => (x.Id, fields)); + foreach (var content in contents) + { + result[(content.Id, byFields.Key)] = content; + } + } + + return result; }, maxBatchSize: batchSize); } diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/GraphQL/GraphQLQueriesTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/GraphQL/GraphQLQueriesTests.cs index 63ecba4eb..4b7db83e2 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/GraphQL/GraphQLQueriesTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/GraphQL/GraphQLQueriesTests.cs @@ -1107,6 +1107,81 @@ public class GraphQLQueriesTests : GraphQLTestBase AssertResult(expected, actual); } + [Fact] + public async Task Should_resolve_referenced_contents_when_field_queries_are_optimized() + { + var contentRefId = DomainId.NewGuid(); + var contentRef = TestContent.CreateSimple(TestSchemas.Reference1.NamedId(), contentRefId, "reference1-field", "reference1"); + + var data = + new ContentData() + .AddField("my-references", + new ContentFieldData() + .AddInvariant(JsonValue.Array(contentRefId))); + + var contentId = DomainId.NewGuid(); + var content = TestContent.Create(contentId, data); + + A.CallTo(() => contentQuery.QueryAsync(MatchsContentContext(), + A.That.HasIdsWithoutTotal(contentRefId), + A._)) + .Returns(ResultList.CreateFrom(0, contentRef)); + + A.CallTo(() => contentQuery.QueryAsync(MatchsContentContext(), + A.That.HasIdsWithoutTotal(contentId), + A._)) + .Returns(ResultList.CreateFrom(1, content)); + + var actual = await ExecuteAsync(new TestQuery + { + Query = @" + query { + findMySchemaContent(id: '{contentId}') @optimizeFieldQueries { + id + flatData { + myReferences { + id + flatData { + reference1Field + } + } + } + } + }", + Args = new + { + contentId, + }, + }); + + var expected = new + { + data = new + { + findMySchemaContent = new + { + id = content.Id, + flatData = new + { + myReferences = new[] + { + new + { + id = contentRefId, + flatData = new + { + reference1Field = "reference1", + }, + }, + }, + }, + }, + }, + }; + + AssertResult(expected, actual); + } + [Fact] public async Task Should_cache_referenced_contents_from_flat_data() { diff --git a/resolved.md b/resolved.md index e1d6b85bc..a77ee435a 100644 --- a/resolved.md +++ b/resolved.md @@ -3,7 +3,8 @@ Items from the backend performance review that are done. Numbering matches [todo.md](todo.md) — resolved items keep their original number so references stay valid. -Partially-addressed items (**8**, **9**) stay in `todo.md` until closed. +Item **6** is not listed here: it is open but accepted as won't-fix, and stays documented +in `todo.md` so it is not re-reported as a new finding. --- @@ -111,3 +112,109 @@ operations over 8 threads against 3000 distinct patterns in a 1000-entry cache (continuous eviction) with 0 exceptions, 0 wrong matches and the cache correctly bounded at 1000; full `Squidex.Domain.Apps.Core.Tests` suite green (1247), plus 25 validation tests in `Squidex.Domain.Apps.Entities.Tests`. + +--- + +### 8. GraphQL field-selection data loader never matched its results — **FIXED** +`backend/src/Squidex.Domain.Apps.Entities/Contents/GraphQL/GraphQLExecutionContext.cs` + +**Was:** two separate defects in the `GetContentsLoaderWithFields` path, which serves +every GraphQL reference resolved under the `@optimizeFieldQueries` directive. + +1. `BuildKeys` wrote `keys[i] = (ids[0], fields)` — every key in the batch was the + *first* id, so one content was requested N times and the other N−1 never were. +2. The batch callback keyed its result dictionary by a freshly merged field set: + + ```csharp + var fields = batch.SelectMany(x => x.Fields).ToHashSet(); + return result.ToDictionary(x => (x.Id, fields)); + ``` + + `NonCachingBatchLoader` then looks the results up with the *original* key. The key + type is `(DomainId, HashSet)` and `HashSet` has no structural equality, so + the tuple comparer fell back to reference equality and **no lookup ever matched**. + Contents were fetched from the database and thrown away; every field-selected + reference resolved to `null`. + + This was unconditional, not a race: `SharedExtensions.FieldNames()` builds a *new* + `HashSet` per resolver invocation (`new FieldNameResolver(...).Iterate(...)`), so the + requested instance and the merged instance were never the same object. + +**Now:** `(1)` was fixed to `ids[i]`. For `(2)`, the key is compared by value: + +```csharp +private static readonly IEqualityComparer> FieldsComparer = HashSet.CreateSetComparer(); + +private sealed class ContentWithFieldsComparer : IEqualityComparer<(DomainId Id, HashSet Fields)> +{ + public bool Equals((DomainId Id, HashSet Fields) x, (DomainId Id, HashSet Fields) y) + => x.Id.Equals(y.Id) && FieldsComparer.Equals(x.Fields, y.Fields); + + public int GetHashCode((DomainId Id, HashSet Fields) obj) + => HashCode.Combine(obj.Id, FieldsComparer.GetHashCode(obj.Fields)); +} +``` + +and the callback groups by field selection instead of merging: + +```csharp +var result = new Dictionary<(DomainId Id, HashSet Fields), EnrichedContent>(ContentWithFieldsComparer.Instance); + +foreach (var byFields in batch.GroupBy(x => x.Fields, FieldsComparer)) +{ + var contents = await QueryContentsByIdsAsync(byFields.Select(x => x.Id), byFields.Key, ct); + + foreach (var content in contents) + { + result[(content.Id, byFields.Key)] = content; + } +} +``` + +Grouping rather than merging matters for correctness: a batch can hold several different +field selections, and merging them would hand a caller fields it did not request. Because +the grouping is by *value*, identical selections coming from different resolvers still +collapse into a single query — which the old reference-equality behaviour could not do. + +`HashSet.CreateSetComparer()` is cached in a static; it allocates a new comparer +on every call. + +**Verified:** a new regression test, +`GraphQLQueriesTests.Should_resolve_referenced_contents_when_field_queries_are_optimized`, +resolves a reference under `@optimizeFieldQueries`. It **fails on the pre-fix code** and +passes after — red-to-green, not just green. Full GraphQL suite (79) and full +`Squidex.Domain.Apps.Entities.Tests` (1527) green. + +--- + +### 9. Generic query-model cache key collided across apps — **FIXED** +`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/ContentQueryParser.cs:276-294` + +**Was:** the cross-schema (`schema == null`) cache key was the constant +`"EDM/__generic"` / `"JSON/__generic"`. The cached model is built from +`context.App.PartitionResolver()`, so whichever app populated the cache first imposed its +languages on every other app's cross-schema `/contents` queries for the 60-minute cache +lifetime — wrong filters accepted, correct ones rejected, across tenants. + +An intermediate fix replaced it with `$"EDM/{app.Version}/{withHidden}"`, which did not +close the hole: `App.Version` is `Entity.Version`, a per-aggregate event-stream position, +so two apps with the same event count still collided. + +**Now:** the key carries the app identity (commit `b7103a12`): + +```csharp +return $"EDM/{app.Id}/{app.Version}/{withHidden}"; +return $"EDM/{app.Id}/{app.Version}/{schema.Id}_{schema.Version}/{withHidden}"; +``` + +`app.Id` is a globally unique `DomainId`, so no two apps can share a key. + +**Deliberately not changed: the `app.Version` over-invalidation.** Keying on `app.Version` +means any app-level event (a contributor edit, a settings tweak) rebuilds the EDM models +of every schema in the app. Narrowing it to a language-specific token looked attractive — +`PartitionResolver` is just `app.Languages.ToResolver()` — but `BuildDataSchema` also +reads `partitioning.GetName(...)` and `IsOptional`, so a key built from the language +*codes* alone could serve a stale model after a language rename or fallback change. +`app.Version` is conservative but provably correct: it changes whenever anything about +the app does. Trading guaranteed correctness for a cache-hit-rate win is the wrong +direction here, so it stays until someone establishes the model's exact dependency set. diff --git a/todo.md b/todo.md index 60df24337..4944d3806 100644 --- a/todo.md +++ b/todo.md @@ -9,9 +9,9 @@ overhead / allocation churn), **S4** low (worth fixing while nearby). Item numbers are stable and never reused. Completed items move to [resolved.md](resolved.md) keeping their number, so gaps in the sequence here are -expected — items **4**, **5** and **7** are done and live there. +expected — items **4**, **5**, **7**, **8** and **9** are done and live there. -**Status: 17 open of 20 — 15 untouched, 1 half fixed (8), 1 attempted but still open (9).** +**Status: 14 open of 20. Item 6 accepted as-is (see below); items 4, 5, 7, 8, 9 are in [resolved.md](resolved.md).** --- @@ -68,88 +68,27 @@ already isolated in `ContentScriptVars`. ## S2 — High -### 9. Generic query-model cache key still collides across apps — **ATTEMPTED, STILL OPEN** -`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/ContentQueryParser.cs:280,290` - -The constant `"EDM/__generic"` was replaced with: - -```csharp -if (schema == null) return $"EDM/{app.Version}/{withHidden}"; -if (schema == null) return $"JSON/{app.Version}/{withHidden}"; -``` - -**This does not close the hole.** `App.Version` is `Entity.Version` — a per-aggregate -event-stream position (`Squidex.Infrastructure/Commands/Entity.cs:24`), not a globally -unique value. Two different apps that have received the same number of events share the -same version, which is the common case for young or low-traffic apps. `EDM/7/False` -means "app A at v7" and "app B at v7" interchangeably. - -The cached model is built from `context.App.PartitionResolver()`, so a colliding app -still parses cross-schema `/contents` queries against **another tenant's languages** — -wrong filters accepted, correct ones rejected, for the 60-minute cache lifetime. - -The schema-scoped keys on lines 283 and 293 are safe: they embed `schema.Id`, a globally -unique `DomainId`. - -**Fix:** put `app.Id` in the key, not just the version — -`$"EDM/{app.Id}/{app.Version}/{withHidden}"`. - -Separately, and unchanged: keying on `app.Version` means *any* app-level event (a -contributor edit, a settings tweak) invalidates the EDM models of every schema in the -app, forcing expensive OData model rebuilds. Keying on the language-config version -instead would invalidate only when something the model actually depends on changes. - ---- - -### 8. GraphQL field-selection data loader — **HALF FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Contents/GraphQL/GraphQLExecutionContext.cs:162,166` - -The key-building bug is fixed — line 188 is now `keys[i] = (ids[i], fields)`, so the -batch requests all N ids instead of the first one N times. - -The second half is untouched. The batch callback still keys its result dictionary by the -*merged* `fields` set: - -```csharp -var fields = batch.SelectMany(x => x.Fields).ToHashSet(); // line 162 -var result = await QueryContentsByIdsAsync(batch.Select(x => x.Id), fields, ct); -return result.ToDictionary(x => (x.Id, fields)); // line 166 -``` - -The keys the loader was *called* with hold the caller's `HashSet` instance; -`fields` here is a freshly allocated one. `HashSet` has no structural equality, so -the tuple comparer falls back to reference equality and **no lookup ever matches**. The -contents are fetched from the database and then thrown away; every field-selected -GraphQL content resolves to null. - -**Fix:** supply an `IEqualityComparer` for the tuple key that compares field sets by -content, or key by a canonical string (sorted field names joined) instead of the set -itself. - ---- - -### 6. Sync-over-async on the authentication path — **OPEN** +### 6. Sync-over-async on the authentication path — **ACCEPTED, WON'T FIX** `backend/src/Squidex/Areas/IdentityServer/Config/Dynamic/DynamicSchemeProvider.cs:129` -The file was touched (a variable rename and whitespace tidy-up), but the blocking call -is unchanged — it just moved from line 134 to 129: - ```csharp var scheme = GetSchemeCoreAsync(name, default).Result; ``` -`Get(string? name)` is an options-resolution hook invoked from the auth pipeline, so -each call parks a thread-pool thread on a DB round trip. Under load this is a classic -thread-pool starvation source, and it deadlocks outright if any sync context is ever -installed. +`Get(string? name)` blocks a thread-pool thread on a DB round trip. **Accepted as-is — +this is not an important path** (dynamic OIDC scheme resolution, only reached for +team-level auth domains, not on ordinary API traffic), so the starvation risk does not +justify the rework. Left documented rather than deleted so it is not re-reported as a +new finding. -Same pattern, lower blast radius: +If it ever does move onto a hot path, the fix is to cache scheme results synchronously +(populated by an async initializer / background refresh) so `Get` can return without +blocking. + +Same pattern elsewhere, also low blast radius: - `Squidex.Domain.Apps.Entities/Contents/DomainObject/Guards/ScriptingExtensions.cs:144` — `.Wait()` on full content validation inside a script callback. - `Squidex.Data.MongoDb/Infrastructure/MongoRepositoryBase.cs:26` — `InitializeAsync(default).Wait()`. -**Fix:** cache scheme results synchronously (populated by an async initializer / -background refresh) so `Get` can return without blocking. - --- ### 10. Unbounded in-memory request-log queue @@ -341,18 +280,18 @@ size limit, so each edit of a script adds another full-source-sized entry for th ## Suggested order of attack -1. **Finish items 9 and 8** — both are one-line-ish completions of work already started, - and both are correctness bugs. Item 9 in particular still leaks one tenant's language - config into another's query model whenever two apps share a version number. -2. **Engine pooling (items 1–3)** — one change in `JintScriptEngine` fixes the largest - open read-path cost, and items 2 and 3 mostly disappear with it. -3. **Item 11** — small, self-contained; removes an uncached full-collection count from a +1. **Engine pooling (items 1–3)** — now the largest open cost by a wide margin. One + change in `JintScriptEngine` addresses it, and items 2 and 3 mostly disappear with it. +2. **Item 11** — small, self-contained; removes an uncached full-collection count from a paged endpoint. -4. **Items 6, 10** — stability under load rather than throughput; worth doing before the - micro-optimisations. -5. **Everything else** — steady-state allocation and lock overhead; measure with a +3. **Item 10** — stability under load rather than throughput; an unbounded queue that + turns a storage outage into an OOM. +4. **Everything else** — steady-state allocation and lock overhead; measure with a profiler on a representative content-list request before and after. +All the correctness-shaped findings are now closed. What remains is genuine performance +work, which is exactly the category that should be profiled before it is written. + --- ## Method / caveats