Repository navigation
Add strong OpenAPI schema tooling - #182
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f72bd4eed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !config.strongSchemas | ||
| && config.pathParameterPlacement | ||
| == SwaggerGenerationConfig.PathParameterPlacement.PATH | ||
| && (!config.includeMethodNotAllowedEndpoints | ||
| || !config.includeFieldValidation); |
There was a problem hiding this comment.
When callers use the new asJson(SwaggerGenerationConfig) overload with includeMethodNotAllowedEndpoints=false and includeFieldValidation=false, this predicate marks the configuration cacheable and the later branch returns apiNormal. The resulting JSON therefore has required request bodies and typed path parameters even though validation was explicitly disabled; cache only the exact normal and permissive profiles, or serialize this combination directly.
Useful? React with 👍 / 👎.
| // add individual entity schema | ||
| ObjectSchema object = asObjectSchema(objectSchemaDefinition); | ||
| ObjectSchema object = | ||
| asObjectSchema(objectSchemaDefinition, false, config.strongSchemas); | ||
| components.addSchemas(objectSchemaDefinition.getName(), object); | ||
|
|
||
| // add create schema with ID removed for auto added ids | ||
| ObjectSchema createObject = asCreateObjectSchema(objectSchemaDefinition); | ||
| ObjectSchema createObject = | ||
| asCreateObjectSchema(objectSchemaDefinition, config.strongSchemas); |
There was a problem hiding this comment.
With strongSchemas=true, the flag is applied to the singular entity and create components here, but the plural component is still built by asJsonCollectionObjectSchema, whose item schema calls the default asObjectSchema(..., false). Consequently a single-object response advertises min/max/pattern constraints while the same objects returned from a collection do not; pass the strong-schema setting through the collection and XML collection helpers too.
Useful? React with 👍 / 👎.
| private void ensureThingifierErrorSchema(final Components components) { | ||
| if (components.getSchemas() != null | ||
| && components.getSchemas().containsKey(THINGIFIER_ERROR_SCHEMA_NAME)) { | ||
| return; |
There was a problem hiding this comment.
If a model already defines an entity or view named ThingifierError, this early return treats that unrelated component as the standard error schema. Every generated 4xx/5xx response will then reference the user's schema rather than an object containing errorMessages; reserve a collision-proof component name or verify/replace the existing component before reusing it.
Useful? React with 👍 / 👎.
| if (rule instanceof MatchesRegexValidationRule) { | ||
| schema.setPattern(((MatchesRegexValidationRule) rule).getRegexToMatch()); |
There was a problem hiding this comment.
For a MatchesRegexValidationRule, runtime validation uses Java Matcher.matches() and requires the entire value to match, whereas OpenAPI/JSON Schema pattern succeeds when any substring matches. For example, a rule [A-Z]+ rejects abcABC at runtime but the generated strong schema accepts it; wrap this rule's expression with whole-string anchors (while handling existing anchors) rather than copying it directly.
Useful? React with 👍 / 👎.
| Assertions.assertEquals( | ||
| "string", createTodoProperties.getAsJsonObject("title").get("type").getAsString()); | ||
| Assertions.assertEquals( | ||
| "boolean", | ||
| createTodoProperties.getAsJsonObject("doneStatus").get("type").getAsString()); | ||
| Assertions.assertEquals( | ||
| "integer", todoProperties.getAsJsonObject("id").get("type").getAsString()); |
There was a problem hiding this comment.
This test checks separate string, boolean, and integer mapping branches in one broad method. Convert these equivalent mapping examples into a parameterized table (including component/property/type), so each row identifies the broken type mapping and the scenario matrix remains visible as required by the repository test rules.
AGENTS.md reference: AGENTS.md:L17-L20
Useful? React with 👍 / 👎.
| Assertions.assertEquals( | ||
| "title", createTodo.getAsJsonArray("required").get(0).getAsString()); | ||
| Assertions.assertEquals(1, title.get("minLength").getAsInt()); | ||
| Assertions.assertEquals(50, title.get("maxLength").getAsInt()); |
There was a problem hiding this comment.
The assertions combine three independent decisions—mandatory fields becoming required, not-empty becoming minLength, and maximum length becoming maxLength—so one test covers multiple domain rules. Split these into named tests so each validation-rule mapping fails independently.
AGENTS.md reference: AGENTS.md:L6-L8
Useful? React with 👍 / 👎.
| .onMissingRequiredCredential(401, "Missing scoped session") | ||
| .onInvalidCredential(403, "Invalid scoped session") |
There was a problem hiding this comment.
Missing credentials and invalid credentials are separate authentication decisions with independently configurable statuses, but this test configures and verifies both together. Give each policy its own test so a regression in one credential path is isolated rather than sharing setup and intent with the other.
AGENTS.md reference: AGENTS.md:L6-L8
Useful? React with 👍 / 👎.
1 participant
Summary
strongschema=trueandpathparams=operationcontrols for generated OpenAPI docsThingifierErrorVerification
mvn -pl thingifier -am testpassed earlier: 1123 tests, 0 failuresmvn -pl thingifier -am -DskipTests verifypassed earliergit diff --checkpassed earlier