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

Remove validation of 204 responses in .validate() #90

Merged
merged 1 commit into from Feb 24, 2018

Conversation

OscarApeland
Copy link
Contributor

@OscarApeland OscarApeland commented Feb 23, 2018

Right now, the default .validate() fails when receiving a HTTP 204 NO CONTENT response trough Express, as Express strips away those fields when not needed.

If the maintainers agree, I'd like to remove 204 from the standard .validate() method in ResponseValidation.swift.

Edit:
A more detailed explanation of why it doesn't work: If you set application/json as Accept and receive a 204, Express strips the Content-Type field making NSURLRequest do its best to guess the Content-Type itself, and as Express does chunk = '' on a 204, it assumes its text/plain. That obviously fails content type validation.

@vadymmarkov
Copy link
Owner

@OscarApeland Is it always like that for 204 response or it only happens while using Express?

@OscarApeland
Copy link
Contributor Author

OscarApeland commented Feb 23, 2018

@vadymmarkov We just use Express, so I don't have any idea about others. I thought that as 204's are NO CONTENT they shouldn't need to have their content validated anyway, just the headers.

@JulianNymark
Copy link

JulianNymark commented Feb 23, 2018

from https://tools.ietf.org/html/rfc7231#section-6.3.5, there's this:

A 204 response is terminated by the first empty line after the header
fields because it cannot contain a message body.

It doesn't seem to put any restrictions / requirements on the headers, other than:

Metadata in the
response header fields refer to the target resource and its selected
representation after the requested action was applied.

It seems Express only wipes the headers related to content on the grounds that it makes sense, (as implied by the comment // strip irrelevant headers in the response.js code.)

@vadymmarkov
Copy link
Owner

Ok, thanks for info @JulianNymark. I think it makes sense to do this change @OscarApeland 👍

@vadymmarkov vadymmarkov merged commit 35de669 into vadymmarkov:master Feb 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants