Split integrations.ts into handlers & data by jfsiii · Pull Request #40685 · elastic/kibana

jfsiii · 2019-07-09T20:19:21Z

Summary

200+ line server/integrations.ts split into {data,handlers}.ts around 100 lines each. The logic/behavior is the same. Functions were just moved around and I think things are much more clear.

handlers functions parse the requests and call the functions from data.

data functions coordinate with the registry, saved objects, etc.

Essentially a mechanical refactor for now. Will work on reducing the work done in the handlers in later commits/PRs.

Behavior should be unchanged.

jfsiii · 2019-07-09T20:21:12Z

x-pack/legacy/plugins/integrations_manager/common/types.ts

 export type IntegrationList = IntegrationListItem[];

-export type IntegrationListItem = Installed<RegistryListItem> | NotInstalled<RegistryListItem>;
+export type IntegrationListItem = Installable<RegistryListItem>;


So you don’t have to do the | each time. Went with Installable over Maybe*, Either*, etc.

jfsiii · 2019-07-09T20:25:06Z

x-pack/legacy/plugins/integrations_manager/server/integrations/data.ts

+import { SAVED_OBJECT_TYPE } from '../../common/constants';
+import * as Registry from '../registry';
+
+export async function getIntegrations(client: SavedObjectsClientContract) {


These functions all accept a saved object client. Mainly because we get the client from the request, but it also means we can inject a client. Figure we’ll come back to this over time and when the client lands in core .

So happy to have TS here...

jfsiii · 2019-07-09T20:25:41Z

x-pack/legacy/plugins/integrations_manager/server/integrations/data.ts

+  }
+}
+
+function createInstallableFrom<T>(from: T, savedObject?: InstallationSavedObject): Installable<T> {


Dropped all the overloads

elasticmachine · 2019-07-09T21:18:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-09T22:36:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

This is consistent with other registry functions. Returned to nested for loops internally.

jfsiii · 2019-07-10T19:04:00Z

x-pack/legacy/plugins/integrations_manager/server/integrations/handlers.ts

  if (asset === 'dashboard') {
    const client = getClient(req);
-    const object = await installAssets(client, pkgkey, asset);
+    const object = await installAssets(client, pkgkey, (entry: ArchiveEntry) => {


Use a filter function like other registry functions

jfsiii · 2019-07-10T19:04:25Z

x-pack/legacy/plugins/integrations_manager/server/registry.ts

    const { path, buffer } = entry;
-    paths.push(path);
+    const { file } = pathParts(path);
+    if (!file) return;


Don’t add directories to the paths. Only files.

jfsiii · 2019-07-10T19:11:09Z

x-pack/legacy/plugins/integrations_manager/server/registry.ts

-    collectReferences(map, { path, desiredType });
-    return map;
-  }, new Map());
+export async function getObjects(


Condensed getObjects & collectReferences into one function. Added a ton of comments.

Went with nested fors vs several map, reduce calls and flatten. For context, these might be the first for loops I’ve written in 5 years. I’m not overly concerned with performance but would like to skip the garbage creation/collection of traversing & transforming as much as possible. I also find this the most clear way to express what we’re doing.

Love these comments. Totally fine with the for loops, I don't love mutating objects that get passed to other functions, so having this all in one function really helps follow that mutation thread for me.

This feels like a function I'd love to have tests for. Can we log a ticket to do that soon, if not now? Not concerned about 100% coverage or anything but testing a few of these functions would be a great start.

For sure. Created this very minimal issue but feel free to add any others you think of.

jasonrhodes · 2019-07-10T19:38:02Z

x-pack/legacy/plugins/integrations_manager/server/registry.ts

+  if (!asset.id) asset.id = file.replace('.json', '');
+
+  return ensureJsonValues(asset);
+}


Mind adding a few more comments on these two functions to explain the diff between getAsset and getObject? I think I follow but a little lost on what's coming out of the cache that needs to be toString-ified.

Updated in 5437feb

jasonrhodes · 2019-07-10T19:39:40Z

x-pack/legacy/plugins/integrations_manager/server/integrations/handlers.ts

+}
+
+// eslint-disable-next-line @typescript-eslint/no-empty-interface
+interface DeleteAssetRequest extends InstallAssetRequest {}


Are we expecting these to diverge at some point?

Not sure if I follow, but I made some changes here. Can you look at 13cef20 and let me know if you still have questions/suggestions?

jasonrhodes · 2019-07-10T19:56:07Z

x-pack/legacy/plugins/integrations_manager/server/integrations/data.ts

+  }));
+
+  const results = await client.bulkGet(searchObjects);
+  const savedObjects: InstallationSavedObject[] = results.saved_objects.filter(o => !o.error); // ignore errors for now


do you know if the saved object client takes a type generic for what its methods return? I like being able to do:

const results = await client.bulkGet<InstallationSavedObject>(searchObjects);

and not having to explicitly type the return of the filter on the next line, if the client method handles that properly. Not a blocker just curious.

I haven’t seen that before but, yeah, they do.

Or something quite similar

kibana/src/core/server/saved_objects/service/saved_objects_client.ts

Line 339 in 40a1bde

async bulkGet<T extends SavedObjectAttributes = any>(

&

kibana/src/core/server/saved_objects/service/saved_objects_client.ts

Lines 149 to 164 in 40a1bde

export type SavedObjectAttribute =

| string

| number

| boolean

| null

| undefined

| SavedObjectAttributes

| SavedObjectAttributes[];

/**

*

* @public

*/

export interface SavedObjectAttributes {

[key: string]: SavedObjectAttribute | SavedObjectAttribute[];

}

Take a look at 98dfac8. I updated all the client.* calls and removed the explicit typing of their return values.

elasticmachine · 2019-07-10T20:02:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jasonrhodes · 2019-07-10T20:04:38Z

x-pack/legacy/plugins/integrations_manager/server/integrations/data.ts

+  return integrationList;
+}
+
+export async function getIntegrationInfo(client: SavedObjectsClientContract, pkgkey: string) {


Is this the single resource version of getIntegrations?

Yes, IIUC. It’s what’s used for the detail page. getIntegrations is what’s used for for the list view.

jasonrhodes · 2019-07-10T20:06:08Z

x-pack/legacy/plugins/integrations_manager/server/integrations/data.ts

+}
+
+export async function getInstallationObject(
+  client: SavedObjectsClientContract,


And this is "get the current install state for this one integration"?

jasonrhodes · 2019-07-10T20:08:10Z

x-pack/legacy/plugins/integrations_manager/server/integrations/data.ts

@@ -0,0 +1,109 @@
+/*


I think a few simple comments on top of each of the exported functions in this file to explain what they do in relationship to each other would go a long way in helping new folks understand the flow here, and then we can massage the actual names of the functions as we better understand how it all fits together or what we need for other cases, etc. Otherwise this is all really clear and clean.

Agreed. Breaking this up helped me see some patterns.

If it’s OK with you, I’d like to hold off on that until I add another asset type (e.g. #38609) because I think that might influence things. Things don’t need to be “final” before I add the comments, just looking to get back to making after this refactoring then see how things fit together.

jasonrhodes

Looks good, had a few suggestions for clarifying via a few more comments, and I had a couple questions, but it's mergeable as-is.

Don't need a generic accessor (e.g. for docs, images, etc) yet. Will factor out when it's needed.

I think this is clearer. It also makes sure we don't loop over any keys other than the "root" ones we just matched (vs all that are in the map/accumulator).

elasticmachine · 2019-07-10T21:26:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-10T22:10:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

John Schulz added 4 commits July 9, 2019 11:15

Split integrations.ts into integrations/{index,handlers}.ts

362555a

Essentially a mechanical refactor for now. Will work on reducing the work done in the handlers in later commits/PRs.

Do what I wanted to do initially but didn't know how.

e89b5b0

Pull in renames from 40548-correct-asset-names

5c37b3a

index -> {handlers,data,registry}.ts. Handlers parse reqs; call data fns

f81e670

Behavior should be unchanged.

jfsiii requested a review from jasonrhodes July 9, 2019 20:19

jfsiii changed the title ~~Split up integrations~~ Split integrations.ts into handlers & data Jul 9, 2019

jfsiii commented Jul 9, 2019

View reviewed changes

Fix eslint/prettier issues. Not sure what happened

f737949

getObjects: accept a filter function & comments for getting references.

0f2ba73

This is consistent with other registry functions. Returned to nested for loops internally.

jfsiii commented Jul 10, 2019

View reviewed changes

jasonrhodes reviewed Jul 10, 2019

View reviewed changes

jasonrhodes approved these changes Jul 10, 2019

View reviewed changes

John Schulz added 4 commits July 10, 2019 16:18

Reorder so all exported functions are at the top of the file.

04247a4

Collapse getAsset into getObject. Add some comments.

5437feb

Don't need a generic accessor (e.g. for docs, images, etc) yet. Will factor out when it's needed.

Add a name for intermediate value. Avoid extra iterations.

9b978e9

I think this is clearer. It also makes sure we don't loop over any keys other than the "root" ones we just matched (vs all that are in the map/accumulator).

Request interfaces no extend already extend values. Add a param type.

13cef20

jfsiii mentioned this pull request Jul 10, 2019

[EPM] Add an example unit test #40802

Closed

2 tasks

Change two names related to Saved Objects. Call SO client with types.

98dfac8

jfsiii merged commit 15b54d5 into elastic:feature-integrations-manager Jul 10, 2019

jfsiii mentioned this pull request Sep 12, 2019

Replace feature with asset in code #40640

Closed

jfsiii deleted the split-up-integrations branch December 30, 2019 16:12

jen-huang added the Team:Fleet Team label for Observability Data Collection Fleet team label Mar 26, 2020

	export type SavedObjectAttribute =
	\| string
	\| number
	\| boolean
	\| null
	\| undefined
	\| SavedObjectAttributes
	\| SavedObjectAttributes[];

	/**
	*
	* @public
	*/
	export interface SavedObjectAttributes {
	[key: string]: SavedObjectAttribute \| SavedObjectAttribute[];
	}

Conversation

jfsiii commented Jul 9, 2019

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 9, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 9, 2019

💚 Build Succeeded

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonrhodes Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 10, 2019

💚 Build Succeeded

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonrhodes left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 10, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 10, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jasonrhodes Jul 10, 2019 •

edited

Loading