Poor type handling in GuzzleFeatureRequester::getAllFeatures
bfenton-smugmug opened this issue · 7 comments
Describe the bug
The call to array_map in GuzzleFeatureRequester::getAllFeatures can throw a warning if it's passed invalid data. Needs additional response error checking.
To reproduce
Induce the sdk\flags endpoint to send either NULL or bad JSON
Expected behavior
The code handles an error condition without emitting the warning.
Logs
E_WARNING: array_map(): Expected parameter 2 to be an array, null given code=2 file="vendor/launchdarkly/server-sdk/src/LaunchDarkly/Impl/Integrations/GuzzleFeatureRequester.php" line=115
SDK version
3.7.1
Language version, developer tools
PHP 7.4
OS/platform
Ubuntu 18.04
In terms of the expected behavior, could you say more about what you mean by "the code handles an error condition without emitting the warning"? That is, besides not emitting a warning, what would you consider to be a desirable behavior in this case?
I ask because the behavior of the PHP SDK has never been very well defined compared to other SDKs in terms of what should happen in case of a bad failure of the overall flag-requesting infrastructure. This is more of an issue in PHP than in other server-side SDKs because it may actually be doing network requests on demand for an individual flag evaluation; in other SDKs, the SDK either has or has not received the flag data at that point so the worst that can happen in an evaluation is that it says "flag not found". At present, the PHP can throw an exception if there is a network error— which I'm not sure is desirable either; maybe it should just be returning the fallback/default variation value instead, as it would for "flag not found". However, that would be a larger change in behavior and I don't know if existing users might be expecting to see exceptions.
I'd probably expect it to go down the "failed request" route, and treat it like a BadResponseException was thrown.
Could you clarify what behavior you are seeing right now besides the logging of the error? That is, what value is your code receiving? (Speaking of the code: I assume you're evaluating a flag, but your steps to reproduce don't include an actual action other than the setup of the invalid data.)
I'll have to add some more logging to find that out. We just noticed that every few minutes we get a pile of these (1-3,000) within a few seconds. I'll ask ops and see if it corresponds to, ex. a network hiccup on the ld-proxy or something.
Thanks. I wouldn't have expected a network issue to show up in this way— that is, it seems more likely that those would cause a straightforward exception or an HTTP error status, rather than a 200 response with malformed JSON— which I think is why we haven't heard of this scenario before.
Closing this due to inactivity, and because it's unclear what a more desirable behavior would be. The proposal to "treat it like a BadResponseException was thrown" would arguably make the SDK less usable in the case of a network glitch, since it would be throwing an exception rather than simply logging a warning.