-
Notifications
You must be signed in to change notification settings - Fork 343
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: handle */* accept header #568
Conversation
packages/http-server/src/server.ts
Outdated
if (output && output.headers && output.headers['Content-type'].includes('application/json')) { | ||
// why would only application/json have the charset? | ||
reply.header('content-type', output.headers['Content-type'] + '; charset=utf-8'); | ||
if (contentType && contentType.includes('application/json')) { |
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.
This TODO was asking a good question, why are we trying to ensure utf-8 is specified in there? Stuff might not be, what if the content type is application/xml; charset=iso-8859-1
, would this make it: application/xml; charset=iso-8859-1; charset=utf-8
?
@XVincentX do you know what the original intent was here?
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.
could be something to consider for another PR 👍
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.
Sadly it is directly related to this code because you are adding this functionality in this PR and I am not sure we should be adding this functionality.
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.
@philsturgeon, I'm not adding anything big really. I just fixed some discrepancies. In most of the tests, json responses had charset=utf-8
, so I added that to the responses that didn't have it.
} | ||
|
||
if (contentType && contentType.includes('application/json')) { | ||
reply.header('content-type', isInUTF8 ? contentType : contentType + '; charset=utf-8'); |
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.
This code is forcing '; charset=utf-8')
onto all responses even when nothing anywhere in the OpenAPI document or anywhere in the HTTP request or anywhere has suggested that we should. Could you explain your reasoning for adding this? Unless there is a compelling reason to add this (which I cannot think of¡) then we should not be adding this.
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.
@philsturgeon, so, it was Fastify that was adding the charset. All of the responses to requests that asked for application/json
were serialized with the default Fastify serializer. This serializer was adding this encoding. In the cases of the adjusted tests files, ; charset=utf-8
was missing. Why? Because of -H "accept: application/xml, application/json"
. So Fastify sees this and decides to serialize the response with
return !!typeIs.is(value, ['application/xml', 'application/*+xml']); |
; charset=utf-8
(because it no longer used the default serializer). And why did this even work? Because of serialize: (data: unknown) => (typeof data === 'string' ? data : xmlSerializer.parse(data)), |
application/json
in the content-type
, because it was done by prism/packages/http-server/src/server.ts
Line 89 in 264d8b2
const response = await prismInstance.process(input, operations); |
basically, my work was done with consistency in mind, but I'm happy to not force that encoding for application/json
. Will need to adjust the tests though :)
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 do not think all this reorganisation is required. All we need to do is to add a hook before sending the payload and if we detect that the accept is */*
, handle the payload differently.
Something among these lines should work and keep the current structure as it is.
server.addHook('onSend', (request, reply, payload, done) => {
if (request.headers.accept === '*/*') {
const s = reply.hasHeader('content-type')
? serializers.find(s => s.regex.test(reply.getHeader('content-type')!))
: serializers[0];
if (s) {
return done(undefined, s.serializer(payload));
}
return done(undefined, payload);
}
});
I've put a simplified and less invasive solution in #578. |
Superseded by #578 |
Fixes #492
Checklist
What kind of change does this PR introduce?
Bug fix.
Does this PR introduce a breaking change?
No.