Browse Source

Fix

pull/1330/head
Sebastian Stehle 2 weeks ago
parent
commit
57dd605398
  1. 2
      backend/src/Squidex.Domain.Apps.Entities/Apps/DomainObject/AppCommandMiddleware.cs
  2. 2
      backend/src/Squidex.Domain.Apps.Entities/Assets/DomainObject/AssetsBulkUpdateCommandMiddleware.cs
  3. 2
      backend/src/Squidex.Domain.Apps.Entities/Contents/DomainObject/ContentsBulkUpdateCommandMiddleware.cs
  4. 46
      backend/src/Squidex.Domain.Apps.Entities/Context.cs
  5. 11
      backend/src/Squidex.Domain.Apps.Entities/ContextHeaders.cs
  6. 3
      backend/src/Squidex.Domain.Apps.Entities/IContextProvider.cs
  7. 11
      backend/src/Squidex.Web/ContextProvider.cs
  8. 3
      backend/tests/Squidex.Domain.Apps.Entities.Tests/Apps/DomainObject/AppCommandMiddlewareTests.cs
  9. 5
      backend/tests/Squidex.Domain.Apps.Entities.Tests/Assets/DomainObject/AssetsBulkUpdateCommandMiddlewareTests.cs
  10. 5
      backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/ContentsBulkUpdateCommandMiddlewareTests.cs
  11. 113
      backend/tests/Squidex.Domain.Apps.Entities.Tests/ContextHeadersTests.cs
  12. 5
      backend/tests/Squidex.Domain.Apps.Entities.Tests/TestHelpers/GivenContext.cs
  13. 122
      backend/tests/Squidex.Web.Tests/ContextProviderTests.cs
  14. 120
      resolved.md
  15. 62
      todo.md

2
backend/src/Squidex.Domain.Apps.Entities/Apps/DomainObject/AppCommandMiddleware.cs

@ -37,7 +37,7 @@ public sealed class AppCommandMiddleware(
{ {
if (result.Payload is App app) if (result.Payload is App app)
{ {
contextProvider.Context.App = app; contextProvider.Context = contextProvider.Context.WithApp(app);
} }
return base.EnrichResultAsync(context, result, ct); return base.EnrichResultAsync(context, result, ct);

2
backend/src/Squidex.Domain.Apps.Entities/Assets/DomainObject/AssetsBulkUpdateCommandMiddleware.cs

@ -56,7 +56,7 @@ public sealed class AssetsBulkUpdateCommandMiddleware(IContextProvider contextPr
return; return;
} }
contextProvider.Context.Change(b => b contextProvider.Context = contextProvider.Context.Clone(b => b
.WithNoAssetEnrichment() .WithNoAssetEnrichment()
.WithNoCleanup() .WithNoCleanup()
.WithUnpublished(true) .WithUnpublished(true)

2
backend/src/Squidex.Domain.Apps.Entities/Contents/DomainObject/ContentsBulkUpdateCommandMiddleware.cs

@ -61,7 +61,7 @@ public sealed class ContentsBulkUpdateCommandMiddleware(
return; return;
} }
contextProvider.Context.Change(b => b contextProvider.Context = contextProvider.Context.Clone(b => b
.WithNoEnrichment() .WithNoEnrichment()
.WithNoCleanup() .WithNoCleanup()
.WithUnpublished(true) .WithUnpublished(true)

46
backend/src/Squidex.Domain.Apps.Entities/Context.cs

@ -5,6 +5,7 @@
// All rights reserved. Licensed under the MIT license. // All rights reserved. Licensed under the MIT license.
// ========================================================================== // ==========================================================================
using System.Collections.Concurrent;
using System.Security.Claims; using System.Security.Claims;
using Squidex.Domain.Apps.Core.Apps; using Squidex.Domain.Apps.Core.Apps;
using Squidex.Infrastructure; using Squidex.Infrastructure;
@ -20,14 +21,21 @@ namespace Squidex.Domain.Apps.Entities;
public sealed class Context public sealed class Context
{ {
private static readonly IReadOnlyDictionary<string, string> EmptyHeaders = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); private static readonly IReadOnlyDictionary<string, string> EmptyHeaders = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
private static readonly char[] Separators = [',', ';'];
public IReadOnlyDictionary<string, string> 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<string, string[]> headerValues = new ConcurrentDictionary<string, string[]>(StringComparer.OrdinalIgnoreCase);
public IReadOnlyDictionary<string, string> Headers { get; }
public ClaimsPermissions UserPermissions { get; } public ClaimsPermissions UserPermissions { get; }
public ClaimsPrincipal UserPrincipal { get; } public ClaimsPrincipal UserPrincipal { get; }
public App App { get; set; } public App App { get; }
public bool IsFrontendClient { get; } public bool IsFrontendClient { get; }
@ -71,6 +79,25 @@ public sealed class Context
return new Context(claimsPrincipal, app); 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) public bool Allows(string permissionId, string schema = Permission.Any)
{ {
return UserPermissions.Allows(permissionId, App.Name, schema); return UserPermissions.Allows(permissionId, App.Name, schema);
@ -90,13 +117,6 @@ public sealed class Context
return context; return context;
} }
public Context Update()
{
context.Headers = headers ?? context.Headers;
return context;
}
public void Remove(string key) public void Remove(string key)
{ {
headers ??= new Dictionary<string, string>(context.Headers, StringComparer.OrdinalIgnoreCase); headers ??= new Dictionary<string, string>(context.Headers, StringComparer.OrdinalIgnoreCase);
@ -110,13 +130,9 @@ public sealed class Context
} }
} }
public Context Change(Action<ICloneBuilder> action) public Context WithApp(App app)
{ {
var builder = new HeaderBuilder(this); return new Context(app, UserPrincipal, UserPermissions, Headers);
action(builder);
return builder.Update();
} }
public Context Clone(Action<ICloneBuilder> action) public Context Clone(Action<ICloneBuilder> action)

11
backend/src/Squidex.Domain.Apps.Entities/ContextHeaders.cs

@ -11,8 +11,6 @@ namespace Squidex.Domain.Apps.Entities;
public static class ContextHeaders public static class ContextHeaders
{ {
private static readonly char[] Separators = [',', ';'];
public const string KeyBatchSize = "X-BatchSize"; public const string KeyBatchSize = "X-BatchSize";
public const string KeyNoCacheKeys = "X-NoCacheKeys"; public const string KeyNoCacheKeys = "X-NoCacheKeys";
public const string KeyNoScripting = "X-NoScripting"; public const string KeyNoScripting = "X-NoScripting";
@ -128,11 +126,8 @@ public static class ContextHeaders
public static IEnumerable<string> AsStrings(this Context context, string key) public static IEnumerable<string> AsStrings(this Context context, string key)
{ {
if (context.Headers.TryGetValue(key, out var value)) // The context parses the header once and keeps the result, because the same headers are
{ // read several times while a query is enriched.
return value.Split(Separators, StringSplitOptions.RemoveEmptyEntries).Select(x => x.Trim()).Distinct(); return context.HeaderValues(key);
}
return [];
} }
} }

3
backend/src/Squidex.Domain.Apps.Entities/IContextProvider.cs

@ -9,5 +9,6 @@ namespace Squidex.Domain.Apps.Entities;
public interface IContextProvider 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; }
} }

11
backend/src/Squidex.Web/ContextProvider.cs

@ -27,5 +27,16 @@ public sealed class ContextProvider(IHttpContextAccessor httpContextAccessor) :
return httpContextAccessor.HttpContext.Context(); return httpContextAccessor.HttpContext.Context();
} }
set
{
if (httpContextAccessor.HttpContext == null)
{
asyncLocal.Value = value;
}
else
{
httpContextAccessor.HttpContext.Features.Set(value);
}
}
} }
} }

3
backend/tests/Squidex.Domain.Apps.Entities.Tests/Apps/DomainObject/AppCommandMiddlewareTests.cs

@ -43,7 +43,8 @@ public class AppCommandMiddlewareTests : HandlerTestBase<App>
await HandleAsync(new UpdateApp(), replaced); 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] [Fact]

5
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); var requestContext = CreateContext(false, PermissionIds.ForApp(id, AppId.Name).Id);
A.CallTo(() => contextProvider.Context) // Assign instead of configuring the getter, so that the fake keeps the value like the real
.Returns(requestContext); // provider does. The middleware replaces the context, because it is immutable.
contextProvider.Context = requestContext;
return requestContext; return requestContext;
} }

5
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, schemaId.Name).Id,
PermissionIds.ForApp(id, AppId.Name, schemaCustomId.Name).Id); PermissionIds.ForApp(id, AppId.Name, schemaCustomId.Name).Id);
A.CallTo(() => contextProvider.Context) // Assign instead of configuring the getter, so that the fake keeps the value like the real
.Returns(requestContext); // provider does. The middleware replaces the context, because it is immutable.
contextProvider.Context = requestContext;
return requestContext; return requestContext;
} }

113
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);
}
}

5
backend/tests/Squidex.Domain.Apps.Entities.Tests/TestHelpers/GivenContext.cs

@ -139,8 +139,9 @@ public class GivenContext
{ {
var result = A.Fake<IContextProvider>(); var result = A.Fake<IContextProvider>();
A.CallTo(() => result.Context) // Assign instead of configuring the getter, so that the fake keeps the value like the real
.Returns(context); // provider does. The context is immutable and is replaced to change it.
result.Context = context;
return result; return result;
} }

122
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<IHttpContextAccessor>();
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<RequestContext>());
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);
}
}

120
resolved.md

@ -621,3 +621,123 @@ The `_Schemas_` marker keeps this key space distinct from the single-schema over
interchangeable. interchangeable.
**Verified:** build clean, all suites green. **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<DomainId, Schema>` 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<string, string[]> 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<IHttpContextAccessor>()` 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.

62
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 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 [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<string>? Fields(this Context context)
=> context.AsStrings(KeyFields).ToHashSet();
public static HashSet<Language> 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 ## Suggested order of attack
1. **Engine pooling (items 1–3)** — by a wide margin the largest remaining cost, and the Only one piece of work is left: **pool the Jint engines (item 1)**. Items 2 and 3 are the
only one left that can dominate a request. One change in `JintScriptEngine` addresses same cost seen from two call sites and mostly disappear once item 1 is done; what remains
it, and items 2 and 3 mostly disappear with it. of them afterwards is the sequential `await` per content, which is worth re-measuring
2. **Items 18 and 19** — steady-state allocation and a warm-cache N+1; both are small and rather than assuming.
neither is likely to show up next to item 1.
**Profile this one before writing it.** Everything correctness- and stability-shaped is
Everything correctness-shaped is closed, as is everything in the stability category. What closed, so what is left is pure throughput, and the estimate that engine construction
remains is pure throughput work — exactly the category that should be profiled before it dominates a scripted content list is read off the loops, not taken from a trace. Engine
is written. Item 1 in particular is worth measuring first: the estimate that it dominates pooling is also the most invasive change on the whole list — it touches the security
a scripted content list comes from reading the loops, not from a trace. 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.
--- ---

Loading…
Cancel
Save