Skip to content

Commit 7fab436

Browse files
dmealingclaude
dmealing
and
claude
committed
fix(codegen): generated partial PATCH no longer clobbers omitted columns (C#, Kotlin)
The only generated update handler is a partial PATCH (both PATCH and PUT map to it), but two ports overwrote every column a PATCH OMITTED — silent data loss on every partial update, in shipped generated code: - C# — the handler bound the body into a full `<Entity> input`; a field absent from the JSON deserialized to its CLR default (0 / null / false / DateTime.MinValue), and `CurrentValues.SetValues(input)` then wrote those defaults over the live row. `PATCH {"status":"ACTIVE"}` on a wide entity destroyed every other column. - Kotlin (base controller) — `it[field] = dto.field` for EVERY field, so an omitted optional field's null was written over the stored value. TS (Zod `InsertSchema.partial()`), Java and Python (present-field merges in the generated repositories / doubles) were already correct — so this is two ports, not one. Each broken port already had the CORRECT logic in its own TPH branch (C#'s per-subtype JSON-body enumeration; Kotlin's `if (dto.f != null)` guard); the base handlers just didn't use it. Fix: - C# base + TPH handlers deserialize each PRESENT property from the raw JSON body (skipping the PK, and — for the base — null values), using the app's CONFIGURED JsonSerializerOptions. The last point matters: a field.timestamp binds to DateTimeOffset via a custom converter registered on ConfigureHttpJsonOptions, and the first cut used default options, which can't read the wire format and 500'd (the TPH handler had the identical latent bug — also fixed). - Kotlin base controller null-guards OPTIONAL fields only. Required props are non-null (Jackson 400s a body missing them before the handler runs), so they stay unconditional — guarding them would be an always-true warning and break -Werror. Contract: an omitted field always survives, in every port. Whether an EXPLICIT null clears a nullable column (the absent-vs-null tristate) is deferred to FR-035. Gate — the forcing function no existing scenario could provide: every prior update scenario sent every non-PK column, so there was never an omitted field to clobber. `fixtures/api-contract-conformance/scenarios/update-partial-omitted-field-survives.yaml` PATCHes one field, OMITS the optional `bio`, and asserts (via the response AND a fresh GET) that `bio` keeps its seed value. It runs on all five ports, both lanes (reference + generated). Verified to FAIL on the pre-fix Kotlin generator with exactly "expected row.bio=First computer programmer, got null" — and ONLY that scenario — then pass after. All five ports: C# 22/22 (both lanes), Kotlin 22/22, Java 22/22, Python 44, TS both lanes. Goldens updated (verified correct, not blind-regenerated): the C# routes unit test pinned the old `(long id, Subscriber input, ...)` signature; the Kotlin snapshot changed exactly one line (`it[label] = dto.label` → null-guarded, `label` being optional). Design context for the broader work (typed patch surface, Kotlin repository generator, optimistic concurrency) in docs/superpowers/specs/2026-07-13-generated-mutation-surface-design.md. Its @rowVersion / ADR-0043 proposal is NOT adopted here: @autoset:onUpdate already models "stamps on every write" and OMDB already derives its optimistic-lock column from it, so a new attribute would violate ADR-0023 (can't-invent-what's-derivable) — a decision left to the owner. Corrects GitHub #198; #197's premise was already refuted on that issue. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hd5NWZ1rKau63vJzrBSpLb
1 parent 4771f41 commit 7fab436

6 files changed

Lines changed: 859 additions & 14 deletions

File tree

‎docs/superpowers/specs/2026-07-13-generated-mutation-surface-design.md‎

Lines changed: 753 additions & 0 deletions
Large diffs are not rendered by default.

‎fixtures/api-contract-conformance/scenarios/update-partial-omitted-field-survives.yaml‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
name: update-partial-omitted-field-survives
2+
description: >
3+
A partial PATCH must not clobber a field it OMITS. This sends only `name` (plus
4+
the required `createdAt`, so DTO-binding ports don't 400 on a missing required
5+
field) and OMITS the optional `bio`, then asserts — via the PATCH response AND a
6+
fresh GET — that `bio` still holds its seed value.
7+
8+
This is the forcing function for a data-corruption bug that shipped in generated
9+
code and no existing scenario could see: the only other update scenario
10+
(update-patch-and-put) sends every non-PK column, so there was never an omitted
11+
field to clobber. Two ports overwrote omitted columns with type defaults on every
12+
partial update:
13+
- C# — the handler bound the body into a full entity (absent fields → CLR
14+
defaults 0/null/false/MinValue) and CurrentValues.SetValues wrote them all.
15+
- Kotlin (base controller) — `it[field] = dto.field` for EVERY field, so an
16+
omitted optional field's null was written over the stored value.
17+
TS (Zod .partial()), Java and Python (present-field merges) were already correct.
18+
19+
Contract asserted here: an omitted field survives, in every port. (Whether an
20+
EXPLICIT null clears a column — the absent-vs-null tristate — is deferred to FR-035;
21+
this scenario deliberately does not exercise it.)
22+
23+
requests:
24+
- id: r1
25+
method: PATCH
26+
path: /api/authors/1
27+
body:
28+
name: "Ada (name patched, bio omitted)"
29+
createdAt: "2026-01-01T10:00:00"
30+
expect:
31+
status: 200
32+
body:
33+
row:
34+
id: 1
35+
name: "Ada (name patched, bio omitted)"
36+
bio: "First computer programmer" # omitted from the PATCH → must be untouched
37+
# Re-read from the DB, not just the mutation's echo, to prove it PERSISTED.
38+
- id: r2
39+
method: GET
40+
path: /api/authors/1
41+
expect:
42+
status: 200
43+
body:
44+
row:
45+
id: 1
46+
name: "Ada (name patched, bio omitted)"
47+
bio: "First computer programmer"

‎server/csharp/MetaObjects.Codegen.Tests/EntityGeneratorTests.cs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,8 +104,11 @@ public void Routes_generator_emits_crud_endpoints()
104104
Assert.Contains("app.MapGet(prefix + \"/subscribers/{id}\", async (long id, AppDbContext db) =>", src);
105105
Assert.Contains("db.Subscribers.FindAsync(id)", src);
106106
Assert.Contains("app.MapPost(prefix + \"/subscribers\", async (Subscriber input, AppDbContext db) =>", src);
107-
// PATCH + PUT share the same update handler (matches TS reference).
108-
Assert.Contains("async System.Threading.Tasks.Task<IResult> UpdateSubscriber(long id, Subscriber input, AppDbContext db)", src);
107+
// PATCH + PUT share the same update handler (matches TS reference). It takes the raw
108+
// HttpContext (not a bound DTO) and PARTIAL-merges only the properties present in the
109+
// JSON body — a bound DTO would fill omitted fields with CLR defaults and clobber them.
110+
Assert.Contains("async System.Threading.Tasks.Task<IResult> UpdateSubscriber(long id, HttpContext http, AppDbContext db)", src);
111+
Assert.Contains("foreach (var prop in body.RootElement.EnumerateObject())", src);
109112
Assert.Contains("app.MapPatch(prefix + \"/subscribers/{id}\", UpdateSubscriber);", src);
110113
Assert.Contains("app.MapPut(prefix + \"/subscribers/{id}\", UpdateSubscriber);", src);
111114
Assert.Contains("app.MapDelete(prefix + \"/subscribers/{id}\", async (long id, AppDbContext db) =>", src);

‎server/csharp/MetaObjects.Codegen/Generators/RoutesGenerator.cs‎

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -181,19 +181,45 @@ protected virtual EmittedFile GenerateStandardRoutes(MetaObject entity, GenConte
181181
sb.AppendLine(" return Results.Created(prefix + \"/" + route + "\", input);");
182182
sb.AppendLine(" });");
183183

184-
// PATCH + PUT share the same handler — TS reference exposes both verbs,
185-
// and both return the updated row (HTTP 200), matching the cross-port
186-
// api-contract. The route id is authoritative for the key: stamp it onto
187-
// the bound input BEFORE SetValues so EF sees the primary key unchanged
188-
// (a default/zero key on the input would otherwise trip "the property is
189-
// part of a key and so cannot be modified").
184+
// PATCH + PUT share the same handler — TS reference exposes both verbs, and both
185+
// return the updated row (HTTP 200), matching the cross-port api-contract.
186+
//
187+
// A PARTIAL merge of the JSON body, NOT a full-row overwrite. The previous
188+
// implementation bound the body into a `cls input` and called
189+
// `CurrentValues.SetValues(input)` — but a field ABSENT from a PATCH body
190+
// deserializes to its CLR default (0 / null / false / DateTime.MinValue), and
191+
// SetValues then wrote those defaults over the live row. A `PATCH {"name":"x"}`
192+
// on a wide entity silently DESTROYED every other column. So enumerate the JSON
193+
// that was actually sent and set only those properties — the same present-field
194+
// merge the TPH per-subtype handler already uses. The PK stays route-authoritative.
195+
//
196+
// Null-valued properties are skipped (write present-NON-null only), matching the
197+
// Kotlin controller and the Java repository merge, so the five ports agree on the
198+
// "omitted field survives" contract. Distinguishing an explicit `null` (clear the
199+
// column) from an omitted field is the deferred tristate (FR-035).
190200
sb.AppendLine();
191-
sb.AppendLine(" async System.Threading.Tasks.Task<IResult> Update" + cls + "(" + pkType + " id, " + cls + " input, AppDbContext db)");
201+
sb.AppendLine(" async System.Threading.Tasks.Task<IResult> Update" + cls + "(" + pkType + " id, HttpContext http, AppDbContext db)");
192202
sb.AppendLine(" {");
193203
sb.AppendLine(" var existing = await db." + dbSet + ".FindAsync(id);");
194204
sb.AppendLine(" if (existing is null) return Results.NotFound(new { error = \"not_found\" });");
195-
sb.AppendLine(" input." + pkProp + " = id;");
196-
sb.AppendLine(" db.Entry(existing).CurrentValues.SetValues(input);");
205+
sb.AppendLine(" using var body = await System.Text.Json.JsonDocument.ParseAsync(http.Request.Body);");
206+
sb.AppendLine(" // Deserialize each value with the app's CONFIGURED JSON options, not defaults —");
207+
sb.AppendLine(" // a field.timestamp binds to DateTimeOffset via a custom converter registered on");
208+
sb.AppendLine(" // ConfigureHttpJsonOptions, and default options cannot read the wire format, throwing 500.");
209+
sb.AppendLine(" var jsonOpts = ((Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Http.Json.JsonOptions>)");
210+
sb.AppendLine(" http.RequestServices.GetService(typeof(Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Http.Json.JsonOptions>))!)");
211+
sb.AppendLine(" .Value.SerializerOptions;");
212+
sb.AppendLine(" var entry = db.Entry(existing);");
213+
sb.AppendLine(" foreach (var prop in body.RootElement.EnumerateObject())");
214+
sb.AppendLine(" {");
215+
sb.AppendLine(" var target = entry.Metadata.FindProperty(prop.Name)");
216+
sb.AppendLine(" ?? entry.Metadata.GetProperties().FirstOrDefault(p => string.Equals(p.Name, prop.Name, System.StringComparison.OrdinalIgnoreCase));");
217+
sb.AppendLine(" if (target is null) continue;");
218+
sb.AppendLine(" if (target.IsPrimaryKey()) continue; // PK is route-authoritative");
219+
sb.AppendLine(" if (prop.Value.ValueKind == System.Text.Json.JsonValueKind.Null) continue; // present-non-null (see note)");
220+
sb.AppendLine(" var clr = System.Nullable.GetUnderlyingType(target.ClrType) ?? target.ClrType;");
221+
sb.AppendLine(" entry.CurrentValues[target] = System.Text.Json.JsonSerializer.Deserialize(prop.Value.GetRawText(), clr, jsonOpts);");
222+
sb.AppendLine(" }");
197223
sb.AppendLine(" await db.SaveChangesAsync();");
198224
sb.AppendLine(" return Results.Ok(existing);");
199225
sb.AppendLine(" }");
@@ -444,6 +470,11 @@ private static void AppendTphSubtypeRoutes(
444470
sb.AppendLine($" var existing = await db.{dbSet}.OfType<{subCls}>().FirstOrDefaultAsync(x => x.{pkProp} == id);");
445471
sb.AppendLine(" if (existing is null) return Results.NotFound(new { error = \"not_found\" });");
446472
sb.AppendLine(" var body = await System.Text.Json.JsonDocument.ParseAsync(http.Request.Body);");
473+
sb.AppendLine(" // Configured JSON options (not defaults) so a custom converter — e.g. the");
474+
sb.AppendLine(" // field.timestamp DateTimeOffset converter — is honored; defaults throw 500 on it.");
475+
sb.AppendLine(" var jsonOpts = ((Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Http.Json.JsonOptions>)");
476+
sb.AppendLine(" http.RequestServices.GetService(typeof(Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Http.Json.JsonOptions>))!)");
477+
sb.AppendLine(" .Value.SerializerOptions;");
447478
sb.AppendLine(" var entry = db.Entry(existing);");
448479
sb.AppendLine(" foreach (var prop in body.RootElement.EnumerateObject())");
449480
sb.AppendLine(" {");
@@ -455,7 +486,7 @@ private static void AppendTphSubtypeRoutes(
455486
sb.AppendLine(" var clr = System.Nullable.GetUnderlyingType(target.ClrType) ?? target.ClrType;");
456487
sb.AppendLine(" object? value = prop.Value.ValueKind == System.Text.Json.JsonValueKind.Null");
457488
sb.AppendLine(" ? null");
458-
sb.AppendLine(" : System.Text.Json.JsonSerializer.Deserialize(prop.Value.GetRawText(), clr);");
489+
sb.AppendLine(" : System.Text.Json.JsonSerializer.Deserialize(prop.Value.GetRawText(), clr, jsonOpts);");
459490
sb.AppendLine(" entry.CurrentValues[target] = value;");
460491
sb.AppendLine(" }");
461492
sb.AppendLine(" await db.SaveChangesAsync();");

‎server/java/codegen-kotlin/src/main/kotlin/com/metaobjects/generator/kotlin/KotlinSpringControllerGenerator.kt‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,18 @@ open class KotlinSpringControllerGenerator : MultiFileDirectGeneratorBase<MetaOb
359359
for (field in entity.metaFields) {
360360
if (field is ObjectField || field is MapField) continue
361361
if (field.name == pkFieldName) continue
362-
append(" it[${field.name}] = dto.${field.name}\n")
362+
// Partial merge: an OPTIONAL field omitted from a PATCH must not clobber the
363+
// stored value. Optional DTO props are nullable + default to null, so absent
364+
// reads as null here — null-guard them (the same merge the TPH per-subtype
365+
// handler uses). Required props are non-null (Jackson rejects a body missing
366+
// them with a 400 before this runs), so they are always present → written
367+
// unconditionally. Guarding a non-null prop would be an always-true warning
368+
// and break -Werror consumers. (Explicit-null-clears is the deferred tristate.)
369+
if (KotlinGenUtil.isRequiredField(field)) {
370+
append(" it[${field.name}] = dto.${field.name}\n")
371+
} else {
372+
append(" if (dto.${field.name} != null) it[${field.name}] = dto.${field.name}\n")
373+
}
363374
}
364375
append(" }\n")
365376
append(" if (updated == 0) ResponseEntity.status(HttpStatus.NOT_FOUND).body(mapOf(\"error\" to \"not_found\") as Any)\n")

‎server/java/codegen-kotlin/src/test/resources/snapshots/entity-with-tph/acme/auth/AuthLineController.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ class AuthLineController {
282282
@RequestMapping(value = ["/{id}"], method = [RequestMethod.PATCH, RequestMethod.PUT])
283283
fun update(@PathVariable id: Long, @Valid @RequestBody dto: AuthLine): ResponseEntity<Any> = transaction {
284284
val updated = AuthLineTable.update({ AuthLineTable.id eq id }) {
285-
it[label] = dto.label
285+
if (dto.label != null) it[label] = dto.label
286286
}
287287
if (updated == 0) ResponseEntity.status(HttpStatus.NOT_FOUND).body(mapOf("error" to "not_found") as Any)
288288
else {

0 commit comments

Comments
 (0)