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 on deleteEntity #60

Closed
DanBradbury opened this issue Sep 13, 2016 · 9 comments
Closed

Error on deleteEntity #60

DanBradbury opened this issue Sep 13, 2016 · 9 comments
Labels

Comments

@DanBradbury
Copy link
Contributor

I'm getting a strange error when deleting a resource using deleteEntity.

Uncaught (in promise) TypeError: Cannot read property 'indexOf' of null(..)
            (anonymous function)    @   utils.js:32
            (anonymous function)    @   utils.js:31

from utils.js

    if (res.status >= 200 && res.status < 300) {
      if (jsonContentTypes.some(function (contentType) {
        return res.headers.get('Content-Type').indexOf(contentType) > -1; //breaks here
      })) {
        return res.json();
      }

      return res;
    }

The DELETE request is successful and the server responds with 204 No Content

Since the server is correctly implementing the jsonapi spec for delete (using jsonapi-resources) I'd expect there to be no problem when the response is handled by redux-json-api

We were thinking this could have just been an issue with a missing Content-Type header but from the HTTP/1.1 RFC we see that Content-Type isn't even mandatory when content is available.

Any HTTP/1.1 message containing an entity-body SHOULD include a Content-Type header field defining the media type of that body

From reading up on jsonapi-resources they made a conscious decision to not include Content-Type on 204 No Content which makes sense to me (no content is coming back in the response so why provide a Content-Type for something that doesn't exist)

Is this something that should be fixed on the server side? Or would it make more sense to adjust the above code to handle the res.status === 204 case?

@Realtin
Copy link

Realtin commented Sep 13, 2016

I have the same issue.

@egeriis egeriis added the bug label Sep 13, 2016
@egeriis
Copy link
Collaborator

egeriis commented Sep 13, 2016

That's interesting. I never noted that the spec notes:

Any requests that contain content

That's definitely an oversight.

I reckon a quick fix would be a check on status code, as you suggest. The correct solution would probably be to check if there is any data in the response body, but I am not even sure that JSON API allows GET, PATCH and POST response body to be empty.

Are you up for making a PR @DanBradbury?

@Petesta
Copy link
Contributor

Petesta commented Sep 13, 2016

😄

@DanBradbury
Copy link
Contributor Author

DanBradbury commented Sep 13, 2016

When updating relationships w/ PATCH it may return a 204

If an update is successful and the server doesn’t update any attributes besides those provided, the server MUST return either a 200 OK status code and response document (as described above) or a 204 No Content status code with no response document.

The same is true for creating resources (POST); must return either 201 or 204 on success

I wasn't aware of any 204s until this morning and can submit a PR to take a look at the status code. I'm under the impression that whenever we have a 204 status the body should be empty (not sure if there is a case where you'd return any other status code with an empty body)

@egeriis
Copy link
Collaborator

egeriis commented Sep 14, 2016

not sure if there is a case where you'd return any other status code with an empty body

No, I think you're right. At least that would fall out of scope, as the API doesn't follow the spec then.

@DanBradbury
Copy link
Contributor Author

I went ahead and took a stab at taking care of this. Please review PR #61

@zweck
Copy link

zweck commented Sep 17, 2016

I'm having this same issue, our JSON-API serializer (https://github.com/fortunejs/fortune-json-api) seems to return 204 for updates by default which is also throwing:

Uncaught (in promise) TypeError: Cannot read property 'indexOf' of null(..)
            (anonymous function)    @   utils.js:32
            (anonymous function)    @   utils.js:31

@evolve2k
Copy link
Contributor

Pull request #61 which resolves this issue has been merged in the last day or so.

@egeriis This issue can now be closed 🚪

@egeriis
Copy link
Collaborator

egeriis commented Sep 23, 2016

I forgot to close the issue 🙂 Expected it to be closed automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants