From 2d2710a7c486ab2dc5c005695df1089d5d1d03a5 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 21 May 2020 13:51:08 +0200 Subject: [PATCH 01/12] packages/cli: move plugin diff prompts to separate file --- .../cli/src/commands/plugin/diff/index.ts | 44 +++------------ .../cli/src/commands/plugin/diff/prompts.ts | 53 +++++++++++++++++++ 2 files changed, 59 insertions(+), 38 deletions(-) create mode 100644 packages/cli/src/commands/plugin/diff/prompts.ts diff --git a/packages/cli/src/commands/plugin/diff/index.ts b/packages/cli/src/commands/plugin/diff/index.ts index 5338594a3b..6300ff64d8 100644 --- a/packages/cli/src/commands/plugin/diff/index.ts +++ b/packages/cli/src/commands/plugin/diff/index.ts @@ -14,12 +14,14 @@ * limitations under the License. */ -import chalk from 'chalk'; -import inquirer from 'inquirer'; import { Command } from 'commander'; import { readTemplateFiles } from './read'; import { handlers, handleAllFiles } from './handlers'; -import { PromptFunc } from './types'; +import { + inquirerPromptFunc, + makeCheckPromptFunc, + yesPromptFunc, +} from './prompts'; const fileHandlers = [ { @@ -41,46 +43,12 @@ const fileHandlers = [ }, ]; -const inquirerPromptFunc: PromptFunc = async (msg) => { - const { result } = await inquirer.prompt({ - type: 'confirm', - name: 'result', - message: chalk.blue(msg), - }); - return result; -}; - -const makeCheck = () => { - let failed = false; - - const promptFunc: PromptFunc = async (msg) => { - failed = true; - console.log(chalk.red(`[Check Failed] ${msg}`)); - return false; - }; - - const finalize = () => { - if (failed) { - throw new Error( - 'Check failed, the plugin is not in sync with the latest template', - ); - } - }; - - return [promptFunc, finalize] as const; -}; - -const yesPromptFunc: PromptFunc = async (msg) => { - console.log(`Accepting: "${msg}"`); - return true; -}; - export default async (cmd: Command) => { let promptFunc = inquirerPromptFunc; let finalize = () => {}; if (cmd.check) { - [promptFunc, finalize] = makeCheck(); + [promptFunc, finalize] = makeCheckPromptFunc(); } else if (cmd.yes) { promptFunc = yesPromptFunc; } diff --git a/packages/cli/src/commands/plugin/diff/prompts.ts b/packages/cli/src/commands/plugin/diff/prompts.ts new file mode 100644 index 0000000000..ac04fc3559 --- /dev/null +++ b/packages/cli/src/commands/plugin/diff/prompts.ts @@ -0,0 +1,53 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import chalk from 'chalk'; +import inquirer from 'inquirer'; +import { PromptFunc } from './types'; + +export const inquirerPromptFunc: PromptFunc = async msg => { + const { result } = await inquirer.prompt({ + type: 'confirm', + name: 'result', + message: chalk.blue(msg), + }); + return result; +}; + +export const makeCheckPromptFunc = () => { + let failed = false; + + const promptFunc: PromptFunc = async msg => { + failed = true; + console.log(chalk.red(`[Check Failed] ${msg}`)); + return false; + }; + + const finalize = () => { + if (failed) { + throw new Error( + 'Check failed, the plugin is not in sync with the latest template', + ); + } + }; + + return [promptFunc, finalize] as const; +}; + +export const yesPromptFunc: PromptFunc = async msg => { + console.log(`Accepting: "${msg}"`); + return true; +}; From 18139238a4cedb1a457b3be24fdacb05568949de Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 21 May 2020 15:54:02 +0200 Subject: [PATCH 02/12] packages/cli: move plugin diff modules to separate diff lib --- .../plugin/{diff/index.ts => diff.ts} | 7 ++++--- .../{commands/plugin => lib}/diff/handlers.ts | 6 +++--- packages/cli/src/lib/diff/index.ts | 20 +++++++++++++++++++ .../{commands/plugin => lib}/diff/prompts.ts | 0 .../src/{commands/plugin => lib}/diff/read.ts | 12 +++++------ .../{commands/plugin => lib}/diff/types.ts | 0 6 files changed, 32 insertions(+), 13 deletions(-) rename packages/cli/src/commands/plugin/{diff/index.ts => diff.ts} (92%) rename packages/cli/src/{commands/plugin => lib}/diff/handlers.ts (97%) create mode 100644 packages/cli/src/lib/diff/index.ts rename packages/cli/src/{commands/plugin => lib}/diff/prompts.ts (100%) rename packages/cli/src/{commands/plugin => lib}/diff/read.ts (93%) rename packages/cli/src/{commands/plugin => lib}/diff/types.ts (100%) diff --git a/packages/cli/src/commands/plugin/diff/index.ts b/packages/cli/src/commands/plugin/diff.ts similarity index 92% rename from packages/cli/src/commands/plugin/diff/index.ts rename to packages/cli/src/commands/plugin/diff.ts index 6300ff64d8..542b94b0ff 100644 --- a/packages/cli/src/commands/plugin/diff/index.ts +++ b/packages/cli/src/commands/plugin/diff.ts @@ -15,13 +15,14 @@ */ import { Command } from 'commander'; -import { readTemplateFiles } from './read'; -import { handlers, handleAllFiles } from './handlers'; import { + readTemplateFiles, + handlers, + handleAllFiles, inquirerPromptFunc, makeCheckPromptFunc, yesPromptFunc, -} from './prompts'; +} from '../../lib/diff'; const fileHandlers = [ { diff --git a/packages/cli/src/commands/plugin/diff/handlers.ts b/packages/cli/src/lib/diff/handlers.ts similarity index 97% rename from packages/cli/src/commands/plugin/diff/handlers.ts rename to packages/cli/src/lib/diff/handlers.ts index 87518eab3e..643c018ddc 100644 --- a/packages/cli/src/commands/plugin/diff/handlers.ts +++ b/packages/cli/src/lib/diff/handlers.ts @@ -18,7 +18,7 @@ import fs from 'fs-extra'; import chalk from 'chalk'; import { dirname } from 'path'; import { diffLines } from 'diff'; -import { paths } from '../../../lib/paths'; +import { paths } from '../paths'; import { TemplateFile, PromptFunc, FileHandler } from './types'; export async function writeTargetFile(targetPath: string, contents: string) { @@ -221,8 +221,8 @@ export async function handleAllFiles( ) { for (const file of files) { const { targetPath } = file; - const fileHandler = fileHandlers.find((handler) => - handler.patterns.some((pattern) => + const fileHandler = fileHandlers.find(handler => + handler.patterns.some(pattern => typeof pattern === 'string' ? pattern === targetPath : pattern.test(targetPath), diff --git a/packages/cli/src/lib/diff/index.ts b/packages/cli/src/lib/diff/index.ts new file mode 100644 index 0000000000..04c5ee2e45 --- /dev/null +++ b/packages/cli/src/lib/diff/index.ts @@ -0,0 +1,20 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export * from './handlers'; +export * from './prompts'; +export * from './read'; +export * from './types'; diff --git a/packages/cli/src/commands/plugin/diff/prompts.ts b/packages/cli/src/lib/diff/prompts.ts similarity index 100% rename from packages/cli/src/commands/plugin/diff/prompts.ts rename to packages/cli/src/lib/diff/prompts.ts diff --git a/packages/cli/src/commands/plugin/diff/read.ts b/packages/cli/src/lib/diff/read.ts similarity index 93% rename from packages/cli/src/commands/plugin/diff/read.ts rename to packages/cli/src/lib/diff/read.ts index a05d7166e2..b408da94e9 100644 --- a/packages/cli/src/commands/plugin/diff/read.ts +++ b/packages/cli/src/lib/diff/read.ts @@ -18,8 +18,8 @@ import fs from 'fs-extra'; import { relative as relativePath } from 'path'; import handlebars from 'handlebars'; import recursiveReadDir from 'recursive-readdir'; -import { paths } from '../../../lib/paths'; -import { version } from '../../../lib/version'; +import { paths } from '../paths'; +import { version } from '../version'; import { PluginInfo, TemplateFile } from './types'; // Reads info from the existing plugin @@ -64,11 +64,9 @@ export async function readTemplate( templateDir: string, templateVars: any, ): Promise { - const templateFilePaths = await recursiveReadDir(templateDir).catch( - (error) => { - throw new Error(`Failed to read template directory: ${error.message}`); - }, - ); + const templateFilePaths = await recursiveReadDir(templateDir).catch(error => { + throw new Error(`Failed to read template directory: ${error.message}`); + }); const templateFiles = new Array(); for (const templateFile of templateFilePaths) { diff --git a/packages/cli/src/commands/plugin/diff/types.ts b/packages/cli/src/lib/diff/types.ts similarity index 100% rename from packages/cli/src/commands/plugin/diff/types.ts rename to packages/cli/src/lib/diff/types.ts From 12c669e904f7686845bd21d8d6866b03f8d1b0b8 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 21 May 2020 18:57:33 +0200 Subject: [PATCH 03/12] packages/cli: split diff template read into two steps and tweak handlers api --- packages/cli/src/commands/plugin/diff.ts | 4 +- packages/cli/src/lib/diff/handlers.ts | 82 +++++++++++------------- packages/cli/src/lib/diff/read.ts | 81 ++++++++++++++++------- packages/cli/src/lib/diff/types.ts | 33 ++++------ 4 files changed, 107 insertions(+), 93 deletions(-) diff --git a/packages/cli/src/commands/plugin/diff.ts b/packages/cli/src/commands/plugin/diff.ts index 542b94b0ff..ba3e99527f 100644 --- a/packages/cli/src/commands/plugin/diff.ts +++ b/packages/cli/src/commands/plugin/diff.ts @@ -16,7 +16,7 @@ import { Command } from 'commander'; import { - readTemplateFiles, + diffTemplateFiles, handlers, handleAllFiles, inquirerPromptFunc, @@ -54,7 +54,7 @@ export default async (cmd: Command) => { promptFunc = yesPromptFunc; } - const templateFiles = await readTemplateFiles('default-plugin'); + const templateFiles = await diffTemplateFiles('default-plugin'); await handleAllFiles(fileHandlers, templateFiles, promptFunc); await finalize(); }; diff --git a/packages/cli/src/lib/diff/handlers.ts b/packages/cli/src/lib/diff/handlers.ts index 643c018ddc..9e46dd92cf 100644 --- a/packages/cli/src/lib/diff/handlers.ts +++ b/packages/cli/src/lib/diff/handlers.ts @@ -14,36 +14,30 @@ * limitations under the License. */ -import fs from 'fs-extra'; import chalk from 'chalk'; -import { dirname } from 'path'; import { diffLines } from 'diff'; -import { paths } from '../paths'; -import { TemplateFile, PromptFunc, FileHandler } from './types'; - -export async function writeTargetFile(targetPath: string, contents: string) { - const path = paths.resolveTarget(targetPath); - await fs.ensureDir(dirname(path)); - await fs.writeFile(path, contents, 'utf8'); -} +import { FileDiff, PromptFunc, FileHandler, WriteFileFunc } from './types'; class PackageJsonHandler { - static async handler(file: TemplateFile, prompt: PromptFunc) { + static async handler( + { path, write, missing, targetContents, templateContents }: FileDiff, + prompt: PromptFunc, + ) { console.log('Checking package.json'); - if (!file.targetExists) { - throw new Error(`${file.targetPath} doesn't exist`); + if (missing) { + throw new Error(`${path} doesn't exist`); } - const pkg = JSON.parse(file.templateContents); - const targetPkg = JSON.parse(file.targetContents); + const pkg = JSON.parse(templateContents); + const targetPkg = JSON.parse(targetContents); - const handler = new PackageJsonHandler(file, prompt, pkg, targetPkg); + const handler = new PackageJsonHandler(write, prompt, pkg, targetPkg); await handler.handle(); } constructor( - private readonly file: TemplateFile, + private readonly writeFunc: WriteFileFunc, private readonly prompt: PromptFunc, private readonly pkg: any, private readonly targetPkg: any, @@ -144,31 +138,29 @@ class PackageJsonHandler { } private async write() { - await fs.writeFile( - paths.resolveTarget(this.file.targetPath), - `${JSON.stringify(this.targetPkg, null, 2)}\n`, - ); + await this.writeFunc(`${JSON.stringify(this.targetPkg, null, 2)}\n`); } } // Make sure the file is an exact match of the template -async function exactMatchHandler(file: TemplateFile, prompt: PromptFunc) { - console.log(`Checking ${file.targetPath}`); +async function exactMatchHandler( + { path, write, missing, targetContents, templateContents }: FileDiff, + prompt: PromptFunc, +) { + console.log(`Checking ${path}`); + const coloredPath = chalk.cyan(path); - const { targetPath, templateContents } = file; - const coloredPath = chalk.cyan(targetPath); - - if (!file.targetExists) { + if (missing) { if (await prompt(`Missing ${coloredPath}, do you want to add it?`)) { - await writeTargetFile(targetPath, templateContents); + await write(templateContents); } return; } - if (file.targetContents === templateContents) { + if (targetContents === templateContents) { return; } - const diffs = diffLines(file.targetContents, templateContents); + const diffs = diffLines(targetContents, templateContents); for (const diff of diffs) { if (diff.added) { process.stdout.write(chalk.green(`+${diff.value}`)); @@ -184,27 +176,29 @@ async function exactMatchHandler(file: TemplateFile, prompt: PromptFunc) { `Outdated ${coloredPath}, do you want to apply the above patch?`, ) ) { - await writeTargetFile(targetPath, templateContents); + await write(templateContents); } } // Adds the file if it is missing, but doesn't check existing files -async function existsHandler(file: TemplateFile, prompt: PromptFunc) { - console.log(`Making sure ${file.targetPath} exists`); +async function existsHandler( + { path, write, missing, templateContents }: FileDiff, + prompt: PromptFunc, +) { + console.log(`Making sure ${path} exists`); - const { targetPath, templateContents } = file; - const coloredPath = chalk.cyan(targetPath); + const coloredPath = chalk.cyan(path); - if (!file.targetExists) { + if (missing) { if (await prompt(`Missing ${coloredPath}, do you want to add it?`)) { - await writeTargetFile(targetPath, templateContents); + await write(templateContents); } return; } } -async function skipHandler(file: TemplateFile) { - console.log(`Skipping ${file.targetPath}`); +async function skipHandler({ path }: FileDiff) { + console.log(`Skipping ${path}`); } export const handlers = { @@ -216,22 +210,20 @@ export const handlers = { export async function handleAllFiles( fileHandlers: FileHandler[], - files: TemplateFile[], + files: FileDiff[], promptFunc: PromptFunc, ) { for (const file of files) { - const { targetPath } = file; + const { path } = file; const fileHandler = fileHandlers.find(handler => handler.patterns.some(pattern => - typeof pattern === 'string' - ? pattern === targetPath - : pattern.test(targetPath), + typeof pattern === 'string' ? pattern === path : pattern.test(path), ), ); if (fileHandler) { await fileHandler.handler(file, promptFunc); } else { - throw new Error(`No template file handler found for ${targetPath}`); + throw new Error(`No template file handler found for ${path}`); } } } diff --git a/packages/cli/src/lib/diff/read.ts b/packages/cli/src/lib/diff/read.ts index b408da94e9..a0bb6364ca 100644 --- a/packages/cli/src/lib/diff/read.ts +++ b/packages/cli/src/lib/diff/read.ts @@ -15,15 +15,29 @@ */ import fs from 'fs-extra'; -import { relative as relativePath } from 'path'; +import { + dirname, + resolve as resolvePath, + relative as relativePath, +} from 'path'; import handlebars from 'handlebars'; import recursiveReadDir from 'recursive-readdir'; import { paths } from '../paths'; import { version } from '../version'; -import { PluginInfo, TemplateFile } from './types'; +import { FileDiff } from './types'; + +export type PluginInfo = { + id: string; + name: string; +}; + +export type TemplatedFile = { + path: string; + contents: string; +}; // Reads info from the existing plugin -export async function readPluginInfo(): Promise { +async function readPluginInfo(): Promise { let name: string; try { const pkg = require(paths.resolveTarget('package.json')); @@ -47,7 +61,7 @@ export async function readPluginInfo(): Promise { return { id, name }; } -export async function readTemplateFile( +async function readTemplateFile( templateFile: string, templateVars: any, ): Promise { @@ -60,51 +74,70 @@ export async function readTemplateFile( return handlebars.compile(contents)(templateVars); } -export async function readTemplate( +async function readTemplate( templateDir: string, templateVars: any, -): Promise { +): Promise { const templateFilePaths = await recursiveReadDir(templateDir).catch(error => { throw new Error(`Failed to read template directory: ${error.message}`); }); - const templateFiles = new Array(); + const templatedFiles = new Array(); for (const templateFile of templateFilePaths) { - // Target file inside the target dir without template extension - const targetFile = templateFile - .replace(templateDir, paths.targetDir) - .replace(/\.hbs$/, ''); - const targetPath = relativePath(paths.targetDir, targetFile); + const path = relativePath(templateDir, templateFile).replace(/\.hbs$/, ''); + const contents = await readTemplateFile(templateFile, templateVars); - const templateContents = await readTemplateFile(templateFile, templateVars); + templatedFiles.push({ path, contents }); + } + + return templatedFiles; +} + +async function diffTemplatedFiles( + targetDir: string, + templatedFiles: TemplatedFile[], +): Promise { + const fileDiffs = new Array(); + for (const { path, contents: templateContents } of templatedFiles) { + const targetPath = resolvePath(targetDir, path); + const targetExists = await fs.pathExists(targetPath); + + const write = async (contents: string) => { + await fs.ensureDir(dirname(targetPath)); + await fs.writeFile(targetPath, contents, 'utf8'); + }; - const targetExists = await fs.pathExists(targetFile); if (targetExists) { - const targetContents = await fs.readFile(targetFile, 'utf8'); - templateFiles.push({ - targetPath, - targetExists, + const targetContents = await fs.readFile(targetPath, 'utf8'); + fileDiffs.push({ + path, + write, + missing: false, targetContents, templateContents, }); } else { - templateFiles.push({ - targetPath, - targetExists, + fileDiffs.push({ + path, + write, + missing: true, + targetContents: '', templateContents, }); } } - return templateFiles; + return fileDiffs; } // Read all template files for a given template, along with all matching files in the target dir -export async function readTemplateFiles(template: string) { +export async function diffTemplateFiles(template: string) { const pluginInfo = await readPluginInfo(); const templateVars = { version, ...pluginInfo }; const templateDir = paths.resolveOwn('templates', template); - return await readTemplate(templateDir, templateVars); + const templatedFiles = await readTemplate(templateDir, templateVars); + const fileDiffs = await diffTemplatedFiles(paths.targetDir, templatedFiles); + return fileDiffs; } diff --git a/packages/cli/src/lib/diff/types.ts b/packages/cli/src/lib/diff/types.ts index 7a04cf8d2e..6c50d9fcf9 100644 --- a/packages/cli/src/lib/diff/types.ts +++ b/packages/cli/src/lib/diff/types.ts @@ -14,35 +14,24 @@ * limitations under the License. */ -export type PluginInfo = { - id: string; - name: string; -}; +export type WriteFileFunc = (contents: string) => Promise; -export type TemplateFile = { +export type FileDiff = { // Relative path within the target directory - targetPath: string; + path: string; + // Wether the target file exists in the target directory. + missing: boolean; + // Contents of the file in the target directory, or an empty string if the file is missing. + targetContents: string; // Contents of the compiled template file templateContents: string; -} & ( - | { - // Whether the template file exists in the target directory - targetExists: true; - // Contents of the file in the target directory, if it exists - targetContents: string; - } - | { - // Whether the template file exists in the target directory - targetExists: false; - } -); + // Write new contents to the target file + write: WriteFileFunc; +}; export type PromptFunc = (msg: string) => Promise; -export type HandlerFunc = ( - file: TemplateFile, - prompt: PromptFunc, -) => Promise; +export type HandlerFunc = (file: FileDiff, prompt: PromptFunc) => Promise; export type FileHandler = { patterns: Array; From f0107283f856441e8fae0111d975c3fdd671e6e1 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 21 May 2020 19:31:14 +0200 Subject: [PATCH 04/12] packages/cli: move reading of plugin template data to plugin diff --- packages/cli/src/commands/plugin/diff.ts | 39 +++++++++++++++++++++++- packages/cli/src/lib/diff/read.ts | 38 ++--------------------- 2 files changed, 40 insertions(+), 37 deletions(-) diff --git a/packages/cli/src/commands/plugin/diff.ts b/packages/cli/src/commands/plugin/diff.ts index ba3e99527f..66db692ab4 100644 --- a/packages/cli/src/commands/plugin/diff.ts +++ b/packages/cli/src/commands/plugin/diff.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import fs from 'fs-extra'; import { Command } from 'commander'; import { diffTemplateFiles, @@ -23,6 +24,13 @@ import { makeCheckPromptFunc, yesPromptFunc, } from '../../lib/diff'; +import { paths } from '../../lib/paths'; +import { version } from '../../lib/version'; + +export type PluginData = { + id: string; + name: string; +}; const fileHandlers = [ { @@ -54,7 +62,36 @@ export default async (cmd: Command) => { promptFunc = yesPromptFunc; } - const templateFiles = await diffTemplateFiles('default-plugin'); + const data = await readPluginData(); + const templateFiles = await diffTemplateFiles('default-plugin', { + version, + ...data, + }); await handleAllFiles(fileHandlers, templateFiles, promptFunc); await finalize(); }; + +// Reads templating data from the existing plugin +async function readPluginData(): Promise { + let name: string; + try { + const pkg = require(paths.resolveTarget('package.json')); + name = pkg.name; + } catch (error) { + throw new Error(`Failed to read target package, ${error}`); + } + + const pluginTsContents = await fs.readFile( + paths.resolveTarget('src/plugin.ts'), + 'utf8', + ); + // TODO: replace with some proper parsing logic or plugin metadata file + const pluginIdMatch = pluginTsContents.match(/id: ['"`](.+?)['"`]/); + if (!pluginIdMatch) { + throw new Error(`Failed to parse plugin.ts, no plugin ID found`); + } + + const id = pluginIdMatch[1]; + + return { id, name }; +} diff --git a/packages/cli/src/lib/diff/read.ts b/packages/cli/src/lib/diff/read.ts index a0bb6364ca..75e1b86d24 100644 --- a/packages/cli/src/lib/diff/read.ts +++ b/packages/cli/src/lib/diff/read.ts @@ -23,44 +23,13 @@ import { import handlebars from 'handlebars'; import recursiveReadDir from 'recursive-readdir'; import { paths } from '../paths'; -import { version } from '../version'; import { FileDiff } from './types'; -export type PluginInfo = { - id: string; - name: string; -}; - export type TemplatedFile = { path: string; contents: string; }; -// Reads info from the existing plugin -async function readPluginInfo(): Promise { - let name: string; - try { - const pkg = require(paths.resolveTarget('package.json')); - name = pkg.name; - } catch (error) { - throw new Error(`Failed to read target package, ${error}`); - } - - const pluginTsContents = await fs.readFile( - paths.resolveTarget('src/plugin.ts'), - 'utf8', - ); - // TODO: replace with some proper parsing logic or plugin metadata file - const pluginIdMatch = pluginTsContents.match(/id: ['"`](.+?)['"`]/); - if (!pluginIdMatch) { - throw new Error(`Failed to parse plugin.ts, no plugin ID found`); - } - - const id = pluginIdMatch[1]; - - return { id, name }; -} - async function readTemplateFile( templateFile: string, templateVars: any, @@ -131,13 +100,10 @@ async function diffTemplatedFiles( } // Read all template files for a given template, along with all matching files in the target dir -export async function diffTemplateFiles(template: string) { - const pluginInfo = await readPluginInfo(); - const templateVars = { version, ...pluginInfo }; - +export async function diffTemplateFiles(template: string, templateData: any) { const templateDir = paths.resolveOwn('templates', template); - const templatedFiles = await readTemplate(templateDir, templateVars); + const templatedFiles = await readTemplate(templateDir, templateData); const fileDiffs = await diffTemplatedFiles(paths.targetDir, templatedFiles); return fileDiffs; } From f38ae61f5ba3f5ecc5f8f0daa9834b925ef608cc Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 21 May 2020 19:42:46 +0200 Subject: [PATCH 05/12] packages/cli: added app:diff + fixes to template and diff lib --- packages/cli/src/commands/app/diff.ts | 74 +++++++++++++++++++ packages/cli/src/index.ts | 9 ++- packages/cli/src/lib/diff/handlers.ts | 31 +++++++- .../default-app/packages/app/tsconfig.json | 20 ----- .../default-app/plugins/welcome/tsconfig.json | 5 -- .../templates/default-app/prettier.config.js | 1 - 6 files changed, 111 insertions(+), 29 deletions(-) create mode 100644 packages/cli/src/commands/app/diff.ts delete mode 100644 packages/cli/templates/default-app/packages/app/tsconfig.json delete mode 100644 packages/cli/templates/default-app/plugins/welcome/tsconfig.json delete mode 100644 packages/cli/templates/default-app/prettier.config.js diff --git a/packages/cli/src/commands/app/diff.ts b/packages/cli/src/commands/app/diff.ts new file mode 100644 index 0000000000..a667407f23 --- /dev/null +++ b/packages/cli/src/commands/app/diff.ts @@ -0,0 +1,74 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Command } from 'commander'; +import { + diffTemplateFiles, + handlers, + handleAllFiles, + inquirerPromptFunc, + makeCheckPromptFunc, + yesPromptFunc, +} from '../../lib/diff'; +import { version } from '../../lib/version'; + +const fileHandlers = [ + { + patterns: ['packages/app/package.json'], + handler: handlers.appPackageJson, + }, + { + patterns: [/tsconfig\.json$/], + handler: handlers.exactMatch, + }, + { + patterns: [ + /README\.md$/, + /\.eslintrc\.js$/, + // make sure files in 1st level of src/ and dev/ exist + /^packages\/app\/(src|dev)\/[^/]+$/, + ], + handler: handlers.exists, + }, + { + patterns: [ + 'lerna.json', + /^src\//, + /^patches\//, + /^packages\/app\/public\//, + /^packages\/app\/cypress/, + // Let plugin:diff take care of the plugins + /^plugins/, + /package\.json$/, + ], + handler: handlers.skip, + }, +]; + +export default async (cmd: Command) => { + let promptFunc = inquirerPromptFunc; + let finalize = () => {}; + + if (cmd.check) { + [promptFunc, finalize] = makeCheckPromptFunc(); + } else if (cmd.yes) { + promptFunc = yesPromptFunc; + } + + const templateFiles = await diffTemplateFiles('default-app', { version }); + await handleAllFiles(fileHandlers, templateFiles, promptFunc); + await finalize(); +}; diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index f02bf6524f..bdd0d3e1f2 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -39,6 +39,13 @@ const main = (argv: string[]) => { .option('--check', 'Enable type checking and linting') .action(actionHandler(() => require('./commands/app/serve'))); + program + .command('app:diff') + .option('--check', 'Fail if changes are required') + .option('--yes', 'Apply all changes') + .description('Diff an existing app with the creation template') + .action(actionHandler(() => require('./commands/app/diff'))); + program .command('create-plugin') .description('Creates a new plugin in the current repository') @@ -157,7 +164,7 @@ function actionHandler( }; } -process.on('unhandledRejection', (rejection) => { +process.on('unhandledRejection', rejection => { if (rejection instanceof Error) { exitWithError(rejection); } else { diff --git a/packages/cli/src/lib/diff/handlers.ts b/packages/cli/src/lib/diff/handlers.ts index 9e46dd92cf..c4efacb198 100644 --- a/packages/cli/src/lib/diff/handlers.ts +++ b/packages/cli/src/lib/diff/handlers.ts @@ -22,6 +22,7 @@ class PackageJsonHandler { static async handler( { path, write, missing, targetContents, templateContents }: FileDiff, prompt: PromptFunc, + variant?: string, ) { console.log('Checking package.json'); @@ -32,19 +33,33 @@ class PackageJsonHandler { const pkg = JSON.parse(templateContents); const targetPkg = JSON.parse(targetContents); - const handler = new PackageJsonHandler(write, prompt, pkg, targetPkg); + const handler = new PackageJsonHandler( + write, + prompt, + pkg, + targetPkg, + variant, + ); await handler.handle(); } + static async appHandler(file: FileDiff, prompt: PromptFunc) { + return PackageJsonHandler.handler(file, prompt, 'app'); + } + constructor( private readonly writeFunc: WriteFileFunc, private readonly prompt: PromptFunc, private readonly pkg: any, private readonly targetPkg: any, + private readonly variant?: string, ) {} async handle() { await this.syncField('main'); + if (this.variant !== 'app') { + await this.syncField('main:src'); + } await this.syncField('types'); await this.syncField('files'); await this.syncScripts(); @@ -78,7 +93,7 @@ class PackageJsonHandler { targetObj[fieldName] = newValue; await this.write(); } - } else { + } else if (fieldName in obj) { if ( await this.prompt( `package.json is missing field ${fullFieldName}, set to ${coloredNewValue}?`, @@ -95,6 +110,10 @@ class PackageJsonHandler { const targetScripts = (this.targetPkg.scripts = this.targetPkg.scripts || {}); + if (!pkgScripts) { + return; + } + for (const key of Object.keys(pkgScripts)) { await this.syncField(key, pkgScripts, targetScripts, 'scripts'); } @@ -132,7 +151,14 @@ class PackageJsonHandler { const targetDeps = (this.targetPkg[fieldName] = this.targetPkg[fieldName] || {}); + if (!pkgDeps) { + return; + } + for (const key of Object.keys(pkgDeps)) { + if (this.variant === 'app' && key.startsWith('plugin-')) { + continue; + } await this.syncField(key, pkgDeps, targetDeps, fieldName); } } @@ -206,6 +232,7 @@ export const handlers = { exists: existsHandler, exactMatch: exactMatchHandler, packageJson: PackageJsonHandler.handler, + appPackageJson: PackageJsonHandler.appHandler, }; export async function handleAllFiles( diff --git a/packages/cli/templates/default-app/packages/app/tsconfig.json b/packages/cli/templates/default-app/packages/app/tsconfig.json deleted file mode 100644 index 0457810645..0000000000 --- a/packages/cli/templates/default-app/packages/app/tsconfig.json +++ /dev/null @@ -1,20 +0,0 @@ -{ - "extends": "@spotify/web-scripts/config/tsconfig.json", - "compilerOptions": { - "target": "es5", - "lib": ["dom", "dom.iterable", "esnext"], - "allowJs": true, - "skipLibCheck": true, - "esModuleInterop": true, - "allowSyntheticDefaultImports": true, - "strict": true, - "forceConsistentCasingInFileNames": true, - "module": "esnext", - "moduleResolution": "node", - "resolveJsonModule": true, - "isolatedModules": true, - "jsx": "react", - "incremental": false - }, - "include": ["src"] -} diff --git a/packages/cli/templates/default-app/plugins/welcome/tsconfig.json b/packages/cli/templates/default-app/plugins/welcome/tsconfig.json deleted file mode 100644 index 5a3931ffce..0000000000 --- a/packages/cli/templates/default-app/plugins/welcome/tsconfig.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "extends": "../../tsconfig.json", - "include": ["src"], - "compilerOptions": {} -} diff --git a/packages/cli/templates/default-app/prettier.config.js b/packages/cli/templates/default-app/prettier.config.js deleted file mode 100644 index 93df970dd6..0000000000 --- a/packages/cli/templates/default-app/prettier.config.js +++ /dev/null @@ -1 +0,0 @@ -module.exports = require('@spotify/web-scripts/config/prettier.config.js'); From 6c2a9d471ae67220c89e85e674108a99950f0175 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 21 May 2020 19:45:12 +0200 Subject: [PATCH 06/12] packages/cli: update prettier config in plugin template --- packages/cli/templates/default-app/package.json.hbs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/cli/templates/default-app/package.json.hbs b/packages/cli/templates/default-app/package.json.hbs index 589e20e298..4fda7ff2d7 100644 --- a/packages/cli/templates/default-app/package.json.hbs +++ b/packages/cli/templates/default-app/package.json.hbs @@ -28,8 +28,19 @@ }, "devDependencies": { "@backstage/cli": "^{{version}}", + "@spotify/prettier-config": "^7.0.0", "lerna": "^3.20.2", "patch-package": "^6.2.2", "prettier": "^1.19.1" + }, + "prettier": "@spotify/prettier-config", + "lint-staged": { + "*.{js,jsx,ts,tsx}": [ + "eslint --fix", + "prettier --write" + ], + "*.{json,md}": [ + "prettier --write" + ] } } From 10d091ca6eb22a94b4ed24764101b1e04222827f Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 25 May 2020 10:03:11 +0200 Subject: [PATCH 07/12] docs: added getting started with utility apis --- docs/getting-started/README.md | 1 + docs/getting-started/utility-apis-fig1.svg | 3 + docs/getting-started/utility-apis.md | 140 +++++++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 docs/getting-started/utility-apis-fig1.svg create mode 100644 docs/getting-started/utility-apis.md diff --git a/docs/getting-started/README.md b/docs/getting-started/README.md index 064776e712..14eb868733 100644 --- a/docs/getting-started/README.md +++ b/docs/getting-started/README.md @@ -7,6 +7,7 @@ The value of Backstage grows with every new plugin that gets added. Here is a co - [Development Environment](development-environment.md) - [Create a Backstage plugin](create-a-plugin.md) - [Structure of a plugin](structure-of-a-plugin.md) +- [Utility APIs](utility-apis.md) - Using Backstage components (TODO) [Back to Docs](../README.md) diff --git a/docs/getting-started/utility-apis-fig1.svg b/docs/getting-started/utility-apis-fig1.svg new file mode 100644 index 0000000000..47123299c7 --- /dev/null +++ b/docs/getting-started/utility-apis-fig1.svg @@ -0,0 +1,3 @@ + + +
fooApiRef
fooApiRef
FooApi
FooApiPlugin
Component
Component
App A
App A
App B
App B
DefaultFooApi
DefaultFooApi
CustomFooApi
CustomFooApi
App C
App CViewer does not support full SVG 1.1 \ No newline at end of file diff --git a/docs/getting-started/utility-apis.md b/docs/getting-started/utility-apis.md new file mode 100644 index 0000000000..3a223e019b --- /dev/null +++ b/docs/getting-started/utility-apis.md @@ -0,0 +1,140 @@ +# Utility APIs + +## Introduction + +Backstage Plugins strive to be self-contained, with as much functionality +as possible residing withing the plugin itself and its backend APIs. There will however +always be a need for plugins to communicate outside of it's boundaries, both with other +plugins and the app itself. + +Backstage provides two primary methods for plugins to communication across +their boundaries in client-side code. The first one being the `createPlugin` API and the registration hooks +passed to the `register` method, and the second one being Utility APIs. +While the `createPlugin` API is focused on the initialization plugins and the app, +the Utility APIs provide ways for plugins to communicate during their entire life cycle. + +## Usage + +Each Utility API is tied to an `ApiRef` instance, which is a global singleton object +without any additional state or functionality, its only purpose is to reference Utility APIs. +`ApiRef`s are create using `createApiRef`, which is exported by `@backstage/core`. +There are many predefined Utility APIs defined in `@backstage/core`, and they're all exported with a name of the pattern `*ApiRef`, for example `errorApiRef`. + +To access one of the Utility APIs inside a React component, use the `useApi` hook exported by +`@backstage/core`, or the `withApis` HOC if you prefer class components. For example, the `ErrorApi` can be accessed like this: + +```tsx +import React, { FC } from 'react'; +import { useApi, errorApiRef } from '@backstage/core'; + +export const MyComponent: FC<{}> = () => { + const errorApi = useApi(errorApiRef); + + // Signal to the app that something went wrong, and display the error to the user. + const handleError = error => { + errorApi.post(error); + }; + + // the rest of the component ... +}; +``` + +Note that there is not explicit type given for the `ErrorApi`, this is because the `errorApiRef` has the type embedded, and `useApi` is able to infer the type. + +Also note that consuming Utility APIs is not limited to plugins, it can be done from any component inside Backstage, including the ones in `@backstage/core`. The only requirement is that they are beneath the `AppProvider` in the react tree. + +## Registering Utility API Implementations + +The Backstage App is responsible for providing implementations for all Utility APIs required by plugins. The example app in this repo registers its APIs inside [src/apis.ts](/packages/app/src/apis.ts). Here's an example of how to wire up the `ErrorApi` inside an app: + +```ts +import { + ApiRegistry, + createApp, + alertApiRef, + errorApiRef, + AlertApiForwarder, + ErrorApiForwarder, +} from '@backstage/core'; + +const builder = ApiRegistry.builder(); + +// The alert API is a self-contained implementation that shows alerts to the user. +builder.add(alertApiRef, new AlertApiForwarder()); + +// The error API uses the alert API to send error notifications to the user. +builder.add(errorApiRef, new ErrorApiForwarder(alertApiForwarder)); + +const app = createApp({ + apis: apiBuilder.build(), + // ... other config +}); +``` + +The `ApiRegistry` is used to register all Utility APIs in the app and associate them with `ApiRef`s. It implements the `ApiHolder` interface, which enables it to provide an API implementation given an `ApiRef`. + +Note that our `ErrorApi` implementation depends on another Utility API, the `AlertApi`. This is the method with which APIs can depend on other APIs, using manual dependency injection at the initialization of the app. In general, if you want +to depend on another Utility API in an implementation of an API, you import the type for that API and make it a +constructor parameter. + +## Custom implementations of Utility APIs + +Defining a custom implementation of a utility API is easy, you simply need to export a class +that `implements` the target API, for example: + +```ts +export class IgnoringErrorApi implements ErrorApi { + post(error: Error, context?: ErrorContext) { + // ignore error + } +} +``` + +The `IgnoringErrorApi` would then be imported in the app, and wired up like this: + +```ts +builder.add(errorApiRef, new IgnoringErrorApi()); +``` + +Note that the above line will cause an error if `IgnoreErrorApi` does not fully +implement the `ErrorApi`, as it is checked by the type embedded in the `errorApiRef` at compile time. + +## Defining custom Utility APIs + +The pattern for plugins defining their own Utility APIs is not fully established yet. The current way is for the plugin to export it's own `ApiRef` and type for the API, along with one or more implementations. It is then up to the app to import, and register those APIs. See for example the lighthouse or graphiql plugins for examples of this. + +The goal is to make this process a bit smoother, but that requires work in other parts of Backstage, like configuration management. So it remains as a TODO. If you have more questions regarding this, or have an idea for and API that you want to share outside your plugin, hit us up in GitHub issues or the Backstage Discord server. + +## Architecture + +The `ApiRef` instances mentioned above provide a point of indirection between consumers and producers +Utility APIs. It allows for plugins and components to depend on APIs in a type-safe way, without +having a direct reference to a concrete implementation of the APIs. The Apps are also given a lot +of flexibility in what implementations to provide. As long as they adhere to the contract established +by an `ApiRef`, they are free to choose any implementation they want. + +The figure below shows the relationship between different Apps, that +provide different implementations of the +FooApi. Components within Plugins +then access the FooApi via the fooApiRef. + +
+Figure showing the relationship between utility APIs, the apps that provide them, and the plugins that consume them +
+ +The current method for connecting Utility API providers and consumers is via the React tree using +an `ApiProvider`, which is added to the `AppProvider` of the `App`. In the future there may potentially +be more ways to do this, in ways that are not tied to react. A design goal of the Utility APIs was to +not have them directly tied to React. + +The indirection provided by Utility APIs also makes it straightforward to test components that depend +on APIs, and to provide a standard common development environment for plugins. A proper test wrapper +with mocked API implementations is not yet ready, but it will provided as a part of +`@backstage/test-utils`. It will provide mocked variants of APIs, with additional methods for asserting +a components interaction with the API. + +The common development environment for plugins is included in `@backstage/dev-utils`, where the exported +`createDevApp` function creates an application with implementations for all core APIs already present. +Contrary to the method for wiring up Utility API implementations in an app created with `createApp`, `createDevApp` +uses automatic dependency injection. This is to make it possible to replace any API implementation, and +having that be reflected in dependents of that API. From 645bee1b4c1a31a10e3fa3ec235fc269f84628cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Sat, 23 May 2020 19:29:21 +0200 Subject: [PATCH 08/12] Add code for building a search index over added/modified entities This is not actually being inserted yet, because doing so needs a little refactoring of the database code that happens in another PR. Adding separate PR for the index to make it easier to review the logic. --- plugins/catalog-backend/src/database/Database.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/catalog-backend/src/database/Database.ts b/plugins/catalog-backend/src/database/Database.ts index c797a57f3c..2413fb217c 100644 --- a/plugins/catalog-backend/src/database/Database.ts +++ b/plugins/catalog-backend/src/database/Database.ts @@ -27,6 +27,7 @@ import { DbEntityResponse, DbLocationsRow, } from './types'; +import { buildEntitySearch } from './search'; function serializeMetadata( metadata: DescriptorEnvelope['metadata'], From 44844af0bdf5479f6f79ee1042994ca9a5fdd754 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Wed, 20 May 2020 14:21:20 +0200 Subject: [PATCH 09/12] Store entities completely in the database --- packages/backend-common/src/logging/index.ts | 1 + .../backend-common/src/logging/voidLogger.ts | 21 +- packages/backend/src/plugins/catalog.ts | 2 +- .../fixtures/two_components.yaml | 2 +- .../src/catalog/DatabaseEntitiesCatalog.ts | 18 +- .../src/catalog/StaticEntitiesCatalog.ts | 20 +- plugins/catalog-backend/src/catalog/types.ts | 6 +- .../src/database/Database.test.ts | 191 +++++++++- .../catalog-backend/src/database/Database.ts | 327 ++++++++++++++---- .../src/database/DatabaseManager.test.ts | 68 ++-- .../src/database/DatabaseManager.ts | 97 +++++- ...0200520140700_location_update_log_table.ts | 2 +- .../src/database/search.test.ts | 2 +- .../src/ingestion/LocationReaders.ts | 3 +- .../ComponentDescriptorV1beta1Parser.ts | 3 +- .../descriptors/DescriptorEnvelopeParser.ts | 4 +- .../src/ingestion/descriptors/types.ts | 38 -- .../ingestion/sources/FileLocationSource.ts | 3 +- .../catalog-backend/src/ingestion/types.ts | 125 +++---- plugins/catalog-backend/src/service/router.ts | 14 +- 20 files changed, 690 insertions(+), 257 deletions(-) rename plugins/catalog-backend/src/ingestion/sources/types.ts => packages/backend-common/src/logging/voidLogger.ts (61%) delete mode 100644 plugins/catalog-backend/src/ingestion/descriptors/types.ts diff --git a/packages/backend-common/src/logging/index.ts b/packages/backend-common/src/logging/index.ts index 06ce76ac54..0ebf0371e9 100644 --- a/packages/backend-common/src/logging/index.ts +++ b/packages/backend-common/src/logging/index.ts @@ -15,3 +15,4 @@ */ export * from './rootLogger'; +export * from './voidLogger'; diff --git a/plugins/catalog-backend/src/ingestion/sources/types.ts b/packages/backend-common/src/logging/voidLogger.ts similarity index 61% rename from plugins/catalog-backend/src/ingestion/sources/types.ts rename to packages/backend-common/src/logging/voidLogger.ts index c078693a19..1837b0e412 100644 --- a/plugins/catalog-backend/src/ingestion/sources/types.ts +++ b/packages/backend-common/src/logging/voidLogger.ts @@ -14,15 +14,14 @@ * limitations under the License. */ -import { ReaderOutput } from '../types'; +import { PassThrough } from 'stream'; +import winston, { Logger } from 'winston'; -export type LocationSource = { - /** - * Reads the contents of a single location. - * - * @param target The location target to read - * @returns The parsed contents, as an array of unverified descriptors - * @throws An error if the location target could not be read - */ - read(target: string): Promise; -}; +/** + * A logger that just throws away all messages. + */ +export function getVoidLogger(): Logger { + return winston.createLogger({ + transports: [new winston.transports.Stream({ stream: new PassThrough() })], + }); +} diff --git a/packages/backend/src/plugins/catalog.ts b/packages/backend/src/plugins/catalog.ts index d48fda5681..e18d021395 100644 --- a/packages/backend/src/plugins/catalog.ts +++ b/packages/backend/src/plugins/catalog.ts @@ -29,7 +29,7 @@ export default async function ({ logger, database }: PluginEnvironment) { const reader = LocationReaders.create(); const parser = DescriptorParsers.create(); - const db = await DatabaseManager.createDatabase(database); + const db = await DatabaseManager.createDatabase(database, logger); runPeriodically( () => DatabaseManager.refreshLocations(db, reader, parser, logger), 10000, diff --git a/plugins/catalog-backend/fixtures/two_components.yaml b/plugins/catalog-backend/fixtures/two_components.yaml index d0e51ca79e..470106116d 100644 --- a/plugins/catalog-backend/fixtures/two_components.yaml +++ b/plugins/catalog-backend/fixtures/two_components.yaml @@ -11,4 +11,4 @@ kind: Component metadata: name: component2 spec: - type: service + type: website diff --git a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts index 5c2c79d427..697da0ea35 100644 --- a/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/catalog/DatabaseEntitiesCatalog.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { NotFoundError } from '@backstage/backend-common'; import { Database } from '../database'; import { DescriptorEnvelope } from '../ingestion/types'; import { EntitiesCatalog } from './types'; @@ -22,12 +23,23 @@ export class DatabaseEntitiesCatalog implements EntitiesCatalog { constructor(private readonly database: Database) {} async entities(): Promise { - const items = await this.database.entities(); + const items = await this.database.transaction(tx => + this.database.entities(tx), + ); return items.map(i => i.entity); } - async entity(name: string): Promise { - const item = await this.database.entity(name); + async entity( + kind: string, + name: string, + namespace: string | undefined, + ): Promise { + const item = await this.database.transaction(tx => + this.database.entity(tx, kind, name, namespace), + ); + if (!item) { + throw new NotFoundError('Entity cannot be found'); + } return item.entity; } } diff --git a/plugins/catalog-backend/src/catalog/StaticEntitiesCatalog.ts b/plugins/catalog-backend/src/catalog/StaticEntitiesCatalog.ts index d74487ab2c..17f7603fb7 100644 --- a/plugins/catalog-backend/src/catalog/StaticEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/catalog/StaticEntitiesCatalog.ts @@ -15,6 +15,7 @@ */ import { NotFoundError } from '@backstage/backend-common'; +import lodash from 'lodash'; import { DescriptorEnvelope } from '../ingestion'; import { EntitiesCatalog } from './types'; @@ -26,14 +27,23 @@ export class StaticEntitiesCatalog implements EntitiesCatalog { } async entities(): Promise { - return this._entities.slice(); + return lodash.cloneDeep(this._entities); } - async entity(name: string): Promise { - const item = this._entities.find(e => e.metadata?.name === name); + async entity( + kind: string, + name: string, + namespace: string | undefined, + ): Promise { + const item = this._entities.find( + e => + kind === e.kind && + name === e.metadata?.name && + namespace === e.metadata?.namespace, + ); if (!item) { - throw new NotFoundError(`Found no entity with name ${name}`); + throw new NotFoundError('Entity cannot be found'); } - return item; + return lodash.cloneDeep(item); } } diff --git a/plugins/catalog-backend/src/catalog/types.ts b/plugins/catalog-backend/src/catalog/types.ts index bb50a00783..92c059d25b 100644 --- a/plugins/catalog-backend/src/catalog/types.ts +++ b/plugins/catalog-backend/src/catalog/types.ts @@ -23,7 +23,11 @@ import { DescriptorEnvelope } from '../ingestion'; export type EntitiesCatalog = { entities(): Promise; - entity(id: string): Promise; + entity( + kind: string, + name: string, + namespace: string | undefined, + ): Promise; }; // diff --git a/plugins/catalog-backend/src/database/Database.test.ts b/plugins/catalog-backend/src/database/Database.test.ts index 459968412e..0eb64916e5 100644 --- a/plugins/catalog-backend/src/database/Database.test.ts +++ b/plugins/catalog-backend/src/database/Database.test.ts @@ -14,30 +14,74 @@ * limitations under the License. */ -import knex from 'knex'; +import { + ConflictError, + getVoidLogger, + NotFoundError, +} from '@backstage/backend-common'; +import Knex from 'knex'; import path from 'path'; import { Database } from './Database'; -import { AddDatabaseLocation, DbLocationsRow } from './types'; +import { + AddDatabaseLocation, + DbEntityRequest, + DbEntityResponse, + DbLocationsRow, +} from './types'; describe('Database', () => { - const database = knex({ - client: 'sqlite3', - connection: ':memory:', - useNullAsDefault: true, - }); - database.client.pool.on('createSuccess', (_eventId: any, resource: any) => { - resource.run('PRAGMA foreign_keys = ON', () => {}); - }); + let database: Knex; + let entityRequest: DbEntityRequest; + let entityResponse: DbEntityResponse; beforeEach(async () => { + database = Knex({ + client: 'sqlite3', + connection: ':memory:', + useNullAsDefault: true, + }); + + await database.raw('PRAGMA foreign_keys = ON'); await database.migrate.latest({ directory: path.resolve(__dirname, 'migrations'), loadExtensions: ['.ts'], }); + + entityRequest = { + entity: { + apiVersion: 'a', + kind: 'b', + metadata: { + name: 'c', + namespace: 'd', + labels: { e: 'f' }, + annotations: { g: 'h' }, + }, + spec: { i: 'j' }, + }, + }; + + entityResponse = { + locationId: undefined, + entity: { + apiVersion: 'a', + kind: 'b', + metadata: { + uid: expect.anything(), + etag: expect.anything(), + generation: expect.anything(), + name: 'c', + namespace: 'd', + labels: { e: 'f' }, + annotations: { g: 'h' }, + }, + spec: { i: 'j' }, + }, + }; }); it('manages locations', async () => { - const db = new Database(database); + const db = new Database(database, getVoidLogger()); const input: AddDatabaseLocation = { type: 'a', target: 'b' }; const output: DbLocationsRow = { id: expect.anything(), @@ -62,7 +106,7 @@ describe('Database', () => { it('instead of adding second location with the same target, returns existing one', async () => { // Prepare - const catalog = new Database(database); + const catalog = new Database(database, getVoidLogger()); const input: AddDatabaseLocation = { type: 'a', target: 'b' }; const output1: DbLocationsRow = await catalog.addLocation(input); @@ -75,4 +119,127 @@ describe('Database', () => { // Locations contain only one record expect(locations).toEqual([output1]); }); + + describe('addEntity', () => { + it('happy path: adds entity to empty database', async () => { + const catalog = new Database(database, getVoidLogger()); + const added = await catalog.transaction(tx => + catalog.addEntity(tx, entityRequest), + ); + expect(added).toStrictEqual(entityResponse); + expect(added.entity.metadata!.generation).toBe(1); + }); + + it('rejects adding the same-named entity twice', async () => { + const catalog = new Database(database, getVoidLogger()); + await catalog.transaction(tx => catalog.addEntity(tx, entityRequest)); + await expect( + catalog.transaction(tx => catalog.addEntity(tx, entityRequest)), + ).rejects.toThrow(ConflictError); + }); + + it('accepts adding the same-named entity twice if on different namespaces', async () => { + const catalog = new Database(database, getVoidLogger()); + entityRequest.entity.metadata!.namespace = 'namespace1'; + await catalog.transaction(tx => catalog.addEntity(tx, entityRequest)); + entityRequest.entity.metadata!.namespace = 'namespace2'; + await expect( + catalog.transaction(tx => catalog.addEntity(tx, entityRequest)), + ).resolves.toBeDefined(); + }); + }); + + describe('updateEntity', () => { + it('can read and no-op-update an entity', async () => { + const catalog = new Database(database, getVoidLogger()); + const added = await catalog.transaction(tx => + catalog.addEntity(tx, entityRequest), + ); + const updated = await catalog.transaction(tx => + catalog.updateEntity(tx, { entity: added.entity }), + ); + expect(updated.entity.apiVersion).toEqual(added.entity.apiVersion); + expect(updated.entity.kind).toEqual(added.entity.kind); + expect(updated.entity.metadata!.etag).not.toEqual( + added.entity.metadata!.etag, + ); + expect(updated.entity.metadata!.generation).toEqual( + added.entity.metadata!.generation, + ); + expect(updated.entity.metadata!.name).toEqual( + added.entity.metadata!.name, + ); + expect(updated.entity.metadata!.namespace).toEqual( + added.entity.metadata!.namespace, + ); + }); + + it('can update name if uid matches', async () => { + const catalog = new Database(database, getVoidLogger()); + const added = await catalog.transaction(tx => + catalog.addEntity(tx, entityRequest), + ); + added.entity.metadata!.name! = 'new!'; + const updated = await catalog.transaction(tx => + catalog.updateEntity(tx, { entity: added.entity }), + ); + expect(updated.entity.metadata!.name).toEqual('new!'); + }); + + it('can update fields if kind, name, and namespace match', async () => { + const catalog = new Database(database, getVoidLogger()); + const added = await catalog.transaction(tx => + catalog.addEntity(tx, entityRequest), + ); + added.entity.apiVersion = 'something.new'; + delete added.entity.metadata!.uid; + delete added.entity.metadata!.generation; + const updated = await catalog.transaction(tx => + catalog.updateEntity(tx, { entity: added.entity }), + ); + expect(updated.entity.apiVersion).toEqual('something.new'); + }); + + it('rejects if kind, name, but not namespace match', async () => { + const catalog = new Database(database, getVoidLogger()); + const added = await catalog.transaction(tx => + catalog.addEntity(tx, entityRequest), + ); + added.entity.apiVersion = 'something.new'; + delete added.entity.metadata!.uid; + delete added.entity.metadata!.generation; + added.entity.metadata!.namespace = 'something.wrong'; + await expect( + catalog.transaction(tx => + catalog.updateEntity(tx, { entity: added.entity }), + ), + ).rejects.toThrow(NotFoundError); + }); + + it('fails to update an entity if etag does not match', async () => { + const catalog = new Database(database, getVoidLogger()); + const added = await catalog.transaction(tx => + catalog.addEntity(tx, entityRequest), + ); + added.entity.metadata!.etag = 'garbage'; + await expect( + catalog.transaction(tx => + catalog.updateEntity(tx, { entity: added.entity }), + ), + ).rejects.toThrow(ConflictError); + }); + + it('fails to update an entity if generation does not match', async () => { + const catalog = new Database(database, getVoidLogger()); + const added = await catalog.transaction(tx => + catalog.addEntity(tx, entityRequest), + ); + added.entity.metadata!.generation! += 100; + await expect( + catalog.transaction(tx => + catalog.updateEntity(tx, { entity: added.entity }), + ), + ).rejects.toThrow(ConflictError); + }); + }); }); diff --git a/plugins/catalog-backend/src/database/Database.ts b/plugins/catalog-backend/src/database/Database.ts index 2413fb217c..cd4ca731a3 100644 --- a/plugins/catalog-backend/src/database/Database.ts +++ b/plugins/catalog-backend/src/database/Database.ts @@ -14,34 +14,43 @@ * limitations under the License. */ -import { InputError, NotFoundError } from '@backstage/backend-common'; +import { + ConflictError, + InputError, + NotFoundError, +} from '@backstage/backend-common'; import Knex from 'knex'; +import lodash from 'lodash'; import { v4 as uuidv4 } from 'uuid'; -import { DescriptorEnvelope } from '../ingestion'; +import { Logger } from 'winston'; +import { DescriptorEnvelope, EntityMeta } from '../ingestion'; +import { buildEntitySearch } from './search'; import { AddDatabaseLocation, DatabaseLocationUpdateLogEvent, DatabaseLocationUpdateLogStatus, DbEntitiesRow, + DbEntitiesSearchRow, DbEntityRequest, DbEntityResponse, DbLocationsRow, } from './types'; -import { buildEntitySearch } from './search'; -function serializeMetadata( - metadata: DescriptorEnvelope['metadata'], -): DbEntitiesRow['metadata'] { - if (!metadata) { - return null; - } - - const output = { ...metadata }; +function getStrippedMetadata(metadata: EntityMeta): EntityMeta { + const output = lodash.cloneDeep(metadata); delete output.uid; delete output.etag; delete output.generation; - return JSON.stringify(output); + return output; +} + +function serializeMetadata(metadata: EntityMeta | undefined): string | null { + if (!metadata) { + return null; + } + + return JSON.stringify(getStrippedMetadata(metadata)); } function serializeSpec( @@ -54,29 +63,32 @@ function serializeSpec( return JSON.stringify(spec); } -function entityRequestToDb(request: DbEntityRequest): DbEntitiesRow { +function toEntityRow( + locationId: string | undefined, + entity: DescriptorEnvelope, +): DbEntitiesRow { return { - id: '', - location_id: request.locationId || null, - etag: new Buffer(uuidv4()).toString('base64').replace(/[^\w]/g, ''), // TODO(freben): Atomicity isn't checked using these yet - generation: 1, // TODO(freben): These aren't updated yet - api_version: request.entity.apiVersion, - kind: request.entity.kind, - name: request.entity.metadata?.name || null, - namespace: request.entity.metadata?.namespace || null, - metadata: serializeMetadata(request.entity.metadata), - spec: serializeSpec(request.entity.spec), + 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 || null, + namespace: entity.metadata!.namespace || null, + metadata: serializeMetadata(entity.metadata), + spec: serializeSpec(entity.spec), }; } -function entityDbToResponse(row: DbEntitiesRow): DbEntityResponse { +function toEntityResponse(row: DbEntitiesRow): DbEntityResponse { const entity: DescriptorEnvelope = { apiVersion: row.api_version, kind: row.kind, metadata: { uid: row.id, etag: row.etag, - generation: row.generation, + generation: Number(row.generation), // cast because of sqlite }, }; @@ -96,49 +108,229 @@ function entityDbToResponse(row: DbEntitiesRow): DbEntityResponse { }; } -export class Database { - constructor(private readonly database: Knex) {} - - async addOrUpdateEntity(request: DbEntityRequest): Promise { - if (!request.entity.metadata?.name) { - throw new InputError(`Entities without names are not yet supported`); - } - - const newRow = entityRequestToDb(request); - - await this.database.transaction(async tx => { - // TODO(freben): Currently, several locations can compete for the same entity - // TODO(freben): If locationId is unset in the input, it won't be overwritten - should we instead replace with null? - const count = await tx('entities') - .where({ name: request.entity.metadata?.name }) - .update({ - ...newRow, - id: undefined, - }); - if (!count) { - await tx('entities').insert({ - ...newRow, - id: uuidv4(), - }); - } - }); +function specsAreEqual( + first: string | null, + second: object | undefined, +): boolean { + if (!first && !second) { + return true; + } else if (!first || !second) { + return false; } - async entities(): Promise { - const items = await this.database('entities') + return lodash.isEqual(JSON.parse(first), second); +} + +function generateUid(): string { + return uuidv4(); +} + +function generateEtag(): string { + return Buffer.from(uuidv4(), 'utf8').toString('base64').replace(/[^\w]/g, ''); +} + +/** + * An abstraction on top of the underlying database, wrapping the basic CRUD + * needs. + */ +export class Database { + constructor( + private readonly database: Knex, + private readonly logger: Logger, + ) {} + + /** + * Runs a transaction. + * + * The callback is expected to make calls back into this class. When it + * completes, the transaction is closed. + * + * @param fn The callback that implements the transaction + */ + async transaction( + fn: (tx: Knex.Transaction) => Promise, + ): Promise { + try { + return await this.database.transaction(fn); + } catch (e) { + this.logger.debug(`Error during transaction, ${e}`); + + if ( + /SQLITE_CONSTRAINT: UNIQUE/.test(e.message) || + /unique constraint/.test(e.message) + ) { + throw new ConflictError(`Rejected due to a conflicting entity`, e); + } + + throw e; + } + } + + /** + * Adds a new entity to the catalog. + * + * @param tx An ongoing transaction + * @param request The entity being added + * @returns The added entity, with uid, etag and generation set + */ + async addEntity( + tx: Knex.Transaction, + request: DbEntityRequest, + ): Promise { + if (request.entity.metadata?.uid !== undefined) { + throw new InputError('May not specify uid for new entities'); + } else if (request.entity.metadata?.etag !== undefined) { + throw new InputError('May not specify etag for new entities'); + } else if (request.entity.metadata?.generation !== undefined) { + throw new InputError('May not specify generation for new entities'); + } + + const newEntity = lodash.cloneDeep(request.entity); + newEntity.metadata = Object.assign({}, newEntity.metadata, { + uid: generateUid(), + etag: generateEtag(), + generation: 1, + }); + + const newRow = toEntityRow(request.locationId, newEntity); + await tx('entities').insert(newRow); + await this.updateEntitiesSearch(tx, newRow.id, newEntity); + + return { locationId: request.locationId, entity: newEntity }; + } + + /** + * Updates an existing entity in the catalog. + * + * The given entity must contain enough information to identify an already + * stored entity in the catalog - either by uid, or by kind + namespace + + * name. If no matching entity is found, the operation fails. + * + * If etag or generation are given, they are taken into account. Attempts to + * update a matching entity, but where the etag and/or generation are not + * equal to the passed values, will fail. + * + * @param tx An ongoing transaction + * @param request The entity being updated + * @returns The updated entity + */ + async updateEntity( + tx: Knex.Transaction, + request: DbEntityRequest, + ): Promise { + const { kind } = request.entity; + const { + uid, + etag: expectedOldEtag, + generation: expectedOldGeneration, + name, + namespace, + } = request.entity.metadata ?? {}; + + // Find existing entities that match the given metadata + let entitySelector: Partial; + if (uid) { + entitySelector = { id: uid }; + } else if (kind && name) { + entitySelector = { + kind, + name: name, + namespace: namespace || null, + }; + } else { + throw new InputError( + 'Must specify either uid, or kind + name + namespace to be able to identify an entity', + ); + } + const oldRows = await tx('entities') + .where(entitySelector) + .select(); + if (oldRows.length !== 1) { + throw new NotFoundError('No matching entity found'); + } + + // Validate the old entity + const oldRow = oldRows[0]; + // The Number cast is here because sqlite reads it as a string, no matter + // what the table actually says + oldRow.generation = Number(oldRow.generation); + if (expectedOldEtag) { + if (expectedOldEtag !== oldRow.etag) { + throw new ConflictError( + `Etag mismatch, expected="${expectedOldEtag}" found="${oldRow.etag}"`, + ); + } + } + if (expectedOldGeneration) { + if (expectedOldGeneration !== oldRow.generation) { + throw new ConflictError( + `Generation mismatch, expected="${expectedOldGeneration}" found="${oldRow.generation}"`, + ); + } + } + + // Build the new shape of the entity + const newEtag = generateEtag(); + const newGeneration = specsAreEqual(oldRow.spec, request.entity.spec) + ? oldRow.generation + : oldRow.generation + 1; + const newEntity = lodash.cloneDeep(request.entity); + newEntity.metadata = Object.assign({}, request.entity.metadata, { + uid: oldRow.id, + etag: newEtag, + generation: newGeneration, + }); + + // Preserve annotations that were set on the old version of the entity, + // unless the new version overwrites them + if (oldRow.metadata) { + const oldMetadata = JSON.parse(oldRow.metadata) as EntityMeta; + if (oldMetadata.annotations) { + newEntity.metadata!.annotations = { + ...oldMetadata.annotations, + ...newEntity.metadata!.annotations, + }; + } + } + + // Store the updated entity; select on the old etag to ensure that we do + // not lose to another writer + const newRow = toEntityRow(request.locationId, newEntity); + const updatedRows = await tx('entities') + .where({ id: oldRow.id, etag: oldRow.etag }) + .update(newRow); + + // If this happens, somebody else changed the entity just now + if (updatedRows !== 1) { + throw new ConflictError(`Failed to update entity`); + } + + await this.updateEntitiesSearch(tx, oldRow.id, newEntity); + return { locationId: request.locationId, entity: newEntity }; + } + + async entities(tx: Knex.Transaction): Promise { + const rows = await tx('entities') .orderBy('namespace', 'name') .select(); - return items.map(entityDbToResponse); + return rows.map(row => toEntityResponse(row)); } - async entity(name: string): Promise { - const items = await this.database('entities') - .where({ name }) + async entity( + tx: Knex.Transaction, + kind: string, + name: string, + namespace?: string, + ): Promise { + const rows = await tx('entities') + .where({ kind, name, namespace: namespace || null }) .select(); - if (!items.length) { - throw new NotFoundError(`Found no entity with name ${name}`); + + if (rows.length !== 1) { + return undefined; } - return entityDbToResponse(items[0]); + + return toEntityResponse(rows[0]); } async addLocation(location: AddDatabaseLocation): Promise { @@ -206,15 +398,20 @@ export class Database { }); } - /* private async updateEntitiesSearch( tx: Knex.Transaction, entityId: string, data: DescriptorEnvelope, ): Promise { - const entries = buildEntitySearch(entityId, data); - await tx('entities_search').where({ entity_id: entityId }).del(); - await tx('entities_search').insert(entries); + try { + const entries = buildEntitySearch(entityId, data); + await tx('entities_search') + .where({ entity_id: entityId }) + .del(); + await tx('entities_search').insert(entries); + } catch { + // ignore intentionally - if this happens, the entity was deleted before + // we got around to writing the entries + } } - */ } diff --git a/plugins/catalog-backend/src/database/DatabaseManager.test.ts b/plugins/catalog-backend/src/database/DatabaseManager.test.ts index 1043733206..2da3c33a00 100644 --- a/plugins/catalog-backend/src/database/DatabaseManager.test.ts +++ b/plugins/catalog-backend/src/database/DatabaseManager.test.ts @@ -14,8 +14,7 @@ * limitations under the License. */ -import { PassThrough } from 'stream'; -import winston from 'winston'; +import { getVoidLogger } from '@backstage/backend-common'; import { ComponentDescriptor, DescriptorParser, @@ -25,16 +24,12 @@ import { import { Database } from './Database'; import { DatabaseManager } from './DatabaseManager'; import { DatabaseLocationUpdateLogStatus, DbLocationsRow } from './types'; +import Knex from 'knex'; describe('DatabaseManager', () => { - const logger = winston.createLogger({ - transports: [new winston.transports.Stream({ stream: new PassThrough() })], - }); - describe('refreshLocations', () => { it('works with no locations added', async () => { const db = ({ - addOrUpdateEntity: jest.fn(), locations: jest.fn().mockResolvedValue([]), } as unknown) as Database; const reader: LocationReader = { @@ -45,33 +40,34 @@ describe('DatabaseManager', () => { }; await expect( - DatabaseManager.refreshLocations(db, reader, parser, logger), + DatabaseManager.refreshLocations(db, reader, parser, getVoidLogger()), ).resolves.toBeUndefined(); expect(reader.read).not.toHaveBeenCalled(); expect(parser.parse).not.toHaveBeenCalled(); }); it('can update a single location', async () => { - const db = ({ - addOrUpdateEntity: jest.fn(), - locations: jest.fn(() => - Promise.resolve([ - { - id: '123', - type: 'some', - target: 'thing', - } as DbLocationsRow, - ]), - ), - addLocationUpdateLogEvent: jest.fn(), - } as unknown) as Database; - + const location: DbLocationsRow = { + id: '123', + type: 'some', + target: 'thing', + }; const desc: ComponentDescriptor = { apiVersion: 'backstage.io/v1beta1', kind: 'Component', metadata: { name: 'c1' }, spec: { type: 'service' }, }; + const tx = (undefined as unknown) as Knex.Transaction; + + const db = ({ + transaction: jest.fn(f => f(tx)), + entity: jest.fn(() => Promise.resolve(undefined)), + addEntity: jest.fn(), + locations: jest.fn(() => Promise.resolve([location])), + addLocationUpdateLogEvent: jest.fn(), + } as Partial) as Database; + const reader: LocationReader = { read: jest.fn(() => Promise.resolve([{ type: 'data', data: desc }])), }; @@ -80,12 +76,12 @@ describe('DatabaseManager', () => { }; await expect( - DatabaseManager.refreshLocations(db, reader, parser, logger), + DatabaseManager.refreshLocations(db, reader, parser, getVoidLogger()), ).resolves.toBeUndefined(); expect(reader.read).toHaveBeenCalledTimes(1); expect(reader.read).toHaveBeenNthCalledWith(1, 'some', 'thing'); - expect(db.addOrUpdateEntity).toHaveBeenCalledTimes(1); - expect(db.addOrUpdateEntity).toHaveBeenNthCalledWith(1, { + expect(db.addEntity).toHaveBeenCalledTimes(1); + expect(db.addEntity).toHaveBeenNthCalledWith(1, undefined, { locationId: '123', entity: expect.objectContaining({ metadata: expect.objectContaining({ name: 'c1' }), @@ -94,8 +90,12 @@ describe('DatabaseManager', () => { }); it('logs successful updates', async () => { + const tx = (undefined as unknown) as Knex.Transaction; + const db = ({ - addOrUpdateEntity: jest.fn(), + transaction: jest.fn(f => f(tx)), + addEntity: jest.fn(), + entity: jest.fn(() => Promise.resolve(undefined)), locations: jest.fn(() => Promise.resolve([ { @@ -122,7 +122,7 @@ describe('DatabaseManager', () => { }; await expect( - DatabaseManager.refreshLocations(db, reader, parser, logger), + DatabaseManager.refreshLocations(db, reader, parser, getVoidLogger()), ).resolves.toBeUndefined(); expect(db.addLocationUpdateLogEvent).toHaveBeenNthCalledWith( @@ -141,8 +141,11 @@ describe('DatabaseManager', () => { }); it('logs unsuccessful updates when parser fails', async () => { + const tx = (undefined as unknown) as Knex.Transaction; + const db = ({ - addOrUpdateEntity: jest.fn(), + transaction: jest.fn(f => f(tx)), + addEntity: jest.fn(), locations: jest.fn(() => Promise.resolve([ { @@ -171,7 +174,7 @@ describe('DatabaseManager', () => { }; await expect( - DatabaseManager.refreshLocations(db, reader, parser, logger), + DatabaseManager.refreshLocations(db, reader, parser, getVoidLogger()), ).resolves.toBeUndefined(); expect(db.addLocationUpdateLogEvent).toHaveBeenNthCalledWith( @@ -191,8 +194,11 @@ describe('DatabaseManager', () => { }); it('logs unsuccessful updates when reader fails', async () => { + const tx = (undefined as unknown) as Knex.Transaction; + const db = ({ - addOrUpdateEntity: jest.fn(), + transaction: jest.fn(f => f(tx)), + addEntity: jest.fn(), locations: jest.fn(() => Promise.resolve([ { @@ -217,7 +223,7 @@ describe('DatabaseManager', () => { }; await expect( - DatabaseManager.refreshLocations(db, reader, parser, logger), + DatabaseManager.refreshLocations(db, reader, parser, getVoidLogger()), ).resolves.toBeUndefined(); expect(db.addLocationUpdateLogEvent).toHaveBeenNthCalledWith( diff --git a/plugins/catalog-backend/src/database/DatabaseManager.ts b/plugins/catalog-backend/src/database/DatabaseManager.ts index 29b0badb7a..7f646971db 100644 --- a/plugins/catalog-backend/src/database/DatabaseManager.ts +++ b/plugins/catalog-backend/src/database/DatabaseManager.ts @@ -15,19 +15,28 @@ */ import Knex from 'knex'; +import lodash from 'lodash'; import path from 'path'; import { Logger } from 'winston'; -import { DescriptorParser, LocationReader, ParserError } from '../ingestion'; +import { + DescriptorEnvelope, + DescriptorParser, + LocationReader, + ParserError, +} from '../ingestion'; import { Database } from './Database'; import { DatabaseLocationUpdateLogStatus, DbEntityRequest } from './types'; export class DatabaseManager { - public static async createDatabase(database: Knex): Promise { + public static async createDatabase( + database: Knex, + logger: Logger, + ): Promise { await database.migrate.latest({ directory: path.resolve(__dirname, 'migrations'), loadExtensions: ['.js'], }); - return new Database(database); + return new Database(database, logger); } private static async logUpdateSuccess( @@ -66,20 +75,25 @@ export class DatabaseManager { for (const location of locations) { try { logger.debug( - `Refreshing location ${location.id} type "${location.type}" target "${location.target}"`, + `Refreshing location id="${location.id}" type="${location.type}" target="${location.target}"`, ); const readerOutput = await reader.read(location.type, location.target); for (const readerItem of readerOutput) { if (readerItem.type === 'error') { - logger.debug(readerItem.error); + logger.info(readerItem.error); continue; } + try { const entity = await parser.parse(readerItem.data); - const dbc: DbEntityRequest = { locationId: location.id, entity }; - await database.addOrUpdateEntity(dbc); + await DatabaseManager.refreshSingleEntity( + database, + location.id, + entity, + logger, + ); await DatabaseManager.logUpdateSuccess( database, location.id, @@ -98,11 +112,76 @@ export class DatabaseManager { ); } } - await DatabaseManager.logUpdateSuccess(database, location.id); + await DatabaseManager.logUpdateSuccess( + database, + location.id, + undefined, + ); } catch (error) { - logger.debug(`Failed to refresh location ${location.id}, ${error}`); + logger.debug( + `Failed to refresh location id="${location.id}", ${error}`, + ); await DatabaseManager.logUpdateFailure(database, location.id, error); } } } + + private static async refreshSingleEntity( + database: Database, + locationId: string, + entity: DescriptorEnvelope, + logger: Logger, + ): Promise { + const { kind } = entity; + const { name, namespace } = entity.metadata || {}; + if (!name) { + throw new Error('Entities without names are not yet supported'); + } + + const request: DbEntityRequest = { + locationId: locationId, + entity: entity, + }; + + logger.debug( + `Read entity kind="${kind}" name="${name}" namespace="${namespace}"`, + ); + + await database.transaction(async tx => { + const previous = await database.entity(tx, kind, name, namespace); + if (!previous) { + logger.debug(`No such entity found, adding`); + await database.addEntity(tx, request); + } else if ( + !DatabaseManager.entitiesAreEqual(previous.entity, request.entity) + ) { + logger.debug(`Different from existing entity, updating`); + await database.updateEntity(tx, request); + } else { + logger.debug(`Equal to existing entity, skipping update`); + } + }); + } + + private static entitiesAreEqual( + first: DescriptorEnvelope, + second: DescriptorEnvelope, + ) { + const firstClone = lodash.cloneDeep(first); + const secondClone = lodash.cloneDeep(second); + + // Remove generated fields + if (firstClone.metadata) { + delete firstClone.metadata.uid; + delete firstClone.metadata.etag; + delete firstClone.metadata.generation; + } + if (secondClone.metadata) { + delete secondClone.metadata.uid; + delete secondClone.metadata.etag; + delete secondClone.metadata.generation; + } + + return lodash.isEqual(firstClone, secondClone); + } } diff --git a/plugins/catalog-backend/src/database/migrations/20200520140700_location_update_log_table.ts b/plugins/catalog-backend/src/database/migrations/20200520140700_location_update_log_table.ts index 594641ead7..b2e1dc0d32 100644 --- a/plugins/catalog-backend/src/database/migrations/20200520140700_location_update_log_table.ts +++ b/plugins/catalog-backend/src/database/migrations/20200520140700_location_update_log_table.ts @@ -27,7 +27,7 @@ export async function up(knex: Knex): Promise { .inTable('locations') .onUpdate('CASCADE') .onDelete('CASCADE'); - table.string('entity_name').notNullable(); + table.string('entity_name').nullable(); }); } diff --git a/plugins/catalog-backend/src/database/search.test.ts b/plugins/catalog-backend/src/database/search.test.ts index 7fe4928f51..26b20d1846 100644 --- a/plugins/catalog-backend/src/database/search.test.ts +++ b/plugins/catalog-backend/src/database/search.test.ts @@ -14,9 +14,9 @@ * limitations under the License. */ +import { DescriptorEnvelope } from '../ingestion'; import { buildEntitySearch, visitEntityPart } from './search'; import { DbEntitiesSearchRow } from './types'; -import { DescriptorEnvelope } from '../ingestion'; describe('search', () => { describe('visitEntityPart', () => { diff --git a/plugins/catalog-backend/src/ingestion/LocationReaders.ts b/plugins/catalog-backend/src/ingestion/LocationReaders.ts index 80f58aed6a..aa88d299e4 100644 --- a/plugins/catalog-backend/src/ingestion/LocationReaders.ts +++ b/plugins/catalog-backend/src/ingestion/LocationReaders.ts @@ -16,8 +16,7 @@ import { FileLocationSource } from './sources/FileLocationSource'; import { GitHubLocationSource } from './sources/GitHubLocationSource'; -import { LocationSource } from './sources/types'; -import { LocationReader, ReaderOutput } from './types'; +import { LocationReader, LocationSource, ReaderOutput } from './types'; export class LocationReaders implements LocationReader { static create(): LocationReader { diff --git a/plugins/catalog-backend/src/ingestion/descriptors/ComponentDescriptorV1beta1Parser.ts b/plugins/catalog-backend/src/ingestion/descriptors/ComponentDescriptorV1beta1Parser.ts index 8d756ac010..974b34aa8e 100644 --- a/plugins/catalog-backend/src/ingestion/descriptors/ComponentDescriptorV1beta1Parser.ts +++ b/plugins/catalog-backend/src/ingestion/descriptors/ComponentDescriptorV1beta1Parser.ts @@ -15,8 +15,7 @@ */ import * as yup from 'yup'; -import { DescriptorEnvelope, ParserError } from '../types'; -import { KindParser } from './types'; +import { DescriptorEnvelope, KindParser, ParserError } from '../types'; export interface ComponentDescriptorV1beta1 extends DescriptorEnvelope { spec: { diff --git a/plugins/catalog-backend/src/ingestion/descriptors/DescriptorEnvelopeParser.ts b/plugins/catalog-backend/src/ingestion/descriptors/DescriptorEnvelopeParser.ts index ccf5fc3051..1f21e2df9e 100644 --- a/plugins/catalog-backend/src/ingestion/descriptors/DescriptorEnvelopeParser.ts +++ b/plugins/catalog-backend/src/ingestion/descriptors/DescriptorEnvelopeParser.ts @@ -89,7 +89,7 @@ export class DescriptorEnvelopeParser { ); const labelsSchema = yup - .object() + .object>() .notRequired() .test({ name: 'metadata.labels.keys', @@ -113,7 +113,7 @@ export class DescriptorEnvelopeParser { }); const annotationsSchema = yup - .object() + .object>() .notRequired() .test({ name: 'metadata.annotations.keys', diff --git a/plugins/catalog-backend/src/ingestion/descriptors/types.ts b/plugins/catalog-backend/src/ingestion/descriptors/types.ts deleted file mode 100644 index 92b5fdab11..0000000000 --- a/plugins/catalog-backend/src/ingestion/descriptors/types.ts +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Copyright 2020 Spotify AB - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { DescriptorEnvelope } from '../types'; - -/** - * Parses and validates a single envelope into its materialized kind. - * - * These parsers may assume that the envelope is already validated and well - * formed. - */ -export type KindParser = { - /** - * Try to parse an envelope into a materialized kind. - * - * @param envelope A valid descriptor envelope - * @returns A materialized type, or undefined if the given version/kind is - * not meant to be handled by this parser - * @throws An Error if the type was handled and found to not be properly - * formatted - */ - tryParse( - envelope: DescriptorEnvelope, - ): Promise; -}; diff --git a/plugins/catalog-backend/src/ingestion/sources/FileLocationSource.ts b/plugins/catalog-backend/src/ingestion/sources/FileLocationSource.ts index 67d889e7e5..9d2794ec4f 100644 --- a/plugins/catalog-backend/src/ingestion/sources/FileLocationSource.ts +++ b/plugins/catalog-backend/src/ingestion/sources/FileLocationSource.ts @@ -15,8 +15,7 @@ */ import fs from 'fs-extra'; -import { ReaderOutput } from '../types'; -import { LocationSource } from './types'; +import { LocationSource, ReaderOutput } from '../types'; import { readDescriptorYaml } from './util'; export class FileLocationSource implements LocationSource { diff --git a/plugins/catalog-backend/src/ingestion/types.ts b/plugins/catalog-backend/src/ingestion/types.ts index 52817737ac..e5083a65f1 100644 --- a/plugins/catalog-backend/src/ingestion/types.ts +++ b/plugins/catalog-backend/src/ingestion/types.ts @@ -18,6 +18,70 @@ import { ComponentDescriptorV1beta1 } from './descriptors/ComponentDescriptorV1b export type ComponentDescriptor = ComponentDescriptorV1beta1; +/** + * Metadata fields common to all versions/kinds of entity. + * + * @see https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#objectmeta-v1-meta + */ +export type EntityMeta = { + /** + * A globally unique ID for the entity. + * + * This field can not be set by the user at creation time, and the server + * will reject an attempt to do so. The field will be populated in read + * operations. The field can (optionally) be specified when performing + * update or delete operations, but the server is free to reject requests + * that do so in such a way that it breaks semantics. + */ + uid?: string; + + /** + * An opaque string that changes for each update operation to any part of + * the entity, including metadata. + * + * This field can not be set by the user at creation time, and the server + * will reject an attempt to do so. The field will be populated in read + * operations. The field can (optionally) be specified when performing + * update or delete operations, and the server will then reject the + * operation if it does not match the current stored value. + */ + etag?: string; + + /** + * A positive nonzero number that indicates the current generation of data + * for this entity; the value is incremented each time the spec changes. + * + * This field can not be set by the user at creation time, and the server + * will reject an attempt to do so. The field will be populated in read + * operations. + */ + generation?: number; + + /** + * The name of the entity. + * + * Must be uniqe within the catalog at any given point in time, for any + * given namespace, for any given kind. + */ + name?: string; + + /** + * The namespace that the entity belongs to. + */ + namespace?: string; + + /** + * Key/value pairs of identifying information attached to the entity. + */ + labels?: Record; + + /** + * Key/value pairs of non-identifying auxiliary information attached to the + * entity. + */ + annotations?: Record; +}; + /** * The format envelope that's common to all versions/kinds. * @@ -37,67 +101,8 @@ export type DescriptorEnvelope = { /** * Optional metadata related to the entity. - * - * @see https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#objectmeta-v1-meta */ - metadata?: { - /** - * A globally unique ID for the entity. - * - * This field can not be set by the user at creation time, and the server - * will reject an attempt to do so. The field will be populated in read - * operations. The field can (optionally) be specified when performing - * update or delete operations, but the server is free to reject requests - * that do so in such a way that it breaks semantics. - */ - uid?: string; - - /** - * An opaque string that changes for each update operation to any part of - * the entity, including metadata. - * - * This field can not be set by the user at creation time, and the server - * will reject an attempt to do so. The field will be populated in read - * operations. The field can (optionally) be specified when performing - * update or delete operations, and the server will then reject the - * operation if it does not match the current stored value. - */ - etag?: string; - - /** - * A positive nonzero number that indicates the current generation of data - * for this entity; the value is incremented each time the spec changes. - * - * This field can not be set by the user at creation time, and the server - * will reject an attempt to do so. The field will be populated in read - * operations. - */ - generation?: number; - - /** - * The name of the entity. - * - * Must be uniqe within the catalog at any given point in time, for any - * given namespace, for any given kind. - */ - name?: string; - - /** - * The namespace that the entity belongs to. - */ - namespace?: string; - - /** - * Key/value pairs of identifying information attached to the entity. - */ - labels?: object; - - /** - * Key/value pairs of non-identifying auxiliary information attached to the - * entity. - */ - annotations?: object; - }; + metadata?: EntityMeta; /** * The specification data describing the entity itself. diff --git a/plugins/catalog-backend/src/service/router.ts b/plugins/catalog-backend/src/service/router.ts index 5bcf58b942..af714e4026 100644 --- a/plugins/catalog-backend/src/service/router.ts +++ b/plugins/catalog-backend/src/service/router.ts @@ -38,16 +38,10 @@ export async function createRouter( if (entitiesCatalog) { // Entities - router - .get('/entities', async (_req, res) => { - const entities = await entitiesCatalog.entities(); - res.status(200).send(entities); - }) - .get('/entities/:id', async (req, res) => { - const { id } = req.params; - const entity = await entitiesCatalog.entity(id); - res.status(200).send(entity); - }); + router.get('/entities', async (_req, res) => { + const entities = await entitiesCatalog.entities(); + res.status(200).send(entities); + }); } // Locations From 9a0885304eb54de144b613d58ad2ab9868038a85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 25 May 2020 14:19:29 +0200 Subject: [PATCH 10/12] Unbreak build --- .../src/ingestion/sources/GitHubLocationSource.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/plugins/catalog-backend/src/ingestion/sources/GitHubLocationSource.ts b/plugins/catalog-backend/src/ingestion/sources/GitHubLocationSource.ts index ac0a2faa23..69ba4911a6 100644 --- a/plugins/catalog-backend/src/ingestion/sources/GitHubLocationSource.ts +++ b/plugins/catalog-backend/src/ingestion/sources/GitHubLocationSource.ts @@ -15,10 +15,9 @@ */ import fetch from 'node-fetch'; -import { ReaderOutput } from '../types'; -import { LocationSource } from './types'; -import { readDescriptorYaml } from './util'; import { URL } from 'url'; +import { LocationSource, ReaderOutput } from '../types'; +import { readDescriptorYaml } from './util'; // Pointing to raw.githubusercontent.com for now // to be changed in the future, after auth and tokens are done @@ -58,7 +57,7 @@ export class GitHubLocationSource implements LocationSource { let rawYaml; try { - rawYaml = await fetch(url.toString()).then((x) => { + rawYaml = await fetch(url.toString()).then(x => { return x.text(); }); } catch (e) { From cdc5324f1eea3b4370fb43ceb4d83e5f4e6958f8 Mon Sep 17 00:00:00 2001 From: Mutugi <48474421+Mutugiii@users.noreply.github.com> Date: Mon, 25 May 2020 17:25:56 +0300 Subject: [PATCH 11/12] Remove from .dockerignore (Fixes #973) (#998) --- .dockerignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.dockerignore b/.dockerignore index 2fac00fd6d..7f7dee96c5 100644 --- a/.dockerignore +++ b/.dockerignore @@ -1,6 +1,5 @@ .git node_modules packages/*/node_modules -packages/*/dist plugins/*/node_modules plugins/*/dist From fca13afbababca8621fcfceec9e5704870ba417c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 25 May 2020 16:51:39 +0200 Subject: [PATCH 12/12] docs/utility-apis: review fixes --- docs/getting-started/utility-apis.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/getting-started/utility-apis.md b/docs/getting-started/utility-apis.md index 3a223e019b..09dc27ce7a 100644 --- a/docs/getting-started/utility-apis.md +++ b/docs/getting-started/utility-apis.md @@ -3,8 +3,8 @@ ## Introduction Backstage Plugins strive to be self-contained, with as much functionality -as possible residing withing the plugin itself and its backend APIs. There will however -always be a need for plugins to communicate outside of it's boundaries, both with other +as possible residing within the plugin itself and its backend APIs. There will however +always be a need for plugins to communicate outside of its boundaries, both with other plugins and the app itself. Backstage provides two primary methods for plugins to communication across @@ -39,7 +39,7 @@ export const MyComponent: FC<{}> = () => { }; ``` -Note that there is not explicit type given for the `ErrorApi`, this is because the `errorApiRef` has the type embedded, and `useApi` is able to infer the type. +Note that there is no explicit type given for `ErrorApi`. This is because the `errorApiRef` has the type embedded, and `useApi` is able to infer the type. Also note that consuming Utility APIs is not limited to plugins, it can be done from any component inside Backstage, including the ones in `@backstage/core`. The only requirement is that they are beneath the `AppProvider` in the react tree. @@ -101,13 +101,13 @@ implement the `ErrorApi`, as it is checked by the type embedded in the `errorApi ## Defining custom Utility APIs -The pattern for plugins defining their own Utility APIs is not fully established yet. The current way is for the plugin to export it's own `ApiRef` and type for the API, along with one or more implementations. It is then up to the app to import, and register those APIs. See for example the lighthouse or graphiql plugins for examples of this. +The pattern for plugins defining their own Utility APIs is not fully established yet. The current way is for the plugin to export its own `ApiRef` and type for the API, along with one or more implementations. It is then up to the app to import, and register those APIs. See for example the [lighthouse](/plugins/lighthouse/src/api.ts) or [graphiql](/plugins/graphiql/src/lib/api/types.ts) plugins for examples of this. -The goal is to make this process a bit smoother, but that requires work in other parts of Backstage, like configuration management. So it remains as a TODO. If you have more questions regarding this, or have an idea for and API that you want to share outside your plugin, hit us up in GitHub issues or the Backstage Discord server. +The goal is to make this process a bit smoother, but that requires work in other parts of Backstage, like configuration management. So it remains as a TODO. If you have more questions regarding this, or have an idea for an API that you want to share outside your plugin, hit us up in [GitHub issues](https://github.com/spotify/backstage/issues/new/choose) or the [Backstage Discord server](https://discord.gg/EBHEGzX). ## Architecture -The `ApiRef` instances mentioned above provide a point of indirection between consumers and producers +The `ApiRef` instances mentioned above provide a point of indirection between consumers and producers of Utility APIs. It allows for plugins and components to depend on APIs in a type-safe way, without having a direct reference to a concrete implementation of the APIs. The Apps are also given a lot of flexibility in what implementations to provide. As long as they adhere to the contract established @@ -131,7 +131,7 @@ The indirection provided by Utility APIs also makes it straightforward to test c on APIs, and to provide a standard common development environment for plugins. A proper test wrapper with mocked API implementations is not yet ready, but it will provided as a part of `@backstage/test-utils`. It will provide mocked variants of APIs, with additional methods for asserting -a components interaction with the API. +a component's interaction with the API. The common development environment for plugins is included in `@backstage/dev-utils`, where the exported `createDevApp` function creates an application with implementations for all core APIs already present.