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

SDK is broken: trying to get KV value throws exception #152

Closed
martinbean opened this issue Dec 8, 2020 · 2 comments · Fixed by #164
Closed

SDK is broken: trying to get KV value throws exception #152

martinbean opened this issue Dec 8, 2020 · 2 comments · Fixed by #164

Comments

@martinbean
Copy link

martinbean commented Dec 8, 2020

When trying to retrieve the value of a key–value pair using the Cloudflare\API\Endpoints\WorkersKV::getAllKeysAndValuesForNamespace method, the SDK throws a JSONException because the response of that call (https://api.cloudflare.com/#workers-kv-namespace-read-key-value-pair) is just plain text, and not JSON as the SDK is naïvely assuming all responses are. However, the API does return application/json as the Content-Type.

The API either needs to return an appropriate content type for responses, or the SDK needs to accommodate plain text responses like this.

@dumityty
Copy link

Just ran into the same issue when I start using the code in PR #139 to use the WorkerKV endpoints.
It looks like the issue is with the Guzzle adapter in the checkError method (\Cloudflare\API\Adapter\Guzzle::checkError) where the response is always assumed to be JSON, though looks like very few endpoints can return plain text values now:

private function checkError(ResponseInterface $response)
    {
        $json = json_decode($response->getBody());
...

@deuill
Copy link
Contributor

deuill commented Mar 8, 2021

I've responded on #162 but responding here as well for clarity -- this issue needs a bit of investigation as the responses for the Workers KV endpoint (e.g. "Some Value") are indeed valid JSON and should be decoded as such via json_decode, e.g.:

$ php -r 'var_dump(json_decode(\'"Some Value"\'));'
string(10) "Some Value"

There's probably something else at play here, I'll give this a look-see and come back with more info.

--- SNIP ---

Ignore the above, the aforementioned endpoint returns application/octet-stream responses, and there's multiple places where these aren't expected, including the checkError function above. I'm opening a PR for fixing that part, but additional fixes will need to be made to #139, which we're also looking to push through.

deuill added a commit that referenced this issue Mar 10, 2021
This commit represents a partial overhaul of error handling for requests
made against the v4 Cloudflare API, with an aim of unifying disparate
kinds of exceptions under a single `ResponseException` type, and the
covering of additional cases where errors were unhandled. Specifically:

  - The `Guzzle::request()` function will now catch Guzzle exceptions
    normally thrown in cases of client and server errors (4xx and 5xx)
    response codes, and convert these to `ResponseException` types
    before re-throwing. These types of errors were previously not caught
    and were instead returned verbatim, expecting downstream clients to
    be aware of internal details of how these functions operate.

  - Conversely, we no longer assume that all responses are JSON-encoded,
    and no longer try to derive errors from non-4xx or 5xx responses.
    All public endpoints under the v4 API are expected to be
    well-behaved in that regard, and never return an error response
    where none is indicated in the HTTP code.

Code has been moved around and test-cases added in support of these
changes. In most cases, these changes won't break any existing
expectations and won't require any changes to downstream code, but users
of the Cloudflare SDK should ensure that they are indeed set up for
catching `ResponseException` instances thrown during requests, and
should not expect to see Guzzle exceptions directly (though these are
still available in calls to `ResponseException::getPrevious()`).

Fixes: #152
jacobbednarz pushed a commit that referenced this issue May 28, 2021
This commit represents a partial overhaul of error handling for requests
made against the v4 Cloudflare API, with an aim of unifying disparate
kinds of exceptions under a single `ResponseException` type, and the
covering of additional cases where errors were unhandled. Specifically:

  - The `Guzzle::request()` function will now catch Guzzle exceptions
    normally thrown in cases of client and server errors (4xx and 5xx)
    response codes, and convert these to `ResponseException` types
    before re-throwing. These types of errors were previously not caught
    and were instead returned verbatim, expecting downstream clients to
    be aware of internal details of how these functions operate.

  - Conversely, we no longer assume that all responses are JSON-encoded,
    and no longer try to derive errors from non-4xx or 5xx responses.
    All public endpoints under the v4 API are expected to be
    well-behaved in that regard, and never return an error response
    where none is indicated in the HTTP code.

Code has been moved around and test-cases added in support of these
changes. In most cases, these changes won't break any existing
expectations and won't require any changes to downstream code, but users
of the Cloudflare SDK should ensure that they are indeed set up for
catching `ResponseException` instances thrown during requests, and
should not expect to see Guzzle exceptions directly (though these are
still available in calls to `ResponseException::getPrevious()`).

Fixes: #152
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 a pull request may close this issue.

3 participants