diff --git a/backend/src/Squidex.Data.EntityFramework/Squidex.Data.EntityFramework.csproj b/backend/src/Squidex.Data.EntityFramework/Squidex.Data.EntityFramework.csproj index b945ed025..b1bb616c7 100644 --- a/backend/src/Squidex.Data.EntityFramework/Squidex.Data.EntityFramework.csproj +++ b/backend/src/Squidex.Data.EntityFramework/Squidex.Data.EntityFramework.csproj @@ -43,13 +43,13 @@ - - - - - - - + + + + + + + diff --git a/backend/src/Squidex.Data.MongoDb/Squidex.Data.MongoDb.csproj b/backend/src/Squidex.Data.MongoDb/Squidex.Data.MongoDb.csproj index b7ccc948c..8609dfa0f 100644 --- a/backend/src/Squidex.Data.MongoDb/Squidex.Data.MongoDb.csproj +++ b/backend/src/Squidex.Data.MongoDb/Squidex.Data.MongoDb.csproj @@ -25,12 +25,12 @@ - - - - - - + + + + + + diff --git a/backend/src/Squidex.Domain.Apps.Core.Model/Squidex.Domain.Apps.Core.Model.csproj b/backend/src/Squidex.Domain.Apps.Core.Model/Squidex.Domain.Apps.Core.Model.csproj index 9c50b0ea1..2887139e2 100644 --- a/backend/src/Squidex.Domain.Apps.Core.Model/Squidex.Domain.Apps.Core.Model.csproj +++ b/backend/src/Squidex.Domain.Apps.Core.Model/Squidex.Domain.Apps.Core.Model.csproj @@ -20,7 +20,7 @@ - + diff --git a/backend/src/Squidex.Domain.Apps.Core.Operations/Squidex.Domain.Apps.Core.Operations.csproj b/backend/src/Squidex.Domain.Apps.Core.Operations/Squidex.Domain.Apps.Core.Operations.csproj index e98a49765..ce9470016 100644 --- a/backend/src/Squidex.Domain.Apps.Core.Operations/Squidex.Domain.Apps.Core.Operations.csproj +++ b/backend/src/Squidex.Domain.Apps.Core.Operations/Squidex.Domain.Apps.Core.Operations.csproj @@ -29,8 +29,8 @@ - - + + diff --git a/backend/src/Squidex.Domain.Apps.Entities/Contents/DomainObject/Guards/WorkflowExtensions.cs b/backend/src/Squidex.Domain.Apps.Entities/Contents/DomainObject/Guards/WorkflowExtensions.cs index 0041d29b8..a36dae881 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Contents/DomainObject/Guards/WorkflowExtensions.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Contents/DomainObject/Guards/WorkflowExtensions.cs @@ -14,27 +14,27 @@ namespace Squidex.Domain.Apps.Entities.Contents.DomainObject.Guards; public static class WorkflowExtensions { - public static ValueTask GetInitialStatusAsync(this ContentOperation operation) + public static async ValueTask GetInitialStatusAsync(this ContentOperation operation) { - var workflow = GetWorkflow(operation); + using var workflow = await GetWorkflowAsync(operation); - return workflow.GetInitialStatusAsync(operation.Schema); + return workflow.GetInitialStatus(); } - public static ValueTask ShouldValidateAsync(this ContentOperation operation, Status status) + public static async ValueTask ShouldValidateAsync(this ContentOperation operation, Status status) { - var workflow = GetWorkflow(operation); + using var workflow = await GetWorkflowAsync(operation); - return workflow.ShouldValidateAsync(operation.Schema, status); + return workflow.ShouldValidate(status); } public static async Task CheckTransitionAsync(this ContentOperation operation, Status status) { if (operation.Schema.Type != SchemaType.Singleton) { - var workflow = GetWorkflow(operation); + using var workflow = await GetWorkflowAsync(operation); - if (!await workflow.CanMoveToAsync(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, status, operation.User)) + if (!workflow.CanMoveTo(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, status, operation.User)) { var values = new { oldStatus = operation.Snapshot.EditingStatus, newStatus = status }; @@ -48,9 +48,9 @@ public static class WorkflowExtensions { if (operation.Schema.Type != SchemaType.Singleton) { - var workflow = GetWorkflow(operation); + using var workflow = await GetWorkflowAsync(operation); - var statusInfo = await workflow.GetInfoAsync(operation.Snapshot.ToContent(), status); + var statusInfo = workflow.GetInfo(status); if (statusInfo == null) { @@ -64,17 +64,17 @@ public static class WorkflowExtensions { if (operation.User != null) { - var workflow = GetWorkflow(operation); + using var workflow = await GetWorkflowAsync(operation); - if (!await workflow.CanUpdateAsync(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, operation.User)) + if (!workflow.CanUpdate(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, operation.User)) { throw new DomainException(T.Get("contents.workflowErrorUpdate", new { status = operation.Snapshot.EditingStatus })); } } } - private static IContentWorkflow GetWorkflow(ContentOperation operation) + private static ValueTask GetWorkflowAsync(ContentOperation operation) { - return operation.Resolve(); + return operation.Resolve().GetWorkflowAsync(operation.App, operation.Schema); } } diff --git a/backend/src/Squidex.Domain.Apps.Entities/Contents/DynamicContentWorkflow.cs b/backend/src/Squidex.Domain.Apps.Entities/Contents/DynamicContentWorkflow.cs index 2d451074d..d9080ad6a 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Contents/DynamicContentWorkflow.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Contents/DynamicContentWorkflow.cs @@ -7,95 +7,104 @@ using System.Security.Claims; using Squidex.Domain.Apps.Core.Contents; -using Squidex.Domain.Apps.Core.Schemas; using Squidex.Domain.Apps.Core.Scripting; -using Squidex.Infrastructure; namespace Squidex.Domain.Apps.Entities.Contents; -public sealed class DynamicContentWorkflow(IScriptEngine scriptEngine, IAppProvider appProvider) : IContentWorkflow +public sealed class DynamicContentWorkflow(WorkflowDefinition definition, IScriptEngine scriptEngine) : IContentWorkflow { - public async ValueTask GetAllAsync(Schema schema) - { - var workflow = await GetWorkflowAsync(schema.AppId.Id, schema.Id); + // The same expression is evaluated for every content of a batch, therefore the compiled script is + // kept around. A script that cannot be compiled is stored as null to not retry it over and over. + private readonly Dictionary scripts = []; - return workflow.Steps.Select(x => new StatusInfo(x.Key, GetColor(x.Value))).ToArray(); - } + // The next statuses of a step without any condition are the same for every content and user. + private readonly Dictionary nextStatuses = []; - public async ValueTask CanPublishInitialAsync(Schema schema, ClaimsPrincipal? user) + public StatusInfo[] GetAll() { - var workflow = await GetWorkflowAsync(schema.AppId.Id, schema.Id); - - return workflow.TryGetTransition(workflow.Initial, Status.Published, out var transition) && IsTrue(transition, null, user); + return definition.AllStatuses; } - public async ValueTask CanMoveToAsync(Content content, Status status, Status next, ClaimsPrincipal? user) + public Status GetInitialStatus() { - var workflow = await GetWorkflowAsync(content.AppId.Id, content.SchemaId.Id); - - return workflow.TryGetTransition(status, next, out var transition) && IsTrue(transition, content.Data, user); + return definition.Workflow.Initial; } - public async ValueTask CanUpdateAsync(Content content, Status status, ClaimsPrincipal? user) + public StatusInfo? GetInfo(Status status) { - var workflow = await GetWorkflowAsync(content.AppId.Id, content.SchemaId.Id); - - if (workflow.TryGetStep(status, out var step)) - { - return step.NoUpdate == null || !IsTrue(step.NoUpdate, content.Data, user); - } - - return true; + return definition.StatusInfos.GetValueOrDefault(status); } - public async ValueTask ShouldValidateAsync(Schema schema, Status status) + public bool ShouldValidate(Status status) { - var workflow = await GetWorkflowAsync(schema.AppId.Id, schema.Id); - - if (workflow.TryGetStep(status, out var step) && step.Validate) + if (definition.Workflow.TryGetStep(status, out var step) && step.Validate) { return true; } - return status == Status.Published && schema.Properties.ValidateOnPublish; + return status == Status.Published && definition.ValidateOnPublish; } - public async ValueTask GetInfoAsync(Content content, Status status) + public bool CanPublishInitial(ClaimsPrincipal? user) { - var workflow = await GetWorkflowAsync(content.AppId.Id, content.SchemaId.Id); + var workflow = definition.Workflow; - if (workflow.TryGetStep(status, out var step)) - { - return new StatusInfo(status, GetColor(step)); - } - - return null; + return workflow.TryGetTransition(workflow.Initial, Status.Published, out var transition) && IsTrue(transition, null, user); } - public async ValueTask GetInitialStatusAsync(Schema schema) + public bool CanMoveTo(Content content, Status status, Status next, ClaimsPrincipal? user) { - var workflow = await GetWorkflowAsync(schema.AppId.Id, schema.Id); + return definition.Workflow.TryGetTransition(status, next, out var transition) && IsTrue(transition, content.Data, user); + } - var (status, _) = workflow.GetInitialStepId(); + public bool CanUpdate(Content content, Status status, ClaimsPrincipal? user) + { + if (definition.Workflow.TryGetStep(status, out var step)) + { + return step.NoUpdate == null || !IsTrue(step.NoUpdate, content.Data, user); + } - return status; + return true; } - public async ValueTask GetNextAsync(Content content, Status status, ClaimsPrincipal? user) + public StatusInfo[] GetNext(Content content, Status status, ClaimsPrincipal? user) { - var result = new List(); + if (nextStatuses.TryGetValue(status, out var cached)) + { + return cached; + } - var workflow = await GetWorkflowAsync(content.AppId.Id, content.SchemaId.Id); + List? result = null; - foreach (var (to, step, transition) in workflow.GetTransitions(status)) + var isStatic = true; + foreach (var (to, _, transition) in definition.Workflow.GetTransitions(status)) { + isStatic = isStatic && IsUnconditional(transition); + if (IsTrue(transition, content.Data, user)) { - result.Add(new StatusInfo(to, GetColor(step))); + result ??= []; + result.Add(definition.StatusInfos[to]); } } - return result.ToArray(); + var statuses = result?.ToArray() ?? []; + if (isStatic) + { + nextStatuses[status] = statuses; + } + + return statuses; + } + + public void Dispose() + { + foreach (var script in scripts.Values) + { + script?.Dispose(); + } + + scripts.Clear(); } private bool IsTrue(WorkflowCondition condition, ContentData? data, ClaimsPrincipal? user) @@ -110,43 +119,45 @@ public sealed class DynamicContentWorkflow(IScriptEngine scriptEngine, IAppProvi if (!string.IsNullOrWhiteSpace(condition?.Expression) && data != null) { + var script = GetScript(condition.Expression); + if (script == null) + { + return false; + } + var vars = new DataScriptVars { Data = data, }; - return scriptEngine.Evaluate(vars, condition.Expression); + return script.Evaluate(vars); } return true; } - private async ValueTask GetWorkflowAsync(DomainId appId, DomainId schemaId) + private IScript? GetScript(string expression) { - Workflow? result = null; - - var app = await appProvider.GetAppAsync(appId, false); - - if (app != null) + if (scripts.TryGetValue(expression, out var script)) { - result = app.Workflows.Values.FirstOrDefault(x => x.SchemaIds.Contains(schemaId)); - - if (result == null) - { - result = app.Workflows.Values.FirstOrDefault(x => x.SchemaIds.Count == 0); - } + return script; } - if (result == null) + try + { + script = scriptEngine.CreateScript(expression); + } + catch { - result = Workflow.Default; + script = null; } - return result; + scripts[expression] = script; + return script; } - private static string GetColor(WorkflowStep step) + private static bool IsUnconditional(WorkflowCondition condition) { - return step.Color ?? StatusColors.Draft; + return condition.Roles == null && string.IsNullOrWhiteSpace(condition.Expression); } } diff --git a/backend/src/Squidex.Domain.Apps.Entities/Contents/DynamicContentWorkflows.cs b/backend/src/Squidex.Domain.Apps.Entities/Contents/DynamicContentWorkflows.cs new file mode 100644 index 000000000..1ed7b085b --- /dev/null +++ b/backend/src/Squidex.Domain.Apps.Entities/Contents/DynamicContentWorkflows.cs @@ -0,0 +1,44 @@ +// ========================================================================== +// Squidex Headless CMS +// ========================================================================== +// Copyright (c) Squidex UG (haftungsbeschraenkt) +// All rights reserved. Licensed under the MIT license. +// ========================================================================== + +using Squidex.Caching; +using Squidex.Domain.Apps.Core.Apps; +using Squidex.Domain.Apps.Core.Contents; +using Squidex.Domain.Apps.Core.Schemas; +using Squidex.Domain.Apps.Core.Scripting; + +namespace Squidex.Domain.Apps.Entities.Contents; + +public sealed class DynamicContentWorkflows(IScriptEngine scriptEngine, ILocalCache localCache) : IContentWorkflows +{ + public ValueTask GetWorkflowAsync(App app, Schema schema, + CancellationToken ct = default) + { + // The definition is only cached for the current request, because the workflow must never be + // stale. It is resolved several times per request, especially once per command guard. + var cacheKey = (nameof(DynamicContentWorkflows), app.Id, app.Version, schema.Id, schema.Version); + + if (!localCache.TryGetValue(cacheKey, out var cached) || cached is not WorkflowDefinition definition) + { + definition = CreateDefinition(app, schema); + + localCache.Add(cacheKey, definition); + } + + return new ValueTask(new DynamicContentWorkflow(definition, scriptEngine)); + } + + private static WorkflowDefinition CreateDefinition(App app, Schema schema) + { + var workflow = + app.Workflows.Values.FirstOrDefault(x => x.SchemaIds.Contains(schema.Id)) ?? + app.Workflows.Values.FirstOrDefault(x => x.SchemaIds.Count == 0) ?? + Workflow.Default; + + return new WorkflowDefinition(workflow, schema.Properties.ValidateOnPublish); + } +} diff --git a/backend/src/Squidex.Domain.Apps.Entities/Contents/IContentWorkflow.cs b/backend/src/Squidex.Domain.Apps.Entities/Contents/IContentWorkflow.cs index f81a93d35..8a7db9eb6 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Contents/IContentWorkflow.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Contents/IContentWorkflow.cs @@ -7,25 +7,24 @@ using System.Security.Claims; using Squidex.Domain.Apps.Core.Contents; -using Squidex.Domain.Apps.Core.Schemas; namespace Squidex.Domain.Apps.Entities.Contents; -public interface IContentWorkflow +public interface IContentWorkflow : IDisposable { - ValueTask GetInitialStatusAsync(Schema schema); + Status GetInitialStatus(); - ValueTask CanMoveToAsync(Content content, Status status, Status next, ClaimsPrincipal? user); + bool CanMoveTo(Content content, Status status, Status next, ClaimsPrincipal? user); - ValueTask CanUpdateAsync(Content content, Status status, ClaimsPrincipal? user); + bool CanUpdate(Content content, Status status, ClaimsPrincipal? user); - ValueTask CanPublishInitialAsync(Schema schema, ClaimsPrincipal? user); + bool CanPublishInitial(ClaimsPrincipal? user); - ValueTask ShouldValidateAsync(Schema schema, Status status); + bool ShouldValidate(Status status); - ValueTask GetInfoAsync(Content content, Status status); + StatusInfo? GetInfo(Status status); - ValueTask GetNextAsync(Content content, Status status, ClaimsPrincipal? user); + StatusInfo[] GetNext(Content content, Status status, ClaimsPrincipal? user); - ValueTask GetAllAsync(Schema schema); + StatusInfo[] GetAll(); } diff --git a/backend/src/Squidex.Domain.Apps.Entities/Contents/IContentWorkflows.cs b/backend/src/Squidex.Domain.Apps.Entities/Contents/IContentWorkflows.cs new file mode 100644 index 000000000..ca2043bda --- /dev/null +++ b/backend/src/Squidex.Domain.Apps.Entities/Contents/IContentWorkflows.cs @@ -0,0 +1,17 @@ +// ========================================================================== +// 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.Core.Schemas; + +namespace Squidex.Domain.Apps.Entities.Contents; + +public interface IContentWorkflows +{ + ValueTask GetWorkflowAsync(App app, Schema schema, + CancellationToken ct = default); +} diff --git a/backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/EnrichWithWorkflows.cs b/backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/EnrichWithWorkflows.cs index f80c6baa7..0e41e967b 100644 --- a/backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/EnrichWithWorkflows.cs +++ b/backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/EnrichWithWorkflows.cs @@ -6,34 +6,44 @@ // ========================================================================== using Squidex.Domain.Apps.Core.Contents; -using Squidex.Infrastructure; namespace Squidex.Domain.Apps.Entities.Contents.Queries.Steps; -public sealed class EnrichWithWorkflows(IContentWorkflow contentWorkflow) : IContentEnricherStep +public sealed class EnrichWithWorkflows(IContentWorkflows contentWorkflows) : IContentEnricherStep { private const string DefaultColor = StatusColors.Draft; public async Task EnrichAsync(Context context, IEnumerable contents, ProvideSchema schemas, CancellationToken ct) { - var cache = new Dictionary<(DomainId, Status), StatusInfo>(); + var withStatuses = ShouldEnrichWithStatuses(context); - foreach (var content in contents) + foreach (var group in contents.GroupBy(x => x.SchemaId.Id)) { ct.ThrowIfCancellationRequested(); - await EnrichColorAsync(content, cache); + var (schema, _) = await schemas(group.Key); - if (ShouldEnrichWithStatuses(context)) + // The workflow is resolved once per schema and caches the compiled conditions and the + // status colors for all contents of the group. + using var workflow = await contentWorkflows.GetWorkflowAsync(context.App, schema, ct); + + foreach (var content in group) { - await EnrichNextsAsync(content, context); - await EnrichCanUpdateAsync(content, context); + ct.ThrowIfCancellationRequested(); + + EnrichColor(content, workflow); + + if (withStatuses) + { + EnrichNexts(content, workflow, context); + EnrichCanUpdate(content, workflow, context); + } } } } - private async Task EnrichNextsAsync(EnrichedContent content, Context context) + private static void EnrichNexts(EnrichedContent content, IContentWorkflow workflow, Context context) { var editingStatus = content.NewStatus ?? content.Status; @@ -53,47 +63,35 @@ public sealed class EnrichWithWorkflows(IContentWorkflow contentWorkflow) : ICon } else { - content.NextStatuses = await contentWorkflow.GetNextAsync(content, editingStatus, context.UserPrincipal); + content.NextStatuses = workflow.GetNext(content, editingStatus, context.UserPrincipal); } } - private async Task EnrichCanUpdateAsync(EnrichedContent content, Context context) + private static void EnrichCanUpdate(EnrichedContent content, IContentWorkflow workflow, Context context) { var editingStatus = content.NewStatus ?? content.Status; - content.CanUpdate = await contentWorkflow.CanUpdateAsync(content, editingStatus, context.UserPrincipal); + content.CanUpdate = workflow.CanUpdate(content, editingStatus, context.UserPrincipal); } - private async Task EnrichColorAsync(EnrichedContent content, Dictionary<(DomainId, Status), StatusInfo> cache) + private static void EnrichColor(EnrichedContent content, IContentWorkflow workflow) { - content.StatusColor = await GetColorAsync(content, content.Status, cache); + content.StatusColor = GetColor(workflow, content.Status); if (content.NewStatus != null) { - content.NewStatusColor = await GetColorAsync(content, content.NewStatus.Value, cache); + content.NewStatusColor = GetColor(workflow, content.NewStatus.Value); } if (content.ScheduleJob != null) { - content.ScheduledStatusColor = await GetColorAsync(content, content.ScheduleJob.Status, cache); + content.ScheduledStatusColor = GetColor(workflow, content.ScheduleJob.Status); } } - private async Task GetColorAsync(Content content, Status status, Dictionary<(DomainId, Status), StatusInfo> cache) + private static string GetColor(IContentWorkflow workflow, Status status) { - if (!cache.TryGetValue((content.SchemaId.Id, status), out var info)) - { - info = await contentWorkflow.GetInfoAsync(content, status); - - if (info == null) - { - info = new StatusInfo(status, DefaultColor); - } - - cache[(content.SchemaId.Id, status)] = info; - } - - return info.Color; + return workflow.GetInfo(status)?.Color ?? DefaultColor; } private static bool ShouldEnrichWithStatuses(Context context) diff --git a/backend/src/Squidex.Domain.Apps.Entities/Contents/WorkflowDefinition.cs b/backend/src/Squidex.Domain.Apps.Entities/Contents/WorkflowDefinition.cs new file mode 100644 index 000000000..f99b8752e --- /dev/null +++ b/backend/src/Squidex.Domain.Apps.Entities/Contents/WorkflowDefinition.cs @@ -0,0 +1,38 @@ +// ========================================================================== +// Squidex Headless CMS +// ========================================================================== +// Copyright (c) Squidex UG (haftungsbeschraenkt) +// All rights reserved. Licensed under the MIT license. +// ========================================================================== + +using Squidex.Domain.Apps.Core.Contents; + +namespace Squidex.Domain.Apps.Entities.Contents; + +public sealed class WorkflowDefinition +{ + public Workflow Workflow { get; } + + public IReadOnlyDictionary StatusInfos { get; } + + public StatusInfo[] AllStatuses { get; } + + public bool ValidateOnPublish { get; } + + public WorkflowDefinition(Workflow workflow, bool validateOnPublish) + { + // The status infos never change for a workflow, therefore they are created once and shared + // by all contents instead of allocating them for every single status lookup. + var statusInfos = new Dictionary(workflow.Steps.Count); + + foreach (var (status, step) in workflow.Steps) + { + statusInfos[status] = new StatusInfo(status, step.Color ?? StatusColors.Draft); + } + + Workflow = workflow; + StatusInfos = statusInfos; + AllStatuses = [.. statusInfos.Values]; + ValidateOnPublish = validateOnPublish; + } +} diff --git a/backend/src/Squidex.Infrastructure/Squidex.Infrastructure.csproj b/backend/src/Squidex.Infrastructure/Squidex.Infrastructure.csproj index de440b882..a00699714 100644 --- a/backend/src/Squidex.Infrastructure/Squidex.Infrastructure.csproj +++ b/backend/src/Squidex.Infrastructure/Squidex.Infrastructure.csproj @@ -24,13 +24,13 @@ - - - - - - - + + + + + + + diff --git a/backend/src/Squidex/Areas/Api/Controllers/Contents/ContentsController.cs b/backend/src/Squidex/Areas/Api/Controllers/Contents/ContentsController.cs index af42473a3..dea8d5719 100644 --- a/backend/src/Squidex/Areas/Api/Controllers/Contents/ContentsController.cs +++ b/backend/src/Squidex/Areas/Api/Controllers/Contents/ContentsController.cs @@ -26,7 +26,7 @@ namespace Squidex.Areas.Api.Controllers.Contents; public sealed class ContentsController( ICommandBus commandBus, IContentQueryService contentQuery, - IContentWorkflow contentWorkflow) + IContentWorkflows contentWorkflows) : ApiController(commandBus) { /// @@ -83,7 +83,7 @@ public sealed class ContentsController( var response = Deferred.AsyncResponse(() => { - return ContentsDto.FromContentsAsync(contents, Resources, Schema, contentWorkflow); + return ContentsDto.FromContentsAsync(contents, Resources, Schema, contentWorkflows); }); return Ok(response); @@ -117,7 +117,7 @@ public sealed class ContentsController( var response = Deferred.AsyncResponse(() => { - return ContentsDto.FromContentsAsync(contents, Resources, Schema, contentWorkflow); + return ContentsDto.FromContentsAsync(contents, Resources, Schema, contentWorkflows); }); return Ok(response); @@ -216,7 +216,7 @@ public sealed class ContentsController( var response = Deferred.AsyncResponse(() => { - return ContentsDto.FromContentsAsync(contents, Resources, null, contentWorkflow); + return ContentsDto.FromContentsAsync(contents, Resources, null, contentWorkflows); }); return Ok(response); @@ -251,7 +251,7 @@ public sealed class ContentsController( var response = Deferred.AsyncResponse(() => { - return ContentsDto.FromContentsAsync(contents, Resources, null, contentWorkflow); + return ContentsDto.FromContentsAsync(contents, Resources, null, contentWorkflows); }); return Ok(response); diff --git a/backend/src/Squidex/Areas/Api/Controllers/Contents/ContentsSharedController.cs b/backend/src/Squidex/Areas/Api/Controllers/Contents/ContentsSharedController.cs index 141ecabdb..11bb32441 100644 --- a/backend/src/Squidex/Areas/Api/Controllers/Contents/ContentsSharedController.cs +++ b/backend/src/Squidex/Areas/Api/Controllers/Contents/ContentsSharedController.cs @@ -25,7 +25,7 @@ namespace Squidex.Areas.Api.Controllers.Contents; public sealed class ContentsSharedController( ICommandBus commandBus, IContentQueryService contentQuery, - IContentWorkflow contentWorkflow) + IContentWorkflows contentWorkflows) : ApiController(commandBus) { private static readonly GraphQLHttpMiddlewareOptions GraphQLOptions = new GraphQLHttpMiddlewareOptions @@ -146,7 +146,7 @@ public sealed class ContentsSharedController( var response = Deferred.AsyncResponse(() => { - return ContentsDto.FromContentsAsync(contents, Resources, null, contentWorkflow); + return ContentsDto.FromContentsAsync(contents, Resources, null, contentWorkflows); }); return Ok(response); @@ -179,7 +179,7 @@ public sealed class ContentsSharedController( var response = Deferred.AsyncResponse(() => { - return ContentsDto.FromContentsAsync(contents, Resources, null, contentWorkflow); + return ContentsDto.FromContentsAsync(contents, Resources, null, contentWorkflows); }); return Ok(response); diff --git a/backend/src/Squidex/Areas/Api/Controllers/Contents/Models/ContentsDto.cs b/backend/src/Squidex/Areas/Api/Controllers/Contents/Models/ContentsDto.cs index f68347947..61afbaed2 100644 --- a/backend/src/Squidex/Areas/Api/Controllers/Contents/Models/ContentsDto.cs +++ b/backend/src/Squidex/Areas/Api/Controllers/Contents/Models/ContentsDto.cs @@ -30,7 +30,7 @@ public sealed class ContentsDto : Resource public StatusInfoDto[] Statuses { get; set; } public static async Task FromContentsAsync(IResultList contents, Resources resources, - Schema? schema, IContentWorkflow workflow) + Schema? schema, IContentWorkflows workflows) { var result = new ContentsDto { @@ -40,8 +40,10 @@ public sealed class ContentsDto : Resource if (schema != null) { - await result.CreateStatusesAsync(workflow, schema); - await result.CreateLinksAsync(resources, workflow, schema); + using var workflow = await workflows.GetWorkflowAsync(resources.Context.App, schema); + + result.CreateStatuses(workflow); + result.CreateLinks(resources, workflow, schema); } else { @@ -51,14 +53,12 @@ public sealed class ContentsDto : Resource return result; } - private async Task CreateStatusesAsync(IContentWorkflow workflow, Schema schema) + private void CreateStatuses(IContentWorkflow workflow) { - var allStatuses = await workflow.GetAllAsync(schema); - - Statuses = allStatuses.Select(StatusInfoDto.FromDomain).ToArray(); + Statuses = workflow.GetAll().Select(StatusInfoDto.FromDomain).ToArray(); } - private async Task CreateLinksAsync(Resources resources, IContentWorkflow workflow, Schema schema) + private void CreateLinks(Resources resources, IContentWorkflow workflow, Schema schema) { var values = new { app = resources.App, schema = schema.Name }; @@ -69,7 +69,7 @@ public sealed class ContentsDto : Resource AddPostLink("create", resources.Url(x => nameof(x.PostContent), values)); - if (resources.CanChangeStatus(values.schema) && await workflow.CanPublishInitialAsync(schema, resources.Context.UserPrincipal)) + if (resources.CanChangeStatus(values.schema) && workflow.CanPublishInitial(resources.Context.UserPrincipal)) { var publishValues = new { values.app, values.schema, publish = true }; diff --git a/backend/src/Squidex/Areas/Api/Controllers/Schemas/Models/QueryModelDto.cs b/backend/src/Squidex/Areas/Api/Controllers/Schemas/Models/QueryModelDto.cs index 0020a13eb..c7ed206cd 100644 --- a/backend/src/Squidex/Areas/Api/Controllers/Schemas/Models/QueryModelDto.cs +++ b/backend/src/Squidex/Areas/Api/Controllers/Schemas/Models/QueryModelDto.cs @@ -5,6 +5,7 @@ // All rights reserved. Licensed under the MIT license. // ========================================================================== +using Squidex.Domain.Apps.Core.Apps; using Squidex.Domain.Apps.Core.Schemas; using Squidex.Domain.Apps.Entities.Contents; using Squidex.Infrastructure.Queries; @@ -20,22 +21,17 @@ public sealed class QueryModelDto public StatusInfoDto[] Statuses { get; set; } - public static async Task FromModelAsync(QueryModel model, Schema? schema, IContentWorkflow workflow) + public static async Task FromModelAsync(QueryModel model, App app, Schema? schema, IContentWorkflows workflows) { var result = SimpleMapper.Map(model, new QueryModelDto()); if (schema != null) { - await result.AssignStatusesAsync(workflow, schema); + using var workflow = await workflows.GetWorkflowAsync(app, schema); + + result.Statuses = workflow.GetAll().Select(StatusInfoDto.FromDomain).ToArray(); } return result; } - - private async Task AssignStatusesAsync(IContentWorkflow workflow, Schema schema) - { - var allStatuses = await workflow.GetAllAsync(schema); - - Statuses = allStatuses.Select(StatusInfoDto.FromDomain).ToArray(); - } } diff --git a/backend/src/Squidex/Areas/Api/Controllers/Schemas/SchemasController.cs b/backend/src/Squidex/Areas/Api/Controllers/Schemas/SchemasController.cs index b82086086..94e4cb091 100644 --- a/backend/src/Squidex/Areas/Api/Controllers/Schemas/SchemasController.cs +++ b/backend/src/Squidex/Areas/Api/Controllers/Schemas/SchemasController.cs @@ -30,7 +30,7 @@ namespace Squidex.Areas.Api.Controllers.Schemas; [ApiExplorerSettings(GroupName = nameof(Schemas))] public sealed class SchemasController( ICommandBus commandBus, - IContentWorkflow workflow, + IContentWorkflows workflows, IAppProvider appProvider, SchemaAIGenerator schemaAIGenerator, ScriptingCompleter scriptingCompleter) @@ -395,7 +395,7 @@ public sealed class SchemasController( var components = await appProvider.GetComponentsAsync(Schema, HttpContext.RequestAborted); var result = ContentQueryModel.Build(Schema, App.PartitionResolver(), components).Flatten(); - var response = await QueryModelDto.FromModelAsync(result, Schema, workflow); + var response = await QueryModelDto.FromModelAsync(result, App, Schema, workflows); return Ok(response); } diff --git a/backend/src/Squidex/Config/Domain/ContentsServices.cs b/backend/src/Squidex/Config/Domain/ContentsServices.cs index 4a04a1b2a..ef4257c7c 100644 --- a/backend/src/Squidex/Config/Domain/ContentsServices.cs +++ b/backend/src/Squidex/Config/Domain/ContentsServices.cs @@ -90,8 +90,8 @@ public static class ContentsServices services.AddSingletonAs() .As(); - services.AddSingletonAs() - .AsOptional(); + services.AddSingletonAs() + .AsOptional(); services.AddSingletonAs() .AsOptional(); diff --git a/backend/src/Squidex/Squidex.csproj b/backend/src/Squidex/Squidex.csproj index b893282dd..2d9c56784 100644 --- a/backend/src/Squidex/Squidex.csproj +++ b/backend/src/Squidex/Squidex.csproj @@ -57,16 +57,16 @@ - - - - - - + + + + + + - - - + + + @@ -80,12 +80,12 @@ - + - + diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/ContentDomainObjectTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/ContentDomainObjectTests.cs index 6c1a6a52a..bc5c2a5f1 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/ContentDomainObjectTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/ContentDomainObjectTests.cs @@ -10,6 +10,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using NodaTime; using Squidex.Domain.Apps.Core; +using Squidex.Domain.Apps.Core.Apps; using Squidex.Domain.Apps.Core.Contents; using Squidex.Domain.Apps.Core.Schemas; using Squidex.Domain.Apps.Core.Scripting; @@ -29,6 +30,7 @@ public class ContentDomainObjectTests : HandlerTestBase { private readonly DomainId contentId = DomainId.NewGuid(); private readonly IContentWorkflow contentWorkflow = A.Fake(); + private readonly IContentWorkflows contentWorkflows = A.Fake(); private readonly IContentRepository contentRepository = A.Fake(); private readonly IScriptEngine scriptEngine = A.Fake(); @@ -100,22 +102,25 @@ public class ContentDomainObjectTests : HandlerTestBase A.CallTo(() => scriptEngine.Execute(A._, A._, A._)) .Returns(JsonValue.Create(43)); - A.CallTo(() => contentWorkflow.GetInitialStatusAsync(Schema)) + A.CallTo(() => contentWorkflows.GetWorkflowAsync(A._, A._, A._)) + .Returns(contentWorkflow); + + A.CallTo(() => contentWorkflow.GetInitialStatus()) .Returns(Status.Draft); - A.CallTo(() => contentWorkflow.CanMoveToAsync(A._, Status.Draft, Status.Published, A._)) + A.CallTo(() => contentWorkflow.CanMoveTo(A._, Status.Draft, Status.Published, A._)) .Returns(true); - A.CallTo(() => contentWorkflow.CanMoveToAsync(A._, Status.Draft, Status.Archived, A._)) + A.CallTo(() => contentWorkflow.CanMoveTo(A._, Status.Draft, Status.Archived, A._)) .Returns(true); - A.CallTo(() => contentWorkflow.CanMoveToAsync(A._, Status.Published, Status.Draft, A._)) + A.CallTo(() => contentWorkflow.CanMoveTo(A._, Status.Published, Status.Draft, A._)) .Returns(true); - A.CallTo(() => contentWorkflow.CanMoveToAsync(A._, Status.Published, Status.Archived, A._)) + A.CallTo(() => contentWorkflow.CanMoveTo(A._, Status.Published, Status.Archived, A._)) .Returns(true); - A.CallTo(() => contentWorkflow.CanUpdateAsync(A._, A._, A._)) + A.CallTo(() => contentWorkflow.CanUpdate(A._, A._, A._)) .Returns(true); patched = patch.MergeInto(data); @@ -127,7 +132,7 @@ public class ContentDomainObjectTests : HandlerTestBase .AddSingleton(AppProvider) .AddSingleton(A.Fake>()) .AddSingleton(log) - .AddSingleton(contentWorkflow) + .AddSingleton(contentWorkflows) .AddSingleton(contentRepository) .AddSingleton(scriptEngine) .AddSingleton(TestUtils.DefaultSerializer) @@ -612,7 +617,7 @@ public class ContentDomainObjectTests : HandlerTestBase var command = new ChangeContentStatus { Status = Status.Archived, StatusJobId = sut.Snapshot.ScheduleJob!.Id }; - A.CallTo(() => contentWorkflow.CanMoveToAsync(A._, Status.Draft, Status.Archived, ApiContext.UserPrincipal)) + A.CallTo(() => contentWorkflow.CanMoveTo(A._, Status.Draft, Status.Archived, ApiContext.UserPrincipal)) .Returns(true); var actual = await PublishAsync(command); @@ -633,7 +638,7 @@ public class ContentDomainObjectTests : HandlerTestBase var command = new ChangeContentStatus { Status = Status.Published, StatusJobId = sut.Snapshot.ScheduleJob!.Id }; - A.CallTo(() => contentWorkflow.CanMoveToAsync(A._, Status.Draft, Status.Published, ApiContext.UserPrincipal)) + A.CallTo(() => contentWorkflow.CanMoveTo(A._, Status.Draft, Status.Published, ApiContext.UserPrincipal)) .Returns(false); var actual = await PublishAsync(command); diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/Guards/GuardContentTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/Guards/GuardContentTests.cs index 49e85ad9e..a7566fca4 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/Guards/GuardContentTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DomainObject/Guards/GuardContentTests.cs @@ -7,6 +7,7 @@ using System.Security.Claims; using Microsoft.Extensions.DependencyInjection; +using Squidex.Domain.Apps.Core.Apps; using Squidex.Domain.Apps.Core.Contents; using Squidex.Domain.Apps.Core.Schemas; using Squidex.Domain.Apps.Core.TestHelpers; @@ -24,6 +25,7 @@ namespace Squidex.Domain.Apps.Entities.Contents.DomainObject.Guards; public class GuardContentTests : GivenContext, IClassFixture { private readonly IContentWorkflow contentWorkflow = A.Fake(); + private readonly IContentWorkflows contentWorkflows = A.Fake(); private readonly IContentRepository contentRepository = A.Fake(); private readonly Schema normalSchema; private readonly Schema normalUnpublishedSchema; @@ -33,6 +35,9 @@ public class GuardContentTests : GivenContext, IClassFixture contentWorkflows.GetWorkflowAsync(A._, A._, A._)) + .Returns(contentWorkflow); + normalUnpublishedSchema = Schema.Unpublish(); @@ -214,7 +219,7 @@ public class GuardContentTests : GivenContext, IClassFixture contentWorkflow.GetInitialStatusAsync(operation.Schema)) + A.CallTo(() => contentWorkflow.GetInitialStatus()) .Returns(Status.Archived); Assert.Equal(Status.Archived, await operation.GetInitialStatusAsync()); @@ -225,7 +230,7 @@ public class GuardContentTests : GivenContext, IClassFixture contentWorkflow.CanUpdateAsync(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, operation.User)) + A.CallTo(() => contentWorkflow.CanUpdate(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, operation.User)) .Returns(false); await Assert.ThrowsAsync(() => operation.CheckUpdateAsync()); @@ -236,7 +241,7 @@ public class GuardContentTests : GivenContext, IClassFixture contentWorkflow.CanUpdateAsync(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, operation.User)) + A.CallTo(() => contentWorkflow.CanUpdate(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, operation.User)) .Returns(true); await operation.CheckUpdateAsync(); @@ -247,8 +252,8 @@ public class GuardContentTests : GivenContext, IClassFixture contentWorkflow.GetInfoAsync(operation.Snapshot.ToContent(), Status.Archived)) - .Returns(ValueTask.FromResult(null)); + A.CallTo(() => contentWorkflow.GetInfo(Status.Archived)) + .Returns(null); await Assert.ThrowsAsync(() => operation.CheckStatusAsync(Status.Archived)); } @@ -258,7 +263,7 @@ public class GuardContentTests : GivenContext, IClassFixture contentWorkflow.GetInfoAsync(operation.Snapshot.ToContent(), Status.Archived)) + A.CallTo(() => contentWorkflow.GetInfo(Status.Archived)) .Returns(new StatusInfo(Status.Archived, StatusColors.Archived)); await operation.CheckStatusAsync(Status.Archived); @@ -271,7 +276,7 @@ public class GuardContentTests : GivenContext, IClassFixture contentWorkflow.GetInfoAsync(operation.Snapshot.ToContent(), Status.Archived)) + A.CallTo(() => contentWorkflow.GetInfo(Status.Archived)) .MustNotHaveHappened(); } @@ -280,7 +285,7 @@ public class GuardContentTests : GivenContext, IClassFixture contentWorkflow.CanMoveToAsync(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, Status.Archived, operation.User)) + A.CallTo(() => contentWorkflow.CanMoveTo(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, Status.Archived, operation.User)) .Returns(false); await Assert.ThrowsAsync(() => operation.CheckTransitionAsync(Status.Archived)); @@ -291,7 +296,7 @@ public class GuardContentTests : GivenContext, IClassFixture contentWorkflow.CanMoveToAsync(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, Status.Archived, operation.User)) + A.CallTo(() => contentWorkflow.CanMoveTo(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, Status.Archived, operation.User)) .Returns(true); await operation.CheckTransitionAsync(Status.Archived); @@ -304,7 +309,7 @@ public class GuardContentTests : GivenContext, IClassFixture contentWorkflow.CanMoveToAsync(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, A._, A._)) + A.CallTo(() => contentWorkflow.CanMoveTo(operation.Snapshot.ToContent(), operation.Snapshot.EditingStatus, A._, A._)) .MustNotHaveHappened(); } @@ -375,7 +380,7 @@ public class GuardContentTests : GivenContext, IClassFixture content) diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DynamicContentWorkflowTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DynamicContentWorkflowTests.cs index 62cbf8baa..9dce7964f 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DynamicContentWorkflowTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/DynamicContentWorkflowTests.cs @@ -1,4 +1,4 @@ -// ========================================================================== +// ========================================================================== // Squidex Headless CMS // ========================================================================== // Copyright (c) Squidex UG (haftungsbeschraenkt) @@ -7,6 +7,7 @@ using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Options; +using Squidex.Caching; using Squidex.Domain.Apps.Core.Apps; using Squidex.Domain.Apps.Core.Contents; using Squidex.Domain.Apps.Core.Schemas; @@ -20,7 +21,7 @@ namespace Squidex.Domain.Apps.Entities.Contents; public class DynamicContentWorkflowTests : GivenContext { private readonly DomainId simpleSchemaId = DomainId.NewGuid(); - private readonly DynamicContentWorkflow sut; + private readonly DynamicContentWorkflows sut; private readonly Workflow workflow = new Workflow( Status.Draft, @@ -86,15 +87,15 @@ public class DynamicContentWorkflowTests : GivenContext TimeoutExecution = TimeSpan.FromSeconds(10), })); - sut = new DynamicContentWorkflow(scriptEngine, AppProvider); + sut = new DynamicContentWorkflows(scriptEngine, new AsyncLocalCache()); } [Fact] public async Task Should_return_info_for_valid_status() { - var content = CreateContent(Status.Draft, 2); + using var sutWorkflow = await GetWorkflowAsync(); - var info = await sut.GetInfoAsync(content, Status.Draft); + var info = sutWorkflow.GetInfo(Status.Draft); Assert.Equal(new StatusInfo(Status.Draft, StatusColors.Draft), info); } @@ -102,9 +103,9 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_return_info_as_null_for_invalid_status() { - var content = CreateContent(Status.Draft, 2); + using var sutWorkflow = await GetWorkflowAsync(); - var info = await sut.GetInfoAsync(content, new Status("Invalid")); + var info = sutWorkflow.GetInfo(new Status("Invalid")); Assert.Null(info); } @@ -112,7 +113,9 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_return_draft_as_initial_status() { - var actual = await sut.GetInitialStatusAsync(Schema); + using var sutWorkflow = await GetWorkflowAsync(); + + var actual = sutWorkflow.GetInitialStatus(); Assert.Equal(Status.Draft, actual); } @@ -120,7 +123,9 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_allow_publish_on_create() { - var actual = await sut.CanPublishInitialAsync(Schema, Mocks.FrontendUser(Role.Editor)); + using var sutWorkflow = await GetWorkflowAsync(); + + var actual = sutWorkflow.CanPublishInitial(Mocks.FrontendUser(Role.Editor)); Assert.True(actual); } @@ -128,7 +133,9 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_not_allow_publish_on_create_if_role_not_allowed() { - var actual = await sut.CanPublishInitialAsync(Schema, Mocks.FrontendUser(Role.Developer)); + using var sutWorkflow = await GetWorkflowAsync(); + + var actual = sutWorkflow.CanPublishInitial(Mocks.FrontendUser(Role.Developer)); Assert.False(actual); } @@ -136,19 +143,11 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_allow_if_transition_is_valid() { - var content = CreateContent(Status.Draft, 2); + using var sutWorkflow = await GetWorkflowAsync(); - var actual = await sut.CanMoveToAsync(content, content.Status, Status.Published, Mocks.FrontendUser(Role.Editor)); - - Assert.True(actual); - } - - [Fact] - public async Task Should_allow_if_transition_is_valid_for_content() - { var content = CreateContent(Status.Draft, 2); - var actual = await sut.CanMoveToAsync(content, content.Status, Status.Published, Mocks.FrontendUser(Role.Editor)); + var actual = sutWorkflow.CanMoveTo(content, content.Status, Status.Published, Mocks.FrontendUser(Role.Editor)); Assert.True(actual); } @@ -156,49 +155,50 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_not_allow_transition_if_role_is_not_allowed() { - var content = CreateContent(Status.Draft, 2); - - var actual = await sut.CanMoveToAsync(content, content.Status, Status.Published, Mocks.FrontendUser(Role.Developer)); + using var sutWorkflow = await GetWorkflowAsync(); - Assert.False(actual); - } - - [Fact] - public async Task Should_allow_transition_if_role_is_allowed() - { var content = CreateContent(Status.Draft, 2); - var actual = await sut.CanMoveToAsync(content, content.Status, Status.Published, Mocks.FrontendUser(Role.Editor)); + var actual = sutWorkflow.CanMoveTo(content, content.Status, Status.Published, Mocks.FrontendUser(Role.Developer)); - Assert.True(actual); + Assert.False(actual); } [Fact] public async Task Should_not_allow_transition_if_data_not_valid() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Draft, 4); - var actual = await sut.CanMoveToAsync(content, content.Status, Status.Published, Mocks.FrontendUser(Role.Editor)); + var actual = sutWorkflow.CanMoveTo(content, content.Status, Status.Published, Mocks.FrontendUser(Role.Editor)); Assert.False(actual); } [Fact] - public async Task Should_be_able_to_update_published() + public async Task Should_evaluate_reused_expression_per_content() { - var content = CreateContent(Status.Published, 2); + using var sutWorkflow = await GetWorkflowAsync(); - var actual = await sut.CanUpdateAsync(content, content.Status, Mocks.FrontendUser(Role.Developer)); + var content1 = CreateContent(Status.Draft, 2); + var content2 = CreateContent(Status.Draft, 4); - Assert.True(actual); + var user = Mocks.FrontendUser(Role.Editor); + + Assert.True(sutWorkflow.CanMoveTo(content1, content1.Status, Status.Published, user)); + Assert.False(sutWorkflow.CanMoveTo(content2, content2.Status, Status.Published, user)); + Assert.True(sutWorkflow.CanMoveTo(content1, content1.Status, Status.Published, user)); } [Fact] - public async Task Should_be_able_to_update_draft() + public async Task Should_be_able_to_update_published() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Published, 2); - var actual = await sut.CanUpdateAsync(content, content.Status, Mocks.FrontendUser(Role.Developer)); + var actual = sutWorkflow.CanUpdate(content, content.Status, Mocks.FrontendUser(Role.Developer)); Assert.True(actual); } @@ -206,9 +206,11 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_not_be_able_to_update_archived() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Archived, 2); - var actual = await sut.CanUpdateAsync(content, content.Status, Mocks.FrontendUser(Role.Developer)); + var actual = sutWorkflow.CanUpdate(content, content.Status, Mocks.FrontendUser(Role.Developer)); Assert.False(actual); } @@ -216,9 +218,11 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_not_be_able_to_update_published_with_true_expression() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Published, 2); - var actual = await sut.CanUpdateAsync(content, content.Status, Mocks.FrontendUser(Role.Owner)); + var actual = sutWorkflow.CanUpdate(content, content.Status, Mocks.FrontendUser(Role.Owner)); Assert.False(actual); } @@ -226,9 +230,11 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_be_able_to_update_published_with_false_expression() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Published, 1); - var actual = await sut.CanUpdateAsync(content, content.Status, Mocks.FrontendUser(Role.Owner)); + var actual = sutWorkflow.CanUpdate(content, content.Status, Mocks.FrontendUser(Role.Owner)); Assert.True(actual); } @@ -236,9 +242,11 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_not_be_able_to_update_published_with_correct_roles() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Published, 2); - var actual = await sut.CanUpdateAsync(content, content.Status, Mocks.FrontendUser(Role.Editor)); + var actual = sutWorkflow.CanUpdate(content, content.Status, Mocks.FrontendUser(Role.Editor)); Assert.False(actual); } @@ -246,9 +254,11 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_be_able_to_update_published_with_incorrect_roles() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Published, 1); - var actual = await sut.CanUpdateAsync(content, content.Status, Mocks.FrontendUser(Role.Owner)); + var actual = sutWorkflow.CanUpdate(content, content.Status, Mocks.FrontendUser(Role.Owner)); Assert.True(actual); } @@ -256,6 +266,8 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_get_next_statuses_for_draft() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Draft, 2); var expected = new[] @@ -263,7 +275,7 @@ public class DynamicContentWorkflowTests : GivenContext new StatusInfo(Status.Archived, StatusColors.Archived), }; - var actual = await sut.GetNextAsync(content, content.Status, Mocks.FrontendUser(Role.Developer)); + var actual = sutWorkflow.GetNext(content, content.Status, Mocks.FrontendUser(Role.Developer)); actual.Should().BeEquivalentTo(expected); } @@ -271,6 +283,8 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_limit_next_statuses_if_expression_does_not_evauate_to_true() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Draft, 4); var expected = new[] @@ -278,7 +292,7 @@ public class DynamicContentWorkflowTests : GivenContext new StatusInfo(Status.Archived, StatusColors.Archived), }; - var actual = await sut.GetNextAsync(content, content.Status, Mocks.FrontendUser(Role.Editor)); + var actual = sutWorkflow.GetNext(content, content.Status, Mocks.FrontendUser(Role.Editor)); actual.Should().BeEquivalentTo(expected); } @@ -286,6 +300,8 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_limit_next_statuses_if_role_is_not_allowed() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Draft, 2); var expected = new[] @@ -294,14 +310,38 @@ public class DynamicContentWorkflowTests : GivenContext new StatusInfo(Status.Published, StatusColors.Published), }; - var actual = await sut.GetNextAsync(content, content.Status, Mocks.FrontendUser(Role.Editor)); + var actual = sutWorkflow.GetNext(content, content.Status, Mocks.FrontendUser(Role.Editor)); actual.Should().BeEquivalentTo(expected); } + [Fact] + public async Task Should_not_reuse_next_statuses_of_conditional_step() + { + using var sutWorkflow = await GetWorkflowAsync(); + + var allowed = CreateContent(Status.Draft, 2); + var denied = CreateContent(Status.Draft, 4); + + var user = Mocks.FrontendUser(Role.Editor); + + sutWorkflow.GetNext(allowed, allowed.Status, user).Should().BeEquivalentTo(new[] + { + new StatusInfo(Status.Archived, StatusColors.Archived), + new StatusInfo(Status.Published, StatusColors.Published), + }); + + sutWorkflow.GetNext(denied, denied.Status, user).Should().BeEquivalentTo(new[] + { + new StatusInfo(Status.Archived, StatusColors.Archived), + }); + } + [Fact] public async Task Should_get_next_statuses_for_archived() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Archived, 2); var expected = new[] @@ -309,7 +349,7 @@ public class DynamicContentWorkflowTests : GivenContext new StatusInfo(Status.Draft, StatusColors.Draft), }; - var actual = await sut.GetNextAsync(content, content.Status, null!); + var actual = sutWorkflow.GetNext(content, content.Status, null!); actual.Should().BeEquivalentTo(expected); } @@ -317,6 +357,8 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_get_next_statuses_for_published() { + using var sutWorkflow = await GetWorkflowAsync(); + var content = CreateContent(Status.Published, 2); var expected = new[] @@ -325,7 +367,7 @@ public class DynamicContentWorkflowTests : GivenContext new StatusInfo(Status.Draft, StatusColors.Draft), }; - var actual = await sut.GetNextAsync(content, content.Status, null!); + var actual = sutWorkflow.GetNext(content, content.Status, null!); actual.Should().BeEquivalentTo(expected); } @@ -333,6 +375,8 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_return_all_statuses() { + using var sutWorkflow = await GetWorkflowAsync(); + var expected = new[] { new StatusInfo(Status.Archived, StatusColors.Archived), @@ -340,7 +384,7 @@ public class DynamicContentWorkflowTests : GivenContext new StatusInfo(Status.Published, StatusColors.Published), }; - var actual = await sut.GetAllAsync(Schema); + var actual = sutWorkflow.GetAll(); actual.Should().BeEquivalentTo(expected); } @@ -348,13 +392,15 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_return_all_statuses_for_simple_schema_workflow() { + using var sutWorkflow = await GetWorkflowAsync(Schema.WithId(simpleSchemaId, "simple-schema")); + var expected = new[] { new StatusInfo(Status.Draft, StatusColors.Draft), new StatusInfo(Status.Published, StatusColors.Published), }; - var actual = await sut.GetAllAsync(Schema.WithId(simpleSchemaId, "simple-schema")); + var actual = sutWorkflow.GetAll(); actual.Should().BeEquivalentTo(expected); } @@ -367,6 +413,8 @@ public class DynamicContentWorkflowTests : GivenContext Workflows = Workflows.Empty, }; + using var sutWorkflow = await GetWorkflowAsync(); + var expected = new[] { new StatusInfo(Status.Archived, StatusColors.Archived), @@ -374,7 +422,7 @@ public class DynamicContentWorkflowTests : GivenContext new StatusInfo(Status.Published, StatusColors.Published), }; - var actual = await sut.GetAllAsync(Schema); + var actual = sutWorkflow.GetAll(); actual.Should().BeEquivalentTo(expected); } @@ -382,7 +430,9 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_not_validate_when_not_publishing() { - var actual = await sut.ShouldValidateAsync(Schema, Status.Draft); + using var sutWorkflow = await GetWorkflowAsync(); + + var actual = sutWorkflow.ShouldValidate(Status.Draft); Assert.False(actual); } @@ -390,12 +440,12 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_not_validate_when_publishing_but_not_enabled() { - var schema = Schema with + using var sutWorkflow = await GetWorkflowAsync(Schema with { Properties = new SchemaProperties { ValidateOnPublish = false }, - }; + }); - var actual = await sut.ShouldValidateAsync(schema, Status.Published); + var actual = sutWorkflow.ShouldValidate(Status.Published); Assert.False(actual); } @@ -403,12 +453,12 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_validate_when_publishing_and_enabled() { - var schema = Schema with + using var sutWorkflow = await GetWorkflowAsync(Schema with { Properties = new SchemaProperties { ValidateOnPublish = true }, - }; + }); - var actual = await sut.ShouldValidateAsync(schema, Status.Published); + var actual = sutWorkflow.ShouldValidate(Status.Published); Assert.True(actual); } @@ -416,24 +466,21 @@ public class DynamicContentWorkflowTests : GivenContext [Fact] public async Task Should_validate_when_enabled_in_step() { - var actual = await sut.ShouldValidateAsync(Schema, Status.Archived); + using var sutWorkflow = await GetWorkflowAsync(); + + var actual = sutWorkflow.ShouldValidate(Status.Archived); Assert.True(actual); } - private EnrichedContent CreateContent(Status status, int value, bool simple = false) + private ValueTask GetWorkflowAsync(Schema? schema = null) { - var content = CreateContent(); - - if (simple) - { - content = content with - { - SchemaId = NamedId.Of(simpleSchemaId, "my-simple-schema"), - }; - } + return sut.GetWorkflowAsync(App, schema ?? Schema, CancellationToken); + } - content = content with + private EnrichedContent CreateContent(Status status, int value) + { + return CreateContent() with { Status = status, Data = @@ -442,7 +489,5 @@ public class DynamicContentWorkflowTests : GivenContext new ContentFieldData() .AddInvariant(value)), }; - - return content; } } diff --git a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/Queries/EnrichWithWorkflowsTests.cs b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/Queries/EnrichWithWorkflowsTests.cs index 79f505406..4a3ae93d1 100644 --- a/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/Queries/EnrichWithWorkflowsTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Entities.Tests/Contents/Queries/EnrichWithWorkflowsTests.cs @@ -1,4 +1,4 @@ -// ========================================================================== +// ========================================================================== // Squidex Headless CMS // ========================================================================== // Copyright (c) Squidex UG (haftungsbeschraenkt) @@ -6,6 +6,7 @@ // ========================================================================== using Squidex.Domain.Apps.Core.Contents; +using Squidex.Domain.Apps.Core.Schemas; using Squidex.Domain.Apps.Entities.Contents.Queries.Steps; using Squidex.Domain.Apps.Entities.TestHelpers; @@ -16,11 +17,15 @@ namespace Squidex.Domain.Apps.Entities.Contents.Queries; public class EnrichWithWorkflowsTests : GivenContext { private readonly IContentWorkflow workflow = A.Fake(); + private readonly IContentWorkflows workflows = A.Fake(); private readonly EnrichWithWorkflows sut; public EnrichWithWorkflowsTests() { - sut = new EnrichWithWorkflows(workflow); + A.CallTo(() => workflows.GetWorkflowAsync(App, Schema, A._)) + .Returns(workflow); + + sut = new EnrichWithWorkflows(workflows); } [Fact] @@ -33,10 +38,10 @@ public class EnrichWithWorkflowsTests : GivenContext new StatusInfo(Status.Published, StatusColors.Published), }; - A.CallTo(() => workflow.GetNextAsync(content, content.Status, FrontendContext.UserPrincipal)) + A.CallTo(() => workflow.GetNext(content, content.Status, FrontendContext.UserPrincipal)) .Returns(nexts); - await sut.EnrichAsync(FrontendContext, [content], null!, CancellationToken); + await sut.EnrichAsync(FrontendContext, [content], SchemaProvider(), CancellationToken); Assert.Equal(nexts, content.NextStatuses); } @@ -46,11 +51,11 @@ public class EnrichWithWorkflowsTests : GivenContext { var content = CreateContent() with { IsSingleton = true, Status = Status.Draft }; - await sut.EnrichAsync(FrontendContext, [content], null!, default); + await sut.EnrichAsync(FrontendContext, [content], SchemaProvider(), default); Assert.Equal(Status.Published, content.NextStatuses?.Single().Status); - A.CallTo(() => workflow.GetNextAsync(content, A._, FrontendContext.UserPrincipal)) + A.CallTo(() => workflow.GetNext(content, A._, FrontendContext.UserPrincipal)) .MustNotHaveHappened(); } @@ -59,11 +64,11 @@ public class EnrichWithWorkflowsTests : GivenContext { var content = CreateContent() with { IsSingleton = true }; - await sut.EnrichAsync(FrontendContext, [content], null!, CancellationToken); + await sut.EnrichAsync(FrontendContext, [content], SchemaProvider(), CancellationToken); Assert.Empty(content.NextStatuses!); - A.CallTo(() => workflow.GetNextAsync(content, A._, FrontendContext.UserPrincipal)) + A.CallTo(() => workflow.GetNext(content, A._, FrontendContext.UserPrincipal)) .MustNotHaveHappened(); } @@ -72,10 +77,10 @@ public class EnrichWithWorkflowsTests : GivenContext { var content = CreateContent(); - A.CallTo(() => workflow.GetInfoAsync(content, content.Status)) + A.CallTo(() => workflow.GetInfo(content.Status)) .Returns(new StatusInfo(Status.Published, StatusColors.Published)); - await sut.EnrichAsync(FrontendContext, [content], null!, CancellationToken); + await sut.EnrichAsync(FrontendContext, [content], SchemaProvider(), CancellationToken); Assert.Equal(StatusColors.Published, content.StatusColor); } @@ -85,10 +90,10 @@ public class EnrichWithWorkflowsTests : GivenContext { var content = CreateContent() with { NewStatus = Status.Archived }; - A.CallTo(() => workflow.GetInfoAsync(content, content.NewStatus!.Value)) + A.CallTo(() => workflow.GetInfo(content.NewStatus!.Value)) .Returns(new StatusInfo(Status.Published, StatusColors.Archived)); - await sut.EnrichAsync(FrontendContext, [content], null!, CancellationToken); + await sut.EnrichAsync(FrontendContext, [content], SchemaProvider(), CancellationToken); Assert.Equal(StatusColors.Archived, content.NewStatusColor); } @@ -98,10 +103,10 @@ public class EnrichWithWorkflowsTests : GivenContext { var content = CreateContent() with { ScheduleJob = ScheduleJob.Build(Status.Archived, User, Timestamp()) }; - A.CallTo(() => workflow.GetInfoAsync(content, content.ScheduleJob.Status)) + A.CallTo(() => workflow.GetInfo(content.ScheduleJob.Status)) .Returns(new StatusInfo(Status.Published, StatusColors.Archived)); - await sut.EnrichAsync(FrontendContext, [content], null!, CancellationToken); + await sut.EnrichAsync(FrontendContext, [content], SchemaProvider(), CancellationToken); Assert.Equal(StatusColors.Archived, content.ScheduledStatusColor); } @@ -111,10 +116,10 @@ public class EnrichWithWorkflowsTests : GivenContext { var content = CreateContent(); - A.CallTo(() => workflow.GetInfoAsync(content, content.Status)) - .Returns(ValueTask.FromResult(null!)); + A.CallTo(() => workflow.GetInfo(content.Status)) + .Returns(null); - await sut.EnrichAsync(FrontendContext, [content], null!, CancellationToken); + await sut.EnrichAsync(FrontendContext, [content], SchemaProvider(), CancellationToken); Assert.Equal(StatusColors.Draft, content.StatusColor); } @@ -124,10 +129,10 @@ public class EnrichWithWorkflowsTests : GivenContext { var content = CreateContent(); - A.CallTo(() => workflow.CanUpdateAsync(content, content.Status, FrontendContext.UserPrincipal)) + A.CallTo(() => workflow.CanUpdate(content, content.Status, FrontendContext.UserPrincipal)) .Returns(true); - await sut.EnrichAsync(FrontendContext, [content], null!, CancellationToken); + await sut.EnrichAsync(FrontendContext, [content], SchemaProvider(), CancellationToken); Assert.True(content.CanUpdate); } @@ -137,11 +142,28 @@ public class EnrichWithWorkflowsTests : GivenContext { var content = CreateContent(); - await sut.EnrichAsync(ApiContext.Clone(b => b.WithResolveFlow(false)), [content], null!, CancellationToken); + await sut.EnrichAsync(ApiContext.Clone(b => b.WithResolveFlow(false)), [content], SchemaProvider(), CancellationToken); Assert.False(content.CanUpdate); - A.CallTo(() => workflow.CanUpdateAsync(content, A._, FrontendContext.UserPrincipal)) + A.CallTo(() => workflow.CanUpdate(content, A._, FrontendContext.UserPrincipal)) .MustNotHaveHappened(); } + + [Fact] + public async Task Should_resolve_workflow_once_per_schema() + { + var content1 = CreateContent(); + var content2 = CreateContent(); + + await sut.EnrichAsync(FrontendContext, [content1, content2], SchemaProvider(), CancellationToken); + + A.CallTo(() => workflows.GetWorkflowAsync(App, Schema, A._)) + .MustHaveHappenedOnceExactly(); + } + + private ProvideSchema SchemaProvider() + { + return x => Task.FromResult((Schema, ResolvedComponents.Empty)); + } } diff --git a/resolved.md b/resolved.md deleted file mode 100644 index e06b77dd5..000000000 --- a/resolved.md +++ /dev/null @@ -1,1236 +0,0 @@ -# Backend Performance — Resolved - -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. - -Most entries are fixes. Items **6** and **14** are closed as *accepted*, **21** as *rejected* -and **23** as *bounded but not fixed* — kept here so they are not re-reported as new findings. -Item **18** records a finding that turned out to be wrong. - ---- - -### 4. Streaming export enriched contents one at a time — **FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/ContentQueryService.cs:49` - -**Was:** `StreamAsync` called `contentEnricher.EnrichAsync(content, ...)` per item. The -single-item overload wraps the content in `Enumerable.Repeat(content, 1)` and runs the -whole pipeline for it — a new result `List`, a new schema-cache `Dictionary`, and every -`IContentEnricherStep` twice. Every batching optimisation in `ResolveReferences`, -`ResolveAssets` and `ConvertData` was defeated, so reference resolution degenerated to -one DB round trip per content. A 100k-content export meant 100k pipeline setups. - -**Now:** - -```csharp -await foreach (var batch in contents.Batch(50, ct).WithCancellation(ct)) -{ - var enriched = await contentEnricher.EnrichAsync(batch, context, ct); - foreach (var content in enriched) - { - yield return content; - } -} -``` - -`Batch` yields `List`, which binds to the `IEnumerable` overload, and that -overload calls `EnrichInternalAsync(contents, cloneData: false, ...)` — matching the -previous single-item behaviour. Reference resolution now amortises across 50 contents -instead of one DB round trip each. - -**Follow-up:** 50 is conservative next to the 200-item batches used elsewhere -(`RuleEnqueuer.BatchSize`). Once profiled, a larger batch would amortise further. - ---- - -### 5. `WriteManyAsync` iterated the unfiltered job list — **FIXED** -`backend/src/Squidex.Data.MongoDb/Domain/Apps/Entities/Contents/MongoContentRepository_SnapshotStore.cs:138` - -**Was:** the method built `validJobs` via `jobs.Where(x => IsValid(x.Value)).ToList()` -and then looped over `jobs`. Two defects in one — the corrupt-data guard was bypassed -(the comment above it notes the data "might throw an exception if we do not ignore it"), -and the sequence was enumerated twice, re-running any upstream projection. - -**Now:** `foreach (var job in jobs)` → `foreach (var job in validJobs)`. - ---- - -### 7. Regex rebuilt per content write — **FIXED (the expensive part)** -`backend/src/Squidex.Domain.Apps.Core.Operations/ValidateContent/Validators/PatternValidator.cs` - -**Was:** every content write constructs a fresh `ContentValidator` and with it a whole -validator object graph. For each pattern field that included -`new Regex($"^{pattern}$", options, Timeout)` in the constructor — a full pattern parse -and interpreter build. A 10k-item import with 5 pattern fields did 50k pattern parses. - -**Now:** `PatternValidator` resolves its `Regex` from a process-wide, 1000-entry -`Squidex.Caching.LRUCache<(string Pattern, RegexOptions Options), Regex>`. The same -import does 5 parses. - -**Why a plain static cache and not an async-local / request-scoped one.** The cacheable -unit turned out to be *only* the `Regex`, and a `Regex` has no dependency on the request -at all — it is a pure function of (pattern, options), and `Regex` instances are -thread-safe for matching. So a process-wide cache is both simpler and strictly more -effective than a request-scoped one, which would rebuild each pattern once per request. - -**Why the validator tree itself is still rebuilt per write.** Caching the graph — even -per request — is not safe. It captures per-item state at several levels: - -| Captured state | Where | -| --- | --- | -| `context.Root.PreviousData` | `DefaultValidatorsFactory` → `NotChangedValidator` | -| `context.Action` (Publish vs not) | `IsRequired` in both factories — changes *which* validators are emitted | -| `context.Mode` (Optimized) | `DependencyValidatorsFactory` short-circuits entirely | -| `context.Root.App` / `.Schema` | closures in `CheckAssets` / `CheckContentsByIds` / `CheckUniqueness` | - -A bulk import is a single request but each item carries its own `PreviousData` and -`CommandId`, so even an `ILocalCache` keyed by schema would hand back a graph wired to -the previous item. The remaining per-write cost is a few hundred small gen-0 allocations -(dictionaries and `AggregateValidator` arrays) — real, but an order of magnitude below -the pattern parse that was removed. Reworking the factories to split -"schema-shaped, cacheable" from "context-bound" validators is the follow-up if profiling -says the churn still matters. - -`RegexOptions.Compiled` was deliberately *not* added: it moves cost into IL emit and the -generated code can never be unloaded, which is a bad trade for user-authored patterns. - -**The cache access is locked, and has to be.** `LRUCache` is a plain `Dictionary` plus a -`LinkedList` with no synchronisation, and its `TryGetValue` *mutates* the recency list — -so there is no lock-free read path. Verified empirically against the shipped -`Squidex.Caching` 8.0.3 assembly: 8 threads hammering an unguarded instance produced -`InvalidOperationException: The LinkedList node does not belong to current LinkedList`, -`ArgumentException: An item with the same key has already been added`, and repeated -`NullReferenceException`s. (The assembly *does* reference `Monitor`, but from other types -in the package — not `LRUCache`.) Validators are constructed concurrently on every -content write, so this path is genuinely contended. - -`new Regex(...)` is built *outside* the lock, so pattern parsing is never serialised -across threads; a cold race can build the same pattern twice, which only wastes a little -work and never returns anything incorrect. The critical section is just the dictionary -and linked-list updates. - -`MemoryCache` would remove the lock, but `PatternValidator` is constructed without -dependency injection, so it would have to create and hold its own cache instance. The lock -is the smaller change and it is already covered by the concurrency harness below. - -**Verified:** `dotnet build` clean (0 warnings); a harness mirroring `GetRegex` ran 1.6M -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. - ---- - -### 6. Sync-over-async on the authentication path — **CLOSED: ACCEPTED, WON'T FIX** -`backend/src/Squidex/Areas/IdentityServer/Config/Dynamic/DynamicSchemeProvider.cs:129` - -```csharp -var scheme = GetSchemeCoreAsync(name, default).Result; -``` - -`Get(string? name)` blocks a thread-pool thread on a DB round trip, which in a hot path -is a classic thread-pool starvation source. - -**Closed as accepted, not fixed.** This is dynamic OIDC scheme resolution — reached only -for team-level auth domains, not on ordinary API traffic — so the risk does not justify -the rework. Recorded here rather than deleted so it is not re-reported as a new finding. - -If it ever moves 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 accepted: -- `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()`. - ---- - -### 12. `ReaderWriterLockSlim` used exclusively for write locks in the ETag path — **FIXED** -`backend/src/Squidex.Web/Pipeline/CachingManager.cs` - -**Was:** `CacheContext` guarded `AddDependency`, `AddDependency`, `AddHeader` and -`Finish` with `ReaderWriterLockSlim` — but every one of them took `EnterWriteLock`. No -code path ever took a read lock, so the reader/writer bookkeeping was pure overhead at -roughly 2–3× the cost of a plain monitor. `AddDependency` is called once per content, -once per schema and once per resolved reference, so a 200-item list with references took -on the order of a thousand write-lock round trips per request. - -**Now:** a plain `Lock` (`System.Threading.Lock`, matching `DisposableObjectBase`), with -each `EnterWriteLock`/`try`/`finally`/`ExitWriteLock` block collapsed to `lock (...)`. - -Two incidental improvements fell out of the rewrite: - -- `Dispose()` no longer has a lock to dispose, so `CacheContext` only disposes the hasher. -- `AddHeader` had its `EnterWriteLock` *inside* the `try`, so a throw from the acquire - would have hit `ExitWriteLock` on an unheld lock and masked the original error with a - `SynchronizationLockException`. `lock` cannot express that shape. - -Nothing about the concurrency contract changed — every operation mutates the hasher and -the sets, so there was never anything a read lock could have protected. - -**Verified:** build clean, `Squidex.Web.Tests` green (167). - ---- - -### 13. Rules dictionary rebuilt per event inside the batch loop — **FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Rules/RuleEnqueuer.cs` - -**Was:** `On(...)` receives batches of 200 events and ran -`Rules = rules.ToReadonlyDictionary(x => x.Id)` for *each* one — a full `Dictionary` -build plus a wrapper allocation per event, even though the events in a batch are -overwhelmingly from the same app. - -Note the rules *lookup* was already cheap: `RulesCacheDuration` defaults to 10s, so -`appProvider.GetRulesAsync` was memoized. The waste was purely the per-event indexing. - -**Now:** the batch is grouped by app, so rules are resolved and indexed once per app and -the context is built once per group: - -```csharp -foreach (var byApp in events.GroupBy(GetAppId)) -{ - if (byApp.Key == null) { continue; } - - var rules = await GetRulesAsync(byApp.Key.Id); - if (rules.Count == 0) { continue; } - - var context = new RulesContext { AppId = byApp.Key, Rules = rules.ToReadonlyDictionary(x => x.Id), ... }; - - foreach (var @event in byApp) { ... } -} -``` - -`GetAppId` returns `null` for restored events and non-`AppEvent` payloads, so they all -collect into one group that is skipped — replacing the two per-event `continue` guards. - -**Why `GroupBy` rather than memoizing per app inside the original loop.** The first -attempt kept the original per-event loop and cached the indexed dictionary in a -`Dictionary`, specifically to avoid reordering events. `GroupBy` does -reorder across apps, so that had to be checked rather than assumed: - -- Rules are scoped to a single app (`context.AppId`, `context.Rules`), so a rule cannot - observe another app's events. -- `RuleQueueWriter` is app-agnostic — it accumulates `CreateFlowInstanceRequest` values - and flushes every 100 regardless of origin. -- `ruleUsageTracker.TrackAsync` is an additive counter per (app, rule, day). -- `GroupBy` preserves source order *within* each group, which is the ordering that can - actually matter. - -Nothing cross-app is order-sensitive, so `GroupBy` is safe — and it is both simpler and -slightly more correct than the memo: keying on `NamedId` (a `sealed record`, -so value equality over id *and* name) means an app renamed mid-batch yields two groups -each carrying its own correct name, where the memo keyed on `.Id` would have reused the -first name seen. - -**Verified:** a new test, -`RuleEnqueuerTests.Should_handle_events_of_multiple_apps_with_the_rules_of_each_app`, -feeds an interleaved two-app batch and asserts each event is handled with its own app's -rules, that the grouped order is what reaches the service, and — via `Assert.Same` on the -`Rules` instance — that indexing happens once per app rather than once per event. It -**fails on the pre-fix code** (the ordering assertion shows `app1, app2, app1, app2` -against the expected `app1, app1, app2, app2`) and passes after. Existing coverage did -not include a multi-app batch at all: `Should_handle_events_in_batches` repeats the *same* -event ten times. Full `Squidex.Domain.Apps.Entities.Tests` green (1528). - ---- - -### 20. Script cache key embedded the entire script source — **FIXED** -`backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/CacheParser.cs:20` - -**Was:** - -```csharp -var cacheKey = $"{typeof(CacheParser)}_Script_{script}"; -``` - -Every parse allocated a new string holding a full copy of the script body, and -`IMemoryCache` then retained that copy as the key — so each cached script was held twice. - -**Now:** `var cacheKey = (typeof(CacheParser), script);` - -The tuple boxes once (one small allocation) but holds a *reference* to the existing -script string, so nothing is copied and the cache no longer keeps a second copy alive. - -**Honest limit:** this removes the allocation and the duplicate retention, not the hash. -`ValueTuple.GetHashCode` still calls `string.GetHashCode()` on the source, which is O(n) -— .NET does not cache string hash codes. Removing that too would mean keying by schema id -+ script version, which needs that context plumbed into `CacheParser` and changes its API. -Not worth it unless profiling says the hash itself shows up. - ---- - -### Tuple cache keys — sweep of the other call sites - -Same change applied where the key was an interpolated string and the cache accepts -`object`. Beyond skipping the string build, a tuple also avoids *formatting* non-string -parts (`DateOnly`, `long`), which the interpolation did on every call. - -| Site | Key before | Key now | -| --- | --- | --- | -| `CachingUsageTracker.GetForMonthAsync` | `$"{typeof(..)}_UsageForMonth_{key}_{date}_{category}"` | `(typeof(..), nameof(GetForMonthAsync), key, date, category)` | -| `CachingUsageTracker.GetAsync` | `$"{typeof(..)}_Usage_{key}_{fromDate}_{toDate}_{category}"` | `(typeof(..), nameof(GetAsync), key, fromDate, toDate, category)` | -| `EventEnricher.FindUserAsync` | `$"{typeof(..)}_Users_{actor.Identifier}"` | `(typeof(EventEnricher), actor.Identifier)` | -| `RuleEnqueuer.GetRulesAsync` | `$"{typeof(..)}_Rules_{appId}"` | `(typeof(RuleEnqueuer), appId)` | -| `UsageGate.CacheKey` | `$"{appId}_Plan"` | `(typeof(UsageGate), nameof(GetPlanForAppAsync), appId)` | -| `UsageGate` notified flag | bare `DomainId` | `(typeof(UsageGate), nameof(TrackNotified), appId)` | -| `CachingGraphQLResolver` | `$"GraphQLModel_{appId}_{etag}"` | `(typeof(CachingGraphQLResolver), app.Id, app.Version)` | -| `AppProvider` × 11 | `$"APPS_ID_{appId}"`, `$"GetSchemasAsync({appId})"`, … | `(nameof(AppProvider), "APPS_ID", appId)`, … | - -Notes: - -- `CachingUsageTracker.GetForMonthAsync` runs on **every API request** (via - `UsageGate.IsBlockedAsync`) and its old key formatted a `DateOnly` — a culture lookup - plus an allocation — before building an ~80-character string. -- `CachingGraphQLResolver` no longer needs - `app.Version.ToString(CultureInfo.InvariantCulture)`; the tuple carries the `long` - directly, so `System.Globalization` was dropped from the file. -- `UsageGate`'s notified flag previously used a bare `DomainId` as the key. It was safe - only because that `MemoryCache` is private to the class; it is now explicit. -- `AppProvider` keys carry `nameof(AppProvider)` plus the lookup name, preserving the - namespacing the old string prefixes provided. The two `TeamCacheKey` overloads and - `CachingGraphQLResolver.CreateCacheKey` had a single call site each and were inlined; - `AppCacheKey` and `SchemaCacheKey` have three each and stayed as helpers. - -**Three sites were deliberately left as strings:** - -- `MongoCountCollection.GetOrAddAsync(string key, …)` — used by `QueryByQuery` and - `MongoAssetRepository`. That key is **persisted as a MongoDB document id**, not an - in-memory cache key. Changing it would change stored data. -- `DataLoaderContext.GetOrAddLoader(string loaderKey, …)` — the GraphQL.DataLoader API - takes a `string`, so `GraphQLExecutionContext.GetContent` cannot use a tuple. -- `Singletons.GetOrAdd(string, …)` — typed `string`, and startup-only. - -**Verified:** build clean (0 warnings). `Squidex.Domain.Apps.Core.Tests` (1243), -`Squidex.Domain.Apps.Entities.Tests` (1528), `Squidex.Infrastructure.Tests` (1031) and -`Squidex.Web.Tests` (167) all green. - ---- - -### 14. `AppProvider` copies cached schema/rule lists on every call — **CLOSED: ACCEPTED** -`backend/src/Squidex.Domain.Apps.Entities/AppProvider.cs` - -`GetSchemasAsync` and `GetRulesAsync` end with `?.ToList() ?? []`, a defensive copy of the -cached list on every call including cache hits, and `GetRuleAsync` copies the whole rule -list just to `Find` one element. - -**Closed as accepted, not fixed.** The copy is a single shallow `List` allocation of -already-immutable elements; returning the cached instance directly would expose it to -mutation by callers, which is a worse trade than the allocation. Recorded here so it is -not re-reported as a new finding. - ---- - -### 15. Faulted tasks were cached permanently in `CollectionProvider` — **FIXED** -`backend/src/Squidex.Data.MongoDb/Domain/Apps/Entities/Contents/CollectionProvider.cs` - -**Was:** - -```csharp -return collections.GetOrAdd((appId, schemaId), CreateCollectionAsync); -``` - -Two defects. `CreateCollectionAsync` creates indexes, so it can fail transiently — and -`GetOrAdd` stored the returned `Task` including a *faulted* one for the process lifetime, -so a single Mongo hiccup on first access permanently broke queries for that app/schema -until restart. Separately, `GetOrAdd` may invoke its factory concurrently for the same -key, issuing duplicate `CreateManyAsync` calls. - -**Now:** the dictionary holds `Lazy>` with `LazyThreadSafetyMode.ExecutionAndPublication`, -so the factory runs exactly once per key even under concurrent access, and the entry is -evicted when it fails: - -```csharp -var collection = collections.GetOrAdd(key, CreateLazyCollection); - -return AwaitCollectionAsync(key, collection); -... -try -{ - return await collection.Value; -} -catch -{ - collections.TryRemove(new KeyValuePair<...>(key, collection)); - throw; -} -``` - -The removal uses the `TryRemove(KeyValuePair)` overload, which only removes when the value -is still the *same* `Lazy` instance. The plain `TryRemove(key)` would race: a second thread -that had already retried and succeeded would have its good entry discarded by the first -thread's cleanup. - -A `using` alias for the key tuple was tried first, but StyleCop's SA1008 rejects the space -before the parenthesis in `using X = (A, B);`, so the tuple type is written out instead. - -**Verified:** build clean, `Squidex.Data.Tests` (180) and all other suites green. - ---- - -### 16. `IsFrontendClient` re-scanned claims on every access — **FIXED (verified)** -`backend/src/Squidex.Domain.Apps.Entities/Context.cs:32,51` -`backend/src/Squidex.Infrastructure/Security/Extensions.cs:70` - -**Was:** `public bool IsFrontendClient => UserPrincipal.IsInClient(DefaultClients.Frontend);` -— a computed property whose implementation was `principal.Claims.Any(x => ...)`, walking -every identity and every claim and allocating an enumerator plus a delegate per call. It is -read from several enrichment steps and from `ConvertData.GenerateConverter` per schema -group, so it ran many times per request against a value that cannot change. - -**Now:** a get-only auto-property assigned once in the private constructor, and -`IsInClient` rewritten from LINQ `Any` to a plain `foreach`, dropping the closure. - -**Verification found the commit did not compile.** Line 32 read -`public bool IsFrontendClient { get; };` — a stray semicolon, `error CS1597: Semicolon -after method or accessor block is not valid`. Removed the semicolon. - -Beyond compiling, the assignment is correct for every construction path: the public -`Context(ClaimsPrincipal, App)` chains to the private constructor via `: this(...)`, -`Anonymous` and `Admin` both go through that public one, and `HeaderBuilder.Build` calls -the private 4-argument constructor directly. All four paths therefore set the field. - ---- - -### 17. `ResolvingReferences()` re-evaluated per content — **FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/ResolveReferences.cs` - -**Was:** `SchemaExtensions.ResolvingReferences` is a lazy -`Fields.OfType<...>().Where(...)` that is never materialized, and `AddReferenceIds` called -it *inside* the per-content loop — so the full field scan plus two LINQ iterator -allocations happened once per content instead of once per schema. - -**Now:** hoisted out of the loop. - -```csharp -var fields = schema.ResolvingReferences().ToList(); - -foreach (var content in contents) -{ - content.Data.AddReferencedIds(fields, ids, components); -} -``` - -(The other call site, the outer `foreach` in `ResolveReferencesAsync`, enumerates the -sequence exactly once and was left alone.) - -**The double `GroupBy` was deliberately left alone.** `ResolveReferences.EnrichAsync` and -`ConvertData` each build `contents.GroupBy(x => x.SchemaId.Id)` twice. This does *not* -cause duplicate schema fetches: `ContentEnricher` passes a `ProvideSchema` delegate backed -by a per-call `schemaCache` dictionary, so the second grouping resolves every schema from -memory. The only real cost is re-materializing the LINQ `Lookup` — one extra pass over the -contents and one set of bucket allocations per step. - -Deduplicating it was tried and reverted: the gain is small enough that it does not justify -threading a materialized `List>` through the method signatures. - -**Verified:** build clean, all suites green. - ---- - -### 10. Unbounded in-memory request-log queue — **FIXED** -`backend/src/Squidex.Infrastructure/Log/BackgroundRequestLogStore.cs` -`backend/src/Squidex.Infrastructure/Log/RequestLogStoreOptions.cs` - -**Was:** `jobs` was an unbounded `ConcurrentQueue`. `LogAsync` enqueues on every -API request while the flush timer drains only once per `WriteIntervall` (1s by default). -If `InsertManyAsync` threw — Mongo unreachable, disk full — the drain aborted and the -surviving entries stayed queued while new ones kept arriving. A sustained storage outage -under load grew the queue until the process ran out of memory: the request *log* taking -down the whole server. - -**Now:** a soft bound with an explicit drop counter. - -```csharp -if (Volatile.Read(ref jobsCount) >= options.MaxPendingItems) -{ - Interlocked.Increment(ref jobsDropped); - return Task.CompletedTask; -} - -Interlocked.Increment(ref jobsCount); - -jobs.Enqueue(request); -``` - -`jobsCount` is decremented as the drain dequeues, so the queue accepts entries again once -it has been written. Each drain reports what it dropped via a new -`LogRequestLogDropped` message, so the gap in the request log is visible rather than -silent. `MaxPendingItems` defaults to 50,000 — roughly 50 seconds of headroom at 1000 -requests/second — and is configurable. - -The bound is deliberately *soft*: two threads can both observe `jobsCount < max` and both -enqueue, so the queue can overshoot by the number of concurrent writers. That is fine for -a backpressure limit and avoids a lock on the hot path. - -A `Channel` with `BoundedChannelFullMode.DropWrite` was the alternative. The counter was -chosen because it keeps the existing drain loop unchanged and makes the drop explicit at -the call site instead of hiding it behind a channel option. - -**Verified:** two new tests — -`Should_drop_logs_when_pending_queue_is_full` and -`Should_accept_logs_again_after_pending_queue_has_been_written`. Both **fail on the -pre-fix code**. The second was additionally mutation-checked: removing the -`Interlocked.Decrement` from the drain loop kills it and nothing else, confirming it -really covers the recovery path rather than passing incidentally. This required splitting -the test helper, because the existing `WaitForCompletion` disposes the store and so cannot -be used to drain twice. `Squidex.Infrastructure.Tests` green (1033). - ---- - -### 11. Cross-schema content queries never used the cached total — **FIXED** -`backend/src/Squidex.Data.MongoDb/Domain/Apps/Entities/Contents/Operations/QueryByQuery.cs` - -**Was:** - -```csharp -var (filter, isDefault) = CreateFilter(app.Id, schemas.Select(x => x.Id), ...); -``` - -`isDefault` was computed and then discarded. The multi-schema overload had no -`else if (isDefault)` branch, unlike the single-schema overload thirty lines below which -routes through `countCollection.GetOrAddAsync`. So the "all schemas" `/contents` endpoint -ran a full uncached `CountDocumentsAsync` over every content in the app on each page. - -**Now:** the branch is mirrored, keyed by app plus the schema set: - -```csharp -else if (isDefault) -{ - var totalKey = CreateTotalKey(app, schemas); - - contentTotal = await countCollection.GetOrAddAsync(totalKey, ct => Collection.Find(filter).CountDocumentsAsync(ct), ct); -} -``` - -**The key needs care, which is why it is not just an interpolated list.** The schema set -depends on the caller's permissions and arrives in no guaranteed order, so the ids are -sorted before hashing — otherwise the same query would produce different keys and never -hit. And the key becomes the `_id` of the count document, where MongoDB caps index keys at -1024 bytes; a raw join of 37-character ids would exceed that at roughly 27 schemas. Hashing -gives a bounded, deterministic key: - -```csharp -var schemaIds = schemas.Select(x => x.Id.ToString()).Order(StringComparer.Ordinal); - -return $"{app.Id}_Schemas_{string.Join('_', schemaIds).ToSha256Base64()}"; -``` - -The `_Schemas_` marker keeps this key space distinct from the single-schema overload's -`$"{appId}_{schemaId}"`. The two must not share entries in any case: their filters differ -(`Filter.In` vs `Filter.Eq`, and different existence guards), so the counts are not -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. - ---- - -### 21. Content DTO link generation — **CLOSED: REJECTED** -`backend/src/Squidex/Areas/Api/Controllers/Contents/Models/ContentDto.cs:156` - -`CreateLinksAsync` issues up to ten `IUrlHelper.Action` calls per content, so a 200-item -frontend page runs on the order of 2000 link generations. - -**Rejected, not fixed.** The proposed fix — building URLs from a cached per-schema prefix -and concatenating the id — bypasses the ASP.NET routing system. Links would stop reflecting -the actual route table, so any change to a route template, a route constraint, or the path -base would silently produce wrong URLs. That is not a trade worth making for link -generation, whatever it costs. Recorded here so it is not re-reported as a new finding. - -If this ever does show up in a profile, the answer has to stay inside the routing system — -for example ASP.NET's own `LinkGenerator` with a cached endpoint lookup — not around it. - ---- - -### 22. The EF data layer never used `AsNoTracking` — **FIXED** -`backend/src/Squidex.Data.EntityFramework/ContentDbContext.cs` -`backend/src/Squidex.Data.EntityFramework/Infrastructure/Extensions.cs:116,150` -plus the entity-materializing reads in the content and asset repositories - -**Was:** not a single `AsNoTracking()` in the layer and no `QueryTrackingBehavior` setting -anywhere. Every entity from every read query got a change-tracking snapshot — on entities -that carry a full content `Data` blob, so roughly double the memory per content read. - -**Now, and the split matters:** - -- `ContentDbContext` gets `QueryTrackingBehavior.NoTracking` as its **default**. That - context is content-only, and every content write goes through `BulkInsertAsync`, never by - mutating a queried entity. -- `AppDbContext` keeps its default, with `AsNoTracking()` applied to the individual read - paths: both `QueryAsync` helpers in `Infrastructure/Extensions.cs` (which most repository - reads funnel through), `EFContentRepository.FindContentAsync`, - `EFAssetRepository.StreamAll`, the `ReadAllAsync` / single-read paths of the content, asset - and asset-folder snapshot stores, `DynamicTables`, and both paths of the generic - `EFSnapshotStore`. - -**Why `AppDbContext` was not flipped globally — corrected.** The first version of this note -claimed ASP.NET Identity's `UserStore.SetTokenAsync` would silently stop persisting under a -global `NoTracking` default, because it assigns `token.Value = value` with no `Update` call. -**That was wrong**, and it was asserted from memory rather than checked. Tested against -Identity 10.0.6 + EF SQLite with the default flipped both ways: the token round trip and the -user update both persist correctly. The reason is that the EF `UserStore` reaches tokens via -`DbSet.FindAsync`, and `Find`/`FindAsync` track the entity regardless of -`QueryTrackingBehavior` — they are not LINQ queries. - -**The real reason, found by auditing the shared libraries** (`D:\squidex-tools\libs`). -`AppDbContext` is not only Squidex's own repositories — `OnModelCreating` also mounts -`UseOpenIddict()`, `UseAssetKeyValueStore` (Tus), `UseChatStore()`, `UseFlows()`, -`UseCronJobs()`, `UseMessagingDataStore()`, `UseMessagingTransport()` and Identity. Two of -those stores read an entity with a **LINQ query**, mutate it, and call `SaveChanges` with no -`Update`, which is exactly the pattern a `NoTracking` default turns into a silent no-op: - -| Store | Code | Effect under a global `NoTracking` default | -| --- | --- | --- | -| `Squidex.AI.EntityFramework/EFChatStore.SetAsync` | `Where(...).FirstOrDefaultAsync()` then `entity.Value = json` | conversation updates never persist | -| `Squidex.Messaging.EntityFramework/EFSubscription` | `query.FirstOrDefaultAsync()` then `efMessage.TimeHandled = now` | **message is never marked handled** | - -The messaging one is the blocker. That assignment *is* the queue's claim on a message, and -the `DbUpdateConcurrencyException` it can raise is the only thing stopping two processes -consuming the same message. With no tracked change, `SaveChangesAsync` issues no UPDATE, so -`TimeHandled` stays null, the concurrency guard can never fire, the callback still runs, and -the next poll matches the same row again — silent infinite redelivery plus duplicate -processing across processes, with no exception anywhere. - -Everything else audited clean: `EFCronJobStore`, `EFAssetKeyValueStore`, `EFEventStore`, -`EFMessagingDataStore` and `EFTransport` all `AddAsync` new entities; `EFFlowStateStore` uses -`ExecuteUpdateAsync` and bulk upsert; OpenIddict uses explicit `Attach` + `Update`; Identity -was verified empirically (see above) and calls `_userStore.Update(user)` explicitly. - -**So the flip is two one-line fixes away.** Adding `dbContext.Update(entity)` before -`SaveChangesAsync` in those two stores would make `AppDbContext` safe to default to -`NoTracking` — and would also remove a latent fragility, since both currently depend on the -tracking configuration of a `DbContext` the library does not own. - -`ContentDbContext` has no such tenants, which is what makes the global flip safe there. - -**Caveat on the explicit approach, which is real.** Enumerating read sites is fragile: a -later sweep found five more entity-materializing reads that the first pass missed — -`EFAssetFolderRepository_SnapshotStore` (both paths), `DynamicTables`, and both paths of the -generic `EFSnapshotStore`, which backs *every* domain object snapshot and streams the whole -table on a rebuild. Those have been fixed too, but a global default would not have needed -finding them. - -The `ReadAllAsync` streams were the worst individual case: they walk every content or asset -in the database for a rebuild, so tracking retained the entire table in the change tracker. - -All seven `SaveChangesAsync` call sites in the layer were checked first — every one -constructs a new entity and `Add`s or bulk-inserts it. None mutate a queried entity, which -is what makes the change safe. - ---- - -### 24. Queries by id spent an extra round trip counting a bounded set — **FIXED** -`backend/src/Squidex.Data.MongoDb/Infrastructure/Queries/LimitExtensions.cs:16` -`backend/src/Squidex.Data.MongoDb/Domain/Apps/Entities/Contents/Operations/QueryByIds.cs:59` -`backend/src/Squidex.Data.MongoDb/Domain/Apps/Entities/Assets/MongoAssetRepository.cs:112` - -**Was:** both id-query paths ran `CountDocumentsAsync` to get the total even though the -filter is `In(ids)`. Since `ContentQueryParser.WithPaging` sets `Take = q.Ids.Count` for id -queries, the guard fired whenever every requested id was found — the normal case — so this -was an extra round trip on the reference-resolution path. - -**Now:** a shared predicate decides when a count can tell you anything new. - -```csharp -public static bool NeedsTotalById(this ClrQuery query, int idCount) -{ - return query.Skip > 0 || query.Take < idCount || query.Random > 0; -} -``` - -**The `Random` term is the non-obvious one.** Both paths finish through -`ToListRandomAsync`, which — when `query.Random > 0` — returns a random *sample* of the -matches rather than all of them. In that case the returned count is not the match count, so -the count query is still required. The first version of this fix omitted that and would have -reported the sample size as the total. - -`NoTotal` semantics are unchanged: it still short-circuits to `-1` before this predicate is -consulted, rather than opportunistically returning a total the caller asked not to have. - ---- - -### 25. `ResolvingAssets()` re-evaluated per content — **FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/ResolveAssets.cs:129` - -The same defect as item 17, in the sibling step: `AddAssetIds` called the lazy -`schema.ResolvingAssets()` inside the per-content loop, rescanning every field of the schema -and allocating two LINQ iterators per content. Hoisted to a single `ToList()` above the loop. - ---- - -### 26. `CalculatePreviewText` filtered all schema fields once per content — **FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/CalculatePreviewText.cs:31` - -`schema.Fields.Where(x => x.RawProperties is RichTextFieldProperties)` sat in the inner loop, -re-scanning every field for every content to produce a list identical for the whole group. -Hoisted, with an early return when the schema has no rich-text fields at all — which is the -common case and previously still paid a full field scan per content. - ---- - -### 27. `EnrichForCaching` re-added the same schema and app dependency per content — **FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/EnrichForCaching.cs` - -**Was:** all three `AddDependency` calls sat in the per-content loop, but only the content one -varies. The other two re-added a key already in the set, so `CachingManager` took its lock and -did a `HashSet.Add` that returned false — a 200-item page paid ~600 lock acquisitions to do -~202 useful ones. - -**Now:** the app and schema dependencies are added once per schema group. - -**They were deliberately left *inside* the group loop rather than hoisted to the top of the -method.** Hoisting looks tidier but changes behaviour for an empty result: with no contents -there are no groups, so today nothing is added, `hasDependency` stays false, and the response -gets no ETag. Adding the app dependency unconditionally would start emitting an ETag for -empty responses — a change in caching behaviour that has nothing to do with this finding. -Once per group is still 1 instead of 200 for the normal single-schema query. - ---- - -### Verification note for items 22 and 24 - -Build clean; `Squidex.Domain.Apps.Entities.Tests` (1541), `Squidex.Domain.Apps.Core.Tests` -(1243), `Squidex.Infrastructure.Tests` (1033), `Squidex.Web.Tests` (176) and the runnable part -of `Squidex.Data.Tests` (180) are all green. - -**That green is weaker than it looks for items 22 and 24.** `Squidex.Data.Tests` contains -~1349 tests, of which only 180 run without the `Dependencies` / `TestContainer` categories — -the ~1169 excluded ones are exactly the EF and MongoDB integration tests that would actually -exercise `AsNoTracking` and `NeedsTotalById` against a real database. Those two items are -reasoned-correct and compile, but they are **not covered by any test that was run here**. -They should be validated against a container run before release. - -Items 25, 26 and 27 are pure hoists with no behavioural change and are covered by the -enrichment tests that did run. - ---- - -### 28. Asset downloads used an exception as the legacy-path fallback — **FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Assets/DefaultAssetFileStore.cs` - -**Was:** `GetFileSizeAsync` and `DownloadAsync` tried the current file name, caught -`AssetNotFoundException`, and retried with the legacy name (no app ID). On an instance that -still holds assets under the old scheme, *every* access to those assets threw and caught -first — and against a cloud store the failed attempt is a full network round trip, so the -fallback roughly doubled the latency of every legacy asset served. - -**Now:** the outcome is remembered per asset in the injected `IMemoryCache`, keyed by -`(typeof(DefaultAssetFileStore), appId, id)` with a one hour sliding lifetime, so the wrong -name is only tried once. `IMemoryCache` rather than `Squidex.Caching.LRUCache` because it is -thread safe on its own — see item 7 for what `LRUCache` does under concurrent access. - -**The memo is a hint, not a decision.** `FileNames(...)` returns both names ordered by what -was last seen to work, and the other one is still tried on failure. That matters because an -asset can move between schemes — a migration, or an eviction followed by a re-probe — and a -cache that *decided* rather than *hinted* would turn a stale entry into a hard failure. The -cost of a wrong hint is one extra round trip, exactly what the code did before. - -Two things fell out of it: the `options.FolderPerApp` case now short-circuits to a single -name with no try/catch at all, and the partial-write hazard flagged in the finding (a retry -appending to a stream the first attempt already wrote to) is now hit far less often, since a -warm asset takes the right branch first. It is not *fixed* — that would need the asset store -to guarantee it writes nothing before failing. - ---- - -### 29. Removing items while iterating a `JsonArray` was quadratic — **FIXED** -`backend/src/Squidex.Domain.Apps.Core.Operations/ConvertContent/ContentConverter.cs:145,175` - -**Was:** `ConvertArray` and `ConvertComponents` both removed in place with -`array.RemoveAt(i); i--;`. `JsonArray` derives from `List`, so each removal shifts -every following element — dropping *k* of *n* items costs O(n·k), and the case where many -items are dropped (entries referencing deleted component schemas) is exactly the case where -the array is large. - -**Now:** a single compaction pass with a write index, then one `RemoveRange` for the tail. - -```csharp -var target = 0; - -for (var i = 0; i < array.Count; i++) -{ - var oldValue = array[i]; - - var (removed, newValue) = ConvertArrayItem(field, oldValue); - if (removed) - { - continue; - } - - array[target] = ReferenceEquals(newValue.Value, oldValue.Value) ? oldValue : newValue; - target++; -} - -array.RemoveRange(target, array.Count - target); -``` - -The write index is always `<= i`, so a slot is only ever overwritten after it has been read — -no read-after-write hazard, and the surviving order is preserved. - -**Verified with new tests** — `ContentConversionRemovalTests`, 27 cases covering nine removal -patterns (none, first, middle, last, adjacent pairs, alternating, all) across three ways an -item gets dropped: a non-object in an array, a component of an unknown schema, and a -component with no discriminator. - -Two checks on the tests themselves, because this is a behaviour-preserving rewrite rather -than a bug fix: - -- They pass against **both** the original `RemoveAt` implementation and the new one, which is - the property that actually matters here — they pin the contract rather than the code. -- Mutation check: deleting the `RemoveRange` line fails 24 of the 27, so they are not - vacuous. - -A first attempt at these tests drove removal through a custom `IContentItemConverter` that -stripped the discriminator; that never removed anything, because `ConvertComponent` checks -the discriminator *before* calling `ConvertNested`. The tests now use inherently invalid -items, which is both simpler and closer to the real cause. - ---- - -### 30. `stream.ToArray()` copied straight back out of the pooled buffer — **FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Assets/Transformations.cs:79` - -**Was:** `GetTextAsync` downloaded into a `DefaultPools.MemoryStream` -(`RecyclableMemoryStreamManager`) and then called `ToArray()`, allocating a fresh array of -the whole file and copying the pooled buffer into it — for a file at the 4 MB limit, straight -onto the large object heap on every call. - -**Now:** - -```csharp -var bytes = new ReadOnlySpan(stream.GetBuffer(), 0, (int)stream.Length); -``` - -`Convert.ToBase64String` and `Encoding.GetString` all have `ReadOnlySpan` overloads, so -nothing downstream changed. - -Worth being precise about why `GetBuffer` is better rather than just "avoids a copy": -`RecyclableMemoryStream.GetBuffer()` still consolidates into a single contiguous buffer when -the stream spans several blocks. The difference is that the buffer it returns comes from the -pool and goes back on dispose, whereas `ToArray` allocates a new GC array every time. -`RecyclableMemoryStream` documents `ToArray` as the call to avoid for exactly this reason. - ---- - -### 23. Full-text search loads a fixed 1000 ids — **CLOSED: BOUNDED, NOT FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/ContentQueryParser.cs:93` -`backend/src/Squidex.Domain.Apps.Entities/Contents/ContentsOptions.cs` -`backend/src/Squidex/appsettings.json` - -**Was:** `new TextQuery(query.FullText, 1000)` — a hardcoded literal. The ids come back and go -into an `In("id", …)` filter, so a query matching more than 1000 items silently loses the -rest, relevance order is discarded, and up to 1000 GUID strings travel to the database on -every page. - -**Now:** the limit is `ContentsOptions.MaxFullTextResults`, configurable as -`contents:maxFullTextResults` and documented in `appsettings.json` with what raising it costs. -Default unchanged at 1000. - -**Why this is closed as bounded rather than fixed.** I proposed paging the text index and -walked it back after working through the constraint: **the full text index and the content -store are separate, independently configured stores**, and any combination is legal — -Mongo+Mongo, Elastic+Mongo, Elastic+SQL, Azure+anything. `MongoContentRepository` takes -`store:mongoDb:contentDatabase` while the text index resolves the default `IMongoDatabase`; -in EF the index is on `AppDbContext` and contents are on `ContentDbContext`. - -So this is a cross-store join, always, and the 1000 is not a magic number — it is the join -buffer. No value for it is correct, because how many survive depends on a filter the index -has never seen. - -Paging the index only works when the index alone decides both membership *and* order. It -does not, in two common cases: - -| Case | Ids that must cross the boundary | -| --- | --- | -| search only, relevance-ordered | the page (~20) | -| search + explicit `$orderby` | all matches | -| search + `$filter` | all matches, or an iterative top-up | - -And the default sort is `lastModified` — `WithSorting` adds it when the caller gives none — -so today's pipeline is already "the 1000 most relevant, displayed newest first", which is -neither. Making paging work would mean changing the default sort for full-text queries to -relevance: a behaviour change, not an optimisation. - -There is also no architectural escape. Pushing the filter into the index means indexing -arbitrary user-filterable fields — reimplementing the query engine on the search side. -Pushing relevance into the content store means the store needs the scores. Either way the -boundary just moves, and because the backends pair arbitrarily you would owe it for every -combination. - -**Left undone, deliberately, and worth knowing about:** a truncated result is still -indistinguishable from a complete one. A caller paging a 5000-hit search gets a confident -wrong total and silently loses the remainder. Returning `total = -1` when the cap is hit — -the codebase's existing "unknown" convention, used by `NoTotal` — would make it visible -without any interface change. Defaulting full-text queries to relevance order is arguably a -bug fix on its own. - ---- - -### 31. Every request rebuilt the caller's permission set — **FIXED** -`backend/src/Squidex.Domain.Apps.Core.Model/Apps/Roles.cs` - -**Was:** `AppResolver` runs on every API request and resolves the caller's role through -`Roles.TryGet` → `Role.ForApp(app, isFrontend)`, which rebuilt the permission set each time: -a prefix `Permission` (three `string.Replace` calls), then a concatenated string and a -`Permission` per role permission, ten more for a frontend caller, plus a `HashSet`, a -`PermissionSet` and a `Role`. - -**Now:** `Roles` memoizes the resolved role in a `ConcurrentDictionary` keyed by -(app, name, isFrontend). `Role` is a record and immutable, so the result is a pure function -of that key. - -**The cache lives on `Roles`, not on `Role`, for two reasons.** `Role` is a `record`, so its -synthesized `Equals`/`GetHashCode` cover every instance field — adding a cache field would -make two logically equal roles compare unequal. And `Roles` instances hang off the cached -`App`, so the natural lifetime is already right. - -**It is bounded, and that is not cosmetic.** `App.Roles` defaults to the *shared static* -`Roles.Empty`, so for every app without custom roles the cache lives on one instance shared -across all tenants and would grow with the number of apps. It is capped at 1000 entries and -cleared wholesale on overflow; hitting the cap degrades to the old behaviour rather than -leaking. An app with its own roles has its own `Roles` instance and never approaches it. - -`Microsoft.Extensions.Caching.Memory` would have been the nicer bound, but -`Squidex.Domain.Apps.Core.Model` is a pure model project with no caching dependency and it -did not seem worth adding one there. - ---- - -### 32. Any app change threw away the whole GraphQL schema — **FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Contents/GraphQL/CachingGraphQLResolver.cs:67` - -**Was:** the cache key was `(typeof(CachingGraphQLResolver), app.Id, app.Version)`. -`app.Version` bumps on *any* app event — a contributor, a client, a role, a setting — so each -one was a cold miss, and the next GraphQL request paid a full `BuildSchema`: a content type, a -result type and a component type per schema, each initialised with a GraphQL field per schema -field, plus queries, mutations and a `FieldMap`. - -**Now:** the key is `(typeof(CachingGraphQLResolver), app.Id)`. - -**The version was redundant, not load-bearing.** The entry is already created with a -validator, and `SchemasHashKey.Create` builds its dictionary starting with -`[app.Id] = app.Version` before adding every schema version. So the app version was in the -validator all along — having it in the key too meant app changes could never *reach* the -validator, they just missed. - -The behavioural difference is which path a change takes: an app-level change now goes through -the validator like a schema change does — the cached schema is served and refreshed — instead -of blocking the next request on a rebuild. That is the same eventual-consistency trade the -design already makes for schema changes, which are the more visible ones. - ---- - -### 33. Asset and content tokens serialized an object per item — **FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Assets/Queries/Steps/CalculateTokens.cs` -`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/CalculateTokens.cs` - -**Was:** both steps allocated a fresh anonymous object per item to hold the edit token, when -only one or two of its fields actually vary: - -```csharp -foreach (var asset in assets) -{ - var token = new { a = asset.AppId.Name, i = asset.Id.ToString(), u = url }; - - asset.EditToken = Convert.ToBase64String(serializer.SerializeToBytes(token)); -} -``` - -**Now:** a private `Token` class is created once per call and its properties are assigned per -item. The short wire names are kept with `[JsonPropertyName]`, so the properties can have -readable names without changing the format. - -**There is a content version of this too**, which the original finding missed — it carries a -fourth field (`s`, the schema name) and sits on the content list path, which is hotter than -the asset one. Both are fixed. - -**The wire format is load-bearing and was pinned first.** The token is base64 of a JSON object -with single-letter keys, decoded by the frontend, and the existing tests only asserted -`EditToken != null` — nothing covered the shape. So a test asserting the exact decoded string -was added and confirmed green against the *old* code before the change, then again after: - -```csharp -var expected = $$"""{"a":"{{asset.AppId.Name}}","i":"{{asset.Id}}","u":"https://squidex.io"}"""; -``` - -This removes the per-item object allocation, not the per-item serialization — the serializer -still runs once per item. Emitting the constant prefix once and varying only the id would go -further, at the cost of hand-writing JSON. - ---- - -### 35. `JobWorker` cached a faulted task for the process lifetime — **FIXED** -`backend/src/Squidex.Domain.Apps.Entities/Jobs/JobWorker.cs` - -**Was:** `processors.GetOrAdd(appId, async key => …)` stored the `Task`, so a -transient failure in `LoadAsync` was cached permanently — jobs for that app never ran again, -and the failure was invisible because every caller saw the *same* exception rather than a new -one. The same defect as item 15. - -**Now:** the entry is removed when the task faults, comparing by reference so a newer -successful entry added by another caller is not discarded. The removal takes the same lock -that guards the dictionary. - ---- - -### 34. Message formatting looked up properties by reflection on every call — **FIXED** -`backend/src/Squidex.Infrastructure/Translations/ResourcesLocalizer.cs` - -**Was:** `ResourcesLocalizer.Get` resolved every `{variable}` placeholder with -`argsType.GetProperty(variableName)` and cached nothing, so each call paid a name search over -the type's members. Mostly harmless on error paths, but `ResolveReferences.CreateFallback` -calls `T.Get("contents.listReferences", new { count = … })` inside the per-content, -per-partition loop of the enrichment pipeline. - -**Now:** a static `ConcurrentDictionary<(Type, string), PropertyInfo?>` memoizes the lookup. - -**Two details that matter more than they look:** - -- **Misses are cached as `null`.** An unknown variable name is a legitimate case — the code - falls back to printing the name — and without caching the negative result those would pay - the reflection cost on every single call, which is the worst case rather than the best. -- **The key is (Type, name), not name.** Different anonymous types share property names, so a - name-only key would hand back another type's `PropertyInfo` and `GetValue` would throw into - the existing `catch`, silently degrading the message to the raw variable name. - -No eviction: the arg types are compiler-generated and the variable names come from the -resource files, so the number of combinations is fixed by the code. - -**Verified:** three new tests in `TTests` — an unknown property, repeated calls with different -values, and two different arg types using the same variable name. The last one is the guard -against the name-only key: mutating the implementation to key by name alone fails it, along -with the two existing case-conversion tests, and leaves the rest passing. -`Squidex.Infrastructure.Tests` green (1036). - ---- - -### Note: `JobWorker` build fix - -While running the suites, `JobWorker.GetJobProcessorAsync` did not compile — the helper from -item 35 had been inlined into it but the method was still declared non-async while using -`await`. Added the `async` modifier and removed a trailing whitespace. No behavioural change; -the `lock` block closes before the `await`, so nothing is held across it. diff --git a/todo.md b/todo.md deleted file mode 100644 index ade11352c..000000000 --- a/todo.md +++ /dev/null @@ -1,100 +0,0 @@ -# Backend Performance — Open - -Analysis of `backend/src` (2131 C# files, ~187k LOC). Ordered by severity: expected -production impact × how hot the code path is. - -Severity key: **S1** critical (can dominate request latency or take the process down), -**S2** high (measurable on every request in a common path), **S3** moderate (steady -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**–**35** are closed and live there. - -**Status: 3 open of 35 — items 1, 2 and 3, which are one root cause. The other 32 are in [resolved.md](resolved.md).** - ---- - -## S1 — Critical - -### 1. A fresh Jint `Engine` is constructed for every script evaluation -`backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/JintScriptEngine.cs:144` - -`CreateEngine` calls `new Engine(...)` on every `Execute` / `ExecuteAsync` / -`TransformAsync`. Building a Jint engine allocates a complete JS realm (global object, -`Object`/`Array`/`JSON`/`Math`/`RegExp` prototypes, intrinsics) plus runs every -registered `IJintExtension.Extend`. Script *parsing* is cached via `CacheParser`, but -engine construction — the expensive half — is not. - -This is the root cause of items 2 and 3, which is why it ranks first. - -**Fix:** pool engines (`ObjectPool`) keyed by the option set, resetting globals -between uses; or hoist one engine per enrichment batch instead of per item. - ---- - -### 2. Workflow enrichment runs one Jint engine per content *per transition* -`backend/src/Squidex.Domain.Apps.Entities/Contents/DynamicContentWorkflow.cs:90,118` -`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/EnrichWithWorkflows.cs:22,30,31` - -`EnrichWithWorkflows` loops over every content and awaits `GetNextAsync` and -`CanUpdateAsync` sequentially. `GetNextAsync` loops over every transition and calls -`IsTrue`, which calls `scriptEngine.Evaluate` whenever the transition has an -expression — a new engine each time (item 1). - -A frontend content list of 200 items with a workflow having 3 conditional transitions -executes **200 × (3 + 1) = 800 engine constructions** in one request, serially. - -`GetWorkflowAsync` additionally re-scans `app.Workflows.Values` with -`SchemaIds.Contains(schemaId)` on every one of those calls. - -**Fix:** cache the resolved `Workflow` per (appId, schemaId) for the batch; memoize -condition results per (transition, contentData); reuse one engine. - ---- - -### 3. Query scripts execute one engine per content, serially -`backend/src/Squidex.Domain.Apps.Entities/Contents/Queries/Steps/ScriptContent.cs:57` - -`foreach (var content in group) await TransformAsync(...)` — every content in the page -gets its own engine construction plus its own -`CancellationTokenSource.CreateLinkedTokenSource`. Any schema with a query script pays -this on every read. - -**Fix:** same as item 1 — reuse the engine across the group; per-content state is -already isolated in `ContentScriptVars`. - ---- - -## Suggested order of attack - -Only **engine pooling (items 1–3)** is left. It is the largest single cost on the list and -also the most invasive change: it touches the security boundary of user-authored scripts, -because a pooled engine must not carry state from one script into the next. - -**Profile before writing it.** The estimate that engine construction dominates a scripted -content list is read off the loops, not taken from a trace, and this is the one item where -the fix is expensive enough that being wrong about the size of the win would matter. - ---- - -## Method / caveats - -Findings come from static reading of the hot paths (content query + enrichment pipeline, -GraphQL execution, write/validation path, event consumers, HTTP pipeline, MongoDB and EF -repositories, asset serving, response/DTO construction) plus scripted scans for -sync-over-async, awaits inside loops, uncached `Regex`, and repeated LINQ materialisation. - -**No profiling or benchmarking was run.** The ordering is a reasoned estimate of impact, -not measured data. Counts like "200 × 4 engine constructions" or "2000 link generations" -are derived from reading the loops, not observed. Confirm the expensive items with a -profiler against a representative workload before investing in the larger refactors. - -Items 21–30 were added in a second pass over areas the first pass had not covered: the -EF data layer, asset serving and transformation, response DTO and link construction, the -full-text search path, and the remaining enrichment steps. Two candidates were dropped -during that pass after checking them: per-content permission checks (already memoized in -`Resources.Can`) and the lazily built static maps in `Adapt` (a benign race that at worst -builds the same dictionary twice). - -Line numbers were verified against the working tree at the time of writing.