refactoring server routes in index management to use common code

[bmcconaghy]

This PR moves boilerplate code to x-pack/server/lib and refactors index management to use this new common code. Please look over the pattern and make sure this is a good way to do it. If we are happy with this, we can start refactoring all the management apps to use this new common code.

[elasticmachine]

Pinging @elastic/es-ui

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[sebelga]

Like we talked briefly on Slack, I think that we could simplify one step further the approach.
I am concerned by 2 things:

• Very deep traversal to access the registerRoute in many places
  (e.g.: import { registerRoute } from '../../../../../../server/lib/register_route';)
• A lot of duplicate "pluginId" and "server" passed around that we could abstract for the api route consumer.

What I would suggest is that the registerRoute become a registerRouter and be something like (not tested)

export const registerRouter = (server, pluginId, apiBasePath = '') => {
  const isEsError = isEsErrorFactory(server);
  const licensePreRouting = licensePreRoutingFactory(server, pluginId);

  const requestHandler = (handler) => async (request, h) => {
    const callWithRequest = callWithRequestFactory(server, request);
    try {
      return handler(request, callWithRequest, h);
    } catch (err) {
      if (isEsError(err)) {
        throw wrapEsError(err);
      }

      throw wrapUnknownError(err);
    }
  };

  return (['get', 'post', 'put', 'delete', 'patch'].reduce((router, method) => {
    router[method] = (path, handler) => {
      server.route({
        path: apiBasePath + path,
        method,
        handler: requestHandler(handler),
        config: { pre: [ licensePreRouting ] }
      });
    };
    return router;
  }, {}));
};

Then we only have 1 place to import that factory (in the index_management/index.js)

...
import { registerRouter } from '../../server/lib/register_route'; // only 2 level traversal

export function indexManagement(kibana)  {
  return new kibana.Plugin({
    ...
    init: function (server) {
      server.expose('addIndexManagementDataEnricher', addIndexManagementDataEnricher);
      const router = registerRouter(server, PLUGIN.ID, '/api/index_management');
      registerLicenseChecker(router);
      registerIndicesRoutes(router);
      registerSettingsRoutes(router);
      registerStatsRoute(router);
      registerMappingRoute(router);
    }
  });
}

Finally, the consumer would simply receive the router (no more import) and call its methods

// register_freeze_route.js

const handler = async (request, callWithRequest, h) => {
  const { indices = [] } = request.payload;
  const params = {
    path: `/${encodeURIComponent(indices.join(','))}/_freeze`,
    method: 'POST',
  };

  await callWithRequest('transport.request', params);
  return h.response();
};

export function registerFreezeRoute(router) {
  router.post('/indices/freeze', handler);
}

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[cjcenizal]

This is awesome. I love all the code reduction. I do find Seb's suggestion easier to follow. I also had a suggestion for making error-handling more transparent to the consumer.

[cjcenizal]

I'm concerned about the inflexibility of hardcoding our error-handling this way. There are edge cases to the way we handle our errors, e.g. rollups, so I'd find value in a solution that makes it easier to handle the default case, but still allows for custom logic. For example, something like this:

const handler = async ({ request, callWithRequest, h, handleDefaultError }) => {
  try {
    const { indices = [] } = request.payload;
    const params = {
      path: `/${encodeURIComponent(indices.join(','))}/_freeze`,
      method: 'POST',
    };

    await callWithRequest('transport.request', params);
    return h.response();
  } catch (err) {
    throw handleDefaultError(err);
  }  
};

[cjcenizal]

I just realized that the existing proposal supports this already. The consumer just needs to put a try/catch in the handler to handle the edge-case, and re-throw the error if the edge-case condition isn't met. So you can ignore my comment above. 😄

[bmcconaghy]

@cjcenizal @seb pushed a refactor per the comments.

[sebelga]

LGTM! Awesome

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request