Skip to content

Commit a074643

Browse files
cemililikclaude
andcommitted
fix(tests): read the spellings SQL and MSBuild actually admit
Five scans answered on a spelling rather than on the thing they name. A schema qualifier hid a composed table name from the audit scan and hid a whole `CREATE TABLE` — name and columns — from the genericity scan, because the name group captured the schema and the pattern then failed at the dot. `OutputItemType='Analyzer'` is valid XML that MSBuild reads, and requiring double quotes reported a correctly wired project as unwired. A policy written in lower case read as having no tenant term, while the sibling scan beside it already folded case. And a type named only by a `typeof(…)` in an attribute argument was invisible to every namespace sweep, because the walker read each attribute's own type and dropped its arguments. The catalogue check compared method names against a flat set of every method in the assembly, so an entry saying "Implemented — FooTests" was satisfied by a method of that name in BarTests — the entry being wrong in the way it is most likely to be wrong. It compares the pair now. Two documentation corrections: the loaders' transaction rationale contradicted itself, claiming a transaction-local setting both expires with its statement and leaks onto the pooled connection afterwards — it is the session-level alternative that would leak, and the transaction exists so the announcement survives to the read that needs it. And a lesson item type key is kebab-case like its siblings. Each fix is fed the shape it missed; the lower-case policy needed a probe of its own, because reverting that one left the suite green without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f138424 commit a074643

11 files changed

Lines changed: 146 additions & 23 deletions

File tree

‎backend/src/LearnStack.Infrastructure.Audit/AuditConfigService.cs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,13 @@ private async Task<IReadOnlyDictionary<string, bool>> OverridesAsync(
142142
/// <para>
143143
/// <c>BEGIN; SET TRANSACTION READ ONLY; SET LOCAL app.tenant_id; SELECT; COMMIT</c> —
144144
/// the announcement is the point, and it is why this cannot ride the request's
145-
/// connection, which has none at step 3. Read-only, so the transaction exists for the
146-
/// <c>SET LOCAL</c> rather than for atomicity: a <c>SET LOCAL</c> outside a transaction
147-
/// lasts for the statement and would leave the pooled connection announcing a tenant
148-
/// afterwards.
145+
/// connection, which has none at step 3. The transaction exists so the announcement is
146+
/// still in effect for the <c>SELECT</c> that follows it, not for atomicity:
147+
/// <c>set_config(…, true)</c> is transaction-local, so outside an explicit transaction it
148+
/// belongs to its own single-statement one and is gone before the read runs. The
149+
/// alternative — announcing at session level — would survive the read and then ride the
150+
/// pooled connection to whoever got it next. Read-only, because neither this nor any
151+
/// reader like it writes.
149152
/// </para>
150153
/// <para>
151154
/// The <c>SELECT</c> names its tenant too, bound from the trusted argument. Row

‎backend/src/Modules/Tenancy/LearnStack.Modules.Tenancy.Infrastructure/FeatureFlags.cs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -202,10 +202,13 @@ private bool Unreadable(FeatureDescriptor descriptor, string raw)
202202
/// <remarks>
203203
/// <para>
204204
/// <c>BEGIN; SET TRANSACTION READ ONLY; SET LOCAL app.tenant_id; SELECT; COMMIT</c> —
205-
/// the announcement is the point. Read-only, so the transaction exists for the
206-
/// <c>SET LOCAL</c> rather than for atomicity: a <c>SET LOCAL</c> outside a transaction
207-
/// lasts for the statement and would leave the pooled connection announcing a tenant
208-
/// afterwards.
205+
/// the announcement is the point. The transaction exists so the announcement is
206+
/// still in effect for the <c>SELECT</c> that follows it, not for atomicity:
207+
/// <c>set_config(…, true)</c> is transaction-local, so outside an explicit transaction it
208+
/// belongs to its own single-statement one and is gone before the read runs. The
209+
/// alternative — announcing at session level — would survive the read and then ride the
210+
/// pooled connection to whoever got it next. Read-only, because neither this nor any
211+
/// reader like it writes.
209212
/// </para>
210213
/// <para>
211214
/// The <c>SELECT</c> names its tenant too, bound from the trusted argument rather than

‎backend/tests/LearnStack.Tests.Architecture/AuditConventionTests.cs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,8 @@ public void The_AuditLog_Write_Scan_Can_Actually_Fail()
358358
ComposedTableName().IsMatch("$\"update {Table} SET tenant_id = @tenant\"").Should().BeTrue(
359359
"a statement's target is its target in either case");
360360
ComposedTableName().IsMatch("$\"copy {Table} FROM STDIN\"").Should().BeTrue();
361+
ComposedTableName().IsMatch("$\"INSERT INTO public.{Table} (id) VALUES (@id)\"").Should().BeTrue(
362+
"a literal schema in front of a composed name composes the same name");
361363
ComposedTableName().IsMatch("SELECT * FROM audit_log WHERE id = @id").Should().BeFalse(
362364
"a literal name is what every other leg here reads");
363365
ComposedTableName().IsMatch("$\"SELECT * FROM audit_log WHERE tenant_id = {tenant}\"").Should().BeFalse(
@@ -431,7 +433,7 @@ private static bool MayNameTheAuditEntry(Type type)
431433
/// </remarks>
432434
[GeneratedRegex(
433435
@"\b(?:(?i:INSERT\s+INTO|MERGE\s+INTO|DELETE\s+FROM|TRUNCATE(?:\s+TABLE)?|COPY|UPDATE)"
434-
+ @"|FROM|JOIN)\s+(?:\{|""\s*\+)")]
436+
+ @"|FROM|JOIN)\s+(?:""?[A-Za-z_][A-Za-z0-9_]*""?\s*\.\s*)?(?:\{|""\s*\+)")]
435437
private static partial Regex ComposedTableName();
436438

437439
/// <summary>

‎backend/tests/LearnStack.Tests.Architecture/ConverterNameProbes.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,16 @@ internal readonly partial struct BorrowedNameProbeId
2727
{
2828
internal sealed class EfCoreValueReader;
2929
}
30+
31+
/// <summary>
32+
/// Names a namespace through a <c>typeof(…)</c> in an attribute argument and nowhere else.
33+
/// </summary>
34+
/// <remarks>
35+
/// The shape <c>Il.Attributes</c> could not see: it read each attribute's own type and
36+
/// dropped the arguments, so a reference written this way was invisible to every namespace
37+
/// sweep built on it. Never instantiated.
38+
/// </remarks>
39+
[System.ComponentModel.DefaultValue(typeof(Microsoft.EntityFrameworkCore.DbContext))]
40+
internal sealed class NamesInAnAttributeArgument
41+
{
42+
}

‎backend/tests/LearnStack.Tests.Architecture/CorpusConsistencyTests.cs‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,13 +88,24 @@ public void Every_Implemented_Rule_Names_A_Test_That_Exists()
8888
"the subject of this rule is every rule the catalogue reports Implemented in this "
8989
+ "assembly, and § Implemented today publishes that number");
9090

91-
var missing = implemented
92-
.Where(rule => !methods.Contains(rule))
91+
// The pair, not the name. A flat set of every method in the assembly let an entry
92+
// saying "Implemented — `FooTests`" be satisfied by a method of that name in
93+
// `BarTests`, which is the entry being wrong in the one way it is most likely to be
94+
// wrong: the rule moved and the Status line did not.
95+
var declared = DeclaredMethods();
96+
97+
var missing = ImplementedEntries()
98+
.Where(entry => NamedClasses(entry.Status).Any(SuiteClasses().Contains))
99+
.Where(entry => !NamedClasses(entry.Status).Any(name =>
100+
declared.TryGetValue(name, out var names) && names.Contains(entry.Rule)))
101+
.Select(entry => entry.Rule)
93102
.ToList();
94103

95104
missing.Should().BeEmpty(
96105
"every rule the catalogue reports Implemented in this assembly is a test method of "
97-
+ "that exact name (Standards 21 § Canonical names and superseded spellings)");
106+
+ "that exact name, in a class its Status line names (Standards 21 § Canonical "
107+
+ "names and superseded spellings)");
108+
98109

99110
// And the other direction of the same failure: an entry naming a test class that exists
100111
// in no suite at all. Without this a renamed or deleted file drops its entries out of
@@ -363,6 +374,16 @@ [.. typeof(CorpusConsistencyTests).Assembly.GetTypes()
363374
private static HashSet<string> TestMethods() =>
364375
[.. TestMethodInfos().Select(method => method.Name)];
365376

377+
/// <summary>The test method names this assembly declares, per declaring class.</summary>
378+
private static Dictionary<string, HashSet<string>> DeclaredMethods() =>
379+
TestMethodInfos()
380+
.Where(method => method.DeclaringType is not null)
381+
.GroupBy(method => method.DeclaringType!.Name, StringComparer.Ordinal)
382+
.ToDictionary(
383+
group => group.Key,
384+
group => group.Select(method => method.Name).ToHashSet(StringComparer.Ordinal),
385+
StringComparer.Ordinal);
386+
366387
/// <summary>The number words the index's summary sentence uses.</summary>
367388
private static readonly Dictionary<string, int> Words = new(StringComparer.OrdinalIgnoreCase)
368389
{

‎backend/tests/LearnStack.Tests.Architecture/CrossCuttingFoundationTests.cs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,9 @@ internal static bool WiresTheAnalyzer(string projectXml)
379379
.Select(match => match.Groups["attributes"].Value)
380380
.Where(attributes => attributes.Contains("LearnStack.Analyzers.csproj", StringComparison.Ordinal))
381381
.Any(attributes =>
382-
Regex.IsMatch(attributes, @"OutputItemType\s*=\s*""Analyzer""")
382+
// Either quote: XML admits both, MSBuild reads both, and requiring double
383+
// ones reported a correctly wired project as unwired.
384+
Regex.IsMatch(attributes, @"OutputItemType\s*=\s*(?:""|')Analyzer(?:""|')")
383385
&& !Regex.IsMatch(attributes, @"\bCondition\s*="));
384386
}
385387

@@ -469,6 +471,8 @@ public static Outcome Value()
469471
WiresTheAnalyzer(Reference + """OutputItemType="Analyzer" />""").Should().BeTrue();
470472
WiresTheAnalyzer("""<ProjectReference OutputItemType="Analyzer" Include="..\LearnStack.Analyzers.csproj" />""")
471473
.Should().BeTrue("the attributes may be written in either order");
474+
WiresTheAnalyzer("""<ProjectReference Include="..\LearnStack.Analyzers.csproj" OutputItemType='Analyzer' />""")
475+
.Should().BeTrue("and in either quote, which is XML's rule and not ours");
472476
WiresTheAnalyzer(Reference + """/>""").Should().BeFalse("a plain reference is not an analyzer");
473477
WiresTheAnalyzer("""<ProjectReference Condition="'$(CI)' == 'true'" Include="..\LearnStack.Analyzers.csproj" OutputItemType="Analyzer" />""")
474478
.Should().BeFalse("a conditional analyzer does not run in the configuration the condition excludes");

‎backend/tests/LearnStack.Tests.Architecture/DomainGenericityTests.cs‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,20 @@ public void The_Domain_Term_Scan_Can_Actually_Fail()
9797
MigratedNames().Should().Contain("outbox_messages")
9898
.And.Contain("idempotency_keys", "a table created in raw SQL belongs to no DbContext");
9999

100+
// A schema qualifier used to take the table AND its columns out of the scan: the name
101+
// group captured the schema, and the pattern then failed at the dot.
102+
var qualified = CreateTable().Match("""
103+
CREATE TABLE public.yoga_levels (
104+
id uuid NOT NULL,
105+
asana_name text NOT NULL
106+
);
107+
""");
108+
qualified.Success.Should().BeTrue();
109+
qualified.Groups["table"].Value.Should().Be("yoga_levels");
110+
ColumnName().Matches(qualified.Groups["columns"].Value)
111+
.Select(match => match.Groups["column"].Value)
112+
.Should().BeEquivalentTo(["id", "asana_name"]);
113+
100114
var created = CreateTable().Match("""
101115
CREATE TABLE kata_sequences (
102116
id uuid NOT NULL,
@@ -493,7 +507,8 @@ private static List<string> Declarators(string code, Match declaration)
493507

494508
/// <summary>A <c>CREATE TABLE</c> statement and the body that declares its columns.</summary>
495509
[GeneratedRegex(
496-
@"CREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?""?(?<table>[A-Za-z_][A-Za-z0-9_]*)""?\s*\((?<columns>.*?)\n\s*\)\s*;",
510+
@"CREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?(?:""?[A-Za-z_][A-Za-z0-9_]*""?\s*\.\s*)?"
511+
+ @"""?(?<table>[A-Za-z_][A-Za-z0-9_]*)""?\s*\((?<columns>.*?)\n\s*\)\s*;",
497512
RegexOptions.Singleline | RegexOptions.IgnoreCase)]
498513
private static partial Regex CreateTable();
499514

‎backend/tests/LearnStack.Tests.Architecture/Il.cs‎

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -247,12 +247,50 @@ private static IEnumerable<TypeReference> ReferencedTypes(TypeDefinition type)
247247
}
248248
}
249249

250+
/// <summary>Every attribute a type carries, and every type those attributes name.</summary>
251+
/// <remarks>
252+
/// The arguments too, not only the attribute's own type. A <c>typeof(…)</c> in an attribute
253+
/// argument — <c>[JsonConverter(typeof(SomeConverter))]</c> — is a reference like any other,
254+
/// and reading the attribute type alone left it out of every namespace sweep built on this.
255+
/// Arrays are walked, because an attribute argument can be one.
256+
/// </remarks>
250257
private static IEnumerable<TypeReference> Attributes(TypeDefinition type) =>
251-
type.CustomAttributes.Select(attribute => attribute.AttributeType)
252-
.Concat(type.Fields.SelectMany(field => field.CustomAttributes)
253-
.Concat(type.Properties.SelectMany(property => property.CustomAttributes))
254-
.Concat(type.Methods.SelectMany(method => method.CustomAttributes))
255-
.Select(attribute => attribute.AttributeType));
258+
type.CustomAttributes
259+
.Concat(type.Fields.SelectMany(field => field.CustomAttributes))
260+
.Concat(type.Properties.SelectMany(property => property.CustomAttributes))
261+
.Concat(type.Methods.SelectMany(method => method.CustomAttributes))
262+
.SelectMany(attribute => new[] { attribute.AttributeType }.Concat(Named(attribute)));
263+
264+
/// <summary>The types one attribute's arguments name.</summary>
265+
private static IEnumerable<TypeReference> Named(CustomAttribute attribute) =>
266+
attribute.ConstructorArguments
267+
.Concat(attribute.Fields.Select(field => field.Argument))
268+
.Concat(attribute.Properties.Select(property => property.Argument))
269+
.SelectMany(NamedBy);
270+
271+
private static IEnumerable<TypeReference> NamedBy(CustomAttributeArgument argument)
272+
{
273+
switch (argument.Value)
274+
{
275+
case TypeReference named:
276+
yield return named;
277+
break;
278+
case CustomAttributeArgument nested:
279+
foreach (var reference in NamedBy(nested))
280+
{
281+
yield return reference;
282+
}
283+
284+
break;
285+
case CustomAttributeArgument[] array:
286+
foreach (var reference in array.SelectMany(NamedBy))
287+
{
288+
yield return reference;
289+
}
290+
291+
break;
292+
}
293+
}
256294

257295
/// <summary>
258296
/// Whether a reference, or any type argument inside it, lives under a namespace.

‎backend/tests/LearnStack.Tests.Architecture/ModuleDependencyTests.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,14 @@ public void The_Domain_EF_Core_Rule_Can_Actually_Fail()
296296
IsVogenEmittedConverter(Probe(module, "LearnStack.Tests.Architecture.Probes.BorrowedNameProbeId/EfCoreValueReader"))
297297
.Should().BeFalse("Vogen emits two names, and a third one inside a value object is written by hand");
298298

299+
// A reference written only as a `typeof(…)` in an attribute argument is a reference.
300+
// Reading the attribute's own type and dropping its arguments left this type looking
301+
// as though it named nothing.
302+
Il.NamesNamespace(
303+
Probe(module, "LearnStack.Tests.Architecture.Probes.NamesInAnAttributeArgument"),
304+
"Microsoft.EntityFrameworkCore")
305+
.Should().BeTrue("an attribute argument names the type it carries");
306+
299307
// And the exception recognises what Vogen actually emits, in every Domain assembly.
300308
DomainAssemblies().Select(assembly => EfCoreDependents(assembly.Location))
301309
.Sum(assembly => assembly.Emitted)

‎backend/tests/LearnStack.Tests.Architecture/TenantScopingTests.cs‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -378,23 +378,39 @@ public void The_Tenant_Term_Check_Can_Actually_Fail()
378378
.Should().BeFalse("a USING that ignores the tenant reads every tenant's rows");
379379
PolicyReadsTheTenant($"CREATE POLICY p ON t USING ({Term}) WITH CHECK ({Term});", "id")
380380
.Should().BeFalse("the self-keyed table compares its own id, not a tenant_id it lacks");
381+
382+
// And the same policy in lower case, which is the same policy. SQL folds case and
383+
// `pg_policies` prints what was written, so a clause-finder keyed on the upper-case
384+
// spelling answers "no tenant term" for a policy that carries one — a structural rule
385+
// reporting a leak that is not there, or missing one that is.
386+
const string Lower = "tenant_id = nullif(current_setting('app.tenant_id', true), '')::uuid";
387+
388+
PolicyReadsTheTenant($"create policy p on t using ({Lower}) with check ({Lower});", "tenant_id")
389+
.Should().BeTrue("a policy written in lower case reads the tenant just as well");
390+
PolicyReadsTheTenant($"create policy p on t using ({Lower}) with check (true);", "tenant_id")
391+
.Should().BeFalse("and it is still refused when one clause ignores the tenant");
381392
}
382393

383394
/// <summary>
384395
/// Whether both clauses of a policy compare the key column to the announced tenant.
385396
/// </summary>
386397
private static bool PolicyReadsTheTenant(string policy, string keyColumn)
387398
{
388-
var usingAt = policy.IndexOf("USING", StringComparison.Ordinal);
389-
var checkAt = policy.IndexOf("WITH CHECK", StringComparison.Ordinal);
399+
// Case-insensitively, as `PermissivePolicyFor` already reads its own keywords: SQL
400+
// folds case, `pg_policies` prints what was written, and a policy spelled `using (…)`
401+
// or naming `current_setting` in another case would otherwise read as absent — a
402+
// structural rule answering "no tenant term" for a policy that has one.
403+
var usingAt = policy.IndexOf("USING", StringComparison.OrdinalIgnoreCase);
404+
var checkAt = policy.IndexOf("WITH CHECK", StringComparison.OrdinalIgnoreCase);
390405

391406
if (usingAt < 0 || checkAt < usingAt)
392407
{
393408
return false;
394409
}
395410

396411
var term = new Regex(
397-
$@"(?<![A-Za-z0-9_]){Regex.Escape(keyColumn)}\s*=\s*NULLIF\(\s*current_setting\(\s*'app\.tenant_id'\s*,\s*true\s*\)\s*,\s*''\s*\)::uuid");
412+
$@"(?<![A-Za-z0-9_]){Regex.Escape(keyColumn)}\s*=\s*NULLIF\(\s*current_setting\(\s*'app\.tenant_id'\s*,\s*true\s*\)\s*,\s*''\s*\)::uuid",
413+
RegexOptions.IgnoreCase);
398414

399415
return term.IsMatch(policy[usingAt..checkAt]) && term.IsMatch(policy[checkAt..]);
400416
}

0 commit comments

Comments
 (0)