Skip to content

Commit 718ce6f

Browse files
dmealingclaude
andcommitted
fix(codegen): payload optionality comes from @required, in every port (#309)
C# emitted `public required` on EVERY payload property regardless of `@required`; Kotlin emitted every property non-null for the same reason. A `template.output` payload is the type an LLM response deserializes into, so neither could represent a response omitting a field the metadata never marked required. The generated C# parser's own doc comment says it throws when the JSON is "missing a `required` property" — the adopter's report, verbatim, from the code. THE RULE: absent `@required` ⇒ optional. `spec/metamodel/field.json` types the attr as an optional boolean defaulting to absent. The DECISION is one cross-language contract; the RENDERING stays idiomatic — C# `T?`, Kotlin `T? = null`, TS `name?: T`, Python `T | None = None`. Only C# and Kotlin changed. TS and Python already read it. Java needed nothing: SpringTypeMapper already emits boxed Integer/Long/Boolean, so every record component is nullable by construction and absent ⇒ optional already held. Getting that right meant checking the type mapper rather than trusting "Java records can't express optional." C# was contradicting ITSELF, not merely the spec: its FR-010 extractor derives required-ness from `@required` and treats an absent optional as benign LOST_OPTIONAL, and `ExtractorGenerator.cs` documents in prose that PayloadCodegen "does not honor @required". One port, two tiers, opposite predicates. Kotlin reuses `KotlinGenUtil.isRequiredField` rather than adding a second predicate. It accepts boolean `true` or the string `"true"`, matching what `KotlinExtractSchemaEmitter` already accepts, so the payload type and the mapper populating it cannot disagree — the lockstep Python's `is_field_required` docstring protects. Python holds the tighter boolean-only threshold on both its tiers, Kotlin the looser one on both. Whether `@required: "true"` is legal metadata at all is a LOADER question and is left open rather than half-decided here. WHY IT SURVIVED FOUR RELEASES: the tests pinned it. C#'s payload suite asserted `public required` on fixtures where NO field carried `@required`, so it could not distinguish "reads the attr" from "hardcodes it" — it would have failed under a correct implementation. A demo test asserted that omitting an unmarked field "fails to compile" while calling them "required members"; it now declares what it asserts and gained the arm nobody had, that omitting an OPTIONAL member must compile. Kotlin's #270 test used non-null as a proxy for "origins don't decide nullability", which the all-non-null emitter also satisfied; it now carries both directions. Every flipped assertion was checked against its own fixture rather than regenerated. That caught a real error mid-fix: `alphaText`/`betaText` in the SHARED corpus ARE `@required: true`, so blanket-flipping them would have replaced one wrong golden with another. Which is the finding — the shared xpkg-collision corpus already carried both arms, and the ports were asserting contradictory output for one shared model. It is now the payload tier's optionality oracle too. Adopter-visible in C# and Kotlin: a payload property that was non-null becomes nullable unless its metadata declares `@required`. Verified: C# 1592 (Codegen 354, +1 new arm) · Kotlin 315 · Java reactor BUILD SUCCESS · codegen-ts 1247 · python codegen 415 · gates 10/10. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DhpswkF1NvwxhFWMmdAT15
1 parent 42a02f9 commit 718ce6f

12 files changed

Lines changed: 180 additions & 59 deletions

File tree

CHANGELOG.md

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,56 @@ command whose whole job is the drift verdict. Pinned by a regression test on the
352352
which needs no Postgres container and covers the silent half: before the fix its failure
353353
output *is* the bug, verbatim.
354354

355+
### Fixed — a payload field's optionality now comes from `@required` in every port ([#309](https://github.com/metaobjectsdev/metaobjects/issues/309), NuGet/Maven)
356+
357+
C# emitted `public required` on **every** generated payload property regardless of
358+
`@required`, and Kotlin emitted every property non-null for the same reason. So a
359+
`template.output` payload — the type an LLM response is deserialized into — could not
360+
represent a response that omits a field the metadata never marked required. The generated
361+
C# parser's own doc comment says it throws when the JSON is *"missing a `required`
362+
property"*, which is #309 exactly: adopting the generator meant rejecting real responses
363+
that a hand-written record accepted.
364+
365+
**The rule, now uniform:** a payload field is optional unless it declares `@required`.
366+
`spec/metamodel/field.json` documents `@required` as an optional boolean defaulting to
367+
absent, so absent ⇒ optional. The **decision** is one cross-language contract; the
368+
**rendering** stays idiomatic — C# `T?`, Kotlin `T? = null`, TypeScript `name?: T`, Python
369+
`T | None = None`.
370+
371+
**Only C# and Kotlin changed.** TypeScript and Python already read `@required`. Java needed
372+
no change and got none: `SpringTypeMapper` already emits boxed `Integer`/`Long`/`Boolean`,
373+
so every record component is nullable by construction and absent ⇒ optional already held.
374+
375+
**C# was contradicting itself, not just the spec.** Its own FR-010 extractor derives
376+
per-field required-ness from `@required` and classifies an absent optional as benign
377+
`LOST_OPTIONAL`; `ExtractorGenerator.cs` even documents that PayloadCodegen *"does not
378+
honor @required"*. One port, two tiers, opposite predicates.
379+
380+
**Kotlin uses that port's existing `KotlinGenUtil.isRequiredField`**, which accepts the
381+
boolean `true` or the string `"true"` — matching what `KotlinExtractSchemaEmitter` already
382+
accepts, so the payload type and the mapper that populates it cannot disagree. That lockstep
383+
is the property Python's `is_field_required` docstring protects; Python holds the tighter
384+
boolean-only threshold on both of its tiers, Kotlin the looser one on both. Whether
385+
`@required: "true"` should be legal metadata at all is a loader question, left open.
386+
387+
**What made this survivable for four releases: the tests pinned it.** C#'s payload suite
388+
asserted `public required` on fixtures where **no field carried `@required`**, so it could
389+
never distinguish "reads the attr" from "hardcodes it" — and one demo test asserted that
390+
omitting an unmarked field "fails to compile" while its comment called them "required
391+
members". That test now declares what it asserts, and gained the arm nobody had: omitting an
392+
**optional** member must compile. Kotlin's `#270` test asserted non-null as a proxy for
393+
"origins don't decide nullability", which the all-non-null emitter also satisfied; it now
394+
carries both arms, so a `@required` field with an `origin.first` stays non-null while an
395+
unmarked sibling with the same origin kind goes nullable.
396+
397+
The shared `template-output-render-conformance/xpkg-collision` corpus turns out to carry
398+
both arms already — `alphaText`/`betaText` are `@required: true` while `fromAlpha`/`fromBeta`
399+
are not — so the ports were asserting **contradictory output for one shared model**. It is
400+
now the payload tier's optionality oracle as well as its collision oracle.
401+
402+
Adopter-visible in C# and Kotlin: a payload property that was non-null becomes nullable
403+
unless its metadata declares `@required`. Mark the fields you genuinely require.
404+
355405
### Fixed — a projection could not borrow an entity's alternate key ([#310](https://github.com/metaobjectsdev/metaobjects/issues/310), all four loaders)
356406

357407
A `object.projection` borrows its key rather than declaring one:

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

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ public class DemoTests
2828
{ "field.string": { "name": "title" } }
2929
]}},
3030
{ "object.projection": { "name": "AuthorBrief", "children": [
31-
{ "field.string": { "name": "displayName" } },
32-
{ "field.int": { "name": "postCount" } },
31+
{ "field.string": { "name": "displayName", "@required": true } },
32+
{ "field.int": { "name": "postCount", "@required": true } },
3333
{ "field.object": { "name": "posts", "isArray": true, "@objectRef": "PostBrief",
3434
"children": [ { "origin.collection": { "@via": "Author.posts" } } ] } }
3535
]}},
@@ -98,10 +98,15 @@ public static string Go(IProvider p)
9898
}
9999

100100
[Fact]
101-
public void Compile_time__a_wrong_shaped_caller_fails_to_compile()
101+
public void Compile_time__a_caller_omitting_a_DECLARED_required_member_fails_to_compile()
102102
{
103-
// The caller omits required members (postCount, posts) — the codegen'd shape
104-
// contract makes this a compile error, not a silent runtime mismatch.
103+
// The caller omits `postCount`, which the metadata marks `@required: true` — the
104+
// codegen'd shape contract makes that a compile error, not a silent runtime mismatch.
105+
//
106+
// #309: this test previously proved the same thing about fields carrying NO
107+
// `@required` at all, because the emitter marked every property `required`. It
108+
// therefore pinned the defect while its comment claimed to demonstrate the design.
109+
// The fixture now declares what the test asserts, so it passes for the stated reason.
105110
var source = GeneratedSource(Load()) + """
106111
public static class BadCaller
107112
{
@@ -113,7 +118,28 @@ public static string Go(IProvider p)
113118
}
114119
""";
115120
var errors = CompileErrors(source);
116-
Assert.True(errors.Count > 0, "expected a wrong-shaped caller to FAIL compilation, but it compiled clean");
121+
Assert.True(errors.Count > 0, "expected a caller omitting a required member to FAIL compilation, but it compiled clean");
122+
}
123+
124+
[Fact]
125+
public void Compile_time__a_caller_omitting_an_OPTIONAL_member_compiles()
126+
{
127+
// The other arm, which no test covered while every property was `required`: `posts`
128+
// carries no `@required`, so omitting it must be legal. This is the shape #309 was
129+
// filed about — an LLM response that simply does not populate an optional field.
130+
var source = GeneratedSource(Load()) + """
131+
public static class PartialCaller
132+
{
133+
public static string Go(IProvider p)
134+
{
135+
var partial = new AuthorBrief { displayName = "Ada", postCount = 1 };
136+
return RenderHandles.RenderContentStrategyPrompt(partial, p);
137+
}
138+
}
139+
""";
140+
var errors = CompileErrors(source);
141+
Assert.True(errors.Count == 0, "expected omitting an OPTIONAL member to compile, got: "
142+
+ string.Join("; ", errors));
117143
}
118144

119145
[Fact]

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

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,11 @@ public void Emits_payload_record_with_scalar_and_nested_array_fields()
4848
{
4949
var src = PayloadCodegen.GeneratePayloadRecords(Load(), "AuthorBrief");
5050
Assert.Contains("public sealed record AuthorBrief", src);
51-
Assert.Contains("public required string displayName { get; init; }", src);
52-
Assert.Contains("public required int postCount { get; init; }", src);
53-
Assert.Contains("public required IReadOnlyList<PostBrief> posts { get; init; }", src);
51+
Assert.Contains("public string? displayName { get; init; }", src);
52+
Assert.Contains("public int? postCount { get; init; }", src);
53+
Assert.Contains("public IReadOnlyList<PostBrief>? posts { get; init; }", src);
5454
Assert.Contains("public sealed record PostBrief", src);
55-
Assert.Contains("public required string title { get; init; }", src);
55+
Assert.Contains("public string? title { get; init; }", src);
5656
}
5757

5858
// Same shape as Model, but the nested @objectRef is authored FULLY-QUALIFIED
@@ -82,7 +82,7 @@ public void Fully_qualified_objectRef_strips_to_bare_record_type_and_resolves_ne
8282
var root = new MetaDataLoader().Load([new InMemoryStringSource(FqnRefModel, id: "fqn.json")]).Root;
8383
var src = PayloadCodegen.GeneratePayloadRecords(root, "AuthorBrief");
8484
// Regression: the FQN must NOT leak into the generated C# type or record name.
85-
Assert.Contains("public required IReadOnlyList<PostBrief> posts { get; init; }", src);
85+
Assert.Contains("public IReadOnlyList<PostBrief>? posts { get; init; }", src);
8686
Assert.Contains("public sealed record PostBrief", src); // nested record DID resolve (FindObject matched the bare name)
8787
Assert.DoesNotContain("acme::ai::", src);
8888
}
@@ -188,13 +188,13 @@ public void No_churn_a_non_colliding_closure_emits_the_exact_pre_fix_bare_name_o
188188
Assert.Equal(
189189
"public sealed record AuthorBrief\n" +
190190
"{\n" +
191-
" public required string displayName { get; init; }\n" +
192-
" public required int postCount { get; init; }\n" +
193-
" public required IReadOnlyList<PostBrief> posts { get; init; }\n" +
191+
" public string? displayName { get; init; }\n" +
192+
" public int? postCount { get; init; }\n" +
193+
" public IReadOnlyList<PostBrief>? posts { get; init; }\n" +
194194
"}\n\n" +
195195
"public sealed record PostBrief\n" +
196196
"{\n" +
197-
" public required string title { get; init; }\n" +
197+
" public string? title { get; init; }\n" +
198198
"}\n",
199199
src);
200200
}
@@ -245,16 +245,16 @@ public void Emits_two_distinct_package_qualified_records_not_one_merged_first_wi
245245
Assert.Equal(
246246
"public sealed record Digest\n" +
247247
"{\n" +
248-
" public required AcmeAlphaNote fromAlpha { get; init; }\n" +
249-
" public required AcmeBetaNote fromBeta { get; init; }\n" +
248+
" public AcmeAlphaNote? fromAlpha { get; init; }\n" +
249+
" public AcmeBetaNote? fromBeta { get; init; }\n" +
250250
"}\n\n" +
251251
"public sealed record AcmeAlphaNote\n" +
252252
"{\n" +
253-
" public required string alphaText { get; init; }\n" +
253+
" public string? alphaText { get; init; }\n" +
254254
"}\n\n" +
255255
"public sealed record AcmeBetaNote\n" +
256256
"{\n" +
257-
" public required string betaText { get; init; }\n" +
257+
" public string? betaText { get; init; }\n" +
258258
"}\n",
259259
src);
260260
Assert.DoesNotContain("record Note", src);

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

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -83,11 +83,11 @@ public void Emits_full_record_for_bare_payloadRef_in_a_named_package()
8383
var file = Assert.Single(files);
8484
Assert.Equal("ClassificationResponse.payload.cs", file.Path);
8585
Assert.Contains("public sealed record ClassificationResponse", file.Content);
86-
Assert.Contains("public required string documentType { get; init; }", file.Content);
87-
Assert.Contains("public required double confidence { get; init; }", file.Content);
88-
Assert.Contains("public required NoteEntry note { get; init; }", file.Content);
86+
Assert.Contains("public string? documentType { get; init; }", file.Content);
87+
Assert.Contains("public double? confidence { get; init; }", file.Content);
88+
Assert.Contains("public NoteEntry? note { get; init; }", file.Content);
8989
Assert.Contains("public sealed record NoteEntry", file.Content);
90-
Assert.Contains("public required string value { get; init; }", file.Content);
90+
Assert.Contains("public string? value { get; init; }", file.Content);
9191
// The FQN must not leak into the file name or the record/type names.
9292
Assert.DoesNotContain("acme::intake::", file.Content);
9393
Assert.DoesNotContain("::", file.Path);
@@ -149,11 +149,11 @@ public void Collision_within_one_payloadRefs_closure_emits_two_distinct_qualifie
149149
Assert.Equal("Digest.payload.cs", file.Path);
150150
Assert.Contains("public sealed record Digest", file.Content);
151151
Assert.Contains("public sealed record AcmeAlphaNote", file.Content);
152-
Assert.Contains("public required string alphaText { get; init; }", file.Content);
152+
Assert.Contains("public string? alphaText { get; init; }", file.Content);
153153
Assert.Contains("public sealed record AcmeBetaNote", file.Content);
154-
Assert.Contains("public required string betaText { get; init; }", file.Content);
155-
Assert.Contains("public required AcmeAlphaNote fromAlpha { get; init; }", file.Content);
156-
Assert.Contains("public required AcmeBetaNote fromBeta { get; init; }", file.Content);
154+
Assert.Contains("public string? betaText { get; init; }", file.Content);
155+
Assert.Contains("public AcmeAlphaNote? fromAlpha { get; init; }", file.Content);
156+
Assert.Contains("public AcmeBetaNote? fromBeta { get; init; }", file.Content);
157157
Assert.DoesNotContain("public sealed record Note", file.Content);
158158
}
159159

@@ -202,14 +202,14 @@ public void Origin_children_are_ignored_for_typing_declared_type_wins()
202202
var file = Assert.Single(files);
203203
Assert.Equal("Digest.payload.cs", file.Path);
204204
// Declared `field.int` wins over the (`@convert`-acknowledged) string passthrough.
205-
Assert.Contains("public required int alias { get; init; }", file.Content);
205+
Assert.Contains("public int? alias { get; init; }", file.Content);
206206
Assert.DoesNotContain("string alias", file.Content);
207207
// Declared `field.string` wins over origin.collection — no list, no via-target type.
208-
Assert.Contains("public required string summary { get; init; }", file.Content);
208+
Assert.Contains("public string? summary { get; init; }", file.Content);
209209
// Declared `field.object @objectRef` + isArray wins over the disagreeing @via walk.
210-
Assert.Contains("public required IReadOnlyList<Highlight> posts { get; init; }", file.Content);
210+
Assert.Contains("public IReadOnlyList<Highlight>? posts { get; init; }", file.Content);
211211
Assert.Contains("public sealed record Highlight", file.Content);
212-
Assert.Contains("public required string snippet { get; init; }", file.Content);
212+
Assert.Contains("public string? snippet { get; init; }", file.Content);
213213
// The ignored @via entity never enters the closure.
214214
Assert.DoesNotContain("record Post", file.Content);
215215
Assert.DoesNotContain("internalNotes", file.Content);

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -245,12 +245,17 @@ public void Document_DigestDoc_resolves_fqn_nested_objectRef_across_collision()
245245
// merged `{ alphaText; betaText }` shape.
246246
Assert.Contains("public sealed record AcmeAlphaNote", records);
247247
Assert.Contains("public sealed record AcmeBetaNote", records);
248+
// #309 — this fixture carries BOTH arms, which is what makes it the payload tier's
249+
// optionality oracle as well as its collision oracle: the shared corpus declares
250+
// `alphaText`/`betaText` as `@required: true` (meta.alpha.json / meta.beta.json)
251+
// while `fromAlpha`/`fromBeta` carry no `@required` (meta.app.json). A port that
252+
// hardcodes either answer now fails on the other half of the same model.
248253
Assert.Contains("public required string alphaText { get; init; }", records);
249254
Assert.Contains("public required string betaText { get; init; }", records);
250255
Assert.DoesNotContain("public sealed record Note", records);
251256
// Digest's own fields point at the qualified names, not at each other's field.
252-
Assert.Contains("public required AcmeAlphaNote fromAlpha { get; init; }", records);
253-
Assert.Contains("public required AcmeBetaNote fromBeta { get; init; }", records);
257+
Assert.Contains("public AcmeAlphaNote? fromAlpha { get; init; }", records);
258+
Assert.Contains("public AcmeBetaNote? fromBeta { get; init; }", records);
254259

255260
var payloadSrc = "namespace Acme.Generated;\n" + records;
256261
var asm = CompileToAssembly(file.Content, payloadSrc);

server/csharp/MetaObjects.Codegen/PayloadCodegen.cs

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,22 @@ private static void EmitClosureRecords(
332332
// Scalar array (e.g. field.string with isArray) -> a list of the scalar.
333333
type = IsArrayField(f) ? $"IReadOnlyList<{scalar}>" : scalar;
334334
}
335-
lines.Add($" public required {type} {f.Name} {{ get; init; }}");
335+
// #309 — optionality comes from the DECLARED @required, like every other
336+
// tier. `required` on an unmarked field made the generated
337+
// `JsonSerializer.Deserialize<Payload>` reject any response omitting it,
338+
// which is fatal for the payload's main job: absorbing an LLM response that
339+
// does not always populate every field. It also contradicted this port's own
340+
// sibling — the FR-010 extractor derives per-field required-ness from
341+
// `@required` and classifies an absent optional as benign LOST_OPTIONAL.
342+
//
343+
// `spec/metamodel/field.json` documents @required as an optional boolean
344+
// defaulting to absent, so absent ⇒ optional. Rendered idiomatically per
345+
// language: C# uses `required` + non-nullable for a required field and a
346+
// nullable property with no `required` otherwise.
347+
bool isRequired = f.Attr(FIELD_ATTR_REQUIRED) is true;
348+
lines.Add(isRequired
349+
? $" public required {type} {f.Name} {{ get; init; }}"
350+
: $" public {type}? {f.Name} {{ get; init; }}");
336351
}
337352
lines.Add("}");
338353
output.Add(string.Join("\n", lines));

0 commit comments

Comments
 (0)