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
feat: Supporting different content-type responses #4264
feat: Supporting different content-type responses #4264
Conversation
Signed-off-by: iifawzi <iifawzie@gmail.com>
if this got approved I will start looking at how should we update |
Signed-off-by: iifawzi <iifawzie@gmail.com>
194d73d
to
e433365
Compare
Signed-off-by: iifawzi <iifawzie@gmail.com>
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.
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?
Signed-off-by: iifawzi <iifawzie@gmail.com>
Signed-off-by: iifawzi <iifawzie@gmail.com>
Signed-off-by: iifawzi <iifawzie@gmail.com>
Signed-off-by: iifawzi <iifawzie@gmail.com>
I will take a look at fastify-swagger to see how possible the syntax can be rendered |
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 I'm still making sure of this, if confirmed, I'm proposing to change the syntax to something similar to the sample below. Introducing 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 |
…sponses Signed-off-by: iifawzi <iifawzie@gmail.com>
8e17a24
to
833dfab
Compare
I've opened a PR at |
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
Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
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.
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.
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. |
Signed-off-by: iifawzi <iifawzie@gmail.com>
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' }
}
}
}
}
}
} |
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. fastify/test/schema-serialization.test.js Line 125 in 6b31fbf
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:
|
I suggest to create a separate issue to track what you needs. |
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. |
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. |
Signed-off-by: iifawzi <iifawzie@gmail.com>
The last commit is addressing the newest APIs that allow the usage of the serializers during the lifetime of a request. |
Signed-off-by: iifawzi <iifawzie@gmail.com>
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.
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 👍🏽
Signed-off-by: iifawzi <iifawzie@gmail.com>
Signed-off-by: iifawzi <iifawzie@gmail.com>
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.
Nice, LGTM 👍
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.
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' })
}
We can target v5 for a follow up PR? |
@climba03003 @Uzlopak PTAL |
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.
Yeah adding to v5 makes sense
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. |
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
npm run test
andnpm run benchmark
and the Code of conduct