Pirate-Weather / pirateweather

Code and documentation for the Pirate Weather API
Apache License 2.0
670 stars 30 forks source link

400 Errors return a 200 status code #317

Closed cloneofghosts closed 1 month ago

cloneofghosts commented 2 months ago

Describe the bug

As noted here whenever the API returns a 400 error the return status code is 200 instead of 400.

Expected behavior

The return status code should be 400

Actual behavior

The return status code is 200

API Endpoint

Production

Location

Ottawa, Ontario

Other details

No response

Troubleshooting steps

cloneofghosts commented 1 month ago

@alexander0042 I saw that #318 was fixed in V2.3 and returns a 400 status code but it looks like this was missed. I checked today and the 400 error I noted in the linked issue is still returning a 200 return status even though the body is showing a 400 error

image

I also noticed that the error body isn't 100% consistent between different errors. Only the error in my screenshot shows the statusCode in the message body and all other ones don't.

I know some you probably might not have control over but would be nice if the others could show the statusCode in the message body.

alexander0042 commented 1 month ago

Nope, this is something that I missed! The FastAPI framework lets me define specific errors, so this is just one that slipped through. I'll correct it stat

cloneofghosts commented 1 month ago

@alexander0042 Been a few days since your comment and the issue still exists. I know this isn't something earth shattering so probably not the highest priority but would be nice if it was fixed. (Ignore the priority label as stuff that is waiting for you to reply has a higher priority than normal as for some items it's been a while)

alexander0042 commented 1 month ago

Thanks for the ping! It's just the one case (invalid location specification) that got missed, which is why you're seeing a mix of different responses. That's actually good news, since it means that what I did (mostly) worked, so just need to copy the line of code over,

The current priority is fixing a weird glitch where the service is going down (and then recovering) for 2-3 minutes every so often. It's not a big deal, but definitely an issue with 2.3, and pretty clear on the monitoring page. I have an idea for how to fix it (something to do with allowing timemachine requests on the regular api endpoint, which I wanted to do, but creates more issues than it's worth), so just testing that before pushing it out

cloneofghosts commented 1 month ago

So the fix for the intermittent downtime will also contain the fix for this issue once pushed live?