Bug/update follower index validation cr #24

sebelga · 2020-04-16T13:51:05Z

This PR adds the test coverage for the bug found on follower indices not being updated.

The minimum test that was required to avoid the regression is the API integration test.

I then gave it some thoughts on how we could improve the confidence in our test coverage and build a convention for future apps. We need to make sure that both the API is working as expected, but also that the component is calling the correct API with the correct payload. For that, we need a single source of truth to declare the API endpoints of our apps.

The server code, the client code and the API integration code ALL read from that single source of truth. So we are sure everything is connected.

API integration test

Verifies that the endpoint exists
Verifies that it accepts the expected payload
Verifies that it throws a 400 Bad request with a wrong payload

Component integration tests

Make sure the form values are sent in the payload
Make sure the component calls the correct endpoint is called with the correct method (the same path the API integration test is calling)

Server route handler & Client API handlers

They both read from the source of truth to declare their route or HTTP request.

cjcenizal · 2020-04-16T22:39:23Z

I added your API integration test via fdc36a5 (thanks!). Can you approve that PR so I can merge the fix without blocking on this proposal?

jloleysens · 2020-04-17T09:06:39Z

x-pack/legacy/plugins/cross_cluster_replication/server/np_ready/routes/api/follower_index.ts

@@ -161,21 +185,10 @@ export const registerFollowerIndexRoutes = ({ router, __LEGACY }: RouteDependenc
   */
  router.put(
    {
-      path: `${API_BASE_PATH}/follower_indices/{id}`,
+      path: getEndpoint('followerIndex', 'edit').path,


I'm not sure I see the benefit of the added getEndpoint machinery.

It seems like another thing we need to build out (replacePlaceholders 👀) and share properly across apps and without a clear benefit over statically declaring these values (at least the path pattern, not convinced about the method) and importing them from a common folder.

If we issue a request (in tests or in app code) against a server on a path that does not exist we should be met with a 404 and so the server is already the source of truth, so this does not seem like a major current pain point.

My understanding of the bug we had here was that we required a body but the logic inside the route handler accepted a body with all undefined values - combined with NP route validation the oversight snuck in because body without route validation is {} (i.e., an acceptable value!).

The best way, in my view, to cover this specific case would be the addition of an API integration test that issues a request against elastic with a value set and checking the response (like what you have added here x-pack/test/api_integration/apis/management/cross_cluster_replication/follower_indices.js)

The major take away for me, from this bug, besides the added tests is that we need to be more thorough in our route validations and explicitly testing sending unexpected values to routes.

sebelga · 2020-04-20T09:24:04Z

It seems like another thing we need to build out (replacePlaceholders 👀) and share properly across apps and without a clear benefit over statically declaring these values (at least the path pattern, not convinced about the method) and importing them from a common folder.

Can you explain the "share properly"? not sure I understand, this is a pure, static function from a lib. Let's leave out, for now, the details of naming or where it will live.

not convinced about the method

Can you explain?

If we issue a request (in tests or in app code) against a server on a path that does not exist we should be met with a 404 and so the server is already the source of truth, so this does not seem like a major current pain point.

I think I did not do a good job of presenting the problem we are trying to solve. Let me rephrase:

Currently, if we make any change to the route handler definition (the endpoint, method or more importantly the contract). The CI will fail for the API integration tests. We then fix the API integration test, we think we are OK, but we have now a broken UI.

How do we make sure that the requests we mock in our component integration tests are the correct ones and avoid having to manually update the API integration tests, the component integration tests, and the UI consuming code when touching the server routes? By having a single place to declare the endpoints.

If we have a common place to declare our API routes, then making any change on the route will break both our API integration tests AND the component integration tests.

This proposal is a first step to solve this.

A second step that I have in mind is to save in a folder the payloads sent by the UI in the component integration tests (the same way jest saves snapshots). And use those payloads in our API integration tests.

[EDIT]: See my answer here for a concrete example elastic#62890 (comment)

jloleysens · 2020-04-20T10:09:37Z

Can you explain the "share properly"?

I think some amount of the work here will be designing an API that expresses the intent of this mechanism. Even once that is done, I still query the value of the mechanism vs the indirection it introduces. Is this, historically, something that developers have struggled with and has lead to bugs? When updating a server route the dev workflow/pattern is typically:

Update the server route
Update the UI (check that we handle it) - ~~in most cases~~ it would be pretty careless to skip this IMO
Update the tests (which will break at any rate)

Which tends to cover our bases.

Even if we deviate from the pattern above, I would suggest that convention can win out over configuration. If we statically declare our server payload types and share that between server and client we will leverage TS to do the checking for us -- instead of needing to build a mechanism.

Can you explain? (re method)

I would argue this is detrimental to DX:

  // It feels like I am looking at docs 😅
  return supertest[method](path)

A second step that I have in mind is to save in a folder the payloads sent by the UI in the component integration tests (the same way jest saves snapshots). And use those payloads in our API integration tests.

👆🏻sounds like an excellent idea that is fully orthogonal to getEndpoint.

sebelga · 2020-04-20T12:15:17Z

I think some amount of the work here will be designing an API that expresses the intent of this mechanism.

So this is what I am talking about to declare an app endpoints, it took me 3 minutes to gather those

// Reusable lib accross our apps
import { EndpointDefinition, createEndPointsGetter } from './my-lib';

import { API_BASE_PATH } from './constants';

// Follower indices
const followerIndexEndpoints = {
  list: {
    method: 'get',
    path: `${API_BASE_PATH}/follower_indices`,
  } as EndpointDefinition,
  get: {
    method: 'get',
    path: `${API_BASE_PATH}/follower_indices/{id}`,
  } as EndpointDefinition,
  create: {
    method: 'post',
    path: `${API_BASE_PATH}/follower_indices`,
  } as EndpointDefinition,
  edit: {
    method: 'put',
    path: `${API_BASE_PATH}/follower_indices/{id}`,
  } as EndpointDefinition,
  delete: {
    method: 'delete',
    path: `${API_BASE_PATH}/follower_indices/{id}`,
  } as EndpointDefinition,
};

const autoFollowPatternEndpoints = {
  list: {
    method: 'get',
    path: `${API_BASE_PATH}/auto-follow-patterns`,
  } as EndpointDefinition,
  get: {
    method: 'get',
    path: `${API_BASE_PATH}/auto-follow-patterns/{id}`,
  } as EndpointDefinition,
  create: {
    method: 'post',
    path: `${API_BASE_PATH}/auto-follow-patterns`,
  } as EndpointDefinition,
  edit: {
    method: 'put',
    path: `${API_BASE_PATH}/auto-follow-patterns/{id}`,
  } as EndpointDefinition,
  delete: {
    method: 'delete',
    path: `${API_BASE_PATH}/auto-follow-patterns/{id}`,
  } as EndpointDefinition,
  pause: {
    method: 'post',
    path: `${API_BASE_PATH}/auto-follow-patterns/{id}/pause`,
  } as EndpointDefinition,
  resume: {
    method: 'post',
    path: `${API_BASE_PATH}/auto-follow-patterns/{id}/resume`,
  } as EndpointDefinition,
};

// Redux like reducer composition
const ccrRoutesEndpoints = {
  followerIndex: followerIndexEndpoints,
  autoFollowPattern: autoFollowPatternEndpoints,
};

export const endPoints = {
  get: createEndPointsGetter(ccrRoutesEndpoints),
};

And this is what I get out of it, everywhere I consume them thanks to TS

I am surprised you don't see the benefit of having a single source of truth TBH.

Current solution:

We declared in 4 different places the same API

Server-side

API integration

Client-side

Component integration

And like I mentioned, it is not only about declaring and maintaining 4 different places, it is about automating the whole testing process.

cjcenizal · 2020-05-29T21:17:52Z

@sebelga @jloleysens FYI, here's a real-life example of how Ingest Manager has implemented a single source of truth for their API routes:

Route patterns are defined as constants here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/common/constants/routes.ts
They're then accessed and interpolated with variable route params through getter methods here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/common/services/routes.ts

jloleysens · 2020-05-30T09:37:57Z

I am glad you shard this CJ! :) I have been giving this some more thought since sebs proposal. Primary difference for me with what he was proposing and what ingest manager are doing are quite small. Sebs implementation of the replace logic was just more generic and he used namespaces; but it comes down to a very similar mechanism. I just didn’t like the idea of more indirection except for where it has maximal impact. It’s still something I am hesitant about tbh. When I see this kind of thing I instantly think about edge cases we are not considering like query params and correctly parsing for ? and &. I don’t want to just consider green fields when it comes to abstractions - there is always a cost too and I want to understand what the cost is before I can make a proper decision. (Not that we use query params in our apis much it’s just that they are part of URLs, also we need to properly encode and decode user input) However, having thought about it a bit more I have come to realize that the added indirection is really very small and the benefit is decent enough so i have changed my position since my initial response. I say let’s try it in ingest pipelines since it is really new and is probably very low effort to do. Cheers, JL

…

On Fri, 29 May 2020 at 23:18, CJ Cenizal ***@***.***> wrote: @sebelga <https://github.com/sebelga> @jloleysens <https://github.com/jloleysens> FYI, here's a real-life example of how Ingest Manager has implemented a single source of truth for their API routes: - Route patterns are defined as constants here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/common/constants/routes.ts - They're then accessed and interpolated with variable route params through getter methods here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/common/services/routes.ts — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB6G67C2H2YR26VE53QQTL3RUARA3ANCNFSM4MJWDGNA> .

cjcenizal · 2020-05-31T13:49:13Z

@jloleysens Thank you for sharing your updated perspective and thoughtful line of reasoning. I'm generally aligned with the approach you describe. I would summarize it as: "Code comes with a cost (greater surface area for introducing bugs, required maintenance, cognitive overhead), and the benefit conveyed by the code must outweigh the cost of adding it."

++ To your ultimate decision to try out a new idea, too.

To you and @sebelga, my 2 cents are that in terms of implementation I would prefer to work with an interface that shifted the responsibility of configuration from passing the right arguments to calling the right method. For example, this:

const { path, method } = getEndpoint('followerIndex', 'edit', { id: encodeURIComponent(id) });

Would become one of these:

const { path, method } = getEditFollowerIndexRoute({ id: encodeURIComponent(id) });

const { path, method } = followerIndexRoute.getEdit({ id: encodeURIComponent(id) });

To me, this interface says what it does: "get the route for editing a follower index". I can understand what it does by reading it, with little thought. In contrast, when I read getEndpoint('followerIndex', 'edit'), I need to build up my understanding by parsing each part of this call and then putting them back together in the right order:

getEndpoint -> "We're retrieving endpoint information. For which concern?"
'followerIndex' -> "For follower indices. We're retrieving endpoint information for follower indices. What are we doing with them?"
'edit' -> "We're editing a follower index. We're retrieving endpoint information for editing a follower index."

sebelga · 2020-06-01T09:58:08Z

I think there are 2 things to consider on API design.

The time it takes to understand it at first sight
The time it takes to write it over and over again and the complexity to read it.

I put more emphasis at the second. I need to train my brain only a few seconds to read this

const { path, method } = getEndpoint('followerIndex', 'edit', { id: encodeURIComponent(id) });

And once I did, I know exactly where to look to read it ('edit' -> 'followerIndex'), so that's where my eyes are going immediately.

With that, I can now declare all my routes, in all our apps with the same interface. This is a huge win as I don't spend any brain power to parse it. Nothing is specific to an app, this could even come from a JSON

const followerIndexEndpoints = {
  list: {
    method: 'get',
    path: `${API_BASE_PATH}/follower_indices`,
  } as EndpointDefinition,
  get: {
    method: 'get',
    path: `${API_BASE_PATH}/follower_indices/{id}`,
  } as EndpointDefinition,
  create: {
    method: 'post',
    path: `${API_BASE_PATH}/follower_indices`,
  } as EndpointDefinition,
  edit: {
    method: 'put',
    path: `${API_BASE_PATH}/follower_indices/{id}`,
  } as EndpointDefinition,
  delete: {
    method: 'delete',
    path: `${API_BASE_PATH}/follower_indices/{id}`,
  } as EndpointDefinition,
};

Aside, the endpoints discoverability is very pleasant DX

sebelga · 2020-06-01T12:24:42Z

What I am suggesting is to have a system in place to:

not have to manually write 1 handler at the time for each route with its logic to replace the part of the route that is dynamic

If you look at the code needed to write an app endpoint, you see that there are only objects and one handler

export const endPoints = {
  get: createEndPointsGetter(ccrRoutesEndpoints),
};

IMO you can't get cleaner than that.

be predictable. It is always written the same way

If you look at those 2 lists

import {
  getEditFollowerIndexRoute,
  getCreateAutoFollowPatternRoute,
  getListFollowerIndexRoute,
  getGetAutoFollowerPatternIndexRoute,
  getDeleteFollowerIndexRoute,
  getUpdateAutoFollowPatternRoute,
} from './my-routes';
const { path, method } = getEditFollowerIndexRoute();
const { path, method } = getCreateAutoFollowPatternRoute();
const { path, method } = getListFollowerIndexRoute();
const { path, method } = getGetAutoFollowerPatternIndexRoute();
const { path, method } = getDeleteFollowerIndexRoute();
const { path, method } = getUpdateFollowernRoute();

import { endPoints } from './my-routes';
const { path, method } = endPoints.get('followerIndex', 'edit');
const { path, method } = endPoints.get('autofollowPattern', 'create');
const { path, method } = endPoints.get('followerIndex', 'list');
const { path, method } = endPoints.get('autofollowPattern', 'get');
const { path, method } = endPoints.get('followerIndex', 'delete');
const { path, method } = endPoints.get('followerIndex', 'edit');

Which one do you find quicker the "Create Autofollow pattern" route? By adding space around the important term, I find the second list much faster to parse.

jloleysens · 2020-06-02T08:26:18Z

I tend to stand more in alignment with @sebelga 's implementation.

It contains the route + method which nicely encapsulates endpoint concerns. Additionally, changes to to the replacement mechanism only happen in one place in Seb's implementation where they happen in every function of the ingest implementation - so I think it will scale a lot better.

I do think we need to encodeURIComponent for data passed in and document that! Or we should make it clear that it is the responsibility of the caller to do so.

Using this in TS will also have good DX, I agree :)

cjcenizal · 2020-06-02T14:11:45Z

Y'all will both be working with this code a lot more than I will, so it's more important that you two are comfortable with its design than I am. 😄

sebelga · 2020-06-03T07:19:15Z

I do think we need to encodeURIComponent for data passed in and document that! Or we should make it clear that it is the responsibility of the caller to do so.

Good point. Should we automatically do it for the consumer to create URL safe paths? Should it be optional through an option prop?

endPoints.get('followerIndex', 'update', { id: 'someTextToEncode' }, { encode: true });

I wonder if we return all paths with encodeURI(path) if it would be enough and don't need the consumer to do it. I guess we'd need to try in our real-life examples.

If you look at ccr, we had to both encodeURI + encodeURIComponent. (https://github.com/elastic/kibana/blob/master/x-pack/plugins/cross_cluster_replication/public/app/services/routing.js#L83) so we might do that by default for the consumer (encode: true is the default), and let him opt out if he needs to. WDYT?

jloleysens · 2020-06-03T08:28:28Z

default for the consumer (encode: true is the default), and let him opt out if he needs to. WDYT?

This makes sense to me 👍

sebelga · 2020-06-03T14:42:23Z

A note on real-life use case of the problem faced by not having a central place to declare routes:

Alison asked me to update "templates" to "index-templates" in our API route (elastic#67282 (review)). Seems like a simple task? Yet I had to locate and modify 7 files (elastic@d0ef2fb) and of course, I forgot some, and the CI failed (https://github.com/elastic/kibana/pull/67282/checks?check_run_id=734757207)

We'll all agree that it is not the best use of our time and CI power 😊

sebelga added 3 commits April 16, 2020 10:14

Move body schema out of route declaration

4b1fb3d

Add API integration test to make sure follower index is updated

7c411f8

Add test coverage with single source of truth for api endpoint

3e273e9

jloleysens reviewed Apr 17, 2020

View reviewed changes

sebelga mentioned this pull request Apr 20, 2020

Move CCR out of legacy elastic/kibana#62890

Merged

sebelga added 2 commits April 20, 2020 14:23

Create reusable lib to create endoint getter

59337a6

Fix

06143cd

cjcenizal mentioned this pull request Apr 24, 2020

Add CCR client integration test that asserts that the UI consumes the expected API when editing a follower index elastic/kibana#64440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/update follower index validation cr #24

Bug/update follower index validation cr #24

sebelga commented Apr 16, 2020 •

edited

Loading

cjcenizal commented Apr 16, 2020

jloleysens Apr 17, 2020

sebelga commented Apr 20, 2020 •

edited

Loading

jloleysens commented Apr 20, 2020 •

edited

Loading

sebelga commented Apr 20, 2020

cjcenizal commented May 29, 2020

jloleysens commented May 30, 2020 via email

cjcenizal commented May 31, 2020 •

edited

Loading

sebelga commented Jun 1, 2020

sebelga commented Jun 1, 2020

jloleysens commented Jun 2, 2020

cjcenizal commented Jun 2, 2020

sebelga commented Jun 3, 2020

jloleysens commented Jun 3, 2020

sebelga commented Jun 3, 2020

Bug/update follower index validation cr #24

Are you sure you want to change the base?

Bug/update follower index validation cr #24

Conversation

sebelga commented Apr 16, 2020 • edited Loading

API integration test

Component integration tests

Server route handler & Client API handlers

cjcenizal commented Apr 16, 2020

jloleysens Apr 17, 2020

Choose a reason for hiding this comment

sebelga commented Apr 20, 2020 • edited Loading

jloleysens commented Apr 20, 2020 • edited Loading

sebelga commented Apr 20, 2020

Current solution:

cjcenizal commented May 29, 2020

jloleysens commented May 30, 2020 via email

cjcenizal commented May 31, 2020 • edited Loading

sebelga commented Jun 1, 2020

sebelga commented Jun 1, 2020

jloleysens commented Jun 2, 2020

cjcenizal commented Jun 2, 2020

sebelga commented Jun 3, 2020

jloleysens commented Jun 3, 2020

sebelga commented Jun 3, 2020

sebelga commented Apr 16, 2020 •

edited

Loading

sebelga commented Apr 20, 2020 •

edited

Loading

jloleysens commented Apr 20, 2020 •

edited

Loading

cjcenizal commented May 31, 2020 •

edited

Loading