Skip to content

Commit de00b59

Browse files
authored
fix(framework): correctness & perf findings from framework review (#278)
* fix(framework): address correctness & perf findings from framework review Fixes from a five-area review of the framework layer (Core, Generator, Hosting, Database/Storage, frontend build). Scoped to the framework; no module behavior changed except where a module consumed a broken framework primitive. High - FeatureFlags gate + audit interceptor read ClaimTypes.NameIdentifier only, so under Keycloak/JWT (MapInboundClaims=false, id in "sub") per-user overrides were ignored and CreatedBy/UpdatedBy went null. Use sub-aware GetUserId()/GetRoles(). - Static-file cache middleware applied a 1yr public,immutable header to ANY request with a "?v=" query key, incl. authenticated HTML (e.g. /dashboard?v=1). Gate the immutable header on static-asset path prefixes. - LocalStorageProvider: ListAsync bypassed the traversal guard entirely; GetFullPath's boundary check lacked a separator (so /app/storage-backup passed an /app/storage check) and used OrdinalIgnoreCase on case-sensitive filesystems. Route ListAsync through the guard; require a separator boundary; OS-appropriate case sensitivity. - Multi-tenant query filter froze the first request's scoped ITenantContext into the cached EF model (cross-tenant leak). Reference the executing DbContext instance via new ITenantScopedDbContext.CurrentTenantId so EF re-evaluates per query. - Source generator re-scanned every referenced assembly (BCL, ASP.NET) on each compile. Filter out System./Microsoft./framework assemblies from the discovery walks. Medium - InertiaResult merged shared data by round-tripping props through a JsonElement DOM then re-serializing the whole dict — props encoded twice per render. Collapse to one Utf8JsonWriter pass (props win on collision, no duplicate keys). - InertiaLayoutDataMiddleware ran antiforgery crypto + menu filtering for every request incl. static assets and health probes. Gate to Inertia (X-Inertia) or HTML (Accept: text/html) requests. - EntityInterceptor/EntityChangeInterceptor only overrode the async SaveChanges path, so a sync SaveChanges() would hard-delete soft-delete entities and skip audit/events. Override the sync path on both. - Generated JSON resolver was uncompilable for init-only props / positional records (CS8852/CS7036). Treat init-only setters as non-settable; guard CreateObject on an accessible parameterless ctor. - Vite vendor plugin skipped rebuild whenever outputs existed, shipping stale React after a version bump. Key a manifest on resolved package versions. Low - PermissionMatcher wildcard match: span-based, no per-check substring allocation. - GlobalExceptionHandler: ArgumentNullException -> 500 (internal bug) not client 400. - InertiaMiddleware.Version: guard empty Assembly.Location (single-file publish crash). - SymbolHelpers: module-namespace prefix match now requires a dot boundary. - ViewPagesEmitter: hint name from module FQN (no collision for same-named classes). - SoftDeleteService.CoerceId: handle Guid / value-object keys instead of throwing. - Maintenance gate: correct the static-asset comment and exempt static-asset paths. - PermissionRegistryBuilder: O(1) HashSet dedup instead of O(n^2) list scan. - app.tsx: in-page hash anchors scroll natively instead of re-requesting via Inertia. - add-component.mjs: resolve packages/ or src/ UI dir; typecheck.mjs: cap tsc concurrency. Regression tests: cross-tenant isolation across a shared cached model; sub-aware GetUserId/GetRoles; wildcard module-boundary; ViewPages same-name collision. Deferred (documented in tasks/todo.md): generator pipeline decomposition onto MetadataReferencesProvider; @simplemodule/ui shared-chunk vendoring. Claude-Session: https://claude.ai/code/session_01GACvG1C7EZGSDDGTaBUFjg * fix(docker): point default SQLite path at writable /app/data so bare `docker run` works The runtime image runs as a non-root appuser, but the app's built-in default connection string ("Data Source=app.db") resolves to the root-owned /app workdir, so a standalone `docker run` of the image crashed at startup with "SQLite Error 14: unable to open database file". The Dockerfile already creates a writable /app/data directory for the database but never pointed the app at it. Set Database__DefaultConnection to Data Source=/app/data/app.db in the runtime stage. docker-compose still overrides this with its PostgreSQL configuration, so the Postgres deployment path is unchanged; only the standalone-SQLite quickstart is fixed.
1 parent 7e84399 commit de00b59

37 files changed

Lines changed: 863 additions & 208 deletions

‎Dockerfile‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,14 @@ COPY --from=build --chown=appuser:appgroup /app/publish .
157157
# Writable directory for SQLite database and local storage
158158
RUN mkdir -p /app/data /app/storage && chown appuser:appgroup /app/data /app/storage
159159

160+
# Default the SQLite database into the writable /app/data dir. The app's built-in
161+
# default is "Data Source=app.db", which resolves to the root-owned /app workdir
162+
# that the non-root appuser cannot write — a bare `docker run` of this image would
163+
# otherwise crash at startup with "SQLite Error 14: unable to open database file".
164+
# docker-compose overrides this (Database__Provider=PostgreSQL + its own connection
165+
# string), so the Postgres deployment path is unaffected.
166+
ENV Database__DefaultConnection="Data Source=/app/data/app.db"
167+
160168
# Deployment version for JS/CSS cache-busting and Inertia stale-version detection.
161169
# Left empty by default so InertiaMiddleware.GetVersion() falls back to the entry
162170
# assembly's last-write timestamp (yyyyMMddHHmmss), which changes on every publish

‎framework/SimpleModule.Core/Authorization/PermissionMatcher.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,12 @@ public static bool IsMatch(string claim, string requirement)
2323
if (string.Equals(claim, requirement, StringComparison.Ordinal))
2424
return true;
2525

26-
// Wildcard: claim ends with ".*"
26+
// Wildcard: claim ends with ".*" — match the "Prefix." span without allocating.
2727
if (
2828
claim.Length > 2
2929
&& claim[^1] == '*'
3030
&& claim[^2] == '.'
31-
&& requirement.StartsWith(claim[..^1], StringComparison.Ordinal)
31+
&& requirement.AsSpan().StartsWith(claim.AsSpan(0, claim.Length - 1), StringComparison.Ordinal)
3232
)
3333
{
3434
return true;

‎framework/SimpleModule.Core/Authorization/PermissionRegistryBuilder.cs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ namespace SimpleModule.Core.Authorization;
88
public sealed class PermissionRegistryBuilder
99
{
1010
private readonly Dictionary<string, List<string>> _byModule = new();
11+
private readonly HashSet<string> _seen = new(StringComparer.Ordinal);
1112

1213
public void AddPermissions<T>()
1314
where T : class
@@ -28,16 +29,18 @@ public void AddPermission(string permission)
2829
var dotIndex = permission.IndexOf('.', StringComparison.Ordinal);
2930
var module = dotIndex >= 0 ? permission[..dotIndex] : "Global";
3031

32+
if (!_seen.Add(permission))
33+
{
34+
return;
35+
}
36+
3137
if (!_byModule.TryGetValue(module, out var list))
3238
{
3339
list = [];
3440
_byModule[module] = list;
3541
}
3642

37-
if (!list.Contains(permission))
38-
{
39-
list.Add(permission);
40-
}
43+
list.Add(permission);
4144
}
4245

4346
public PermissionRegistry Build()

‎framework/SimpleModule.Core/Exceptions/GlobalExceptionHandler.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,14 @@ CancellationToken cancellationToken
2323
ErrorMessages.ValidationErrorTitle,
2424
ve.Errors
2525
),
26+
// ArgumentNullException almost always signals an internal bug (a null
27+
// reaching a guard clause), not bad client input — let it fall through to
28+
// the 500 branch so it logs at Error and doesn't leak the parameter name.
29+
ArgumentNullException => (
30+
StatusCodes.Status500InternalServerError,
31+
ErrorMessages.InternalServerErrorTitle,
32+
null
33+
),
2634
ArgumentException => (
2735
StatusCodes.Status400BadRequest,
2836
ErrorMessages.ValidationErrorTitle,

‎framework/SimpleModule.Core/Extensions/ClaimsPrincipalExtensions.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
using System.Collections.Generic;
2+
using System.Linq;
13
using System.Security.Claims;
24
using SimpleModule.Core.Authorization;
35

@@ -14,6 +16,17 @@ public static class ClaimsPrincipalExtensions
1416
?? principal.FindFirstValue(ClaimTypes.NameIdentifier);
1517
}
1618

19+
/// <summary>
20+
/// Gets the user's role values, supporting both OpenIddict/JWT ("role", emitted when
21+
/// <c>MapInboundClaims = false</c>) and ASP.NET Identity (<see cref="ClaimTypes.Role"/>).
22+
/// </summary>
23+
public static IEnumerable<string> GetRoles(this ClaimsPrincipal principal)
24+
{
25+
return principal
26+
.Claims.Where(c => c.Type == "role" || c.Type == ClaimTypes.Role)
27+
.Select(c => c.Value);
28+
}
29+
1730
/// <summary>
1831
/// Returns null for Admin users (no scoping — sees all resources), or the user ID for regular users.
1932
/// </summary>

‎framework/SimpleModule.Core/FeatureFlags/EndpointFeatureFlagExtensions.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
using System.Security.Claims;
21
using Microsoft.AspNetCore.Builder;
32
using Microsoft.AspNetCore.Http;
43
using Microsoft.Extensions.DependencyInjection;
4+
using SimpleModule.Core.Extensions;
55

66
namespace SimpleModule.Core.FeatureFlags;
77

@@ -22,8 +22,8 @@ public static TBuilder RequireFeature<TBuilder>(this TBuilder builder, string fe
2222
return await next(context);
2323
}
2424

25-
var userId = context.HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
26-
var roles = context.HttpContext.User.FindAll(ClaimTypes.Role).Select(c => c.Value);
25+
var userId = context.HttpContext.User.GetUserId();
26+
var roles = context.HttpContext.User.GetRoles();
2727

2828
var isEnabled = await featureFlagService.IsEnabledAsync(featureName, userId, roles);
2929

‎framework/SimpleModule.Core/Inertia/InertiaMiddleware.cs‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System.Linq;
12
using Microsoft.AspNetCore.Builder;
23
using Microsoft.AspNetCore.Http;
34

@@ -63,11 +64,29 @@ private static string GetVersion()
6364
// This changes on every recompile/publish, ensuring cache-busting without manual config.
6465
var entryAssembly =
6566
System.Reflection.Assembly.GetEntryAssembly() ?? typeof(InertiaMiddleware).Assembly;
66-
var buildTime = File.GetLastWriteTimeUtc(entryAssembly.Location);
67-
return buildTime.ToString(
68-
"yyyyMMddHHmmss",
69-
System.Globalization.CultureInfo.InvariantCulture
70-
);
67+
68+
// Assembly.Location is empty for single-file–published apps; File.GetLastWriteTimeUtc("")
69+
// would throw and abort all Inertia rendering (this runs in a static initializer). Fall
70+
// back to the informational/assembly version so the app still starts.
71+
var location = entryAssembly.Location;
72+
if (!string.IsNullOrEmpty(location) && File.Exists(location))
73+
{
74+
var buildTime = File.GetLastWriteTimeUtc(location);
75+
return buildTime.ToString(
76+
"yyyyMMddHHmmss",
77+
System.Globalization.CultureInfo.InvariantCulture
78+
);
79+
}
80+
81+
var informationalVersion = entryAssembly
82+
.GetCustomAttributes(typeof(System.Reflection.AssemblyInformationalVersionAttribute), false)
83+
.OfType<System.Reflection.AssemblyInformationalVersionAttribute>()
84+
.FirstOrDefault()
85+
?.InformationalVersion;
86+
87+
return !string.IsNullOrEmpty(informationalVersion)
88+
? informationalVersion
89+
: entryAssembly.GetName().Version?.ToString() ?? "1.0.0";
7190
}
7291

7392
private static string GetEncodedUrl(this HttpRequest request)

‎framework/SimpleModule.Core/Inertia/InertiaResult.cs‎

Lines changed: 72 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
using System.Buffers;
2+
using System.Linq;
3+
using System.Text;
14
using System.Text.Json;
25
using Microsoft.AspNetCore.Http;
36
using Microsoft.AspNetCore.Http.Json;
@@ -29,28 +32,18 @@ public async Task ExecuteAsync(HttpContext httpContext)
2932
{
3033
var options = GetSerializerOptions(httpContext);
3134
var sharedData = httpContext.RequestServices.GetService<InertiaSharedData>();
32-
var mergedProps = MergeProps(_props, sharedData, options);
33-
34-
var pageData = new
35-
{
36-
component = _component,
37-
props = mergedProps,
38-
url = httpContext.Request.Path + httpContext.Request.QueryString,
39-
version = InertiaMiddleware.Version,
40-
};
35+
var url = httpContext.Request.Path + httpContext.Request.QueryString;
36+
var pageJson = SerializePage(_component, _props, sharedData, url, options);
4137

4238
if (httpContext.Request.IsInertia())
4339
{
4440
httpContext.Response.Headers[InertiaHttpExtensions.InertiaHeader] = "true";
4541
httpContext.Response.Headers["Vary"] = InertiaHttpExtensions.InertiaHeader;
4642
httpContext.Response.ContentType = "application/json";
47-
var json = JsonSerializer.Serialize(pageData, options);
48-
await httpContext.Response.WriteAsync(json);
43+
await httpContext.Response.WriteAsync(pageJson);
4944
return;
5045
}
5146

52-
var pageJson = JsonSerializer.Serialize(pageData, options);
53-
5447
var renderer = httpContext.RequestServices.GetRequiredService<IInertiaPageRenderer>();
5548
await renderer.RenderPageAsync(httpContext, pageJson);
5649
}
@@ -84,36 +77,89 @@ private static JsonSerializerOptions GetSerializerOptions(HttpContext httpContex
8477
return fallback;
8578
}
8679

87-
private static object MergeProps(
80+
/// <summary>
81+
/// Serializes the Inertia page envelope in a single pass. When there is no shared
82+
/// data, endpoint props stream straight to the output with no intermediate DOM.
83+
/// When shared data is present, endpoint props must be materialized once (to know
84+
/// their keys), then shared data and props are written into one object — endpoint
85+
/// props keep priority (shared keys they define are skipped), so no key is emitted
86+
/// twice.
87+
/// </summary>
88+
private static string SerializePage(
89+
string component,
8890
object? props,
8991
InertiaSharedData? sharedData,
92+
string url,
9093
JsonSerializerOptions options
9194
)
9295
{
93-
if (sharedData is null || sharedData.All.Count == 0)
96+
var buffer = new ArrayBufferWriter<byte>();
97+
using (var writer = new Utf8JsonWriter(buffer))
9498
{
95-
return props ?? new { };
99+
writer.WriteStartObject();
100+
writer.WriteString("component", component);
101+
102+
writer.WritePropertyName("props");
103+
if (sharedData is null || sharedData.All.Count == 0)
104+
{
105+
JsonSerializer.Serialize(writer, props ?? EmptyProps, options);
106+
}
107+
else
108+
{
109+
WriteMergedProps(writer, props, sharedData, options);
110+
}
111+
112+
writer.WriteString("url", url);
113+
writer.WriteString("version", InertiaMiddleware.Version);
114+
writer.WriteEndObject();
96115
}
97116

98-
var result = new Dictionary<string, object?>();
117+
return Encoding.UTF8.GetString(buffer.WrittenSpan);
118+
}
99119

100-
// Add shared data first (lower priority)
120+
private static void WriteMergedProps(
121+
Utf8JsonWriter writer,
122+
object? props,
123+
InertiaSharedData sharedData,
124+
JsonSerializerOptions options
125+
)
126+
{
127+
// Endpoint props are materialized once so their top-level keys are known;
128+
// this is unavoidable for a correct merge but happens a single time.
129+
using var propsDoc =
130+
props is null ? null : JsonSerializer.SerializeToDocument(props, options);
131+
132+
var propKeys =
133+
propsDoc is null
134+
? null
135+
: new HashSet<string>(
136+
propsDoc.RootElement.EnumerateObject().Select(p => p.Name),
137+
StringComparer.Ordinal
138+
);
139+
140+
writer.WriteStartObject();
141+
142+
// Shared data (lower priority) — skip any key the endpoint props also define.
101143
foreach (var kvp in sharedData.All)
102144
{
103-
result[kvp.Key] = kvp.Value;
145+
if (propKeys is not null && propKeys.Contains(kvp.Key))
146+
continue;
147+
148+
writer.WritePropertyName(kvp.Key);
149+
JsonSerializer.Serialize(writer, kvp.Value, options);
104150
}
105151

106-
// Add endpoint props (higher priority — overwrites shared data)
107-
// Use JSON round-trip to merge endpoint props into shared data
108-
if (props is not null)
152+
// Endpoint props (higher priority).
153+
if (propsDoc is not null)
109154
{
110-
var json = JsonSerializer.SerializeToElement(props, options);
111-
foreach (var property in json.EnumerateObject())
155+
foreach (var property in propsDoc.RootElement.EnumerateObject())
112156
{
113-
result[property.Name] = property.Value;
157+
property.WriteTo(writer);
114158
}
115159
}
116160

117-
return result;
161+
writer.WriteEndObject();
118162
}
163+
164+
private static readonly object EmptyProps = new();
119165
}
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
namespace SimpleModule.Database;
2+
3+
/// <summary>
4+
/// Implemented by a module <c>DbContext</c> that applies multi-tenant query filters via
5+
/// <see cref="ModuleModelBuilderExtensions.ApplyMultiTenantFilters{TContext}"/>.
6+
/// <para>
7+
/// The filter references this property on the <em>executing</em> context instance, which EF
8+
/// Core re-evaluates on every query. Expose the request's current tenant here (e.g. from an
9+
/// injected scoped <see cref="SimpleModule.Core.Entities.ITenantContext"/>) so each request is
10+
/// filtered by its own tenant, rather than a value frozen into the cached model.
11+
/// </para>
12+
/// </summary>
13+
public interface ITenantScopedDbContext
14+
{
15+
/// <summary>The tenant id for the current request, or <c>null</c> when no tenant is set.</summary>
16+
string? CurrentTenantId { get; }
17+
}

0 commit comments

Comments
 (0)