Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split integrations.ts into handlers & data #40685

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import {
SavedObject,
SavedObjectsBulkGetObject,
SavedObjectsClientContract,
} from 'src/core/server/saved_objects';
Expand Down Expand Up @@ -53,11 +54,12 @@ export async function getInstallationObject(
export async function installAssets(
client: SavedObjectsClientContract,
pkgkey: string,
asset: string
filter = (entry: Registry.ArchiveEntry): boolean => true
) {
const toBeSavedObjects = await Registry.getObjects(pkgkey, asset);
const toBeSavedObjects = await Registry.getObjects(pkgkey, filter);
const createResults = await client.bulkCreate(toBeSavedObjects, { overwrite: true });
const installed = createResults.saved_objects.map(({ id, type }) => ({ id, type }));
const createdObjects: SavedObject[] = createResults.saved_objects;
const installed = createdObjects.map(({ id, type }) => ({ id, type }));
const results = await client.create(
SAVED_OBJECT_TYPE,
{ installed },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import { Request, InstallationSavedObject } from '../../common/types';
import { ArchiveEntry, pathParts } from '../registry';
import { getClient } from '../saved_objects';
import { getIntegrations, getIntegrationInfo, installAssets, removeInstallation } from './data';

Expand All @@ -20,6 +21,9 @@ interface InstallAssetRequest extends PackageRequest {
};
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we expecting these to diverge at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


export async function handleGetList(req: Request) {
const client = getClient(req);
const integrationList = await getIntegrations(client);
Expand All @@ -41,7 +45,10 @@ export async function handleRequestInstall(req: InstallAssetRequest) {

if (asset === 'dashboard') {
const client = getClient(req);
const object = await installAssets(client, pkgkey, asset);
const object = await installAssets(client, pkgkey, (entry: ArchiveEntry) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a filter function like other registry functions

const { type } = pathParts(entry.path);
return type === asset;
});

created.push(object);
}
Expand All @@ -53,7 +60,7 @@ export async function handleRequestInstall(req: InstallAssetRequest) {
};
}

export async function handleRequestDelete(req: InstallAssetRequest) {
export async function handleRequestDelete(req: DeleteAssetRequest) {
const { pkgkey, asset } = req.params;
const client = getClient(req);
const deleted = await removeInstallation(client, pkgkey);
Expand Down
100 changes: 62 additions & 38 deletions x-pack/legacy/plugins/integrations_manager/server/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { SavedObject, SavedObjectReference } from 'src/core/server/saved_objects';
import { SavedObject } from 'src/core/server/saved_objects';
import { RegistryList, RegistryPackage } from '../common/types';
import { cacheGet, cacheSet, cacheHas } from './cache';
import { ArchiveEntry, untarBuffer, unzipBuffer } from './extract';
import { fetchUrl, getResponseStream } from './requests';
import { streamToBuffer } from './streams';

export { ArchiveEntry } from './extract';

const REGISTRY = process.env.REGISTRY || 'http://integrations-registry.app.elstc.co';

export async function fetchList(): Promise<RegistryList> {
Expand All @@ -28,9 +30,13 @@ export async function getArchiveInfo(
const paths: string[] = [];
const onEntry = (entry: ArchiveEntry) => {
const { path, buffer } = entry;
paths.push(path);
const { file } = pathParts(path);
if (!file) return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (cacheHas(path)) return;
if (buffer) cacheSet(path, buffer);
if (buffer) {
cacheSet(path, buffer);
paths.push(path);
}
};

await extract(key, filter, onEntry);
Expand Down Expand Up @@ -67,42 +73,48 @@ async function fetchArchiveBuffer(key: string): Promise<Buffer> {
return getResponseStream(`${REGISTRY}/package/${key}`).then(streamToBuffer);
}

export async function getObjects(pkgkey: string, desiredType?: string): Promise<SavedObject[]> {
const paths = await getArchiveInfo(`${pkgkey}.tar.gz`);
const toBeSavedObjects = paths.reduce((map, path) => {
collectReferences(map, { path, desiredType });
return map;
}, new Map());
export async function getObjects(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pkgkey: string,
filter = (entry: ArchiveEntry): boolean => true
): Promise<SavedObject[]> {
// Create a Map b/c some values, especially index-patterns, are referenced multiple times
const objects: Map<string, SavedObject> = new Map();

// Get paths which match the given filter
const paths = await getArchiveInfo(`${pkgkey}.tar.gz`, filter);

// Add all objects which matched filter to the Map
paths.map(getObject).forEach(obj => objects.set(obj.id, obj));

// Each of those objects might have `references` property like [{id, type, name}]
for (const object of objects.values()) {
// For each of those objects
for (const reference of object.references) {
// Get the objects they reference. Call same function with a new filter
const referencedObjects = await getObjects(pkgkey, (entry: ArchiveEntry) => {
// Skip anything we've already stored
if (objects.has(reference.id)) return false;

// Is the archive entry the reference we want?
const { type, file } = pathParts(entry.path);
const isType = type === reference.type;
const isJson = file === `${reference.id}.json`;
return isType && isJson;
});

// Add referenced objects to the Map
referencedObjects.forEach(ro => objects.set(ro.id, ro));
}
}

return Array.from(toBeSavedObjects.values(), ensureJsonValues);
// return the array of unique objects
return Array.from(objects.values());
}

interface CollectReferencesOptions {
path: string;
desiredType?: string; // TODO: from enum or similar of acceptable asset types
}
function collectReferences(
toBeSavedObjects: Map<string, SavedObject> = new Map(),
{ path, desiredType }: CollectReferencesOptions
) {
export function pathParts(path: string) {
const [pkgkey, service, type, file] = path.split('/');
if (type !== desiredType) return;
if (toBeSavedObjects.has(path)) return;
if (!/\.json$/.test(path)) return;

const asset = getAsset(path);
if (!asset.type) asset.type = type;
if (!asset.id) asset.id = file.replace('.json', '');
toBeSavedObjects.set(path, asset);

const references: SavedObjectReference[] = asset.references;
return references.reduce((map, reference) => {
collectReferences(toBeSavedObjects, {
path: `${pkgkey}/${service}/${reference.type}/${reference.id}.json`,
desiredType: reference.type,
});
return map;
}, toBeSavedObjects);
return { pkgkey, service, type, file };
}

// the assets from the registry are malformed
Expand All @@ -125,8 +137,20 @@ function ensureJsonValues(obj: SavedObject) {

function getAsset(key: string) {
const value = cacheGet(key);
if (value !== undefined) {
const json = value.toString('utf8');
return JSON.parse(json);
}
if (value === undefined) throw new Error(`Cannot find asset ${key}`);

const json = value.toString('utf8');
const asset = JSON.parse(json);

return asset;
}

function getObject(key: string) {
const asset = getAsset(key);
const { type, file } = pathParts(key);

if (!asset.type) asset.type = type;
if (!asset.id) asset.id = file.replace('.json', '');

return ensureJsonValues(asset);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 5437feb