[App Search] Schema: Set up server routes, grand foray into shared/connected Kea logic #99548

cee-chen · 2021-05-06T20:49:47Z

Summary

Whoof, a lot going on here! There (currently) isn't any changed UI/UX to test this with, but if you go to the schema pages of an engine or meta engine you can see the XHR call getting made on page load and inspect the API response.

This PR is also a semi-experimental foray into sharing some base values/actions between 2 separate-but-similar logic files. Indexed vs Meta Engines is generally a good candidate for this and there's some other meta engine logic where I might attempt something similar later if this is well-received by y'all.

Strongly recommend following along by commit!

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

- values & actions shared between both default source engine & meta engine pages

- significantly different actions (no updating) & API response from source engines, hence the separate files + fix typing issue - without Partial<>, all 4 enum types are expected instead of 1-4 - for some reason this causes an error in a separate a util file, not sure why (Typescript issue?)

cee-chen · 2021-05-06T20:57:22Z

...terprise_search/public/applications/app_search/components/schema/schema_meta_engine_logic.ts

+interface MetaEngineSchemaValues extends SchemaBaseValues {
+  fields: MetaEngineSchemaApiResponse['fields'];
+  conflictingFields: MetaEngineSchemaApiResponse['conflictingFields'];
+  conflictingFieldsCount: number;
+  hasConflicts: boolean;
+}
+
+interface MetaEngineSchemaActions extends SchemaBaseActions {
+  loadMetaEngineSchema(): void;
+  onMetaEngineSchemaLoad(response: MetaEngineSchemaApiResponse): MetaEngineSchemaApiResponse;
+}
+
+export const MetaEngineSchemaLogic = kea<


Hopefully comparing the two different logic files between source engines & meta engines helps illustrate why I chose to separate them - they're all in 1 file right now in the standalone UI and it's pretty hard to grok as a result. Hoping this is at least tidier.

It's honestly weird because the Meta Engine view, aside from the schema data, is almost completely different from the default/indexed schema view - it has no actual actions to take for example.

Some day I'll have a breakthrough genius idea on how to better organize our meta engine vs indexed engine logic/views/etc., but that day is not today 🙃

x-pack/plugins/enterprise_search/public/applications/app_search/components/schema/types.ts

x-pack/plugins/enterprise_search/server/routes/app_search/schema.ts

cee-chen · 2021-05-06T21:04:26Z

x-pack/plugins/enterprise_search/public/applications/shared/schema/types.ts

@@ -31,7 +31,7 @@ export type Schema = Record<string, SchemaType>;

 // This is a mapping of schema field types ("text", "number", "geolocation", "date")
 // to the names of source engines which utilize that type
-export type SchemaConflictFieldTypes = Record<SchemaType, string[]>;
+export type SchemaConflictFieldTypes = Partial<Record<SchemaType, string[]>>;


Not sure how I didn't run into a type error with this before now, but without the Partial<> Typescript yells if you have just e.g. { text: ['some-engine'], number: ['another-engine'] } - it wants all the keys from SchemaType otherwise 🤷

...terprise_search/public/applications/app_search/components/schema/schema_meta_engine_logic.ts

...k/plugins/enterprise_search/public/applications/app_search/components/schema/schema_logic.ts

cee-chen · 2021-05-06T21:14:49Z

...gins/enterprise_search/public/applications/app_search/components/schema/schema_logic.test.ts

+    mostRecentIndexJob: {
+      percentageComplete: 100,
+      numDocumentsWithErrors: 10,
+      activeReindexJobId: 'some-id',
+      isActive: false,
+      hasErrors: true,
+    },


FYI that a large majority of this data we're no longer using after 7.12/the document storage refactor - percentageComplete will always come back as 100 and isActive as false. I still kept it in here as an example, but def would be nice to clean up our APIs some day

cee-chen · 2021-05-06T21:18:09Z

...gins/enterprise_search/public/applications/app_search/components/schema/schema_logic.test.ts

+  /**
+   * Unfortunately, we can't mount({ schema: ... }) & have to use an action to set schema
+   * because of the separate connected logic file - our LogicMounter test helper sets context
+   * for only the currently tested file
+   */
+  const mountAndSetSchema = ({ schema, ...values }: { schema: Schema; [key: string]: unknown }) => {
+    mount(values);
+    SchemaLogic.actions.setSchema(schema);
+  };


Just want to flag this as one very unfortunate hiccup in the connect approach to sharing state... our LogicMounter test helper automatically uses the current path to populate values, so we can't populate connected logics' values. So doing this:

mount({ schema: {} }); // belongs to SchemaBaseLogic, not SchemaLogic

does nothing and doesn't actually correctly set the value. I used this as a workaround in this file for now, but not sure if we should try to come up with a better/more elegant solution or just want for .extend() type support 🤷

That's interesting. How would this look if we bypassed the helper? 🤔

I assume something like this:

resetContext({ defaults: { enterprise_search: { app_search: { some_base_logic: { someSharedValue: 'hello', }, some_extended_logic: { someSpecificValue: 'world', }, }, }, }, });

Looking at it that way, I can definitely see the potential of extending our test mount helper to mount with separate files, but it would be fairly tricky and be a more complex API/usage than our current helper

Ahhh ok, that clears it up for me, thanks for posting that.

cee-chen · 2021-05-06T21:19:17Z

...k/plugins/enterprise_search/public/applications/app_search/components/schema/schema_logic.ts

+  connect: {
+    values: [SchemaBaseLogic, ['dataLoading', 'schema']],
+    actions: [SchemaBaseLogic, ['loadSchema', 'setSchema']],
+  },


FWIW though, I do actually kind of like that shared values/actions are so explicitly listed rather than .extend() being somewhat magical / invisible & having to dive into another file to see what's shared. Tradeoffs everywhere I guess!

I feel the same way. 💯

cee-chen · 2021-05-06T21:28:47Z

@elasticmachine merge upstream

JasonStoltz

I really like the base logic, it keeps things DRY without adding a ton of overhead.

Just a few questions and comments.

x-pack/plugins/enterprise_search/public/applications/app_search/components/schema/types.ts

...enterprise_search/public/applications/app_search/components/schema/schema_base_logic.test.ts

...gins/enterprise_search/public/applications/app_search/components/schema/schema_base_logic.ts

...gins/enterprise_search/public/applications/app_search/components/schema/schema_logic.test.ts

...k/plugins/enterprise_search/public/applications/app_search/components/schema/schema_logic.ts

...terprise_search/public/applications/app_search/components/schema/schema_meta_engine_logic.ts

...ise_search/public/applications/app_search/components/schema/schema_meta_engine_logic.test.ts

...terprise_search/public/applications/app_search/components/schema/schema_meta_engine_logic.ts

Much clean, such simple

cee-chen

@JasonStoltz check it out!!! 82a711c

-31 lines! This is so much simpler/cleaner haha. Thanks for showing me the light!! It somehow never occurred to me the shared action itself could continue to be extended via reducers 🤦‍♀️ Seriously so cool.

cee-chen · 2021-05-10T20:56:20Z

...enterprise_search/public/applications/app_search/components/schema/schema_base_logic.test.ts

@@ -23,6 +23,9 @@ describe('SchemaBaseLogic', () => {
    some_text_field: SchemaType.Text,
    some_number_field: SchemaType.Number,
  };
+  const MOCK_RESPONSE = {
+    schema: MOCK_SCHEMA,
+  } as any;


I'm being a bit lazy and using any in this test file to avoid the whole SchemaApiResponse | MetaEngineSchemaApiResponse typing shenanigans. I could cast as unknown as SchemaApiResponse if folks prefer me to stop taking shortcuts 😅

cee-chen · 2021-05-10T20:57:55Z

...gins/enterprise_search/public/applications/app_search/components/schema/schema_base_logic.ts

+  onSchemaLoad(
+    response: SchemaApiResponse | MetaEngineSchemaApiResponse
+  ): SchemaApiResponse | MetaEngineSchemaApiResponse;


Typing comments continued - this is mostly here as dev documentation. I override/extend the onSchemaLoad action typing in each individual extended logic file (see below comments).

cee-chen · 2021-05-10T20:58:31Z

...k/plugins/enterprise_search/public/applications/app_search/components/schema/schema_logic.ts

@@ -35,7 +35,6 @@ interface SchemaValues extends SchemaBaseValues {
 }

 interface SchemaActions extends SchemaBaseActions {
-  loadIndexedEngineSchema(): void;
  onSchemaLoad(response: SchemaApiResponse): SchemaApiResponse;


See above comment - this fixes the onSchemaLoad type differences between source & meta engines

cee-chen · 2021-05-10T20:58:39Z

...terprise_search/public/applications/app_search/components/schema/schema_meta_engine_logic.ts

@@ -18,8 +18,7 @@ interface MetaEngineSchemaValues extends SchemaBaseValues {
 }

 interface MetaEngineSchemaActions extends SchemaBaseActions {
-  loadMetaEngineSchema(): void;
-  onMetaEngineSchemaLoad(response: MetaEngineSchemaApiResponse): MetaEngineSchemaApiResponse;
+  onSchemaLoad(response: MetaEngineSchemaApiResponse): MetaEngineSchemaApiResponse;


See above comment - this fixes the onSchemaLoad type differences between source & meta engines

cee-chen · 2021-05-10T20:59:06Z

...terprise_search/public/applications/app_search/components/schema/schema_meta_engine_logic.ts

-  actions: {
-    loadMetaEngineSchema: true,
-    onMetaEngineSchemaLoad: (response) => response,
+    actions: [SchemaBaseLogic, ['loadSchema', 'onSchemaLoad']],


I cleaned up setSchema from this list since meta engines isn't using that action (bad copypasta on my part)

cee-chen · 2021-05-10T21:06:12Z

...terprise_search/public/applications/app_search/components/schema/schema_meta_engine_logic.ts

-    loadMetaEngineSchema: () => {
-      actions.loadSchema(actions.onMetaEngineSchemaLoad);
-    },
-  }),


Doh I forgot to include this comment in the batch just now, but just wanted to reiterate that it's SUPER cool I was able to drop the actions and listeners block for MetaEngineSchemaLogic totally. Thanks again Jason, you're a genius! 🧠

kibanamachine · 2021-05-10T22:40:56Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1368	1371	+3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`enterpriseSearch`	2.0MB	2.0MB	+4.9KB

History

💔 Build #124859 failed 3345322
💔 Build #124850 failed 542e58f

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

JasonStoltz

Great work! Glad that worked out for you.

…nnected Kea logic (elastic#99548) * Set up server API routes * Set up types * Set up shared/base Schema logic file - values & actions shared between both default source engine & meta engine pages * Add default/indexed engine SchemaLogic * Add MetaEnginesSchemaLogic - significantly different actions (no updating) & API response from source engines, hence the separate files + fix typing issue - without Partial<>, all 4 enum types are expected instead of 1-4 - for some reason this causes an error in a separate a util file, not sure why (Typescript issue?) * Update Schema & MetaEngineSchema views with loaders * PR feedback: comment nit * PR feedback: Remove unnecessary async/awaits * PR feedback: Simplify loadSchema to be shared by base logic Much clean, such simple Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-05-11T18:50:58Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…nnected Kea logic (#99548) (#99833) * Set up server API routes * Set up types * Set up shared/base Schema logic file - values & actions shared between both default source engine & meta engine pages * Add default/indexed engine SchemaLogic * Add MetaEnginesSchemaLogic - significantly different actions (no updating) & API response from source engines, hence the separate files + fix typing issue - without Partial<>, all 4 enum types are expected instead of 1-4 - for some reason this causes an error in a separate a util file, not sure why (Typescript issue?) * Update Schema & MetaEngineSchema views with loaders * PR feedback: comment nit * PR feedback: Remove unnecessary async/awaits * PR feedback: Simplify loadSchema to be shared by base logic Much clean, such simple Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Constance <[email protected]>

- Missed this in elastic#99548

* Add schema add field modal to page * Add SchemaCallouts component * Add empty state * Add SchemaTable component - Main update functionality is covered by the shared SchemaFieldTypeSelect component + Readd 'Recently added' i18n strings removed in https://github.com/elastic/kibana/pull/98955/files/d93e31dd415ba61354ab3714cd2728d60efa6ddc#r624038044 squash with table * Add final update types button behavior + expand tests more explicitly & cleanly * Fix i18n just throw my body in the trash * Update x-pack/plugins/enterprise_search/public/applications/app_search/components/schema/views/schema.test.tsx Co-authored-by: Jason Stoltzfus <[email protected]> * [Misc] Add missing SchemaBaseLogic state check - Missed this in #99548 * Update x-pack/plugins/enterprise_search/public/applications/app_search/components/schema/components/empty_state.tsx Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Jason Stoltzfus <[email protected]>

* Add schema add field modal to page * Add SchemaCallouts component * Add empty state * Add SchemaTable component - Main update functionality is covered by the shared SchemaFieldTypeSelect component + Readd 'Recently added' i18n strings removed in https://github.com/elastic/kibana/pull/98955/files/d93e31dd415ba61354ab3714cd2728d60efa6ddc#r624038044 squash with table * Add final update types button behavior + expand tests more explicitly & cleanly * Fix i18n just throw my body in the trash * Update x-pack/plugins/enterprise_search/public/applications/app_search/components/schema/views/schema.test.tsx Co-authored-by: Jason Stoltzfus <[email protected]> * [Misc] Add missing SchemaBaseLogic state check - Missed this in elastic#99548 * Update x-pack/plugins/enterprise_search/public/applications/app_search/components/schema/components/empty_state.tsx Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Jason Stoltzfus <[email protected]>

* Add schema add field modal to page * Add SchemaCallouts component * Add empty state * Add SchemaTable component - Main update functionality is covered by the shared SchemaFieldTypeSelect component + Readd 'Recently added' i18n strings removed in https://github.com/elastic/kibana/pull/98955/files/d93e31dd415ba61354ab3714cd2728d60efa6ddc#r624038044 squash with table * Add final update types button behavior + expand tests more explicitly & cleanly * Fix i18n just throw my body in the trash * Update x-pack/plugins/enterprise_search/public/applications/app_search/components/schema/views/schema.test.tsx Co-authored-by: Jason Stoltzfus <[email protected]> * [Misc] Add missing SchemaBaseLogic state check - Missed this in #99548 * Update x-pack/plugins/enterprise_search/public/applications/app_search/components/schema/components/empty_state.tsx Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Jason Stoltzfus <[email protected]> Co-authored-by: Constance <[email protected]> Co-authored-by: Jason Stoltzfus <[email protected]>

cee-chen added 6 commits May 6, 2021 09:15

Set up server API routes

312128a

Set up types

6cc7e18

Set up shared/base Schema logic file

e19ad5b

- values & actions shared between both default source engine & meta engine pages

Add default/indexed engine SchemaLogic

ff05b53

Update Schema & MetaEngineSchema views with loaders

542e58f

cee-chen added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels May 6, 2021

cee-chen requested review from JasonStoltz and a team May 6, 2021 20:49

cee-chen commented May 6, 2021

View reviewed changes

Merge branch 'master' into schema-2

3345322

JasonStoltz requested changes May 10, 2021

View reviewed changes

cee-chen added 4 commits May 10, 2021 12:50

Merge branch 'master' into schema-2

0c60338

PR feedback: comment nit

c12cdea

PR feedback: Remove unnecessary async/awaits

78c8f6d

PR feedback: Simplify loadSchema to be shared by base logic

82a711c

Much clean, such simple

cee-chen commented May 10, 2021

View reviewed changes

JasonStoltz approved these changes May 11, 2021

View reviewed changes

cee-chen merged commit c28213b into elastic:master May 11, 2021

kibanamachine mentioned this pull request May 11, 2021

[7.x] [App Search] Schema: Set up server routes, grand foray into shared/connected Kea logic (#99548) #99833

Merged

cee-chen deleted the schema-2 branch May 11, 2021 18:52

cee-chen mentioned this pull request May 12, 2021

[App Search] Schema view #99868

Merged

29 tasks

cee-chen added a commit to cee-chen/kibana that referenced this pull request May 12, 2021

[Misc] Add missing SchemaBaseLogic state check

12dea36

- Missed this in elastic#99548

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Schema: Set up server routes, grand foray into shared/connected Kea logic #99548

[App Search] Schema: Set up server routes, grand foray into shared/connected Kea logic #99548

cee-chen commented May 6, 2021

cee-chen May 6, 2021

cee-chen May 6, 2021

cee-chen May 6, 2021

cee-chen May 6, 2021

JasonStoltz May 10, 2021

cee-chen May 10, 2021 •

edited

Loading

JasonStoltz May 10, 2021 •

edited

Loading

cee-chen May 6, 2021 •

edited

Loading

scottybollinger May 10, 2021

cee-chen commented May 6, 2021

JasonStoltz left a comment

cee-chen left a comment

cee-chen May 10, 2021

cee-chen May 10, 2021

cee-chen May 10, 2021

cee-chen May 10, 2021

cee-chen May 10, 2021

cee-chen May 10, 2021 •

edited

Loading

kibanamachine commented May 10, 2021

JasonStoltz left a comment

kibanamachine commented May 11, 2021

[App Search] Schema: Set up server routes, grand foray into shared/connected Kea logic #99548

[App Search] Schema: Set up server routes, grand foray into shared/connected Kea logic #99548

Conversation

cee-chen commented May 6, 2021

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen May 10, 2021 • edited Loading

Choose a reason for hiding this comment

JasonStoltz May 10, 2021 • edited Loading

Choose a reason for hiding this comment

cee-chen May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen commented May 6, 2021

JasonStoltz left a comment

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen May 10, 2021 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented May 10, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

JasonStoltz left a comment

Choose a reason for hiding this comment

kibanamachine commented May 11, 2021

💚 Backport successful

cee-chen May 10, 2021 •

edited

Loading

JasonStoltz May 10, 2021 •

edited

Loading

cee-chen May 6, 2021 •

edited

Loading

cee-chen May 10, 2021 •

edited

Loading