From 154c77bf7f02b11c3480fcb1194902d8495073a1 Mon Sep 17 00:00:00 2001
From: Joon Park <joonp@spotify.com>
Date: Wed, 27 Oct 2021 13:09:04 +0100
Subject: [PATCH] EntitiesSearchFilter type refactor

The current EntitiesSearchFilter's matchValueExists field is a bit confusing - it's not immediately clear the nuances of its behavior until you closely examine the implementation in NextEntitiesCatalog. By splitting the filter into to distinct types (one for key and another for key + value) as well as renaming the fields, I think this expresses the intent of the filters more cleanly. This also allows filtering by negation for key + value filters, which is not possible with the current implementation).

Signed-off-by: Joon Park <joonp@spotify.com>
---
 plugins/catalog-backend/src/catalog/index.ts  |  2 ++
 plugins/catalog-backend/src/catalog/types.ts  | 24 +++++++++++++------
 .../src/service/request/basicEntityFilter.ts  |  8 +++----
 .../request/parseEntityFilterParams.ts        | 15 +++++++-----
 4 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/plugins/catalog-backend/src/catalog/index.ts b/plugins/catalog-backend/src/catalog/index.ts
index 885a2f5389..460aefddba 100644
--- a/plugins/catalog-backend/src/catalog/index.ts
+++ b/plugins/catalog-backend/src/catalog/index.ts
@@ -23,6 +23,8 @@ export type {
   EntityUpsertResponse,
   PageInfo,
   EntitiesSearchFilter,
+  EntitiesKeyFilter,
+  EntitiesValuesFilter,
   EntityFilter,
   EntityPagination,
 } from './types';
diff --git a/plugins/catalog-backend/src/catalog/types.ts b/plugins/catalog-backend/src/catalog/types.ts
index df693bfd42..3f74f2ee98 100644
--- a/plugins/catalog-backend/src/catalog/types.ts
+++ b/plugins/catalog-backend/src/catalog/types.ts
@@ -39,7 +39,7 @@ export type EntityPagination = {
 /**
  * Matches rows in the entities_search table.
  */
-export type EntitiesSearchFilter = {
+export type EntitiesValuesFilter = {
   /**
    * The key to match on.
    *
@@ -50,18 +50,28 @@ export type EntitiesSearchFilter = {
   /**
    * Match on plain equality of values.
    *
-   * If undefined, this factor is not taken into account. Otherwise, match on
+   * Match on
    * values that are equal to any of the given array items. Matches are always
    * case insensitive.
    */
-  matchValueIn?: string[];
+  values: string[];
 
-  /**
-   * Match on existence of key.
-   */
-  matchValueExists?: boolean;
+  negate?: boolean;
 };
 
+export type EntitiesKeyFilter = {
+  /**
+   * The key to match on.
+   *
+   * Matches are always case insensitive.
+   */
+  key: string;
+
+  negate?: boolean;
+};
+
+export type EntitiesSearchFilter = EntitiesValuesFilter | EntitiesKeyFilter;
+
 export type PageInfo =
   | {
       hasNextPage: false;
diff --git a/plugins/catalog-backend/src/service/request/basicEntityFilter.ts b/plugins/catalog-backend/src/service/request/basicEntityFilter.ts
index 1cf9ca95f0..cdc736e232 100644
--- a/plugins/catalog-backend/src/service/request/basicEntityFilter.ts
+++ b/plugins/catalog-backend/src/service/request/basicEntityFilter.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { EntitiesSearchFilter, EntityFilter } from '../../catalog';
+import { EntitiesValuesFilter, EntityFilter } from '../../catalog';
 
 /**
  * Forms a full EntityFilter based on a single key-value(s) object.
@@ -22,7 +22,7 @@ import { EntitiesSearchFilter, EntityFilter } from '../../catalog';
 export function basicEntityFilter(
   items: Record<string, string | string[]>,
 ): EntityFilter {
-  const filtersByKey: Record<string, EntitiesSearchFilter> = {};
+  const filtersByKey: Record<string, EntitiesValuesFilter> = {};
 
   for (const [key, value] of Object.entries(items)) {
     const values = [value].flat();
@@ -30,9 +30,9 @@ export function basicEntityFilter(
     const f =
       key in filtersByKey
         ? filtersByKey[key]
-        : (filtersByKey[key] = { key, matchValueIn: [] });
+        : (filtersByKey[key] = { key, values: [] });
 
-    f.matchValueIn!.push(...values);
+    f.values!.push(...values);
   }
 
   return { anyOf: [{ allOf: Object.values(filtersByKey) }] };
diff --git a/plugins/catalog-backend/src/service/request/parseEntityFilterParams.ts b/plugins/catalog-backend/src/service/request/parseEntityFilterParams.ts
index 452958b7ae..e779c193cb 100644
--- a/plugins/catalog-backend/src/service/request/parseEntityFilterParams.ts
+++ b/plugins/catalog-backend/src/service/request/parseEntityFilterParams.ts
@@ -15,7 +15,11 @@
  */
 
 import { InputError } from '@backstage/errors';
-import { EntitiesSearchFilter, EntityFilter } from '../../catalog';
+import {
+  EntitiesSearchFilter,
+  EntitiesValuesFilter,
+  EntityFilter,
+} from '../../catalog';
 import { parseStringsParam } from './common';
 
 /**
@@ -75,11 +79,10 @@ export function parseEntityFilterString(
     const f =
       key in filtersByKey ? filtersByKey[key] : (filtersByKey[key] = { key });
 
-    if (value === undefined) {
-      f.matchValueExists = true;
-    } else {
-      f.matchValueIn = f.matchValueIn || [];
-      f.matchValueIn.push(value);
+    if (value !== undefined) {
+      const valuesFilter = f as EntitiesValuesFilter;
+      valuesFilter.values = valuesFilter.values || [];
+      valuesFilter.values.push(value);
     }
   }