From c8d8a1de053187649cf7c0a61422517e85520bc1 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 20 Dec 2024 12:08:32 +0100 Subject: [PATCH 1/7] fix: omit collecting backend-common package if backend-defaults is included Signed-off-by: blam --- .../config-loader/src/schema/collect.test.ts | 55 +++++++++++++++++ packages/config-loader/src/schema/collect.ts | 61 ++++++++++++------- .../config-loader/src/schema/compile.test.ts | 16 +++++ packages/config-loader/src/schema/types.ts | 4 ++ 4 files changed, 115 insertions(+), 21 deletions(-) diff --git a/packages/config-loader/src/schema/collect.test.ts b/packages/config-loader/src/schema/collect.test.ts index 8c6d1c6b90..1e5c3c2bc5 100644 --- a/packages/config-loader/src/schema/collect.test.ts +++ b/packages/config-loader/src/schema/collect.test.ts @@ -93,6 +93,61 @@ describe('collectConfigSchemas', () => { ]); }); + it('should not include schemas for backend-common if theres a backend-defaults package', async () => { + mockDir.setContent({ + root: { + 'package.json': JSON.stringify({ + name: 'root', + configSchema: mockSchema, + dependencies: {}, + }), + node_modules: { + a: { + 'package.json': JSON.stringify({ + name: 'a', + dependencies: { + b: '0.0.0', + }, + }), + }, + b: { + 'package.json': JSON.stringify({ + name: 'a', + dependencies: { + b: '0.0.0', + }, + }), + }, + '@backstage': { + 'backend-common': { + 'package.json': JSON.stringify({ + name: '@backstage/backend-common', + configSchema: { ...mockSchema, title: 'backend-common' }, + }), + }, + 'backend-defaults': { + 'package.json': JSON.stringify({ + name: '@backstage/backend-defaults', + configSchema: { ...mockSchema, title: 'backend-defaults' }, + }), + }, + }, + }, + }, + }); + + process.chdir(mockDir.path); + + await expect( + collectConfigSchemas([], [path.join('root', 'package.json')]), + ).resolves.toEqual([ + { + path: path.join('root', 'package.json'), + value: mockSchema, + }, + ]); + }); + it('should find schema in transitive dependencies and explicit path', async () => { mockDir.setContent({ root: { diff --git a/packages/config-loader/src/schema/collect.ts b/packages/config-loader/src/schema/collect.ts index 7120c12d90..bf0fb8dd05 100644 --- a/packages/config-loader/src/schema/collect.ts +++ b/packages/config-loader/src/schema/collect.ts @@ -45,7 +45,7 @@ export async function collectConfigSchemas( packagePaths: string[], ): Promise { const schemas = new Array(); - const tsSchemaPaths = new Array(); + const tsSchemaPaths = new Array<{ packageName: string; path: string }>(); const visitedPackageVersions = new Map>(); // pkgName: [versions...] const currentDir = await fs.realpath(process.cwd()); @@ -115,22 +115,25 @@ export async function collectConfigSchemas( ); } if (isDts) { - tsSchemaPaths.push( - relativePath( + tsSchemaPaths.push({ + path: relativePath( currentDir, resolvePath(dirname(pkgPath), pkg.configSchema), ), - ); + packageName: pkg.name, + }); } else { const path = resolvePath(dirname(pkgPath), pkg.configSchema); const value = await fs.readJson(path); schemas.push({ + packageName: pkg.name, value, path: relativePath(currentDir, path), }); } } else { schemas.push({ + packageName: pkg.name, value: pkg.configSchema, path: relativePath(currentDir, pkgPath), }); @@ -151,14 +154,27 @@ export async function collectConfigSchemas( const tsSchemas = await compileTsSchemas(tsSchemaPaths); - return schemas.concat(tsSchemas); + return schemas + .concat(tsSchemas) + .filter( + ({ packageName }, _, original) => + true || + !( + packageName === '@backstage/backend-common' && + original.some( + ({ packageName: p }) => p === '@backstage/backend-defaults', + ) + ), + ); } // This handles the support of TypeScript .d.ts config schema declarations. // We collect all typescript schema definition and compile them all in one go. // This is much faster than compiling them separately. -async function compileTsSchemas(paths: string[]) { - if (paths.length === 0) { +async function compileTsSchemas( + entries: { path: string; packageName: string }[], +) { + if (entries.length === 0) { return []; } @@ -168,20 +184,23 @@ async function compileTsSchemas(paths: string[]) { 'typescript-json-schema' ); - const program = getProgramFromFiles(paths, { - incremental: false, - isolatedModules: true, - lib: ['ES5'], // Skipping most libs speeds processing up a lot, we just need the primitive types anyway - noEmit: true, - noResolve: true, - skipLibCheck: true, // Skipping lib checks speeds things up - skipDefaultLibCheck: true, - strict: true, - typeRoots: [], // Do not include any additional types - types: [], - }); + const program = getProgramFromFiles( + entries.map(({ path }) => path), + { + incremental: false, + isolatedModules: true, + lib: ['ES5'], // Skipping most libs speeds processing up a lot, we just need the primitive types anyway + noEmit: true, + noResolve: true, + skipLibCheck: true, // Skipping lib checks speeds things up + skipDefaultLibCheck: true, + strict: true, + typeRoots: [], // Do not include any additional types + types: [], + }, + ); - const tsSchemas = paths.map(path => { + const tsSchemas = entries.map(({ path, packageName }) => { let value; try { const generator = buildGenerator( @@ -228,7 +247,7 @@ async function compileTsSchemas(paths: string[]) { if (!value) { throw new Error(`Invalid schema in ${path}, missing Config export`); } - return { path, value }; + return { path, value, packageName }; }); return tsSchemas; diff --git a/packages/config-loader/src/schema/compile.test.ts b/packages/config-loader/src/schema/compile.test.ts index ff0a9543aa..b39d2537d1 100644 --- a/packages/config-loader/src/schema/compile.test.ts +++ b/packages/config-loader/src/schema/compile.test.ts @@ -21,10 +21,12 @@ describe('compileConfigSchemas', () => { const validate = compileConfigSchemas([ { path: 'a', + packageName: 'a', value: { type: 'object', properties: { a: { type: 'string' } } }, }, { path: 'b', + packageName: 'b', value: { type: 'object', properties: { b: { type: 'number' } } }, }, ]); @@ -64,6 +66,7 @@ describe('compileConfigSchemas', () => { const validate = compileConfigSchemas([ { path: 'a1', + packageName: 'a1', value: { type: 'object', properties: { @@ -80,6 +83,7 @@ describe('compileConfigSchemas', () => { }, { path: 'a2', + packageName: 'a2', value: { type: 'object', properties: { @@ -126,6 +130,7 @@ describe('compileConfigSchemas', () => { compileConfigSchemas([ { path: 'a1', + packageName: 'a1', value: { type: 'object', properties: { a: { type: 'string', visibility: 'frontend' } }, @@ -133,6 +138,7 @@ describe('compileConfigSchemas', () => { }, { path: 'a2', + packageName: 'a2', value: { type: 'object', properties: { a: { type: 'string', visibility: 'secret' } }, @@ -148,6 +154,7 @@ describe('compileConfigSchemas', () => { const validate = compileConfigSchemas([ { path: 'a1', + packageName: 'a1', value: { type: 'object', properties: { @@ -179,6 +186,7 @@ describe('compileConfigSchemas', () => { const validate = compileConfigSchemas([ { path: 'a1', + packageName: 'a1', value: { type: 'object', properties: { @@ -242,6 +250,7 @@ describe('deepVisibility', () => { const validate = compileConfigSchemas([ { path: 'a1', + packageName: 'a1', value: { type: 'object', properties: { @@ -257,6 +266,7 @@ describe('deepVisibility', () => { }, { path: 'a2', + packageName: 'a2', value: { type: 'object', deepVisibility: 'secret', @@ -305,6 +315,7 @@ describe('deepVisibility', () => { compileConfigSchemas([ { path: 'a1', + packageName: 'a1', value: { type: 'object', properties: { @@ -320,6 +331,7 @@ describe('deepVisibility', () => { }, { path: 'a2', + packageName: 'a2', value: { type: 'object', deepVisibility: 'secret', @@ -346,6 +358,7 @@ describe('deepVisibility', () => { compileConfigSchemas([ { path: 'a2', + packageName: 'a2', value: { type: 'object', deepVisibility: 'secret', @@ -376,6 +389,7 @@ describe('deepVisibility', () => { compileConfigSchemas([ { path: 'a2', + packageName: 'a2', value: { type: 'object', properties: { @@ -398,6 +412,7 @@ describe('deepVisibility', () => { compileConfigSchemas([ { path: 'a1', + packageName: 'a1', value: { type: 'object', properties: { @@ -418,6 +433,7 @@ describe('deepVisibility', () => { }, { path: 'a2', + packageName: 'a2', value: { type: 'object', deepVisibility: 'secret', diff --git a/packages/config-loader/src/schema/types.ts b/packages/config-loader/src/schema/types.ts index 8f679c93fc..4ba65245da 100644 --- a/packages/config-loader/src/schema/types.ts +++ b/packages/config-loader/src/schema/types.ts @@ -29,6 +29,10 @@ export type ConfigSchemaPackageEntry = { * The relative path that the configuration schema was discovered at. */ path: string; + /** + * The package name for the package this belongs to + */ + packageName: string; }; /** From b51173266c160086be31b0675d7144c934d7329b Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 20 Dec 2024 13:04:39 +0100 Subject: [PATCH 2/7] chore: fixing tests Signed-off-by: blam --- .../config-loader/src/schema/collect.test.ts | 72 +++++++++++-------- packages/config-loader/src/schema/collect.ts | 1 - 2 files changed, 43 insertions(+), 30 deletions(-) diff --git a/packages/config-loader/src/schema/collect.test.ts b/packages/config-loader/src/schema/collect.test.ts index 1e5c3c2bc5..131c9253c2 100644 --- a/packages/config-loader/src/schema/collect.test.ts +++ b/packages/config-loader/src/schema/collect.test.ts @@ -17,6 +17,7 @@ import { createMockDirectory } from '@backstage/backend-test-utils'; import { collectConfigSchemas } from './collect'; import path from 'path'; +import { dependencies } from 'webpack'; // cwd must be restored const origDir = process.cwd(); @@ -68,6 +69,7 @@ describe('collectConfigSchemas', () => { { path: path.join('node_modules', 'a', 'package.json'), value: mockSchema, + packageName: 'a', }, ]); }); @@ -89,6 +91,7 @@ describe('collectConfigSchemas', () => { { path: path.join('root', 'package.json'), value: mockSchema, + packageName: 'root', }, ]); }); @@ -98,40 +101,29 @@ describe('collectConfigSchemas', () => { root: { 'package.json': JSON.stringify({ name: 'root', - configSchema: mockSchema, - dependencies: {}, + dependencies: { + '@backstage/backend-common': '1', + '@backstage/backend-defaults': '1', + }, + configSchema: { ...mockSchema, title: 'root' }, }), - node_modules: { - a: { + }, + node_modules: { + '@backstage': { + 'backend-common': { 'package.json': JSON.stringify({ - name: 'a', - dependencies: { - b: '0.0.0', - }, + name: '@backstage/backend-common', + version: '1', + configSchema: { ...mockSchema, title: 'backend-common' }, }), }, - b: { + 'backend-defaults': { 'package.json': JSON.stringify({ - name: 'a', - dependencies: { - b: '0.0.0', - }, + name: '@backstage/backend-defaults', + version: '1', + configSchema: { ...mockSchema, title: 'backend-defaults' }, }), }, - '@backstage': { - 'backend-common': { - 'package.json': JSON.stringify({ - name: '@backstage/backend-common', - configSchema: { ...mockSchema, title: 'backend-common' }, - }), - }, - 'backend-defaults': { - 'package.json': JSON.stringify({ - name: '@backstage/backend-defaults', - configSchema: { ...mockSchema, title: 'backend-defaults' }, - }), - }, - }, }, }, }); @@ -139,11 +131,22 @@ describe('collectConfigSchemas', () => { process.chdir(mockDir.path); await expect( - collectConfigSchemas([], [path.join('root', 'package.json')]), + collectConfigSchemas(['root'], [path.join('root', 'package.json')]), ).resolves.toEqual([ { path: path.join('root', 'package.json'), - value: mockSchema, + value: { ...mockSchema, title: 'root' }, + packageName: 'root', + }, + { + path: path.join( + 'node_modules', + '@backstage', + 'backend-defaults', + 'package.json', + ), + value: { ...mockSchema, title: 'backend-defaults' }, + packageName: '@backstage/backend-defaults', }, ]); }); @@ -214,18 +217,22 @@ describe('collectConfigSchemas', () => { { path: path.join('node_modules', 'b', 'package.json'), value: { ...mockSchema, title: 'b' }, + packageName: 'b', }, { path: path.join('node_modules', 'c1', 'package.json'), value: { ...mockSchema, title: 'c1' }, + packageName: 'c1', }, { path: path.join('node_modules', 'd1', 'package.json'), value: { ...mockSchema, title: 'd1' }, + packageName: 'd1', }, { path: path.join('root', 'package.json'), value: { ...mockSchema, title: 'root' }, + packageName: 'root', }, ]), ); @@ -268,10 +275,12 @@ describe('collectConfigSchemas', () => { { path: path.join('node_modules', 'a', 'package.json'), value: { ...mockSchema, title: 'inline' }, + packageName: 'a', }, { path: path.join('node_modules', 'b', 'schema.json'), value: { ...mockSchema, title: 'external' }, + packageName: 'b', }, { path: path.join('node_modules', 'c', 'schema.d.ts'), @@ -286,6 +295,7 @@ describe('collectConfigSchemas', () => { }, required: ['tsKey'], }, + packageName: 'c', }, ]), ); @@ -339,14 +349,17 @@ describe('collectConfigSchemas', () => { { path: path.join('node_modules', 'a', 'package.json'), value: mockSchema, + packageName: 'a', }, { path: path.join('node_modules', 'b', 'package.json'), value: { ...mockSchema, title: 'b' }, + packageName: 'b', }, { path: path.join('node_modules', 'c', 'package.json'), value: { ...mockSchema, title: 'c1' }, + packageName: 'c', }, { path: path.join( @@ -357,6 +370,7 @@ describe('collectConfigSchemas', () => { 'package.json', ), value: { ...mockSchema, title: 'c2' }, + packageName: 'c', }, ]), ); diff --git a/packages/config-loader/src/schema/collect.ts b/packages/config-loader/src/schema/collect.ts index bf0fb8dd05..eb788d1038 100644 --- a/packages/config-loader/src/schema/collect.ts +++ b/packages/config-loader/src/schema/collect.ts @@ -158,7 +158,6 @@ export async function collectConfigSchemas( .concat(tsSchemas) .filter( ({ packageName }, _, original) => - true || !( packageName === '@backstage/backend-common' && original.some( From 8ecf8cb449148bb98f369ab76d46310a1efaf3e8 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 20 Dec 2024 13:06:43 +0100 Subject: [PATCH 3/7] chore: changeset Signed-off-by: blam --- .changeset/metal-ravens-hammer.md | 5 +++++ packages/config-loader/src/schema/collect.test.ts | 1 - 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 .changeset/metal-ravens-hammer.md diff --git a/.changeset/metal-ravens-hammer.md b/.changeset/metal-ravens-hammer.md new file mode 100644 index 0000000000..9a834b4715 --- /dev/null +++ b/.changeset/metal-ravens-hammer.md @@ -0,0 +1,5 @@ +--- +'@backstage/config-loader': patch +--- + +Exclude `@backstage/backend-common` from schema collection if `@backstage/backend-defaults` is present diff --git a/packages/config-loader/src/schema/collect.test.ts b/packages/config-loader/src/schema/collect.test.ts index 131c9253c2..53db5c6da7 100644 --- a/packages/config-loader/src/schema/collect.test.ts +++ b/packages/config-loader/src/schema/collect.test.ts @@ -17,7 +17,6 @@ import { createMockDirectory } from '@backstage/backend-test-utils'; import { collectConfigSchemas } from './collect'; import path from 'path'; -import { dependencies } from 'webpack'; // cwd must be restored const origDir = process.cwd(); From 5c399429d9545ca9e35b6b4b552a55a7abbb3c4d Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 20 Dec 2024 13:50:33 +0100 Subject: [PATCH 4/7] chore: small refactor and some new config.d.ts Signed-off-by: blam --- packages/backend-defaults/config.d.ts | 18 +++++++++++++++++ packages/config-loader/src/schema/collect.ts | 21 ++++++++++---------- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/packages/backend-defaults/config.d.ts b/packages/backend-defaults/config.d.ts index e2a748f85d..0cdd4ef244 100644 --- a/packages/backend-defaults/config.d.ts +++ b/packages/backend-defaults/config.d.ts @@ -22,6 +22,24 @@ export interface Config { }; backend?: { + /** + * Backend configuration for when request authentication is enabled + * + * @deprecated this will be removed when the backwards compatibility is no longer needed with backend-common + */ + auth?: { + /** Keys shared by all backends for signing and validating backend tokens. */ + keys?: { + /** + * Secret for generating tokens. Should be a base64 string, recommended + * length is 24 bytes. + * + * @visibility secret + */ + secret: string; + }[]; + }; + /** * The full base URL of the backend, as seen from the browser's point of * view as it makes calls to the backend. diff --git a/packages/config-loader/src/schema/collect.ts b/packages/config-loader/src/schema/collect.ts index eb788d1038..074248b89b 100644 --- a/packages/config-loader/src/schema/collect.ts +++ b/packages/config-loader/src/schema/collect.ts @@ -153,18 +153,17 @@ export async function collectConfigSchemas( ]); const tsSchemas = await compileTsSchemas(tsSchemaPaths); + const allSchemas = schemas.concat(tsSchemas); - return schemas - .concat(tsSchemas) - .filter( - ({ packageName }, _, original) => - !( - packageName === '@backstage/backend-common' && - original.some( - ({ packageName: p }) => p === '@backstage/backend-defaults', - ) - ), - ); + const isMissingBackendDefaults = !allSchemas.some( + ({ packageName }) => packageName === '@backstage/backend-defaults', + ); + + // Filter out the backend-common schema unless the backend-defaults schema is missing + return allSchemas.filter( + ({ packageName }) => + packageName !== '@backstage/backend-common' || isMissingBackendDefaults, + ); } // This handles the support of TypeScript .d.ts config schema declarations. From b6cb774823dd6b7360df81823c314ddf9527a009 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 20 Dec 2024 13:58:17 +0100 Subject: [PATCH 5/7] chore: fixing config schema again Signed-off-by: blam --- packages/backend-defaults/config.d.ts | 31 +++++++++++---------------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/packages/backend-defaults/config.d.ts b/packages/backend-defaults/config.d.ts index 0cdd4ef244..aa97313921 100644 --- a/packages/backend-defaults/config.d.ts +++ b/packages/backend-defaults/config.d.ts @@ -22,24 +22,6 @@ export interface Config { }; backend?: { - /** - * Backend configuration for when request authentication is enabled - * - * @deprecated this will be removed when the backwards compatibility is no longer needed with backend-common - */ - auth?: { - /** Keys shared by all backends for signing and validating backend tokens. */ - keys?: { - /** - * Secret for generating tokens. Should be a base64 string, recommended - * length is 24 bytes. - * - * @visibility secret - */ - secret: string; - }[]; - }; - /** * The full base URL of the backend, as seen from the browser's point of * view as it makes calls to the backend. @@ -104,6 +86,19 @@ export interface Config { * Options used by the default auth, httpAuth and userInfo services. */ auth?: { + /** + * Keys shared by all backends for signing and validating backend tokens. + * @deprecated this will be removed when the backwards compatibility is no longer needed with backend-common + */ + keys?: { + /** + * Secret for generating tokens. Should be a base64 string, recommended + * length is 24 bytes. + * + * @visibility secret + */ + secret: string; + }[]; /** * This disables the otherwise default auth policy, which requires all * requests to be authenticated with either user or service credentials. From ba840639250cd1647dda46b05c361f0593dbcbe3 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 20 Dec 2024 14:09:47 +0100 Subject: [PATCH 6/7] chore: smol comment Signed-off-by: blam --- .changeset/metal-ravens-hammer.md | 1 + packages/config-loader/src/schema/collect.ts | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.changeset/metal-ravens-hammer.md b/.changeset/metal-ravens-hammer.md index 9a834b4715..e8a97f412c 100644 --- a/.changeset/metal-ravens-hammer.md +++ b/.changeset/metal-ravens-hammer.md @@ -1,5 +1,6 @@ --- '@backstage/config-loader': patch +'@backstage/backend-defaults': patch --- Exclude `@backstage/backend-common` from schema collection if `@backstage/backend-defaults` is present diff --git a/packages/config-loader/src/schema/collect.ts b/packages/config-loader/src/schema/collect.ts index 074248b89b..35ca786a7f 100644 --- a/packages/config-loader/src/schema/collect.ts +++ b/packages/config-loader/src/schema/collect.ts @@ -159,7 +159,10 @@ export async function collectConfigSchemas( ({ packageName }) => packageName === '@backstage/backend-defaults', ); - // Filter out the backend-common schema unless the backend-defaults schema is missing + // Filter out the backend-common schema unless the backend-defaults schema is missing. + // This was causing issue with merging of the schemas. + // This should be removed when we have no need for backend-common anymore, and other + // packages are no longer depending on it. return allSchemas.filter( ({ packageName }) => packageName !== '@backstage/backend-common' || isMissingBackendDefaults, From 4e7f18f604a4033ef70991bc837cd61d6a4d5028 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 20 Dec 2024 14:15:58 +0100 Subject: [PATCH 7/7] chore: simplify Signed-off-by: blam --- packages/config-loader/src/schema/collect.ts | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/config-loader/src/schema/collect.ts b/packages/config-loader/src/schema/collect.ts index 35ca786a7f..c7af64e0f3 100644 --- a/packages/config-loader/src/schema/collect.ts +++ b/packages/config-loader/src/schema/collect.ts @@ -155,18 +155,20 @@ export async function collectConfigSchemas( const tsSchemas = await compileTsSchemas(tsSchemaPaths); const allSchemas = schemas.concat(tsSchemas); - const isMissingBackendDefaults = !allSchemas.some( + const hasBackendDefaults = allSchemas.some( ({ packageName }) => packageName === '@backstage/backend-defaults', ); - // Filter out the backend-common schema unless the backend-defaults schema is missing. - // This was causing issue with merging of the schemas. - // This should be removed when we have no need for backend-common anymore, and other - // packages are no longer depending on it. - return allSchemas.filter( - ({ packageName }) => - packageName !== '@backstage/backend-common' || isMissingBackendDefaults, - ); + if (hasBackendDefaults) { + // We filter out backend-common schemas here to avoid issues with + // schema merging over different versions of the same schema. + // led to issues such as https://github.com/backstage/backstage/issues/28170 + return allSchemas.filter( + ({ packageName }) => packageName !== '@backstage/backend-common', + ); + } + + return allSchemas; } // This handles the support of TypeScript .d.ts config schema declarations.