From a1d1c6a994f67f48278e32d67dc6848fd07efef0 Mon Sep 17 00:00:00 2001 From: djgosnell Date: Tue, 14 Jul 2026 11:16:33 -0400 Subject: [PATCH 1/9] Fix ProjectSchemaReader to honor NamingStyle and MapTo (#324) The migration tool's ProjectSchemaReader read the naming style from a property literally named 'Naming' (real API is NamingStyle) and never parsed MapTo, so snapshots/DDL used C# property names while the runtime uses physical names. - ExtractTableDef: detect the real NamingStyle override, parsing the enum value from the expression/getter body (mirrors runtime SchemaParser). - ExtractColumnDef: parse MapTo (chained .MapTo and standalone MapTo, the latter via a new GenericNameSyntax case); set the physical column name (MapTo ?? styled(prop)) and populate ColumnDef.MappedName. - Ref FK constraint column now reuses the resolved column name so it stays consistent when a Ref uses MapTo/NamingStyle. - De-mask the 3 ProjectSchemaReaderIndexTests naming tests: the fictional 'public NamingStyle Naming' becomes the real 'protected override NamingStyle NamingStyle', so they now exercise the real API. Step 1 of 4. --- .../324-migration-honor-naming-mapto/plan.md | 71 +++++++++++++++++++ .../workflow.md | 53 ++++++++++++++ .../ProjectSchemaReaderIndexTests.cs | 6 +- src/Quarry.Tool/Schema/ProjectSchemaReader.cs | 43 +++++++++-- 4 files changed, 163 insertions(+), 10 deletions(-) create mode 100644 _sessions/324-migration-honor-naming-mapto/plan.md create mode 100644 _sessions/324-migration-honor-naming-mapto/workflow.md diff --git a/_sessions/324-migration-honor-naming-mapto/plan.md b/_sessions/324-migration-honor-naming-mapto/plan.md new file mode 100644 index 00000000..f33c18a4 --- /dev/null +++ b/_sessions/324-migration-honor-naming-mapto/plan.md @@ -0,0 +1,71 @@ +# Plan: 324 — Migration tool honors NamingStyle and MapTo + +## Goal +Make the migration tool's `ProjectSchemaReader` honor the `NamingStyle` override and per-column +`MapTo("physical")`, so `quarry migrate add`/`diff` produce snapshots and DDL whose column names match +the runtime's physical names — and so adding/removing a mapping produces a real diff step, not a silent +no-op. + +## Representation (the key concept) +`ColumnDef.Name` becomes the **physical** column name (`MapTo-arg ?? ToColumnName(prop.Name, style)`), +and `ColumnDef.MappedName` is set to the `MapTo` argument (or null). This is required because the +migration DDL is generated from `col.Name` (`MigrationCodeGenerator` → `DdlRenderer`), which ignore +`MappedName`. `SchemaDiffer` keys columns by `Name`, so a changed physical name naturally yields +add/drop/rename steps instead of a no-op. Populating `MappedName` keeps the snapshot artifact's +`.MapTo(...)` and the `SchemaHasher`/`RenameMatcher` inputs faithful. See workflow.md Decisions. + +## Steps + +- [x] **Step 1 — `ProjectSchemaReader`: honor `NamingStyle` + `MapTo` (+ Ref FK consistency), and de-mask the 3 tests.** + - `ExtractTableDef`: change the naming-style detection from the fictional `prop.Name == "Naming"` to the + real `prop.Name == "NamingStyle"`. Parse the value from the expression body (and getter-arrow) as a + member access (`NamingStyle.SnakeCase` etc.) → `NamingStyleKind`, keeping `Exact` as default. Mirror + runtime `SchemaParser.ExtractNamingStyle`. + - `ExtractColumnDef`: add a `MapTo` case to the fluent-chain walk that extracts the first string-literal + argument → `mappedName`. Extend the inline method-name extraction to also handle `GenericNameSyntax` + (so standalone `MapTo("x")` is recognized, not only chained `.MapTo("x")`). Compute + `columnName = mappedName ?? ToColumnName(prop.Name, namingStyle)` and pass `mappedName:` to the + `ColumnDef` constructor. + - `ExtractTableDef` Ref branch: use the resolved column name (`colDef?.Name ?? ToColumnName(...)`) for the + FK constraint column so a MapTo'd/styled Ref column and its FK stay consistent. + - Update the 3 masking tests (`ProjectSchemaReaderIndexTests.cs` lines ~90, ~377, ~397): + `public NamingStyle Naming => NamingStyle.SnakeCase;` → `protected override NamingStyle NamingStyle => NamingStyle.SnakeCase;`. + (These must change together with the reader fix, else they'd fail.) + - Tests: full existing suite stays green; the de-masked tests now exercise the real API. + - Depends on: nothing. + +- [ ] **Step 2 — Unit coverage: `NamingStyle` and `MapTo` honored (new test file `ProjectSchemaReaderNamingMapToTests.cs`).** + - `NamingStyle.SnakeCase` via the real override → `TableDef.NamingStyle == SnakeCase`, columns styled + (`UserName` → `user_name`), `MappedName == null`. + - `MapTo` chained (`Col X => Mapped<…>().MapTo("x")` and a plain `Identity().MapTo(...)`/`Length().MapTo(...)`) + → `Name == "x"`, `MappedName == "x"`. + - `MapTo` standalone generic (`Col X => MapTo("x")`) → `Name == "x"`, `MappedName == "x"`. + - `MapTo` overrides `NamingStyle` (snake-case schema + a column with `MapTo("explicit")`) → `Name == "explicit"`. + - No-MapTo / `Exact` → `Name == "PropertyName"`, `MappedName == null` (regression guard). + - Depends on: Step 1. + +- [ ] **Step 3 — Diff coverage: add/remove `MapTo` is not a no-op.** + - Extract v1 (column with `MapTo("credit_limit")`) and v2 (same column without `MapTo`) via + `ProjectSchemaReader`; run `SchemaDiffer.Diff(v1, v2)` and assert the result is **non-empty** and + contains a `RenameColumn` (or a drop+add) for that column — never zero steps. Assert the reverse + direction (add a mapping) likewise produces steps. + - Depends on: Step 1. + +- [ ] **Step 4 — End-to-end guard: `AccountSchema`/`credit_limit` physical-name parity.** + - Extract the **real committed** `AccountSchema` via `ProjectSchemaReader` (compile its source with the + minimal supporting types it needs — `Money`, `MoneyMapping`, `UserSchema`; prefer the real sample + sources, fall back to inline stubs only if the dependency graph is impractical — decision recorded in + Working Notes at implementation time). Assert `CreditLimit` column `Name == "credit_limit"` & + `MappedName == "credit_limit"`, and `Balance` (Mapped, no MapTo) `Name == "Balance"` & `MappedName == null`. + - Also assert the generated migration/snapshot code for this schema contains `credit_limit` (ties the + tool's DDL-bound column name to the runtime's physical name; the runtime side is already covered by + `CrossDialectSchemaTests.Select_MapToColumn_CreditLimit`). + - Depends on: Step 1. + +## Non-goals / notes +- No change to `SchemaDiffer`, `DdlRenderer`, `MigrationCodeGenerator`, `SnapshotCodeGenerator`, or the + `ColumnDef` model — they already consume `Name`/`MappedName` correctly. +- `Quarry.Migration/SchemaResolver` and `Quarry.Generator/SchemaParser` already honor NamingStyle+MapTo; + untouched. +- Behavioral-correction note (projects that previously ran `migrate add` on a NamingStyle/MapTo schema + now see a physical-name diff on their next `migrate add`) goes in the PR body — no CHANGELOG file exists. diff --git a/_sessions/324-migration-honor-naming-mapto/workflow.md b/_sessions/324-migration-honor-naming-mapto/workflow.md new file mode 100644 index 00000000..c31fc5b3 --- /dev/null +++ b/_sessions/324-migration-honor-naming-mapto/workflow.md @@ -0,0 +1,53 @@ +# Workflow: 324-migration-honor-naming-mapto +## Config +platform: github +base-branch: master +## State +phase: IMPLEMENT +status: active +issue: #324 +pr: +## Problem Statement +Migration tooling (`quarry migrate add`/`diff` via `ProjectSchemaReader`) does not honor two +column-naming mechanisms the runtime source generator honors: +- `NamingStyle` override (e.g. `NamingStyle.SnakeCase`) +- per-column `MapTo("physical_name")` + +Result: snapshots/DDL use C# property names while runtime uses physical names → migrations create +columns the runtime never queries; adding/removing a mapping produces zero diff steps (silent no-op). + +Root causes (per issue #324): +- `ProjectSchemaReader.ExtractTableDef` reads a property literally named `"Naming"` (~line 113); real + API is `NamingStyle`. Override never detected → stays `NamingStyleKind.Exact`. +- `ProjectSchemaReader.ExtractColumnDef` (~231–319) recognizes only `Computed`/`Collation`; no `MapTo` + case, so `ColumnDef.MappedName` is always null. +- `DatabaseSchemaReader.NormalizeForDiff` (~218) rebuilds columns dropping `MappedName`; diff keys off + `Name` only. +- Masking tests: `ProjectSchemaReaderIndexTests.cs` naming tests declare a fictional `NamingStyle Naming` + property matching the buggy check, so they pass without exercising the real API. + +### Baseline (2026-07-14, INTAKE) +Full suite green: Quarry.Tests 3388, Quarry.Migration.Tests 201, Quarry.Analyzers.Tests 146. No pre-existing failures. + +## Decisions +| Date | Decision | +|------|----------| +| 2026-07-14 | **Representation = Option A** (issue step 3 "simplest"): `ColumnDef.Name` = physical name (`MapTo-arg ?? ToColumnName(prop.Name, style)`); `MappedName` = the `MapTo` argument (or null). Rationale: `MigrationCodeGenerator` (`t.Column(col.Name,…)`) and `DdlRenderer` (`col.Name`) render `Name` and **ignore `MappedName`**, so the physical DDL name must live in `Name`. Populating `MappedName` too keeps `SnapshotCodeGenerator` re-emitting `.MapTo(...)`, and keeps `SchemaHasher`/`RenameMatcher` (both consume `MappedName`) aligned. Option B (Name=logical, MappedName=physical) would additionally require changing MigrationCodeGenerator + builder→operation path — larger blast radius, rejected. | +| 2026-07-14 | **NamingStyle detection**: match the real property name `"NamingStyle"` (was `"Naming"`), parsing expression-body (and getter-arrow) member access → `NamingStyleKind`, keeping `Exact` default. Mirrors runtime `SchemaParser.ExtractNamingStyle`. | +| 2026-07-14 | **MapTo forms**: handle both chained `.MapTo("x")` and standalone generic `MapTo("x")`. Extend `ProjectSchemaReader`'s inline method-name extraction to also handle `GenericNameSyntax` (currently only MemberAccess/Identifier), mirroring runtime `GetMethodName`. | +| 2026-07-14 | **Ref FK column consistency**: in `ExtractTableDef`'s `Ref` branch, use the resolved column name (`colDef.Name`) for the FK constraint column instead of recomputing `ToColumnName(prop.Name,…)` (which ignores MapTo). Keeps FK column == actual column when a Ref uses MapTo. Fallback to recompute if colDef is null. | +| 2026-07-14 | **Issue bullet 3 is stale**: `DatabaseSchemaReader.cs` / `NormalizeForDiff` do not exist. The diff keys columns off `ColumnDef.Name` in `SchemaDiffer.DiffColumns`; no change needed there — once `Name` carries the physical name, add/remove-MapTo diffs stop being no-ops. | +| 2026-07-14 | **No changelog file exists** — behavioral-correction note goes in the PR body instead of a CHANGELOG. | +## Working Notes +- **Migration snapshot pipeline** (the buggy path): `MigrateCommands` → `ProjectSchemaReader.ExtractSchemaSnapshot` → `SchemaSnapshot` → `SchemaDiffer.Diff` (keys columns by `ColumnDef.Name`, threads `MappedName` through `Equals`/`RenameMatcher`) → `MigrationStep` → `MigrationCodeGenerator` (`t.Column(col.Name,…)`, **no MapTo**) → runtime `MigrationBuilder` → `MigrationOperation`(`ColumnDefinition`) → `DdlRenderer` (`col.Name`). Snapshot artifact round-trips via `SnapshotCodeGenerator` (`.Name()`+`.MapTo()`) ↔ `SnapshotCompiler` (Shared `ColumnDefBuilder.Name`/`.MapTo`). +- **Three schema readers exist** — only ProjectSchemaReader (tool) is buggy. `Quarry.Generator/SchemaParser` (runtime SQL) and `Quarry.Migration/SchemaResolver` (SchemaMap for result materialization) both already honor NamingStyle+MapTo correctly. SchemaResolver output type (`SchemaMap`) is unrelated to the migration snapshot path. +- **`MigrationCodeGenerator` ignores `MappedName` entirely** (lines 126–135, 186–189) → this is *why* Option A (Name=physical) is required, not just convenient. +- **3 masking tests**, not 2: `ProjectSchemaReaderIndexTests.cs` lines 90, 377, 397 declare fictional `public NamingStyle Naming => …`. `SchemaTests.cs:30` already uses the real `protected override NamingStyle NamingStyle =>` (tests runtime `Schema` property — unaffected). +- **MapTo API forms** (verified): `Schema.MapTo(string)` standalone (`Col X => MapTo("x")`), and `.MapTo(string)` on `ColumnBuilder`/`RefBuilder` chained (`… .Mapped<…>().MapTo("x")`). AccountSchema.CreditLimit uses the chained form. +- **Round-trip is consistent** either way (with/without MappedName), but MappedName is populated per Decision above. +## Suspend State +## Session Log +| Date | Phases | Summary | +|------|--------|---------| +| 2026-07-14 | INTAKE→DESIGN | Loaded issue #324, created worktree/branch, clean test baseline. | +| 2026-07-14 | DESIGN→PLAN→IMPLEMENT | Verified full migration pipeline + 3 schema readers; fast-path design+plan approved (all 4 steps). | diff --git a/src/Quarry.Tests/Migration/ProjectSchemaReaderIndexTests.cs b/src/Quarry.Tests/Migration/ProjectSchemaReaderIndexTests.cs index 28b6f612..1f3a89f6 100644 --- a/src/Quarry.Tests/Migration/ProjectSchemaReaderIndexTests.cs +++ b/src/Quarry.Tests/Migration/ProjectSchemaReaderIndexTests.cs @@ -87,7 +87,7 @@ public void ExtractSchema_NamingStyleSnakeCase_AppliedToColumnNames() public class UserSchema : Schema { public static string Table => ""users""; - public NamingStyle Naming => NamingStyle.SnakeCase; + protected override NamingStyle NamingStyle => NamingStyle.SnakeCase; public Key UserId { get; } public Col UserName { get; } }"; @@ -374,7 +374,7 @@ public void ExtractIndex_SnakeCaseNaming_ConvertsColumnNames() public class UserSchema : Schema { public static string Table => ""users""; - public NamingStyle Naming => NamingStyle.SnakeCase; + protected override NamingStyle NamingStyle => NamingStyle.SnakeCase; public Key UserId { get; } public Col UserName { get; } @@ -394,7 +394,7 @@ public void ExtractIndex_SnakeCaseNaming_WhereBoolColumnConverted() public class UserSchema : Schema { public static string Table => ""users""; - public NamingStyle Naming => NamingStyle.SnakeCase; + protected override NamingStyle NamingStyle => NamingStyle.SnakeCase; public Key UserId { get; } public Col Email { get; } public Col IsActive { get; } diff --git a/src/Quarry.Tool/Schema/ProjectSchemaReader.cs b/src/Quarry.Tool/Schema/ProjectSchemaReader.cs index 254e7f3b..b9cfe5a5 100644 --- a/src/Quarry.Tool/Schema/ProjectSchemaReader.cs +++ b/src/Quarry.Tool/Schema/ProjectSchemaReader.cs @@ -110,19 +110,29 @@ private static bool InheritsFromSchema(INamedTypeSymbol symbol) characterSet = literal2.Token.ValueText; } } - else if (prop.Name == "Naming") + else if (prop.Name == "NamingStyle") { + // Mirror the runtime SchemaParser: the real API is the overridable + // Schema.NamingStyle property, e.g. `protected override NamingStyle NamingStyle => NamingStyle.SnakeCase;`. var syntax = prop.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(); if (syntax is PropertyDeclarationSyntax propSyntax) { var expr = propSyntax.Initializer?.Value ?? propSyntax.ExpressionBody?.Expression; - if (expr != null) + if (expr == null && propSyntax.AccessorList != null) { - var exprText = expr.ToString(); - if (exprText.Contains("SnakeCase")) namingStyle = NamingStyleKind.SnakeCase; - else if (exprText.Contains("CamelCase")) namingStyle = NamingStyleKind.CamelCase; - else if (exprText.Contains("LowerCase")) namingStyle = NamingStyleKind.LowerCase; + var getter = propSyntax.AccessorList.Accessors + .FirstOrDefault(a => a.IsKind(SyntaxKind.GetAccessorDeclaration)); + expr = getter?.ExpressionBody?.Expression; } + + var valueName = (expr as MemberAccessExpressionSyntax)?.Name.Identifier.Text; + namingStyle = valueName switch + { + "SnakeCase" => NamingStyleKind.SnakeCase, + "CamelCase" => NamingStyleKind.CamelCase, + "LowerCase" => NamingStyleKind.LowerCase, + _ => NamingStyleKind.Exact + }; } } } @@ -148,7 +158,9 @@ private static bool InheritsFromSchema(INamedTypeSymbol symbol) { var refEntityType = colType.TypeArguments[0] as INamedTypeSymbol; var refEntity = refEntityType?.Name ?? colType.TypeArguments[0].Name; - var colName = NamingConventions.ToColumnName(colProp.Name, namingStyle); + // Use the resolved column name (honors MapTo/NamingStyle) so the FK + // constraint column stays consistent with the actual column name. + var colName = colDef?.Name ?? NamingConventions.ToColumnName(colProp.Name, namingStyle); // Resolve PK column from referenced entity var refPkColumn = ResolvePrimaryKeyColumn(refEntityType, namingStyle); @@ -267,6 +279,7 @@ private static string ResolvePrimaryKeyColumn(INamedTypeSymbol? entityType, Nami // Walk fluent chain for additional modifiers string? computedExpression = null; string? collation = null; + string? mappedName = null; var syntax = prop.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(); if (syntax is PropertyDeclarationSyntax propSyntax) @@ -282,10 +295,13 @@ private static string ResolvePrimaryKeyColumn(INamedTypeSymbol? entityType, Nami var current = expression; while (current is InvocationExpressionSyntax invocation) { + // GenericNameSyntax covers the standalone generic form `MapTo("...")`; + // MemberAccess covers the chained `....MapTo("...")` form. var methodName = invocation.Expression switch { MemberAccessExpressionSyntax ma => ma.Name.Identifier.Text, IdentifierNameSyntax id => id.Identifier.Text, + GenericNameSyntax gen => gen.Identifier.Text, _ => null }; @@ -301,6 +317,12 @@ private static string ResolvePrimaryKeyColumn(INamedTypeSymbol? entityType, Nami if (arg is LiteralExpressionSyntax literal) collation = literal.Token.ValueText; } + else if (methodName == "MapTo" && invocation.ArgumentList.Arguments.Count > 0) + { + var arg = invocation.ArgumentList.Arguments[0].Expression; + if (arg is LiteralExpressionSyntax literal) + mappedName = literal.Token.ValueText; + } if (invocation.Expression is MemberAccessExpressionSyntax memberAccess) current = memberAccess.Expression; @@ -309,11 +331,18 @@ private static string ResolvePrimaryKeyColumn(INamedTypeSymbol? entityType, Nami } } + // A per-column MapTo("physical") overrides the naming-style-derived name. Mirrors the + // runtime SchemaParser (columnName = MappedName ?? ToColumnName(prop, style)) so the + // migration snapshot/DDL uses the same physical name the runtime queries. + if (mappedName != null) + columnName = mappedName; + return new ColumnDef( name: columnName, clrType: clrType, isNullable: isNullable, kind: kind, + mappedName: mappedName, referencedEntityName: referencedEntity, computedExpression: computedExpression, collation: collation); From f49ce868e8613c39c642d275f7550e70874013c4 Mon Sep 17 00:00:00 2001 From: djgosnell Date: Tue, 14 Jul 2026 11:19:34 -0400 Subject: [PATCH 2/9] Add NamingStyle/MapTo extraction unit tests (#324) Covers: NamingStyle.SnakeCase styling with null MappedName; MapTo in standalone generic (MapTo), chained (.MapTo after a modifier), and generic-not-outermost forms; MapTo overriding the naming style per column; and an Exact/no-MapTo regression guard. Step 2 of 4. --- .../324-migration-honor-naming-mapto/plan.md | 2 +- .../ProjectSchemaReaderNamingMapToTests.cs | 180 ++++++++++++++++++ 2 files changed, 181 insertions(+), 1 deletion(-) create mode 100644 src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs diff --git a/_sessions/324-migration-honor-naming-mapto/plan.md b/_sessions/324-migration-honor-naming-mapto/plan.md index f33c18a4..b2c82596 100644 --- a/_sessions/324-migration-honor-naming-mapto/plan.md +++ b/_sessions/324-migration-honor-naming-mapto/plan.md @@ -34,7 +34,7 @@ add/drop/rename steps instead of a no-op. Populating `MappedName` keeps the snap - Tests: full existing suite stays green; the de-masked tests now exercise the real API. - Depends on: nothing. -- [ ] **Step 2 — Unit coverage: `NamingStyle` and `MapTo` honored (new test file `ProjectSchemaReaderNamingMapToTests.cs`).** +- [x] **Step 2 — Unit coverage: `NamingStyle` and `MapTo` honored (new test file `ProjectSchemaReaderNamingMapToTests.cs`).** - `NamingStyle.SnakeCase` via the real override → `TableDef.NamingStyle == SnakeCase`, columns styled (`UserName` → `user_name`), `MappedName == null`. - `MapTo` chained (`Col X => Mapped<…>().MapTo("x")` and a plain `Identity().MapTo(...)`/`Length().MapTo(...)`) diff --git a/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs b/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs new file mode 100644 index 00000000..306f0f63 --- /dev/null +++ b/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs @@ -0,0 +1,180 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Quarry.Shared.Migration; +using Quarry.Tool.Schema; + +namespace Quarry.Tests.Migration; + +/// +/// Verifies that honors the real NamingStyle override and +/// per-column MapTo("physical") — the two column-naming mechanisms the runtime source generator +/// honors — so migration snapshots/DDL use the same physical names the runtime queries (issue #324). +/// +[TestFixture] +public class ProjectSchemaReaderNamingMapToTests +{ + private static readonly string QuarryCoreAssemblyPath = typeof(Schema).Assembly.Location; + private static readonly string SystemRuntimeAssemblyPath = typeof(object).Assembly.Location; + + private static CSharpCompilation CreateCompilation(params string[] sources) + { + var parseOptions = new CSharpParseOptions(LanguageVersion.Latest); + var syntaxTrees = sources.Select(s => CSharpSyntaxTree.ParseText(s, parseOptions)).ToList(); + + var references = new List + { + MetadataReference.CreateFromFile(QuarryCoreAssemblyPath), + MetadataReference.CreateFromFile(SystemRuntimeAssemblyPath), + MetadataReference.CreateFromFile(typeof(System.Data.IDbConnection).Assembly.Location), + }; + + var runtimeDir = System.Runtime.InteropServices.RuntimeEnvironment.GetRuntimeDirectory(); + references.Add(MetadataReference.CreateFromFile(Path.Combine(runtimeDir, "System.Runtime.dll"))); + references.Add(MetadataReference.CreateFromFile(Path.Combine(runtimeDir, "System.Collections.dll"))); + references.Add(MetadataReference.CreateFromFile(Path.Combine(runtimeDir, "System.Linq.dll"))); + references.Add(MetadataReference.CreateFromFile(Path.Combine(runtimeDir, "System.Linq.Expressions.dll"))); + references.Add(MetadataReference.CreateFromFile(Path.Combine(runtimeDir, "netstandard.dll"))); + + return CSharpCompilation.Create( + "TestAssembly", + syntaxTrees, + references, + new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary) + .WithNullableContextOptions(NullableContextOptions.Enable)); + } + + private static TableDef ExtractSingleTable(string source) + { + var compilation = CreateCompilation(source); + var snapshot = ProjectSchemaReader.ExtractSchemaSnapshot(compilation, 1, "test", null); + Assert.That(snapshot.Tables, Has.Count.EqualTo(1), "Expected exactly one table"); + return snapshot.Tables[0]; + } + + private static ColumnDef Column(TableDef table, string name) => + table.Columns.Single(c => c.Name == name); + + [Test] + public void NamingStyle_SnakeCase_StylesColumnNames_AndLeavesMappedNameNull() + { + var source = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + protected override NamingStyle NamingStyle => NamingStyle.SnakeCase; + public Key UserId { get; } + public Col UserName { get; } +}"; + var table = ExtractSingleTable(source); + + Assert.That(table.NamingStyle, Is.EqualTo(NamingStyleKind.SnakeCase)); + Assert.That(Column(table, "user_id").MappedName, Is.Null); + Assert.That(Column(table, "user_name").MappedName, Is.Null); + } + + [Test] + public void MapTo_StandaloneGeneric_SetsPhysicalNameAndMappedName() + { + var source = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + public Key UserId { get; } + public Col UserName => MapTo(""user_name""); +}"; + var table = ExtractSingleTable(source); + + var col = Column(table, "user_name"); + Assert.That(col.Name, Is.EqualTo("user_name")); + Assert.That(col.MappedName, Is.EqualTo("user_name")); + // The un-mapped property name must not survive as a column. + Assert.That(table.Columns.Any(c => c.Name == "UserName"), Is.False); + } + + [Test] + public void MapTo_ChainedAfterModifier_SetsPhysicalNameAndMappedName() + { + var source = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + public Key UserId { get; } + public Col AccountName => Length(100).MapTo(""account_name""); +}"; + var table = ExtractSingleTable(source); + + var col = Column(table, "account_name"); + Assert.That(col.Name, Is.EqualTo("account_name")); + Assert.That(col.MappedName, Is.EqualTo("account_name")); + } + + [Test] + public void MapTo_GenericNotOutermostInChain_StillDetected() + { + // MapTo(...) is the inner call; a trailing modifier makes it non-outermost. + var source = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + public Key UserId { get; } + public Col UserName => MapTo(""user_name"").Unique(); +}"; + var table = ExtractSingleTable(source); + + var col = Column(table, "user_name"); + Assert.That(col.MappedName, Is.EqualTo("user_name")); + } + + [Test] + public void MapTo_OverridesNamingStyle_ForThatColumnOnly() + { + var source = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + protected override NamingStyle NamingStyle => NamingStyle.SnakeCase; + public Key UserId { get; } + public Col FirstName { get; } + public Col UserName => MapTo(""explicit_name""); +}"; + var table = ExtractSingleTable(source); + + // Sibling without MapTo is styled by the naming convention. + Assert.That(Column(table, "first_name").MappedName, Is.Null); + // MapTo wins over the naming style for its column. + var mapped = Column(table, "explicit_name"); + Assert.That(mapped.MappedName, Is.EqualTo("explicit_name")); + Assert.That(table.Columns.Any(c => c.Name == "user_name"), Is.False); + } + + [Test] + public void NoMapTo_ExactNaming_UsesPropertyNameAndNullMappedName() + { + var source = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + public Key UserId { get; } + public Col UserName { get; } + public Col Email => Length(100); +}"; + var table = ExtractSingleTable(source); + + Assert.That(table.NamingStyle, Is.EqualTo(NamingStyleKind.Exact)); + Assert.That(Column(table, "UserName").MappedName, Is.Null); + // A modifier chain without MapTo must not fabricate a mapped name. + Assert.That(Column(table, "Email").MappedName, Is.Null); + } +} From 61a159be683b1fe467a3ab5d1fda94edfd7834fd Mon Sep 17 00:00:00 2001 From: djgosnell Date: Tue, 14 Jul 2026 11:21:50 -0400 Subject: [PATCH 3/9] Add diff test: MapTo add/remove is not a silent no-op (#324) Extracts snapshots via ProjectSchemaReader with and without MapTo and asserts SchemaDiffer.Diff produces a RenameColumn (or drop+add) in both directions, never zero steps. Step 3 of 4. --- .../324-migration-honor-naming-mapto/plan.md | 2 +- .../ProjectSchemaReaderNamingMapToTests.cs | 82 +++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/_sessions/324-migration-honor-naming-mapto/plan.md b/_sessions/324-migration-honor-naming-mapto/plan.md index b2c82596..d354bfd5 100644 --- a/_sessions/324-migration-honor-naming-mapto/plan.md +++ b/_sessions/324-migration-honor-naming-mapto/plan.md @@ -44,7 +44,7 @@ add/drop/rename steps instead of a no-op. Populating `MappedName` keeps the snap - No-MapTo / `Exact` → `Name == "PropertyName"`, `MappedName == null` (regression guard). - Depends on: Step 1. -- [ ] **Step 3 — Diff coverage: add/remove `MapTo` is not a no-op.** +- [x] **Step 3 — Diff coverage: add/remove `MapTo` is not a no-op.** - Extract v1 (column with `MapTo("credit_limit")`) and v2 (same column without `MapTo`) via `ProjectSchemaReader`; run `SchemaDiffer.Diff(v1, v2)` and assert the result is **non-empty** and contains a `RenameColumn` (or a drop+add) for that column — never zero steps. Assert the reverse diff --git a/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs b/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs index 306f0f63..66058f38 100644 --- a/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs +++ b/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs @@ -54,6 +54,88 @@ private static TableDef ExtractSingleTable(string source) private static ColumnDef Column(TableDef table, string name) => table.Columns.Single(c => c.Name == name); + private static SchemaSnapshot ExtractSnapshot(string source, int version) => + ProjectSchemaReader.ExtractSchemaSnapshot(CreateCompilation(source), version, "test", null); + + // A mapping change must surface as either a single RenameColumn or a drop+add pair — + // never zero steps. Threshold-dependent rename scoring means we accept either shape. + private static void AssertColumnTransition( + IReadOnlyList steps, string oldName, string newName) + { + Assert.That(steps, Is.Not.Empty, "Expected a diff step for the mapping change, got a no-op"); + + var asRename = steps.Any(s => + s.StepType == MigrationStepType.RenameColumn + && (s.OldValue as string) == oldName + && (s.NewValue as string) == newName); + + var asDropAdd = + steps.Any(s => s.StepType == MigrationStepType.DropColumn && s.ColumnName == oldName) + && steps.Any(s => s.StepType == MigrationStepType.AddColumn && s.ColumnName == newName); + + Assert.That(asRename || asDropAdd, Is.True, + $"Expected a rename {oldName}->{newName} or a drop+add, but got: " + + string.Join(", ", steps.Select(s => $"{s.StepType}({s.ColumnName})"))); + } + + [Test] + public void RemovingMapTo_ProducesDiffStep_NotNoOp() + { + const string withMapTo = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + public Key UserId { get; } + public Col UserName => MapTo(""user_name""); +}"; + const string withoutMapTo = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + public Key UserId { get; } + public Col UserName { get; } +}"; + var v1 = ExtractSnapshot(withMapTo, 1); + var v2 = ExtractSnapshot(withoutMapTo, 2); + + var steps = SchemaDiffer.Diff(v1, v2); + + AssertColumnTransition(steps, "user_name", "UserName"); + } + + [Test] + public void AddingMapTo_ProducesDiffStep_NotNoOp() + { + const string withoutMapTo = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + public Key UserId { get; } + public Col UserName { get; } +}"; + const string withMapTo = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + public Key UserId { get; } + public Col UserName => MapTo(""user_name""); +}"; + var v1 = ExtractSnapshot(withoutMapTo, 1); + var v2 = ExtractSnapshot(withMapTo, 2); + + var steps = SchemaDiffer.Diff(v1, v2); + + AssertColumnTransition(steps, "UserName", "user_name"); + } + [Test] public void NamingStyle_SnakeCase_StylesColumnNames_AndLeavesMappedNameNull() { From 240376e734e6edc3e7f2496a4bb1be85ad2bae5c Mon Sep 17 00:00:00 2001 From: djgosnell Date: Tue, 14 Jul 2026 11:27:39 -0400 Subject: [PATCH 4/9] Add end-to-end MapTo parity guard for AccountSchema (#324) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compiles the real committed AccountSchema.cs + Money.cs (loaded via CallerFilePath) with a minimal UserSchema stub, and asserts the tool extracts credit_limit (not CreditLimit) and that the generated migration code carries the physical name — matching the runtime SQL already covered by CrossDialectSchemaTests.Select_MapToColumn_CreditLimit. Step 4 of 4. --- .../324-migration-honor-naming-mapto/plan.md | 5 +- .../ProjectSchemaReaderNamingMapToTests.cs | 47 +++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/_sessions/324-migration-honor-naming-mapto/plan.md b/_sessions/324-migration-honor-naming-mapto/plan.md index d354bfd5..6d11b654 100644 --- a/_sessions/324-migration-honor-naming-mapto/plan.md +++ b/_sessions/324-migration-honor-naming-mapto/plan.md @@ -51,7 +51,10 @@ add/drop/rename steps instead of a no-op. Populating `MappedName` keeps the snap direction (add a mapping) likewise produces steps. - Depends on: Step 1. -- [ ] **Step 4 — End-to-end guard: `AccountSchema`/`credit_limit` physical-name parity.** +- [x] **Step 4 — End-to-end guard: `AccountSchema`/`credit_limit` physical-name parity.** + Implemented by compiling the real `AccountSchema.cs` + real `Money.cs` (loaded via `[CallerFilePath]` + sibling `../Samples`) with a minimal `UserSchema` stub; asserts extracted `credit_limit`/`Balance` + and that `MigrationCodeGenerator` output contains `"credit_limit"` and not `"CreditLimit"`. - Extract the **real committed** `AccountSchema` via `ProjectSchemaReader` (compile its source with the minimal supporting types it needs — `Money`, `MoneyMapping`, `UserSchema`; prefer the real sample sources, fall back to inline stubs only if the dependency graph is impractical — decision recorded in diff --git a/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs b/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs index 66058f38..8d68db4c 100644 --- a/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs +++ b/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs @@ -1,3 +1,4 @@ +using System.Runtime.CompilerServices; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Quarry.Shared.Migration; @@ -259,4 +260,50 @@ public class UserSchema : Schema // A modifier chain without MapTo must not fabricate a mapped name. Assert.That(Column(table, "Email").MappedName, Is.Null); } + + // Loads a committed sample source file (sibling ../Samples of this test file) so the guard + // tracks the REAL AccountSchema, not a copy that could silently drift. + private static string ReadSampleSource(string fileName, [CallerFilePath] string thisFilePath = "") + { + var dir = Path.GetDirectoryName(thisFilePath)!; + return File.ReadAllText(Path.Combine(dir, "..", "Samples", fileName)); + } + + [Test] + public void AccountSchema_MapTo_YieldsPhysicalCreditLimit_MatchingRuntime() + { + // Real AccountSchema + real Money/MoneyMapping; UserSchema is stubbed to the minimum the + // Ref needs (the real one drags in the whole sample graph). AccountSchema itself is verbatim. + var accountSchema = ReadSampleSource("AccountSchema.cs"); + var money = ReadSampleSource("Money.cs"); + const string userSchemaStub = @" +using Quarry; +namespace Quarry.Tests.Samples; +public class UserSchema : Schema +{ + public static string Table => ""users""; + public Key UserId => Identity(); +}"; + var compilation = CreateCompilation(accountSchema, money, userSchemaStub); + var snapshot = ProjectSchemaReader.ExtractSchemaSnapshot(compilation, 1, "AccountsInit", null); + + var accounts = snapshot.Tables.Single(t => t.TableName == "accounts"); + + // CreditLimit => Mapped().MapTo("credit_limit") + var creditLimit = accounts.Columns.Single(c => c.Name == "credit_limit"); + Assert.That(creditLimit.MappedName, Is.EqualTo("credit_limit")); + Assert.That(accounts.Columns.Any(c => c.Name == "CreditLimit"), Is.False, + "The property name must not survive; the runtime queries the physical 'credit_limit'."); + + // Balance => Mapped() with no MapTo keeps its Exact property name. + Assert.That(accounts.Columns.Single(c => c.Name == "Balance").MappedName, Is.Null); + + // The migration DDL is generated from col.Name, so it must carry the physical name — + // matching the runtime SQL asserted by CrossDialectSchemaTests.Select_MapToColumn_CreditLimit. + var steps = SchemaDiffer.Diff(null, snapshot); + var migration = MigrationCodeGenerator.GenerateMigrationClass( + 1, "AccountsInit", steps, null, snapshot, "Test"); + Assert.That(migration, Does.Contain("\"credit_limit\"")); + Assert.That(migration, Does.Not.Contain("\"CreditLimit\"")); + } } From f37f49351ee568e3c427612ce7cfe35a2950e8c2 Mon Sep 17 00:00:00 2001 From: djgosnell Date: Tue, 14 Jul 2026 11:28:14 -0400 Subject: [PATCH 5/9] chore(session): enter REVIEW phase (#324) --- _sessions/324-migration-honor-naming-mapto/workflow.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/_sessions/324-migration-honor-naming-mapto/workflow.md b/_sessions/324-migration-honor-naming-mapto/workflow.md index c31fc5b3..37435703 100644 --- a/_sessions/324-migration-honor-naming-mapto/workflow.md +++ b/_sessions/324-migration-honor-naming-mapto/workflow.md @@ -3,7 +3,7 @@ platform: github base-branch: master ## State -phase: IMPLEMENT +phase: REVIEW status: active issue: #324 pr: @@ -51,3 +51,4 @@ Full suite green: Quarry.Tests 3388, Quarry.Migration.Tests 201, Quarry.Analyzer |------|--------|---------| | 2026-07-14 | INTAKE→DESIGN | Loaded issue #324, created worktree/branch, clean test baseline. | | 2026-07-14 | DESIGN→PLAN→IMPLEMENT | Verified full migration pipeline + 3 schema readers; fast-path design+plan approved (all 4 steps). | +| 2026-07-14 | IMPLEMENT→REVIEW | All 4 steps committed (f594cc1, 41c6659, 72d6865, 6ac9148); full suite green (3397/201/146). | From a061951276f879068b134b05cc1cd4720f05969a Mon Sep 17 00:00:00 2001 From: djgosnell Date: Tue, 14 Jul 2026 11:48:24 -0400 Subject: [PATCH 6/9] Review remediation: mirror runtime NamingStyle acceptance exactly (#324) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review findings F1-F8: - F2/F7 (A): tighten ProjectSchemaReader NamingStyle detection to mirror SchemaParser.ExtractNamingStyle exactly — gate on !IsStatic && IsOverride and read only an expression-bodied member access. The prior version also honored initializer/getter-arrow forms the runtime ignores, which would let migration and runtime disagree on column names (the #324 bug inverted). - F4/F5 (B): add parity tests — snake_case driven through MigrationCodeGenerator, and getter-arrow / non-override / non-literal-MapTo forms asserting Exact / null MappedName (matching runtime). - F1 (B): record the Step-4 sample-compilation decision in Working Notes. - F8 (B): behavioral-correction note deferred to PR body. - F3/F6 (D): dismissed (see review.md). --- .../review.md | 40 +++++++++ .../workflow.md | 4 +- .../ProjectSchemaReaderNamingMapToTests.cs | 87 +++++++++++++++++++ src/Quarry.Tool/Schema/ProjectSchemaReader.cs | 23 ++--- 4 files changed, 139 insertions(+), 15 deletions(-) create mode 100644 _sessions/324-migration-honor-naming-mapto/review.md diff --git a/_sessions/324-migration-honor-naming-mapto/review.md b/_sessions/324-migration-honor-naming-mapto/review.md new file mode 100644 index 00000000..04482ba4 --- /dev/null +++ b/_sessions/324-migration-honor-naming-mapto/review.md @@ -0,0 +1,40 @@ +# Review: 324-migration-honor-naming-mapto + +## Classifications +| ID | Class | Rec | Sev | Section | Finding | Action Taken | +|----|-------|-----|-----|---------|---------|--------------| +| F2 | A | A | M | Correctness | NamingStyle detection too permissive vs runtime (no override guard; reads initializer/getter-arrow) → tool/runtime can disagree again | Fixed: `ProjectSchemaReader.cs:113` now gates on `!IsStatic && IsOverride` and reads expression-body member-access only, mirroring `SchemaParser.ExtractNamingStyle`. | +| F7 | A | A | M | Consistency | NamingStyle extraction diverges from runtime SchemaParser acceptance set (same root as F2) | Fixed by the F2 change (tool now accepts exactly the runtime's form set). Extracting a shared cross-project helper was out of scope; exact mirroring achieves parity. | +| F1 | B | B | L | Plan | Step-4 stub decision recorded only in a code comment, not workflow.md Working Notes | Fixed: added a Working Note documenting the real-AccountSchema + Money / stub-UserSchema decision. | +| F4 | B | B | L | Test | NamingStyle path not driven through MigrationCodeGenerator (only MapTo path has the DDL-level assertion) | Fixed: added `NamingStyle_SnakeCase_MigrationCodeUsesStyledColumnNames` asserting the generated migration contains `user_name` and not `UserName`. | +| F5 | B | B | L | Test | No coverage for the divergent parsing forms (getter-arrow/initializer/non-override) or non-literal MapTo | Fixed: added getter-arrow, non-override, and non-literal-MapTo parity tests (all assert Exact / null-MappedName, matching runtime). | +| F8 | B | B | M | Integration | Intended compat break (physical-name diffs/hash shift) not yet documented for users (PR body pending) | To be documented in the PR body Breaking Changes section at PR creation (this step). | +| F3 | D | D | L | Test | Test compilation not checked for diagnostics before extraction | Dismissed: matches the existing `ProjectSchemaReaderIndexTests` harness; assertions target specific physical names that only arise if parsing succeeds, so silent degradation can't produce a false pass. | +| F6 | D | D | L | Test | ReadSampleSource via [CallerFilePath] has no existence guard (environment-coupled) | Dismissed: CI runs `dotnet build` + `dotnet test --no-build` in one workspace with no PathMap/ContinuousIntegrationBuild, so CallerFilePath resolves to the real path; accepted fixture-loading pattern. | + +## Plan Compliance +| ID | Finding | Sev | Why It Matters | +| F1 | Step 4 told the author to record the "stub `UserSchema` vs. compile the whole real sample graph" decision in Working Notes at implementation time; it is only captured in a code comment in `ProjectSchemaReaderNamingMapToTests.cs:274-276`, not in `workflow.md` Working Notes. | L | Minor process gap; the rationale is discoverable in the test but the audit trail the plan asked for is missing. | + +## Correctness +| ID | Finding | Sev | Why It Matters | +| F2 | NamingStyle detection in `ProjectSchemaReader.cs:113-136` does not faithfully mirror runtime `SchemaParser.ExtractNamingStyle` (`SchemaParser.cs:250-288`): it (a) matches any declared member named `NamingStyle` without the runtime's `!IsStatic && IsOverride` guard, (b) does not validate the property type, and (c) reads `Initializer?.Value` and the getter-arrow body — forms the runtime ignores (runtime only reads `propSyntax.ExpressionBody`). For a legal auto-property-initializer override (`protected override NamingStyle NamingStyle { get; } = NamingStyle.SnakeCase;`) or a getter-arrow body, the tool yields styled names while the runtime yields `Exact`, so the migration emits `user_name` columns the runtime never queries — reintroducing the exact #324 class of mismatch, inverted. | M | Undermines the fix's core guarantee (migration names == runtime physical names) for legal C# forms; the safe design is to mirror the runtime's guard set and expression-body-only parsing exactly. | + +## Security +| ID | Finding | Sev | Why It Matters | +No concerns. Inputs are developer-authored schema source parsed via Roslyn; `MapTo`/`NamingStyle` values flow into generated code exactly as `Name`/`MappedName` did before (property names previously, physical names now) — no new external-input or injection surface introduced by this diff. + +## Test Quality +| ID | Finding | Sev | Why It Matters | +| F3 | The test compilation (`CreateCompilation`) is never checked for diagnostics before extraction; a missing/incorrect `MetadataReference` would let the semantic model degrade silently (error types still expose `.Name`) rather than fail with a clear cause. | L | A broken reference set could make a test pass for the wrong reason or fail cryptically instead of pinpointing the setup error. | +| F4 | NamingStyle physical-name parity is asserted only at the `TableDef`/`ColumnDef` level (`NamingStyle_SnakeCase_...`); unlike the MapTo/`credit_limit` case, no test drives a snake_case schema through `MigrationCodeGenerator` (the DDL-bound path). | L | A regression in styled-name emission into the actual migration code would go uncaught, since only the MapTo path has the end-to-end `MigrationCodeGenerator` assertion. | +| F5 | No coverage for the parsing forms that actually diverge from the runtime (getter-arrow / auto-property-initializer / non-override `NamingStyle` — see F2) nor for `MapTo` with a non-literal argument (e.g. `MapTo(SomeConst)`). | L | The untested edge cases are precisely where tool/runtime can disagree; happy-path-only coverage hides the divergence. | +| F6 | `ReadSampleSource` locates `../Samples` via `[CallerFilePath]` (`ProjectSchemaReaderNamingMapToTests.cs:266-271`) with no existence guard, so the guard test depends on the compile-time source path still existing at run time. | L | Running the built test assembly on a different machine/path (or with deterministic/path-mapped builds) breaks with `FileNotFoundException`; robust but environment-coupled. | + +## Codebase Consistency +| ID | Finding | Sev | Why It Matters | +| F7 | The NamingStyle extraction is a bespoke reimplementation rather than aligning with the runtime `SchemaParser.ExtractNamingStyle` reference impl the plan named as the model; the accepted-forms set and guards differ (see F2), so the two schema readers can disagree. The MapTo/`GenericNameSyntax` changes, by contrast, correctly mirror runtime `GetMethodName`/`ParseColumnModifiers` and follow the file's existing `Computed`/`Collation` else-if idiom. | M | Divergence between the tool and runtime readers is the root cause of #324-style bugs; the two should share guard/parse semantics (ideally the same helper) to stay in lockstep. | + +## Integration / Breaking Changes +| ID | Finding | Sev | Why It Matters | +| F8 | Because `ColumnDef.Name` now carries the physical name, the next `migrate add` on any existing project using `NamingStyle` or `MapTo` produces a non-empty diff: column rename-or-drop+add, FK constraint-name shifts (`FK_{table}_{physicalName}` via `colDef?.Name`), and a changed snapshot hash chain (`SchemaHasher` includes `MappedName`). This is the intended behavioral correction, but it is a real compat break in migration lineage; the plan defers the notice to the PR body, which is not yet created (`workflow.md` `pr:` empty). | M | Users must be told their next migration will contain corrective rename/DDL steps and altered hashes; without the PR-body note the break is silent. | diff --git a/_sessions/324-migration-honor-naming-mapto/workflow.md b/_sessions/324-migration-honor-naming-mapto/workflow.md index 37435703..7d03a305 100644 --- a/_sessions/324-migration-honor-naming-mapto/workflow.md +++ b/_sessions/324-migration-honor-naming-mapto/workflow.md @@ -3,7 +3,7 @@ platform: github base-branch: master ## State -phase: REVIEW +phase: REMEDIATE status: active issue: #324 pr: @@ -45,6 +45,8 @@ Full suite green: Quarry.Tests 3388, Quarry.Migration.Tests 201, Quarry.Analyzer - **3 masking tests**, not 2: `ProjectSchemaReaderIndexTests.cs` lines 90, 377, 397 declare fictional `public NamingStyle Naming => …`. `SchemaTests.cs:30` already uses the real `protected override NamingStyle NamingStyle =>` (tests runtime `Schema` property — unaffected). - **MapTo API forms** (verified): `Schema.MapTo(string)` standalone (`Col X => MapTo("x")`), and `.MapTo(string)` on `ColumnBuilder`/`RefBuilder` chained (`… .Mapped<…>().MapTo("x")`). AccountSchema.CreditLimit uses the chained form. - **Round-trip is consistent** either way (with/without MappedName), but MappedName is populated per Decision above. +- **Step 4 sample-compilation decision (F1)**: the real `UserSchema.cs` drags in the whole sample graph (OrderSchema, UserAddressSchema, AddressSchema, `HasMany`/`HasManyThrough`), so the E2E guard compiles the **real** `AccountSchema.cs` + real `Money.cs` (loaded via `[CallerFilePath]` from `../Samples`) with a **minimal `UserSchema` stub** (just `Table` + `Key UserId`). AccountSchema itself — the drift-guard target — stays verbatim. +- **REVIEW remediation (F2/F7)**: the first NamingStyle fix was *more* permissive than the runtime (read initializer + getter-arrow, no override guard). That is itself a divergence bug (migration would style names the runtime leaves Exact). Tightened to mirror `SchemaParser.ExtractNamingStyle` exactly: `!IsStatic && IsOverride` + expression-body-with-member-access only. F5 tests lock this parity in. ## Suspend State ## Session Log | Date | Phases | Summary | diff --git a/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs b/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs index 8d68db4c..6353cd65 100644 --- a/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs +++ b/src/Quarry.Tests/Migration/ProjectSchemaReaderNamingMapToTests.cs @@ -261,6 +261,93 @@ public class UserSchema : Schema Assert.That(Column(table, "Email").MappedName, Is.Null); } + [Test] + public void NamingStyle_SnakeCase_MigrationCodeUsesStyledColumnNames() + { + // Symmetry with the MapTo end-to-end guard: drive a snake_case schema through the DDL-bound + // MigrationCodeGenerator (which emits col.Name) and confirm the styled physical name lands there. + var source = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + protected override NamingStyle NamingStyle => NamingStyle.SnakeCase; + public Key UserId { get; } + public Col UserName { get; } +}"; + var snapshot = ExtractSnapshot(source, 1); + var steps = SchemaDiffer.Diff(null, snapshot); + var migration = MigrationCodeGenerator.GenerateMigrationClass( + 1, "UsersInit", steps, null, snapshot, "Test"); + + Assert.That(migration, Does.Contain("\"user_name\"")); + Assert.That(migration, Does.Not.Contain("\"UserName\"")); + } + + [Test] + public void NamingStyle_GetterArrowBody_IgnoredExactly_LikeRuntime() + { + // The runtime SchemaParser honors ONLY an expression-bodied override; a getter-arrow body + // yields Exact. The tool must match, or the migration would style names the runtime does not. + var source = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + protected override NamingStyle NamingStyle { get => NamingStyle.SnakeCase; } + public Key UserId { get; } + public Col UserName { get; } +}"; + var table = ExtractSingleTable(source); + + Assert.That(table.NamingStyle, Is.EqualTo(NamingStyleKind.Exact)); + Assert.That(table.Columns.Any(c => c.Name == "user_name"), Is.False); + } + + [Test] + public void NamingStyle_NonOverrideProperty_Ignored_LikeRuntime() + { + // A same-named property that is not an override must not be treated as the naming style + // (the runtime requires IsOverride). + var source = @" +using Quarry; + +public class UserSchema : Schema +{ + public static string Table => ""users""; + public new NamingStyle NamingStyle => NamingStyle.SnakeCase; + public Key UserId { get; } + public Col UserName { get; } +}"; + var table = ExtractSingleTable(source); + + Assert.That(table.NamingStyle, Is.EqualTo(NamingStyleKind.Exact)); + Assert.That(table.Columns.Any(c => c.Name == "user_name"), Is.False); + } + + [Test] + public void MapTo_NonLiteralArgument_NotExtracted_LikeRuntime() + { + // Only string literals are extracted (matching runtime); a non-literal MapTo argument leaves + // the column on its property/styled name, so tool and runtime still agree. + var source = @" +using Quarry; + +public class UserSchema : Schema +{ + private const string ColName = ""user_name""; + public static string Table => ""users""; + public Key UserId { get; } + public Col UserName => MapTo(ColName); +}"; + var table = ExtractSingleTable(source); + + Assert.That(Column(table, "UserName").MappedName, Is.Null); + Assert.That(table.Columns.Any(c => c.Name == "user_name"), Is.False); + } + // Loads a committed sample source file (sibling ../Samples of this test file) so the guard // tracks the REAL AccountSchema, not a copy that could silently drift. private static string ReadSampleSource(string fileName, [CallerFilePath] string thisFilePath = "") diff --git a/src/Quarry.Tool/Schema/ProjectSchemaReader.cs b/src/Quarry.Tool/Schema/ProjectSchemaReader.cs index b9cfe5a5..1dec9633 100644 --- a/src/Quarry.Tool/Schema/ProjectSchemaReader.cs +++ b/src/Quarry.Tool/Schema/ProjectSchemaReader.cs @@ -110,23 +110,18 @@ private static bool InheritsFromSchema(INamedTypeSymbol symbol) characterSet = literal2.Token.ValueText; } } - else if (prop.Name == "NamingStyle") + else if (prop.Name == "NamingStyle" && !prop.IsStatic && prop.IsOverride) { - // Mirror the runtime SchemaParser: the real API is the overridable - // Schema.NamingStyle property, e.g. `protected override NamingStyle NamingStyle => NamingStyle.SnakeCase;`. + // Mirror the runtime SchemaParser.ExtractNamingStyle EXACTLY: only an override of + // the virtual Schema.NamingStyle property, expressed as an expression body with a + // member access (`protected override NamingStyle NamingStyle => NamingStyle.SnakeCase;`). + // Honoring any looser form here than the runtime does would let the migration and the + // runtime disagree on column names again (the #324 bug, inverted). var syntax = prop.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(); - if (syntax is PropertyDeclarationSyntax propSyntax) + if (syntax is PropertyDeclarationSyntax propSyntax + && propSyntax.ExpressionBody?.Expression is MemberAccessExpressionSyntax memberAccess) { - var expr = propSyntax.Initializer?.Value ?? propSyntax.ExpressionBody?.Expression; - if (expr == null && propSyntax.AccessorList != null) - { - var getter = propSyntax.AccessorList.Accessors - .FirstOrDefault(a => a.IsKind(SyntaxKind.GetAccessorDeclaration)); - expr = getter?.ExpressionBody?.Expression; - } - - var valueName = (expr as MemberAccessExpressionSyntax)?.Name.Identifier.Text; - namingStyle = valueName switch + namingStyle = memberAccess.Name.Identifier.Text switch { "SnakeCase" => NamingStyleKind.SnakeCase, "CamelCase" => NamingStyleKind.CamelCase, From 435a12e076b26bc8c0755b600cba014456fa9575 Mon Sep 17 00:00:00 2001 From: djgosnell Date: Tue, 14 Jul 2026 11:49:18 -0400 Subject: [PATCH 7/9] chore(session): add PR body and review action-taken (#324) --- .../_pr-body.md | 65 +++++++++++++++++++ .../workflow.md | 1 + 2 files changed, 66 insertions(+) create mode 100644 _sessions/324-migration-honor-naming-mapto/_pr-body.md diff --git a/_sessions/324-migration-honor-naming-mapto/_pr-body.md b/_sessions/324-migration-honor-naming-mapto/_pr-body.md new file mode 100644 index 00000000..066fca07 --- /dev/null +++ b/_sessions/324-migration-honor-naming-mapto/_pr-body.md @@ -0,0 +1,65 @@ +## Summary +- Closes #324 + +Makes the migration tool's `ProjectSchemaReader` honor the two column-naming mechanisms the runtime +source generator already honors — the `NamingStyle` override and per-column `MapTo("physical")` — so +`quarry migrate add`/`diff` produce snapshots and DDL whose column names match the physical names the +runtime actually queries. + +## Reason for Change +The migration reader detected the naming style from a property literally named `Naming` (the real API is +`NamingStyle`) and never parsed `MapTo`. As a result: +- Snapshots/DDL used C# **property** names (`UserName`, `CreditLimit`) while the runtime emits SQL using + **physical** names (`user_name`, `credit_limit`) — a generated migration created columns the runtime + never queries. +- Adding or removing a mapping between two schema versions produced **zero** diff steps — the change was + silently ignored, so the physical column the runtime targets was never created or renamed. + +## Impact +- `src/Quarry.Tool/Schema/ProjectSchemaReader.cs` + - `ExtractTableDef` now detects the real `NamingStyle` override, mirroring the runtime + `SchemaParser.ExtractNamingStyle` **exactly**: it gates on `!IsStatic && IsOverride` and reads only an + expression-bodied member access (`protected override NamingStyle NamingStyle => NamingStyle.SnakeCase;`). + Honoring any looser form than the runtime would let the two disagree on column names again. + - `ExtractColumnDef` now parses `MapTo` — both the chained `.MapTo("x")` and the standalone generic + `MapTo("x")` (via a new `GenericNameSyntax` case, mirroring runtime `GetMethodName`). The physical + column name becomes `MapTo ?? ToColumnName(prop, style)` and `ColumnDef.MappedName` is populated so the + snapshot re-emits `.MapTo(...)` and `SchemaHasher`/`RenameMatcher` stay faithful. + - A `Ref`'s foreign-key constraint column now reuses the resolved column name, so it stays consistent + when a Ref uses `MapTo`/`NamingStyle`. +- This fixes both `migrate add` and `migrate diff` (both call `ProjectSchemaReader.ExtractSchemaSnapshot`). + +No changes were needed to `SchemaDiffer`, `DdlRenderer`, `MigrationCodeGenerator`, `SnapshotCodeGenerator`, +or the `ColumnDef` model — they already consume `Name`/`MappedName` correctly (the migration DDL name is +generated from `ColumnDef.Name`). The runtime `SchemaParser` and the `Quarry.Migration/SchemaResolver` +(SchemaMap for result materialization) already honored both mechanisms and are untouched. + +## Plan items implemented as specified +- Honor `NamingStyle` and `MapTo` in `ProjectSchemaReader`; de-mask the three naming tests in + `ProjectSchemaReaderIndexTests` (fictional `NamingStyle Naming` → the real `override NamingStyle`). +- Unit coverage for `NamingStyle` and `MapTo` (chained, standalone-generic, override-vs-style precedence, + and Exact regression). +- Diff coverage proving a `MapTo` add/remove produces a rename-or-drop+add, never a silent no-op. +- End-to-end guard compiling the real committed `AccountSchema` (`credit_limit`) and asserting the + extracted name and the generated migration code carry the physical name — matching the runtime SQL + already covered by `CrossDialectSchemaTests.Select_MapToColumn_CreditLimit`. + +## Gaps in original plan implemented +- The issue's third location bullet (`DatabaseSchemaReader.cs` / `NormalizeForDiff`) was **stale** — no such + file/method exists. The diff already keys columns off `ColumnDef.Name` in `SchemaDiffer`, so once `Name` + carries the physical name, add/remove-`MapTo` diffs stop being no-ops with no change needed there. +- Review found the initial `NamingStyle` detection was *more permissive* than the runtime (it also read + initializer/getter-arrow forms and skipped the override guard) — itself a divergence risk. Tightened to + mirror the runtime exactly, with added parity tests for getter-arrow, non-override, and non-literal-`MapTo` + forms. + +## Breaking Changes +- Consumer-facing: A project that already ran `migrate add` on a schema using `NamingStyle` or `MapTo` has + snapshots recorded with **PascalCase** (property) column names. After this fix, its **next** `migrate add` + will show a physical-name diff — column rename (or drop+add), foreign-key constraint-name shifts + (`FK_{table}_{physicalName}`), and a changed snapshot hash chain (`SchemaHasher` includes `MappedName`). + Those projects were already broken (their migrations diverged from what the runtime queries), so this is a + one-time **correction**, not a regression. Projects using `Exact` naming and no `MapTo` are unaffected + (verified: the committed `1_SimpleMigration`, `Quarry.Sample.Aot`, `Quarry.Sample.WebApp` migrations use + `Exact`; the snake_case and `MapTo` samples have no committed migrations). +- Internal: none. diff --git a/_sessions/324-migration-honor-naming-mapto/workflow.md b/_sessions/324-migration-honor-naming-mapto/workflow.md index 7d03a305..7eb0b00c 100644 --- a/_sessions/324-migration-honor-naming-mapto/workflow.md +++ b/_sessions/324-migration-honor-naming-mapto/workflow.md @@ -54,3 +54,4 @@ Full suite green: Quarry.Tests 3388, Quarry.Migration.Tests 201, Quarry.Analyzer | 2026-07-14 | INTAKE→DESIGN | Loaded issue #324, created worktree/branch, clean test baseline. | | 2026-07-14 | DESIGN→PLAN→IMPLEMENT | Verified full migration pipeline + 3 schema readers; fast-path design+plan approved (all 4 steps). | | 2026-07-14 | IMPLEMENT→REVIEW | All 4 steps committed (f594cc1, 41c6659, 72d6865, 6ac9148); full suite green (3397/201/146). | +| 2026-07-14 | REVIEW→REMEDIATE | Rebased on origin/master (#325); agent analysis → 8 findings (2A/4B/2D); accepted as recommended. Remediated F1-F8; suite green (3415/201/146). | From 7ed7ecf1b3c7ba41b3532802caea1587d6ca56c1 Mon Sep 17 00:00:00 2001 From: djgosnell Date: Tue, 14 Jul 2026 11:49:36 -0400 Subject: [PATCH 8/9] chore(session): record PR #327 (#324) --- _sessions/324-migration-honor-naming-mapto/workflow.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/_sessions/324-migration-honor-naming-mapto/workflow.md b/_sessions/324-migration-honor-naming-mapto/workflow.md index 7eb0b00c..402bfde4 100644 --- a/_sessions/324-migration-honor-naming-mapto/workflow.md +++ b/_sessions/324-migration-honor-naming-mapto/workflow.md @@ -6,7 +6,7 @@ base-branch: master phase: REMEDIATE status: active issue: #324 -pr: +pr: #327 ## Problem Statement Migration tooling (`quarry migrate add`/`diff` via `ProjectSchemaReader`) does not honor two column-naming mechanisms the runtime source generator honors: From 5d3e3d1c37fdb75c8ad8b39fa5a0b3a2b6a971e9 Mon Sep 17 00:00:00 2001 From: djgosnell Date: Tue, 14 Jul 2026 11:53:59 -0400 Subject: [PATCH 9/9] chore(session): enter FINALIZE, CI green (#324) --- _sessions/324-migration-honor-naming-mapto/workflow.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/_sessions/324-migration-honor-naming-mapto/workflow.md b/_sessions/324-migration-honor-naming-mapto/workflow.md index 402bfde4..078e1b54 100644 --- a/_sessions/324-migration-honor-naming-mapto/workflow.md +++ b/_sessions/324-migration-honor-naming-mapto/workflow.md @@ -3,7 +3,7 @@ platform: github base-branch: master ## State -phase: REMEDIATE +phase: FINALIZE status: active issue: #324 pr: #327 @@ -55,3 +55,4 @@ Full suite green: Quarry.Tests 3388, Quarry.Migration.Tests 201, Quarry.Analyzer | 2026-07-14 | DESIGN→PLAN→IMPLEMENT | Verified full migration pipeline + 3 schema readers; fast-path design+plan approved (all 4 steps). | | 2026-07-14 | IMPLEMENT→REVIEW | All 4 steps committed (f594cc1, 41c6659, 72d6865, 6ac9148); full suite green (3397/201/146). | | 2026-07-14 | REVIEW→REMEDIATE | Rebased on origin/master (#325); agent analysis → 8 findings (2A/4B/2D); accepted as recommended. Remediated F1-F8; suite green (3415/201/146). | +| 2026-07-14 | REMEDIATE→FINALIZE | Pushed; PR #327 created; CI green (success). |