feat: Supporting different content-type responses

[iifawzi]

Signed-off-by: iifawzi [email protected]

Hi, this feature supports using different content types responses I've added as many tests as possible to handle all possible cases.

Handling https://github.com/fastify/help/issues/621 and https://github.com/fastify/fastify/issues/3902

Checklist

• [x] run npm run test and npm run benchmark
• [x] tests and/or benchmarks are included
• [x] documentation is changed or added
• [x] commit message and code follows the Developer's Certification of Origin and the Code of conduct

[iifawzi]

if this got approved I will start looking at how should we update fastify-swagger to support this too

[iifawzi]

My only concern is how this syntax will be rendered by fastify-swagger and similar modules. Can you check and possibly open a matching PR there too?

I will take a look at fastify-swagger to see how possible the syntax can be rendered

[iifawzi]

I'm still investigating on the swagger part, the initial thoughts are that the syntax needs to be changed a little bit in order to allow the description and x-response-description properties to be set when different content types are used because it can only be set based on the status code, so having the status code pointing directly to an array wouldn't work.

I'm still making sure of this, if confirmed, I'm proposing to change the syntax to something similar to the sample below. Introducing contentTypes, will make us able to handle the use of multiple type contents appropriately and the use of the statusCode-based properties such as description or anything.

const schema = {
      response: {
        200: {
           description: '....',
           contentTypes: [
                 { 
                      content: 'application/json', 
                      schema: { 
                        id: { type: 'number' }, 
                        name: { type: 'string' } 
                      } 
                },
                {
                  content: 'application/vnd.v1+json',
                  schema: {
                    type: 'array',
                    items: { $ref: 'test' }
                     }
                }
        ],
           
        }
      }
    }

EDIT: I think I can confirm that, it also applies to the headers.

[iifawzi]

I've opened a PR at fastify-swagger, I'm not aware of any other modules that would need to adapt, let me know if there are any. @mcollina

[iifawzi]

I do agree, it makes sense, I will work on making it similar to the OpenAPI Specification as much as possible if not the same.

[iifawzi]

Syntax is now matching the OpenAPI Specification

const schema = {
      response: {
        200: {
          description: 'Response schema that support different content types',
          content: {
            'application/json': {
              schema: {
                name: { type: 'string' },
                image: { type: 'string' },
                address: { type: 'string' }
              }
            },
            'application/vnd.v1+json': {
              schema: {
                type: 'array',
                items: { $ref: 'test' }
              }
            }
          }
        },
        '3xx': {
          content: {
            'application/vnd.v2+json': {
              schema: {
                fullName: { type: 'string' },
                phone: { type: 'string' }
              }
            }
          }
        }
      }
    }

[iifawzi]

are there any updates regarding this?

[mcollina]

@Uzlopak @Eomm PTAL

[Uzlopak]

@mcollina

Please see the discussion regarding deprecating the "short-form" https://github.com/fastify/fastify/pull/4264#discussion_r977505336

[matthyk]

I love this feature! I have implemented a plugin that implements the same functionality with constraints but requires a separate route for each different content type.

Since the schema selection happens based on the content type header, should it be documented in the documentation that the user is responsible for correctly processing the Accept header and setting the appropriate content type header?

[iifawzi]

Just summing this up, last commit includes, changing the use of For in to For of, allowing the default schema to benefit from the feature:

    schema: {
      response: {
        default: {
          content: {
            'application/json': {
              schema: {
                details: { type: 'string' }
              }
            }
          }
        }
      }
    }

Adding the contentType to the compile arguments:

    for (const mediaName of Object.keys(schema.content)) {
          const contentSchema = schema.content[mediaName].schema
          contentTypesSchemas[mediaName] = compile({
            schema: contentSchema,
            url,
            method,
            httpStatus: statusCode,
            contentType: mediaName
          })
        }

I still need to document that the contentType option is now exposed, somewhere in the docs.

Tests are added to ensure the additions.

[mcollina]

@Eomm PTAL

[Fdawgs]

@Uzlopak mind taking a gander when you get a spare second as well, please?

[Uzlopak]

I write this here and not in the comment above because I think we need all interested contributors to give feedback. So especially @mcollina @Eomm @jsumners @Fdawgs @climba03003

First of all @iifawzi did a great job and this PR is already processed for about a month. So dont get demotivated.

The current implementation is focussing on the fastify-swagger integration. It makes sense in the way, that other content type responses can be declared in the schema.

But in my personal opinion this PR is lacking the aspect that we have to integrate it properly into fastify-core.

I think the issue is best described by the unit test, which @iifawzi provided. See https://github.com/fastify/fastify/blob/6b31fbf948873d5f0d809501b09796a8b9ce7926/test/schema-serialization.test.js#L125

So what happens is, that the serialization is done in the handler. Which is imho an anti pattern. The unit tests is also just working as expected because all of the content types are variations of json.

What we actually need is an equivalent to the ContentTypeParser but for Serializiation.

This is what I would expect on the fastify-core part:

const Fastify = require('fastify')
const xmlSerializer = require('some-randowm-xml-serializer')

const fastify = new Fastify()
fastify.addContentTypeSerializer('text/xml', {}, xmlSerializer)

  fastify.get('/', {
    schema: {
      response: {
        200: {
          content: {
            'application/json': {
              schema: {
                name: { type: 'string' },
                image: { type: 'string' },
                address: { type: 'string' }
              }
            },
            'text/xml': {
              schema: {
                name: { type: 'string' },
                image: { type: 'string' },
                address: { type: 'string' }
              }
            }
          }
        }
      }
    }
  }, function (req, reply) {
    reply.send({ id: 1, name: 'Foo', image: 'profile picture', address: 'New Node' })
  })

So instead of having a big switch case where the content-type header sent is sent by the handler, we should let fastify core handle how to serialize it. I mean for this we have the onSerialization lifecycle step.

Also we have to keep in mind, that we use default because we define default as application/json. But using / as content type means default. We dont handle that case.

We should also keep in mind, that we can have weighted values for the Accept-header. So we dont handle them with this PR. This PR expects to have only one value for the accept header.

So what I propose:

• [ ] implement a ContentTypeSerializer
• [ ] use fastify-accept-negotiator to handle accept header

[climba03003]

So what I propose:

@fastify/accepts is designed to support accept header. The user should determine should they support the content-type.

@fastify/accepts-serializer is designed to support custom content-type that is not a json.

I suggest to create a separate issue to track what you needs. This PR is independent feature and should works with @fastify/accepts, @fastify/accepts-serializer and even @fastify/swagger.

[Uzlopak]

The unit test would not work out of the box with this implementatiion, if modified like this:

  fastify.inject({ method: 'GET', url: '/', headers: { Accept: 'text/html, application/xhtml+xml, application/json;q=0.9' } }, (err, res) => {
    t.error(err)
    t.equal(res.headers['Content-Type'], 'application/json')
    t.equal(res.payload, JSON.stringify({ name: 'Foo', image: 'profile picture', address: 'New Node' }))
    t.equal(res.statusCode, 200)
  })

Maybe we need to integrate @fastify/accepts-serializer into core. Or we need to improve the documentation and give an example on how to do it with @fastify/accepts-serializer

But based on the unit test I would not get a clue that I actually should use @fastify/accepts-serializer to not be forced to bloat the handler.

[iifawzi]

I just want to share how I thought about this, firstly, this shouldn't always be handled the same way as I handled it in the unit test, I just needed to have that big switch case to simplify the test process and test all different cases.

In the end, this feature is checking the content type returned from the handler, it doesn't check the Accept header, so there will be no issue on Fastify side given the modified example above, it's left to the developer how he can benefit from the feature (that if the handler returned a specific content-type, we will validate the response based on that content-type), How the handler is returning different content types? it's left to the developer, he might not even use that switch case, he might have some different business logic that return a content-type based on anything else.

[iifawzi]

The last commit is addressing the newest APIs that allow the usage of the serializers during the lifetime of a request. Kindly, can you take another look @metcoder95?

[mcollina]

I think that implementing this feature in a different way would be a breaking change.

We can target v5 for a follow up PR?

[mcollina]

@climba03003 @Uzlopak PTAL

[github-actions[bot]]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.