paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 48 Packages Projects Releases Wiki Activity

Merge pull request #7817 from backstage/joonp/entitiessearchfilter-refactor

Browse Source 
[Mini PRFC] EntitiesSearchFilter type refactor
This commit is contained in:
Fredrik Adelöw
2021-11-22 13:48:38 +01:00
committed by GitHub
parent 3487a35ee4 7f82ce9f51
commit cd3aac016f
12 changed files with 201 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.changeset/smart-fans-complain.md
+42
View File
@@ -0,0 +1,42 @@
---
'@backstage/plugin-catalog-backend': minor
---
**BREAKING** EntitiesSearchFilter fields have changed.
EntitiesSearchFilter now has only two fields: `key` and `value`. The `matchValueIn` and `matchValueExists` fields are no longer are supported. Previous filters written using the `matchValueIn` and `matchValueExists` fields can be rewritten as follows:
Filtering by existence of key only:
```diff
filter: {
{
key: 'abc',
- matchValueExists: true,
},
}
```
Filtering by key and values:
```diff
filter: {
{
key: 'abc',
- matchValueExists: true,
- matchValueIn: ['xyz'],
+ values: ['xyz'],
},
}
```
Negation of filters can now be achieved through a `not` object:
```
filter: {
not: {
key: 'abc',
values: ['xyz'],
},
}
```
plugins/catalog-backend/api-report.md
+7 -5
View File
@@ -846,8 +846,7 @@ export type EntitiesResponse = {
// @public
export type EntitiesSearchFilter = {
key: string;
matchValueIn?: string[];
matchValueExists?: boolean;
values?: string[];
};
// Warning: (ae-missing-release-tag) "entity" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
@@ -877,6 +876,9 @@ export type EntityFilter =
| {
anyOf: EntityFilter[];
}
| {
not: EntityFilter;
}
| EntitiesSearchFilter;
// Warning: (ae-missing-release-tag) "EntityPagination" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
@@ -1546,9 +1548,9 @@ export class UrlReaderProcessor implements CatalogProcessor {
// Warnings were encountered during analysis:
//
// src/catalog/types.d.ts:97:8 - (tsdoc-param-tag-with-invalid-name) The @param block should be followed by a valid parameter name: The identifier cannot non-word characters
// src/catalog/types.d.ts:98:8 - (tsdoc-param-tag-with-invalid-name) The @param block should be followed by a valid parameter name: The identifier cannot non-word characters
// src/catalog/types.d.ts:99:8 - (tsdoc-param-tag-with-invalid-name) The @param block should be followed by a valid parameter name: The identifier cannot non-word characters
// src/catalog/types.d.ts:94:8 - (tsdoc-param-tag-with-invalid-name) The @param block should be followed by a valid parameter name: The identifier cannot non-word characters
// src/catalog/types.d.ts:95:8 - (tsdoc-param-tag-with-invalid-name) The @param block should be followed by a valid parameter name: The identifier cannot non-word characters
// src/catalog/types.d.ts:96:8 - (tsdoc-param-tag-with-invalid-name) The @param block should be followed by a valid parameter name: The identifier cannot non-word characters
// src/ingestion/processors/GithubMultiOrgReaderProcessor.d.ts:23:9 - (ae-forgotten-export) The symbol "GithubMultiOrgConfig" needs to be exported by the entry point index.d.ts
// src/ingestion/types.d.ts:8:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/legacy/database/types.d.ts:98:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
plugins/catalog-backend/src/catalog/types.ts
+4 -9
View File
@@ -25,6 +25,7 @@ import { Entity, EntityRelationSpec } from '@backstage/catalog-model';
export type EntityFilter =
| { allOf: EntityFilter[] }
| { anyOf: EntityFilter[] }
| { not: EntityFilter }
| EntitiesSearchFilter;
/**
@@ -50,16 +51,10 @@ export type EntitiesSearchFilter = {
/**
* Match on plain equality of values.
*
* If undefined, this factor is not taken into account. Otherwise, match on
* values that are equal to any of the given array items. Matches are always
* case insensitive.
* Match on values that are equal to any of the given array items. Matches are
* always case insensitive.
*/
matchValueIn?: string[];
/**
* Match on existence of key.
*/
matchValueExists?: boolean;
values?: string[];
};
export type PageInfo =
plugins/catalog-backend/src/legacy/database/CommonDatabase.test.ts
+1 -27
View File
@@ -536,9 +536,7 @@ describe('CommonDatabase', () => {
filter: {
anyOf: [
{
allOf: [
{ key: 'metadata.annotations.foo', matchValueExists: true },
],
allOf: [{ key: 'metadata.annotations.foo' }],
},
],
},
@@ -558,30 +556,6 @@ describe('CommonDatabase', () => {
},
]),
);
const nonExistRows = await db.transaction(async tx =>
db.entities(tx, {
filter: {
anyOf: [
{
allOf: [
{ key: 'metadata.annotations.foo', matchValueExists: false },
],
},
],
},
}),
);
expect(nonExistRows.entities.length).toEqual(1);
expect(nonExistRows.entities).toEqual(
expect.arrayContaining([
{
locationId: undefined,
entity: expect.objectContaining({ kind: 'k3' }),
},
]),
);
});
});
plugins/catalog-backend/src/legacy/database/CommonDatabase.ts
+11 -14
View File
@@ -224,7 +224,8 @@ export class CommonDatabase implements Database {
if (
request?.filter &&
(request.filter.hasOwnProperty('key') ||
request.filter.hasOwnProperty('allOf'))
request.filter.hasOwnProperty('allOf') ||
request.filter.hasOwnProperty('not'))
) {
throw new Error(
'Filters for the legacy CommonDatabase must obey the { anyOf: [{ allOf: [] }] } format.',
@@ -236,13 +237,14 @@ export class CommonDatabase implements Database {
for (const filter of singleFilter.allOf) {
if (
filter.hasOwnProperty('anyOf') ||
filter.hasOwnProperty('allOf')
filter.hasOwnProperty('allOf') ||
filter.hasOwnProperty('not')
) {
throw new Error(
'Nested filters are not supported in the legacy CommonDatabase',
);
}
const { key, matchValueIn, matchValueExists } = filter;
const { key, values } = 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.
@@ -250,24 +252,19 @@ export class CommonDatabase implements Database {
.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) {
if (values) {
if (values.length === 1) {
this.andWhere({ value: values[0].toLowerCase() });
} else if (values.length > 1) {
this.andWhere(
'value',
'in',
matchValueIn.map(v => v.toLowerCase()),
values.map(v => v.toLowerCase()),
);
}
}
});
// Explicitly evaluate matchValueExists as a boolean since it may be undefined
this.andWhere(
'id',
matchValueExists === false ? 'not in' : 'in',
matchQuery,
);
this.andWhere('id', 'in', matchQuery);
}
});
}
plugins/catalog-backend/src/legacy/service/router.test.ts
+3 -3
View File
@@ -115,11 +115,11 @@ describe('createRouter readonly disabled', () => {
anyOf: [
{
allOf: [
{ key: 'a', matchValueIn: ['1', '2'] },
{ key: 'b', matchValueIn: ['3'] },
{ key: 'a', values: ['1', '2'] },
{ key: 'b', values: ['3'] },
],
},
{ allOf: [{ key: 'c', matchValueIn: ['4'] }] },
{ allOf: [{ key: 'c', values: ['4'] }] },
],
},
});
plugins/catalog-backend/src/service/NextEntitiesCatalog.test.ts
+86 -8
View File
@@ -282,7 +282,6 @@ describe('NextEntitiesCatalog', () => {
const testFilter = {
key: 'spec.test',
matchValueExists: true,
};
const request = { filter: testFilter };
const { entities } = await catalog.entities(request);
@@ -293,7 +292,42 @@ describe('NextEntitiesCatalog', () => {
);
it.each(databases.eachSupportedId())(
'should return correct entity for nested filter',
'should return correct entity for negation 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 = {
not: {
key: 'spec.test',
},
};
const request = { filter: testFilter };
const { entities } = await catalog.entities(request);
expect(entities.length).toBe(1);
expect(entities[0]).toEqual(entity1);
},
);
it.each(databases.eachSupportedId())(
'should return correct entities for nested filter',
async databaseId => {
const { knex } = await createDatabase(databaseId);
const entity1: Entity = {
@@ -328,24 +362,27 @@ describe('NextEntitiesCatalog', () => {
const testFilter1 = {
key: 'metadata.org',
matchValueExists: true,
matchValueIn: ['b'],
values: ['b'],
};
const testFilter2 = {
key: 'metadata.desc',
matchValueExists: true,
};
const testFilter3 = {
key: 'metadata.color',
matchValueExists: true,
matchValueIn: ['blue'],
values: ['blue'],
};
const testFilter4 = {
not: {
key: 'metadata.color',
values: ['red'],
},
};
const request = {
filter: {
allOf: [
testFilter1,
{
anyOf: [testFilter2, testFilter3],
anyOf: [testFilter2, testFilter3, testFilter4],
},
],
},
@@ -357,5 +394,46 @@ describe('NextEntitiesCatalog', () => {
expect(entities).toContainEqual(entity4);
},
);
it.each(databases.eachSupportedId())(
'should return correct entities for complex negation 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: {},
};
await addEntityToSearch(knex, entity1);
await addEntityToSearch(knex, entity2);
const catalog = new NextEntitiesCatalog(knex);
const testFilter1 = {
key: 'metadata.org',
values: ['b'],
};
const testFilter2 = {
key: 'metadata.desc',
};
const request = {
filter: {
not: {
allOf: [testFilter1, testFilter2],
},
},
};
const { entities } = await catalog.entities(request);
expect(entities.length).toBe(1);
expect(entities).toContainEqual(entity1);
},
);
});
});
plugins/catalog-backend/src/service/NextEntitiesCatalog.ts
+27 -32
View File
@@ -78,33 +78,29 @@ function stringifyPagination(input: { limit: number; offset: number }) {
function addCondition(
queryBuilder: Knex.QueryBuilder,
db: Knex,
{ key, matchValueIn, matchValueExists }: EntitiesSearchFilter,
filter: EntitiesSearchFilter,
negate: boolean = false,
) {
// 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) {
.where({ key: filter.key.toLowerCase() })
.andWhere(function keyFilter() {
if (filter.values) {
if (filter.values.length === 1) {
this.where({ value: filter.values[0].toLowerCase() });
} else if (filter.values.length > 1) {
this.andWhere(
'value',
'in',
matchValueIn.map(v => v.toLowerCase()),
filter.values.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,
);
queryBuilder.andWhere('entity_id', negate ? 'not in' : 'in', matchQuery);
}
function isEntitiesSearchFilter(
@@ -113,46 +109,45 @@ function isEntitiesSearchFilter(
return filter.hasOwnProperty('key');
}
function isAndEntityFilter(
filter: { allOf: EntityFilter[] } | EntityFilter,
): filter is { allOf: EntityFilter[] } {
return filter.hasOwnProperty('allOf');
}
function isOrEntityFilter(
filter: { anyOf: EntityFilter[] } | EntityFilter,
): filter is { anyOf: EntityFilter[] } {
return filter.hasOwnProperty('anyOf');
}
function isNegationEntityFilter(
filter: { not: EntityFilter } | EntityFilter,
): filter is { not: EntityFilter } {
return filter.hasOwnProperty('not');
}
function parseFilter(
filter: EntityFilter,
query: Knex.QueryBuilder,
db: Knex,
negate: boolean = false,
): Knex.QueryBuilder {
if (isEntitiesSearchFilter(filter)) {
return query.andWhere(function filterFunction() {
addCondition(this, db, filter);
addCondition(this, db, filter, negate);
});
}
if (isOrEntityFilter(filter)) {
return query.andWhere(function filterFunction() {
if (isNegationEntityFilter(filter)) {
return parseFilter(filter.not, query, db, !negate);
}
return query[negate ? 'andWhereNot' : 'andWhere'](function filterFunction() {
if (isOrEntityFilter(filter)) {
for (const subFilter of filter.anyOf ?? []) {
this.orWhere(subQuery => parseFilter(subFilter, subQuery, db));
}
});
}
if (isAndEntityFilter(filter)) {
return query.andWhere(function filterFunction() {
} else {
for (const subFilter of filter.allOf ?? []) {
this.andWhere(subQuery => parseFilter(subFilter, subQuery, db));
}
});
}
return query;
}
});
}
export class NextEntitiesCatalog implements EntitiesCatalog {
plugins/catalog-backend/src/service/NextRouter.test.ts
+3 -3
View File
@@ -104,11 +104,11 @@ describe('createNextRouter readonly disabled', () => {
anyOf: [
{
allOf: [
{ key: 'a', matchValueIn: ['1', '2'] },
{ key: 'b', matchValueIn: ['3'] },
{ key: 'a', values: ['1', '2'] },
{ key: 'b', values: ['3'] },
],
},
{ allOf: [{ key: 'c', matchValueIn: ['4'] }] },
{ allOf: [{ key: 'c', values: ['4'] }] },
],
},
});
plugins/catalog-backend/src/service/request/basicEntityFilter.ts
+2 -2
View File
@@ -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) }] };
plugins/catalog-backend/src/service/request/parseEntityFilterParams.test.ts
+12 -12
View File
@@ -28,7 +28,7 @@ describe('parseEntityFilterParams', () => {
it('supports single-string format', () => {
const result = parseEntityFilterParams({ filter: 'a=1' })!;
expect(result).toEqual({
anyOf: [{ allOf: [{ key: 'a', matchValueIn: ['1'] }] }],
anyOf: [{ allOf: [{ key: 'a', values: ['1'] }] }],
});
});
@@ -38,8 +38,8 @@ describe('parseEntityFilterParams', () => {
});
expect(result).toEqual({
anyOf: [
{ allOf: [{ key: 'a', matchValueIn: ['1'] }] },
{ allOf: [{ key: 'b', matchValueIn: ['2'] }] },
{ allOf: [{ key: 'a', values: ['1'] }] },
{ allOf: [{ key: 'b', values: ['2'] }] },
],
});
});
@@ -50,11 +50,11 @@ describe('parseEntityFilterParams', () => {
});
expect(result).toEqual({
anyOf: [
{ allOf: [{ key: 'a', matchValueIn: ['1'] }] },
{ allOf: [{ key: 'a', values: ['1'] }] },
{
allOf: [
{ key: 'b', matchValueIn: ['2', '3'] },
{ key: 'c', matchValueIn: ['4'] },
{ key: 'b', values: ['2', '3'] },
{ key: 'c', values: ['4'] },
],
},
],
@@ -70,17 +70,17 @@ describe('parseEntityFilterString', () => {
it('works for the happy path', () => {
expect(parseEntityFilterString('')).toBeUndefined();
expect(parseEntityFilterString('a=1,b=2,a=3,c,d=')).toEqual([
{ key: 'a', matchValueIn: ['1', '3'] },
{ key: 'b', matchValueIn: ['2'] },
{ key: 'c', matchValueExists: true },
{ key: 'd', matchValueIn: [''] },
{ key: 'a', values: ['1', '3'] },
{ key: 'b', values: ['2'] },
{ key: 'c' },
{ key: 'd', values: [''] },
]);
});
it('trims values', () => {
expect(parseEntityFilterString(' a = 1 , b = 2 , a = 3 ')).toEqual([
{ key: 'a', matchValueIn: ['1', '3'] },
{ key: 'b', matchValueIn: ['2'] },
{ key: 'a', values: ['1', '3'] },
{ key: 'b', values: ['2'] },
]);
});
plugins/catalog-backend/src/service/request/parseEntityFilterParams.ts
+3 -5
View File
@@ -75,11 +75,9 @@ 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) {
f.values = f.values || [];
f.values.push(value);
}
}