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

Update put.js send 200 or 204 depending on pre-existance of resource #1785

Merged
merged 5 commits into from
May 31, 2024

Conversation

jeff-zucker
Copy link
Member

The HTTP spec says that the Server MUST return a 200 or 204 if PUT replaces and existing resource. See second paragraph of https://httpwg.org/specs/rfc9110#PUT.

Brought up by @CxRes.

The HTTP spec says that the Server MUST return a 200 or 204 if PUT replaces and existing resource.  See second paragraph of https://httpwg.org/specs/rfc9110#PUT.

Brought up by @CxRes.
@jeff-zucker jeff-zucker requested a review from bourgeoa May 24, 2024 19:46
@zg009
Copy link
Contributor

zg009 commented May 25, 2024

@jeff-zucker Just to confirm, should the successful PUT return a 200 or a 204, or the 200 and 201 as you changed?

@jeff-zucker
Copy link
Member Author

@jeff-zucker Just to confirm, should the successful PUT return a 200 or a 204, or the 200 and 201 as you changed?

My understanding is that

  • when PUT creates a new file, return 201
  • when PUT replaces an existing file and content is sent back, return 200
  • when PUT replaces an existing file and no content is sent back, return 204

I don't know in what circumstances, if any, NSS returns content from a PUT. If that never happens, my PR should be changed to make 204 the response when file pre-exists. If there are times when NSS does return content, those times should trigger a change from 204 to 200.

@CxRes
Copy link
Collaborator

CxRes commented May 25, 2024

I don't know in what circumstances, if any, NSS returns content from a PUT.

If that is the case, changing 201 to 204 is adequate.

I would update this, but I to have @jeff-zucker do it seems easier.

@jeff-zucker
Copy link
Member Author

I would update this, but I to have @jeff-zucker do it seems easier.

Done.

@zg009
Copy link
Contributor

zg009 commented May 25, 2024

The tests are passing but I'll wait for Alain's input.

@CxRes
Copy link
Collaborator

CxRes commented May 25, 2024

@zg009 Some tests are failing, but that is mostly a simple matter of changing 201 to 204 status codes. What is non-trivial here is that we need to add a few more test cases to distinguish between 201 and 204 cases (i.e. we want coverage for both), which will add a few more tests.

@zg009
Copy link
Contributor

zg009 commented May 28, 2024

@CxRes Can you tell me which tests or the output? For some reason all of mine are still passing. I need to check if there is something wrong with my local repository or if it is another issue.

@bourgeoa
Copy link
Member

@zg009 You can see this is the GitHub CI.
I suppose your local version needs a git pull

lib/handlers/put.js Outdated Show resolved Hide resolved
@CxRes CxRes self-requested a review May 29, 2024 17:50
CxRes and others added 2 commits May 29, 2024 23:21
Co-authored-by: Alain Bourgeois <alain.bourgeois10@gmail.com>
@CxRes CxRes changed the title Update put.js send 200 or 201 depending on prexistance of resource Update put.js send 200 or 204 depending on prexistance of resource May 30, 2024
@CxRes CxRes changed the title Update put.js send 200 or 204 depending on prexistance of resource Update put.js send 200 or 204 depending on pre-existance of resource May 30, 2024
@bourgeoa bourgeoa merged commit f589a20 into main May 31, 2024
2 checks passed
@bourgeoa bourgeoa deleted the jeff-zucker-patch-1 branch May 31, 2024 06:45
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.

4 participants