diff --git a/.changeset/isolate-invalid-feature-flags.md b/.changeset/isolate-invalid-feature-flags.md new file mode 100644 index 0000000000..eb20fb85ca --- /dev/null +++ b/.changeset/isolate-invalid-feature-flags.md @@ -0,0 +1,5 @@ +--- +'@backstage/frontend-app-api': patch +--- + +Invalid feature flag declarations no longer crash the app during bootstrap. They are now reported through the error collector and skipped, letting the rest of the app load normally. diff --git a/packages/frontend-app-api/report.api.md b/packages/frontend-app-api/report.api.md index b6b502e2da..d3026bf538 100644 --- a/packages/frontend-app-api/report.api.md +++ b/packages/frontend-app-api/report.api.md @@ -148,6 +148,13 @@ export type AppErrorTypes = { bootstrapPluginId: string; }; }; + FEATURE_FLAG_INVALID: { + context: { + pluginId: string; + flagName: string; + error: Error; + }; + }; ROUTE_DUPLICATE: { context: { routeId: string; diff --git a/packages/frontend-app-api/src/wiring/apiFactories.test.ts b/packages/frontend-app-api/src/wiring/apiFactories.test.ts new file mode 100644 index 0000000000..34fa2fa3ee --- /dev/null +++ b/packages/frontend-app-api/src/wiring/apiFactories.test.ts @@ -0,0 +1,295 @@ +/* + * Copyright 2025 The Backstage Authors + * + * 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 { + createFrontendPlugin, + createFrontendModule, + featureFlagsApiRef, + createApiRef, + type FeatureFlagsApi, +} from '@backstage/frontend-plugin-api'; +import { + registerFeatureFlagDeclarationsInHolder, + wrapFeatureFlagApiFactory, +} from './apiFactories'; +import { createErrorCollector } from './createErrorCollector'; + +function createValidatingMockFeatureFlagsApi(): FeatureFlagsApi & { + flags: Array<{ name: string; pluginId: string }>; +} { + const flagNameRegex = /^[a-z]+[a-z0-9-]+$/; + const flags = new Array<{ name: string; pluginId: string }>(); + return { + flags, + registerFlag(flag) { + if (!flagNameRegex.test(flag.name)) { + throw new Error( + `The '${flag.name}' feature flag must start with a lowercase letter and only contain lowercase letters, numbers and hyphens.`, + ); + } + flags.push(flag); + }, + getRegisteredFlags() { + return flags; + }, + isActive() { + return false; + }, + save() {}, + }; +} + +describe('registerFeatureFlagDeclarationsInHolder', () => { + it('should isolate invalid flags and still register valid ones from a plugin', () => { + const featureFlagsApi = createValidatingMockFeatureFlagsApi(); + const collector = createErrorCollector(); + + const plugin = createFrontendPlugin({ + pluginId: 'test', + featureFlags: [ + { name: 'valid-flag' }, + { name: 'bad/flag' }, + { name: 'another-valid' }, + ], + extensions: [], + }); + + registerFeatureFlagDeclarationsInHolder( + { + get: ref => + (ref.id === featureFlagsApiRef.id + ? featureFlagsApi + : undefined) as any, + }, + [plugin], + collector, + ); + + expect(featureFlagsApi.flags).toEqual([ + expect.objectContaining({ name: 'valid-flag', pluginId: 'test' }), + expect.objectContaining({ name: 'another-valid', pluginId: 'test' }), + ]); + + const errors = collector.collectErrors()!; + expect(errors).toHaveLength(1); + expect(errors[0]).toMatchObject({ + code: 'FEATURE_FLAG_INVALID', + context: { pluginId: 'test', flagName: 'bad/flag' }, + }); + expect(errors[0].message).toContain("Plugin 'test'"); + expect(errors[0].message).toContain("'bad/flag'"); + }); + + it('should report each invalid flag independently across multiple plugins', () => { + const featureFlagsApi = createValidatingMockFeatureFlagsApi(); + const collector = createErrorCollector(); + + const pluginA = createFrontendPlugin({ + pluginId: 'alpha', + featureFlags: [{ name: 'ok-flag' }, { name: 'UPPER' }], + extensions: [], + }); + const pluginB = createFrontendPlugin({ + pluginId: 'beta', + featureFlags: [{ name: 'x/y' }, { name: 'good-one' }], + extensions: [], + }); + + registerFeatureFlagDeclarationsInHolder( + { + get: ref => + (ref.id === featureFlagsApiRef.id + ? featureFlagsApi + : undefined) as any, + }, + [pluginA, pluginB], + collector, + ); + + expect(featureFlagsApi.flags).toEqual([ + expect.objectContaining({ name: 'ok-flag', pluginId: 'alpha' }), + expect.objectContaining({ name: 'good-one', pluginId: 'beta' }), + ]); + + const errors = collector.collectErrors()!; + expect(errors).toHaveLength(2); + expect(errors).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + code: 'FEATURE_FLAG_INVALID', + context: expect.objectContaining({ + pluginId: 'alpha', + flagName: 'UPPER', + }), + }), + expect.objectContaining({ + code: 'FEATURE_FLAG_INVALID', + context: expect.objectContaining({ + pluginId: 'beta', + flagName: 'x/y', + }), + }), + ]), + ); + }); + + it('should isolate invalid flags declared by a frontend module', () => { + const featureFlagsApi = createValidatingMockFeatureFlagsApi(); + const collector = createErrorCollector(); + + const mod = createFrontendModule({ + pluginId: 'my-plugin', + featureFlags: [{ name: 'mod-valid' }, { name: 'mod/invalid' }], + extensions: [], + }); + + registerFeatureFlagDeclarationsInHolder( + { + get: ref => + (ref.id === featureFlagsApiRef.id + ? featureFlagsApi + : undefined) as any, + }, + [mod], + collector, + ); + + expect(featureFlagsApi.flags).toEqual([ + expect.objectContaining({ name: 'mod-valid', pluginId: 'my-plugin' }), + ]); + + const errors = collector.collectErrors()!; + expect(errors).toHaveLength(1); + expect(errors[0]).toMatchObject({ + code: 'FEATURE_FLAG_INVALID', + context: { pluginId: 'my-plugin', flagName: 'mod/invalid' }, + }); + }); + + it('should isolate non-validation errors thrown by registerFlag', () => { + const featureFlagsApi: FeatureFlagsApi = { + registerFlag() { + throw new Error('database connection lost'); + }, + getRegisteredFlags: () => [], + isActive: () => false, + save() {}, + }; + const collector = createErrorCollector(); + + const plugin = createFrontendPlugin({ + pluginId: 'broken', + featureFlags: [{ name: 'any-flag' }], + extensions: [], + }); + + registerFeatureFlagDeclarationsInHolder( + { + get: ref => + (ref.id === featureFlagsApiRef.id + ? featureFlagsApi + : undefined) as any, + }, + [plugin], + collector, + ); + + const errors = collector.collectErrors()!; + expect(errors).toHaveLength(1); + expect(errors[0]).toMatchObject({ + code: 'FEATURE_FLAG_INVALID', + context: expect.objectContaining({ + pluginId: 'broken', + flagName: 'any-flag', + }), + }); + expect(errors[0].message).toContain('database connection lost'); + }); +}); + +describe('wrapFeatureFlagApiFactory', () => { + it('should not wrap factories for non-feature-flag APIs', () => { + const otherApiRef = createApiRef<{}>({ id: 'test.other' }); + const factory = { + api: otherApiRef, + deps: {}, + factory: () => ({}), + }; + const collector = createErrorCollector(); + + const result = wrapFeatureFlagApiFactory(factory, [], collector); + expect(result).toBe(factory); + }); + + it('should wrap the feature flags factory and isolate invalid flags', () => { + const featureFlagsApi = createValidatingMockFeatureFlagsApi(); + const collector = createErrorCollector(); + + const plugin = createFrontendPlugin({ + pluginId: 'test', + featureFlags: [{ name: 'good-flag' }, { name: 'bad/flag' }], + extensions: [], + }); + + const factory = { + api: featureFlagsApiRef, + deps: {}, + factory: () => featureFlagsApi, + }; + + const wrapped = wrapFeatureFlagApiFactory(factory, [plugin], collector); + expect(wrapped).not.toBe(factory); + + const result = wrapped.factory({}); + expect(result).toBe(featureFlagsApi); + expect(featureFlagsApi.flags).toEqual([ + expect.objectContaining({ name: 'good-flag', pluginId: 'test' }), + ]); + + const errors = collector.collectErrors()!; + expect(errors).toHaveLength(1); + expect(errors[0]).toMatchObject({ + code: 'FEATURE_FLAG_INVALID', + context: { pluginId: 'test', flagName: 'bad/flag' }, + }); + }); + + it('should not throw from the wrapped factory when flags are invalid', () => { + const featureFlagsApi = createValidatingMockFeatureFlagsApi(); + const collector = createErrorCollector(); + + const plugin = createFrontendPlugin({ + pluginId: 'crashy', + featureFlags: [{ name: 'inspt/show-test-banner' }], + extensions: [], + }); + + const factory = { + api: featureFlagsApiRef, + deps: {}, + factory: () => featureFlagsApi, + }; + + const wrapped = wrapFeatureFlagApiFactory(factory, [plugin], collector); + + expect(() => wrapped.factory({})).not.toThrow(); + expect(featureFlagsApi.flags).toEqual([]); + + const errors = collector.collectErrors()!; + expect(errors).toHaveLength(1); + expect(errors[0].code).toBe('FEATURE_FLAG_INVALID'); + }); +}); diff --git a/packages/frontend-app-api/src/wiring/apiFactories.ts b/packages/frontend-app-api/src/wiring/apiFactories.ts index 6ccc75b9c1..3196c5de46 100644 --- a/packages/frontend-app-api/src/wiring/apiFactories.ts +++ b/packages/frontend-app-api/src/wiring/apiFactories.ts @@ -51,10 +51,11 @@ export type ApiFactoryEntry = { export function registerFeatureFlagDeclarationsInHolder( apis: ApiHolder, features: FrontendFeature[], + collector: ErrorCollector, ) { const featureFlagApi = apis.get(featureFlagsApiRef); if (featureFlagApi) { - registerFeatureFlagDeclarations(featureFlagApi, features); + registerFeatureFlagDeclarations(featureFlagApi, features, collector); } } @@ -65,6 +66,7 @@ export function registerFeatureFlagDeclarationsInHolder( export function wrapFeatureFlagApiFactory( factory: AnyApiFactory, features: FrontendFeature[], + collector: ErrorCollector, ) { if (factory.api.id !== featureFlagsApiRef.id) { return factory; @@ -76,7 +78,7 @@ export function wrapFeatureFlagApiFactory( const featureFlagApi = factory.factory( deps, ) as typeof featureFlagsApiRef.T; - registerFeatureFlagDeclarations(featureFlagApi, features); + registerFeatureFlagDeclarations(featureFlagApi, features, collector); return featureFlagApi; }, } as AnyApiFactory; @@ -137,7 +139,11 @@ export function syncFinalApiFactories(options: { return true; }); const changedFactories = changedEntries.map(entry => - wrapFeatureFlagApiFactory(entry.factory, options.features), + wrapFeatureFlagApiFactory( + entry.factory, + options.features, + options.collector, + ), ); options.appApiRegistry.setAll(changedFactories); options.apiResolver.invalidate( @@ -174,25 +180,45 @@ const EMPTY_API_HOLDER: ApiHolder = { function registerFeatureFlagDeclarations( featureFlagApi: typeof featureFlagsApiRef.T, features: FrontendFeature[], + collector: ErrorCollector, ) { for (const feature of features) { if (OpaqueFrontendPlugin.isType(feature)) { - OpaqueFrontendPlugin.toInternal(feature).featureFlags.forEach(flag => - featureFlagApi.registerFlag({ - name: flag.name, - description: flag.description, - pluginId: feature.id, - }), - ); + const pluginId = feature.id; + for (const flag of OpaqueFrontendPlugin.toInternal(feature) + .featureFlags) { + try { + featureFlagApi.registerFlag({ + name: flag.name, + description: flag.description, + pluginId, + }); + } catch (error) { + collector.report({ + code: 'FEATURE_FLAG_INVALID', + message: `Plugin '${pluginId}' declared invalid feature flag '${flag.name}': ${error}`, + context: { pluginId, flagName: flag.name, error: error as Error }, + }); + } + } } if (isInternalFrontendModule(feature)) { - toInternalFrontendModule(feature).featureFlags.forEach(flag => - featureFlagApi.registerFlag({ - name: flag.name, - description: flag.description, - pluginId: feature.pluginId, - }), - ); + const pluginId = feature.pluginId; + for (const flag of toInternalFrontendModule(feature).featureFlags) { + try { + featureFlagApi.registerFlag({ + name: flag.name, + description: flag.description, + pluginId, + }); + } catch (error) { + collector.report({ + code: 'FEATURE_FLAG_INVALID', + message: `Plugin '${pluginId}' declared invalid feature flag '${flag.name}': ${error}`, + context: { pluginId, flagName: flag.name, error: error as Error }, + }); + } + } } } } diff --git a/packages/frontend-app-api/src/wiring/createErrorCollector.ts b/packages/frontend-app-api/src/wiring/createErrorCollector.ts index 7077e0e9c4..5cf839b286 100644 --- a/packages/frontend-app-api/src/wiring/createErrorCollector.ts +++ b/packages/frontend-app-api/src/wiring/createErrorCollector.ts @@ -97,6 +97,9 @@ export type AppErrorTypes = { bootstrapPluginId: string; }; }; + FEATURE_FLAG_INVALID: { + context: { pluginId: string; flagName: string; error: Error }; + }; // routing ROUTE_DUPLICATE: { context: { routeId: string }; diff --git a/packages/frontend-app-api/src/wiring/prepareSpecializedApp.tsx b/packages/frontend-app-api/src/wiring/prepareSpecializedApp.tsx index 3a09804a18..517abe3669 100644 --- a/packages/frontend-app-api/src/wiring/prepareSpecializedApp.tsx +++ b/packages/frontend-app-api/src/wiring/prepareSpecializedApp.tsx @@ -342,7 +342,7 @@ export function prepareSpecializedApp( if (providedApis) { // Reused session state already carries a fully prepared API holder, so the // bootstrap path only needs to register feature flag declarations on top. - registerFeatureFlagDeclarationsInHolder(providedApis, features); + registerFeatureFlagDeclarationsInHolder(providedApis, features, collector); } else { // Bootstrap materializes only the immediately visible API factories. Any // predicate-gated API roots are revisited during finalization. @@ -355,7 +355,7 @@ export function prepareSpecializedApp( }); const apiFactories = Array.from( bootstrapApiFactoryEntries.values(), - entry => wrapFeatureFlagApiFactory(entry.factory, features), + entry => wrapFeatureFlagApiFactory(entry.factory, features, collector), ); appApiRegistry.registerAll(apiFactories); }