Allow non-JSON responses in miqFetch (add options.skipJsonParsing)

[mturley]

Needed for https://bugzilla.redhat.com/show_bug.cgi?id=1788730

In RedHatCloudForms/cfme-migration_analytics#37, we are adding a new API endpoint which delivers a binary response (content-disposition: attachment) intended for the UI to make available as a file download. Rationale for the decision to handle the file download via the API instead of a UI worker can be found in the relevant discussions at RedHatCloudForms/cfme-migration_analytics#19 and RedHatCloudForms/cfme-migration_analytics#33.

Currently, the API methods provided here (which we must use to handle authentication) assume API responses will always be JSON, and call response.json() as part of processing the fetch() response. This causes an error when trying to make a call to the new endpoint, since it is returning binary gzip data.

This PR adds a new option to miqFetch called skipJsonParsing which simply skips that .json() call, instead resolving the promise with the raw response object directly. This gives the calling function in the UI access to response.blob() and response.headers(), which are necessary for reading the binary data and attachment filename.

To test these changes, you can run the following code in a browser's JavaScript console:

API.get('/api', { skipJsonParsing: true }).then(response => console.log({ response }));

Links

BZ for backport to ivanchuk: https://bugzilla.redhat.com/show_bug.cgi?id=1788730
JIRA task for feature: https://issues.redhat.com/browse/MIGENG-241
Issue for feature: RedHatCloudForms/cfme-migration_analytics#19
Dependent PR: RedHatCloudForms/cfme-migration_analytics#37
Initial closed PR with implementation context: RedHatCloudForms/cfme-migration_analytics#33

[mturley]

@miq-bot add_label ivanchuk/yes

[mturley]

@miq-bot add_label enhancement

[himdel]

Looks good, except.. is skipJsonParsing an actual fetch option, or just a new miqFetch option?

If the latter, we should make sure it doesn't end up in fetchOpts .. there's a bunch of deletes in processOptions, can you add it there please?

[himdel]

And as the code is, the option will get ignored for non-200 responses.

If we want to support those as well, maybe something like..

  if (response.status === 204) {
    // No content
    return Promise.resolve(null);
  }

  const maybeJson = options.skipJsonParsing ? (r) => r.json() : (r) => r;    // <--

  if (response.status >= 300) {
    // Not 1** or 2**
    // clone() because otherwise if json() fails, you can't call text()
    return maybeJson(response.clone())    // <--
      .catch(tryHtmlError(response))
      .then(rejectWithData(response));
  }

  return maybeJson(response);    // <--

?

(and possibly a fix for error modal not being able to meaningfully display the response, but not sure that's really possible for binary responses anyway :))

[himdel]

Oh, one more idea (just an idea really :)).. do we need an option?

Because, theoretically the data is already available in the responses content-type header, right?

No harm in being explicit though.

[mturley]

Looks good, except.. is skipJsonParsing an actual fetch option, or just a new miqFetch option?

The latter.

If the latter, we should make sure it doesn't end up in fetchOpts .. there's a bunch of deletes in processOptions, can you add it there please?

I already did 😄

And as the code is, the option will get ignored for non-200 responses.

That's an interesting point, but it seems like (at least with the 404 I tried: /api/red_hat_migration_analytics_payload?task_id=12345) non-200 responses will return JSON to describe the error, even if the endpoint returns non-JSON content on success.

Oh, one more idea (just an idea really :)).. do we need an option?
Because, theoretically the data is already available in the responses content-type header, right?

That's true... are you suggesting I just resolve with the raw response like this if response.headers('content-disposition') includes 'attachment' (or content-type is application/binary)? I was considering having it automatically call response.blob() in that case, so the promise always resolved to a response body (instead of the entire Response meta-object). But since my UI code also needs to be able to access that header to get the filename from it, I thought it would be nice to have an explicit way to skip any kind of body parsing and make it clear that the promise resolves to a Response. Maybe the option would be better named rawResponse or something.

[mturley]

To be clear, I think it would be ideal if the miqFetch promise always returns JSON when it is rejected, and the option only modifies the expected response type in the successful resolve case. That way I could reuse existing error handling code in the UI which expects JSON responses.

[miq-bot]

Checked commit mturley@d8428c0 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍪

[himdel]

Ah, perfect :)
OK, in that case this seems to match your use case, LGTM 👍

[mturley]

Thanks @himdel !

[simaishi]

Ivanchuk backport details:

$ git log -1
commit 44b61f3c0cbf3c827cb57db6fc290758fd97aa7e
Author: Martin Hradil <[email protected]>
Date:   Mon Feb 3 14:54:03 2020 +0100

    Merge pull request #6645 from mturley/api-fetch-allow-non-json-responses

    Allow non-JSON responses in miqFetch (add `options.skipJsonParsing`)

    (cherry picked from commit a10fb3702a2c8fe066390e9b3d5bec2a3889e2d7)

    https://bugzilla.redhat.com/show_bug.cgi?id=1805818