Fix: Load stale data whenever there's any http error #130
Conversation
| @@ -91,6 +91,8 @@ public function getFeatures(): iterable | |||
| if ($response->getStatusCode() === 200) { | |||
| $data = (string) $response->getBody(); | |||
| $this->setLastValidState($data); | |||
| } else { | |||
| throw new HttpResponseException("Invalid status code: '{$response->getStatusCode()}'"); | |||
There was a problem hiding this comment.
I think this is correct. I don't believe PHP is ever going to do the HTTP 304 dance like the other SDKs and anything in the 500 range should be fine to get handled in the catch.
There's two 400 range HTTP status codes I want to clarify:
401 and 429. Do we want to raise error events in the dispatcher when the client is either being denied due to a bad token or rate limited by our infra? I want to say yes, just a sanity check that you've thought about it and agree
There was a problem hiding this comment.
304 should be handled automatically and shouldn't be any issue.
429 seems like the perfect use-case for this, I'm a little conflicted on the 401, but ultimately I think that the SDK should try to fail as little as possible.
There is a time limit for the stale cache, meaning if the 401 prevails it will fail with an exception eventually, while temporary outages somewhere along the line (like some proxy having an outage) which could cause the 401 will be handled gracefully.
There was a problem hiding this comment.
ultimately I think that the SDK should try to fail as little as possible.
Agree with this.
Not critical, but should they body also be supplied in the exception message to help devs debug errors, in case it's available?
There was a problem hiding this comment.
@eblanshey I don't think it would be useful given the exception is caught immediately so nothing ever propagates further. Or did you mean somewhere else?
There was a problem hiding this comment.
429 seems like the perfect use-case for this, I'm a little conflicted on the 401, but ultimately I think that the SDK should try to fail as little as possible.
Definitely don't think this should cause a failure, idly wondering about the value of being able to respond to a 401 with a hook. Don't think it's harmful to have though so yeah, looks good
There was a problem hiding this comment.
There is a time limit for the stale cache, meaning if the 401 prevails it will fail with an exception eventually, while temporary outages somewhere along the line (like some proxy having an outage) which could cause the 401 will be handled gracefully.
Not related to this PR so maybe a conversation for a different time and place but is this what we want? My gut feel is the SDK raising an exception almost anywhere is surprising and not really handleable by the caller. Wouldn't mind unpacking this at some point (honestly I'd like a standard set of behaviours for our SDKs on this, which we don't really have right now)
There was a problem hiding this comment.
I don't think it would be useful given the exception is caught immediately so nothing ever propagates further. Or did you mean somewhere else?
The exception is passed to the FetchingDataFailedEvent, so I'd hook into that event to add a message to our logs. The body would help as much as the header code for context. For example, it could say "Incorrect API key", or "API key expired" or "Account banned", to name a few examples off the top of my head.
But I see this was already merged so I guess we'll leave it at that.
Description
Fixes incorrect behavior when stale fallback data weren't returned when non-200 status code was returned.
Fixes #129
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: