nodeSolidServer / node-solid-server

Solid server on top of the file-system in NodeJS
https://solidproject.org/for-developers/pod-server
Other
1.78k stars 299 forks source link

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

Closed jeff-zucker closed 4 months ago

jeff-zucker commented 4 months ago

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.

zg009 commented 4 months ago

@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 commented 4 months ago

@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

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 commented 4 months ago

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 commented 4 months ago

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

Done.

zg009 commented 4 months ago

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

CxRes commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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