diff --git a/.changeset/short-apples-return.md b/.changeset/short-apples-return.md new file mode 100644 index 0000000000..1339de3410 --- /dev/null +++ b/.changeset/short-apples-return.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-app-api': patch +--- + +Added validation during the application startup that detects if there are any plugins present that have not had their required external routes bound. Failing the validation will cause a hard crash as it is a programmer error. It lets you detect early on that there are dangling routes, rather than having them cause an error later on. diff --git a/packages/core-app-api/src/app/AppManager.test.tsx b/packages/core-app-api/src/app/AppManager.test.tsx index b09d424737..920fe1a759 100644 --- a/packages/core-app-api/src/app/AppManager.test.tsx +++ b/packages/core-app-api/src/app/AppManager.test.tsx @@ -545,8 +545,47 @@ describe('Integration Test', () => { }); expect(errorLogs).toEqual([ expect.stringContaining( - 'Parameter :thing is duplicated in path /test/:thing/some/:thing', + 'The above error occurred in the component', ), + ]); + }); + + it('should throw an error when required external plugin routes are not bound', () => { + const app = new AppManager({ + apis: [], + defaultApis: [], + themes: [ + { + id: 'light', + title: 'Light Theme', + variant: 'light', + Provider: ({ children }) => <>{children}, + }, + ], + icons, + plugins: [], + components, + configLoader: async () => [], + }); + + const Provider = app.getProvider(); + const Router = app.getRouter(); + const { error: errorLogs } = withLogCollector(() => { + expect(() => + render( + + + + } /> + + + , + ), + ).toThrow( + /^External route 'extRouteRef1' of the 'blob' plugin must be bound to a target route/, + ); + }); + expect(errorLogs).toEqual([ expect.stringContaining( 'The above error occurred in the component', ), diff --git a/packages/core-app-api/src/app/AppManager.tsx b/packages/core-app-api/src/app/AppManager.tsx index d52e656559..820b4f5c11 100644 --- a/packages/core-app-api/src/app/AppManager.tsx +++ b/packages/core-app-api/src/app/AppManager.tsx @@ -64,7 +64,10 @@ import { } from '../routing/collectors'; import { RoutingProvider } from '../routing/RoutingProvider'; import { RouteTracker } from '../routing/RouteTracker'; -import { validateRoutes } from '../routing/validation'; +import { + validateRouteParameters, + validateRouteBindings, +} from '../routing/validation'; import { AppContextProvider } from './AppContext'; import { AppIdentityProxy } from '../apis/implementations/IdentityApi/AppIdentityProxy'; import { @@ -193,7 +196,7 @@ export class AppManager implements BackstageApp { private readonly themes: AppTheme[]; private readonly configLoader?: AppConfigLoader; private readonly defaultApis: Iterable; - private readonly bindRoutes: AppOptions['bindRoutes']; + private readonly routeBindings: Map; private readonly appIdentityProxy = new AppIdentityProxy(); private readonly apiFactoryRegistry: ApiFactoryRegistry; @@ -206,7 +209,7 @@ export class AppManager implements BackstageApp { this.themes = options.themes as AppTheme[]; this.configLoader = options.configLoader ?? defaultConfigLoader; this.defaultApis = options.defaultApis ?? []; - this.bindRoutes = options.bindRoutes; + this.routeBindings = generateBoundRoutes(options.bindRoutes); this.apiFactoryRegistry = new ApiFactoryRegistry(); } @@ -225,6 +228,9 @@ export class AppManager implements BackstageApp { getProvider(): ComponentType<{}> { const appContext = new AppContextImpl(this); + // We only validate routes once + let routesHaveBeenValidated = false; + const Provider = ({ children }: PropsWithChildren<{}>) => { const appThemeApi = useMemo( () => AppThemeSelector.createWithStorage(this.themes), @@ -245,8 +251,6 @@ export class AppManager implements BackstageApp { }, }); - validateRoutes(result.routePaths, result.routeParents); - // TODO(Rugvip): Restructure the public API so that we can get an immediate view of // the app, rather than having to wait for the provider to render. // For now we need to push the additional plugins we find during @@ -259,6 +263,15 @@ export class AppManager implements BackstageApp { return result; }, [children]); + if (!routesHaveBeenValidated) { + routesHaveBeenValidated = true; + validateRouteParameters(routePaths, routeParents); + validateRouteBindings( + this.routeBindings, + this.plugins as Iterable>, + ); + } + const loadedConfig = useConfigLoader( this.configLoader, this.components, @@ -318,7 +331,7 @@ export class AppManager implements BackstageApp { routePaths={routePaths} routeParents={routeParents} routeObjects={routeObjects} - routeBindings={generateBoundRoutes(this.bindRoutes)} + routeBindings={this.routeBindings} basePath={getBasePath(loadedConfig.api)} > {children} diff --git a/packages/core-app-api/src/routing/RoutingProvider.test.tsx b/packages/core-app-api/src/routing/RoutingProvider.test.tsx index fbdd34ae68..3391bab8b0 100644 --- a/packages/core-app-api/src/routing/RoutingProvider.test.tsx +++ b/packages/core-app-api/src/routing/RoutingProvider.test.tsx @@ -39,7 +39,7 @@ import { routeParentCollector, routeObjectCollector, } from './collectors'; -import { validateRoutes } from './validation'; +import { validateRouteParameters } from './validation'; import { RouteResolver } from './RouteResolver'; import { AnyRouteRef, RouteFunc } from './types'; import { AppContextProvider } from '../app/AppContext'; @@ -323,7 +323,7 @@ describe('discovery', () => { }, }); - expect(() => validateRoutes(routePaths, routeParents)).toThrow( + expect(() => validateRouteParameters(routePaths, routeParents)).toThrow( 'Parameter :id is duplicated in path /foo/:id/bar/:id', ); }); diff --git a/packages/core-app-api/src/routing/validation.ts b/packages/core-app-api/src/routing/validation.ts index 51078a0130..c5d3e4aca9 100644 --- a/packages/core-app-api/src/routing/validation.ts +++ b/packages/core-app-api/src/routing/validation.ts @@ -14,9 +14,16 @@ * limitations under the License. */ +import { + BackstagePlugin, + ExternalRouteRef, + RouteRef, + SubRouteRef, +} from '@backstage/core-plugin-api'; import { AnyRouteRef } from './types'; -export function validateRoutes( +// Validates that there is no duplication of route parameter names +export function validateRouteParameters( routePaths: Map, routeParents: Map, ) { @@ -54,3 +61,30 @@ export function validateRoutes( } } } + +// Validates that all non-optional external routes have been bound +export function validateRouteBindings( + routeBindings: Map, + plugins: Iterable>>, +) { + for (const plugin of plugins) { + if (!plugin.externalRoutes) { + continue; + } + + for (const [name, externalRouteRef] of Object.entries( + plugin.externalRoutes, + )) { + if (externalRouteRef.optional) { + continue; + } + + if (!routeBindings.has(externalRouteRef)) { + throw new Error( + `External route '${name}' of the '${plugin.getId()}' plugin must be bound to a target route. ` + + 'See https://backstage.io/link?bind-routes for details.', + ); + } + } + } +}