[kfetch] Add support for interceptors #22128

sorenlouv · 2018-08-17T12:08:09Z

Closes #19060
Closes #21982

This PR adds support for http interceptors similar to angular http interceptors.

This makes it possible for plugins to transform request config and response of http requests made with kfetch. This is useful for adding additional headers.

To add an interceptor:

import { addInterceptor } from 'ui/kfetch';

addInterceptor({
  request: async function(config) {
    return config;
  },

  // catch error and return new config object
  requestError: async function(e) {
    const config = {};
    return config;
  },

  response: async function(response) {
    return response;
  },

  // catch error and return new response object
  responseError: async function(e) {
    const response = {};
    return response;
  }
});

Behaviour
Request hooks start from the newest interceptor and end with the oldest, and the response hooks are the opposite. That means that if you add an interceptor before any other code runs your interceptor will always be the last to run before sending the request and the first to run when the response comes back. Each additional interceptor adds an additional "layer" to the wrapper around the request, it's the new first and last interceptor, which makes a lot of sense to me.

if a request hook from one interceptor throws/rejects another interceptor will get the error in their requestError hook and can resolve to allow the request to continue
if a response hook from one interceptor throws/rejects it will call the responseError hook from the next interceptor.
If all interceptors reject in their request* hooks the request is never sent to the server but the responseError hooks are still called so one of the responseError hooks could still cause the request to ultimately resolve.

elasticmachine · 2018-08-17T12:18:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sorenlouv · 2018-08-17T12:23:16Z

@spalger I need some of your typescript foo:

error TS2345: Argument of type '(acc: Promise<never>, interceptor: Interceptor) => Promise<any>' is not assignable to parameter of type '(previousValue: Promise<any>, currentValue: Interceptor, currentIndex: number, array: Interceptor...'.

Types of parameters 'acc' and 'previousValue' are incompatible.
Type 'Promise<any>' is not assignable to type 'Promise<never>'.
Type 'any' is not assignable to type 'never'.
   
      return interceptors.reduce((acc, interceptor) => {
                                   ~~~~~~~~~~~~~~~~~~~~~~~

in https://github.com/elastic/kibana/blob/09983592029ec06b9e6cc90550df4ae137de341c/src/ui/public/kfetch/kfetch.ts#L88-L97

elasticmachine · 2018-08-17T12:47:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-08-17T16:23:16Z

@epixa @sqren Just so I'm clear on this, we intend to update all places where we use $http.interceptors to use kfetch.interceptors in tandem, right?

E.g. we will update xsrf.js like this (and we'll do the same elsewhere):

import $ from 'jquery';
import { set } from 'lodash';
import { interceptors } from 'ui/kfetch';

const xsrfRequestInterceptor = {
  request: function (opts) {
    const { kbnXsrfToken = true } = opts;
    if (kbnXsrfToken) {
      set(opts, ['headers', 'kbn-version'], internals.version);
    }
    return opts;
  }
};

export function initChromeXsrfApi(chrome, internals) {

  chrome.getXsrfToken = function () {
    return internals.version;
  };

  $.ajaxPrefilter(function ({ kbnXsrfToken = true }, originalOptions, jqXHR) {
    if (kbnXsrfToken) {
      jqXHR.setRequestHeader('kbn-version', internals.version);
    }
  });

  chrome.$setupXsrfRequestInterceptor = function ($httpProvider) {
    // Ensure all requests go through the interceptor, regardless of which fetch module is used.
    $httpProvider.interceptors.push(xsrfRequestInterceptor);
    interceptors.push(xsrfRequestInterceptor);
  };
}

cjcenizal

I think we still need to export interceptors from index.ts, right?

export { kfetch, interceptors } from './kfetch';
export { kfetchAbortable } from './kfetch_abortable';

Though personally, I think I'd lean towards just exposing an "add" method, to avoid exposing the data structure:

export { kfetch, addInterceptor } from './kfetch';
export { kfetchAbortable } from './kfetch_abortable';

nreese · 2018-08-17T16:42:22Z

Good timing on this PR. @elastic/kibana-sharing is in the process of moving the reporting top nav menus to EUI and react. I would like to migrate all of the reporting API calls from $http to kfetch. Will this PR add intercepters for checkXPackInfoChange?

I have a WIP PR that is trying to update the xpackInfo into vanilla javascript
#22103

cjcenizal

🔥 This code looks AWESOME. Just had a few suggestions and requests, mostly around testing.

cjcenizal · 2018-08-17T16:37:20Z

src/ui/public/kfetch/fetch_error.ts

+ * under the License.
+ */
+
+export class FetchError extends Error {


I know this is a tiny bit outside of scope but can we rename this KFetchError? This way it will be really obvious which module it has originated from. I think FetchError is just a bit too vague. I mention this because I've introduced a SearchError in the rollups feature branch and seeing this error makes me think that the SearchError name is too vague too. I'll probably rename it CourierSearchError or something.

Doing a rename will require making a change in one other place in the codebase: https://github.com/elastic/kibana/blob/master/src/ui/public/error_auto_create_index/error_auto_create_index.test.js

Sure, will change.

cjcenizal · 2018-08-17T16:43:32Z

src/ui/public/kfetch/kfetch.test.ts

+        addedByInterceptor: true,
+        foo: 'bar',
+      });
+    });


Can we add a test verifying that a Promise is a valid return value?

it('response: should modify response via Promise interceptor', async () => { fetchMock.get(matcherName, new Response(JSON.stringify({ foo: 'bar' }))); interceptors.push({ response: res => { return new Promise((resolve) => { resolve({ ...res, addedByInterceptor: true, }); } }, }); const resp = await kfetch({ pathname: 'my/path' }); expect(resp).toEqual({ addedByInterceptor: true, foo: 'bar', }); });

This also begs the question, what's the expected behavior if an interceptor returns a rejected promise?

cjcenizal · 2018-08-17T16:45:59Z

src/ui/public/kfetch/kfetch.test.ts

+          'kbn-version': 'my-version',
+        },
+      });
+    });


Can we also verify a promise return val here?

it('request: should add headers via Promise interceptor', async () => { fetchMock.get(matcherName, new Response(JSON.stringify({ foo: 'bar' }))); interceptors.push({ request: config => { return new Promise(resolve => { resolve({ ...config, headers: { ...config.headers, addedByInterceptor: true, }, }); }); }, }); await kfetch({ pathname: 'my/path', headers: { myHeader: 'foo' }, }); expect(fetchMock.lastOptions(matcherName)).toEqual({ method: 'GET', credentials: 'same-origin', headers: { addedByInterceptor: true, myHeader: 'foo', 'Content-Type': 'application/json', 'kbn-version': 'my-version', }, }); });

cjcenizal · 2018-08-17T16:48:01Z

src/ui/public/kfetch/kfetch.test.ts

+      });
+
+      expect(resp).rejects.toThrow('my custom error');
+    });


And here.

it('responseError: should throw custom error when a rejected Promise is returned', () => { fetchMock.get(matcherName, { status: 404, }); interceptors.push({ responseError: e => { return new Promise((resolve, reject) => { reject(new Error('my custom error')); }) }, }); const resp = kfetch({ pathname: 'my/path', }); expect(resp).rejects.toThrow('my custom error'); });

cjcenizal · 2018-08-17T16:48:28Z

src/ui/public/kfetch/kfetch.test.ts

+
+      expect(resp).resolves.toBe('resolved valued');
+    });
+  });


Can we add some tests for requestError?

cjcenizal · 2018-08-17T16:53:45Z

src/ui/public/kfetch/default_interceptor.ts

+import { metadata } from 'ui/metadata';
+import { Interceptor } from './kfetch';
+
+export const defaultInterceptor: Interceptor = {


From a "core principles" standpoint, I like the dogfooding and consistency aspect of creating a default interceptor to apply these defaults, but at the same this adds indirection to the code. In the end this indirection hurts more than it helps, in my opinion. I'll leave it up to you but personally I think I would find the code easier to follow if we assigned defaults within kfetch, as we were doing originally.

I agree this is overabstraction. Will change.

cjcenizal · 2018-08-17T17:43:21Z

@nreese I'm assuming that as a second part of this PR, we should update all the places where we consume $httpProvider.interceptors to add the same interceptors for kfetch and then test thoroughly to make sure they all work as expected:

@epixa @sqren Sound good?

sorenlouv · 2018-08-18T09:55:17Z

Though personally, I think I'd lean towards just exposing an "add" method, to avoid exposing the data structure
@cjcenizal

I'm okay with doing that. Reason I exposed access to the data structure is because I don't know the exact use-cases, and then I lean towards more flexibility. Eg. will plugin authors want to delete or re-order interceptors? If not, add method should suffice.

elasticmachine · 2018-08-18T10:26:42Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sorenlouv · 2018-08-18T10:36:31Z

retest

elasticmachine · 2018-08-18T11:16:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-08-18T15:07:34Z

I'm okay with doing that. Reason I exposed access to the data structure is because I don't know the exact use-cases, and then I lean towards more flexibility. Eg. will plugin authors want to delete or re-order interceptors?

If you're on the fence, I think I'd lean towards YAGNI here, and wait for the need to delete/re-order to arise before adding that to the interface. But I don't feel very strongly about this, so I'm totally OK leaving it as-is if you have a strong hunch it will be useful. 😅

sorenlouv · 2018-08-18T19:16:04Z

I'm assuming that as a second part of this PR, we should update all the places where we consume $httpProvider.interceptors to add the same interceptors for kfetch and then test thoroughly to make sure they all work as expected:

xsrf.js
on_session_timeout.js
on_unauthorized_response.js
check_xpack_info_change.js
@cjcenizal

Yeah, that's probably a good idea. Do you think we should keep them as interceptors, or move them into kfetch core? I agree with your earlier comment that interceptors causes additional indirection, and perhaps we should use interceptors as an escape hatch mostly aimed at 3rd parties who can't just modify the core. What do you think?

The interceptor in xsrf.js can probably be removed. kbn-version header is already added in kfetch core.

trevan · 2018-08-18T23:29:04Z

@sqren, my opinion is since those other files are in x-pack and you can run Kibana without them using the OSS version, they should be interceptors so that they don't cause problems for the OSS version.

elasticmachine · 2018-08-18T23:49:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-08-19T00:26:55Z

I agree with @trevan. I think we should also try to avoid making any more changes than we have to, to reduce risk of introducing bugs.

Longer term I think it could make sense to look at what we’re using the interceptors for and consider another mechanism. I’m sure the platform team will have thoughts on that!

elasticmachine · 2018-08-19T00:54:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sorenlouv · 2018-08-19T09:29:12Z

retest

sorenlouv · 2018-08-30T09:33:20Z

retest

elasticmachine · 2018-08-30T11:11:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sorenlouv · 2018-08-30T12:12:55Z

Yay! CI passing.

nreese · 2018-08-30T13:17:31Z

Does anyone know if reporting is using kfetch?

It is all $http at the moment but will be migrated to kfetch in the near future

sorenlouv · 2018-08-30T13:29:16Z

It is all $http at the moment but will be migrated to kfetch in the near future

Odd. The tests have been failing consistently until I made the last commit.

elasticmachine · 2018-08-30T17:56:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-08-31T01:00:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

spalger · 2018-08-31T01:08:03Z

retest

elasticmachine · 2018-08-31T02:48:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

LGTM, would you mind updating the PR description before merging?

cjcenizal · 2018-08-31T03:55:07Z

@spalger What do you think of holding off on merging until #22128 (comment) has been addressed? I hate to put off merging this PR any longer than necessary, but it seems important we ensure that all existing interceptors are integrated into kfetch before merging, because it seems likely that people will migrate from $http to kfetch under this assumption.

sorenlouv · 2018-08-31T08:47:35Z

Personally I think that migrating $http interceptors is a separate task. Merging this in will fix #21982, unblock @trevan and allow anybody to migrate $http interceptors as they like. I don't want to delay that too much.

I'll gladly help migrating the current $http interceptors but since I don't know their domain, I don't want to lead that effort.

Eg. just like the initial PR for kfetch didn't migrate anything. Instead subsequent PRs have started to make use of it.

cjcenizal · 2018-08-31T13:12:41Z

Good point @sqren. And I suppose people today are already assuming that kfetch behaves identically to $http, so blocking this PR from being merged wouldn't solve that problem. I'll create a separate issue to track the interceptor migration.

sorenlouv · 2018-08-31T13:17:04Z

Thanks @cjcenizal! Merging now, then.

sorenlouv · 2018-08-31T13:18:37Z

Btw. @spalger I updated the description so usage is now correct, and added your notes about the behaviour. Ok?

elasticmachine · 2018-08-31T13:52:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-08-31T15:56:36Z

Created #22594 to track interceptor parity between kfetch and $http.

sorenlouv requested review from epixa, cjcenizal and spalger August 17, 2018 12:08

sorenlouv force-pushed the add-kfetch-interceptor branch from 0998359 to d4e17b3 Compare August 17, 2018 12:36

cjcenizal added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v6.5.0 labels Aug 17, 2018

cjcenizal reviewed Aug 17, 2018

View reviewed changes

cjcenizal suggested changes Aug 17, 2018

View reviewed changes

sorenlouv changed the title ~~[kfetch] Add support for interceptors (WIP)~~ [kfetch] Add support for interceptors Aug 18, 2018

sorenlouv force-pushed the add-kfetch-interceptor branch 2 times, most recently from b99783a to 5a81a38 Compare August 18, 2018 23:09

sorenlouv force-pushed the add-kfetch-interceptor branch from 5a81a38 to 343ad74 Compare August 19, 2018 00:13

Add tests for withDefaultOptions

8f84cec

Remove unnecessary noop

bd970fd

sorenlouv force-pushed the add-kfetch-interceptor branch from 868ddcd to bd970fd Compare August 30, 2018 23:19

sorenlouv mentioned this pull request Aug 31, 2018

fetch exception should be rethrowed in kfetch's promise #21982

Closed

spalger approved these changes Aug 31, 2018

View reviewed changes

sorenlouv merged commit 1daa265 into elastic:master Aug 31, 2018

sorenlouv deleted the add-kfetch-interceptor branch August 31, 2018 13:17

zube bot added [zube]: Done and removed [zube]: In Review labels Aug 31, 2018

sorenlouv added a commit to sorenlouv/kibana that referenced this pull request Aug 31, 2018

[kfetch] Add support for interceptors (elastic#22128)

31dce9f

sorenlouv mentioned this pull request Aug 31, 2018

[6.x] [kfetch] Add support for interceptors (#22128) #22584

Merged

cjcenizal mentioned this pull request Aug 31, 2018

Interceptor parity between $http and kfetch #22594

Closed

4 tasks

sorenlouv added a commit that referenced this pull request Sep 3, 2018

[kfetch] Add support for interceptors (#22128) (#22584)

f2e9d2d

[kfetch] Add support for interceptors #22128

[kfetch] Add support for interceptors #22128

Conversation

sorenlouv commented Aug 17, 2018 • edited by zube bot Loading

elasticmachine commented Aug 17, 2018

💔 Build Failed

sorenlouv commented Aug 17, 2018 • edited Loading

elasticmachine commented Aug 17, 2018

💔 Build Failed

cjcenizal commented Aug 17, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

nreese commented Aug 17, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

sorenlouv Aug 18, 2018

Choose a reason for hiding this comment

cjcenizal Aug 17, 2018

Choose a reason for hiding this comment

cjcenizal Aug 17, 2018

Choose a reason for hiding this comment

cjcenizal Aug 17, 2018

Choose a reason for hiding this comment

cjcenizal Aug 17, 2018

Choose a reason for hiding this comment

cjcenizal Aug 17, 2018

Choose a reason for hiding this comment

cjcenizal Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

sorenlouv Aug 18, 2018

Choose a reason for hiding this comment

cjcenizal commented Aug 17, 2018

sorenlouv commented Aug 18, 2018

elasticmachine commented Aug 18, 2018

💔 Build Failed

sorenlouv commented Aug 18, 2018

elasticmachine commented Aug 18, 2018

💔 Build Failed

cjcenizal commented Aug 18, 2018

sorenlouv commented Aug 18, 2018 • edited Loading

trevan commented Aug 18, 2018

elasticmachine commented Aug 18, 2018

💔 Build Failed

cjcenizal commented Aug 19, 2018

elasticmachine commented Aug 19, 2018

💔 Build Failed

sorenlouv commented Aug 19, 2018

sorenlouv commented Aug 30, 2018

elasticmachine commented Aug 30, 2018

💚 Build Succeeded

sorenlouv commented Aug 30, 2018

nreese commented Aug 30, 2018

sorenlouv commented Aug 30, 2018 • edited Loading

elasticmachine commented Aug 30, 2018

💚 Build Succeeded

elasticmachine commented Aug 31, 2018

💔 Build Failed

spalger commented Aug 31, 2018

elasticmachine commented Aug 31, 2018

💚 Build Succeeded

spalger left a comment

Choose a reason for hiding this comment

cjcenizal commented Aug 31, 2018

sorenlouv commented Aug 31, 2018 • edited Loading

cjcenizal commented Aug 31, 2018

sorenlouv commented Aug 31, 2018

sorenlouv commented Aug 31, 2018

elasticmachine commented Aug 31, 2018

💔 Build Failed

cjcenizal commented Aug 31, 2018

sorenlouv commented Aug 17, 2018 •

edited by zube bot

Loading

sorenlouv commented Aug 17, 2018 •

edited

Loading

cjcenizal Aug 17, 2018 •

edited

Loading

cjcenizal Aug 17, 2018 •

edited

Loading

sorenlouv commented Aug 18, 2018 •

edited

Loading

sorenlouv commented Aug 30, 2018 •

edited

Loading

sorenlouv commented Aug 31, 2018 •

edited

Loading