Skip to content
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

fix: nested examples in OpenAPI schemas for request body & response #772

Merged
merged 6 commits into from
Jan 15, 2024

Conversation

shfrmn
Copy link
Contributor

@shfrmn shfrmn commented Dec 13, 2023

Summary

Fixes #771

This PR addresses the following issue — example values in nested schemas of request body and response are not getting transformed into OpenAPI compliant format. For example, fastify schema below is not being processed correctly. Specifically .examples field is currently preserved as is on any nested properties, even though array is not a valid format for OpenAPI examples.

{
  type: 'object',
  properties: {
    hello: {
      type: 'string',
      examples: ['world']
    }
  }
}

OpenAPI spec for example values

I will spell out my current understanding of the standard in case it will help someone in the future.

  • In general, example values can be in two following formats
  • However, .examples is not supported at the top level of .schema objects, in .properties of objects and .items of arrays. This means those are only allowed to have a single example.
  • Despite that, top-level of request body and response schemas can still have multiple examples, because it's possible to places .examples field outside of schema — this behavior is already implemented and covered by the test suite.
  • Parameters (path, query and headers) can have either a single example or multiple examples (in the object format)

Change description

Code

Tests

Overall, I found current test coverage for example values very lacking and introduced much more extensive set of tests. Because these tests are very repetitive I had to decide whether to generalize them or not. I opted for explicit repetitive testing of each combination: single/multiple example values, parameters/body/response, shallow/deeply nested schemas, strings/numbers/objects/arrays. I added some grouping to the tests and believe explicitly testing each use case is the most maintainable approach within the current testing setup, but imho using fast-check would definitely help with repetitiveness and test quality.

The combined test file diff on this PR is a bit of a spaghetti monster, so I broke it down into two commits that are much easier to review separately.

Commit 1 - removed some tests:

  • Two tests here were against the specification (asserted incorrect output)
  • Four other tests that I removed for clarity of the diff output and reintroduced in the next commit

Commit 2 - added 30 explicit tests for each significant combination of configuration

Documentation

  • Updated example in the README to match the implementation

Checklist

@shfrmn shfrmn changed the title Fix: Incorrect format of nested examples in OpenAPI schemas for request body & response Fix: Nested examples in OpenAPI schemas for request body & response Dec 13, 2023
@shfrmn shfrmn changed the title Fix: Nested examples in OpenAPI schemas for request body & response fix: nested examples in OpenAPI schemas for request body & response Dec 13, 2023
})

test('uses examples if has property required in body', async (t) => {
test('marks request body as required', async (t) => {
Copy link
Contributor Author

@shfrmn shfrmn Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test had a wrong description. I fixed it since it mentions examples, but doesn't test anything related to them.

Originally introduced in #541

test('transforms examples in example if single string example', async (t) => {
t.plan(2)
const fastify = Fastify()
test('move examples from "x-examples" to examples field', async (t) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x-examples behavior & test are unchanged, this is just how the diff shows.

@shfrmn
Copy link
Contributor Author

shfrmn commented Dec 14, 2023

Apologies for changes after I already requested a review, but I realized there was a better implementation.

My initial solution, that you can find in the earlier commits, made changes to schemaToMedia function and introduced a boolean parameter that was used in recursion. The downsides of that approach were:

  • The purpose of schemaToMedia is to augment schema into a media type object, but I was misusing it for transforming schema examples. In the process examples were first reassigned from schema to media and then back.
  • Making changes to a function that's already in use is not ideal
  • The second boolean parameter was awkward

New solution, even though more LoC, has several advantages:

  • Doesn't mess with schemaToMedia
  • Explicit intent (transforms examples within schema objects)
  • Extensible, because it allows to easily add support for more nested schema types in the future (e.g. anyOf and allOf). I'm leaving it out of the scope of this PR, but might submit another one later.
  • Decouples extraction of nested schemas from example resolution

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@shfrmn
Copy link
Contributor Author

shfrmn commented Jan 15, 2024

@mcollina When would be a good time to merge this?

@mcollina mcollina merged commit b1258ca into fastify:master Jan 15, 2024
19 checks passed
@mcollina
Copy link
Member

Now ;)

@shfrmn
Copy link
Contributor Author

shfrmn commented Jan 17, 2024

@mcollina Sorry for bothering you, but 8.14 didn't get published after merging. Not sure if it's a CI issue or needs to be done manually.

@climba03003
Copy link
Member

climba03003 commented Jan 17, 2024

@shfrmn Published.
https://www.npmjs.com/package/@fastify/swagger/v/8.14.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Example values for individual request body fields are not compliant with OpenAPI specification
3 participants