fastify / fastify-swagger

Swagger documentation generator for Fastify
MIT License
889 stars 199 forks source link

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

Closed shfrmn closed 6 months ago

shfrmn commented 7 months ago

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.

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:

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

Documentation

Checklist

shfrmn commented 7 months ago

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:

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

shfrmn commented 6 months ago

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

mcollina commented 6 months ago

Now ;)

shfrmn commented 6 months ago

@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 commented 6 months ago

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