diff --git a/backend/src/Squidex.Domain.Apps.Entities/Apps/DomainObject/AppCommandMiddleware.cs b/backend/src/Squidex.Domain.Apps.Entities/Apps/DomainObject/AppCommandMiddleware.cs index 86d7eca14..0b669e4fe 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Apps/DomainObject/AppCommandMiddleware.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Apps/DomainObject/AppCommandMiddleware.cs @@ -37,7 +37,7 @@ public sealed class AppCommandMiddleware( { if (result.Payload is App app) { - contextProvider.Context.App = app; + contextProvider.Context = contextProvider.Context.WithApp(app); } return base.EnrichResultAsync(context, result, ct); diff --git a/backend/src/Squidex.Domain.Apps.Entities/Assets/DomainObject/AssetsBulkUpdateCommandMiddleware.cs b/backend/src/Squidex.Domain.Apps.Entities/Assets/DomainObject/AssetsBulkUpdateCommandMiddleware.cs index f032cd059..69cc1d20c 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Assets/DomainObject/AssetsBulkUpdateCommandMiddleware.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Assets/DomainObject/AssetsBulkUpdateCommandMiddleware.cs @@ -56,7 +56,7 @@ public sealed class AssetsBulkUpdateCommandMiddleware(IContextProvider contextPr return; } - contextProvider.Context.Change(b => b + contextProvider.Context = contextProvider.Context.Clone(b => b .WithNoAssetEnrichment() .WithNoCleanup() .WithUnpublished(true) diff --git a/backend/src/Squidex.Domain.Apps.Entities/Contents/DomainObject/ContentsBulkUpdateCommandMiddleware.cs b/backend/src/Squidex.Domain.Apps.Entities/Contents/DomainObject/ContentsBulkUpdateCommandMiddleware.cs index e10dc32be..d427b7e32 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Contents/DomainObject/ContentsBulkUpdateCommandMiddleware.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Contents/DomainObject/ContentsBulkUpdateCommandMiddleware.cs @@ -61,7 +61,7 @@ public sealed class ContentsBulkUpdateCommandMiddleware( return; } - contextProvider.Context.Change(b => b + contextProvider.Context = contextProvider.Context.Clone(b => b .WithNoEnrichment() .WithNoCleanup() .WithUnpublished(true) diff --git a/backend/src/Squidex.Domain.Apps.Entities/Context.cs b/backend/src/Squidex.Domain.Apps.Entities/Context.cs index 11260798b..7d31b40cc 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Context.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Context.cs @@ -5,6 +5,7 @@ // All rights reserved. Licensed under the MIT license. // ========================================================================== +using System.Collections.Concurrent; using System.Security.Claims; using Squidex.Domain.Apps.Core.Apps; using Squidex.Infrastructure; @@ -20,14 +21,21 @@ namespace Squidex.Domain.Apps.Entities; public sealed class Context { private static readonly IReadOnlyDictionary EmptyHeaders = new Dictionary(StringComparer.OrdinalIgnoreCase); + private static readonly char[] Separators = [',', ';']; - public IReadOnlyDictionary Headers { get; private set; } + // Splitting a header is not free and the same headers are read several times per request, for + // example once per schema of a query. A concurrent dictionary is used because a context is + // shared between the parallel resolvers of a GraphQL query. The context is immutable, so the + // parsed values never have to be invalidated. + private readonly ConcurrentDictionary headerValues = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); + + public IReadOnlyDictionary Headers { get; } public ClaimsPermissions UserPermissions { get; } public ClaimsPrincipal UserPrincipal { get; } - public App App { get; set; } + public App App { get; } public bool IsFrontendClient { get; } @@ -71,6 +79,25 @@ public sealed class Context return new Context(claimsPrincipal, app); } + internal string[] HeaderValues(string key) + { + if (headerValues.TryGetValue(key, out var result)) + { + return result; + } + + if (!Headers.TryGetValue(key, out var value)) + { + return []; + } + + result = value.Split(Separators, StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries).Distinct().ToArray(); + + headerValues[key] = result; + + return result; + } + public bool Allows(string permissionId, string schema = Permission.Any) { return UserPermissions.Allows(permissionId, App.Name, schema); @@ -90,13 +117,6 @@ public sealed class Context return context; } - public Context Update() - { - context.Headers = headers ?? context.Headers; - - return context; - } - public void Remove(string key) { headers ??= new Dictionary(context.Headers, StringComparer.OrdinalIgnoreCase); @@ -110,13 +130,9 @@ public sealed class Context } } - public Context Change(Action action) + public Context WithApp(App app) { - var builder = new HeaderBuilder(this); - - action(builder); - - return builder.Update(); + return new Context(app, UserPrincipal, UserPermissions, Headers); } public Context Clone(Action action) diff --git a/backend/src/Squidex.Domain.Apps.Entities/ContextHeaders.cs b/backend/src/Squidex.Domain.Apps.Entities/ContextHeaders.cs index 6238b900f..5c2e4f9b6 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/ContextHeaders.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/ContextHeaders.cs @@ -11,8 +11,6 @@ namespace Squidex.Domain.Apps.Entities; public static class ContextHeaders { - private static readonly char[] Separators = [',', ';']; - public const string KeyBatchSize = "X-BatchSize"; public const string KeyNoCacheKeys = "X-NoCacheKeys"; public const string KeyNoScripting = "X-NoScripting"; @@ -128,11 +126,8 @@ public static class ContextHeaders public static IEnumerable AsStrings(this Context context, string key) { - if (context.Headers.TryGetValue(key, out var value)) - { - return value.Split(Separators, StringSplitOptions.RemoveEmptyEntries).Select(x => x.Trim()).Distinct(); - } - - return []; + // The context parses the header once and keeps the result, because the same headers are + // read several times while a query is enriched. + return context.HeaderValues(key); } } diff --git a/backend/src/Squidex.Domain.Apps.Entities/IContextProvider.cs b/backend/src/Squidex.Domain.Apps.Entities/IContextProvider.cs index 12a6b23c1..7875f1939 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/IContextProvider.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/IContextProvider.cs @@ -9,5 +9,6 @@ namespace Squidex.Domain.Apps.Entities; public interface IContextProvider { - Context Context { get; } + // The context is immutable, so it has to be replaced to change it for the current request. + Context Context { get; set; } } diff --git a/backend/src/Squidex.Web/ContextProvider.cs b/backend/src/Squidex.Web/ContextProvider.cs index c8e80f7da..747a50a56 100644 --- a/backend/src/Squidex.Web/ContextProvider.cs +++ b/backend/src/Squidex.Web/ContextProvider.cs @@ -27,5 +27,16 @@ public sealed class ContextProvider(IHttpContextAccessor httpContextAccessor) : return httpContextAccessor.HttpContext.Context(); } + set + { + if (httpContextAccessor.HttpContext == null) + { + asyncLocal.Value = value; + } + else + { + httpContextAccessor.HttpContext.Features.Set(value); + } + } } } diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Apps/DomainObject/AppCommandMiddlewareTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Apps/DomainObject/AppCommandMiddlewareTests.cs index bd6005f35..d6c9311db 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Apps/DomainObject/AppCommandMiddlewareTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Apps/DomainObject/AppCommandMiddlewareTests.cs @@ -43,7 +43,8 @@ public class AppCommandMiddlewareTests : HandlerTestBase await HandleAsync(new UpdateApp(), replaced); - Assert.Same(replaced, ApiContext.App); + // The context is immutable, so the provider gets a new one instead of an updated one. + Assert.Same(replaced, ApiContextProvider.Context.App); } [Fact] diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Assets/DomainObject/AssetsBulkUpdateCommandMiddlewareTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Assets/DomainObject/AssetsBulkUpdateCommandMiddlewareTests.cs index d37c2148e..e0361672e 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Assets/DomainObject/AssetsBulkUpdateCommandMiddlewareTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Assets/DomainObject/AssetsBulkUpdateCommandMiddlewareTests.cs @@ -183,8 +183,9 @@ public class AssetsBulkUpdateCommandMiddlewareTests : GivenContext { var requestContext = CreateContext(false, PermissionIds.ForApp(id, AppId.Name).Id); - A.CallTo(() => contextProvider.Context) - .Returns(requestContext); + // Assign instead of configuring the getter, so that the fake keeps the value like the real + // provider does. The middleware replaces the context, because it is immutable. + contextProvider.Context = requestContext; return requestContext; } diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/ContentsBulkUpdateCommandMiddlewareTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/ContentsBulkUpdateCommandMiddlewareTests.cs index 13243f3b3..e797e7f95 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/ContentsBulkUpdateCommandMiddlewareTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/ContentsBulkUpdateCommandMiddlewareTests.cs @@ -595,8 +595,9 @@ public class ContentsBulkUpdateCommandMiddlewareTests : GivenContext PermissionIds.ForApp(id, AppId.Name, schemaId.Name).Id, PermissionIds.ForApp(id, AppId.Name, schemaCustomId.Name).Id); - A.CallTo(() => contextProvider.Context) - .Returns(requestContext); + // Assign instead of configuring the getter, so that the fake keeps the value like the real + // provider does. The middleware replaces the context, because it is immutable. + contextProvider.Context = requestContext; return requestContext; } diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/ContextHeadersTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/ContextHeadersTests.cs new file mode 100644 index 000000000..638f873ae --- /dev/null +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/ContextHeadersTests.cs @@ -0,0 +1,113 @@ +// ========================================================================== +// Squidex Headless CMS +// ========================================================================== +// Copyright (c) Squidex UG (haftungsbeschraenkt) +// All rights reserved. Licensed under the MIT license. +// ========================================================================== + +using Squidex.Domain.Apps.Core.Apps; +using Squidex.Domain.Apps.Entities.Contents; +using Squidex.Infrastructure; + +namespace Squidex.Domain.Apps.Entities; + +public class ContextHeadersTests +{ + private readonly Context sut = Context.Anonymous(new App { Name = "my-app" }); + + [Fact] + public void Should_return_empty_when_header_is_not_set() + { + Assert.Empty(sut.AsStrings("X-Fields")); + } + + [Theory] + [InlineData("a,b", new[] { "a", "b" })] + [InlineData("a, b", new[] { "a", "b" })] + [InlineData(" a ; b ", new[] { "a", "b" })] + [InlineData("a,,b", new[] { "a", "b" })] + [InlineData("a,a,b", new[] { "a", "b" })] + public void Should_split_and_trim_header(string value, string[] expected) + { + var context = sut.Clone(b => b.SetHeader("X-Fields", value)); + + Assert.Equal(expected, context.AsStrings("X-Fields").ToArray()); + } + + [Fact] + public void Should_ignore_entries_that_are_only_whitespace() + { + // These used to survive as an empty string, which made Languages() throw. + var context = sut.Clone(b => b.SetHeader("X-Fields", " , ")); + + Assert.Empty(context.AsStrings("X-Fields")); + } + + [Fact] + public void Should_reuse_parsed_header() + { + var context = sut.Clone(b => b.SetHeader("X-Fields", "a,b")); + + var actual1 = context.AsStrings("X-Fields"); + var actual2 = context.AsStrings("X-Fields"); + + Assert.Same(actual1, actual2); + } + + [Fact] + public void Should_not_share_parsed_headers_with_clone() + { + var context1 = sut.Clone(b => b.SetHeader("X-Fields", "a,b")); + + // Parse before cloning, so that a shared cache would be visible. + Assert.Equal(["a", "b"], context1.AsStrings("X-Fields").ToArray()); + + var context2 = context1.Clone(b => b.SetHeader("X-Fields", "c")); + + Assert.Equal(["c"], context2.AsStrings("X-Fields").ToArray()); + Assert.Equal(["a", "b"], context1.AsStrings("X-Fields").ToArray()); + } + + [Fact] + public void Should_not_see_header_that_has_been_removed_in_clone() + { + var context1 = sut.Clone(b => b.SetHeader("X-Fields", "a,b")); + + Assert.NotEmpty(context1.AsStrings("X-Fields")); + + var context2 = context1.Clone(b => b.Remove("X-Fields")); + + Assert.Empty(context2.AsStrings("X-Fields")); + Assert.NotEmpty(context1.AsStrings("X-Fields")); + } + + [Fact] + public void Should_return_same_context_when_nothing_is_changed() + { + var context = sut.Clone(_ => { }); + + Assert.Same(sut, context); + } + + [Fact] + public void Should_parse_languages_from_header() + { + var context = sut.Clone(b => b.WithLanguages(["en", "de", "en"])); + + Assert.Equal([Language.EN, Language.DE], context.Languages().ToArray()); + } + + [Fact] + public void Should_keep_headers_when_app_is_replaced() + { + var context1 = sut.Clone(b => b.SetHeader("X-Fields", "a,b")); + + var context2 = context1.WithApp(new App { Name = "other-app" }); + + Assert.Equal("other-app", context2.App.Name); + Assert.Equal(["a", "b"], context2.AsStrings("X-Fields").ToArray()); + + // The original must not be affected. + Assert.Equal("my-app", context1.App.Name); + } +} diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/TestHelpers/GivenContext.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/TestHelpers/GivenContext.cs index 3e33b8382..71dca6827 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/TestHelpers/GivenContext.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/TestHelpers/GivenContext.cs @@ -139,8 +139,9 @@ public class GivenContext { var result = A.Fake(); - A.CallTo(() => result.Context) - .Returns(context); + // Assign instead of configuring the getter, so that the fake keeps the value like the real + // provider does. The context is immutable and is replaced to change it. + result.Context = context; return result; } diff --git a/backend/tests/Squidex.Web.Tests/ContextProviderTests.cs b/backend/tests/Squidex.Web.Tests/ContextProviderTests.cs new file mode 100644 index 000000000..69bf5e319 --- /dev/null +++ b/backend/tests/Squidex.Web.Tests/ContextProviderTests.cs @@ -0,0 +1,122 @@ +// ========================================================================== +// Squidex Headless CMS +// ========================================================================== +// Copyright (c) Squidex UG (haftungsbeschraenkt) +// All rights reserved. Licensed under the MIT license. +// ========================================================================== + +using Microsoft.AspNetCore.Http; +using Squidex.Domain.Apps.Core.Apps; +using RequestContext = Squidex.Domain.Apps.Entities.Context; + +namespace Squidex.Web; + +public class ContextProviderTests +{ + private readonly IHttpContextAccessor httpContextAccessor = A.Fake(); + private readonly HttpContext httpContext = new DefaultHttpContext(); + private readonly App app = new App { Name = "my-app" }; + private readonly ContextProvider sut; + + public ContextProviderTests() + { + // A fake would return a dummy http context, but the fallback is only used when it is null. + A.CallTo(() => httpContextAccessor.HttpContext) + .Returns(null); + + sut = new ContextProvider(httpContextAccessor); + } + + private void UseHttpContext() + { + A.CallTo(() => httpContextAccessor.HttpContext) + .Returns(httpContext); + } + + [Fact] + public void Should_provide_context_from_http_context() + { + UseHttpContext(); + + var context = new RequestContext(httpContext.User, app); + + httpContext.Features.Set(context); + + Assert.Same(context, sut.Context); + } + + [Fact] + public void Should_create_context_when_http_context_has_none() + { + UseHttpContext(); + + Assert.NotNull(sut.Context); + } + + [Fact] + public void Should_reuse_created_context_of_http_context() + { + UseHttpContext(); + + Assert.Same(sut.Context, sut.Context); + } + + [Fact] + public void Should_read_headers_from_request() + { + UseHttpContext(); + + httpContext.Request.Headers["X-Fields"] = "a,b"; + + Assert.Equal("a,b", sut.Context.Headers["X-Fields"]); + } + + [Fact] + public void Should_write_context_to_http_context() + { + UseHttpContext(); + + var context = new RequestContext(httpContext.User, app); + + sut.Context = context; + + Assert.Same(context, httpContext.Features.Get()); + Assert.Same(context, sut.Context); + } + + [Fact] + public void Should_replace_context_of_http_context() + { + UseHttpContext(); + + var context1 = sut.Context; + var context2 = context1.Clone(b => b.SetHeader("X-Fields", "a,b")); + + sut.Context = context2; + + Assert.NotSame(context1, sut.Context); + Assert.Same(context2, sut.Context); + } + + [Fact] + public void Should_provide_anonymous_context_without_http_context() + { + Assert.NotNull(sut.Context); + } + + [Fact] + public void Should_reuse_anonymous_context_without_http_context() + { + Assert.Same(sut.Context, sut.Context); + } + + [Fact] + public void Should_write_context_without_http_context() + { + var context = new RequestContext(httpContext.User, app); + + sut.Context = context; + + Assert.Same(context, sut.Context); + } +} diff --git a/resolved.md b/resolved.md index 7dc2cbb72..13b5d5319 100644 --- a/resolved.md +++ b/resolved.md @@ -621,3 +621,123 @@ The `_Schemas_` marker keeps this key space distinct from the single-schema over interchangeable. **Verified:** build clean, all suites green. + +--- + +### 18. Sequential N+1 schema and component lookups — **CLOSED: FINDING WAS WRONG** +`backend/src/Squidex.Domain.Apps.Entities/AppProviderExtensions.cs` +`backend/src/Squidex/Areas/Api/Controllers/Contents/Generator/SchemasOpenApiGenerator.cs` + +The original finding claimed the OpenAPI docs endpoint "serialises 100 round trips" for an +app with 100 schemas. **That is not true, and the claim was never verified.** + +`ContentOpenApiController` calls `appProvider.GetSchemasAsync(AppId, ...)` *before* +`GenerateAsync`, and `AppProvider.GetSchemasAsync` writes every schema into the +request-scoped local cache under `SchemaCacheKey(appId, schema.Id)`. Inside +`GetComponentsAsync`, the component lookup is +`appProvider.GetSchemaAsync(appId, schemaId, false, ct)`, which reads that exact same key +through `GetOrCreate`. Component schemas belong to the same app by construction, so every +one of those lookups is a local-cache hit. Zero database round trips, the loop just walks +an in-memory dictionary. + +**The remaining path is real but small and not worth the risk.** `ContentEnricher` does +*not* pre-warm the cache, so a content query whose schema has component fields does pay one +round trip per distinct component schema, sequentially, on the first use in a request — +typically a handful. + +Parallelising the resolver was considered and rejected. `GetComponentsAsync` is recursive +over a shared `Dictionary` and relies on inserting each schema *before* +recursing into it, which is what breaks reference cycles between component schemas. +Running the lookups concurrently would mean unsynchronised writes to that dictionary and +would lose the cycle guarantee, in exchange for saving a couple of milliseconds on a path +that only pays the cost once per request. `AppProvider.GetOrCreate` also has a +check-then-act race that concurrency would expose. + +--- + +### 19. Header parsing re-split and re-allocated on every read — **FIXED** +`backend/src/Squidex.Domain.Apps.Entities/Context.cs` +`backend/src/Squidex.Domain.Apps.Entities/ContextHeaders.cs` + +**Was:** `AsStrings` ran `value.Split(...).Select(x => x.Trim()).Distinct()` on every call — +a split array, two LINQ iterators and an internal `HashSet` each time. The same headers are +read repeatedly per request: `ConvertData.GenerateConverter` reads `Languages()` and +`ResolveUrls()` once per schema group, and `Fields()` is read from several steps. The +headers never change once a request is running. + +**Now:** `Context` parses each header once into a `string[]` and keeps it. + +```csharp +private readonly ConcurrentDictionary headerValues = new (StringComparer.OrdinalIgnoreCase); +``` + +A `ConcurrentDictionary` rather than a plain one, because a `Context` is shared between the +parallel resolvers of a GraphQL query. The cache is cleared whenever `Headers` is assigned, +which is the only way it can change (`Context.Change`). + +`Fields()` and `Languages()` still build their own `HashSet` per call, deliberately. Their +results are handed to callers that retain them — `Q.WithFields`, `ExcludeOtherFields` — so +returning a shared instance would let one caller mutate another's copy. Caching the parsed +`string[]` removes the expensive part while leaving ownership exactly as it was. + +**This also fixed a latent crash.** The rewrite uses +`StringSplitOptions.RemoveEmptyEntries | TrimEntries`, which drops whitespace-only entries. +The old order — split, *then* trim — turned a header like `X-Languages: " , "` into a +single empty string, and `Language.GetLanguage("")` calls `Guard.NotNullOrEmpty` and +throws. Verified the difference against the runtime rather than assuming it. + +**Verified:** a new `ContextHeadersTests` covering splitting, trimming, deduplication, +memoization (`Assert.Same`), invalidation on change and on removal, clone isolation, and +the whitespace case. Two of them **fail on the pre-fix code** — the memoization test and +the whitespace test — which are exactly the two behaviours that changed. +`Squidex.Domain.Apps.Entities.Tests` green (1540). + +--- + +### Immutable `Context` (follow-up to 19) +`backend/src/Squidex.Domain.Apps.Entities/Context.cs` +`backend/src/Squidex.Domain.Apps.Entities/IContextProvider.cs` +`backend/src/Squidex.Web/ContextProvider.cs` + +`Context` was mutable in two ways: `Headers { get; private set; }` changed by `Change()`, +and a public `App { get; set; }`. That is what forced the header cache added in item 19 to +carry invalidation logic. + +**Now `IContextProvider.Context` has a setter and `Context` is immutable.** Both setters +are gone, along with `Change()` and `ICloneBuilder.Update()`; `Clone()` and a new +`WithApp()` return a new instance. The header cache needs no invalidation at all — a +`Context` parses each header at most once for its whole lifetime. + +The three mutation sites in the codebase became replacements: + +```csharp +contextProvider.Context = contextProvider.Context.WithApp(app); // AppCommandMiddleware +contextProvider.Context = contextProvider.Context.Clone(b => b.WithNoEnrichment()…); // both bulk middlewares +``` + +`ContextProvider` stores it symmetrically to how it reads it — `HttpContext.Features` when +there is a request, the `AsyncLocal` fallback when there is not. `AppResolver` already +replaced the whole context this way, so the pattern was established. + +**Why this is safe.** Replacing a reference is only equivalent to mutating in place if +nobody holds the old one. Every consumer of `IContextProvider` was checked: +`AssetCommandMiddleware`, `ContentCommandMiddleware`, `RuleCommandMiddleware`, +`EnrichWithAppIdCommandMiddleware` and both bulk middlewares all read +`contextProvider.Context` fresh at the point of use. None capture it in a field or across +an await that spans a replacement. + +**Three existing tests failed and were right to.** Their doubles pinned the getter with +`A.CallTo(() => provider.Context).Returns(ctx)`, which made a *replacement* invisible while +the old in-place mutation had been visible. The fakes now assign (`provider.Context = ctx`) +so FakeItEasy tracks the property like the real provider, and +`AppCommandMiddlewareTests` asserts through `ApiContextProvider.Context.App` rather than +through a now-stale local reference. + +**New `ContextProviderTests`** covers both storage paths: reading from and writing to +`HttpContext.Features`, header population, and the `AsyncLocal` fallback. Writing it +surfaced a trap worth knowing about — `A.Fake()` returns a *dummy* +`HttpContext` rather than `null`, so the fallback path is never reached unless the fake is +explicitly configured to return null. + +**Verified:** build clean. Entities 1541, Web 176, Core 1243, Infrastructure 1033, +Data 180 — all green. diff --git a/todo.md b/todo.md index 013f7a69f..434d33844 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**–**17** and **20** are closed and live there. +expected — items **4**–**20** are closed and live there. -**Status: 5 open of 20 — items 1, 2, 3, 18, 19. The other 15 are in [resolved.md](resolved.md).** +**Status: 3 open of 20 — items 1, 2, 3, all the same root cause. The other 17 are in [resolved.md](resolved.md).** --- @@ -66,55 +66,19 @@ already isolated in `ContentScriptVars`. --- -## S3 — Moderate - -### 18. Sequential N+1 schema and component lookups -`backend/src/Squidex.Domain.Apps.Entities/AppProviderExtensions.cs:30` -`backend/src/Squidex/Areas/Api/Controllers/Contents/Generator/SchemasOpenApiGenerator.cs:39,48` - -`ResolveSchemasAsync` awaits `appProvider.GetSchemaAsync` once per id in a loop; the -OpenAPI generator awaits `GetComponentsAsync(schema, ...)` once per schema in a loop. -For an app with 100 schemas the OpenAPI docs endpoint serialises 100 round trips that -have no dependency on each other. - -**Fix:** `await Task.WhenAll(...)` over the lookups, or add a batch accessor. Both are -warm-cache paths, which is why this sits at S3 rather than S2. - ---- - -### 19. Header parsing re-splits and re-allocates on every read -`backend/src/Squidex.Domain.Apps.Entities/Contents/ContentHeaders.cs:140,150` -`backend/src/Squidex.Domain.Apps.Entities/ContextHeaders.cs:133` - -```csharp -public static HashSet? Fields(this Context context) - => context.AsStrings(KeyFields).ToHashSet(); - -public static HashSet Languages(this Context context) - => context.AsStrings(KeyLanguages).Select(Language.GetLanguage).ToHashSet(); -``` - -`AsStrings` does `value.Split(...).Select(Trim).Distinct()`. Each call allocates the -split array, two LINQ iterators and a `HashSet`. `ConvertData.GenerateConverter` calls -`Languages()` **and** `ResolveUrls().ToList()` per schema group, and `Fields()` is read -from several steps. The headers never change for the lifetime of a `Context`. - -**Fix:** memoize the parsed values on `Context`, invalidating in the clone builder. - ---- - ## Suggested order of attack -1. **Engine pooling (items 1–3)** — by a wide margin the largest remaining cost, and the - only one left that can dominate a request. One change in `JintScriptEngine` addresses - it, and items 2 and 3 mostly disappear with it. -2. **Items 18 and 19** — steady-state allocation and a warm-cache N+1; both are small and - neither is likely to show up next to item 1. - -Everything correctness-shaped is closed, as is everything in the stability category. What -remains is pure throughput work — exactly the category that should be profiled before it -is written. Item 1 in particular is worth measuring first: the estimate that it dominates -a scripted content list comes from reading the loops, not from a trace. +Only one piece of work is left: **pool the Jint engines (item 1)**. Items 2 and 3 are the +same cost seen from two call sites and mostly disappear once item 1 is done; what remains +of them afterwards is the sequential `await` per content, which is worth re-measuring +rather than assuming. + +**Profile this one before writing it.** Everything correctness- and stability-shaped is +closed, so what is left is pure throughput, and the estimate that engine construction +dominates a scripted content list is read off the loops, not taken from a trace. Engine +pooling is also the most invasive change on the whole list — it touches the security +boundary of user-authored scripts, since a pooled engine must not carry state from one +script into the next. That is worth confirming is a real cost before taking the risk. ---