-
-
Notifications
You must be signed in to change notification settings - Fork 215
fix(openapi): always set requestBody.required to true when schema.body exists #903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…y exists Fastify requires a body when schema.body is defined, even if all properties are optional. This ensures the OpenAPI spec correctly reflects Fastify's runtime behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a mismatch between Fastify's runtime validation behavior and the generated OpenAPI specification for request bodies. When a route has schema.body defined, Fastify requires a body to be present (at least {}), even if all properties are optional. The fix ensures requestBody.required is always set to true when schema.body exists.
Key changes:
- Modified
prepareOpenapiMethodto setrequired: truewhen creating therequestBodyobject - Removed conditional
requiredlogic fromresolveBodyParamsthat only set it when there were required properties - Added comprehensive test coverage for the optional-properties case
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lib/spec/openapi/utils.js |
Sets requestBody.required = true unconditionally when schema.body is defined, removing the previous conditional logic based on required properties |
test/spec/openapi/option.test.js |
Adds new test case verifying that requestBody.required is true even when all body properties are optional |
test/spec/openapi/schema.test.js |
Updates existing test assertions to expect required: true in all requestBody objects |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@Fdawgs when you get a moment, could you please take a look at this PR? Thanks! |
ilteoood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
but honestly I think that on the user perspective this could be considered as a breaking change.
Imagine if someone is generating TS types from the fastify's schema: objects that before were not required, now are flagged as required 🤔
gurgunday
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
@gurgunday @Fdawgs can we merge it ? |
|
Not sure if we should count this as a bug fix or semver major I lean towards the latter Cc @mcollina |
|
@Fdawgs @gurgunday just a quick bump — anything else needed before we merge? |
|
Hello team! Any chance that this can be merged sometime soon? Regarding the notes above regarding this being a breaking change. It's important to note that this package is specific to Fastify, and the current implementation is actually broken. For quite some time we've been manually fixing things. @ilteoood mentioned that "objects that before were not required are now flagged as required". While that's correct when focusing only on this package, it's important to state that the objects that were not required were actually ALWAYS required due to how Fastify works. In any case where there was a body defined, the Fastify API would generate a 400 error if no body was sent. So while the generated code that came out of the OpenAPI spec would have an optional body, in reality it has never been optional. The generated API from this plugin has always been broken in this respect, so any usage of the API that backed the generation would always fail if there was a body defined. While it could be said that following proper semver processes, this could be considered a breaking change, I would challenge that simply because the current generated code is broken - it does not create a valid specification based on how Fastify works. Since this is a plugin specific to Fastify, and everyone using Fastify MUST pass at least an empty object when a body is defined, this really is fixing a bug so the generated code matches the expectations of Fastify. Either way - bug fix or major version - it would be nice to not have to patch the package to provide valid functionality, so I appreciate if this PR could be looked at again! Sorry for the long-winded comment! |
|
This is not only a fix for schemas where all props are optional, but also for ones where we use a ref to another schema that has even only required props. +1 |
Fastify requires a body when schema.body is defined, even if all properties are optional. This ensures the OpenAPI spec correctly reflects Fastify's runtime behavior. This solves the issue #894
Problem
When a route has
schema.bodydefined, Fastify requires that the request includes a body (at least{}), even if all properties inside that body are optional. However, the OpenAPI schema generated by@fastify/swaggerwas only settingrequestBody.requiredtotruewhen there were required properties inside the object. If the object had only optional properties, thenrequestBody.requiredwas omitted, incorrectly marking the request body as optional.This mismatch between the OpenAPI spec and Fastify's actual runtime behavior can mislead client code generators and API consumers.
Solution
This fix ensures that
requestBody.requiredis always set totruewhenschema.bodyis defined, regardless of whether any properties inside the body are required. The fix setsrequired: truewhen creating therequestBodyobject inprepareOpenapiMethod, making it explicit that the body is required whenever a body schema is defined.Example
Before (incorrect):
{ "requestBody": { "content": { "application/json": { "schema": { "type": "object", "properties": { "hello": { "type": "string" } } } } } } }After (correct):
{ "requestBody": { "required": true, "content": { "application/json": { "schema": { "type": "object", "properties": { "hello": { "type": "string" } } } } } } }Changes
required: truewhen creatingrequestBodyobject inprepareOpenapiMethodrequired: truewhenschema.bodyis definedFixes the issue where the OpenAPI spec incorrectly showed
requestBodyas optional when Fastify actually requires it at runtime.Checklist
npm run test && npm run benchmark --if-presentand the Code of conduct