More improvements

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<T>` 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.

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<EnrichedAsset> EmptyAssets = new EmptyDataLoaderResult<EnrichedAsset>();
     private static readonly EmptyDataLoaderResult<EnrichedContent> EmptyContents = new EmptyDataLoaderResult<EnrichedContent>();
+
+    // 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<HashSet<string>> FieldsComparer = HashSet<string>.CreateSetComparer();
+
     private readonly IDataLoaderContextAccessor dataLoaders;
     private readonly int batchSize;
 
+    private sealed class ContentWithFieldsComparer : IEqualityComparer<(DomainId Id, HashSet<string> Fields)>
+    {
+        public static readonly ContentWithFieldsComparer Instance = new ContentWithFieldsComparer();
+
+        private ContentWithFieldsComparer()
+        {
+        }
+
+        public bool Equals((DomainId Id, HashSet<string> Fields) x, (DomainId Id, HashSet<string> Fields) y)
+        {
+            return x.Id.Equals(y.Id) && FieldsComparer.Equals(x.Fields, y.Fields);
+        }
+
+        public int GetHashCode((DomainId Id, HashSet<string> 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<string> Fields), EnrichedContent>(nameof(GetContentsLoaderWithFields),
             async (batch, ct) =>
             {
-                var fields = batch.SelectMany(x => x.Fields).ToHashSet();
+                var result = new Dictionary<(DomainId Id, HashSet<string> 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);
     }
 

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<Q>.That.HasIdsWithoutTotal(contentRefId),
+                A<CancellationToken>._))
+            .Returns(ResultList.CreateFrom(0, contentRef));
+
+        A.CallTo(() => contentQuery.QueryAsync(MatchsContentContext(),
+                A<Q>.That.HasIdsWithoutTotal(contentId),
+                A<CancellationToken>._))
+            .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()
     {

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<string>)` and `HashSet<T>` 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<HashSet<string>> FieldsComparer = HashSet<string>.CreateSetComparer();
+
+private sealed class ContentWithFieldsComparer : IEqualityComparer<(DomainId Id, HashSet<string> Fields)>
+{
+    public bool Equals((DomainId Id, HashSet<string> Fields) x, (DomainId Id, HashSet<string> Fields) y)
+        => x.Id.Equals(y.Id) && FieldsComparer.Equals(x.Fields, y.Fields);
+
+    public int GetHashCode((DomainId Id, HashSet<string> 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<string> 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<string>.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.

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<string>` instance;
-`fields` here is a freshly allocated one. `HashSet<T>` 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