From 3bff1d6a683ed22330e49869cff295dfa973df21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 18 Jun 2020 11:54:12 +0200 Subject: [PATCH] address comments --- packages/catalog-model/package.json | 1 + packages/catalog-model/src/entity/Entity.ts | 11 +- packages/catalog-model/src/entity/index.ts | 1 + packages/config-loader/src/lib/reader.ts | 15 ++- packages/config-loader/src/lib/secrets.ts | 3 +- packages/config/src/types.ts | 2 +- .../src/catalog/DatabaseEntitiesCatalog.ts | 2 +- .../src/database/CommonDatabase.ts | 114 ++++++++---------- .../RegisterComponentResultDialog.test.tsx | 13 +- 9 files changed, 83 insertions(+), 79 deletions(-) diff --git a/packages/catalog-model/package.json b/packages/catalog-model/package.json index a9bece27ab..2516263e8a 100644 --- a/packages/catalog-model/package.json +++ b/packages/catalog-model/package.json @@ -20,6 +20,7 @@ "clean": "backstage-cli clean" }, "dependencies": { + "@backstage/config": "^0.1.1-alpha.9", "@types/yup": "^0.28.2", "lodash": "^4.17.15", "uuid": "^8.0.0", diff --git a/packages/catalog-model/src/entity/Entity.ts b/packages/catalog-model/src/entity/Entity.ts index d4f10ab883..4f245da8b7 100644 --- a/packages/catalog-model/src/entity/Entity.ts +++ b/packages/catalog-model/src/entity/Entity.ts @@ -14,6 +14,8 @@ * limitations under the License. */ +import { JsonObject } from '@backstage/config'; + /** * The format envelope that's common to all versions/kinds of entity. * @@ -39,7 +41,7 @@ export type Entity = { /** * The specification data describing the entity itself. */ - spec?: object; + spec?: JsonObject; }; /** @@ -48,7 +50,7 @@ export type Entity = { * @see https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#objectmeta-v1-meta * @see https://kubernetes.io/docs/concepts/overview/working-with-objects/kubernetes-objects/ */ -export type EntityMeta = Record & { +export type EntityMeta = JsonObject & { /** * A globally unique ID for the entity. * @@ -112,3 +114,8 @@ export type EntityMeta = Record & { */ annotations?: Record; }; + +/** + * The keys of EntityMeta that are auto-generated. + */ +export const entityMetaGeneratedFields = ['uid', 'etag', 'generation'] as const; diff --git a/packages/catalog-model/src/entity/index.ts b/packages/catalog-model/src/entity/index.ts index a1aed856ff..380f5458cc 100644 --- a/packages/catalog-model/src/entity/index.ts +++ b/packages/catalog-model/src/entity/index.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +export { entityMetaGeneratedFields } from './Entity'; export type { Entity, EntityMeta } from './Entity'; export * from './policies'; export { diff --git a/packages/config-loader/src/lib/reader.ts b/packages/config-loader/src/lib/reader.ts index 4efaf1986a..584d0e0591 100644 --- a/packages/config-loader/src/lib/reader.ts +++ b/packages/config-loader/src/lib/reader.ts @@ -14,11 +14,11 @@ * limitations under the License. */ -import yaml from 'yaml'; +import { AppConfig, JsonObject, JsonValue } from '@backstage/config'; import { basename } from 'path'; -import { isObject } from './utils'; -import { JsonValue, JsonObject, AppConfig } from '@backstage/config'; +import yaml from 'yaml'; import { ReaderContext } from './types'; +import { isObject } from './utils'; /** * Reads and parses, and validates, and transforms a single config file. @@ -67,9 +67,12 @@ export async function readConfigFile( const out: JsonObject = {}; for (const [key, value] of Object.entries(obj)) { - const result = await transform(value, `${path}.${key}`); - if (result !== undefined) { - out[key] = result; + // undefined covers optional fields + if (value !== undefined) { + const result = await transform(value, `${path}.${key}`); + if (result !== undefined) { + out[key] = result; + } } } diff --git a/packages/config-loader/src/lib/secrets.ts b/packages/config-loader/src/lib/secrets.ts index c4157120ae..348a41db58 100644 --- a/packages/config-loader/src/lib/secrets.ts +++ b/packages/config-loader/src/lib/secrets.ts @@ -122,7 +122,7 @@ export async function readSecret( const { path } = secret; const parts = typeof path === 'string' ? path.split('.') : path; - let value: JsonValue = await parser(content); + let value: JsonValue | undefined = await parser(content); for (const [index, part] of parts.entries()) { if (!isObject(value)) { const errPath = parts.slice(0, index).join('.'); @@ -132,6 +132,7 @@ export async function readSecret( } value = value[part]; } + return String(value); } diff --git a/packages/config/src/types.ts b/packages/config/src/types.ts index aaadcd70dc..3e46874c1d 100644 --- a/packages/config/src/types.ts +++ b/packages/config/src/types.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -export type JsonObject = { [key in string]: JsonValue }; +export type JsonObject = { [key in string]?: JsonValue }; export type JsonArray = JsonValue[]; export type JsonValue = | JsonObject diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts index c7cc4249db..d6809e468f 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts @@ -49,7 +49,7 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { const response = await this.database.transaction(tx => this.entityByNameInternal(tx, kind, name, namespace), ); - return response ? response.entity : undefined; + return response?.entity; } async addOrUpdateEntity( diff --git a/plugins/catalog-backend/src/database/CommonDatabase.ts b/plugins/catalog-backend/src/database/CommonDatabase.ts index e986d25c34..c860f52d1e 100644 --- a/plugins/catalog-backend/src/database/CommonDatabase.ts +++ b/plugins/catalog-backend/src/database/CommonDatabase.ts @@ -22,6 +22,7 @@ import { import { Entity, EntityMeta, + entityMetaGeneratedFields, generateEntityEtag, generateEntityUid, Location, @@ -44,63 +45,9 @@ import type { EntityFilters, } from './types'; -function serializeMetadata(metadata: EntityMeta): string { - const withoutGeneratedFields = lodash.cloneDeep(metadata); - delete withoutGeneratedFields.uid; - delete withoutGeneratedFields.etag; - delete withoutGeneratedFields.generation; - return JSON.stringify(withoutGeneratedFields); -} - -function serializeSpec(spec: Entity['spec']): DbEntitiesRow['spec'] { - if (!spec) { - return null; - } - - return JSON.stringify(spec); -} - -function toEntityRow( - locationId: string | undefined, - entity: Entity, -): DbEntitiesRow { - return { - id: entity.metadata.uid!, - location_id: locationId || null, - etag: entity.metadata.etag!, - generation: entity.metadata.generation!, - api_version: entity.apiVersion, - kind: entity.kind, - name: entity.metadata.name, - namespace: entity.metadata.namespace || null, - metadata: serializeMetadata(entity.metadata), - spec: serializeSpec(entity.spec), - }; -} - -function toEntityResponse(row: DbEntitiesRow): DbEntityResponse { - const entity: Entity = { - apiVersion: row.api_version, - kind: row.kind, - metadata: { - ...(JSON.parse(row.metadata) as Entity['metadata']), - uid: row.id, - etag: row.etag, - generation: Number(row.generation), // cast because of sqlite - }, - }; - - if (row.spec) { - const spec = JSON.parse(row.spec); - entity.spec = spec; - } - - return { - locationId: row.location_id || undefined, - entity, - }; -} - +/** + * The core database implementation. + */ export class CommonDatabase implements Database { constructor( private readonly database: Knex, @@ -149,7 +96,7 @@ export class CommonDatabase implements Database { generation: 1, }; - const newRow = toEntityRow(request.locationId, newEntity); + const newRow = this.toEntityRow(request.locationId, newEntity); await tx('entities').insert(newRow); await this.updateEntitiesSearch(tx, newRow.id, newEntity); @@ -202,7 +149,7 @@ export class CommonDatabase implements Database { // Store the updated entity; select on the old etag to ensure that we do // not lose to another writer - const newRow = toEntityRow(request.locationId, request.entity); + const newRow = this.toEntityRow(request.locationId, request.entity); const updatedRows = await tx('entities') .where({ id: oldRow.id, etag: oldRow.etag }) .update(newRow); @@ -270,7 +217,7 @@ export class CommonDatabase implements Database { .orderBy('name', 'asc') .groupBy('id'); - return rows.map(row => toEntityResponse(row)); + return rows.map(row => this.toEntityResponse(row)); } async entity( @@ -289,7 +236,7 @@ export class CommonDatabase implements Database { return undefined; } - return toEntityResponse(rows[0]); + return this.toEntityResponse(rows[0]); } async entityByUid( @@ -304,7 +251,7 @@ export class CommonDatabase implements Database { return undefined; } - return toEntityResponse(rows[0]); + return this.toEntityResponse(rows[0]); } async removeEntity(txOpaque: unknown, uid: string): Promise { @@ -466,4 +413,47 @@ export class CommonDatabase implements Database { } } } + + private toEntityRow( + locationId: string | undefined, + entity: Entity, + ): DbEntitiesRow { + return { + id: entity.metadata.uid!, + location_id: locationId || null, + etag: entity.metadata.etag!, + generation: entity.metadata.generation!, + api_version: entity.apiVersion, + kind: entity.kind, + name: entity.metadata.name, + namespace: entity.metadata.namespace || null, + metadata: JSON.stringify( + lodash.omit(entity.metadata, ...entityMetaGeneratedFields), + ), + spec: entity.spec ? JSON.stringify(entity.spec) : null, + }; + } + + private toEntityResponse(row: DbEntitiesRow): DbEntityResponse { + const entity: Entity = { + apiVersion: row.api_version, + kind: row.kind, + metadata: { + ...(JSON.parse(row.metadata) as EntityMeta), + uid: row.id, + etag: row.etag, + generation: Number(row.generation), // cast because of sqlite + }, + }; + + if (row.spec) { + const spec = JSON.parse(row.spec); + entity.spec = spec; + } + + return { + locationId: row.location_id || undefined, + entity, + }; + } } diff --git a/plugins/register-component/src/components/RegisterComponentResultDialog/RegisterComponentResultDialog.test.tsx b/plugins/register-component/src/components/RegisterComponentResultDialog/RegisterComponentResultDialog.test.tsx index d918e72489..71933a0302 100644 --- a/plugins/register-component/src/components/RegisterComponentResultDialog/RegisterComponentResultDialog.test.tsx +++ b/plugins/register-component/src/components/RegisterComponentResultDialog/RegisterComponentResultDialog.test.tsx @@ -14,13 +14,12 @@ * limitations under the License. */ -import React, { ComponentProps } from 'react'; -import { render, cleanup } from '@testing-library/react'; -import { RegisterComponentResultDialog } from './RegisterComponentResultDialog'; -import { ThemeProvider } from '@material-ui/core'; import { lightTheme } from '@backstage/theme'; +import { ThemeProvider } from '@material-ui/core'; +import { cleanup, render } from '@testing-library/react'; +import React, { ComponentProps } from 'react'; import { MemoryRouter } from 'react-router-dom'; -import { Entity } from '@backstage/catalog-model'; +import { RegisterComponentResultDialog } from './RegisterComponentResultDialog'; const setup = ( props?: Partial>, @@ -52,6 +51,7 @@ it('should show a list of components if success', async () => { const { rendered } = setup({ entities: [ { + apiVersion: 'backstage.io/v1alpha1', kind: 'Component', metadata: { name: 'Component1', @@ -61,6 +61,7 @@ it('should show a list of components if success', async () => { }, }, { + apiVersion: 'backstage.io/v1alpha1', kind: 'Component', metadata: { name: 'Component2', @@ -69,7 +70,7 @@ it('should show a list of components if success', async () => { type: 'service', }, }, - ] as Entity[], + ], }); expect(