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

switch from 404 to 200 response from REST API when there's no error #658

Merged
merged 2 commits into from
Jun 1, 2023

Conversation

KipK
Copy link
Collaborator

@KipK KipK commented May 25, 2023

Fix browser console spawning HTTP 404 error when API answer was correct. Was preventing to catch real 404 errors.

@jeremypoulter , do you think we should also change the {msg: "no override"} and co by empty objects {} ?

@KipK KipK requested a review from jeremypoulter May 25, 2023 10:28
@jeremypoulter
Copy link
Collaborator

Yes an empty object should be returned (apart from the DELETE) where we are also returning {"msg":...} in other 200 cases

@jeremypoulter
Copy link
Collaborator

We may also have to consider replacing the DELETE method with something else... I think that is the main reason I went for 404

@jeremypoulter
Copy link
Collaborator

Actually some of those should be 404, eg if you fetch /schedule/1234 and that does not exist 404 is valid to return. However when you are posting to a fixed resource like /limits an empty object is more appropriate.

Also on the subject of HTTP error codes I noticed a few cases where JSON parsing is returning 500 these should be a 4xx return code asit is a client error not a server error, probably 400

@KipK
Copy link
Collaborator Author

KipK commented May 27, 2023

Also on the subject of HTTP error codes I noticed a few cases where JSON parsing is returning 500 these should be a 4xx return code asit is a client error not a server error, probably 400

HTTP 415 should also fit well for this :
The HTTP 415 Unsupported Media Type client error response code indicates that the server refuses to accept the request because the payload format is in an unsupported format.

@KipK
Copy link
Collaborator Author

KipK commented May 27, 2023

We may also have to consider replacing the DELETE method with something else... I think that is the main reason I went for 404

not sure of what to do on this. DELETE still looks appropriated to remove ressources. What would fits better according to you?

@jeremypoulter
Copy link
Collaborator

So I think for specifiic instances of things (eg /schedule/1234) we stick to DELETE with 404 return code. If the client is fetching these when they don't exist I see that as a bug or assumption on the client side. the client should use the /schedule endpoint to get the list of valid resources.

For other cases where it is a fixed resource we should POST (or maybe PUT) something to clear the resource, probably and empty object so it then makes sense to get back an empty object when you fetch that endpoint again.

on the JSON parsing I think we go with 400 seems more appropriate and inlne with other uses in the API.

@KipK
Copy link
Collaborator Author

KipK commented May 27, 2023

For other cases where it is a fixed resource we should POST (or maybe PUT) something to clear the resource, probably and empty object so it then makes sense to get back an empty object when you fetch that endpoint again.

Agree /limit PUT {} , /override PUT {} , sounds logical.
Shouldn't we also use PUT for creating them?
and what about /claim/{id} and /schedule/{id}, switch to PUT too ?

@jeremypoulter
Copy link
Collaborator

jeremypoulter commented May 27, 2023

Maybe we can do the PUT changes as a separate PR (or rather expanding the scope of where PUT is used)

@KipK
Copy link
Collaborator Author

KipK commented May 27, 2023

Maybe we can do the PUT changes as a separate PR (or rather expanding the scope of where PUT is used)

yes , considering it will also be a braking change for current implementations. Those ones currently shouldn't break anything.

src/web_server.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@jeremypoulter jeremypoulter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, does this need a GUI change as well?

@KipK
Copy link
Collaborator Author

KipK commented May 31, 2023

No main tree is ok. 👍

@jeremypoulter jeremypoulter merged commit 384bbc8 into OpenEVSE:master Jun 1, 2023
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.

2 participants