Skip to content

Commit 4ccdd2e

Browse files
dmealingclaude
andcommitted
fix(codegen-spring): a POST must not validate the server-generated key out of the body
No Java adopter could create anything through the generated API for an entity whose primary key is `@required` — the shape every real model has, because a shared BaseEntity declares `id` required. The vanilla create handler validated the whole request DTO: if (!validator.validate(dto).isEmpty()) -> 400 {"error":"validation"} SpringDtoGenerator emits ONE record for both request and response, so a `@required` PK earns @NotNull — true of the RESPONSE, false of the request, because `identity.primary @generation:uuid` means the database supplies the key. Proven against a running port with two requests: the body an adopter sends 400s, the same body plus a client-invented id 201s. The only workaround defeated the declared key strategy. The generator already knew the rule. settableFields() returns "effective scalars MINUS the primary key MINUS the discriminator" and its javadoc says the PK "is auto-generated ... so neither is validated from the body" — applied on the TPH create path and to <Entity>Patch, never on the vanilla one. That is why a TPH entity POSTed fine while a plain one did not: same model, same generator, two write paths, one of which had learned the rule. The vanilla create now emits SERVER_OWNED_ON_CREATE (primary-key components plus @autoset) and filters violations by top-level property name. validate(dto) is KEPT rather than narrowed to per-property validateValue, because it is what cascades @Valid into nested value objects: the violations are filtered, not the validation weakened, and a nested path like "labels[0].code" cannot match a top-level name, so cascaded constraints still reject. An entity with no server-owned component keeps its previous handler byte-for-byte. No gate could have caught this. The api-contract corpus's only entity declares Author.id with NO @required, and SpringControllerGeneratorTest's AUTHOR_FIXTURE has the same blind spot — so the DTO never annotated the key, the guard had nothing to over-reject, and both lanes stayed green in all five ports. The new REQUIRED_PK_FIXTURE carries the shape they lacked; making the shared corpus PK @required is recommended separately, since widening a cross-port gate may turn other ports red and is a decision to take knowingly. Found by building a Java adopter app against the published plugin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015NrWUxA6euwvnFcwKxcK6A
1 parent 3e1a8c0 commit 4ccdd2e

3 files changed

Lines changed: 188 additions & 1 deletion

File tree

‎CHANGELOG.md‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,35 @@ edit (two registered `description` strings) and was ruled a hold, as 1.0.4's was
189189

190190
### Fixed
191191

192+
- **Java: no POST could create anything for an entity whose primary key is `@required`.** The
193+
generated Spring controller's vanilla create handler validated the WHOLE request DTO:
194+
`if (!validator.validate(dto).isEmpty()) return 400 {"error":"validation"}`. But
195+
`SpringDtoGenerator` emits ONE record used as both request and response body, so a `@required`
196+
primary key earns `@NotNull` — a true statement about the RESPONSE and a false one about the
197+
request, since `identity.primary @generation: uuid|increment` means the database supplies the
198+
key and the caller must not invent one. Every create therefore answered 400, and the only
199+
workaround was a client-supplied primary key, which defeats the declared generation strategy.
200+
201+
The generator already knew the rule and one door never called it:
202+
`SpringDtoGenerator.settableFields(...)` returns "effective scalars MINUS the primary key MINUS
203+
the discriminator", and its javadoc states it outright — *"the PK is auto-generated and the
204+
discriminator is injected from the URL, so neither is validated from the body."* It was applied
205+
on the TPH create path and to `<Entity>Patch`. So a TPH entity POSTed fine while a plain one
206+
did not: same model, same generator, two write paths, one of which had learned the rule.
207+
208+
The vanilla create now emits a `SERVER_OWNED_ON_CREATE` set (primary-key components plus
209+
`@autoSet` columns) and filters violations by top-level property name. `validator.validate(dto)`
210+
is KEPT rather than narrowed to per-property `validateValue`, because it is what cascades
211+
`@Valid` into nested value objects — the violations are filtered, not the validation weakened,
212+
and a nested path like `labels[0].code` can never match a top-level name, so cascaded
213+
constraints still reject. An entity with no server-owned component keeps its previous handler
214+
byte-for-byte.
215+
216+
**No corpus could have caught it**, which is worth recording: the api-contract corpus's only
217+
entity declares `Author.id` with no `@required`, so no port's DTO ever annotated the key and
218+
both lanes stayed green everywhere. Found by building a Java adopter app, where a shared
219+
`BaseEntity` declares `id` required — the shape real models actually have.
220+
192221
- **Java/Kotlin: a `<loader><libraries>` entry naming a LAYER (`iam/db`) failed the build,
193222
so the JVM could opt into a library's design but never its tables.** Libraries are
194223
layered and the selection is layer-granular — `"iam"` is the inert core, `"iam/db"` the

‎server/java/codegen-spring/src/main/java/com/metaobjects/generator/spring/SpringControllerGenerator.java‎

Lines changed: 69 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,7 @@ protected void emit(MetaObject entity, Path outRoot) {
227227
}
228228
src.append(");\n\n");
229229
appendSortDefaultOrders(src, entity, sortFields);
230+
appendServerOwnedOnCreate(src, entity);
230231

231232
// Repository wiring — constructor injection (Spring's recommended idiom; avoids
232233
// field-injection magic, plays well with final fields + final test seams). The
@@ -296,7 +297,31 @@ protected void emit(MetaObject entity, Path outRoot) {
296297
// any field-constraint violation BEFORE persisting.
297298
src.append(" @PostMapping\n");
298299
src.append(" public ResponseEntity<?> create(@RequestBody ").append(dtoName).append(" dto) {\n");
299-
src.append(" if (!validator.validate(dto).isEmpty()) {\n");
300+
// A caller does not supply server-owned columns, so a @NotNull on one must not reject
301+
// the request. The DTO is BOTH the request and the response body, so `id` carries
302+
// @NotNull because it is @required in the RESPONSE — while being identity.primary
303+
// @generation:uuid, i.e. a column the database defaults and the caller must not invent.
304+
// Validating the whole DTO therefore made every POST a 400 for any entity whose PK is
305+
// @required, and the only "workaround" was a client-invented primary key.
306+
//
307+
// The TPH create path has always applied this rule, through
308+
// SpringDtoGenerator.settableFields ("the PK is auto-generated and the discriminator is
309+
// injected from the URL, so neither is validated from the body") — which is why a TPH
310+
// entity POSTed fine while a vanilla one did not. This is the vanilla path adopting the
311+
// same rule, so the two write surfaces finally agree.
312+
//
313+
// validator.validate(dto) is KEPT rather than narrowed to per-property validateValue:
314+
// it is what cascades @Valid into nested value objects. The violations are FILTERED,
315+
// and only by TOP-LEVEL property name — a nested path like "labels[0].code" can never
316+
// match, so cascaded constraints still reject.
317+
if (serverOwnedOnCreate(entity).isEmpty()) {
318+
// Byte-identical to the pre-fix handler for an entity with no server-owned column,
319+
// so this change moves only the output it has to.
320+
src.append(" if (!validator.validate(dto).isEmpty()) {\n");
321+
} else {
322+
src.append(" if (validator.validate(dto).stream()\n");
323+
src.append(" .anyMatch(v -> !SERVER_OWNED_ON_CREATE.contains(v.getPropertyPath().toString()))) {\n");
324+
}
300325
src.append(" return ResponseEntity.badRequest().body(Map.of(\"error\", \"validation\"));\n");
301326
src.append(" }\n");
302327
// Issue #203: honor @autoSet — stamp EVERY onCreate AND onUpdate column with now() before
@@ -941,4 +966,47 @@ private static void appendSortDefaultOrders(StringBuilder src, MetaObject owner,
941966
.append(entries).append(");\n\n");
942967
}
943968

969+
/**
970+
* The DTO record components a CALLER never supplies on create, because the server owns them:
971+
* the primary-key field(s) and every {@code @autoSet} column.
972+
*
973+
* <p>This is the same concept {@link SpringDtoGenerator#settableFields(MetaObject, String)}
974+
* already excludes for the TPH create path and the {@code <Entity>Patch} — stated here as the
975+
* COMPLEMENT (what to ignore) rather than the settable set, because the vanilla create
976+
* validates the whole DTO for its {@code @Valid} cascade and filters the result.</p>
977+
*
978+
* <p>Returned as record-component names, which is what a {@code ConstraintViolation}'s
979+
* property path spells for a record.</p>
980+
*/
981+
private static List<String> serverOwnedOnCreate(MetaObject entity) {
982+
List<String> pkFields = entity.getIdentities(true).stream()
983+
.filter(com.metaobjects.identity.MetaIdentity::isPrimary)
984+
.findFirst()
985+
.map(com.metaobjects.identity.MetaIdentity::getFields)
986+
.orElse(List.of());
987+
988+
List<String> out = new ArrayList<>();
989+
for (MetaField f : SpringDtoGenerator.scalarFields(entity)) {
990+
if (pkFields.contains(f.getName()) || AutoSetSupport.isAutoSet(f)) {
991+
String component = SpringNaming.recordComponentName(f.getName());
992+
if (!out.contains(component)) out.add(component);
993+
}
994+
}
995+
return out;
996+
}
997+
998+
private static void appendServerOwnedOnCreate(StringBuilder src, MetaObject entity) {
999+
List<String> serverOwned = serverOwnedOnCreate(entity);
1000+
if (serverOwned.isEmpty()) return; // no constant, and the handler keeps its old shape
1001+
src.append(" /** Components the server owns on create (primary key, @autoSet): a\n");
1002+
src.append(" * @NotNull on one of these describes the RESPONSE shape and must not\n");
1003+
src.append(" * reject a create body that legitimately omits it. */\n");
1004+
src.append(" private static final Set<String> SERVER_OWNED_ON_CREATE = Set.of(");
1005+
for (int i = 0; i < serverOwned.size(); i++) {
1006+
if (i > 0) src.append(", ");
1007+
src.append('"').append(serverOwned.get(i)).append('"');
1008+
}
1009+
src.append(");\n\n");
1010+
}
1011+
9441012
}

‎server/java/codegen-spring/src/test/java/com/metaobjects/generator/spring/SpringControllerGeneratorTest.java‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,4 +246,94 @@ public void viewKindEntitiesAreSkipped() throws Exception {
246246
assertFalse("view-kind entities must NOT produce a controller; saw " + controller + " present",
247247
Files.exists(controller));
248248
}
249+
250+
/**
251+
* The shape every real adopter has and no other fixture in this file had: a primary key
252+
* that is {@code @required} AND server-generated. {@code AUTHOR_FIXTURE}'s {@code id}
253+
* carries no {@code @required}, so the DTO never put {@code @NotNull} on it and the create
254+
* handler's over-broad validation could not be observed — which is exactly why FINDINGS F25
255+
* shipped and why this fixture exists rather than a tweak to that one.
256+
*/
257+
private static final String REQUIRED_PK_FIXTURE = """
258+
{
259+
"metadata.root": { "package": "acme::ship", "children": [
260+
{ "object.entity": { "name": "Parcel", "children": [
261+
{ "field.uuid": { "name": "id", "@required": true } },
262+
{ "field.string": { "name": "code", "@maxLength": 24, "@required": true } },
263+
{ "field.timestamp": { "name": "createdAt", "@required": true } },
264+
{ "source.rdb": { "@table": "parcel" } },
265+
{ "identity.primary": { "name": "pk", "@fields": ["id"], "@generation": "uuid" } }
266+
] } }
267+
] }
268+
}
269+
""";
270+
271+
/**
272+
* A POST must not be rejected for omitting a column the SERVER owns.
273+
*
274+
* <p>The DTO is both the request and the response body, so a {@code @required} primary key
275+
* earns {@code @NotNull} — a true statement about the response, and a false one about the
276+
* request, because {@code @generation: uuid} means the caller must NOT invent the key.
277+
* Validating the whole DTO therefore turned every create into a 400, and the only workaround
278+
* was a client-supplied primary key, which defeats the declared strategy (FINDINGS F25).</p>
279+
*
280+
* <p>The TPH create path already applied this rule via
281+
* {@code SpringDtoGenerator.settableFields}; this pins the vanilla path to the same answer.</p>
282+
*/
283+
@Test
284+
public void createDoesNotValidateServerOwnedColumnsOutOfTheBody() throws Exception {
285+
Path outDir = tempFolder.newFolder("ctrl-pk").toPath();
286+
Path workspace = tempFolder.newFolder("ctrl-pk-fx").toPath();
287+
MetaDataLoader loader = SpringTestFixtures.loadFixture(workspace, "parcel-pk", REQUIRED_PK_FIXTURE);
288+
289+
SpringControllerGenerator gen = new SpringControllerGenerator();
290+
Map<String, String> args = new HashMap<>();
291+
args.put("outputDir", outDir.toString());
292+
gen.setArgs(args);
293+
gen.execute(loader);
294+
295+
String src = Files.readString(outDir.resolve("acme/ship/ParcelController.java"));
296+
297+
assertTrue("the server-owned set must name the generated primary key; saw:\n" + src,
298+
src.contains("SERVER_OWNED_ON_CREATE = Set.of(\"id\")"));
299+
300+
// The create handler must FILTER violations, not simply reject any.
301+
assertTrue("create must ignore violations on server-owned components; saw:\n" + src,
302+
src.contains(".anyMatch(v -> !SERVER_OWNED_ON_CREATE.contains(v.getPropertyPath().toString()))"));
303+
assertFalse("create must no longer reject on ANY violation; saw:\n" + src,
304+
src.contains("if (!validator.validate(dto).isEmpty())"));
305+
306+
// And it must still VALIDATE — the cascade into nested beans is why validate(dto) is
307+
// kept rather than narrowed to per-property validateValue.
308+
assertTrue("create must still run the validator; saw:\n" + src,
309+
src.contains("validator.validate(dto)"));
310+
}
311+
312+
/**
313+
* The rule keys on being the PRIMARY KEY, not on being {@code @required}.
314+
*
315+
* <p>{@code AUTHOR_FIXTURE}'s {@code id} carries no {@code @required}, so the DTO puts no
316+
* {@code @NotNull} on it and the filter has nothing to ignore — but the component is still
317+
* server-owned ({@code @generation: increment}), so the set names it anyway. Pinning this
318+
* keeps the two cases from drifting into two rules: a key the database generates is not
319+
* caller-supplied whether or not the model also calls it required.</p>
320+
*/
321+
@Test
322+
public void serverOwnedKeysOnThePrimaryKeyNotOnRequired() throws Exception {
323+
Path outDir = tempFolder.newFolder("ctrl-nopk").toPath();
324+
Path workspace = tempFolder.newFolder("ctrl-nopk-fx").toPath();
325+
MetaDataLoader loader = SpringTestFixtures.loadFixture(workspace, "author-nopk", AUTHOR_FIXTURE);
326+
327+
SpringControllerGenerator gen = new SpringControllerGenerator();
328+
Map<String, String> args = new HashMap<>();
329+
args.put("outputDir", outDir.toString());
330+
gen.setArgs(args);
331+
gen.execute(loader);
332+
333+
String src = Files.readString(outDir.resolve("acme/blog/AuthorController.java"));
334+
assertTrue("a generated PK is server-owned even without @required; saw:\n" + src,
335+
src.contains("SERVER_OWNED_ON_CREATE = Set.of(\"id\")"));
336+
assertFalse("and the over-broad guard is gone here too; saw:\n" + src,
337+
src.contains("if (!validator.validate(dto).isEmpty())"));
338+
}
249339
}

0 commit comments

Comments
 (0)