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.

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

Error when REST response is empty #107

Closed
isopterix opened this issue May 7, 2018 · 18 comments
Closed

Error when REST response is empty #107

isopterix opened this issue May 7, 2018 · 18 comments
Labels
enhancement💡 feature Feature: new addition or enhancement to existing solutions help wanted 🛠 question❔
Milestone

Comments

@isopterix
Copy link

I have a mutation request which when successful responds with a 204 "No content" status. When coupled with apollo-link-error I am constantly getting a network error: Unexpected end of JSON input.

It appears that apollo-link-rest is trying to parse the empty body of the response.

Any ideas how to mitigate this? BTW I am trying to talk to a SharePoint REST api, so there is not much I can to tweak the server's response.

There should be a way to tell the apollo-link-rest how to deal with an empty response...

My mutation call:

                  <Mutation mutation={M_WRITE_PROJECT_DETAILS}>
                    {(writeProjectDetails, { data }) => (
                      <Form.Button
                        content="Save"
                        onClick={() => {
                          const token = localStorage.getItem("token");
                          writeProjectDetails({
                            variables: {
                              projectId: project.Id,
                              input: {
                                __metadata: {
                                  type: "SP.Data.ProjectListItem"
                                },
                                Title: project.Title
                              }
                            },
                            context: {
                              headers: {
                                "X-RequestDigest": token,
                                "X-HTTP-Method": "MERGE",
                                "IF-MATCH": "*"
                              }
                            }
                          });
                        }}
                      />
                    )}
                  </Mutation>

The corresponding gql query:

const M_WRITE_PROJECT_DETAILS = gql`
  mutation writeToSPList($projectId: String, $input: String) {
    writeToSPList(projectId: $projectId, input: $input)
      @rest(
        type: "Project"
        path: "/web/lists/GetByTitle('Projects')/items(:projectId)"
        method: "POST"
      ) {
      NoResponse
    }
  }
`;

"NoResponse" is obviously null since there is no response, but then again I cannot send a mutation without any response fields... unless I am missing something.

@fbartho
Copy link
Collaborator

fbartho commented May 7, 2018

@isopterix -- I don't think we really have an easy way to deal with this right now. -- My initial thought would be to recommend you wrap Fetch & provide a custom Fetch implementation that replaces the body of 204 responses with {} -- I think without building a better feature, that's the only way I could do this today!

@marnusw
Copy link
Contributor

marnusw commented May 12, 2018

@isopterix I tried to reproduce this issue with the test below, but the test passes for an empty body. It appears that fetch response.json() returns {} when the body is an empty string. Could you try to trace this more closely in your live setup to see where the error occurs?

  describe('response parsing', () => {
    it('supports empty response bodies', async () => {
      expect.assertions(1);

      const link = new RestLink({ uri: '/api' });

      fetchMock.post('/api/posts', {
        status: 204,
        body: '',
      });

      const mutation = gql`
        mutation postEmptyResponse($input: String!) {
          newPost(input: $input)
            @rest(type: "Post", path: "/posts", method: "POST") {
            NoResponse
          }
        }
      `;
      const {data} = await makePromise<Result>(
        execute(link, {
          operationName: 'postEmptyResponse',
          query: mutation,
          variables: { input: 'Love apollo' },
        }),
      );

      expect(data).toEqual({
        newPost: {
          NoResponse: null,
          __typename: 'Post',
        }
      });
    });
  });

@marnusw
Copy link
Contributor

marnusw commented May 12, 2018

@isopterix never mind my last comment. It seems the browser implementations are less forgiving than the test environment.

@marnusw
Copy link
Contributor

marnusw commented May 12, 2018

The one problem that I see with detecting an empty body is that there is no sure way to tell what is in the body without calling res.json() or res.text(), and always calling both just in case is too redundant. Otherwise one might look at the Content-Length header, but I'm not too sure if it will always be reliable. Another option would be only to parse JSON if the Content-Type is application/json, but it's also conceivable that some APIs might set that header in common for all responses and even on an empty 204 response.

Since the rest link produces a networkError on any status code above 300 the response parsing need only be sufficiently specialized to handle the 2xx status codes correctly. Of the ones one might reasonably expect to encounter, being:

  • 200 OK
  • 201 Created
  • 202 Accepted
  • 204 No Content
  • (any of the others?)

the 204 responses should be the only one to have an empty body. Would it be an option to inspect the status code and return a default {} when it is 204? While it would be a test for a very specific edge case it would make the library more HTTP standard compliant which seems to be a good thing.

@fbartho would you have any preference for one of these approaches? I'll make the PR if one of these options, or a combination thereof, sounds reasonable.

@fbartho
Copy link
Collaborator

fbartho commented May 12, 2018

Perhaps there’s a middle path here? Could both behaviors be supported?

I’m pretty supportive of interpreting 204 as {} by default myself, or as “null” is null the semantic equivalent of no content in JSON?

@marnusw
Copy link
Contributor

marnusw commented May 12, 2018

I think in order for the upstream GraphQL interpretation of the empty body to work we should return {} which will then result in

data: {
  __typename: "...",
  NoResponse: null,
}

When you say "Could both behaviors be supported?" do you mean looking at the 204 status code and looking at the headers? I guess when the Content-Length is actually present and also 0 then we could take that as another indication of an empty body and return the default.

Although I proposed the Content-Type header earlier I'm actually not exactly clear on what should be done when it is set to something other than json. I think if this header is to be interpreted it will probably need to be done with a responseSerializer config option similar to the proposed bodySerializer. That is probably overkill though, and wouldn't address the empty body issue in particular.

I'm happy to do the 204 and Content-Length implementation.

@isopterix
Copy link
Author

So far I followed the earlier advice and implemented a custom fetch to handle the 204 response. But I had difficulty to get a proper response back. Having the option to get this functionality out of the box would be awesome.

Unfortunately, I am still learning how to navigate the JS world...

@fbartho fbartho added enhancement💡 help wanted 🛠 question❔ feature Feature: new addition or enhancement to existing solutions labels May 14, 2018
@fbartho fbartho added this to the v0.3.0 milestone May 22, 2018
@fbartho
Copy link
Collaborator

fbartho commented May 22, 2018

Fixed via #111 !

@fbartho fbartho closed this as completed May 22, 2018
@isopterix
Copy link
Author

isopterix commented May 25, 2018

Sweet! Thank you very much.

Now, there is only one little problem still left :) Right after I worked out my own temporary solution, I ran into yet another problem with the REST API of my target SharePoint Server... If you delete an entry you get a 200 response with no body content :)

Could we potentially generalize the behavior, i.e. if there is no body content respond with the "NoResponse" tag? If I am not mistaken the patch currently only addresses the 204 special case.

@marnusw
Copy link
Contributor

marnusw commented May 25, 2018

@isopterix If the server sets the Content-Length to zero properly it should be working even on 200 responses, the intent is to check for that as well.

@isopterix
Copy link
Author

Looking forward to give it a spin.

@juanparadox
Copy link

Does this cover any successful response with empty content? Like a 200 with no content?

@marnusw
Copy link
Contributor

marnusw commented Dec 11, 2018

Yes. The fix that was merged for this checks for a 204 status or a Content-Length: 0 header and returns an empty object.

If you have a 200 empty response without a Content-Length header it will not work since the body is not parsed for this check.

@dljcollette
Copy link

@isopterix are you able to confirm that this works? I'm trying to do a mutation which gives me a 200 status and Content-Length: 0. However, I'm still getting a Network error: Unexpected end of JSON input. I'm using version 0.7.0.

@dljcollette
Copy link

I see that the response headers are empty when debugging apollo-link-rest, while my browser is actually showing headers. Does apollo-link-rest manipulate / reset the headers in the response object?

@fbartho
Copy link
Collaborator

fbartho commented Mar 20, 2019

Apollo-link-rest does not manipulate response headers @dljcollette! Hope that helps

@thomaszdxsn
Copy link

I meet same error

has any workaround for status 200 with empty body?

@marnusw
Copy link
Contributor

marnusw commented Sep 12, 2019

@thomaszdxsn the only way to accommodate that would be to use res.text() instead of res.json() and check for valid JSON before parsing with JSON.parse(). This means you loose out on the streaming JSON parsing and as such is not something likely to be done.

The best solution would be to change your API so it uses the semantically correct status code 204 when the body is empty, or to return something in the 200 response body. I think even an empty JSON encoded object would work, as long as there is just something in the response body that is valid JSON.

If you really can't change the API, perhaps you can pass a custom fetch function into apollo-link-rest were you could inspect and change the response before eventually returning for ALR to process. It'll be a hack, but it might work for you.

Or, again, just ensure the API returns the Content-length: 0 header in the response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement💡 feature Feature: new addition or enhancement to existing solutions help wanted 🛠 question❔
Projects
None yet
Development

No branches or pull requests

6 participants