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

Assert the proper status code for PUT #38

Closed
wants to merge 1 commit into from

Conversation

untitaker
Copy link
Member

@untitaker untitaker commented Jun 11, 2016

See https://tools.ietf.org/html/rfc7231#section-4.3.4

Money quote:

If the target resource does not have a current representation and the
PUT successfully creates one, then the origin server MUST inform the
user agent by sending a 201 (Created) response.  If the target
resource does have a current representation and that representation
is successfully modified in accordance with the state of the enclosed
representation, then the origin server MUST send either a 200 (OK) or
a 204 (No Content) response to indicate successful completion of the
request.

@untitaker
Copy link
Member Author

untitaker commented Jun 11, 2016

There are more instances in the code where this is a problem, will fix all of them eventually.

EDIT: Done

See https://tools.ietf.org/html/rfc7231#section-4.3.4

Money quote:

    If the target resource does not have a current representation and the
    PUT successfully creates one, then the origin server MUST inform the
    user agent by sending a 201 (Created) response.  If the target
    resource does have a current representation and that representation
    is successfully modified in accordance with the state of the enclosed
    representation, then the origin server MUST send either a 200 (OK) or
    a 204 (No Content) response to indicate successful completion of the
    request.
@raucao
Copy link
Member

raucao commented Jun 12, 2016

It's a MUST in the spec, but in real-world implementations of both servers and clients it's basically treated as a SHOULD and most programs send and accept 200s. If the storage backend doesn't inform your middleware, then it can be expensive to look it up first.

How about making it a warning instead of a failure (similar to how we treat content types)?

@untitaker
Copy link
Member Author

I agree that nobody would care about this part. Warning sounds fine, but then I'd propose that warnings are fatal by default as well.

@untitaker
Copy link
Member Author

I.e. you specifically need to opt out of a spec violation instead of opt in. Perhaps the user should be able to selectively skip tests and warnings.

@raucao
Copy link
Member

raucao commented Jun 12, 2016

Maybe expressive override keys in the config file?

@untitaker
Copy link
Member Author

Yes. I'd say it's only useful in combination with #33 because otherwise errors might pop up in subsequent tests, which you'll also have to skip.

@untitaker
Copy link
Member Author

Closing this for now.

@untitaker untitaker closed this Aug 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants