feat: Supporting different content-type responses

[iifawzi]

Signed-off-by: iifawzi iifawzie@gmail.com

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

Handling fastify/help#621 and #3902

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• 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

[mcollina]

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?

[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

[mcollina]

lgtm

[climba03003]

I am not against this feature to be added.
But the new syntax looks weird to me. If it can be somehow look like the OpenAPI 3.1 syntax, it looks great and reduce the redundant loop inside @fastify/swagger.

The reason behind is that, OpenAPI and JSON-Schema are moving toward each another. It will certainly reduce the works of schema mutation works in the future.

[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' }
              }
            }
          }
        }
      }
    }

[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

fastify/test/schema-serialization.test.js

Line 125 in 6b31fbf

switch (req.headers.accept) {

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?

[Eomm]

I both agree and don't agree with @Uzlopak 😁

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.

I think here the tests could be just more "real", but the logic is that the dev must set the response.type() I think

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

I don't agree about introducing a new component, we have already the reply serializer, BUT I agree that maybe we could integrate those components instead of relying on the json serializer internal. The concept would be to add a reply serializer with an optional json schema.
I would like to play a bit with this PR and arrange a proposal during the weekend 👍🏽

[metcoder95]

Nice, LGTM 👍

[Eomm]

Regarding this comment: #4264 (review)

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

The target should be something like:

// new feature (now you can define only a plugin-scope function
fastify.setReplySerializer('application/json5', function (payload, statusCode){
  return `my serialized ${statusCode} content: ${payload}`
})

// the route adds the json schema that configures the route's context reply serializer map

function handler(request, reply) {
  reply.type('application/json5')
  reply.send({ fullName: 'Jhon Smith', phone: '01090000000', authMethod: 'google' })
}

[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

[Uzlopak]

Yeah adding to v5 makes sense

[github-actions]

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.