Skip to content

Commit be48ccb

Browse files
dmealingclaude
andcommitted
refactor(csharp): post-merge review + simplify pass on codegen
Quality-gate pass (code-reviewer + code-simplifier) on the view-DDL / FK / @storage / object-field codegen units. Fixes from review: - Composite PRIMARY KEY and UNIQUE indexes now follow the identity's declared @fields order instead of field-declaration order (matches the FK path and the TS reference; was a latent wrong-key-shape bug, previously untested). - CodegenRunner errors on duplicate generated output paths (mirrors codegen-ts runGen); value-object POCO emission had widened the silent-clobber surface. - ObjectNavProperty routes its required-check through CSharpNaming.IsRequired, consistent with ScalarProperty. Simplifications (output byte-identical; conformance + codegen string asserts green): - Hoisted the triplicated private StripPkg into CSharpNaming.StripPkg. - Extracted ResolveRelation shared by ToManyFk/ToOneFk; ToOneFk match reads against the resolved target name. Adds a composite-key ordering test. Full solution green (282 tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 80f9e02 commit be48ccb

6 files changed

Lines changed: 78 additions & 48 deletions

File tree

server/csharp/MetaObjects.Codegen.Tests/PostgresSchemaTests.cs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,30 @@ public void CreateTable_omits_foreign_key_for_logical_only_reference()
114114
Assert.DoesNotContain("CONSTRAINT tags_", sql);
115115
}
116116

117+
[Fact]
118+
public void Composite_pk_and_unique_index_preserve_declared_field_order()
119+
{
120+
// @fields order ("b","a") differs from field-declaration order ("a","b");
121+
// the DDL must follow @fields, not declaration order.
122+
const string m = """
123+
{ "metadata.root": { "package": "acme", "children": [
124+
{ "object.entity": { "name": "Link", "children": [
125+
{ "source.dbTable": { "@name": "links" } },
126+
{ "field.long": { "name": "a" } },
127+
{ "field.long": { "name": "b" } },
128+
{ "identity.primary": { "@fields": ["b", "a"] } },
129+
{ "identity.secondary": { "name": "byBA", "@fields": ["b", "a"], "@unique": true } }
130+
]}}
131+
]}}
132+
""";
133+
var r = new MetaDataLoader().Load([new InMemorySource(m, id: "ck.json")]);
134+
Assert.Empty(r.Errors);
135+
var sql = PostgresSchema.BuildSchema(r.Root);
136+
137+
Assert.Contains("PRIMARY KEY (b, a)", sql);
138+
Assert.Contains("CREATE UNIQUE INDEX links_byBA_uniq ON links (b, a);", sql);
139+
}
140+
117141
[Fact]
118142
public void Flattened_object_field_expands_to_prefixed_columns()
119143
{

server/csharp/MetaObjects.Codegen/CSharpNaming.cs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,17 @@ public static class CSharpNaming
4747
public static string Pascal(string name) =>
4848
name.Length == 0 ? name : char.ToUpperInvariant(name[0]) + name[1..];
4949

50+
/// <summary>
51+
/// The bare object name from a possibly package-qualified reference (the segment
52+
/// after the last <c>::</c>), for resolving an <c>@objectRef</c>/<c>@references</c>
53+
/// against <see cref="MetaRoot.FindObject"/>. Shared by the schema + generators.
54+
/// </summary>
55+
public static string StripPkg(string name)
56+
{
57+
var i = name.LastIndexOf("::", StringComparison.Ordinal);
58+
return i < 0 ? name : name[(i + 2)..];
59+
}
60+
5061
/// <summary>
5162
/// Cosmetic pluralization for a DbSet property name + the route collection
5263
/// segment (the table name itself comes from [Table]). Shared so the DbContext

server/csharp/MetaObjects.Codegen/CodegenRunner.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,20 @@ public static RunResult Run(GenConfig config, MetaRoot root, IReadOnlyList<IGene
2929

3030
Directory.CreateDirectory(config.OutDir);
3131
var results = new List<WriteResult>();
32+
// Two generators (or a name collision between an entity and a value-object
33+
// POCO) writing the same path would silently clobber each other; error
34+
// instead, mirroring codegen-ts's runGen duplicate-path check.
35+
var emitted = new Dictionary<string, string>(StringComparer.Ordinal);
3236

3337
foreach (var generator in generators)
3438
{
3539
foreach (var file in generator.Generate(ctx))
3640
{
3741
var full = Path.GetFullPath(Path.Combine(config.OutDir, file.Path));
42+
if (!emitted.TryAdd(full, generator.Name))
43+
throw new InvalidOperationException(
44+
$"duplicate generated output path \"{file.Path}\" — emitted by both " +
45+
$"\"{emitted[full]}\" and \"{generator.Name}\".");
3846
if (File.Exists(full) && !File.ReadAllText(full).Contains(GeneratedMarker, StringComparison.Ordinal))
3947
{
4048
warnings.Add($"refusing to overwrite hand-written file: {file.Path}");

server/csharp/MetaObjects.Codegen/Generators/DbContextGenerator.cs

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ public IEnumerable<EmittedFile> Generate(GenContext ctx)
7272
// when @objectRef can't be resolved.
7373
private string? OwnedTypeConfig(MetaObject entity, MetaField field, GenContext ctx)
7474
{
75-
if (field.ObjectRef is not { } oref || ctx.Root.FindObject(StripPkg(oref)) is not { } vo)
75+
if (field.ObjectRef is not { } oref || ctx.Root.FindObject(CSharpNaming.StripPkg(oref)) is not { } vo)
7676
{
7777
ctx.Warn($"{Name}: object-typed field \"{entity.Name}.{field.Name}\" has an unresolved @objectRef \"{field.ObjectRef}\" — no owned-type config emitted.");
7878
return null;
@@ -95,10 +95,4 @@ public IEnumerable<EmittedFile> Generate(GenContext ctx)
9595
sb.Append(" });");
9696
return sb.ToString();
9797
}
98-
99-
private static string StripPkg(string s)
100-
{
101-
var i = s.LastIndexOf("::", StringComparison.Ordinal);
102-
return i < 0 ? s : s[(i + 2)..];
103-
}
10498
}

server/csharp/MetaObjects.Codegen/Generators/EntityGenerator.cs

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -164,14 +164,14 @@ private static string ScalarProperty(MetaObject owner, MetaField field, IReadOnl
164164
// a warning) when the @objectRef can't be resolved.
165165
private string? ObjectNavProperty(MetaObject owner, MetaField field, GenContext ctx)
166166
{
167-
if (field.ObjectRef is not { } oref || ctx.Root.FindObject(StripPkg(oref)) is not { } target)
167+
if (field.ObjectRef is not { } oref || ctx.Root.FindObject(CSharpNaming.StripPkg(oref)) is not { } target)
168168
{
169169
ctx.Warn($"{Name}: object-typed field \"{owner.Name}.{field.Name}\" has an unresolved @objectRef \"{field.ObjectRef}\" — skipped.");
170170
return null;
171171
}
172172
var typeName = CSharpNaming.Pascal(target.Name);
173173
var propName = CSharpNaming.Pascal(field.Name);
174-
var required = field.OwnAttr(FIELD_ATTR_REQUIRED) is true;
174+
var required = CSharpNaming.IsRequired(owner, field);
175175
return required
176176
? $" public {typeName} {propName} {{ get; set; }} = default!;"
177177
: $" public {typeName}? {propName} {{ get; set; }}";
@@ -201,17 +201,11 @@ private static List<MetaObject> ReferencedValueObjects(IReadOnlyList<MetaObject>
201201
void Enqueue(string? objectRef)
202202
{
203203
if (objectRef is null) return;
204-
var target = ctx.Root.FindObject(StripPkg(objectRef));
204+
var target = ctx.Root.FindObject(CSharpNaming.StripPkg(objectRef));
205205
// Only plain value objects (no source) become POCOs; entities/projections
206206
// are already in `mapped`.
207207
if (target is null || !target.IsValue() || target.DbView is not null) return;
208208
if (seen.Add(target.Name)) queue.Enqueue(target);
209209
}
210210
}
211-
212-
private static string StripPkg(string s)
213-
{
214-
var i = s.LastIndexOf("::", StringComparison.Ordinal);
215-
return i < 0 ? s : s[(i + 2)..];
216-
}
217211
}

server/csharp/MetaObjects.Codegen/Schema/PostgresSchema.cs

Lines changed: 31 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,6 @@ public static string CreateTable(MetaObject entity, MetaRoot root)
5959
{
6060
var table = entity.DbTable ?? entity.Name;
6161
var pk = entity.PrimaryIdentity();
62-
var pkCols = (pk?.Fields ?? []).ToHashSet(StringComparer.Ordinal);
6362

6463
var lines = new List<string>();
6564
foreach (var f in entity.Fields())
@@ -76,7 +75,8 @@ public static string CreateTable(MetaObject entity, MetaRoot root)
7675
}
7776
if (pk is not null && pk.Fields.Count > 0)
7877
{
79-
var cols = entity.Fields().Where(f => pkCols.Contains(f.Name)).Select(Col);
78+
// Preserve the identity's declared @fields order (not field-declaration order).
79+
var cols = pk.Fields.Select(name => ResolveColumn(entity, name));
8080
lines.Add($" PRIMARY KEY ({string.Join(", ", cols)})");
8181
}
8282
// FOREIGN KEY constraints from enforced identity.reference children.
@@ -93,7 +93,7 @@ public static string CreateTable(MetaObject entity, MetaRoot root)
9393
// UNIQUE indexes from secondary identities marked unique.
9494
foreach (var sec in entity.SecondaryIdentities().Where(i => i.Unique))
9595
{
96-
var cols = entity.Fields().Where(f => sec.Fields.Contains(f.Name)).Select(Col);
96+
var cols = sec.Fields.Select(name => ResolveColumn(entity, name));
9797
sb.AppendLine($"CREATE UNIQUE INDEX {table}_{sec.Name}_uniq ON {table} ({string.Join(", ", cols)});");
9898
}
9999
return sb.ToString();
@@ -106,7 +106,7 @@ public static string CreateTable(MetaObject entity, MetaRoot root)
106106
private static IEnumerable<string> ObjectFieldColumns(MetaObject entity, MetaField f, MetaRoot root)
107107
{
108108
if (f.Storage == STORAGE_FLATTENED && f.ObjectRef is { } oref &&
109-
root.FindObject(StripPkg(oref)) is { } nested)
109+
root.FindObject(CSharpNaming.StripPkg(oref)) is { } nested)
110110
{
111111
var prefix = Col(f) + "_";
112112
foreach (var nf in nested.Fields().Where(n => CSharpNaming.ScalarFor(n.SubType) is not null))
@@ -127,7 +127,7 @@ private static IEnumerable<string> ObjectFieldColumns(MetaObject entity, MetaFie
127127
private static string? ForeignKeyClause(MetaObject entity, MetaReferenceIdentity fk, MetaRoot root)
128128
{
129129
if (fk.TargetEntity is not { } targetName || fk.Fields.Count == 0) return null;
130-
var target = root.FindObject(StripPkg(targetName));
130+
var target = root.FindObject(CSharpNaming.StripPkg(targetName));
131131
if (target is null) return null;
132132

133133
var fkCols = fk.Fields.Select(js => ResolveColumn(entity, js)).ToList();
@@ -173,17 +173,17 @@ public static string CreateView(MetaObject projection, MetaRoot root, Action<str
173173
if (origin is MetaPassthroughOrigin pt && pt.Via is null && pt.From is { } from && from.Contains('.'))
174174
{
175175
var (ent, field) = SplitDot(from);
176-
baseEntity ??= StripPkg(ent);
177-
if (StripPkg(ent) != baseEntity) { blocked = "passthrough from multiple base entities"; break; }
176+
baseEntity ??= CSharpNaming.StripPkg(ent);
177+
if (CSharpNaming.StripPkg(ent) != baseEntity) { blocked = "passthrough from multiple base entities"; break; }
178178
cols.Add($" {ResolveColumn(root.FindObject(baseEntity), field)} AS {Col(f)}");
179179
}
180180
// passthrough WITH via → forward a field from a to-one related entity (correlated subquery).
181181
else if (origin is MetaPassthroughOrigin ptv && ptv.Via is { } pvia && ptv.From is { } pfrom &&
182182
pvia.Contains('.') && pfrom.Contains('.'))
183183
{
184184
var (baseEnt, relName) = SplitDot(pvia);
185-
baseEntity ??= StripPkg(baseEnt);
186-
if (StripPkg(baseEnt) != baseEntity) { blocked = "passthrough via a different base entity"; break; }
185+
baseEntity ??= CSharpNaming.StripPkg(baseEnt);
186+
if (CSharpNaming.StripPkg(baseEnt) != baseEntity) { blocked = "passthrough via a different base entity"; break; }
187187
if (ToOneFk(root, baseEntity, relName) is not { } fk)
188188
{ blocked = $"unresolved to-one FK for @via \"{pvia}\" (base needs an identity.reference)"; break; }
189189
var srcCol = ResolveColumn(fk.Target, SplitDot(pfrom).Tail);
@@ -194,8 +194,8 @@ public static string CreateView(MetaObject projection, MetaRoot root, Action<str
194194
of.Contains('.') && via.Contains('.'))
195195
{
196196
var (baseEnt, relName) = SplitDot(via);
197-
baseEntity ??= StripPkg(baseEnt);
198-
if (StripPkg(baseEnt) != baseEntity) { blocked = "aggregate over a different base entity"; break; }
197+
baseEntity ??= CSharpNaming.StripPkg(baseEnt);
198+
if (CSharpNaming.StripPkg(baseEnt) != baseEntity) { blocked = "aggregate over a different base entity"; break; }
199199
if (ToManyFk(root, baseEntity, relName) is not { } fk)
200200
{ blocked = $"unresolved to-many FK for @via \"{via}\" (target needs an identity.reference back to {baseEntity})"; break; }
201201
var ofCol = ResolveColumn(fk.Target, SplitDot(of).Tail);
@@ -205,9 +205,9 @@ public static string CreateView(MetaObject projection, MetaRoot root, Action<str
205205
else if (origin is MetaCollectionOrigin coll && coll.Via is { } cvia && cvia.Contains('.') && f.ObjectRef is { } objRef)
206206
{
207207
var (baseEnt, relName) = SplitDot(cvia);
208-
baseEntity ??= StripPkg(baseEnt);
209-
if (StripPkg(baseEnt) != baseEntity) { blocked = "collection over a different base entity"; break; }
210-
var nested = root.FindObject(StripPkg(objRef));
208+
baseEntity ??= CSharpNaming.StripPkg(baseEnt);
209+
if (CSharpNaming.StripPkg(baseEnt) != baseEntity) { blocked = "collection over a different base entity"; break; }
210+
var nested = root.FindObject(CSharpNaming.StripPkg(objRef));
211211
if (ToManyFk(root, baseEntity, relName) is not { } fk || nested is null)
212212
{ blocked = $"unresolved collection @via \"{cvia}\" / @objectRef \"{objRef}\""; break; }
213213
var pairs = nested.Fields()
@@ -238,29 +238,31 @@ private static (string Head, string Tail) SplitDot(string s)
238238
return (s[..i], s[(i + 1)..]);
239239
}
240240

241-
private static string StripPkg(string s)
242-
{
243-
var i = s.LastIndexOf("::", StringComparison.Ordinal);
244-
return i < 0 ? s : s[(i + 2)..];
245-
}
246-
247241
private static string ResolveColumn(MetaObject? obj, string fieldName) =>
248242
obj?.Fields().FirstOrDefault(f => f.Name == fieldName)?.DbColumn ?? fieldName;
249243

244+
// Resolve a named relationship on the base entity to its (base, target) objects.
245+
private static (MetaObject Base, MetaObject Target)? ResolveRelation(
246+
MetaRoot root, string baseEntity, string relName)
247+
{
248+
var baseObj = root.FindObject(baseEntity);
249+
var rel = baseObj?.Relationships().FirstOrDefault(r => r.Name == relName);
250+
if (baseObj is null || rel?.ObjectRef is not { } objRef) return null;
251+
var target = root.FindObject(CSharpNaming.StripPkg(objRef));
252+
return target is null ? null : (baseObj, target);
253+
}
254+
250255
// Resolve a to-many relationship to the FK that lives on the *target* entity:
251256
// the target carries an identity.reference back to the base. Used by aggregate
252257
// and collection origins (subquery scans the target, filtered by the base PK).
253258
private static (MetaObject Target, string TargetTable, string FkCol, string ParentCol)? ToManyFk(
254259
MetaRoot root, string baseEntity, string relName)
255260
{
256-
var baseObj = root.FindObject(baseEntity);
257-
var rel = baseObj?.Relationships().FirstOrDefault(r => r.Name == relName);
258-
if (baseObj is null || rel?.ObjectRef is not { } objRef) return null;
259-
var target = root.FindObject(StripPkg(objRef));
260-
if (target is null) return null;
261+
if (ResolveRelation(root, baseEntity, relName) is not { } rel) return null;
262+
var (baseObj, target) = rel;
261263

262264
var fkRef = target.ReferenceIdentities()
263-
.FirstOrDefault(r => r.TargetEntity is { } te && StripPkg(te) == baseEntity);
265+
.FirstOrDefault(r => r.TargetEntity is { } te && CSharpNaming.StripPkg(te) == baseEntity);
264266
if (fkRef is null || fkRef.Fields.Count == 0) return null;
265267

266268
var fkCol = ResolveColumn(target, fkRef.Fields[0]);
@@ -278,14 +280,11 @@ private static (MetaObject Target, string TargetTable, string FkCol, string Pare
278280
private static (MetaObject Target, string TargetTable, string TargetKeyCol, string BaseFkCol)? ToOneFk(
279281
MetaRoot root, string baseEntity, string relName)
280282
{
281-
var baseObj = root.FindObject(baseEntity);
282-
var rel = baseObj?.Relationships().FirstOrDefault(r => r.Name == relName);
283-
if (baseObj is null || rel?.ObjectRef is not { } objRef) return null;
284-
var target = root.FindObject(StripPkg(objRef));
285-
if (target is null) return null;
283+
if (ResolveRelation(root, baseEntity, relName) is not { } rel) return null;
284+
var (baseObj, target) = rel;
286285

287286
var fkRef = baseObj.ReferenceIdentities()
288-
.FirstOrDefault(r => r.TargetEntity is { } te && StripPkg(te) == StripPkg(objRef));
287+
.FirstOrDefault(r => r.TargetEntity is { } te && CSharpNaming.StripPkg(te) == target.Name);
289288
if (fkRef is null || fkRef.Fields.Count == 0) return null;
290289

291290
var baseFkCol = ResolveColumn(baseObj, fkRef.Fields[0]);

0 commit comments

Comments
 (0)