Skip to content
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.

Sign up for GitHub

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

apollo-datasource-rest: Dont' parse as json when status code is 204 No Content #2446

Merged
merged 6 commits into from
Apr 5, 2019
+21 −0
Merged

apollo-datasource-rest: Dont' parse as json when status code is 204 No Content #2446

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
24236c7
"HTTP 204 No content" should not parse as json, even if application/j…
bartverbruggen Mar 14, 2019
6b788bf
Merge branch 'master' of github.com:apollographql/apollo-server
bartverbruggen Mar 18, 2019
9aa3342
update test
bartverbruggen Mar 18, 2019
1c797a4
Merge branch 'master' of github.com:apollographql/apollo-server
bartverbruggen Apr 4, 2019
38e600d
use AND instead of OR for the response.status 😔
bartverbruggen Apr 4, 2019
935f3bd
Update RESTDataSource.ts
abernix Apr 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions packages/apollo-datasource-rest/src/RESTDataSource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,10 @@ export abstract class RESTDataSource<TContext = any> extends DataSource {
protected parseBody(response: Response): Promise<object | string> {
const contentType = response.headers.get('Content-Type');
if (
contentType &&
(contentType.startsWith('application/json') ||
contentType.startsWith('application/hal+json'))
response.status !== 204 ||
Copy link
Member

@abernix abernix Apr 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you didn't mean to put this as an && inside the parenthesis that contain contentType.startsWith(...) || ...?

The failing tests are because of Content-type: text/plain and you're always short-circuiting when the response.status !== 204, which would be most all of the time, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is embarrassing..., it, indeed, should have been && instead of ||, it is not necessary to put it inside the parenthesis because it should pass all three conditions.

Thank you very much!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to be embarrassed. That's what failing tests and code-reviews are meant to help with. 😄

Thank you for the first-time contribution!

(contentType &&
(contentType.startsWith('application/json') ||
contentType.startsWith('application/hal+json')))
) {
return response.json();
} else {
Expand Down
18 changes: 18 additions & 0 deletions packages/apollo-datasource-rest/src/__tests__/RESTDataSource.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,24 @@ describe('RESTDataSource', () => {

expect(data).toEqual('bar');
});

it('returns data as a string when response status code is 204 no content', async () => {
const dataSource = new class extends RESTDataSource {
baseURL = 'https://api.example.com';

getFoo() {
return this.get('');
}
}();

dataSource.httpCache = httpCache;

fetch.mockResponseOnce('', { 'Content-Type': 'application/json' }, 204);

const data = await dataSource.getFoo();

expect(data).toEqual('');
});
});

describe('memoization', () => {
Expand Down