fix responding with application/json #604

lag-of-death · 2019-09-12T10:10:54Z

Fixes #599

Checklist

Tests have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What is the current behavior? What is the new behavior?

Handling application/json in Accept header is inconsistent.
This PR implements a small-footprint solution. But we should really consider a solution similar to the one proposed in: #568

The issue:

We receive: Accept: application/json, text/plain
Fastify looks into the serializers and sees that there is no serializer for application/json, but founds text/plain serializer and uses this. This is wrong. This happens because our application/json serializer is a fallback serializer (when no serializer in serializers (https://github.com/stoplightio/prism/blob/master/packages/http-server/src/serializers.ts) is found)

Why the added charset=utf8?

This is to solve the inconsistency we have. This was also addressed in #568, but didn't make it to master.

An example of the inconsistency from master:

prism/packages/http-server/src/__tests__/server.oas.spec.ts

Lines 309 to 319 in 887e2d0

    
           it('returns a valid response when multiple choices are given', async () => { 
        
             const response = await server.fastify.inject({ 
        
               method: 'GET', 
        
               url: '/pets/10', 
        
               headers: { 
        
                 accept: 'idonotexist/something,application/json', 
        
               }, 
        
             }); 
        
             expect(response.statusCode).toBe(200); 
        
             expect(response.headers).toHaveProperty('content-type', 'application/json; charset=utf-8');

prism/packages/http-server/src/__tests__/server.oas.spec.ts

Lines 323 to 332 in 887e2d0

    
           const response = await server.fastify.inject({ 
        
             method: 'GET', 
        
             url: '/pets/10', 
        
             headers: { 
        
               accept: 'application/json,application/xml', 
        
             }, 
        
           }); 
        
           expect(response.statusCode).toBe(200); 
        
           expect(response.headers).toHaveProperty('content-type', 'application/json');

So, in the first example we have application/json; charset=utf-8, but in the second one only application/json.
And why is this happening? Because #568 (comment)

philsturgeon · 2019-09-13T16:36:30Z

CHANGELOG.md

@@ -22,6 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Prism is correctly handling the `csv` collection format argument property in OAS2 documents #577
 - Prism is correctly returning the response when the request has `*/*` as Accept header #578
 - Prism is correctly returning a single root node with the payload for XML data #578
+- Prism is correctly serializing application/json responses


These changelog messages are not as effective at communicating what happened.

"Prism is" we dont need this because we know its Prism
"correctly" we know we fixed something this in in the fixed section

What was fixed? What was it doing before? What is the scenario in which application/json scenarios were broken?

People read changelogs to find out if a specific situation that they might have noticed has been fixed, but this is not giving them that information. It's just kinda vaguely saying "something about json works better than before"

The correct changeling here would be a line explaining that Prism is now giving higher priority to application/json Accept content type, and it's not a "fallback" format anymore.

Moreover this is not in the right section, it should go in the Unreleased part

philsturgeon · 2019-09-13T17:28:25Z

packages/http-server/src/server.ts

@@ -41,6 +41,10 @@ export const createServer = (operations: IHttpOperation[], opts: IPrismHttpServe
  });

  server.addHook('onSend', (request, reply, payload, done) => {
+    if (reply.getHeader('content-type') === 'application/json') {


What is the reasoning for this? If the api designer has defined the header as application/jsonwhy do we want to force the utf8 charset on there?

What about support for application/{dataType}+json or application/json; charset=cp1250?

Source: https://tools.ietf.org/html/rfc6839

@philsturgeon, @StefanDywersant, charset=utf-8 was already enforced by fastify with its default application/json serializer. We sometimes returned application/json without charset=utf-8 because... the response was wrongly serialized by some other serializer (like instead of a serializer for JSON, the data was serialized with a serializer for XML). application/{whatever}+json is here https://github.com/stoplightio/prism/blob/master/packages/http-server/src/serializers.ts#L9
Also, haven't found other charset than utf-8 logic in the code

What about support for application/{dataType}+json

This scenario is already supported in the serialiser. I would not bother about the charset — it is actually something that is inferred by the browser most of the times. I am quite sure there was also a proposal to remove it completely from the content type header.

philsturgeon · 2019-09-13T17:29:58Z

test-harness/specs/E2E-Accept-Content-Negotiation-AC-15.oas2.txt

+          examples:
+            application/json:
+              schema:
+                nullable: true


This test might just show that no body is output when schema is a null, maybe the more standard type: string would create more confidence that this test is doing what we want for the reasons we want.

I agree with @philsturgeon here and more importantly 204 responses do not have content and so Fastify will delete it no matter if you're trying to send it to the client, so let's change this to a regular 200.

XVincentX

Couple of changes requested, but this is going in the right direction.

@StefanDywersant I am starting to agree with you and @lag-of-death that the current serialiser package is giving us more headache than solving us problems — so I think you were right and we'll probably look for a better solution soon.

In mean time, let's stick with what we have right now.

XVincentX · 2019-09-17T11:40:35Z

packages/http-server/src/serializers.ts

@@ -11,6 +11,13 @@ export default [
    },
    serializer: JSON.stringify,
  },
+  {
+    regex: {


You do not need to create a new section, you can simply modify the one at line 9 and add application/json in the array

XVincentX · 2019-09-17T11:41:54Z

packages/http-server/src/server.ts

@@ -41,6 +41,10 @@ export const createServer = (operations: IHttpOperation[], opts: IPrismHttpServe
  });

  server.addHook('onSend', (request, reply, payload, done) => {
+    if (reply.getHeader('content-type') === 'application/json') {


What about support for application/{dataType}+json

This scenario is already supported in the serialiser. I would not bother about the charset — it is actually something that is inferred by the browser most of the times. I am quite sure there was also a proposal to remove it completely from the content type header.

XVincentX · 2019-09-17T11:42:37Z

test-harness/specs/E2E-Accept-Content-Negotiation-AC-15.oas2.txt

+          examples:
+            application/json:
+              schema:
+                nullable: true


I agree with @philsturgeon here and more importantly 204 responses do not have content and so Fastify will delete it no matter if you're trying to send it to the client, so let's change this to a regular 200.

XVincentX · 2019-09-17T11:42:52Z

test-harness/specs/E2E-Accept-Content-Negotiation-AC-15.oas3.txt

+  /check-email:
+    get:
+      responses:
+        "204":


Ditto, do not use 204 here.

Suggested change

"204":

"200":

XVincentX · 2019-09-17T11:43:06Z

test-harness/specs/E2E-Accept-Content-Negotiation-AC-4.oas2.txt

@@ -22,7 +22,7 @@ mock -p 4010
 curl -i -X GET http://localhost:4010/todos -H "accept: application/xml, application/json"
 ====expect====
 HTTP/1.1 200 OK
-content-type: application/json
+content-type: application/json; charset=utf-8


It's ok to return just application/json

XVincentX · 2019-09-17T11:43:58Z

CHANGELOG.md

@@ -22,6 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Prism is correctly handling the `csv` collection format argument property in OAS2 documents #577
 - Prism is correctly returning the response when the request has `*/*` as Accept header #578
 - Prism is correctly returning a single root node with the payload for XML data #578
+- Prism is correctly serializing application/json responses


The correct changeling here would be a line explaining that Prism is now giving higher priority to application/json Accept content type, and it's not a "fallback" format anymore.

lag-of-death added 2 commits September 12, 2019 12:09

fix: fix responding with application/json

cbd2e00

test: add test

8a9de64

lag-of-death changed the title ~~[wip] fix: fix responding with application/json~~ fix responding with application/json Sep 12, 2019

lag-of-death requested review from XVincentX, philsturgeon, mallachari, karol-maciaszek and pytlesk4 September 12, 2019 11:06

chore: add changelog line

aad810c

philsturgeon reviewed Sep 13, 2019

View reviewed changes

XVincentX suggested changes Sep 17, 2019

View reviewed changes

XVincentX added 2 commits September 17, 2019 15:04

Merge branch 'master' into fix/handle-accept-header

9ae4c72

fix: code review addressings (#622)

19f1f45

XVincentX merged commit 8ec1761 into master Sep 17, 2019

XVincentX deleted the fix/handle-accept-header branch September 17, 2019 14:07

lag-of-death mentioned this pull request Sep 17, 2019

fix: improved serializer selection #620

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix responding with application/json #604

fix responding with application/json #604

lag-of-death commented Sep 12, 2019 •

edited

Loading

philsturgeon Sep 13, 2019

XVincentX Sep 17, 2019

XVincentX Sep 17, 2019

philsturgeon Sep 13, 2019

karol-maciaszek Sep 16, 2019

lag-of-death Sep 16, 2019 •

edited

Loading

XVincentX Sep 17, 2019

philsturgeon Sep 13, 2019

XVincentX Sep 17, 2019

XVincentX left a comment

XVincentX Sep 17, 2019

XVincentX Sep 17, 2019

XVincentX Sep 17, 2019

XVincentX Sep 17, 2019

XVincentX Sep 17, 2019

XVincentX Sep 17, 2019

	it('returns a valid response when multiple choices are given', async () => {
	const response = await server.fastify.inject({
	method: 'GET',
	url: '/pets/10',
	headers: {
	accept: 'idonotexist/something,application/json',
	},
	});

	expect(response.statusCode).toBe(200);
	expect(response.headers).toHaveProperty('content-type', 'application/json; charset=utf-8');

fix responding with application/json #604

fix responding with application/json #604

Conversation

lag-of-death commented Sep 12, 2019 • edited Loading

Checklist

What is the current behavior? What is the new behavior?

Why the added charset=utf8?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lag-of-death Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XVincentX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lag-of-death commented Sep 12, 2019 •

edited

Loading

lag-of-death Sep 16, 2019 •

edited

Loading