Allow nested EntityFilters

This makes the format of EntityFilters more flexible, and paves the way for the permissions system, which requires composing multiple _collections_ of filters.

Signed-off-by: Joon Park <joonp@spotify.com>

.changeset/slimy-days-leave.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-catalog-backend': patch
+---
+
+Allow nested EntityFilters

plugins/catalog-backend/api-report.md

@@ -872,7 +872,7 @@ export type EntityAncestryResponse = {
 // @public
 export type EntityFilter = {
   anyOf: {
-    allOf: EntitiesSearchFilter[];
+    allOf: (EntitiesSearchFilter | EntityFilter)[];
   }[];
 };
 

plugins/catalog-backend/src/catalog/types.ts

@@ -23,7 +23,7 @@ import { Entity, EntityRelationSpec } from '@backstage/catalog-model';
  * individual filters must match.
  */
 export type EntityFilter = {
-  anyOf: { allOf: EntitiesSearchFilter[] }[];
+  anyOf: { allOf: (EntitiesSearchFilter | EntityFilter)[] }[];
 };
 
 /**

plugins/catalog-backend/src/legacy/database/CommonDatabase.ts

@@ -46,7 +46,11 @@ import {
   DbPageInfo,
   Transaction,
 } from './types';
-import { EntityPagination } from '../../catalog/types';
+import {
+  EntityPagination,
+  EntityFilter,
+  EntitiesSearchFilter,
+} from '../../catalog/types';
 
 // The number of items that are sent per batch to the database layer, when
 // doing .batchInsert calls to knex. This needs to be low enough to not cause
@@ -219,11 +223,13 @@ export class CommonDatabase implements Database {
 
     for (const singleFilter of request?.filter?.anyOf ?? []) {
       entitiesQuery = entitiesQuery.orWhere(function singleFilterFn() {
-        for (const {
-          key,
-          matchValueIn,
-          matchValueExists,
-        } of singleFilter.allOf) {
+        for (const filter of singleFilter.allOf) {
+          if (isEntityFilter(filter)) {
+            throw new Error(
+              'Nested filters are not supported in the legacy CommonDatabase',
+            );
+          }
+          const { key, matchValueIn, matchValueExists } = filter;
           // 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.
@@ -606,3 +612,9 @@ function deduplicateRelations(
     r => `${r.source_full_name}:${r.target_full_name}:${r.type}`,
   );
 }
+
+function isEntityFilter(
+  filter: EntitiesSearchFilter | EntityFilter,
+): filter is EntityFilter {
+  return filter.hasOwnProperty('anyOf');
+}

plugins/catalog-backend/src/service/NextEntitiesCatalog.test.ts

@@ -23,6 +23,7 @@ import {
   DbFinalEntitiesRow,
   DbRefreshStateReferencesRow,
   DbRefreshStateRow,
+  DbSearchRow,
 } from '../database/tables';
 import { NextEntitiesCatalog } from './NextEntitiesCatalog';
 
@@ -73,6 +74,52 @@ describe('NextEntitiesCatalog', () => {
     }
   }
 
+  async function addEntityToSearch(knex: Knex, entity: Entity) {
+    const id = uuid();
+    const entityRef = stringifyEntityRef(entity);
+    const entityJson = JSON.stringify(entity);
+
+    await knex<DbRefreshStateRow>('refresh_state').insert({
+      entity_id: id,
+      entity_ref: entityRef,
+      unprocessed_entity: entityJson,
+      errors: '[]',
+      next_update_at: '2031-01-01 23:00:00',
+      last_discovery_at: '2021-04-01 13:37:00',
+    });
+
+    await knex<DbFinalEntitiesRow>('final_entities').insert({
+      entity_id: id,
+      final_entity: entityJson,
+      hash: 'h',
+      stitch_ticket: '',
+    });
+
+    await insertSearchRow(knex, id, null, entity);
+  }
+
+  async function insertSearchRow(
+    knex: Knex,
+    id: string,
+    previousKey: string | null,
+    previousValue: Object,
+  ) {
+    return Promise.all(
+      Object.entries(previousValue).map(async ([key, value]) => {
+        const currentKey = `${previousKey ? `${previousKey}.` : ``}${key}`;
+        if (typeof value === 'object') {
+          await insertSearchRow(knex, id, currentKey, value);
+        } else {
+          await knex<DbSearchRow>('search').insert({
+            entity_id: id,
+            key: currentKey,
+            value: value,
+          });
+        }
+      }),
+    );
+  }
+
   describe('entityAncestry', () => {
     it.each(databases.eachSupportedId())(
       'should return the ancestry with one parent, %p',
@@ -209,4 +256,112 @@ describe('NextEntitiesCatalog', () => {
       60_000,
     );
   });
+
+  describe('entities', () => {
+    it.each(databases.eachSupportedId())(
+      'should return correct entity for simple filter',
+      async databaseId => {
+        const { knex } = await createDatabase(databaseId);
+        const entity1: Entity = {
+          apiVersion: 'a',
+          kind: 'k',
+          metadata: { name: 'one' },
+          spec: {},
+        };
+        const entity2: Entity = {
+          apiVersion: 'a',
+          kind: 'k',
+          metadata: { name: 'two' },
+          spec: {
+            test: 'test value',
+          },
+        };
+        await addEntityToSearch(knex, entity1);
+        await addEntityToSearch(knex, entity2);
+        const catalog = new NextEntitiesCatalog(knex);
+
+        const testFilter = {
+          key: 'spec.test',
+          matchValueExists: true,
+        };
+        const request = {
+          filter: { anyOf: [{ allOf: [testFilter] }] },
+        };
+        const { entities } = await catalog.entities(request);
+
+        expect(entities.length).toBe(1);
+        expect(entities[0]).toEqual(entity2);
+      },
+    );
+
+    it.each(databases.eachSupportedId())(
+      'should return correct entity for nested filter',
+      async databaseId => {
+        const { knex } = await createDatabase(databaseId);
+        const entity1: Entity = {
+          apiVersion: 'a',
+          kind: 'k',
+          metadata: { name: 'one', org: 'a', desc: 'description' },
+          spec: {},
+        };
+        const entity2: Entity = {
+          apiVersion: 'a',
+          kind: 'k',
+          metadata: { name: 'two', org: 'b', desc: 'description' },
+          spec: {},
+        };
+        const entity3: Entity = {
+          apiVersion: 'a',
+          kind: 'k',
+          metadata: { name: 'three', org: 'b', color: 'red' },
+          spec: {},
+        };
+        const entity4: Entity = {
+          apiVersion: 'a',
+          kind: 'k',
+          metadata: { name: 'four', org: 'b', color: 'blue' },
+          spec: {},
+        };
+        await addEntityToSearch(knex, entity1);
+        await addEntityToSearch(knex, entity2);
+        await addEntityToSearch(knex, entity3);
+        await addEntityToSearch(knex, entity4);
+        const catalog = new NextEntitiesCatalog(knex);
+
+        const testFilter1 = {
+          key: 'metadata.org',
+          matchValueExists: true,
+          matchValueIn: ['b'],
+        };
+        const testFilter2 = {
+          key: 'metadata.desc',
+          matchValueExists: true,
+        };
+        const testFilter3 = {
+          key: 'metadata.color',
+          matchValueExists: true,
+          matchValueIn: ['blue'],
+        };
+        const request = {
+          filter: {
+            anyOf: [
+              {
+                allOf: [
+                  testFilter1,
+                  {
+                    anyOf: [{ allOf: [testFilter2] }, { allOf: [testFilter3] }],
+                  },
+                ],
+              },
+            ],
+          },
+        };
+        const { entities } = await catalog.entities(request);
+
+        expect(entities.length).toBe(2);
+        expect(entities).toContainEqual(entity2);
+        expect(entities).toContainEqual(entity4);
+      },
+    );
+  });
 });

plugins/catalog-backend/src/service/NextEntitiesCatalog.ts

@@ -23,6 +23,8 @@ import {
   EntitiesResponse,
   EntityAncestryResponse,
   EntityPagination,
+  EntityFilter,
+  EntitiesSearchFilter,
 } from '../catalog/types';
 import {
   DbFinalEntitiesRow,
@@ -73,6 +75,64 @@ function stringifyPagination(input: { limit: number; offset: number }) {
   return base64;
 }
 
+function addCondition(
+  queryBuilder: Knex.QueryBuilder,
+  db: Knex,
+  { key, matchValueIn, matchValueExists }: EntitiesSearchFilter,
+) {
+  // 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 = db<DbSearchRow>('search')
+    .select('entity_id')
+    .where(function keyFilter() {
+      this.andWhere({ key: key.toLowerCase() });
+      if (matchValueExists !== false && matchValueIn) {
+        if (matchValueIn.length === 1) {
+          this.andWhere({ value: matchValueIn[0].toLowerCase() });
+        } else if (matchValueIn.length > 1) {
+          this.andWhere(
+            'value',
+            'in',
+            matchValueIn.map(v => v.toLowerCase()),
+          );
+        }
+      }
+    });
+  // Explicitly evaluate matchValueExists as a boolean since it may be undefined
+  queryBuilder.andWhere(
+    'entity_id',
+    matchValueExists === false ? 'not in' : 'in',
+    matchQuery,
+  );
+}
+
+function isEntityFilter(
+  filter: EntitiesSearchFilter | EntityFilter,
+): filter is EntityFilter {
+  return filter.hasOwnProperty('anyOf');
+}
+
+function parseFilter(
+  filters: EntityFilter,
+  query: Knex.QueryBuilder,
+  db: Knex,
+): Knex.QueryBuilder {
+  let cumulativeQuery = query;
+  for (const singleFilter of filters?.anyOf ?? []) {
+    cumulativeQuery = cumulativeQuery.orWhere(function singleFilterFn() {
+      for (const filter of singleFilter.allOf) {
+        if (isEntityFilter(filter)) {
+          this.andWhere(subQuery => parseFilter(filter, subQuery, db));
+        } else {
+          addCondition(this, db, filter);
+        }
+      }
+    });
+  }
+  return cumulativeQuery;
+}
+
 export class NextEntitiesCatalog implements EntitiesCatalog {
   constructor(private readonly database: Knex) {}
 
@@ -80,41 +140,8 @@ export class NextEntitiesCatalog implements EntitiesCatalog {
     const db = this.database;
 
     let entitiesQuery = db<DbFinalEntitiesRow>('final_entities');
-
-    for (const singleFilter of request?.filter?.anyOf ?? []) {
-      entitiesQuery = entitiesQuery.orWhere(function singleFilterFn() {
-        for (const {
-          key,
-          matchValueIn,
-          matchValueExists,
-        } of singleFilter.allOf) {
-          // 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 = db<DbSearchRow>('search')
-            .select('entity_id')
-            .where(function keyFilter() {
-              this.andWhere({ key: key.toLowerCase() });
-              if (matchValueExists !== false && matchValueIn) {
-                if (matchValueIn.length === 1) {
-                  this.andWhere({ value: matchValueIn[0].toLowerCase() });
-                } else if (matchValueIn.length > 1) {
-                  this.andWhere(
-                    'value',
-                    'in',
-                    matchValueIn.map(v => v.toLowerCase()),
-                  );
-                }
-              }
-            });
-          // Explicitly evaluate matchValueExists as a boolean since it may be undefined
-          this.andWhere(
-            'entity_id',
-            matchValueExists === false ? 'not in' : 'in',
-            matchQuery,
-          );
-        }
-      });
+    if (request?.filter) {
+      entitiesQuery = parseFilter(request.filter, entitiesQuery, db);
     }
 
     // TODO: move final_entities to use entity_ref