From 6109e354951a30bcabddf1c550ee70dec61249dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Fri, 9 Oct 2020 13:33:20 +0200 Subject: [PATCH 1/3] feat(catalog-backend): remove byUid and byName as special cases --- .../src/catalog/DatabaseEntitiesCatalog.ts | 17 +------ plugins/catalog-backend/src/catalog/types.ts | 4 +- .../ingestion/HigherOrderOperations.test.ts | 2 - .../src/service/router.test.ts | 46 ++++++++++--------- plugins/catalog-backend/src/service/router.ts | 22 +++++---- 5 files changed, 38 insertions(+), 53 deletions(-) diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts index 2575411e7c..c43e1b2f05 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts @@ -15,13 +15,12 @@ */ import { NotFoundError } from '@backstage/backend-common'; +import type { Entity } from '@backstage/catalog-model'; import { - EntityName, generateUpdatedEntity, getEntityName, LOCATION_ANNOTATION, } from '@backstage/catalog-model'; -import type { Entity } from '@backstage/catalog-model'; import type { Database, DbEntityResponse, EntityFilters } from '../database'; import type { EntitiesCatalog } from './types'; @@ -35,20 +34,6 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { return items.map(i => i.entity); } - async entityByUid(uid: string): Promise { - const response = await this.database.transaction(tx => - this.database.entityByUid(tx, uid), - ); - return response?.entity; - } - - async entityByName(name: EntityName): Promise { - const response = await this.database.transaction(tx => - this.database.entityByName(tx, name), - ); - return response?.entity; - } - async addOrUpdateEntity( entity: Entity, locationId?: string, diff --git a/plugins/catalog-backend/src/catalog/types.ts b/plugins/catalog-backend/src/catalog/types.ts index 5d84cbcebc..810d51a6c1 100644 --- a/plugins/catalog-backend/src/catalog/types.ts +++ b/plugins/catalog-backend/src/catalog/types.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Entity, EntityName, Location } from '@backstage/catalog-model'; +import { Entity, Location } from '@backstage/catalog-model'; import type { EntityFilters } from '../database'; // @@ -23,8 +23,6 @@ import type { EntityFilters } from '../database'; export type EntitiesCatalog = { entities(filters?: EntityFilters): Promise; - entityByUid(uid: string): Promise; - entityByName(name: EntityName): Promise; addOrUpdateEntity(entity: Entity, locationId?: string): Promise; addEntities(entities: Entity[], locationId?: string): Promise; removeEntityByUid(uid: string): Promise; diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts index 15d9168732..81753fe6d1 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts @@ -36,8 +36,6 @@ describe('HigherOrderOperations', () => { beforeAll(() => { entitiesCatalog = { entities: jest.fn(), - entityByUid: jest.fn(), - entityByName: jest.fn(), addOrUpdateEntity: jest.fn(), addEntities: jest.fn(), removeEntityByUid: jest.fn(), diff --git a/plugins/catalog-backend/src/service/router.test.ts b/plugins/catalog-backend/src/service/router.test.ts index ecf4b0e614..ec5c11be33 100644 --- a/plugins/catalog-backend/src/service/router.test.ts +++ b/plugins/catalog-backend/src/service/router.test.ts @@ -32,8 +32,6 @@ describe('createRouter', () => { beforeAll(async () => { entitiesCatalog = { entities: jest.fn(), - entityByUid: jest.fn(), - entityByName: jest.fn(), addOrUpdateEntity: jest.fn(), addEntities: jest.fn(), removeEntityByUid: jest.fn(), @@ -100,23 +98,27 @@ describe('createRouter', () => { name: 'c', }, }; - entitiesCatalog.entityByUid.mockResolvedValue(entity); + entitiesCatalog.entities.mockResolvedValue([entity]); const response = await request(app).get('/entities/by-uid/zzz'); - expect(entitiesCatalog.entityByUid).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entityByUid).toHaveBeenCalledWith('zzz'); + expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); + expect(entitiesCatalog.entities).toHaveBeenCalledWith([ + { key: 'metadata.uid', values: ['zzz'] }, + ]); expect(response.status).toEqual(200); expect(response.body).toEqual(expect.objectContaining(entity)); }); it('responds with a 404 for missing entities', async () => { - entitiesCatalog.entityByUid.mockResolvedValue(undefined); + entitiesCatalog.entities.mockResolvedValue([]); const response = await request(app).get('/entities/by-uid/zzz'); - expect(entitiesCatalog.entityByUid).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entityByUid).toHaveBeenCalledWith('zzz'); + expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); + expect(entitiesCatalog.entities).toHaveBeenCalledWith([ + { key: 'metadata.uid', values: ['zzz'] }, + ]); expect(response.status).toEqual(404); expect(response.text).toMatch(/uid/); }); @@ -132,31 +134,31 @@ describe('createRouter', () => { namespace: 'ns', }, }; - entitiesCatalog.entityByName.mockResolvedValue(entity); + entitiesCatalog.entities.mockResolvedValue([entity]); const response = await request(app).get('/entities/by-name/k/ns/n'); - expect(entitiesCatalog.entityByName).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entityByName).toHaveBeenCalledWith({ - kind: 'k', - namespace: 'ns', - name: 'n', - }); + expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); + expect(entitiesCatalog.entities).toHaveBeenCalledWith([ + { key: 'kind', values: ['k'] }, + { key: 'metadata.namespace', values: ['ns'] }, + { key: 'metadata.name', values: ['n'] }, + ]); expect(response.status).toEqual(200); expect(response.body).toEqual(expect.objectContaining(entity)); }); it('responds with a 404 for missing entities', async () => { - entitiesCatalog.entityByName.mockResolvedValue(undefined); + entitiesCatalog.entities.mockResolvedValue([]); const response = await request(app).get('/entities/by-name/b/d/c'); - expect(entitiesCatalog.entityByName).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entityByName).toHaveBeenCalledWith({ - kind: 'b', - namespace: 'd', - name: 'c', - }); + expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); + expect(entitiesCatalog.entities).toHaveBeenCalledWith([ + { key: 'kind', values: ['b'] }, + { key: 'metadata.namespace', values: ['d'] }, + { key: 'metadata.name', values: ['c'] }, + ]); expect(response.status).toEqual(404); expect(response.text).toMatch(/name/); }); diff --git a/plugins/catalog-backend/src/service/router.ts b/plugins/catalog-backend/src/service/router.ts index 8da589c721..06a7ea43d3 100644 --- a/plugins/catalog-backend/src/service/router.ts +++ b/plugins/catalog-backend/src/service/router.ts @@ -54,11 +54,13 @@ export async function createRouter( }) .get('/entities/by-uid/:uid', async (req, res) => { const { uid } = req.params; - const entity = await entitiesCatalog.entityByUid(uid); - if (!entity) { + const entities = await entitiesCatalog.entities([ + { key: 'metadata.uid', values: [uid] }, + ]); + if (!entities.length) { res.status(404).send(`No entity with uid ${uid}`); } - res.status(200).send(entity); + res.status(200).send(entities[0]); }) .delete('/entities/by-uid/:uid', async (req, res) => { const { uid } = req.params; @@ -67,19 +69,19 @@ export async function createRouter( }) .get('/entities/by-name/:kind/:namespace/:name', async (req, res) => { const { kind, namespace, name } = req.params; - const entity = await entitiesCatalog.entityByName({ - kind, - namespace, - name, - }); - if (!entity) { + const entities = await entitiesCatalog.entities([ + { key: 'kind', values: [kind] }, + { key: 'metadata.namespace', values: [namespace] }, + { key: 'metadata.name', values: [name] }, + ]); + if (!entities.length) { res .status(404) .send( `No entity with kind ${kind} namespace ${namespace} name ${name}`, ); } - res.status(200).send(entity); + res.status(200).send(entities[0]); }); } From 67e7d476a2e653916c6e2a0cccf2d25a648914c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Fri, 9 Oct 2020 16:24:33 +0200 Subject: [PATCH 2/3] feat(catalog-backend): simplify the search filters structure --- .../src/catalog/DatabaseEntitiesCatalog.ts | 9 +- .../src/database/CommonDatabase.test.ts | 17 +--- .../src/database/CommonDatabase.ts | 99 ++++++++++--------- .../src/database/search.test.ts | 3 + .../catalog-backend/src/database/search.ts | 35 +++++-- plugins/catalog-backend/src/database/types.ts | 26 ++++- .../ingestion/HigherOrderOperations.test.ts | 13 +-- .../src/ingestion/HigherOrderOperations.ts | 11 +-- .../src/service/router.test.ts | 42 ++++---- plugins/catalog-backend/src/service/router.ts | 29 +++--- 10 files changed, 155 insertions(+), 129 deletions(-) diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts index c43e1b2f05..8619355c91 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts @@ -85,12 +85,9 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { const location = entityResponse.entity.metadata.annotations?.[LOCATION_ANNOTATION]; const colocatedEntities = location - ? await this.database.entities(tx, [ - { - key: `metadata.annotations.${LOCATION_ANNOTATION}`, - values: [location], - }, - ]) + ? await this.database.entities(tx, { + [`metadata.annotations.${LOCATION_ANNOTATION}`]: location, + }) : [entityResponse]; for (const dbResponse of colocatedEntities) { await this.database.removeEntityByUid( diff --git a/plugins/catalog-backend/src/database/CommonDatabase.test.ts b/plugins/catalog-backend/src/database/CommonDatabase.test.ts index 840af74825..f3ca212df0 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.test.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.test.ts @@ -347,7 +347,7 @@ describe('CommonDatabase', () => { await db.transaction(async tx => { await db.addEntities(tx, [{ entity: e1 }, { entity: e2 }]); }); - const result = await db.transaction(async tx => db.entities(tx, [])); + const result = await db.transaction(async tx => db.entities(tx, {})); expect(result.length).toEqual(2); expect(result).toEqual( expect.arrayContaining([ @@ -389,10 +389,7 @@ describe('CommonDatabase', () => { await expect( db.transaction(async tx => - db.entities(tx, [ - { key: 'kind', values: ['k2'] }, - { key: 'spec.c', values: ['some'] }, - ]), + db.entities(tx, { kind: 'k2', 'spec.c': 'some' }), ), ).resolves.toEqual([ { @@ -427,10 +424,7 @@ describe('CommonDatabase', () => { }); const rows = await db.transaction(async tx => - db.entities(tx, [ - { key: 'apiVersion', values: ['a'] }, - { key: 'spec.c', values: [null, 'some'] }, - ]), + db.entities(tx, { apiVersion: 'a', 'spec.c': [null, 'some'] }), ); expect(rows.length).toEqual(3); @@ -477,10 +471,7 @@ describe('CommonDatabase', () => { }); const rows = await db.transaction(async tx => - db.entities(tx, [ - { key: 'ApiVersioN', values: ['A'] }, - { key: 'spEc.C', values: [null, 'some'] }, - ]), + db.entities(tx, { ApiVersioN: 'A', 'spEc.C': [null, 'some'] }), ); expect(rows.length).toEqual(3); diff --git a/plugins/catalog-backend/src/database/CommonDatabase.ts b/plugins/catalog-backend/src/database/CommonDatabase.ts index 49b36e0933..563fd24c80 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.ts @@ -219,61 +219,64 @@ export class CommonDatabase implements Database { let entitiesQuery = tx('entities'); - for (const filter of filters || []) { - const key = filter.key.toLowerCase().replace(/[*]/g, '%'); - const keyOp = filter.key.includes('*') ? 'like' : '='; + if (filters && Object.keys(filters).length) { + for (const [matchKey, matchVal] of Object.entries(filters)) { + const key = matchKey.toLowerCase().replace(/[*]/g, '%'); + const keyOp = key.includes('*') ? 'like' : '='; + const values = Array.isArray(matchVal) ? matchVal : [matchVal]; - let matchNulls = false; - const matchIn: string[] = []; - const matchLike: string[] = []; + let matchNulls = false; + const matchIn: string[] = []; + const matchLike: string[] = []; - for (const value of filter.values) { - if (!value) { - matchNulls = true; - } else if (value.includes('*')) { - matchLike.push(value.toLowerCase().replace(/[*]/g, '%')); - } else { - matchIn.push(value.toLowerCase()); + for (const value of values) { + if (!value) { + matchNulls = true; + } else if (value.includes('*')) { + matchLike.push(value.toLowerCase().replace(/[*]/g, '%')); + } else { + matchIn.push(value.toLowerCase()); + } } - } - // NOTE(freben): This used to be a set of OUTER JOIN, which may seem to - // make a lot of sense. However, it had abysmal performance on sqlite - // when datasets grew large, so we're using IN instead. - const matchQuery = tx('entities_search') - .select('entity_id') - .where(function keyFilter() { - this.andWhere('key', keyOp, key); - this.andWhere(function valueFilter() { - if (matchIn.length === 1) { - this.orWhere({ value: matchIn[0] }); - } else if (matchIn.length > 1) { - this.orWhereIn('value', matchIn); - } - if (matchLike.length) { - for (const x of matchLike) { - this.orWhere('value', 'like', tx.raw('?', [x])); + // NOTE(freben): This used to be a set of OUTER JOIN, which may seem to + // make a lot of sense. However, it had abysmal performance on sqlite + // when datasets grew large, so we're using IN instead. + const matchQuery = tx('entities_search') + .select('entity_id') + .where(function keyFilter() { + this.andWhere('key', keyOp, key); + this.andWhere(function valueFilter() { + if (matchIn.length === 1) { + this.orWhere({ value: matchIn[0] }); + } else if (matchIn.length > 1) { + this.orWhereIn('value', matchIn); } - } - if (matchNulls) { - // Match explicit nulls, and then handle absence separately below - this.orWhereNull('value'); - } + if (matchLike.length) { + for (const x of matchLike) { + this.orWhere('value', 'like', tx.raw('?', [x])); + } + } + if (matchNulls) { + // Match explicit nulls, and then handle absence separately below + this.orWhereNull('value'); + } + }); }); - }); - // Handle absence as nulls as well - entitiesQuery = entitiesQuery.andWhere(function match() { - this.whereIn('id', matchQuery); - if (matchNulls) { - this.orWhereNotIn( - 'id', - tx('entities_search') - .select('entity_id') - .where('key', keyOp, key), - ); - } - }); + // Handle absence as nulls as well + entitiesQuery = entitiesQuery.andWhere(function match() { + this.whereIn('id', matchQuery); + if (matchNulls) { + this.orWhereNotIn( + 'id', + tx('entities_search') + .select('entity_id') + .where('key', keyOp, key), + ); + } + }); + } } const rows = await entitiesQuery diff --git a/plugins/catalog-backend/src/database/search.test.ts b/plugins/catalog-backend/src/database/search.test.ts index d108820ca9..c6793cbdf4 100644 --- a/plugins/catalog-backend/src/database/search.test.ts +++ b/plugins/catalog-backend/src/database/search.test.ts @@ -24,8 +24,11 @@ describe('search', () => { const output = traverse(input); expect(output).toEqual([ { key: 'a', value: 'b' }, + { key: 'a.b', value: true }, { key: 'a', value: 'c' }, + { key: 'a.c', value: true }, { key: 'a', value: 'd' }, + { key: 'a.d', value: true }, ]); }); diff --git a/plugins/catalog-backend/src/database/search.ts b/plugins/catalog-backend/src/database/search.ts index 863b86ee9a..9e58d2468b 100644 --- a/plugins/catalog-backend/src/database/search.ts +++ b/plugins/catalog-backend/src/database/search.ts @@ -35,7 +35,7 @@ const MAX_VALUE_LENGTH = 200; type Kv = { key: string; - value: any; + value: unknown; }; // Helper for traversing through a nested structure and outputting a list of @@ -58,15 +58,17 @@ type Kv = { // "a", 1 // "b.c", null // "b.e": "f" +// "b.e.f": true // "b.e": "g" +// "b.e.g": true // "h.i": 1 // "h.j": "k" // "h.i": 2 // "h.j": "l" -export function traverse(root: any): Kv[] { +export function traverse(root: unknown): Kv[] { const output: Kv[] = []; - function visit(path: string, current: any) { + function visit(path: string, current: unknown) { if (SPECIAL_KEYS.includes(path)) { return; } @@ -89,14 +91,29 @@ export function traverse(root: any): Kv[] { // array if (Array.isArray(current)) { for (const item of current) { - // keep the same path as currently + // NOTE(freben): The reason that these are output in two different ways, + // is to support use cases where you want to express that MORE than one + // tag is present in a list. Since the EntityFilters structure is a + // record, you can't have several entries of the same key. Therefore + // you will have to match on + // + // { "a.b": ["true"], "a.c": ["true"] } + // + // rather than + // + // { "a": ["b", "c"] } + // + // because the latter means EITHER b or c has to be present. visit(path, item); + if (typeof item === 'string') { + output.push({ key: `${path}.${item}`, value: true }); + } } return; } // object - for (const [key, value] of Object.entries(current)) { + for (const [key, value] of Object.entries(current!)) { visit(path ? `${path}.${key}` : key, value); } } @@ -113,12 +130,12 @@ export function mapToRows( ): DbEntitiesSearchRow[] { const result: DbEntitiesSearchRow[] = []; - for (let { key, value } of input) { - key = key.toLowerCase(); - if (value === undefined || value === null) { + for (const { key: rawKey, value: rawValue } of input) { + const key = rawKey.toLowerCase(); + if (rawValue === undefined || rawValue === null) { result.push({ entity_id: entityId, key, value: null }); } else { - value = String(value).toLowerCase(); + const value = String(rawValue).toLowerCase(); if (value.length <= MAX_VALUE_LENGTH) { result.push({ entity_id: entityId, key, value }); } diff --git a/plugins/catalog-backend/src/database/types.ts b/plugins/catalog-backend/src/database/types.ts index 20cc123c0a..14e8d5be64 100644 --- a/plugins/catalog-backend/src/database/types.ts +++ b/plugins/catalog-backend/src/database/types.ts @@ -67,11 +67,27 @@ export type DatabaseLocationUpdateLogEvent = { message?: string; }; -export type EntityFilter = { - key: string; - values: (string | null)[]; -}; -export type EntityFilters = EntityFilter[]; +/** + * Filter matcher for a single entity field. + * + * Can be either null or a string, or an array of those. Null and the empty + * string are treated equally, and match both a present field with a null or + * empty value, as well as an absent field. + * + * A filter may contain asterisks (*) that are treated as wildcards for zero + * or more arbitrary characters. + */ +export type EntityFilter = null | string | (null | string)[]; + +/** + * A set of filter matchers used for filtering entities. + * + * The keys are full dot-separated paths into the structure of an entity, for + * example "metadata.name". You can also address any item in an array the same + * way, e.g. "a.b.c": "x" works if b is an array of objects that have a c field + * and any of those have the value x. + */ +export type EntityFilters = Record; /** * An abstraction on top of the underlying database, wrapping the basic CRUD diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts index 81753fe6d1..909139d678 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.test.ts @@ -204,14 +204,11 @@ describe('HigherOrderOperations', () => { target: 'thing', }); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenNthCalledWith( - 1, - expect.arrayContaining([ - { key: 'kind', values: ['Component'] }, - { key: 'metadata.namespace', values: [ENTITY_DEFAULT_NAMESPACE] }, - { key: 'metadata.name', values: ['c1'] }, - ]), - ); + expect(entitiesCatalog.entities).toHaveBeenNthCalledWith(1, { + kind: 'Component', + 'metadata.namespace': ENTITY_DEFAULT_NAMESPACE, + 'metadata.name': ['c1'], + }); expect(entitiesCatalog.addEntities).toHaveBeenCalledTimes(1); expect(entitiesCatalog.addEntities).toHaveBeenNthCalledWith( 1, diff --git a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts index 737166436c..fa73c58348 100644 --- a/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts +++ b/plugins/catalog-backend/src/ingestion/HigherOrderOperations.ts @@ -277,12 +277,11 @@ export class HigherOrderOperations implements HigherOrderOperation { }> { const markTimestamp = process.hrtime(); - const names = newEntities.map(e => e.metadata.name); - const oldEntities = await this.entitiesCatalog.entities([ - { key: 'kind', values: [kind] }, - { key: 'metadata.namespace', values: [namespace] }, - { key: 'metadata.name', values: names }, - ]); + const oldEntities = await this.entitiesCatalog.entities({ + kind: kind, + 'metadata.namespace': namespace, + 'metadata.name': newEntities.map(e => e.metadata.name), + }); const oldEntitiesByName = new Map( oldEntities.map(e => [e.metadata.name, e]), diff --git a/plugins/catalog-backend/src/service/router.test.ts b/plugins/catalog-backend/src/service/router.test.ts index ec5c11be33..f7ab4878f8 100644 --- a/plugins/catalog-backend/src/service/router.test.ts +++ b/plugins/catalog-backend/src/service/router.test.ts @@ -81,11 +81,11 @@ describe('createRouter', () => { expect(response.status).toEqual(200); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenCalledWith([ - { key: 'a', values: ['1', null, '3'] }, - { key: 'b', values: ['4'] }, - { key: 'c', values: [null] }, - ]); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + a: ['1', null, '3'], + b: ['4'], + c: [null], + }); }); }); @@ -103,9 +103,9 @@ describe('createRouter', () => { const response = await request(app).get('/entities/by-uid/zzz'); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenCalledWith([ - { key: 'metadata.uid', values: ['zzz'] }, - ]); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + 'metadata.uid': 'zzz', + }); expect(response.status).toEqual(200); expect(response.body).toEqual(expect.objectContaining(entity)); }); @@ -116,9 +116,9 @@ describe('createRouter', () => { const response = await request(app).get('/entities/by-uid/zzz'); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenCalledWith([ - { key: 'metadata.uid', values: ['zzz'] }, - ]); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + 'metadata.uid': 'zzz', + }); expect(response.status).toEqual(404); expect(response.text).toMatch(/uid/); }); @@ -139,11 +139,11 @@ describe('createRouter', () => { const response = await request(app).get('/entities/by-name/k/ns/n'); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenCalledWith([ - { key: 'kind', values: ['k'] }, - { key: 'metadata.namespace', values: ['ns'] }, - { key: 'metadata.name', values: ['n'] }, - ]); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + kind: 'k', + 'metadata.namespace': 'ns', + 'metadata.name': 'n', + }); expect(response.status).toEqual(200); expect(response.body).toEqual(expect.objectContaining(entity)); }); @@ -154,11 +154,11 @@ describe('createRouter', () => { const response = await request(app).get('/entities/by-name/b/d/c'); expect(entitiesCatalog.entities).toHaveBeenCalledTimes(1); - expect(entitiesCatalog.entities).toHaveBeenCalledWith([ - { key: 'kind', values: ['b'] }, - { key: 'metadata.namespace', values: ['d'] }, - { key: 'metadata.name', values: ['c'] }, - ]); + expect(entitiesCatalog.entities).toHaveBeenCalledWith({ + kind: 'b', + 'metadata.namespace': 'd', + 'metadata.name': 'c', + }); expect(response.status).toEqual(404); expect(response.text).toMatch(/name/); }); diff --git a/plugins/catalog-backend/src/service/router.ts b/plugins/catalog-backend/src/service/router.ts index 06a7ea43d3..94dc76ab9d 100644 --- a/plugins/catalog-backend/src/service/router.ts +++ b/plugins/catalog-backend/src/service/router.ts @@ -54,9 +54,9 @@ export async function createRouter( }) .get('/entities/by-uid/:uid', async (req, res) => { const { uid } = req.params; - const entities = await entitiesCatalog.entities([ - { key: 'metadata.uid', values: [uid] }, - ]); + const entities = await entitiesCatalog.entities({ + 'metadata.uid': uid, + }); if (!entities.length) { res.status(404).send(`No entity with uid ${uid}`); } @@ -69,11 +69,11 @@ export async function createRouter( }) .get('/entities/by-name/:kind/:namespace/:name', async (req, res) => { const { kind, namespace, name } = req.params; - const entities = await entitiesCatalog.entities([ - { key: 'kind', values: [kind] }, - { key: 'metadata.namespace', values: [namespace] }, - { key: 'metadata.name', values: [name] }, - ]); + const entities = await entitiesCatalog.entities({ + kind: kind, + 'metadata.namespace': namespace, + 'metadata.name': name, + }); if (!entities.length) { res .status(404) @@ -123,7 +123,7 @@ export async function createRouter( function translateQueryToEntityFilters( request: express.Request, ): EntityFilters { - const filters: EntityFilters = []; + const filters: EntityFilters = {}; for (const [key, valueOrValues] of Object.entries(request.query)) { const values = Array.isArray(valueOrValues) @@ -134,10 +134,13 @@ function translateQueryToEntityFilters( throw new InputError('Complex query parameters are not supported'); } - filters.push({ - key, - values: values.map(v => v || null) as string[], - }); + // This one always emits arrays + let matchers = filters[key] as (string | null)[]; + if (!matchers) { + matchers = []; + filters[key] = matchers; + } + matchers.push(...(values.map(v => v || null) as (string | null)[])); } return filters; From 4a867ed4a4d4b68db4b02fb9f8f8d7a2145e3515 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 12 Oct 2020 15:45:19 +0200 Subject: [PATCH 3/3] chore: fix comments --- .../src/database/CommonDatabase.ts | 106 +++++++++--------- plugins/catalog-backend/src/service/router.ts | 9 +- 2 files changed, 54 insertions(+), 61 deletions(-) diff --git a/plugins/catalog-backend/src/database/CommonDatabase.ts b/plugins/catalog-backend/src/database/CommonDatabase.ts index 563fd24c80..dca5072b30 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.ts @@ -219,64 +219,62 @@ export class CommonDatabase implements Database { let entitiesQuery = tx('entities'); - if (filters && Object.keys(filters).length) { - for (const [matchKey, matchVal] of Object.entries(filters)) { - const key = matchKey.toLowerCase().replace(/[*]/g, '%'); - const keyOp = key.includes('*') ? 'like' : '='; - const values = Array.isArray(matchVal) ? matchVal : [matchVal]; + for (const [matchKey, matchVal] of Object.entries(filters ?? {})) { + const key = matchKey.toLowerCase().replace(/[*]/g, '%'); + const keyOp = key.includes('%') ? 'like' : '='; + const values = Array.isArray(matchVal) ? matchVal : [matchVal]; - let matchNulls = false; - const matchIn: string[] = []; - const matchLike: string[] = []; + let matchNulls = false; + const matchIn: string[] = []; + const matchLike: string[] = []; - for (const value of values) { - if (!value) { - matchNulls = true; - } else if (value.includes('*')) { - matchLike.push(value.toLowerCase().replace(/[*]/g, '%')); - } else { - matchIn.push(value.toLowerCase()); - } + for (const value of values) { + if (!value) { + matchNulls = true; + } else if (value.includes('*')) { + matchLike.push(value.toLowerCase().replace(/[*]/g, '%')); + } else { + matchIn.push(value.toLowerCase()); } - - // NOTE(freben): This used to be a set of OUTER JOIN, which may seem to - // make a lot of sense. However, it had abysmal performance on sqlite - // when datasets grew large, so we're using IN instead. - const matchQuery = tx('entities_search') - .select('entity_id') - .where(function keyFilter() { - this.andWhere('key', keyOp, key); - this.andWhere(function valueFilter() { - if (matchIn.length === 1) { - this.orWhere({ value: matchIn[0] }); - } else if (matchIn.length > 1) { - this.orWhereIn('value', matchIn); - } - if (matchLike.length) { - for (const x of matchLike) { - this.orWhere('value', 'like', tx.raw('?', [x])); - } - } - if (matchNulls) { - // Match explicit nulls, and then handle absence separately below - this.orWhereNull('value'); - } - }); - }); - - // Handle absence as nulls as well - entitiesQuery = entitiesQuery.andWhere(function match() { - this.whereIn('id', matchQuery); - if (matchNulls) { - this.orWhereNotIn( - 'id', - tx('entities_search') - .select('entity_id') - .where('key', keyOp, key), - ); - } - }); } + + // NOTE(freben): This used to be a set of OUTER JOIN, which may seem to + // make a lot of sense. However, it had abysmal performance on sqlite + // when datasets grew large, so we're using IN instead. + const matchQuery = tx('entities_search') + .select('entity_id') + .where(function keyFilter() { + this.andWhere('key', keyOp, key); + this.andWhere(function valueFilter() { + if (matchIn.length === 1) { + this.orWhere({ value: matchIn[0] }); + } else if (matchIn.length > 1) { + this.orWhereIn('value', matchIn); + } + if (matchLike.length) { + for (const x of matchLike) { + this.orWhere('value', 'like', tx.raw('?', [x])); + } + } + if (matchNulls) { + // Match explicit nulls, and then handle absence separately below + this.orWhereNull('value'); + } + }); + }); + + // Handle absence as nulls as well + entitiesQuery = entitiesQuery.andWhere(function match() { + this.whereIn('id', matchQuery); + if (matchNulls) { + this.orWhereNotIn( + 'id', + tx('entities_search') + .select('entity_id') + .where('key', keyOp, key), + ); + } + }); } const rows = await entitiesQuery diff --git a/plugins/catalog-backend/src/service/router.ts b/plugins/catalog-backend/src/service/router.ts index 94dc76ab9d..a262e9b5f4 100644 --- a/plugins/catalog-backend/src/service/router.ts +++ b/plugins/catalog-backend/src/service/router.ts @@ -123,7 +123,7 @@ export async function createRouter( function translateQueryToEntityFilters( request: express.Request, ): EntityFilters { - const filters: EntityFilters = {}; + const filters: Record = {}; for (const [key, valueOrValues] of Object.entries(request.query)) { const values = Array.isArray(valueOrValues) @@ -134,12 +134,7 @@ function translateQueryToEntityFilters( throw new InputError('Complex query parameters are not supported'); } - // This one always emits arrays - let matchers = filters[key] as (string | null)[]; - if (!matchers) { - matchers = []; - filters[key] = matchers; - } + const matchers = key in filters ? filters[key] : (filters[key] = []); matchers.push(...(values.map(v => v || null) as (string | null)[])); }