From 928db6a7f0827789cfd0debb56b7e9b6b8bca842 Mon Sep 17 00:00:00 2001 From: JounQin Date: Mon, 14 Oct 2024 18:46:47 +0800 Subject: [PATCH 01/28] feat: support fetch pod metrics with custom resources Signed-off-by: JounQin --- .changeset/stupid-crabs-wash.md | 5 ++ .../service/KubernetesFanOutHandler.test.ts | 74 +++++++++++++++++++ .../src/service/KubernetesFanOutHandler.ts | 6 +- 3 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 .changeset/stupid-crabs-wash.md diff --git a/.changeset/stupid-crabs-wash.md b/.changeset/stupid-crabs-wash.md new file mode 100644 index 0000000000..d9e7f317df --- /dev/null +++ b/.changeset/stupid-crabs-wash.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-kubernetes-backend': patch +--- + +Support fetch pod metrics with custom resources diff --git a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts index 3491268d39..ac6a59a7c4 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts @@ -31,6 +31,7 @@ import { registerMswTestHooks, } from '@backstage/backend-test-utils'; import { + CustomResourceFetchResponse, FetchResponse, KubernetesRequestAuth, ObjectsByEntityResponse, @@ -1428,5 +1429,78 @@ describe('KubernetesFanOutHandler', () => { }), ); }); + it('fetch pod metrics when pods used', async () => { + getClustersByEntity.mockImplementation(() => + Promise.resolve({ + clusters: [ + { + name: 'test-cluster', + title: 'cluster-title', + url: '', + authMetadata: {}, + }, + ], + }), + ); + + sut = getKubernetesFanOutHandler([]); + + const resources: CustomResourceFetchResponse[] = [ + { + type: 'customresources', + resources: [ + { + apiVersion: 'v1', + kind: 'Pod', + metadata: { + namespace: `ns-test-component-test-cluster`, + }, + }, + ], + }, + ]; + + fetchObjectsForService.mockImplementation(async () => ({ + responses: resources, + errors: [], + })); + + const result = await sut.getCustomResourcesByEntity( + { + entity, + auth: {}, + customResources: [ + { + group: '', + apiVersion: 'v1', + plural: 'pods', + }, + ], + }, + { credentials: mockCredentials }, + ); + + expect(fetchObjectsForService).toHaveBeenCalledTimes(1); + expect(fetchPodMetricsByNamespaces).toHaveBeenCalledTimes(1); + expect(fetchPodMetricsByNamespaces).toHaveBeenCalledWith( + expect.anything(), + { type: 'anonymous' }, + new Set(['ns-test-component-test-cluster']), + expect.anything(), + ); + expect(result).toStrictEqual({ + items: [ + { + cluster: { + name: 'test-cluster', + title: 'cluster-title', + }, + errors: [], + podMetrics: [POD_METRICS_FIXTURE], + resources, + }, + ], + }); + }); }); }); diff --git a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts index 54a038fffd..31c0c91807 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts @@ -147,7 +147,11 @@ export interface KubernetesFanOutHandlerOptions export interface KubernetesRequestBody extends ObjectsByEntityRequest {} const isPodFetchResponse = (fr: FetchResponse): fr is PodFetchResponse => - fr.type === 'pods'; + fr.type === 'pods' || + (fr.type === 'customresources' && + fr.resources.length > 0 && + fr.resources[0].apiVersion === 'v1' && + fr.resources[0].kind === 'Pod'); const isString = (str: string | undefined): str is string => str !== undefined; const numberOrBigIntToNumberOrString = ( From b5e002b1cccc3a6dcb0f1095ae1f4b2e0b94a498 Mon Sep 17 00:00:00 2001 From: Jason Liu Date: Sat, 23 Nov 2024 14:14:17 +1100 Subject: [PATCH 02/28] Make GitHub environment Scaffolder action use auth to resolve reviewers Changes the github:environment:create Scaffolder action to request and use a backend auth token when resolving the reviewer entityRefs from the Backstage catalog. This is because previously it would throw a 401 error when backend auth was not disabled. The logic for requesting the token is copied from the existing catalog:fetch action, which needs to do a similar thing. Also slightly clarifies that Backstage entityRefs are expected in the reviewers list for this action. Signed-off-by: Jason Liu --- .changeset/long-geese-report.md | 5 ++++ .../report.api.md | 2 ++ .../src/actions/githubEnvironment.test.ts | 18 +++++++++++++++ .../src/actions/githubEnvironment.ts | 23 +++++++++++++++---- 4 files changed, 43 insertions(+), 5 deletions(-) create mode 100644 .changeset/long-geese-report.md diff --git a/.changeset/long-geese-report.md b/.changeset/long-geese-report.md new file mode 100644 index 0000000000..1e638703c9 --- /dev/null +++ b/.changeset/long-geese-report.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend-module-github': patch +--- + +Change `github:environment:create` action to request and use a token when resolving reviewer entity refs from the Backstage catalog. diff --git a/plugins/scaffolder-backend-module-github/report.api.md b/plugins/scaffolder-backend-module-github/report.api.md index 547c61768f..e954384444 100644 --- a/plugins/scaffolder-backend-module-github/report.api.md +++ b/plugins/scaffolder-backend-module-github/report.api.md @@ -3,6 +3,7 @@ > Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/). ```ts +import { AuthService } from '@backstage/backend-plugin-api'; import { BackendFeature } from '@backstage/backend-plugin-api'; import { CatalogApi } from '@backstage/catalog-client'; import { Config } from '@backstage/config'; @@ -103,6 +104,7 @@ export function createGithubDeployKeyAction(options: { export function createGithubEnvironmentAction(options: { integrations: ScmIntegrationRegistry; catalogClient?: CatalogApi; + auth?: AuthService; }): TemplateAction< { repoUrl: string; diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts b/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts index 1397942a7c..898e1842f7 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.test.ts @@ -20,6 +20,7 @@ import { TemplateAction } from '@backstage/plugin-scaffolder-node'; import { ConfigReader } from '@backstage/config'; import { ScmIntegrations } from '@backstage/integration'; import { CatalogApi } from '@backstage/catalog-client'; +import { mockCredentials, mockServices } from '@backstage/backend-test-utils'; const mockOctokit = { rest: { @@ -71,6 +72,14 @@ describe('github:environment:create', () => { }); const integrations = ScmIntegrations.fromConfig(config); + + const credentials = mockCredentials.user(); + + const token = mockCredentials.service.token({ + onBehalfOf: credentials, + targetPluginId: 'catalog', + }); + let action: TemplateAction; const mockContext = createMockActionContext({ @@ -78,6 +87,7 @@ describe('github:environment:create', () => { repoUrl: 'github.com?repo=repository&owner=owner', name: 'envname', }, + secrets: { backstageToken: token }, }); beforeEach(() => { @@ -122,6 +132,7 @@ describe('github:environment:create', () => { action = createGithubEnvironmentAction({ integrations, catalogClient: mockCatalogClient as CatalogApi, + auth: mockServices.auth(), }); }); @@ -453,6 +464,13 @@ describe('github:environment:create', () => { }, }); + expect(mockCatalogClient.getEntitiesByRefs).toHaveBeenCalledWith( + { + entityRefs: ['group:default/team-a', 'user:default/johndoe'], + }, + { token }, + ); + expect( mockOctokit.rest.repos.createOrUpdateEnvironment, ).toHaveBeenCalledWith({ diff --git a/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.ts b/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.ts index 29c0a3ce38..ef59b611fe 100644 --- a/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.ts +++ b/plugins/scaffolder-backend-module-github/src/actions/githubEnvironment.ts @@ -26,6 +26,7 @@ import Sodium from 'libsodium-wrappers'; import { examples } from './gitHubEnvironment.examples'; import { CatalogApi } from '@backstage/catalog-client'; import { Entity } from '@backstage/catalog-model'; +import { AuthService } from '@backstage/backend-plugin-api'; /** * Creates an `github:environment:create` Scaffolder action that creates a Github Environment. @@ -35,8 +36,9 @@ import { Entity } from '@backstage/catalog-model'; export function createGithubEnvironmentAction(options: { integrations: ScmIntegrationRegistry; catalogClient?: CatalogApi; + auth?: AuthService; }) { - const { integrations, catalogClient } = options; + const { integrations, catalogClient, auth } = options; // For more information on how to define custom actions, see // https://backstage.io/docs/features/software-templates/writing-custom-actions return createTemplateAction<{ @@ -140,7 +142,8 @@ export function createGithubEnvironmentAction(options: { reviewers: { title: 'Reviewers', type: 'array', - description: 'Reviewers for this environment', + description: + 'Reviewers for this environment. Must be a list of Backstage entity references.', items: { type: 'string', }, @@ -163,6 +166,11 @@ export function createGithubEnvironmentAction(options: { reviewers, } = ctx.input; + const { token } = (await auth?.getPluginRequestToken({ + onBehalfOf: await ctx.getInitiatorCredentials(), + targetPluginId: 'catalog', + })) ?? { token: ctx.secrets?.backstageToken }; + // When environment creation step is executed right after a repo publish step, the repository might not be available immediately. // Add a 2-second delay before initiating the steps in this action. await new Promise(resolve => setTimeout(resolve, 2000)); @@ -190,9 +198,14 @@ export function createGithubEnvironmentAction(options: { if (reviewers) { let reviewersEntityRefs: Array = []; // Fetch reviewers from Catalog - const catalogResponse = await catalogClient?.getEntitiesByRefs({ - entityRefs: reviewers, - }); + const catalogResponse = await catalogClient?.getEntitiesByRefs( + { + entityRefs: reviewers, + }, + { + token, + }, + ); if (catalogResponse?.items?.length) { reviewersEntityRefs = catalogResponse.items; } From 118c930b082355a4aa92c107c14eebc13f7215c6 Mon Sep 17 00:00:00 2001 From: aramissennyeydd Date: Sun, 24 Nov 2024 16:50:55 -0500 Subject: [PATCH 03/28] chore: organize sidebar a bit Signed-off-by: aramissennyeydd --- microsite/sidebars.js | 403 +++++++++++++++++++++++------------------- 1 file changed, 218 insertions(+), 185 deletions(-) diff --git a/microsite/sidebars.js b/microsite/sidebars.js index 954b3630b1..00c728e024 100644 --- a/microsite/sidebars.js +++ b/microsite/sidebars.js @@ -105,7 +105,7 @@ module.exports = { }, { type: 'category', - label: 'Backstage Search', + label: 'Search', items: [ 'features/search/search-overview', 'features/search/getting-started', @@ -116,6 +116,66 @@ module.exports = { 'features/search/how-to-guides', ], }, + { + type: 'category', + label: 'Auth and Identity', + items: [ + 'auth/index', + { + type: 'category', + label: 'Included providers', + items: [ + 'auth/auth0/provider', + 'auth/atlassian/provider', + 'auth/aws-alb/provider', + 'auth/microsoft/provider', + 'auth/microsoft/easy-auth', + 'auth/bitbucket/provider', + 'auth/bitbucketServer/provider', + 'auth/cloudflare/provider', + 'auth/github/provider', + 'auth/gitlab/provider', + 'auth/google/provider', + 'auth/google/gcp-iap-auth', + 'auth/guest/provider', + 'auth/okta/provider', + 'auth/oauth2-proxy/provider', + 'auth/onelogin/provider', + 'auth/vmware-cloud/provider', + ], + }, + 'auth/identity-resolver', + 'auth/oauth', + 'auth/oidc', + 'auth/add-auth-provider', + 'auth/service-to-service-auth', + 'auth/autologout', + 'auth/troubleshooting', + ], + }, + { + type: 'category', + label: 'Permissions', + items: [ + 'permissions/overview', + 'permissions/concepts', + 'permissions/getting-started', + 'permissions/writing-a-policy', + 'permissions/frontend-integration', + 'permissions/custom-rules', + { + type: 'category', + label: 'Tutorial: using Permissions in your plugin', + items: [ + 'permissions/plugin-authors/01-setup', + 'permissions/plugin-authors/02-adding-a-basic-permission-check', + 'permissions/plugin-authors/03-adding-a-resource-permission-check', + 'permissions/plugin-authors/04-authorizing-access-to-paginated-data', + 'permissions/plugin-authors/05-frontend-authorization', + ], + }, + ], + }, { type: 'category', label: 'TechDocs', @@ -136,6 +196,11 @@ module.exports = { 'features/techdocs/faqs', ], }, + { + type: 'category', + label: 'Notifications', + items: ['notifications/index'], + }, ], Integrations: [ 'integrations/index', @@ -276,59 +341,6 @@ module.exports = { 'conf/writing', 'conf/defining', ], - Notifications: ['notifications/index'], - 'Auth and identity': [ - 'auth/index', - { - type: 'category', - label: 'Included providers', - items: [ - 'auth/auth0/provider', - 'auth/atlassian/provider', - 'auth/aws-alb/provider', - 'auth/microsoft/provider', - 'auth/microsoft/easy-auth', - 'auth/bitbucket/provider', - 'auth/bitbucketServer/provider', - 'auth/cloudflare/provider', - 'auth/github/provider', - 'auth/gitlab/provider', - 'auth/google/provider', - 'auth/google/gcp-iap-auth', - 'auth/guest/provider', - 'auth/okta/provider', - 'auth/oauth2-proxy/provider', - 'auth/onelogin/provider', - 'auth/vmware-cloud/provider', - ], - }, - 'auth/identity-resolver', - 'auth/oauth', - 'auth/oidc', - 'auth/add-auth-provider', - 'auth/service-to-service-auth', - 'auth/autologout', - 'auth/troubleshooting', - ], - Permissions: [ - 'permissions/overview', - 'permissions/concepts', - 'permissions/getting-started', - 'permissions/writing-a-policy', - 'permissions/frontend-integration', - 'permissions/custom-rules', - { - type: 'category', - label: 'Tutorial: using Permissions in your plugin', - items: [ - 'permissions/plugin-authors/01-setup', - 'permissions/plugin-authors/02-adding-a-basic-permission-check', - 'permissions/plugin-authors/03-adding-a-resource-permission-check', - 'permissions/plugin-authors/04-authorizing-access-to-paginated-data', - 'permissions/plugin-authors/05-frontend-authorization', - ], - }, - ], Tooling: [ { type: 'category', @@ -350,115 +362,125 @@ module.exports = { }, 'tooling/package-metadata', ], - 'New Backend System': [ - 'backend-system/index', + Framework: [ { type: 'category', - label: 'Architecture', + label: 'Backend System', items: [ - 'backend-system/architecture/index', - 'backend-system/architecture/services', - 'backend-system/architecture/plugins', - 'backend-system/architecture/extension-points', - 'backend-system/architecture/modules', - 'backend-system/architecture/feature-loaders', - 'backend-system/architecture/naming-patterns', + 'backend-system/index', + { + type: 'category', + label: 'Architecture', + items: [ + 'backend-system/architecture/index', + 'backend-system/architecture/services', + 'backend-system/architecture/plugins', + 'backend-system/architecture/extension-points', + 'backend-system/architecture/modules', + 'backend-system/architecture/feature-loaders', + 'backend-system/architecture/naming-patterns', + ], + }, + { + type: 'category', + label: 'Building Backends', + items: [ + 'backend-system/building-backends/index', + 'backend-system/building-backends/migrating', + ], + }, + { + type: 'category', + label: 'Building Plugins & Modules', + items: [ + 'backend-system/building-plugins-and-modules/index', + 'backend-system/building-plugins-and-modules/testing', + 'backend-system/building-plugins-and-modules/migrating', + ], + }, + { + type: 'category', + label: 'Core Services', + items: [ + 'backend-system/core-services/index', + 'backend-system/core-services/auth', + 'backend-system/core-services/cache', + 'backend-system/core-services/database', + 'backend-system/core-services/discovery', + 'backend-system/core-services/http-auth', + 'backend-system/core-services/http-router', + 'backend-system/core-services/identity', + 'backend-system/core-services/lifecycle', + 'backend-system/core-services/logger', + 'backend-system/core-services/permissions', + 'backend-system/core-services/plugin-metadata', + 'backend-system/core-services/root-config', + 'backend-system/core-services/root-health', + 'backend-system/core-services/root-http-router', + 'backend-system/core-services/root-lifecycle', + 'backend-system/core-services/root-logger', + 'backend-system/core-services/scheduler', + 'backend-system/core-services/token-manager', + 'backend-system/core-services/url-reader', + 'backend-system/core-services/user-info', + ], + }, ], }, { type: 'category', - label: 'Building Backends', + label: 'New Frontend System', items: [ - 'backend-system/building-backends/index', - 'backend-system/building-backends/migrating', - ], - }, - { - type: 'category', - label: 'Building Plugins & Modules', - items: [ - 'backend-system/building-plugins-and-modules/index', - 'backend-system/building-plugins-and-modules/testing', - 'backend-system/building-plugins-and-modules/migrating', - ], - }, - { - type: 'category', - label: 'Core Services', - items: [ - 'backend-system/core-services/index', - 'backend-system/core-services/auth', - 'backend-system/core-services/cache', - 'backend-system/core-services/database', - 'backend-system/core-services/discovery', - 'backend-system/core-services/http-auth', - 'backend-system/core-services/http-router', - 'backend-system/core-services/identity', - 'backend-system/core-services/lifecycle', - 'backend-system/core-services/logger', - 'backend-system/core-services/permissions', - 'backend-system/core-services/plugin-metadata', - 'backend-system/core-services/root-config', - 'backend-system/core-services/root-health', - 'backend-system/core-services/root-http-router', - 'backend-system/core-services/root-lifecycle', - 'backend-system/core-services/root-logger', - 'backend-system/core-services/scheduler', - 'backend-system/core-services/token-manager', - 'backend-system/core-services/url-reader', - 'backend-system/core-services/user-info', - ], - }, - ], - 'New Frontend System': [ - 'frontend-system/index', - { - type: 'category', - label: 'Architecture', - items: [ - 'frontend-system/architecture/index', - 'frontend-system/architecture/app', - 'frontend-system/architecture/plugins', - 'frontend-system/architecture/extensions', - 'frontend-system/architecture/extension-blueprints', - 'frontend-system/architecture/extension-overrides', - 'frontend-system/architecture/references', - 'frontend-system/architecture/utility-apis', - 'frontend-system/architecture/routes', - 'frontend-system/architecture/naming-patterns', - 'frontend-system/architecture/migrations', - ], - }, - { - type: 'category', - label: 'Building Plugins', - items: [ - 'frontend-system/building-plugins/index', - 'frontend-system/building-plugins/testing', - 'frontend-system/building-plugins/common-extension-blueprints', - 'frontend-system/building-plugins/built-in-data-refs', - 'frontend-system/building-plugins/migrating', - ], - }, - { - type: 'category', - label: 'Building Apps', - items: [ - 'frontend-system/building-apps/index', - 'frontend-system/building-apps/configuring-extensions', - 'frontend-system/building-apps/built-in-extensions', - 'frontend-system/building-apps/plugin-conversion', - 'frontend-system/building-apps/migrating', - ], - }, - { - type: 'category', - label: 'Utility APIs', - items: [ - 'frontend-system/utility-apis/index', - 'frontend-system/utility-apis/creating', - 'frontend-system/utility-apis/consuming', - 'frontend-system/utility-apis/configuring', + 'frontend-system/index', + { + type: 'category', + label: 'Architecture', + items: [ + 'frontend-system/architecture/index', + 'frontend-system/architecture/app', + 'frontend-system/architecture/plugins', + 'frontend-system/architecture/extensions', + 'frontend-system/architecture/extension-blueprints', + 'frontend-system/architecture/extension-overrides', + 'frontend-system/architecture/references', + 'frontend-system/architecture/utility-apis', + 'frontend-system/architecture/routes', + 'frontend-system/architecture/naming-patterns', + 'frontend-system/architecture/migrations', + ], + }, + { + type: 'category', + label: 'Building Plugins', + items: [ + 'frontend-system/building-plugins/index', + 'frontend-system/building-plugins/testing', + 'frontend-system/building-plugins/common-extension-blueprints', + 'frontend-system/building-plugins/built-in-data-refs', + 'frontend-system/building-plugins/migrating', + ], + }, + { + type: 'category', + label: 'Building Apps', + items: [ + 'frontend-system/building-apps/index', + 'frontend-system/building-apps/configuring-extensions', + 'frontend-system/building-apps/built-in-extensions', + 'frontend-system/building-apps/plugin-conversion', + 'frontend-system/building-apps/migrating', + ], + }, + { + type: 'category', + label: 'Utility APIs', + items: [ + 'frontend-system/utility-apis/index', + 'frontend-system/utility-apis/creating', + 'frontend-system/utility-apis/consuming', + 'frontend-system/utility-apis/configuring', + ], + }, ], }, ], @@ -468,19 +490,6 @@ module.exports = { 'dls/contributing-to-storybook', 'dls/figma', ], - 'API Reference': [ - { - type: 'category', - label: 'Guides', - items: ['api/utility-apis'], - }, - { - type: 'category', - label: 'API Reference', - items: ['reference/index'], - }, - 'api/deprecations', - ], Tutorials: [ 'tutorials/quickstart-app-plugin', 'tutorials/react-router-stable-migration', @@ -496,22 +505,7 @@ module.exports = { 'tutorials/enable-public-entry', 'tutorials/setup-opentelemetry', ], - 'Architecture Decision Records (ADRs)': [ - 'architecture-decisions/adrs-overview', - 'architecture-decisions/adrs-adr001', - 'architecture-decisions/adrs-adr002', - 'architecture-decisions/adrs-adr003', - 'architecture-decisions/adrs-adr004', - 'architecture-decisions/adrs-adr005', - 'architecture-decisions/adrs-adr006', - 'architecture-decisions/adrs-adr007', - 'architecture-decisions/adrs-adr008', - 'architecture-decisions/adrs-adr009', - 'architecture-decisions/adrs-adr010', - 'architecture-decisions/adrs-adr011', - 'architecture-decisions/adrs-adr012', - 'architecture-decisions/adrs-adr013', - ], + FAQ: ['faq/index', 'faq/product', 'faq/technical'], Accessibility: ['accessibility/index'], Contribute: [ @@ -519,7 +513,46 @@ module.exports = { 'contribute/getting-involved', 'contribute/project-structure', ], - References: ['references/glossary'], + References: [ + 'references/glossary', + { + type: 'category', + label: 'API Reference', + items: [ + { + type: 'category', + label: 'Guides', + items: ['api/utility-apis'], + }, + { + type: 'category', + label: 'API Reference', + items: ['reference/index'], + }, + 'api/deprecations', + ], + }, + { + type: 'category', + label: 'Architecture Decision Records (ADRs)', + items: [ + 'architecture-decisions/adrs-overview', + 'architecture-decisions/adrs-adr001', + 'architecture-decisions/adrs-adr002', + 'architecture-decisions/adrs-adr003', + 'architecture-decisions/adrs-adr004', + 'architecture-decisions/adrs-adr005', + 'architecture-decisions/adrs-adr006', + 'architecture-decisions/adrs-adr007', + 'architecture-decisions/adrs-adr008', + 'architecture-decisions/adrs-adr009', + 'architecture-decisions/adrs-adr010', + 'architecture-decisions/adrs-adr011', + 'architecture-decisions/adrs-adr012', + 'architecture-decisions/adrs-adr013', + ], + }, + ], }, releases: { 'Release Notes': releases.map(release => `releases/${release}`), From 2a9bd224a82e04a0ee97a8a9ce053e30437c23d2 Mon Sep 17 00:00:00 2001 From: Jason Liu Date: Thu, 5 Dec 2024 22:18:52 +1100 Subject: [PATCH 04/28] Pass auth service when initializing GitHub scaffolder actions Signed-off-by: Jason Liu --- plugins/scaffolder-backend-module-github/src/module.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend-module-github/src/module.ts b/plugins/scaffolder-backend-module-github/src/module.ts index f837ad0456..0387473fdb 100644 --- a/plugins/scaffolder-backend-module-github/src/module.ts +++ b/plugins/scaffolder-backend-module-github/src/module.ts @@ -51,8 +51,9 @@ export const githubModule = createBackendModule({ scaffolder: scaffolderActionsExtensionPoint, config: coreServices.rootConfig, discovery: coreServices.discovery, + auth: coreServices.auth, }, - async init({ scaffolder, config, discovery }) { + async init({ scaffolder, config, discovery, auth }) { const integrations = ScmIntegrations.fromConfig(config); const githubCredentialsProvider = DefaultGithubCredentialsProvider.fromIntegrations(integrations); @@ -75,6 +76,7 @@ export const githubModule = createBackendModule({ createGithubEnvironmentAction({ integrations, catalogClient, + auth, }), createGithubIssuesLabelAction({ integrations, From 63427d39ad882ed436ecc0fbca74a766b451205d Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Thu, 5 Dec 2024 13:45:41 +0000 Subject: [PATCH 05/28] canon: use inset box-shadows for button styles The use of box-shadows for button borders meant that when a button was styled with a border, it expanded by 1px. This commit retains the use of shadows for borders, but switches them to inset shadows so that the component size stays consistent between buttons with and without borders (and between the same button in hover and non-hover states). Signed-off-by: MT Lewis --- packages/canon/src/components/Button/styles.css | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/canon/src/components/Button/styles.css b/packages/canon/src/components/Button/styles.css index aaf1cae8bf..01f917c4b1 100644 --- a/packages/canon/src/components/Button/styles.css +++ b/packages/canon/src/components/Button/styles.css @@ -36,18 +36,18 @@ .button.primary:hover { background-color: transparent; - box-shadow: 0 0 0 1px var(--canon-outline-focus); + box-shadow: inset 0 0 0 1px var(--canon-outline-focus); color: var(--canon-text-primary); } .button.secondary { background-color: transparent; - box-shadow: 0 0 0 1px var(--canon-outline); + box-shadow: inset 0 0 0 1px var(--canon-outline); color: var(--canon-text-primary); } .button.secondary:hover { - box-shadow: 0 0 0 1px var(--canon-outline-hover); + box-shadow: inset 0 0 0 1px var(--canon-outline-hover); } .button.tertiary { From 53d01e866f97db63c6df11607afc622e480afb73 Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 10 Dec 2024 17:10:48 +0100 Subject: [PATCH 06/28] chore: move to using useApi instead Signed-off-by: blam --- .../src/hooks/useCustomFieldExtensions.ts | 7 ++-- .../src/components/Router/Router.test.tsx | 32 ++++++++++++++----- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/plugins/scaffolder-react/src/hooks/useCustomFieldExtensions.ts b/plugins/scaffolder-react/src/hooks/useCustomFieldExtensions.ts index cd67b33753..7c54614dd9 100644 --- a/plugins/scaffolder-react/src/hooks/useCustomFieldExtensions.ts +++ b/plugins/scaffolder-react/src/hooks/useCustomFieldExtensions.ts @@ -14,7 +14,7 @@ * limitations under the License. */ import { useAsync, useMountEffect } from '@react-hookz/web'; -import { useApiHolder, useElementFilter } from '@backstage/core-plugin-api'; +import { useApi, useElementFilter } from '@backstage/core-plugin-api'; import { formFieldsApiRef } from '../next'; import { FieldExtensionOptions } from '../extensions'; import { @@ -33,10 +33,9 @@ export const useCustomFieldExtensions = < outlet: React.ReactNode, ) => { // Get custom fields created with FormFieldBlueprint - const apiHolder = useApiHolder(); - const formFieldsApi = apiHolder.get(formFieldsApiRef); + const formFieldsApi = useApi(formFieldsApiRef); const [{ result: blueprintFields }, methods] = useAsync( - formFieldsApi?.getFormFields ?? (async () => []), + formFieldsApi?.getFormFields, [], ); useMountEffect(methods.execute); diff --git a/plugins/scaffolder/src/components/Router/Router.test.tsx b/plugins/scaffolder/src/components/Router/Router.test.tsx index c062c723cf..c551f34a51 100644 --- a/plugins/scaffolder/src/components/Router/Router.test.tsx +++ b/plugins/scaffolder/src/components/Router/Router.test.tsx @@ -15,7 +15,11 @@ */ import React from 'react'; import { Router } from './Router'; -import { renderInTestApp } from '@backstage/test-utils'; +import { + renderInTestApp, + TestApiProvider, + TestAppOptions, +} from '@backstage/test-utils'; import { createScaffolderFieldExtension, ScaffolderFieldExtensions, @@ -26,12 +30,23 @@ import { ScaffolderLayouts, } from '@backstage/plugin-scaffolder-react'; import { TemplateListPage, TemplateWizardPage } from '../../alpha/components'; +import { formFieldsApiRef } from '@backstage/plugin-scaffolder-react/alpha'; jest.mock('../../alpha/components', () => ({ TemplateWizardPage: jest.fn(() => null), TemplateListPage: jest.fn(() => null), })); +const renderInApp = (element: React.ReactElement, opts?: TestAppOptions) => + renderInTestApp( + [] }]]} + > + {element} + , + opts, + ); + describe('Router', () => { beforeEach(() => { (TemplateWizardPage as jest.Mock).mockClear(); @@ -39,13 +54,13 @@ describe('Router', () => { }); describe('/', () => { it('should render the TemplateListPage', async () => { - await renderInTestApp(); + await renderInApp(); expect(TemplateListPage).toHaveBeenCalled(); }); it('should render user-provided TemplateListPage', async () => { - const { getByText } = await renderInTestApp( + const { getByText } = await renderInApp( <>foobar, @@ -62,7 +77,7 @@ describe('Router', () => { describe('/templates/:templateName', () => { it('should render the TemplateWizard page', async () => { - await renderInTestApp(, { + await renderInApp(, { routeEntries: ['/templates/default/foo'], }); @@ -70,7 +85,7 @@ describe('Router', () => { }); it('should render user-provided TemplateWizardPage', async () => { - const { getByText } = await renderInTestApp( + const { getByText } = await renderInApp( <>foobar, @@ -87,13 +102,14 @@ describe('Router', () => { it('should pass through the FormProps property', async () => { const transformErrorsMock = jest.fn(); - await renderInTestApp( + await renderInApp( , + { routeEntries: ['/templates/default/foo'], }, @@ -118,7 +134,7 @@ describe('Router', () => { }), ); - await renderInTestApp( + await renderInApp( @@ -146,7 +162,7 @@ describe('Router', () => { }), ); - await renderInTestApp( + await renderInApp( From 0d6efa49ac7931a721e7fda97766947feead8c62 Mon Sep 17 00:00:00 2001 From: aramissennyeydd Date: Tue, 10 Dec 2024 18:12:02 -0700 Subject: [PATCH 07/28] reorganize based on pr feedback Signed-off-by: aramissennyeydd --- microsite/sidebars.js | 194 ++++++++++++++++++++---------------------- 1 file changed, 91 insertions(+), 103 deletions(-) diff --git a/microsite/sidebars.js b/microsite/sidebars.js index 00c728e024..40a8fa2893 100644 --- a/microsite/sidebars.js +++ b/microsite/sidebars.js @@ -49,73 +49,6 @@ module.exports = { 'getting-started/keeping-backstage-updated', ], 'Core Features': [ - { - type: 'category', - label: 'Software Catalog', - items: [ - 'features/software-catalog/software-catalog-overview', - 'features/software-catalog/life-of-an-entity', - 'features/software-catalog/configuration', - 'features/software-catalog/system-model', - 'features/software-catalog/descriptor-format', - 'features/software-catalog/references', - 'features/software-catalog/well-known-annotations', - 'features/software-catalog/well-known-relations', - 'features/software-catalog/well-known-statuses', - 'features/software-catalog/extending-the-model', - 'features/software-catalog/external-integrations', - 'features/software-catalog/catalog-customization', - 'features/software-catalog/software-catalog-api', - 'features/software-catalog/creating-the-catalog-graph', - 'features/software-catalog/faq', - ], - }, - { - type: 'category', - label: 'Kubernetes', - items: [ - 'features/kubernetes/overview', - 'features/kubernetes/installation', - 'features/kubernetes/configuration', - 'features/kubernetes/authentication', - 'features/kubernetes/authentication-strategies', - 'features/kubernetes/troubleshooting', - 'features/kubernetes/proxy', - ], - }, - { - type: 'category', - label: 'Software Templates', - items: [ - 'features/software-templates/software-templates-index', - 'features/software-templates/configuration', - 'features/software-templates/adding-templates', - 'features/software-templates/writing-templates', - 'features/software-templates/input-examples', - 'features/software-templates/builtin-actions', - 'features/software-templates/writing-custom-actions', - 'features/software-templates/writing-tests-for-actions', - 'features/software-templates/writing-custom-field-extensions', - 'features/software-templates/writing-custom-step-layouts', - 'features/software-templates/authorizing-scaffolder-template-details', - 'features/software-templates/migrating-to-rjsf-v5', - 'features/software-templates/migrating-from-v1beta2-to-v1beta3', - 'features/software-templates/dry-run-testing', - ], - }, - { - type: 'category', - label: 'Search', - items: [ - 'features/search/search-overview', - 'features/search/getting-started', - 'features/search/concepts', - 'features/search/architecture', - 'features/search/search-engines', - 'features/search/collators', - 'features/search/how-to-guides', - ], - }, { type: 'category', label: 'Auth and Identity', @@ -153,6 +86,24 @@ module.exports = { 'auth/troubleshooting', ], }, + { + type: 'category', + label: 'Kubernetes', + items: [ + 'features/kubernetes/overview', + 'features/kubernetes/installation', + 'features/kubernetes/configuration', + 'features/kubernetes/authentication', + 'features/kubernetes/authentication-strategies', + 'features/kubernetes/troubleshooting', + 'features/kubernetes/proxy', + ], + }, + { + type: 'category', + label: 'Notifications', + items: ['notifications/index'], + }, { type: 'category', label: 'Permissions', @@ -176,6 +127,60 @@ module.exports = { }, ], }, + { + type: 'category', + label: 'Search', + items: [ + 'features/search/search-overview', + 'features/search/getting-started', + 'features/search/concepts', + 'features/search/architecture', + 'features/search/search-engines', + 'features/search/collators', + 'features/search/how-to-guides', + ], + }, + { + type: 'category', + label: 'Software Catalog', + items: [ + 'features/software-catalog/software-catalog-overview', + 'features/software-catalog/life-of-an-entity', + 'features/software-catalog/configuration', + 'features/software-catalog/system-model', + 'features/software-catalog/descriptor-format', + 'features/software-catalog/references', + 'features/software-catalog/well-known-annotations', + 'features/software-catalog/well-known-relations', + 'features/software-catalog/well-known-statuses', + 'features/software-catalog/extending-the-model', + 'features/software-catalog/external-integrations', + 'features/software-catalog/catalog-customization', + 'features/software-catalog/software-catalog-api', + 'features/software-catalog/creating-the-catalog-graph', + 'features/software-catalog/faq', + ], + }, + { + type: 'category', + label: 'Software Templates', + items: [ + 'features/software-templates/software-templates-index', + 'features/software-templates/configuration', + 'features/software-templates/adding-templates', + 'features/software-templates/writing-templates', + 'features/software-templates/input-examples', + 'features/software-templates/builtin-actions', + 'features/software-templates/writing-custom-actions', + 'features/software-templates/writing-tests-for-actions', + 'features/software-templates/writing-custom-field-extensions', + 'features/software-templates/writing-custom-step-layouts', + 'features/software-templates/authorizing-scaffolder-template-details', + 'features/software-templates/migrating-to-rjsf-v5', + 'features/software-templates/migrating-from-v1beta2-to-v1beta3', + 'features/software-templates/dry-run-testing', + ], + }, { type: 'category', label: 'TechDocs', @@ -196,11 +201,6 @@ module.exports = { 'features/techdocs/faqs', ], }, - { - type: 'category', - label: 'Notifications', - items: ['notifications/index'], - }, ], Integrations: [ 'integrations/index', @@ -341,27 +341,6 @@ module.exports = { 'conf/writing', 'conf/defining', ], - Tooling: [ - { - type: 'category', - label: 'Backstage CLI', - items: [ - 'tooling/cli/overview', - 'tooling/cli/build-system', - 'tooling/cli/commands', - ], - }, - { - type: 'category', - label: 'Local Development', - items: [ - 'tooling/local-dev/linking-local-packages', - 'tooling/local-dev/debugging', - 'tooling/local-dev/profiling', - ], - }, - 'tooling/package-metadata', - ], Framework: [ { type: 'category', @@ -483,6 +462,23 @@ module.exports = { }, ], }, + { + 'Backstage CLI': [ + 'tooling/cli/overview', + 'tooling/cli/build-system', + 'tooling/cli/commands', + { + type: 'category', + label: 'Local Development', + items: [ + 'tooling/local-dev/linking-local-packages', + 'tooling/local-dev/debugging', + 'tooling/local-dev/profiling', + ], + }, + 'tooling/package-metadata', + ], + } ], 'Designing for Backstage': [ 'dls/design', @@ -519,16 +515,8 @@ module.exports = { type: 'category', label: 'API Reference', items: [ - { - type: 'category', - label: 'Guides', - items: ['api/utility-apis'], - }, - { - type: 'category', - label: 'API Reference', - items: ['reference/index'], - }, + 'api/utility-apis', + 'reference/index', 'api/deprecations', ], }, From 7df1988a03a150c81b1715d55c3397c81a92d865 Mon Sep 17 00:00:00 2001 From: aramissennyeydd Date: Tue, 10 Dec 2024 18:14:47 -0700 Subject: [PATCH 08/28] flatten references as well Signed-off-by: aramissennyeydd --- microsite/sidebars.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/microsite/sidebars.js b/microsite/sidebars.js index 40a8fa2893..7d0ac3103e 100644 --- a/microsite/sidebars.js +++ b/microsite/sidebars.js @@ -510,16 +510,6 @@ module.exports = { 'contribute/project-structure', ], References: [ - 'references/glossary', - { - type: 'category', - label: 'API Reference', - items: [ - 'api/utility-apis', - 'reference/index', - 'api/deprecations', - ], - }, { type: 'category', label: 'Architecture Decision Records (ADRs)', @@ -540,6 +530,10 @@ module.exports = { 'architecture-decisions/adrs-adr013', ], }, + 'api/deprecations', + 'references/glossary', + 'api/utility-apis', + 'reference/index', ], }, releases: { From f8299f0296d18cfbe9d4e7b4e32aad08b80e3a59 Mon Sep 17 00:00:00 2001 From: aramissennyeydd Date: Tue, 10 Dec 2024 18:31:45 -0700 Subject: [PATCH 09/28] fix prettier Signed-off-by: aramissennyeydd --- microsite/sidebars.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/microsite/sidebars.js b/microsite/sidebars.js index 7d0ac3103e..64eaff8025 100644 --- a/microsite/sidebars.js +++ b/microsite/sidebars.js @@ -478,7 +478,7 @@ module.exports = { }, 'tooling/package-metadata', ], - } + }, ], 'Designing for Backstage': [ 'dls/design', From 53efa41cc8c152cdde80d93227027f552f2c1e38 Mon Sep 17 00:00:00 2001 From: blam Date: Wed, 11 Dec 2024 09:56:01 +0100 Subject: [PATCH 10/28] chore: code-review comments Signed-off-by: blam --- .../src/hooks/useCustomFieldExtensions.ts | 2 +- .../src/components/Router/Router.test.tsx | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/plugins/scaffolder-react/src/hooks/useCustomFieldExtensions.ts b/plugins/scaffolder-react/src/hooks/useCustomFieldExtensions.ts index 7c54614dd9..d1e670330e 100644 --- a/plugins/scaffolder-react/src/hooks/useCustomFieldExtensions.ts +++ b/plugins/scaffolder-react/src/hooks/useCustomFieldExtensions.ts @@ -35,7 +35,7 @@ export const useCustomFieldExtensions = < // Get custom fields created with FormFieldBlueprint const formFieldsApi = useApi(formFieldsApiRef); const [{ result: blueprintFields }, methods] = useAsync( - formFieldsApi?.getFormFields, + formFieldsApi.getFormFields, [], ); useMountEffect(methods.execute); diff --git a/plugins/scaffolder/src/components/Router/Router.test.tsx b/plugins/scaffolder/src/components/Router/Router.test.tsx index c551f34a51..9c752ed3c1 100644 --- a/plugins/scaffolder/src/components/Router/Router.test.tsx +++ b/plugins/scaffolder/src/components/Router/Router.test.tsx @@ -37,7 +37,10 @@ jest.mock('../../alpha/components', () => ({ TemplateListPage: jest.fn(() => null), })); -const renderInApp = (element: React.ReactElement, opts?: TestAppOptions) => +const wrapInApisAndRender = ( + element: React.ReactElement, + opts?: TestAppOptions, +) => renderInTestApp( [] }]]} @@ -54,13 +57,13 @@ describe('Router', () => { }); describe('/', () => { it('should render the TemplateListPage', async () => { - await renderInApp(); + await wrapInApisAndRender(); expect(TemplateListPage).toHaveBeenCalled(); }); it('should render user-provided TemplateListPage', async () => { - const { getByText } = await renderInApp( + const { getByText } = await wrapInApisAndRender( <>foobar, @@ -77,7 +80,7 @@ describe('Router', () => { describe('/templates/:templateName', () => { it('should render the TemplateWizard page', async () => { - await renderInApp(, { + await wrapInApisAndRender(, { routeEntries: ['/templates/default/foo'], }); @@ -85,7 +88,7 @@ describe('Router', () => { }); it('should render user-provided TemplateWizardPage', async () => { - const { getByText } = await renderInApp( + const { getByText } = await wrapInApisAndRender( <>foobar, @@ -102,7 +105,7 @@ describe('Router', () => { it('should pass through the FormProps property', async () => { const transformErrorsMock = jest.fn(); - await renderInApp( + await wrapInApisAndRender( { }), ); - await renderInApp( + await wrapInApisAndRender( @@ -162,7 +165,7 @@ describe('Router', () => { }), ); - await renderInApp( + await wrapInApisAndRender( From c36bd357980f143c4fa247b1148c04c63d3edf39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Tue, 10 Dec 2024 15:13:41 +0100 Subject: [PATCH 11/28] break circular imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/good-pens-dance.md | 6 + .../EntityAutocompletePicker.tsx | 2 +- .../UserListPicker/useAllEntitiesCount.ts | 2 +- .../UserListPicker/useOwnedEntitiesCount.ts | 2 +- .../UserListPicker/useStarredEntitiesCount.ts | 2 +- plugins/catalog-react/src/filters.ts | 2 +- .../src/hooks/useEntityListProvider.tsx | 2 +- plugins/catalog-react/src/utils/index.ts | 2 +- .../src/components/SchemaView/ArrayView.tsx | 60 --- .../src/components/SchemaView/ChildView.tsx | 127 ------- .../src/components/SchemaView/MatchView.tsx | 46 --- .../components/SchemaView/MetadataView.tsx | 108 ------ .../src/components/SchemaView/ObjectView.tsx | 95 ----- .../src/components/SchemaView/ScalarView.tsx | 34 -- .../src/components/SchemaView/SchemaView.tsx | 353 +++++++++++++++++- 15 files changed, 361 insertions(+), 482 deletions(-) create mode 100644 .changeset/good-pens-dance.md delete mode 100644 plugins/config-schema/src/components/SchemaView/ArrayView.tsx delete mode 100644 plugins/config-schema/src/components/SchemaView/ChildView.tsx delete mode 100644 plugins/config-schema/src/components/SchemaView/MatchView.tsx delete mode 100644 plugins/config-schema/src/components/SchemaView/MetadataView.tsx delete mode 100644 plugins/config-schema/src/components/SchemaView/ObjectView.tsx delete mode 100644 plugins/config-schema/src/components/SchemaView/ScalarView.tsx diff --git a/.changeset/good-pens-dance.md b/.changeset/good-pens-dance.md new file mode 100644 index 0000000000..05ebce1bc6 --- /dev/null +++ b/.changeset/good-pens-dance.md @@ -0,0 +1,6 @@ +--- +'@backstage/plugin-catalog-react': patch +'@backstage/plugin-config-schema': patch +--- + +Internal refactor to break potential circular imports diff --git a/plugins/catalog-react/src/components/EntityAutocompletePicker/EntityAutocompletePicker.tsx b/plugins/catalog-react/src/components/EntityAutocompletePicker/EntityAutocompletePicker.tsx index eba4cc3a7a..fd88a7a31e 100644 --- a/plugins/catalog-react/src/components/EntityAutocompletePicker/EntityAutocompletePicker.tsx +++ b/plugins/catalog-react/src/components/EntityAutocompletePicker/EntityAutocompletePicker.tsx @@ -31,7 +31,7 @@ import { useEntityList, } from '../../hooks/useEntityListProvider'; import { EntityFilter } from '../../types'; -import { reduceBackendCatalogFilters } from '../../utils'; +import { reduceBackendCatalogFilters } from '../../utils/filters'; /** @public */ export type AllowedEntityFilters = { diff --git a/plugins/catalog-react/src/components/UserListPicker/useAllEntitiesCount.ts b/plugins/catalog-react/src/components/UserListPicker/useAllEntitiesCount.ts index cecd0e4db7..fa564382d8 100644 --- a/plugins/catalog-react/src/components/UserListPicker/useAllEntitiesCount.ts +++ b/plugins/catalog-react/src/components/UserListPicker/useAllEntitiesCount.ts @@ -20,7 +20,7 @@ import { useMemo, useRef } from 'react'; import useAsync from 'react-use/esm/useAsync'; import { catalogApiRef } from '../../api'; import { useEntityList } from '../../hooks'; -import { reduceCatalogFilters } from '../../utils'; +import { reduceCatalogFilters } from '../../utils/filters'; export function useAllEntitiesCount() { const catalogApi = useApi(catalogApiRef); diff --git a/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.ts b/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.ts index 10481e80a8..dbdc040165 100644 --- a/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.ts +++ b/plugins/catalog-react/src/components/UserListPicker/useOwnedEntitiesCount.ts @@ -21,7 +21,7 @@ import useAsync from 'react-use/esm/useAsync'; import { catalogApiRef } from '../../api'; import { EntityOwnerFilter, EntityUserFilter } from '../../filters'; import { useEntityList } from '../../hooks'; -import { CatalogFilters, reduceCatalogFilters } from '../../utils'; +import { CatalogFilters, reduceCatalogFilters } from '../../utils/filters'; import useAsyncFn from 'react-use/esm/useAsyncFn'; import useDeepCompareEffect from 'react-use/esm/useDeepCompareEffect'; diff --git a/plugins/catalog-react/src/components/UserListPicker/useStarredEntitiesCount.ts b/plugins/catalog-react/src/components/UserListPicker/useStarredEntitiesCount.ts index 082c683642..70dde024f8 100644 --- a/plugins/catalog-react/src/components/UserListPicker/useStarredEntitiesCount.ts +++ b/plugins/catalog-react/src/components/UserListPicker/useStarredEntitiesCount.ts @@ -23,7 +23,7 @@ import useAsync from 'react-use/esm/useAsync'; import { catalogApiRef } from '../../api'; import { EntityUserFilter } from '../../filters'; import { useEntityList, useStarredEntities } from '../../hooks'; -import { reduceCatalogFilters } from '../../utils'; +import { reduceCatalogFilters } from '../../utils/filters'; export function useStarredEntitiesCount() { const catalogApi = useApi(catalogApiRef); diff --git a/plugins/catalog-react/src/filters.ts b/plugins/catalog-react/src/filters.ts index 6c68dad9aa..c033298983 100644 --- a/plugins/catalog-react/src/filters.ts +++ b/plugins/catalog-react/src/filters.ts @@ -22,7 +22,7 @@ import { } from '@backstage/catalog-model'; import { AlphaEntity } from '@backstage/catalog-model/alpha'; import { EntityFilter, UserListFilterKind } from './types'; -import { getEntityRelations } from './utils'; +import { getEntityRelations } from './utils/getEntityRelations'; /** * Filter entities based on Kind. diff --git a/plugins/catalog-react/src/hooks/useEntityListProvider.tsx b/plugins/catalog-react/src/hooks/useEntityListProvider.tsx index 274ec52a18..b96357eb62 100644 --- a/plugins/catalog-react/src/hooks/useEntityListProvider.tsx +++ b/plugins/catalog-react/src/hooks/useEntityListProvider.tsx @@ -48,7 +48,7 @@ import { reduceBackendCatalogFilters, reduceCatalogFilters, reduceEntityFilters, -} from '../utils'; +} from '../utils/filters'; import { useApi } from '@backstage/core-plugin-api'; import { QueryEntitiesResponse } from '@backstage/catalog-client'; diff --git a/plugins/catalog-react/src/utils/index.ts b/plugins/catalog-react/src/utils/index.ts index 5afc32af63..5b2159ae2f 100644 --- a/plugins/catalog-react/src/utils/index.ts +++ b/plugins/catalog-react/src/utils/index.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -export * from './filters'; + export { getEntityRelations } from './getEntityRelations'; export { getEntitySourceLocation } from './getEntitySourceLocation'; export type { EntitySourceLocation } from './getEntitySourceLocation'; diff --git a/plugins/config-schema/src/components/SchemaView/ArrayView.tsx b/plugins/config-schema/src/components/SchemaView/ArrayView.tsx deleted file mode 100644 index abe2cfc327..0000000000 --- a/plugins/config-schema/src/components/SchemaView/ArrayView.tsx +++ /dev/null @@ -1,60 +0,0 @@ -/* - * Copyright 2021 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 Box from '@material-ui/core/Box'; -import Typography from '@material-ui/core/Typography'; -import { Schema } from 'jsonschema'; -import React from 'react'; -import { ChildView } from './ChildView'; -import { MetadataView } from './MetadataView'; -import { SchemaViewProps } from './types'; - -export function ArrayView({ path, depth, schema }: SchemaViewProps) { - const itemDepth = depth + 1; - const itemPath = path ? `${path}[]` : '[]'; - const itemSchema = schema.items; - - return ( - <> - - {schema.description && ( - - {schema.description} - - )} - - - Items - - {schema.additionalItems && schema.additionalItems !== true && ( - <> - Additional Items - - - )} - - ); -} diff --git a/plugins/config-schema/src/components/SchemaView/ChildView.tsx b/plugins/config-schema/src/components/SchemaView/ChildView.tsx deleted file mode 100644 index b39e445688..0000000000 --- a/plugins/config-schema/src/components/SchemaView/ChildView.tsx +++ /dev/null @@ -1,127 +0,0 @@ -/* - * Copyright 2021 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 { JsonValue } from '@backstage/types'; -import Box from '@material-ui/core/Box'; -import Chip from '@material-ui/core/Chip'; -import Divider from '@material-ui/core/Divider'; -import Typography from '@material-ui/core/Typography'; -import { makeStyles } from '@material-ui/core/styles'; -import { Schema } from 'jsonschema'; -import React, { useEffect, useRef } from 'react'; -import { useScrollTargets } from '../ScrollTargetsContext/ScrollTargetsContext'; -import { SchemaView } from './SchemaView'; - -export interface MetadataViewRowProps { - label: string; - text?: string; - data?: JsonValue; -} - -function titleVariant(depth: number) { - if (depth <= 1) { - return 'h2'; - } else if (depth === 2) { - return 'h3'; - } else if (depth === 3) { - return 'h4'; - } else if (depth === 4) { - return 'h5'; - } - return 'h6'; -} - -const useChildViewStyles = makeStyles(theme => ({ - title: { - marginBottom: 0, - }, - chip: { - marginLeft: theme.spacing(1), - marginRight: 0, - marginBottom: 0, - }, -})); - -export function ChildView({ - path, - depth, - schema, - required, - lastChild, -}: { - path: string; - depth: number; - schema?: Schema; - required?: boolean; - lastChild?: boolean; -}) { - const classes = useChildViewStyles(); - const titleRef = useRef(null); - const scroll = useScrollTargets(); - - useEffect(() => { - return scroll?.setScrollListener(path, () => { - titleRef.current?.scrollIntoView({ behavior: 'smooth' }); - }); - }, [scroll, path]); - - const chips = new Array(); - const chipProps = { size: 'small' as const, classes: { root: classes.chip } }; - - if (required) { - chips.push( - , - ); - } - - const visibility = (schema as { visibility?: string })?.visibility; - if (visibility === 'frontend') { - chips.push( - , - ); - } else if (visibility === 'secret') { - chips.push( - , - ); - } - - return ( - - - - - - {path} - - {chips.length > 0 && } - {chips} - - {schema && ( - - )} - - - ); -} diff --git a/plugins/config-schema/src/components/SchemaView/MatchView.tsx b/plugins/config-schema/src/components/SchemaView/MatchView.tsx deleted file mode 100644 index ba97ff9db5..0000000000 --- a/plugins/config-schema/src/components/SchemaView/MatchView.tsx +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Copyright 2021 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 Typography from '@material-ui/core/Typography'; -import { Schema } from 'jsonschema'; -import React from 'react'; -import { ChildView } from './ChildView'; - -export function MatchView({ - path, - depth, - schema, - label, -}: { - path: string; - depth: number; - schema: Schema[]; - label: string; -}) { - return ( - <> - {label} - {schema.map((optionSchema, index) => ( - - ))} - - ); -} diff --git a/plugins/config-schema/src/components/SchemaView/MetadataView.tsx b/plugins/config-schema/src/components/SchemaView/MetadataView.tsx deleted file mode 100644 index f3de041bed..0000000000 --- a/plugins/config-schema/src/components/SchemaView/MetadataView.tsx +++ /dev/null @@ -1,108 +0,0 @@ -/* - * Copyright 2021 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 { JsonValue } from '@backstage/types'; -import Paper from '@material-ui/core/Paper'; -import Table from '@material-ui/core/Table'; -import TableBody from '@material-ui/core/TableBody'; -import TableCell from '@material-ui/core/TableCell'; -import TableRow from '@material-ui/core/TableRow'; -import Typography from '@material-ui/core/Typography'; -import { Schema } from 'jsonschema'; -import React from 'react'; - -export interface MetadataViewRowProps { - label: string; - text?: string; - data?: JsonValue; -} - -export function MetadataViewRow({ label, text, data }: MetadataViewRowProps) { - if (text === undefined && data === undefined) { - return null; - } - return ( - - - - {label} - - - - - {data ? JSON.stringify(data) : text} - - - - ); -} - -export function MetadataView({ schema }: { schema: Schema }) { - return ( - - - - - - {schema.additionalProperties === true && ( - - )} - {schema.additionalItems === true && ( - - )} - - - - - - - - - - - - - - - -
- - ); -} diff --git a/plugins/config-schema/src/components/SchemaView/ObjectView.tsx b/plugins/config-schema/src/components/SchemaView/ObjectView.tsx deleted file mode 100644 index c7cf000e10..0000000000 --- a/plugins/config-schema/src/components/SchemaView/ObjectView.tsx +++ /dev/null @@ -1,95 +0,0 @@ -/* - * Copyright 2021 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 Box from '@material-ui/core/Box'; -import Typography from '@material-ui/core/Typography'; -import React from 'react'; -import { ChildView } from './ChildView'; -import { MetadataView } from './MetadataView'; -import { SchemaViewProps } from './types'; - -function isRequired(name: string, required?: boolean | string[]) { - if (required === true) { - return true; - } - if (Array.isArray(required)) { - return required.includes(name); - } - return false; -} - -export function ObjectView({ path, depth, schema }: SchemaViewProps) { - const properties = Object.entries(schema.properties ?? {}); - const patternProperties = Object.entries(schema.patternProperties ?? {}); - - return ( - <> - {depth > 0 && ( - - {schema.description && ( - - {schema.description} - - )} - - - )} - {properties.length > 0 && ( - <> - {depth > 0 && Properties} - {properties.map(([name, propSchema], index) => ( - - ))} - - )} - {patternProperties.length > 0 && ( - <> - {depth > 0 && ( - Pattern Properties - )} - {patternProperties.map(([name, propSchema], index) => ( - ` : name} - depth={depth + 1} - schema={propSchema} - lastChild={index === patternProperties.length - 1} - required={isRequired(name, schema.required)} - /> - ))} - - )} - {schema.additionalProperties && schema.additionalProperties !== true && ( - <> - Additional Properties - - - )} - - ); -} diff --git a/plugins/config-schema/src/components/SchemaView/ScalarView.tsx b/plugins/config-schema/src/components/SchemaView/ScalarView.tsx deleted file mode 100644 index aeea1c04d7..0000000000 --- a/plugins/config-schema/src/components/SchemaView/ScalarView.tsx +++ /dev/null @@ -1,34 +0,0 @@ -/* - * Copyright 2021 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 Box from '@material-ui/core/Box'; -import Typography from '@material-ui/core/Typography'; -import React from 'react'; -import { MetadataView } from './MetadataView'; -import { SchemaViewProps } from './types'; - -export function ScalarView({ schema }: SchemaViewProps) { - return ( - <> - {schema.description && ( - - {schema.description} - - )} - - - ); -} diff --git a/plugins/config-schema/src/components/SchemaView/SchemaView.tsx b/plugins/config-schema/src/components/SchemaView/SchemaView.tsx index defb7fc353..d7a8db4f69 100644 --- a/plugins/config-schema/src/components/SchemaView/SchemaView.tsx +++ b/plugins/config-schema/src/components/SchemaView/SchemaView.tsx @@ -14,11 +14,20 @@ * limitations under the License. */ -import React from 'react'; -import { ArrayView } from './ArrayView'; -import { MatchView } from './MatchView'; -import { ObjectView } from './ObjectView'; -import { ScalarView } from './ScalarView'; +import { JsonValue } from '@backstage/types'; +import Box from '@material-ui/core/Box'; +import Chip from '@material-ui/core/Chip'; +import Divider from '@material-ui/core/Divider'; +import Paper from '@material-ui/core/Paper'; +import Table from '@material-ui/core/Table'; +import TableBody from '@material-ui/core/TableBody'; +import TableCell from '@material-ui/core/TableCell'; +import TableRow from '@material-ui/core/TableRow'; +import Typography from '@material-ui/core/Typography'; +import { makeStyles } from '@material-ui/core/styles'; +import { Schema } from 'jsonschema'; +import React, { useEffect, useRef } from 'react'; +import { useScrollTargets } from '../ScrollTargetsContext/ScrollTargetsContext'; import { SchemaViewProps } from './types'; export function SchemaView(props: SchemaViewProps) { @@ -60,3 +69,337 @@ export function SchemaView(props: SchemaViewProps) { return ; } } + +function ArrayView({ path, depth, schema }: SchemaViewProps) { + const itemDepth = depth + 1; + const itemPath = path ? `${path}[]` : '[]'; + const itemSchema = schema.items; + + return ( + <> + + {schema.description && ( + + {schema.description} + + )} + + + Items + + {schema.additionalItems && schema.additionalItems !== true && ( + <> + Additional Items + + + )} + + ); +} + +function isRequired(name: string, required?: boolean | string[]) { + if (required === true) { + return true; + } + if (Array.isArray(required)) { + return required.includes(name); + } + return false; +} + +function ObjectView({ path, depth, schema }: SchemaViewProps) { + const properties = Object.entries(schema.properties ?? {}); + const patternProperties = Object.entries(schema.patternProperties ?? {}); + + return ( + <> + {depth > 0 && ( + + {schema.description && ( + + {schema.description} + + )} + + + )} + {properties.length > 0 && ( + <> + {depth > 0 && Properties} + {properties.map(([name, propSchema], index) => ( + + ))} + + )} + {patternProperties.length > 0 && ( + <> + {depth > 0 && ( + Pattern Properties + )} + {patternProperties.map(([name, propSchema], index) => ( + ` : name} + depth={depth + 1} + schema={propSchema} + lastChild={index === patternProperties.length - 1} + required={isRequired(name, schema.required)} + /> + ))} + + )} + {schema.additionalProperties && schema.additionalProperties !== true && ( + <> + Additional Properties + + + )} + + ); +} + +interface MetadataViewRowProps { + label: string; + text?: string; + data?: JsonValue; +} + +function titleVariant(depth: number) { + if (depth <= 1) { + return 'h2'; + } else if (depth === 2) { + return 'h3'; + } else if (depth === 3) { + return 'h4'; + } else if (depth === 4) { + return 'h5'; + } + return 'h6'; +} + +const useChildViewStyles = makeStyles(theme => ({ + title: { + marginBottom: 0, + }, + chip: { + marginLeft: theme.spacing(1), + marginRight: 0, + marginBottom: 0, + }, +})); + +function ChildView({ + path, + depth, + schema, + required, + lastChild, +}: { + path: string; + depth: number; + schema?: Schema; + required?: boolean; + lastChild?: boolean; +}) { + const classes = useChildViewStyles(); + const titleRef = useRef(null); + const scroll = useScrollTargets(); + + useEffect(() => { + return scroll?.setScrollListener(path, () => { + titleRef.current?.scrollIntoView({ behavior: 'smooth' }); + }); + }, [scroll, path]); + + const chips = new Array(); + const chipProps = { size: 'small' as const, classes: { root: classes.chip } }; + + if (required) { + chips.push( + , + ); + } + + const visibility = (schema as { visibility?: string })?.visibility; + if (visibility === 'frontend') { + chips.push( + , + ); + } else if (visibility === 'secret') { + chips.push( + , + ); + } + + return ( + + + + + + {path} + + {chips.length > 0 && } + {chips} + + {schema && ( + + )} + + + ); +} + +function MatchView({ + path, + depth, + schema, + label, +}: { + path: string; + depth: number; + schema: Schema[]; + label: string; +}) { + return ( + <> + {label} + {schema.map((optionSchema, index) => ( + + ))} + + ); +} + +function ScalarView({ schema }: SchemaViewProps) { + return ( + <> + {schema.description && ( + + {schema.description} + + )} + + + ); +} + +interface MetadataViewRowProps { + label: string; + text?: string; + data?: JsonValue; +} + +function MetadataViewRow({ label, text, data }: MetadataViewRowProps) { + if (text === undefined && data === undefined) { + return null; + } + return ( + + + + {label} + + + + + {data ? JSON.stringify(data) : text} + + + + ); +} + +function MetadataView({ schema }: { schema: Schema }) { + return ( + + + + + + {schema.additionalProperties === true && ( + + )} + {schema.additionalItems === true && ( + + )} + + + + + + + + + + + + + + + +
+ + ); +} From 350fb0943b661e15bb64030ec754c66cff9566d8 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Dec 2024 09:35:49 +0100 Subject: [PATCH 12/28] catalog-backend: roll back filter join strategy Signed-off-by: Patrik Oldsberg --- .changeset/empty-colts-promise.md | 2 +- .../src/service/DefaultEntitiesCatalog.ts | 2 - .../request/applyEntityFilterToQuery.ts | 118 +----------------- 3 files changed, 4 insertions(+), 118 deletions(-) diff --git a/.changeset/empty-colts-promise.md b/.changeset/empty-colts-promise.md index 9f7f979b98..80cea4d42e 100644 --- a/.changeset/empty-colts-promise.md +++ b/.changeset/empty-colts-promise.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-backend': patch --- -Use a join based strategy for filtering, when having small page sizes +Internal refactor of filter parsing logic. diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts index 633b485604..cddc95f496 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts @@ -142,7 +142,6 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { targetQuery: entitiesQuery, onEntityIdField: 'final_entities.entity_id', knex: db, - strategy: limit !== undefined && limit <= 500 ? 'join' : 'in', }); } @@ -305,7 +304,6 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { targetQuery: inner, onEntityIdField: 'final_entities.entity_id', knex: this.database, - strategy: limit <= 500 ? 'join' : 'in', }); } diff --git a/plugins/catalog-backend/src/service/request/applyEntityFilterToQuery.ts b/plugins/catalog-backend/src/service/request/applyEntityFilterToQuery.ts index bf5c69d6b3..b2a7fb6708 100644 --- a/plugins/catalog-backend/src/service/request/applyEntityFilterToQuery.ts +++ b/plugins/catalog-backend/src/service/request/applyEntityFilterToQuery.ts @@ -33,12 +33,6 @@ function isOrEntityFilter( return filter.hasOwnProperty('anyOf'); } -function isAndEntityFilter( - filter: EntityFilter, -): filter is { allOf: EntityFilter[] } { - return filter.hasOwnProperty('allOf'); -} - function isNegationEntityFilter( filter: EntityFilter, ): filter is { not: EntityFilter } { @@ -122,102 +116,6 @@ function applyInStrategy( ); } -/** - * Applies filtering through a number of JOINs with the search table. Example: - * - * ``` - * SELECT * FROM final_entities - * LEFT OUTER JOIN search AS filter_0 - * ON filter_0.entity_id = final_entities.entity_id - * AND filter_0.key = 'kind' - * LEFT OUTER JOIN search AS filter_1 - * ON filter_1.entity_id = final_entities.entity_id - * AND filter_1.key = 'spec.lifecycle' - * WHERE (filter_0.value = 'component' AND filter_1.value = 'production') - * AND final_entities.final_entity IS NOT NULL - * ``` - * - * This strategy has very good performance on nested medium complexity queries - * on pg, but can be slow on sqlite. It also has much larger variance than the - * IN strategy: for small page sizes (< 500 or so, depending on circumstances) - * it generates a fast plan, but then at some threshold switches over to scans - * which suddenly lead to much worse performance than IN. Therefore it can be - * important to pick carefully between the strategies. - */ -function applyJoinStrategy( - filter: EntityFilter, - targetQuery: Knex.QueryBuilder, - onEntityIdField: string, -): Knex.QueryBuilder { - // First we traverse the entire query tree to gather up all of the unique keys - // that are tested against, and make sure to make an outer join on the search - // table for each of them. As we do so, collect the table aliases made along - // the way. In the end, this map may contain for example - // `{ 'kind': 'filter_0', 'spec.lifecycle': 'filter_1' }` - const keyToSearchTableAlias = new Map(); - function recursiveMakeJoinAliases(filterNode: EntityFilter) { - if (isNegationEntityFilter(filterNode)) { - recursiveMakeJoinAliases(filterNode.not); - } else if (isOrEntityFilter(filterNode)) { - filterNode.anyOf.forEach(recursiveMakeJoinAliases); - } else if (isAndEntityFilter(filterNode)) { - filterNode.allOf.forEach(recursiveMakeJoinAliases); - } else { - const key = filterNode.key.toLowerCase(); - if (!keyToSearchTableAlias.has(key)) { - const alias = `filter_${keyToSearchTableAlias.size}`; - keyToSearchTableAlias.set(key, alias); - targetQuery.leftOuterJoin({ [alias]: 'search' }, inner => - inner - .on(`${alias}.entity_id`, onEntityIdField) - .andOnVal(`${alias}.key`, key), - ); - } - } - } - recursiveMakeJoinAliases(filter); - - // Then we traverse the query tree again, this time building up the actual - // WHERE query based on values from the aliases above - function recursiveBuildQuery( - queryBuilder: Knex.QueryBuilder, - filterNode: EntityFilter, - ) { - if (isNegationEntityFilter(filterNode)) { - queryBuilder.whereNot(inner => - recursiveBuildQuery(inner, filterNode.not), - ); - } else if (isOrEntityFilter(filterNode)) { - // This extra nesting is needed to make sure that the ORs are grouped - // separately and not "leak" next to ANDs in the caller's query. - queryBuilder.andWhere(inner => { - for (const subFilter of filterNode.anyOf) { - inner.orWhere(inner2 => recursiveBuildQuery(inner2, subFilter)); - } - }); - } else if (isAndEntityFilter(filterNode)) { - for (const subFilter of filterNode.allOf) { - queryBuilder.andWhere(inner => recursiveBuildQuery(inner, subFilter)); - } - } else { - const key = filterNode.key.toLowerCase(); - const values = filterNode.values?.map(v => v.toLowerCase()); - const column = `${keyToSearchTableAlias.get(key)}.value`; - if (!values) { - queryBuilder.whereNotNull(column); - } else if (values.length === 1) { - // Null check needed since NULL = 'string' evaluates to NULL, not FALSE - queryBuilder.whereNotNull(column).andWhere(column, values[0]); - } else { - queryBuilder.whereIn(column, values); - } - } - } - recursiveBuildQuery(targetQuery, filter); - - return targetQuery; -} - // The actual exported function export function applyEntityFilterToQuery(options: { filter: EntityFilter; @@ -226,17 +124,7 @@ export function applyEntityFilterToQuery(options: { knex: Knex; strategy?: 'in' | 'join'; }): Knex.QueryBuilder { - const { - filter, - targetQuery, - onEntityIdField, - knex, - strategy = 'in', - } = options; - if (strategy === 'in') { - return applyInStrategy(filter, targetQuery, onEntityIdField, knex, false); - } else if (strategy === 'join') { - return applyJoinStrategy(filter, targetQuery, onEntityIdField); - } - throw new Error(`Unsupported filtering strategy ${strategy}`); + const { filter, targetQuery, onEntityIdField, knex } = options; + + return applyInStrategy(filter, targetQuery, onEntityIdField, knex, false); } From 3c3a7e62f6689d66ef784ba6a056f1d87a41c762 Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 12 Dec 2024 10:31:14 +0100 Subject: [PATCH 13/28] chore: revert css-loader bump Signed-off-by: blam --- .changeset/honest-buttons-clean.md | 5 +++++ packages/cli/package.json | 2 +- yarn.lock | 28 ++-------------------------- 3 files changed, 8 insertions(+), 27 deletions(-) create mode 100644 .changeset/honest-buttons-clean.md diff --git a/.changeset/honest-buttons-clean.md b/.changeset/honest-buttons-clean.md new file mode 100644 index 0000000000..952aff614b --- /dev/null +++ b/.changeset/honest-buttons-clean.md @@ -0,0 +1,5 @@ +--- +'@backstage/cli': patch +--- + +Revert `css-loader@v7` bump diff --git a/packages/cli/package.json b/packages/cli/package.json index e5ad8f0d8f..9d93f6007b 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -88,7 +88,7 @@ "commander": "^12.0.0", "cross-fetch": "^4.0.0", "cross-spawn": "^7.0.3", - "css-loader": "^7.0.0", + "css-loader": "^6.5.1", "ctrlc-windows": "^2.1.0", "esbuild": "^0.24.0", "esbuild-loader": "^4.0.0", diff --git a/yarn.lock b/yarn.lock index 098bb4c173..1baea64682 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4027,7 +4027,7 @@ __metadata: commander: ^12.0.0 cross-fetch: ^4.0.0 cross-spawn: ^7.0.3 - css-loader: ^7.0.0 + css-loader: ^6.5.1 ctrlc-windows: ^2.1.0 del: ^8.0.0 esbuild: ^0.24.0 @@ -26339,7 +26339,7 @@ __metadata: languageName: node linkType: hard -"css-loader@npm:^6.7.1": +"css-loader@npm:^6.5.1, css-loader@npm:^6.7.1": version: 6.11.0 resolution: "css-loader@npm:6.11.0" dependencies: @@ -26363,30 +26363,6 @@ __metadata: languageName: node linkType: hard -"css-loader@npm:^7.0.0": - version: 7.1.2 - resolution: "css-loader@npm:7.1.2" - dependencies: - icss-utils: ^5.1.0 - postcss: ^8.4.33 - postcss-modules-extract-imports: ^3.1.0 - postcss-modules-local-by-default: ^4.0.5 - postcss-modules-scope: ^3.2.0 - postcss-modules-values: ^4.0.0 - postcss-value-parser: ^4.2.0 - semver: ^7.5.4 - peerDependencies: - "@rspack/core": 0.x || 1.x - webpack: ^5.27.0 - peerDependenciesMeta: - "@rspack/core": - optional: true - webpack: - optional: true - checksum: 15bfd90d778ddab90ee1d04c8c8bcc13ea6c0791d01b52b09d1b1c753b3410f7a7788a510d93726a9878e70b7c1a140f21efdf5c96e1857872107551d3897822 - languageName: node - linkType: hard - "css-select@npm:^4.1.3, css-select@npm:^4.2.1": version: 4.3.0 resolution: "css-select@npm:4.3.0" From 29180ec3d4ac400a7a594278823cfb8c73d2898a Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Wed, 11 Dec 2024 12:16:43 +0100 Subject: [PATCH 14/28] fix: move startup lifecycle back to plugin service Signed-off-by: Camila Belo --- .changeset/curvy-fishes-rule.md | 5 +++ .changeset/fast-tools-fix.md | 10 +++++ .changeset/rude-pears-tie.md | 1 - .../backend-defaults/report-httpRouter.api.md | 4 +- .../report-rootHttpRouter.api.md | 2 - .../createLifecycleMiddleware.test.ts | 27 ------------- .../createLifecycleMiddleware.ts | 18 +-------- .../http/createLifecycleMiddleware.test.ts | 18 +++++++-- .../http/createLifecycleMiddleware.ts | 26 +++++++++---- .../httpRouter/httpRouterServiceFactory.ts | 5 ++- .../rootHttpRouterServiceFactory.test.ts | 24 +++++------- .../rootHttpRouterServiceFactory.ts | 38 +++++++------------ 12 files changed, 77 insertions(+), 101 deletions(-) create mode 100644 .changeset/curvy-fishes-rule.md create mode 100644 .changeset/fast-tools-fix.md rename packages/backend-defaults/src/entrypoints/{rootHttpRouter => httpRouter}/createLifecycleMiddleware.test.ts (71%) rename packages/backend-defaults/src/entrypoints/{rootHttpRouter => httpRouter}/createLifecycleMiddleware.ts (85%) diff --git a/.changeset/curvy-fishes-rule.md b/.changeset/curvy-fishes-rule.md new file mode 100644 index 0000000000..2ce535166d --- /dev/null +++ b/.changeset/curvy-fishes-rule.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-defaults': patch +--- + +Fix server response time by moving the lifecycle startup hooks back to the plugin lifecycle service. diff --git a/.changeset/fast-tools-fix.md b/.changeset/fast-tools-fix.md new file mode 100644 index 0000000000..25dd292c82 --- /dev/null +++ b/.changeset/fast-tools-fix.md @@ -0,0 +1,10 @@ +--- +'@backstage/backend-defaults': minor +--- + +**BREAKING CHANGE**: The `LifecycleMiddlewareOptions.startupRequestPauseTimeout` has been removed. Use the `backend.lifecycle.startupRequestPauseTimeout` setting in your `app-config.yaml` file to customize how the `createLifecycleMiddleware` function should behave. Also the root config service is required as an options when calling the `createLifecycleMiddleware` function: + +```diff +- createLifecycleMiddleware({ lifecycle, startupRequestPauseTimeout }) ++ createLifecycleMiddleware({ config, lifecycle }) +``` diff --git a/.changeset/rude-pears-tie.md b/.changeset/rude-pears-tie.md index c0cb970727..e29d517959 100644 --- a/.changeset/rude-pears-tie.md +++ b/.changeset/rude-pears-tie.md @@ -3,4 +3,3 @@ --- Remove use of the `stoppable` library on the `DefaultRootHttpRouterService` as Node's native http server [close](https://nodejs.org/api/http.html#serverclosecallback) method already drains requests. -Also, we pass a new `lifecycleMiddleware` to the `rootHttpRouterServiceFactory` configure function that must be called manually if you don't call `applyDefaults`. diff --git a/packages/backend-defaults/report-httpRouter.api.md b/packages/backend-defaults/report-httpRouter.api.md index 4f6c6f06dc..55c3f0b208 100644 --- a/packages/backend-defaults/report-httpRouter.api.md +++ b/packages/backend-defaults/report-httpRouter.api.md @@ -10,7 +10,6 @@ import express from 'express'; import { HttpAuthService } from '@backstage/backend-plugin-api'; import { HttpRouterService } from '@backstage/backend-plugin-api'; import { HttpRouterServiceAuthPolicy } from '@backstage/backend-plugin-api'; -import { HumanDuration } from '@backstage/types'; import { LifecycleService } from '@backstage/backend-plugin-api'; import { RequestHandler } from 'express'; import { RootConfigService } from '@backstage/backend-plugin-api'; @@ -51,9 +50,10 @@ export const httpRouterServiceFactory: ServiceFactory< // @public export interface LifecycleMiddlewareOptions { + // (undocumented) + config: RootConfigService; // (undocumented) lifecycle: LifecycleService; - startupRequestPauseTimeout?: HumanDuration; } // (No @packageDocumentation comment for this package) diff --git a/packages/backend-defaults/report-rootHttpRouter.api.md b/packages/backend-defaults/report-rootHttpRouter.api.md index 4489fed107..41830ebf17 100644 --- a/packages/backend-defaults/report-rootHttpRouter.api.md +++ b/packages/backend-defaults/report-rootHttpRouter.api.md @@ -134,8 +134,6 @@ export interface RootHttpRouterConfigureContext { // (undocumented) lifecycle: LifecycleService; // (undocumented) - lifecycleMiddleware: RequestHandler; - // (undocumented) logger: LoggerService; // (undocumented) middleware: MiddlewareFactory; diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts b/packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.test.ts similarity index 71% rename from packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts rename to packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.test.ts index 76af28cc6e..31eecf14bb 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.test.ts @@ -78,31 +78,4 @@ describe('createLifecycleMiddleware', () => { new ServiceUnavailableError('Service has not started up yet'), ); }); - - it('should delay service shutdown for the default timeout duration', async () => { - jest.useFakeTimers(); - const defaultTimeout = 30000; - const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); - createLifecycleMiddleware({ lifecycle }); - const beforeShutdownPromise = lifecycle.beforeShutdown().then(() => { - jest.useRealTimers(); - }); - jest.advanceTimersByTime(defaultTimeout); - return expect(beforeShutdownPromise).resolves.toBeUndefined(); - }); - - it('should delay service shutdown for the configured timeout duration - time in human duration', async () => { - jest.useFakeTimers(); - const configuredTimeout = 20000; - const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); - createLifecycleMiddleware({ - lifecycle, - serverShutdownDelay: { milliseconds: configuredTimeout }, - }); - const beforeShutdownPromise = lifecycle.beforeShutdown().then(() => { - jest.useRealTimers(); - }); - jest.advanceTimersByTime(configuredTimeout); - return expect(beforeShutdownPromise).resolves.toBeUndefined(); - }); }); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts b/packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.ts similarity index 85% rename from packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts rename to packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.ts index 3745f7f48d..99fb82a41e 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.ts @@ -34,12 +34,6 @@ export interface LifecycleMiddlewareOptions { * Defaults to 5 seconds. */ startupRequestPauseTimeout?: HumanDuration; - /** - * The maximum time that the server will wait for stop accepting traffic, before returning an error. - * - * Defaults to 0 seconds. - */ - serverShutdownDelay?: HumanDuration; } /** @@ -59,8 +53,7 @@ export interface LifecycleMiddlewareOptions { export function createLifecycleMiddleware( options: LifecycleMiddlewareOptions, ): RequestHandler { - const { lifecycle, startupRequestPauseTimeout, serverShutdownDelay } = - options; + const { lifecycle, startupRequestPauseTimeout } = options; let state: 'init' | 'up' | 'down' = 'init'; const waiting = new Set<{ @@ -79,15 +72,6 @@ export function createLifecycleMiddleware( } }); - lifecycle.addBeforeShutdownHook(async () => { - const timeoutMs = durationToMilliseconds( - serverShutdownDelay ?? DEFAULT_SERVER_SHUTDOWN_TIMEOUT, - ); - return await new Promise(resolve => { - setTimeout(resolve, timeoutMs); - }); - }); - lifecycle.addShutdownHook(async () => { state = 'down'; for (const item of waiting) { diff --git a/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.test.ts b/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.test.ts index 3b85c8cb92..36ca2da61b 100644 --- a/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.test.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.test.ts @@ -20,10 +20,14 @@ import { mockServices } from '@backstage/backend-test-utils'; import { ServiceUnavailableError } from '@backstage/errors'; describe('createLifecycleMiddleware', () => { + const config = mockServices.rootConfig.mock(); it('should pause requests when plugin is not ready', async () => { const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); - const middleware = createLifecycleMiddleware({ lifecycle }); + const middleware = createLifecycleMiddleware({ + config, + lifecycle, + }); const next = jest.fn(); middleware({} as any, {} as any, next); @@ -41,7 +45,7 @@ describe('createLifecycleMiddleware', () => { it('should throw ServiceUnavailableError after shutdown', async () => { const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); - const middleware = createLifecycleMiddleware({ lifecycle }); + const middleware = createLifecycleMiddleware({ config, lifecycle }); const next = jest.fn(); middleware({} as any, {} as any, next); @@ -65,7 +69,15 @@ describe('createLifecycleMiddleware', () => { const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); const middleware = createLifecycleMiddleware({ lifecycle, - startupRequestPauseTimeout: { milliseconds: 1 }, + config: mockServices.rootConfig({ + data: { + backend: { + lifecycle: { + startupRequestPauseTimeout: { milliseconds: 1 }, + }, + }, + }, + }), }); const next = jest.fn(); diff --git a/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts b/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts index 3f8ee3ea69..444b0c08f9 100644 --- a/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts @@ -14,7 +14,11 @@ * limitations under the License. */ -import { LifecycleService } from '@backstage/backend-plugin-api'; +import { + RootConfigService, + LifecycleService, +} from '@backstage/backend-plugin-api'; +import { readDurationFromConfig } from '@backstage/config'; import { ServiceUnavailableError } from '@backstage/errors'; import { HumanDuration, durationToMilliseconds } from '@backstage/types'; import { RequestHandler } from 'express'; @@ -26,13 +30,8 @@ export const DEFAULT_TIMEOUT = { seconds: 5 }; * @public */ export interface LifecycleMiddlewareOptions { + config: RootConfigService; lifecycle: LifecycleService; - /** - * The maximum time that paused requests will wait for the service to start, before returning an error. - * - * Defaults to 5 seconds. - */ - startupRequestPauseTimeout?: HumanDuration; } /** @@ -52,7 +51,7 @@ export interface LifecycleMiddlewareOptions { export function createLifecycleMiddleware( options: LifecycleMiddlewareOptions, ): RequestHandler { - const { lifecycle, startupRequestPauseTimeout = DEFAULT_TIMEOUT } = options; + const { lifecycle } = options; let state: 'init' | 'up' | 'down' = 'init'; const waiting = new Set<{ @@ -81,6 +80,17 @@ export function createLifecycleMiddleware( waiting.clear(); }); + let startupRequestPauseTimeout: HumanDuration = DEFAULT_TIMEOUT; + + if ( + 'config' in options && + options.config.has('backend.lifecycle.startupRequestPauseTimeout') + ) { + startupRequestPauseTimeout = readDurationFromConfig(options.config, { + key: 'backend.lifecycle.startupRequestPauseTimeout', + }); + } + const timeoutMs = durationToMilliseconds(startupRequestPauseTimeout); return (_req, _res, next) => { diff --git a/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts index a448827e16..6f8792aa3d 100644 --- a/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts @@ -22,6 +22,7 @@ import { HttpRouterServiceAuthPolicy, } from '@backstage/backend-plugin-api'; import { + createLifecycleMiddleware, createCookieAuthRefreshMiddleware, createCredentialsBarrier, createAuthIntegrationRouter, @@ -42,11 +43,12 @@ export const httpRouterServiceFactory = createServiceFactory({ deps: { plugin: coreServices.pluginMetadata, config: coreServices.rootConfig, + lifecycle: coreServices.lifecycle, rootHttpRouter: coreServices.rootHttpRouter, auth: coreServices.auth, httpAuth: coreServices.httpAuth, }, - async factory({ auth, httpAuth, config, plugin, rootHttpRouter }) { + async factory({ auth, httpAuth, config, plugin, rootHttpRouter, lifecycle }) { const router = PromiseRouter(); rootHttpRouter.use(`/api/${plugin.getId()}`, router); @@ -57,6 +59,7 @@ export const httpRouterServiceFactory = createServiceFactory({ }); router.use(createAuthIntegrationRouter({ auth })); + router.use(createLifecycleMiddleware({ config, lifecycle })); router.use(credentialsBarrier.middleware); router.use(createCookieAuthRefreshMiddleware({ auth, httpAuth })); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts index fb0d6cdad0..0f2b6463e4 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts @@ -207,20 +207,21 @@ describe('rootHttpRouterServiceFactory', () => { it('should wait the server to shutdown', async () => { jest.useFakeTimers(); + const serverStopMock = jest.fn(); + let app: Express | undefined = undefined; const lifecycleMock = new BackendLifecycleImpl(mockServices.rootLogger()); const tester = ServiceFactoryTester.from( rootHttpRouterServiceFactory({ configure(options) { - console.log('configure'); options.app.use(options.healthRouter); - options.app.use(options.lifecycleMiddleware); options.app.get('/test', (_req, res) => { res.status(200).send({ status: 'ok' }).end(); }); options.app.use(options.middleware.error()); app = options.app; + options.server.addListener('close', serverStopMock); }, }), { @@ -306,18 +307,6 @@ describe('rootHttpRouterServiceFactory', () => { jest.advanceTimersByTime(1); - // No longer accepting requests after shutdown - await request(app!) - .get('/test') - .expect(503, { - error: { - name: 'ServiceUnavailableError', - message: 'Service is shutting down', - }, - request: { method: 'GET', url: '/test' }, - response: { statusCode: 503 }, - }); - await request(app) .get('/.backstage/health/v1/liveness') .expect(200, { status: 'ok' }); @@ -327,6 +316,11 @@ describe('rootHttpRouterServiceFactory', () => { status: 'error', }); - return await expect(beforeShutdownPromise).resolves.toBeUndefined(); + return expect( + beforeShutdownPromise.then(() => { + expect(serverStopMock).toHaveBeenCalled(); + jest.useRealTimers(); + }), + ).resolves.toBeUndefined(); }); }); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts index 4afdfb1ec2..e5c64124fd 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts @@ -30,9 +30,8 @@ import { } from './http'; import { DefaultRootHttpRouter } from './DefaultRootHttpRouter'; import { createHealthRouter } from './createHealthRouter'; -import { createLifecycleMiddleware } from './createLifecycleMiddleware'; +import { durationToMilliseconds } from '@backstage/types'; import { readDurationFromConfig } from '@backstage/config'; -import { HumanDuration } from '@backstage/types'; /** * @public @@ -46,7 +45,6 @@ export interface RootHttpRouterConfigureContext { logger: LoggerService; lifecycle: LifecycleService; healthRouter: RequestHandler; - lifecycleMiddleware: RequestHandler; applyDefaults: () => void; } @@ -95,26 +93,6 @@ const rootHttpRouterServiceFactoryWithOptions = ( const healthRouter = createHealthRouter({ config, health }); - let startupRequestPauseTimeout: HumanDuration | undefined; - if (config.has('backend.lifecycle.startupRequestPauseTimeout')) { - startupRequestPauseTimeout = readDurationFromConfig(config, { - key: 'backend.lifecycle.startupRequestPauseTimeout', - }); - } - - let serverShutdownDelay: HumanDuration | undefined; - if (config.has('backend.lifecycle.serverShutdownDelay')) { - serverShutdownDelay = readDurationFromConfig(config, { - key: 'backend.lifecycle.serverShutdownDelay', - }); - } - - const lifecycleMiddleware = createLifecycleMiddleware({ - lifecycle, - startupRequestPauseTimeout, - serverShutdownDelay, - }); - const server = await createHttpServer( app, readHttpServerOptions(config.getOptionalConfig('backend')), @@ -130,7 +108,6 @@ const rootHttpRouterServiceFactoryWithOptions = ( logger, lifecycle, healthRouter, - lifecycleMiddleware, applyDefaults() { if (process.env.NODE_ENV === 'development') { app.set('json spaces', 2); @@ -140,13 +117,24 @@ const rootHttpRouterServiceFactoryWithOptions = ( app.use(middleware.compression()); app.use(middleware.logging()); app.use(healthRouter); - app.use(lifecycleMiddleware); app.use(routes); app.use(middleware.notFound()); app.use(middleware.error()); }, }); + if (config.has('backend.lifecycle.serverShutdownDelay')) { + const serverShutdownDelay = readDurationFromConfig(config, { + key: 'backend.lifecycle.serverShutdownDelay', + }); + lifecycle.addBeforeShutdownHook(async () => { + const timeoutMs = durationToMilliseconds(serverShutdownDelay); + return await new Promise(resolve => { + setTimeout(resolve, timeoutMs); + }); + }); + } + lifecycle.addShutdownHook(() => server.stop()); await server.start(); From dff4a9706757c19c120ec5e823ce85a339389142 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Thu, 12 Dec 2024 13:34:06 +0100 Subject: [PATCH 15/28] Fixups Co-authored-by: Patrik Oldsberg Signed-off-by: Camila Belo --- .changeset/fast-tools-fix.md | 2 +- .../httpRouter/http/createLifecycleMiddleware.ts | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/.changeset/fast-tools-fix.md b/.changeset/fast-tools-fix.md index 25dd292c82..11807f045e 100644 --- a/.changeset/fast-tools-fix.md +++ b/.changeset/fast-tools-fix.md @@ -2,7 +2,7 @@ '@backstage/backend-defaults': minor --- -**BREAKING CHANGE**: The `LifecycleMiddlewareOptions.startupRequestPauseTimeout` has been removed. Use the `backend.lifecycle.startupRequestPauseTimeout` setting in your `app-config.yaml` file to customize how the `createLifecycleMiddleware` function should behave. Also the root config service is required as an options when calling the `createLifecycleMiddleware` function: +**BREAKING PRODUCERS**: The `LifecycleMiddlewareOptions.startupRequestPauseTimeout` has been removed. Use the `backend.lifecycle.startupRequestPauseTimeout` setting in your `app-config.yaml` file to customize how the `createLifecycleMiddleware` function should behave. Also the root config service is required as an option when calling the `createLifecycleMiddleware` function: ```diff - createLifecycleMiddleware({ lifecycle, startupRequestPauseTimeout }) diff --git a/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts b/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts index 444b0c08f9..366d19f8bf 100644 --- a/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts @@ -51,7 +51,7 @@ export interface LifecycleMiddlewareOptions { export function createLifecycleMiddleware( options: LifecycleMiddlewareOptions, ): RequestHandler { - const { lifecycle } = options; + const { config, lifecycle } = options; let state: 'init' | 'up' | 'down' = 'init'; const waiting = new Set<{ @@ -82,11 +82,8 @@ export function createLifecycleMiddleware( let startupRequestPauseTimeout: HumanDuration = DEFAULT_TIMEOUT; - if ( - 'config' in options && - options.config.has('backend.lifecycle.startupRequestPauseTimeout') - ) { - startupRequestPauseTimeout = readDurationFromConfig(options.config, { + if (config.has('backend.lifecycle.startupRequestPauseTimeout')) { + startupRequestPauseTimeout = readDurationFromConfig(config, { key: 'backend.lifecycle.startupRequestPauseTimeout', }); } From 02bd2cb19c2faa755518b8eb42fdcf5d8c1c29e9 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Dec 2024 12:00:33 +0100 Subject: [PATCH 16/28] catalog-backend: avoid JSON parsing when possible Signed-off-by: Patrik Oldsberg --- .changeset/curly-teachers-marry.md | 5 ++ .../config/vocabularies/Backstage/accept.txt | 1 + plugins/catalog-backend/src/catalog/types.ts | 20 ++++- .../service/AuthorizedEntitiesCatalog.test.ts | 2 +- .../src/service/AuthorizedEntitiesCatalog.ts | 11 ++- .../src/service/CatalogBuilder.ts | 12 ++- .../service/DefaultEntitiesCatalog.test.ts | 55 ++++++++---- .../src/service/DefaultEntitiesCatalog.ts | 48 +++++----- .../src/service/createRouter.test.ts | 34 +++---- .../src/service/createRouter.ts | 36 +++++--- .../src/service/response/index.ts | 22 +++++ .../src/service/response/process.ts | 62 +++++++++++++ .../src/service/response/write.ts | 89 +++++++++++++++++++ plugins/catalog-backend/src/service/util.ts | 56 +++++++----- .../src/tests/integration.test.ts | 8 +- 15 files changed, 361 insertions(+), 100 deletions(-) create mode 100644 .changeset/curly-teachers-marry.md create mode 100644 plugins/catalog-backend/src/service/response/index.ts create mode 100644 plugins/catalog-backend/src/service/response/process.ts create mode 100644 plugins/catalog-backend/src/service/response/write.ts diff --git a/.changeset/curly-teachers-marry.md b/.changeset/curly-teachers-marry.md new file mode 100644 index 0000000000..21f41da291 --- /dev/null +++ b/.changeset/curly-teachers-marry.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': minor +--- + +Added a new `catalog.enableRawJsonResponse` configuration option that avoids JSON deserialization and serialization if possible when reading entities. This can significantly improve the overall performance of the catalog, but it removes the backwards compatibility processing that ensures that both `entity.relation[].target` and `entity.relation[].targetRef` are present in returned entities. diff --git a/.github/vale/config/vocabularies/Backstage/accept.txt b/.github/vale/config/vocabularies/Backstage/accept.txt index e0389f0adb..fc0fd5021e 100644 --- a/.github/vale/config/vocabularies/Backstage/accept.txt +++ b/.github/vale/config/vocabularies/Backstage/accept.txt @@ -97,6 +97,7 @@ deliverables denormalized dependabot deps +deserialization destructured destructuring Deutsche diff --git a/plugins/catalog-backend/src/catalog/types.ts b/plugins/catalog-backend/src/catalog/types.ts index b3e52fb480..d00c38f536 100644 --- a/plugins/catalog-backend/src/catalog/types.ts +++ b/plugins/catalog-backend/src/catalog/types.ts @@ -52,8 +52,22 @@ export type EntitiesRequest = { credentials: BackstageCredentials; }; +/** + * Encapsulates either a deserialized or serialized entities to be sent in a response. + * @internal + */ +export type EntitiesResponseItems = + | { + type: 'objects'; + entities: (Entity | null)[]; + } + | { + type: 'raw'; + entities: (string | null)[]; + }; + export type EntitiesResponse = { - entities: Entity[]; + entities: EntitiesResponseItems; pageInfo: PageInfo; }; @@ -86,7 +100,7 @@ export interface EntitiesBatchResponse { * The list of entities, in the same order as the refs in the request. Entries * that are null signify that no entity existed with that ref. */ - items: Array; + items: EntitiesResponseItems; } export type EntityAncestryResponse = { @@ -224,7 +238,7 @@ export interface QueryEntitiesResponse { /** * The entities for the current pagination request */ - items: Entity[]; + items: EntitiesResponseItems; pageInfo: { /** diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts index 3e2dee7ae0..2207802859 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts @@ -226,7 +226,7 @@ describe('AuthorizedEntitiesCatalog', () => { ]; fakeCatalog.queryEntities.mockResolvedValue({ - items: entities, + items: { type: 'objects', entities }, pageInfo: { nextCursor: { isPrevious: false, diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts index 5358e90825..225c87cae9 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts @@ -60,7 +60,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { if (authorizeDecision.result === AuthorizeResult.DENY) { return { - entities: [], + entities: { type: 'objects', entities: [] }, pageInfo: { hasNextPage: false }, }; } @@ -92,7 +92,10 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { if (authorizeDecision.result === AuthorizeResult.DENY) { return { - items: new Array(request.entityRefs.length).fill(null), + items: { + type: 'objects', + entities: new Array(request.entityRefs.length).fill(null), + }, }; } @@ -123,7 +126,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { if (authorizeDecision.result === AuthorizeResult.DENY) { return { - items: [], + items: { type: 'objects', entities: [] }, pageInfo: {}, totalItems: 0, }; @@ -208,7 +211,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { allOf: [permissionFilter, basicEntityFilter({ 'metadata.uid': uid })], }, }); - if (entities.length === 0) { + if (entities.entities.length === 0) { throw new NotAllowedError(); } } diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index 31ec40b887..fd052bf474 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -115,6 +115,7 @@ import { UrlReaderService, SchedulerService, } from '@backstage/backend-plugin-api'; +import { entitiesResponseToObjects } from './response'; /** * This is a duplicate of the alpha `CatalogPermissionRule` type, for use in the stable API. @@ -485,6 +486,10 @@ export class CatalogBuilder { discovery, }); + const enableRawJson = config.getOptionalBoolean( + 'catalog.enableRawJsonResponses', + ); + const policy = this.buildEntityPolicy(); const processors = this.buildProcessors(); const parser = this.parser || defaultEntityDataParser; @@ -521,6 +526,7 @@ export class CatalogBuilder { database: dbClient, logger, stitcher, + enableRawJson, }); let permissionsService: PermissionsService; @@ -566,7 +572,10 @@ export class CatalogBuilder { }, }); - const entitiesByRef = keyBy(entities, stringifyEntityRef); + const entitiesByRef = keyBy( + entitiesResponseToObjects(entities), + stringifyEntityRef, + ); return resourceRefs.map( resourceRef => @@ -629,6 +638,7 @@ export class CatalogBuilder { auth, httpAuth, permissionsService, + enableRawJson, }); await connectEntityProviders(providerDatabase, entityProviders); diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts index bb2ba3d587..379c861ba1 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts @@ -38,6 +38,7 @@ import { Stitcher } from '../stitching/types'; import { DefaultEntitiesCatalog } from './DefaultEntitiesCatalog'; import { EntitiesRequest } from '../catalog/types'; import { buildEntitySearch } from '../database/operations/stitcher/buildEntitySearch'; +import { entitiesResponseToObjects } from './response'; jest.setTimeout(60_000); @@ -310,10 +311,11 @@ describe('DefaultEntitiesCatalog', () => { const testFilter = { key: 'spec.test', }; - const { entities } = await catalog.entities({ + const res = await catalog.entities({ filter: testFilter, credentials: mockCredentials.none(), }); + const entities = entitiesResponseToObjects(res.entities); expect(entities.length).toBe(1); expect(entities[0]).toEqual(entity2); @@ -351,10 +353,11 @@ describe('DefaultEntitiesCatalog', () => { key: 'spec.test', }, }; - const { entities } = await catalog.entities({ + const res = await catalog.entities({ filter: testFilter, credentials: mockCredentials.none(), }); + const entities = entitiesResponseToObjects(res.entities); expect(entities.length).toBe(1); expect(entities[0]).toEqual(entity1); @@ -416,7 +419,7 @@ describe('DefaultEntitiesCatalog', () => { values: ['red'], }, }; - const { entities } = await catalog.entities({ + const res = await catalog.entities({ filter: { allOf: [ testFilter1, @@ -427,6 +430,7 @@ describe('DefaultEntitiesCatalog', () => { }, credentials: mockCredentials.none(), }); + const entities = entitiesResponseToObjects(res.entities); expect(entities.length).toBe(2); expect(entities).toContainEqual(entity2); @@ -465,7 +469,7 @@ describe('DefaultEntitiesCatalog', () => { const testFilter2 = { key: 'metadata.desc', }; - const { entities } = await catalog.entities({ + const res = await catalog.entities({ filter: { not: { allOf: [testFilter1, testFilter2], @@ -474,6 +478,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }); + const entities = entitiesResponseToObjects(res.entities); expect(entities.length).toBe(1); expect(entities).toContainEqual(entity1); @@ -509,10 +514,11 @@ describe('DefaultEntitiesCatalog', () => { key: 'kind', values: [], }; - const { entities } = await catalog.entities({ + const res = await catalog.entities({ filter: testFilter, credentials: mockCredentials.none(), }); + const entities = entitiesResponseToObjects(res.entities); expect(entities.length).toBe(0); }, @@ -553,10 +559,11 @@ describe('DefaultEntitiesCatalog', () => { stitcher, }); - const { entities } = await catalog.entities(); + const res = await catalog.entities(); + const entities = entitiesResponseToObjects(res.entities); expect( - entities.find(e => e.metadata.name === 'one')!.relations, + entities.find(e => e?.metadata.name === 'one')!.relations, ).toEqual([ { type: 'r', @@ -565,7 +572,7 @@ describe('DefaultEntitiesCatalog', () => { }, ]); expect( - entities.find(e => e.metadata.name === 'two')!.relations, + entities.find(e => e?.metadata.name === 'two')!.relations, ).toEqual([ { type: 'r', @@ -615,7 +622,9 @@ describe('DefaultEntitiesCatalog', () => { return catalog .entities({ ...request, credentials: mockCredentials.none() }) .then(response => - response.entities.map(e => e.metadata.name).toSorted(), + entitiesResponseToObjects(response.entities) + .map(e => e!.metadata.name) + .toSorted(), ); } @@ -678,7 +687,11 @@ describe('DefaultEntitiesCatalog', () => { ): Promise { return catalog .entities({ ...request, credentials: mockCredentials.none() }) - .then(response => response.entities.map(e => e.metadata.name)); + .then(response => + entitiesResponseToObjects(response.entities).map( + e => e!.metadata.name, + ), + ); } await expect( @@ -764,7 +777,7 @@ describe('DefaultEntitiesCatalog', () => { stitcher, }); - const { items } = await catalog.entitiesBatch({ + const res = await catalog.entitiesBatch({ entityRefs: [ 'k:default/two', 'k:default/one', @@ -775,6 +788,7 @@ describe('DefaultEntitiesCatalog', () => { ], credentials: mockCredentials.none(), }); + const items = entitiesResponseToObjects(res.items); expect(items.map(e => e && stringifyEntityRef(e))).toEqual([ 'k:default/two', @@ -819,11 +833,12 @@ describe('DefaultEntitiesCatalog', () => { stitcher, }); - const { items } = await catalog.entitiesBatch({ + const res = await catalog.entitiesBatch({ entityRefs: ['k:default/two', 'k:default/one'], filter: { key: 'spec.owner', values: ['me'] }, credentials: mockCredentials.none(), }); + const items = entitiesResponseToObjects(res.items); expect(items.map(e => e && stringifyEntityRef(e))).toEqual([ 'k:default/two', @@ -1830,7 +1845,9 @@ describe('DefaultEntitiesCatalog', () => { orderFields: [{ field: 'metadata.title', order: 'asc' }], credentials: mockCredentials.none(), }) - .then(r => r.items.map(e => e.metadata.name)), + .then(r => + entitiesResponseToObjects(r.items).map(e => e!.metadata.name), + ), ).resolves.toEqual(['CC', 'BB', 'AA']); // 'AA' has no title, ends up last await expect( @@ -1839,7 +1856,9 @@ describe('DefaultEntitiesCatalog', () => { orderFields: [{ field: 'metadata.title', order: 'desc' }], credentials: mockCredentials.none(), }) - .then(r => r.items.map(e => e.metadata.name)), + .then(r => + entitiesResponseToObjects(r.items).map(e => e!.metadata.name), + ), ).resolves.toEqual(['BB', 'CC', 'AA']); // 'AA' has no title, ends up last }, ); @@ -1869,7 +1888,9 @@ describe('DefaultEntitiesCatalog', () => { limit: 10, credentials: mockCredentials.none(), }) - .then(r => r.items.map(e => e.metadata.name)), + .then(r => + entitiesResponseToObjects(r.items).map(e => e!.metadata.name), + ), ).resolves.toEqual(['AA', 'BB']); // simulate a situation where stitching is not yet complete @@ -1884,7 +1905,9 @@ describe('DefaultEntitiesCatalog', () => { limit: 10, credentials: mockCredentials.none(), }) - .then(r => r.items.map(e => e.metadata.name)), + .then(r => + entitiesResponseToObjects(r.items).map(e => e!.metadata.name), + ), ).resolves.toEqual(['BB']); }, ); diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts index cddc95f496..ee9b46ce55 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts @@ -45,13 +45,14 @@ import { import { Stitcher } from '../stitching/types'; import { - expandLegacyCompoundRelationRefsInResponse, + expandLegacyCompoundRelationsInEntity, isQueryEntitiesCursorRequest, isQueryEntitiesInitialRequest, } from './util'; import { EntityFilter } from '@backstage/plugin-catalog-node'; import { LoggerService } from '@backstage/backend-plugin-api'; import { applyEntityFilterToQuery } from './request/applyEntityFilterToQuery'; +import { processRawEntitiesResult } from './response'; const DEFAULT_LIMIT = 200; @@ -104,15 +105,18 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { private readonly database: Knex; private readonly logger: LoggerService; private readonly stitcher: Stitcher; + private readonly enableRawJson: boolean; constructor(options: { database: Knex; logger: LoggerService; stitcher: Stitcher; + enableRawJson?: boolean; }) { this.database = options.database; this.logger = options.logger; this.stitcher = options.stitcher; + this.enableRawJson = Boolean(options.enableRawJson); } async entities(request?: EntitiesRequest): Promise { @@ -190,16 +194,19 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { }; } - let entities: Entity[] = rows.map(e => JSON.parse(e.final_entity!)); - - if (request?.fields) { - entities = entities.map(e => request.fields!(e)); - } - - expandLegacyCompoundRelationRefsInResponse(entities); - return { - entities, + entities: processRawEntitiesResult( + rows.map(r => r.final_entity!), + this.enableRawJson + ? request?.fields + : e => { + expandLegacyCompoundRelationsInEntity(e); + if (request?.fields) { + return request.fields(e); + } + return e; + }, + ), pageInfo, }; } @@ -207,7 +214,7 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { async entitiesBatch( request: EntitiesBatchRequest, ): Promise { - const lookup = new Map(); + const lookup = new Map(); for (const chunk of lodashChunk(request.entityRefs, 200)) { let query = this.database('final_entities') @@ -227,17 +234,13 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { } for (const row of await query) { - lookup.set(row.entityRef, row.entity ? JSON.parse(row.entity) : null); + lookup.set(row.entityRef, row.entity ? row.entity : null); } } - let items = request.entityRefs.map(ref => lookup.get(ref) ?? null); + const items = request.entityRefs.map(ref => lookup.get(ref) ?? null); - if (request.fields) { - items = items.map(e => e && request.fields!(e)); - } - - return { items }; + return { items: processRawEntitiesResult(items, request.fields) }; } async queryEntities( @@ -505,12 +508,11 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { } : undefined; - const items = rows - .map(e => JSON.parse(e.final_entity!)) - .map(e => (request.fields ? request.fields(e) : e)); - return { - items, + items: processRawEntitiesResult( + rows.map(r => r.final_entity!), + request.fields, + ), pageInfo: { ...(!!prevCursor && { prevCursor }), ...(!!nextCursor && { nextCursor }), diff --git a/plugins/catalog-backend/src/service/createRouter.test.ts b/plugins/catalog-backend/src/service/createRouter.test.ts index e728017f0b..8efb6543a3 100644 --- a/plugins/catalog-backend/src/service/createRouter.test.ts +++ b/plugins/catalog-backend/src/service/createRouter.test.ts @@ -136,7 +136,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: [entities[0]], + items: { type: 'objects', entities: [entities[0]] }, pageInfo: {}, totalItems: 1, }); @@ -149,7 +149,7 @@ describe('createRouter readonly disabled', () => { it('parses single and multiple request parameters and passes them down', async () => { entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: [], + items: { type: 'objects', entities: [] }, pageInfo: {}, totalItems: 0, }); @@ -185,7 +185,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items, + items: { type: 'objects', entities: items }, totalItems: 100, pageInfo: { nextCursor: mockCursor() }, }); @@ -203,7 +203,7 @@ describe('createRouter readonly disabled', () => { it('parses initial request', async () => { entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: [], + items: { type: 'objects', entities: [] }, pageInfo: {}, totalItems: 0, }); @@ -239,7 +239,7 @@ describe('createRouter readonly disabled', () => { it('parses encoded params request', async () => { entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: [], + items: { type: 'objects', entities: [] }, pageInfo: {}, totalItems: 0, }); @@ -283,7 +283,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items, + items: { type: 'objects', entities: items }, totalItems: 100, pageInfo: { nextCursor: mockCursor() }, }); @@ -312,7 +312,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items, + items: { type: 'objects', entities: items }, totalItems: 100, pageInfo: { nextCursor: mockCursor({ fullTextFilter: { term: 'mySearch' } }), @@ -354,7 +354,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items, + items: { type: 'objects', entities: items }, totalItems: 100, pageInfo: { nextCursor: mockCursor() }, }); @@ -379,7 +379,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items, + items: { type: 'objects', entities: items }, totalItems: 100, pageInfo: { nextCursor: mockCursor() }, }); @@ -402,7 +402,7 @@ describe('createRouter readonly disabled', () => { }, }; entitiesCatalog.entities.mockResolvedValue({ - entities: [entity], + entities: { type: 'objects', entities: [entity] }, pageInfo: { hasNextPage: false }, }); @@ -419,7 +419,7 @@ describe('createRouter readonly disabled', () => { it('responds with a 404 for missing entities', async () => { entitiesCatalog.entities.mockResolvedValue({ - entities: [], + entities: { type: 'objects', entities: [] }, pageInfo: { hasNextPage: false }, }); @@ -446,7 +446,7 @@ describe('createRouter readonly disabled', () => { }, }; entitiesCatalog.entitiesBatch.mockResolvedValue({ - items: [entity], + items: { type: 'objects', entities: [entity] }, }); const response = await request(app).get('/entities/by-name/k/ns/n'); @@ -462,7 +462,7 @@ describe('createRouter readonly disabled', () => { it('responds with a 404 for missing entities', async () => { entitiesCatalog.entitiesBatch.mockResolvedValue({ - items: [null], + items: { type: 'objects', entities: [null] }, }); const response = await request(app).get('/entities/by-name/b/d/c'); @@ -533,7 +533,9 @@ describe('createRouter readonly disabled', () => { }, }; const entityRef = stringifyEntityRef(entity); - entitiesCatalog.entitiesBatch.mockResolvedValue({ items: [entity] }); + entitiesCatalog.entitiesBatch.mockResolvedValue({ + items: { type: 'objects', entities: [entity] }, + }); const response = await request(app) .post('/entities/by-refs?filter=kind=Component') .set('Content-Type', 'application/json') @@ -908,7 +910,7 @@ describe('createRouter readonly enabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: [entities[0]], + items: { type: 'objects', entities: [entities[0]] }, pageInfo: {}, totalItems: 1, }); @@ -1128,7 +1130,7 @@ describe('NextRouter permissioning', () => { }, }; entitiesCatalog.entities.mockResolvedValueOnce({ - entities: [spideySense], + entities: { type: 'objects', entities: [spideySense] }, pageInfo: { hasNextPage: false }, }); diff --git a/plugins/catalog-backend/src/service/createRouter.ts b/plugins/catalog-backend/src/service/createRouter.ts index d70419c565..b670253089 100644 --- a/plugins/catalog-backend/src/service/createRouter.ts +++ b/plugins/catalog-backend/src/service/createRouter.ts @@ -44,7 +44,7 @@ import { createEntityArrayJsonStream, disallowReadonlyMode, encodeCursor, - expandLegacyCompoundRelationRefsInResponse, + expandLegacyCompoundRelationsInEntity, locationInput, validateRequestBody, } from './util'; @@ -60,6 +60,11 @@ import { import { LocationAnalyzer } from '@backstage/plugin-catalog-node'; import { AuthorizedValidationService } from './AuthorizedValidationService'; import { DeferredPromise, createDeferred } from '@backstage/types'; +import { + processEntitiesResponseItems, + writeEntitiesResponse, + writeSingleEntityResponse, +} from './response'; /** * Options used by {@link createRouter}. @@ -80,6 +85,7 @@ export interface RouterOptions { auth: AuthService; httpAuth: HttpAuthService; permissionsService: PermissionsService; + enableRawJson?: boolean; } /** @@ -107,6 +113,7 @@ export async function createRouter( permissionsService, auth, httpAuth, + enableRawJson = false, } = options; const readonlyEnabled = @@ -164,7 +171,7 @@ export async function createRouter( res.setHeader('link', `<${url.pathname}${url.search}>; rel="next"`); } - res.json(entities); + writeEntitiesResponse(res, entities); return; } @@ -203,12 +210,17 @@ export async function createRouter( : { credentials, fields, limit, cursor }, ); - if (result.items.length) { + if (result.items.entities.length) { await locks?.writeLock; signal.throwIfAborted(); - expandLegacyCompoundRelationRefsInResponse(result.items); + if (!enableRawJson) { + processEntitiesResponseItems( + result.items, + expandLegacyCompoundRelationsInEntity, + ); + } if (!responseStream.send(result.items)) { // The kernel buffer is full. Create the lock but do not await it // yet - we can better spend our time going to the next round of @@ -241,8 +253,8 @@ export async function createRouter( credentials: await httpAuth.credentials(req), }); - res.json({ - items, + writeEntitiesResponse(res, items, entities => ({ + items: entities, totalItems, pageInfo: { ...(pageInfo.nextCursor && { @@ -252,7 +264,7 @@ export async function createRouter( prevCursor: encodeCursor(pageInfo.prevCursor), }), }, - }); + })); }) .get('/entities/by-uid/:uid', async (req, res) => { const { uid } = req.params; @@ -260,10 +272,9 @@ export async function createRouter( filter: basicEntityFilter({ 'metadata.uid': uid }), credentials: await httpAuth.credentials(req), }); - if (!entities.length) { + if (!writeSingleEntityResponse(res, entities)) { throw new NotFoundError(`No entity with uid ${uid}`); } - res.status(200).json(entities[0]); }) .delete('/entities/by-uid/:uid', async (req, res) => { const { uid } = req.params; @@ -278,12 +289,11 @@ export async function createRouter( entityRefs: [stringifyEntityRef({ kind, namespace, name })], credentials: await httpAuth.credentials(req), }); - if (!items[0]) { + if (!writeSingleEntityResponse(res, items)) { throw new NotFoundError( `No entity named '${name}' found, with kind '${kind}' in namespace '${namespace}'`, ); } - res.status(200).json(items[0]); }) .get( '/entities/by-name/:kind/:namespace/:name/ancestry', @@ -298,13 +308,13 @@ export async function createRouter( ) .post('/entities/by-refs', async (req, res) => { const request = entitiesBatchRequest(req); - const response = await entitiesCatalog.entitiesBatch({ + const { items } = await entitiesCatalog.entitiesBatch({ entityRefs: request.entityRefs, filter: parseEntityFilterParams(req.query), fields: parseEntityTransformParams(req.query, request.fields), credentials: await httpAuth.credentials(req), }); - res.status(200).json(response); + writeEntitiesResponse(res, items, entities => ({ items: entities })); }) .get('/entity-facets', async (req, res) => { const response = await entitiesCatalog.facets({ diff --git a/plugins/catalog-backend/src/service/response/index.ts b/plugins/catalog-backend/src/service/response/index.ts new file mode 100644 index 0000000000..73c82c2aee --- /dev/null +++ b/plugins/catalog-backend/src/service/response/index.ts @@ -0,0 +1,22 @@ +/* + * Copyright 2024 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. + */ + +export { + processRawEntitiesResult, + processEntitiesResponseItems, + entitiesResponseToObjects, +} from './process'; +export { writeSingleEntityResponse, writeEntitiesResponse } from './write'; diff --git a/plugins/catalog-backend/src/service/response/process.ts b/plugins/catalog-backend/src/service/response/process.ts new file mode 100644 index 0000000000..26ab544a4d --- /dev/null +++ b/plugins/catalog-backend/src/service/response/process.ts @@ -0,0 +1,62 @@ +/* + * Copyright 2024 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 { Entity } from '@backstage/catalog-model'; +import { EntitiesResponseItems } from '../../catalog/types'; + +export function processRawEntitiesResult( + serializedEntities: (string | null)[], + transform?: (entity: Entity) => Entity, +): EntitiesResponseItems { + if (transform) { + return { + type: 'objects', + entities: serializedEntities.map(e => + e !== null ? transform(JSON.parse(e)) : e, + ), + }; + } + + return { + type: 'raw', + entities: serializedEntities, + }; +} + +export function processEntitiesResponseItems( + response: EntitiesResponseItems, + transform?: (entity: Entity) => Entity, +) { + if (!transform) { + return response; + } + if (response.type === 'raw') { + return processRawEntitiesResult(response.entities, transform); + } + return { + type: 'objects', + entities: response.entities.map(e => (e !== null ? transform(e) : e)), + }; +} + +export function entitiesResponseToObjects( + response: EntitiesResponseItems, +): (Entity | null)[] { + if (response.type === 'objects') { + return response.entities; + } + return response.entities.map(e => (e !== null ? JSON.parse(e) : e)); +} diff --git a/plugins/catalog-backend/src/service/response/write.ts b/plugins/catalog-backend/src/service/response/write.ts new file mode 100644 index 0000000000..1f9392e3a9 --- /dev/null +++ b/plugins/catalog-backend/src/service/response/write.ts @@ -0,0 +1,89 @@ +/* + * Copyright 2024 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 { Response } from 'express'; +import { EntitiesResponseItems } from '../../catalog/types'; +import { JsonValue } from '@backstage/types'; + +const JSON_CONTENT_TYPE = 'application/json; charset=utf-8'; + +export function writeSingleEntityResponse( + res: Response, + response: EntitiesResponseItems, +): boolean { + const entity = response.entities[0]; + if (!entity) { + return false; + } + + if (typeof entity === 'string') { + res.setHeader('Content-Type', JSON_CONTENT_TYPE); + res.status(200); + res.write(entity); + } else { + res.json(entity); + } + + return true; +} + +export function writeEntitiesResponse( + res: Response, + response: EntitiesResponseItems, + responseWrapper?: (entities: JsonValue) => JsonValue, +) { + if (response.type === 'objects') { + res.json( + responseWrapper + ? responseWrapper?.(response.entities) + : response.entities, + ); + return; + } + + res.setHeader('Content-Type', JSON_CONTENT_TYPE); + res.status(200); + + // responseWrapper allows the caller to render the entities within an object + let trailing = ''; + if (responseWrapper) { + const marker = `__MARKER_${Math.random().toString(36).slice(2, 10)}__`; + const wrapped = JSON.stringify(responseWrapper(marker)); + const parts = wrapped.split(marker); + if (parts.length !== 2) { + throw new Error( + `Entity items response was incorrectly wrapped into ${parts.length} different parts`, + ); + } + res.write(parts[0], 'utf8'); + trailing = parts[1]; + } + + let first = true; + for (const entity of response.entities) { + if (first) { + res.write('[', 'utf8'); + first = false; + } else { + res.write(',', 'utf8'); + } + res.write(entity, 'utf8'); + } + res.end(']'); + if (trailing) { + res.write(trailing, 'utf8'); + } +} diff --git a/plugins/catalog-backend/src/service/util.ts b/plugins/catalog-backend/src/service/util.ts index c05fb14558..2126cf25b3 100644 --- a/plugins/catalog-backend/src/service/util.ts +++ b/plugins/catalog-backend/src/service/util.ts @@ -20,6 +20,7 @@ import lodash from 'lodash'; import { z } from 'zod'; import { Cursor, + EntitiesResponseItems, QueryEntitiesCursorRequest, QueryEntitiesInitialRequest, QueryEntitiesRequest, @@ -148,35 +149,32 @@ export function decodeCursor(encodedCursor: string) { // sure that all adopters have re-stitched their entities so that the new // targetRef field is present on them, and that they have stopped consuming // the now-removed old field -// TODO(jhaals): Remove this in April 2022 -export function expandLegacyCompoundRelationRefsInResponse( - entities: Entity[], -): void { - for (const entity of entities) { - if (entity.relations) { - for (const relation of entity.relations as any) { - if (!relation.targetRef && relation.target) { - // This is the case where an old-form entity, not yet stitched with - // the updated code, was in the database - relation.targetRef = stringifyEntityRef(relation.target); - } else if (!relation.target && relation.targetRef) { - // This is the case where a new-form entity, stitched with the - // updated code, was in the database but we still want to produce - // the old data shape as well for compatibility reasons - relation.target = parseEntityRef(relation.targetRef); - } +// TODO(patriko): Remove this in catalog 2.0 +export function expandLegacyCompoundRelationsInEntity(entity: Entity): Entity { + if (entity.relations) { + for (const relation of entity.relations as any) { + if (!relation.targetRef && relation.target) { + // This is the case where an old-form entity, not yet stitched with + // the updated code, was in the database + relation.targetRef = stringifyEntityRef(relation.target); + } else if (!relation.target && relation.targetRef) { + // This is the case where a new-form entity, stitched with the + // updated code, was in the database but we still want to produce + // the old data shape as well for compatibility reasons + relation.target = parseEntityRef(relation.targetRef); } } } + return entity; } export interface EntityArrayJsonStream { - send(entities: Entity[]): boolean; + send(entities: EntitiesResponseItems): boolean; complete(): void; close(): void; } -// Helps stream Entity[] as a JSON response stream to avoid performance issues +// Helps stream EntitiesResponseItems[] as a JSON response stream to avoid performance issues export function createEntityArrayJsonStream( res: Response, ): EntityArrayJsonStream { @@ -186,19 +184,33 @@ export function createEntityArrayJsonStream( let completed = false; return { - send(entities) { + send(response) { if (firstSend) { res.setHeader('Content-Type', 'application/json; charset=utf-8'); res.status(200); res.flushHeaders(); } + if (response.type === 'raw') { + let result = true; + for (const item of response.entities) { + if (firstSend) { + result ||= res.write('['); + firstSend = false; + } else { + result ||= res.write(','); + } + result ||= res.write(item); + } + return result; + } + let data: string; if (prettyPrint) { - data = JSON.stringify(entities, null, 2); + data = JSON.stringify(response.entities, null, 2); data = firstSend ? data.slice(0, -2) : `,\n${data.slice(2, -2)}`; } else { - data = JSON.stringify(entities); + data = JSON.stringify(response.entities); data = firstSend ? data.slice(0, -1) : `,${data.slice(1, -1)}`; } diff --git a/plugins/catalog-backend/src/tests/integration.test.ts b/plugins/catalog-backend/src/tests/integration.test.ts index 737bb107ca..fdafbd5edf 100644 --- a/plugins/catalog-backend/src/tests/integration.test.ts +++ b/plugins/catalog-backend/src/tests/integration.test.ts @@ -55,6 +55,7 @@ import { DefaultStitcher } from '../stitching/DefaultStitcher'; import { mockServices } from '@backstage/backend-test-utils'; import { LoggerService } from '@backstage/backend-plugin-api'; import { DatabaseManager } from '@backstage/backend-common'; +import { entitiesResponseToObjects } from '../service/response'; const voidLogger = mockServices.logger.mock(); @@ -365,7 +366,12 @@ class TestHarness { async getOutputEntities(): Promise> { const { entities } = await this.#catalog.entities(); - return Object.fromEntries(entities.map(e => [stringifyEntityRef(e), e])); + return Object.fromEntries( + entitiesResponseToObjects(entities).map(e => [ + stringifyEntityRef(e!), + e!, + ]), + ); } async refresh(options: RefreshOptions) { From 6f09b9af9983a2a1a258ba11760e941e41b8754e Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Dec 2024 13:30:11 +0100 Subject: [PATCH 17/28] catalog-backend: respose item type object + single entity refactor and tests Signed-off-by: Patrik Oldsberg --- plugins/catalog-backend/src/catalog/types.ts | 2 +- .../src/service/AuthorizedEntitiesCatalog.ts | 6 +- .../src/service/createRouter.test.ts | 32 ++--- .../src/service/createRouter.ts | 16 +-- .../src/service/response/process.ts | 6 +- .../src/service/response/write.test.ts | 125 ++++++++++++++++++ .../src/service/response/write.ts | 29 ++-- 7 files changed, 170 insertions(+), 46 deletions(-) create mode 100644 plugins/catalog-backend/src/service/response/write.test.ts diff --git a/plugins/catalog-backend/src/catalog/types.ts b/plugins/catalog-backend/src/catalog/types.ts index d00c38f536..1fbf337986 100644 --- a/plugins/catalog-backend/src/catalog/types.ts +++ b/plugins/catalog-backend/src/catalog/types.ts @@ -58,7 +58,7 @@ export type EntitiesRequest = { */ export type EntitiesResponseItems = | { - type: 'objects'; + type: 'object'; entities: (Entity | null)[]; } | { diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts index 225c87cae9..073e153dcc 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts @@ -60,7 +60,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { if (authorizeDecision.result === AuthorizeResult.DENY) { return { - entities: { type: 'objects', entities: [] }, + entities: { type: 'object', entities: [] }, pageInfo: { hasNextPage: false }, }; } @@ -93,7 +93,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { if (authorizeDecision.result === AuthorizeResult.DENY) { return { items: { - type: 'objects', + type: 'object', entities: new Array(request.entityRefs.length).fill(null), }, }; @@ -126,7 +126,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { if (authorizeDecision.result === AuthorizeResult.DENY) { return { - items: { type: 'objects', entities: [] }, + items: { type: 'object', entities: [] }, pageInfo: {}, totalItems: 0, }; diff --git a/plugins/catalog-backend/src/service/createRouter.test.ts b/plugins/catalog-backend/src/service/createRouter.test.ts index 8efb6543a3..d4940e4a35 100644 --- a/plugins/catalog-backend/src/service/createRouter.test.ts +++ b/plugins/catalog-backend/src/service/createRouter.test.ts @@ -136,7 +136,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: { type: 'objects', entities: [entities[0]] }, + items: { type: 'object', entities: [entities[0]] }, pageInfo: {}, totalItems: 1, }); @@ -149,7 +149,7 @@ describe('createRouter readonly disabled', () => { it('parses single and multiple request parameters and passes them down', async () => { entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: { type: 'objects', entities: [] }, + items: { type: 'object', entities: [] }, pageInfo: {}, totalItems: 0, }); @@ -185,7 +185,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: { type: 'objects', entities: items }, + items: { type: 'object', entities: items }, totalItems: 100, pageInfo: { nextCursor: mockCursor() }, }); @@ -203,7 +203,7 @@ describe('createRouter readonly disabled', () => { it('parses initial request', async () => { entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: { type: 'objects', entities: [] }, + items: { type: 'object', entities: [] }, pageInfo: {}, totalItems: 0, }); @@ -239,7 +239,7 @@ describe('createRouter readonly disabled', () => { it('parses encoded params request', async () => { entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: { type: 'objects', entities: [] }, + items: { type: 'object', entities: [] }, pageInfo: {}, totalItems: 0, }); @@ -283,7 +283,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: { type: 'objects', entities: items }, + items: { type: 'object', entities: items }, totalItems: 100, pageInfo: { nextCursor: mockCursor() }, }); @@ -312,7 +312,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: { type: 'objects', entities: items }, + items: { type: 'object', entities: items }, totalItems: 100, pageInfo: { nextCursor: mockCursor({ fullTextFilter: { term: 'mySearch' } }), @@ -354,7 +354,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: { type: 'objects', entities: items }, + items: { type: 'object', entities: items }, totalItems: 100, pageInfo: { nextCursor: mockCursor() }, }); @@ -379,7 +379,7 @@ describe('createRouter readonly disabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: { type: 'objects', entities: items }, + items: { type: 'object', entities: items }, totalItems: 100, pageInfo: { nextCursor: mockCursor() }, }); @@ -402,7 +402,7 @@ describe('createRouter readonly disabled', () => { }, }; entitiesCatalog.entities.mockResolvedValue({ - entities: { type: 'objects', entities: [entity] }, + entities: { type: 'object', entities: [entity] }, pageInfo: { hasNextPage: false }, }); @@ -419,7 +419,7 @@ describe('createRouter readonly disabled', () => { it('responds with a 404 for missing entities', async () => { entitiesCatalog.entities.mockResolvedValue({ - entities: { type: 'objects', entities: [] }, + entities: { type: 'object', entities: [] }, pageInfo: { hasNextPage: false }, }); @@ -446,7 +446,7 @@ describe('createRouter readonly disabled', () => { }, }; entitiesCatalog.entitiesBatch.mockResolvedValue({ - items: { type: 'objects', entities: [entity] }, + items: { type: 'object', entities: [entity] }, }); const response = await request(app).get('/entities/by-name/k/ns/n'); @@ -462,7 +462,7 @@ describe('createRouter readonly disabled', () => { it('responds with a 404 for missing entities', async () => { entitiesCatalog.entitiesBatch.mockResolvedValue({ - items: { type: 'objects', entities: [null] }, + items: { type: 'object', entities: [null] }, }); const response = await request(app).get('/entities/by-name/b/d/c'); @@ -534,7 +534,7 @@ describe('createRouter readonly disabled', () => { }; const entityRef = stringifyEntityRef(entity); entitiesCatalog.entitiesBatch.mockResolvedValue({ - items: { type: 'objects', entities: [entity] }, + items: { type: 'object', entities: [entity] }, }); const response = await request(app) .post('/entities/by-refs?filter=kind=Component') @@ -910,7 +910,7 @@ describe('createRouter readonly enabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: { type: 'objects', entities: [entities[0]] }, + items: { type: 'object', entities: [entities[0]] }, pageInfo: {}, totalItems: 1, }); @@ -1130,7 +1130,7 @@ describe('NextRouter permissioning', () => { }, }; entitiesCatalog.entities.mockResolvedValueOnce({ - entities: { type: 'objects', entities: [spideySense] }, + entities: { type: 'object', entities: [spideySense] }, pageInfo: { hasNextPage: false }, }); diff --git a/plugins/catalog-backend/src/service/createRouter.ts b/plugins/catalog-backend/src/service/createRouter.ts index b670253089..7208c29a89 100644 --- a/plugins/catalog-backend/src/service/createRouter.ts +++ b/plugins/catalog-backend/src/service/createRouter.ts @@ -23,7 +23,7 @@ import { stringifyEntityRef, } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; -import { InputError, NotFoundError, serializeError } from '@backstage/errors'; +import { InputError, serializeError } from '@backstage/errors'; import express from 'express'; import yn from 'yn'; import { z } from 'zod'; @@ -272,9 +272,7 @@ export async function createRouter( filter: basicEntityFilter({ 'metadata.uid': uid }), credentials: await httpAuth.credentials(req), }); - if (!writeSingleEntityResponse(res, entities)) { - throw new NotFoundError(`No entity with uid ${uid}`); - } + writeSingleEntityResponse(res, entities, `No entity with uid ${uid}`); }) .delete('/entities/by-uid/:uid', async (req, res) => { const { uid } = req.params; @@ -289,11 +287,11 @@ export async function createRouter( entityRefs: [stringifyEntityRef({ kind, namespace, name })], credentials: await httpAuth.credentials(req), }); - if (!writeSingleEntityResponse(res, items)) { - throw new NotFoundError( - `No entity named '${name}' found, with kind '${kind}' in namespace '${namespace}'`, - ); - } + writeSingleEntityResponse( + res, + items, + `No entity named '${name}' found, with kind '${kind}' in namespace '${namespace}'`, + ); }) .get( '/entities/by-name/:kind/:namespace/:name/ancestry', diff --git a/plugins/catalog-backend/src/service/response/process.ts b/plugins/catalog-backend/src/service/response/process.ts index 26ab544a4d..7e7730d697 100644 --- a/plugins/catalog-backend/src/service/response/process.ts +++ b/plugins/catalog-backend/src/service/response/process.ts @@ -23,7 +23,7 @@ export function processRawEntitiesResult( ): EntitiesResponseItems { if (transform) { return { - type: 'objects', + type: 'object', entities: serializedEntities.map(e => e !== null ? transform(JSON.parse(e)) : e, ), @@ -47,7 +47,7 @@ export function processEntitiesResponseItems( return processRawEntitiesResult(response.entities, transform); } return { - type: 'objects', + type: 'object', entities: response.entities.map(e => (e !== null ? transform(e) : e)), }; } @@ -55,7 +55,7 @@ export function processEntitiesResponseItems( export function entitiesResponseToObjects( response: EntitiesResponseItems, ): (Entity | null)[] { - if (response.type === 'objects') { + if (response.type === 'object') { return response.entities; } return response.entities.map(e => (e !== null ? JSON.parse(e) : e)); diff --git a/plugins/catalog-backend/src/service/response/write.test.ts b/plugins/catalog-backend/src/service/response/write.test.ts new file mode 100644 index 0000000000..412aefe404 --- /dev/null +++ b/plugins/catalog-backend/src/service/response/write.test.ts @@ -0,0 +1,125 @@ +/* + * Copyright 2024 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 express from 'express'; +import { mockErrorHandler } from '@backstage/backend-test-utils'; +import request from 'supertest'; +import { writeSingleEntityResponse } from './write'; + +describe('writeSingleEntityResponse', () => { + const app = express(); + app.use(express.json()); + app.get('/echo', (req, res) => { + writeSingleEntityResponse(res, req.body, 'not found'); + }); + app.use(mockErrorHandler()); + + describe('in object form', () => { + it('should write a single entity', async () => { + const res = await request(app) + .get('/echo') + .send({ + type: 'object', + entities: [{ kind: 'Component' }, { kind: 'User' }], + }); + + expect(res.status).toBe(200); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toEqual({ kind: 'Component' }); + }); + + it('should write a missing entity', async () => { + const res = await request(app) + .get('/echo') + .send({ type: 'object', entities: [null] }); + + expect(res.status).toBe(404); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toMatchObject({ + error: { name: 'NotFoundError', message: 'not found' }, + }); + }); + + it('should write no entities', async () => { + const res = await request(app) + .get('/echo') + .send({ type: 'object', entities: [] }); + + expect(res.status).toBe(404); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toMatchObject({ + error: { name: 'NotFoundError', message: 'not found' }, + }); + }); + }); + + describe('in raw form', () => { + it('should write a single entity', async () => { + const res = await request(app) + .get('/echo') + .send({ + type: 'raw', + entities: ['{"kind":"Component"}', '{"kind":"User"}'], + }); + + expect(res.status).toBe(200); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toEqual({ kind: 'Component' }); + }); + + it('should write a missing entity', async () => { + const res = await request(app) + .get('/echo') + .send({ type: 'raw', entities: [null] }); + + expect(res.status).toBe(404); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toMatchObject({ + error: { name: 'NotFoundError', message: 'not found' }, + }); + }); + + it('should write no entities', async () => { + const res = await request(app) + .get('/echo') + .send({ type: 'raw', entities: [] }); + + expect(res.status).toBe(404); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toMatchObject({ + error: { name: 'NotFoundError', message: 'not found' }, + }); + }); + }); +}); diff --git a/plugins/catalog-backend/src/service/response/write.ts b/plugins/catalog-backend/src/service/response/write.ts index 1f9392e3a9..1dac8d3ce4 100644 --- a/plugins/catalog-backend/src/service/response/write.ts +++ b/plugins/catalog-backend/src/service/response/write.ts @@ -17,27 +17,29 @@ import { Response } from 'express'; import { EntitiesResponseItems } from '../../catalog/types'; import { JsonValue } from '@backstage/types'; +import { NotFoundError } from '@backstage/errors'; const JSON_CONTENT_TYPE = 'application/json; charset=utf-8'; export function writeSingleEntityResponse( res: Response, response: EntitiesResponseItems, -): boolean { - const entity = response.entities[0]; - if (!entity) { - return false; - } + notFoundMessage: string, +) { + if (response.type === 'object') { + if (!response.entities[0]) { + throw new NotFoundError(notFoundMessage); + } - if (typeof entity === 'string') { - res.setHeader('Content-Type', JSON_CONTENT_TYPE); - res.status(200); - res.write(entity); + res.json(response.entities[0]); } else { - res.json(entity); - } + if (!response.entities[0]) { + throw new NotFoundError(notFoundMessage); + } - return true; + res.setHeader('Content-Type', JSON_CONTENT_TYPE); + res.end(response.entities[0]); + } } export function writeEntitiesResponse( @@ -45,7 +47,7 @@ export function writeEntitiesResponse( response: EntitiesResponseItems, responseWrapper?: (entities: JsonValue) => JsonValue, ) { - if (response.type === 'objects') { + if (response.type === 'object') { res.json( responseWrapper ? responseWrapper?.(response.entities) @@ -55,7 +57,6 @@ export function writeEntitiesResponse( } res.setHeader('Content-Type', JSON_CONTENT_TYPE); - res.status(200); // responseWrapper allows the caller to render the entities within an object let trailing = ''; From daee494dfa26dbdca4cf8f6f3e656f8dda6e29bd Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Dec 2024 13:39:48 +0100 Subject: [PATCH 18/28] catalog-backend: async response writing + tests and fixes Signed-off-by: Patrik Oldsberg --- .../src/service/createRouter.ts | 8 +- .../src/service/response/write.test.ts | 158 +++++++++++++++++- .../src/service/response/write.ts | 28 ++-- 3 files changed, 177 insertions(+), 17 deletions(-) diff --git a/plugins/catalog-backend/src/service/createRouter.ts b/plugins/catalog-backend/src/service/createRouter.ts index 7208c29a89..f89ae745ce 100644 --- a/plugins/catalog-backend/src/service/createRouter.ts +++ b/plugins/catalog-backend/src/service/createRouter.ts @@ -171,7 +171,7 @@ export async function createRouter( res.setHeader('link', `<${url.pathname}${url.search}>; rel="next"`); } - writeEntitiesResponse(res, entities); + await writeEntitiesResponse(res, entities); return; } @@ -253,7 +253,7 @@ export async function createRouter( credentials: await httpAuth.credentials(req), }); - writeEntitiesResponse(res, items, entities => ({ + await writeEntitiesResponse(res, items, entities => ({ items: entities, totalItems, pageInfo: { @@ -312,7 +312,9 @@ export async function createRouter( fields: parseEntityTransformParams(req.query, request.fields), credentials: await httpAuth.credentials(req), }); - writeEntitiesResponse(res, items, entities => ({ items: entities })); + await writeEntitiesResponse(res, items, entities => ({ + items: entities, + })); }) .get('/entity-facets', async (req, res) => { const response = await entitiesCatalog.facets({ diff --git a/plugins/catalog-backend/src/service/response/write.test.ts b/plugins/catalog-backend/src/service/response/write.test.ts index 412aefe404..b1a688b48d 100644 --- a/plugins/catalog-backend/src/service/response/write.test.ts +++ b/plugins/catalog-backend/src/service/response/write.test.ts @@ -17,7 +17,7 @@ import express from 'express'; import { mockErrorHandler } from '@backstage/backend-test-utils'; import request from 'supertest'; -import { writeSingleEntityResponse } from './write'; +import { writeEntitiesResponse, writeSingleEntityResponse } from './write'; describe('writeSingleEntityResponse', () => { const app = express(); @@ -123,3 +123,159 @@ describe('writeSingleEntityResponse', () => { }); }); }); + +describe('writeEntitiesResponse', () => { + const app = express(); + app.use(express.json()); + app.get('/echo', (req, res) => { + writeEntitiesResponse(res, req.body); + }); + app.get('/wrapped', (req, res) => { + writeEntitiesResponse(res, req.body, entities => ({ + page: 1, + items: entities, + totalItems: 1337, + })); + }); + app.use(mockErrorHandler()); + + describe('in object form', () => { + it('should return empty list', async () => { + const res = await request(app).get('/echo').send({ + type: 'object', + entities: [], + }); + + expect(res.status).toBe(200); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toEqual([]); + }); + + it('should return mixed objects', async () => { + const res = await request(app) + .get('/echo') + .send({ + type: 'object', + entities: [{ kind: 'Component' }, null, { kind: 'User' }, null], + }); + + expect(res.status).toBe(200); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toEqual([ + { kind: 'Component' }, + null, + { kind: 'User' }, + null, + ]); + }); + + it('should wrap response of empty list', async () => { + const res = await request(app) + .get('/wrapped') + .send({ type: 'object', entities: [] }); + + expect(res.status).toBe(200); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toEqual({ page: 1, items: [], totalItems: 1337 }); + }); + + it('should wrap response of mixed list', async () => { + const res = await request(app) + .get('/wrapped') + .send({ + type: 'object', + entities: [{ kind: 'Component' }, null, { kind: 'User' }, null], + }); + + expect(res.status).toBe(200); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toEqual({ + page: 1, + items: [{ kind: 'Component' }, null, { kind: 'User' }, null], + totalItems: 1337, + }); + }); + }); + + describe('in raw form', () => { + it('should return empty list', async () => { + const res = await request(app).get('/echo').send({ + type: 'raw', + entities: [], + }); + + expect(res.status).toBe(200); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toEqual([]); + }); + + it('should return mixed objects', async () => { + const res = await request(app) + .get('/echo') + .send({ + type: 'raw', + entities: ['{"kind":"Component"}', null, '{"kind":"User"}', null], + }); + + expect(res.status).toBe(200); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toEqual([ + { kind: 'Component' }, + null, + { kind: 'User' }, + null, + ]); + }); + + it('should wrap response of empty list', async () => { + const res = await request(app) + .get('/wrapped') + .send({ type: 'raw', entities: [] }); + + expect(res.status).toBe(200); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toEqual({ page: 1, items: [], totalItems: 1337 }); + }); + + it('should wrap response of mixed list', async () => { + const res = await request(app) + .get('/wrapped') + .send({ + type: 'raw', + entities: ['{"kind":"Component"}', null, '{"kind":"User"}', null], + }); + + expect(res.status).toBe(200); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toEqual({ + page: 1, + items: [{ kind: 'Component' }, null, { kind: 'User' }, null], + totalItems: 1337, + }); + }); + }); +}); diff --git a/plugins/catalog-backend/src/service/response/write.ts b/plugins/catalog-backend/src/service/response/write.ts index 1dac8d3ce4..950dd99ba7 100644 --- a/plugins/catalog-backend/src/service/response/write.ts +++ b/plugins/catalog-backend/src/service/response/write.ts @@ -42,7 +42,7 @@ export function writeSingleEntityResponse( } } -export function writeEntitiesResponse( +export async function writeEntitiesResponse( res: Response, response: EntitiesResponseItems, responseWrapper?: (entities: JsonValue) => JsonValue, @@ -63,7 +63,7 @@ export function writeEntitiesResponse( if (responseWrapper) { const marker = `__MARKER_${Math.random().toString(36).slice(2, 10)}__`; const wrapped = JSON.stringify(responseWrapper(marker)); - const parts = wrapped.split(marker); + const parts = wrapped.split(`"${marker}"`); if (parts.length !== 2) { throw new Error( `Entity items response was incorrectly wrapped into ${parts.length} different parts`, @@ -75,16 +75,18 @@ export function writeEntitiesResponse( let first = true; for (const entity of response.entities) { - if (first) { - res.write('[', 'utf8'); - first = false; - } else { - res.write(',', 'utf8'); - } - res.write(entity, 'utf8'); - } - res.end(']'); - if (trailing) { - res.write(trailing, 'utf8'); + const prefix = first ? '[' : ','; + first = false; + + await new Promise((resolve, reject) => { + res.write(prefix + entity, 'utf8', err => { + if (err) { + reject(err); + } else { + resolve(err); + } + }); + }); } + res.end(`${first ? '[' : ''}]${trailing}`); } From b018b8e7e6d9d54ed93f39ca900998bc9c361431 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Dec 2024 13:54:33 +0100 Subject: [PATCH 19/28] catalog-backend: add response processing tests Signed-off-by: Patrik Oldsberg --- .../src/service/response/process.test.ts | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) create mode 100644 plugins/catalog-backend/src/service/response/process.test.ts diff --git a/plugins/catalog-backend/src/service/response/process.test.ts b/plugins/catalog-backend/src/service/response/process.test.ts new file mode 100644 index 0000000000..60b96608f7 --- /dev/null +++ b/plugins/catalog-backend/src/service/response/process.test.ts @@ -0,0 +1,115 @@ +/* + * Copyright 2024 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 { Entity } from '@backstage/catalog-model'; +import { + entitiesResponseToObjects, + processEntitiesResponseItems, + processRawEntitiesResult, +} from './process'; + +const mockTransform = (entity: Entity): Entity => ({ + ...entity, + kind: `transformed-${entity.kind}`, +}); + +describe('processRawEntitiesResult', () => { + it('should prefer keeping results in raw form', () => { + expect(processRawEntitiesResult(['{"kind":"test"}', null])).toEqual({ + type: 'raw', + entities: ['{"kind":"test"}', null], + }); + + expect( + processRawEntitiesResult(['{"kind":"test"}', null], mockTransform), + ).toEqual({ + type: 'object', + entities: [{ kind: 'transformed-test' }, null], + }); + }); +}); + +describe('processEntitiesResponseItems', () => { + it('should transform entities in object form', () => { + expect( + processEntitiesResponseItems({ + type: 'object', + entities: [{ kind: 'test' } as Entity, null], + }), + ).toEqual({ + type: 'object', + entities: [{ kind: 'test' }, null], + }); + + expect( + processEntitiesResponseItems( + { + type: 'object', + entities: [{ kind: 'test' } as Entity, null], + }, + mockTransform, + ), + ).toEqual({ + type: 'object', + entities: [{ kind: 'transformed-test' }, null], + }); + }); + + it('should transform entities in raw form', () => { + expect( + processEntitiesResponseItems({ + type: 'raw', + entities: ['{"kind":"test"}', null], + }), + ).toEqual({ + type: 'raw', + entities: ['{"kind":"test"}', null], + }); + + expect( + processEntitiesResponseItems( + { + type: 'raw', + entities: ['{"kind":"test"}', null], + }, + mockTransform, + ), + ).toEqual({ + type: 'object', + entities: [{ kind: 'transformed-test' }, null], + }); + }); +}); + +describe('entitiesResponseToObjects', () => { + it('should convert entities in object form', () => { + expect( + entitiesResponseToObjects({ + type: 'object', + entities: [null, { kind: 'test' } as Entity], + }), + ).toEqual([null, { kind: 'test' }]); + }); + + it('should convert entities in raw form', () => { + expect( + entitiesResponseToObjects({ + type: 'raw', + entities: [null, '{"kind":"test"}'], + }), + ).toEqual([null, { kind: 'test' }]); + }); +}); From d706bb5b34b53b2d2379459e19a470368d99a7e9 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Dec 2024 14:23:39 +0100 Subject: [PATCH 20/28] catalog-backend: update entities response writing Signed-off-by: Patrik Oldsberg --- .../src/service/response/write.ts | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/plugins/catalog-backend/src/service/response/write.ts b/plugins/catalog-backend/src/service/response/write.ts index 950dd99ba7..0962ee15d1 100644 --- a/plugins/catalog-backend/src/service/response/write.ts +++ b/plugins/catalog-backend/src/service/response/write.ts @@ -78,15 +78,18 @@ export async function writeEntitiesResponse( const prefix = first ? '[' : ','; first = false; - await new Promise((resolve, reject) => { - res.write(prefix + entity, 'utf8', err => { - if (err) { - reject(err); - } else { - resolve(err); - } + const needsDrain = !res.write(prefix + entity, 'utf8'); + if (needsDrain) { + await new Promise(resolve => { + const cont = () => { + res.off('drain', cont); + res.off('close', cont); + resolve(); + }; + res.on('drain', cont); + res.on('close', cont); }); - }); + } } res.end(`${first ? '[' : ''}]${trailing}`); } From 31e2ca8d1836c21eb4922d171dedcc6c2424ef3a Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Dec 2024 14:34:16 +0100 Subject: [PATCH 21/28] catalog-backend: integration tests for raw response + fix raw json stream Signed-off-by: Patrik Oldsberg --- .../src/service/createRouter.test.ts | 5 +- plugins/catalog-backend/src/service/util.ts | 18 ++--- .../src/tests/integration.test.ts | 81 +++++++++++++++---- 3 files changed, 76 insertions(+), 28 deletions(-) diff --git a/plugins/catalog-backend/src/service/createRouter.test.ts b/plugins/catalog-backend/src/service/createRouter.test.ts index d4940e4a35..312900bcf1 100644 --- a/plugins/catalog-backend/src/service/createRouter.test.ts +++ b/plugins/catalog-backend/src/service/createRouter.test.ts @@ -860,7 +860,7 @@ describe('createRouter readonly disabled', () => { }); }); -describe('createRouter readonly enabled', () => { +describe('createRouter readonly and raw json enabled', () => { let entitiesCatalog: jest.Mocked; let app: express.Express; let locationService: jest.Mocked; @@ -883,6 +883,7 @@ describe('createRouter readonly enabled', () => { getLocationByEntity: jest.fn(), }; const router = await createRouter({ + enableRawJson: true, entitiesCatalog, locationService, logger: mockServices.logger.mock(), @@ -910,7 +911,7 @@ describe('createRouter readonly enabled', () => { ]; entitiesCatalog.queryEntities.mockResolvedValueOnce({ - items: { type: 'object', entities: [entities[0]] }, + items: { type: 'raw', entities: [JSON.stringify(entities[0])] }, pageInfo: {}, totalItems: 1, }); diff --git a/plugins/catalog-backend/src/service/util.ts b/plugins/catalog-backend/src/service/util.ts index 2126cf25b3..7ff7c8fcb3 100644 --- a/plugins/catalog-backend/src/service/util.ts +++ b/plugins/catalog-backend/src/service/util.ts @@ -192,17 +192,13 @@ export function createEntityArrayJsonStream( } if (response.type === 'raw') { - let result = true; + let needsDrain = false; for (const item of response.entities) { - if (firstSend) { - result ||= res.write('['); - firstSend = false; - } else { - result ||= res.write(','); - } - result ||= res.write(item); + const prefix = firstSend ? '[' : ','; + firstSend = false; + needsDrain ||= !res.write(prefix + item, 'utf8'); } - return result; + return !needsDrain; } let data: string; @@ -215,13 +211,13 @@ export function createEntityArrayJsonStream( } firstSend = false; - return res.write(data); + return res.write(data, 'utf8'); }, complete() { if (firstSend) { res.json([]); } else { - res.end(prettyPrint ? '\n]' : ']'); + res.end(prettyPrint ? '\n]' : ']', 'utf8'); } completed = true; }, diff --git a/plugins/catalog-backend/src/tests/integration.test.ts b/plugins/catalog-backend/src/tests/integration.test.ts index fdafbd5edf..32116ace0d 100644 --- a/plugins/catalog-backend/src/tests/integration.test.ts +++ b/plugins/catalog-backend/src/tests/integration.test.ts @@ -30,7 +30,6 @@ import { processingResult, } from '@backstage/plugin-catalog-node'; import { PermissionEvaluator } from '@backstage/plugin-permission-common'; -import { JsonObject } from '@backstage/types'; import { createHash } from 'crypto'; import { Knex } from 'knex'; import { EntitiesCatalog } from '../catalog/types'; @@ -199,7 +198,7 @@ class TestHarness { readonly #proxyProgressTracker: ProxyProgressTracker; static async create(options?: { - config?: JsonObject; + enableRawJson?: boolean; logger?: LoggerService; db?: Knex; permissions?: PermissionEvaluator; @@ -209,21 +208,19 @@ class TestHarness { emit: CatalogProcessorEmit, ): Promise; }) { - const config = new ConfigReader( - options?.config ?? { - backend: { - database: { - client: 'better-sqlite3', - connection: ':memory:', - }, - }, - catalog: { - stitchingStrategy: { - mode: 'immediate', - }, + const config = new ConfigReader({ + backend: { + database: { + client: 'better-sqlite3', + connection: ':memory:', }, }, - ); + catalog: { + stitchingStrategy: { + mode: 'immediate', + }, + }, + }); const logger = options?.logger ?? mockServices.logger.mock(); const db = options?.db ?? @@ -280,6 +277,7 @@ class TestHarness { database: db, logger, stitcher, + enableRawJson: options?.enableRawJson, }); const proxyProgressTracker = new ProxyProgressTracker( new NoopProgressTracker(), @@ -787,4 +785,57 @@ describe('Catalog Backend Integration', () => { ], }); }); + + it('should return valid responses in raw JSON mode', async () => { + const harness = await TestHarness.create({ + enableRawJson: true, + }); + + const entityA = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'a', + annotations: { + 'backstage.io/managed-by-location': 'url:.', + 'backstage.io/managed-by-origin-location': 'url:.', + }, + }, + }; + const entityB = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'b', + annotations: { + 'backstage.io/managed-by-location': 'url:.', + 'backstage.io/managed-by-origin-location': 'url:.', + }, + }, + }; + + await harness.setInputEntities([entityA, entityB]); + await expect(harness.process()).resolves.toEqual({}); + + await expect(harness.getOutputEntities()).resolves.toEqual({ + 'component:default/a': { + ...entityA, + metadata: { + ...entityA.metadata, + etag: expect.any(String), + uid: expect.any(String), + }, + relations: [], + }, + 'component:default/b': { + ...entityB, + metadata: { + ...entityB.metadata, + etag: expect.any(String), + uid: expect.any(String), + }, + relations: [], + }, + }); + }); }); From e97b87136717ed3063a0b7bebe5506c3d8d6f633 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Dec 2024 14:35:37 +0100 Subject: [PATCH 22/28] catalog-backend: moved createEntityArrayJsonStream to response utils Signed-off-by: Patrik Oldsberg --- .../src/service/createRouter.ts | 2 +- .../response/createEntityArrayJsonStream.ts | 79 +++++++++++++++++++ .../src/service/response/index.ts | 1 + plugins/catalog-backend/src/service/util.ts | 64 +-------------- 4 files changed, 82 insertions(+), 64 deletions(-) create mode 100644 plugins/catalog-backend/src/service/response/createEntityArrayJsonStream.ts diff --git a/plugins/catalog-backend/src/service/createRouter.ts b/plugins/catalog-backend/src/service/createRouter.ts index f89ae745ce..6ea9ea1c13 100644 --- a/plugins/catalog-backend/src/service/createRouter.ts +++ b/plugins/catalog-backend/src/service/createRouter.ts @@ -41,7 +41,6 @@ import { parseEntityFacetParams } from './request/parseEntityFacetParams'; import { parseEntityOrderParams } from './request/parseEntityOrderParams'; import { LocationService, RefreshService } from './types'; import { - createEntityArrayJsonStream, disallowReadonlyMode, encodeCursor, expandLegacyCompoundRelationsInEntity, @@ -61,6 +60,7 @@ import { LocationAnalyzer } from '@backstage/plugin-catalog-node'; import { AuthorizedValidationService } from './AuthorizedValidationService'; import { DeferredPromise, createDeferred } from '@backstage/types'; import { + createEntityArrayJsonStream, processEntitiesResponseItems, writeEntitiesResponse, writeSingleEntityResponse, diff --git a/plugins/catalog-backend/src/service/response/createEntityArrayJsonStream.ts b/plugins/catalog-backend/src/service/response/createEntityArrayJsonStream.ts new file mode 100644 index 0000000000..4762afd2d9 --- /dev/null +++ b/plugins/catalog-backend/src/service/response/createEntityArrayJsonStream.ts @@ -0,0 +1,79 @@ +/* + * Copyright 2024 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 { EntitiesResponseItems } from '../../catalog/types'; +import { Response } from 'express'; + +export interface EntityArrayJsonStream { + send(entities: EntitiesResponseItems): boolean; + complete(): void; + close(): void; +} + +// Helps stream EntitiesResponseItems[] as a JSON response stream to avoid performance issues +export function createEntityArrayJsonStream( + res: Response, +): EntityArrayJsonStream { + // Imitate the httpRouter behavior of pretty-printing in development + const prettyPrint = process.env.NODE_ENV === 'development'; + let firstSend = true; + let completed = false; + + return { + send(response) { + if (firstSend) { + res.setHeader('Content-Type', 'application/json; charset=utf-8'); + res.status(200); + res.flushHeaders(); + } + + if (response.type === 'raw') { + let needsDrain = false; + for (const item of response.entities) { + const prefix = firstSend ? '[' : ','; + firstSend = false; + needsDrain ||= !res.write(prefix + item, 'utf8'); + } + return !needsDrain; + } + + let data: string; + if (prettyPrint) { + data = JSON.stringify(response.entities, null, 2); + data = firstSend ? data.slice(0, -2) : `,\n${data.slice(2, -2)}`; + } else { + data = JSON.stringify(response.entities); + data = firstSend ? data.slice(0, -1) : `,${data.slice(1, -1)}`; + } + + firstSend = false; + return res.write(data, 'utf8'); + }, + complete() { + if (firstSend) { + res.json([]); + } else { + res.end(prettyPrint ? '\n]' : ']', 'utf8'); + } + completed = true; + }, + close() { + if (!completed) { + res.end(); + } + }, + }; +} diff --git a/plugins/catalog-backend/src/service/response/index.ts b/plugins/catalog-backend/src/service/response/index.ts index 73c82c2aee..2e01071dbd 100644 --- a/plugins/catalog-backend/src/service/response/index.ts +++ b/plugins/catalog-backend/src/service/response/index.ts @@ -20,3 +20,4 @@ export { entitiesResponseToObjects, } from './process'; export { writeSingleEntityResponse, writeEntitiesResponse } from './write'; +export { createEntityArrayJsonStream } from './createEntityArrayJsonStream'; diff --git a/plugins/catalog-backend/src/service/util.ts b/plugins/catalog-backend/src/service/util.ts index 7ff7c8fcb3..9847cb7225 100644 --- a/plugins/catalog-backend/src/service/util.ts +++ b/plugins/catalog-backend/src/service/util.ts @@ -15,12 +15,11 @@ */ import { InputError, NotAllowedError } from '@backstage/errors'; -import { Request, Response } from 'express'; +import { Request } from 'express'; import lodash from 'lodash'; import { z } from 'zod'; import { Cursor, - EntitiesResponseItems, QueryEntitiesCursorRequest, QueryEntitiesInitialRequest, QueryEntitiesRequest, @@ -167,64 +166,3 @@ export function expandLegacyCompoundRelationsInEntity(entity: Entity): Entity { } return entity; } - -export interface EntityArrayJsonStream { - send(entities: EntitiesResponseItems): boolean; - complete(): void; - close(): void; -} - -// Helps stream EntitiesResponseItems[] as a JSON response stream to avoid performance issues -export function createEntityArrayJsonStream( - res: Response, -): EntityArrayJsonStream { - // Imitate the httpRouter behavior of pretty-printing in development - const prettyPrint = process.env.NODE_ENV === 'development'; - let firstSend = true; - let completed = false; - - return { - send(response) { - if (firstSend) { - res.setHeader('Content-Type', 'application/json; charset=utf-8'); - res.status(200); - res.flushHeaders(); - } - - if (response.type === 'raw') { - let needsDrain = false; - for (const item of response.entities) { - const prefix = firstSend ? '[' : ','; - firstSend = false; - needsDrain ||= !res.write(prefix + item, 'utf8'); - } - return !needsDrain; - } - - let data: string; - if (prettyPrint) { - data = JSON.stringify(response.entities, null, 2); - data = firstSend ? data.slice(0, -2) : `,\n${data.slice(2, -2)}`; - } else { - data = JSON.stringify(response.entities); - data = firstSend ? data.slice(0, -1) : `,${data.slice(1, -1)}`; - } - - firstSend = false; - return res.write(data, 'utf8'); - }, - complete() { - if (firstSend) { - res.json([]); - } else { - res.end(prettyPrint ? '\n]' : ']', 'utf8'); - } - completed = true; - }, - close() { - if (!completed) { - res.end(); - } - }, - }; -} From 9621b02ec619349b3768700e35158278a401538a Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Dec 2024 14:40:32 +0100 Subject: [PATCH 23/28] catalog-backend: update DefaultEntitiesCatalog tests with new response structure Signed-off-by: Patrik Oldsberg --- .../service/DefaultEntitiesCatalog.test.ts | 141 +++++++++++++----- 1 file changed, 100 insertions(+), 41 deletions(-) diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts index 379c861ba1..070b93ab2c 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.test.ts @@ -896,7 +896,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response1 = await catalog.queryEntities(request1); - expect(response1.items).toEqual([entityFrom('A'), entityFrom('B')]); + expect(entitiesResponseToObjects(response1.items)).toEqual([ + entityFrom('A'), + entityFrom('B'), + ]); expect(response1.pageInfo.nextCursor).toBeDefined(); expect(response1.pageInfo.prevCursor).toBeUndefined(); expect(response1.totalItems).toBe(names.length); @@ -908,7 +911,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response2 = await catalog.queryEntities(request2); - expect(response2.items).toEqual([entityFrom('C'), entityFrom('D')]); + expect(entitiesResponseToObjects(response2.items)).toEqual([ + entityFrom('C'), + entityFrom('D'), + ]); expect(response2.pageInfo.nextCursor).toBeDefined(); expect(response2.pageInfo.prevCursor).toBeDefined(); expect(response2.totalItems).toBe(names.length); @@ -920,7 +926,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response3 = await catalog.queryEntities(request3); - expect(response3.items).toEqual([entityFrom('E'), entityFrom('F')]); + expect(entitiesResponseToObjects(response3.items)).toEqual([ + entityFrom('E'), + entityFrom('F'), + ]); expect(response3.pageInfo.nextCursor).toBeDefined(); expect(response3.pageInfo.prevCursor).toBeDefined(); expect(response3.totalItems).toBe(names.length); @@ -932,7 +941,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response4 = await catalog.queryEntities(request4); - expect(response4.items).toEqual([entityFrom('C'), entityFrom('D')]); + expect(entitiesResponseToObjects(response4.items)).toEqual([ + entityFrom('C'), + entityFrom('D'), + ]); expect(response4.pageInfo.nextCursor).toBeDefined(); expect(response4.pageInfo.prevCursor).toBeDefined(); expect(response4.totalItems).toBe(names.length); @@ -944,7 +956,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response5 = await catalog.queryEntities(request5); - expect(response5.items).toEqual([entityFrom('A'), entityFrom('B')]); + expect(entitiesResponseToObjects(response5.items)).toEqual([ + entityFrom('A'), + entityFrom('B'), + ]); expect(response5.pageInfo.nextCursor).toBeDefined(); expect(response5.pageInfo.prevCursor).toBeUndefined(); expect(response5.totalItems).toBe(names.length); @@ -956,7 +971,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response6 = await catalog.queryEntities(request6); - expect(response6.items).toEqual([entityFrom('C'), entityFrom('D')]); + expect(entitiesResponseToObjects(response6.items)).toEqual([ + entityFrom('C'), + entityFrom('D'), + ]); expect(response6.pageInfo.nextCursor).toBeDefined(); expect(response6.pageInfo.prevCursor).toBeDefined(); expect(response6.totalItems).toBe(names.length); @@ -968,7 +986,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response7 = await catalog.queryEntities(request7); - expect(response7.items).toEqual([entityFrom('E'), entityFrom('F')]); + expect(entitiesResponseToObjects(response7.items)).toEqual([ + entityFrom('E'), + entityFrom('F'), + ]); expect(response7.pageInfo.nextCursor).toBeDefined(); expect(response7.pageInfo.prevCursor).toBeDefined(); expect(response7.totalItems).toBe(names.length); @@ -980,7 +1001,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response7bis = await catalog.queryEntities(request7bis); - expect(response7bis.items).toEqual([ + expect(entitiesResponseToObjects(response7bis.items)).toEqual([ entityFrom('E'), entityFrom('F'), entityFrom('G'), @@ -996,7 +1017,9 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response8 = await catalog.queryEntities(request8); - expect(response8.items).toEqual([entityFrom('G')]); + expect(entitiesResponseToObjects(response8.items)).toEqual([ + entityFrom('G'), + ]); expect(response8.pageInfo.nextCursor).toBeUndefined(); expect(response8.pageInfo.prevCursor).toBeDefined(); expect(response8.totalItems).toBe(names.length); @@ -1050,7 +1073,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response1 = await catalog.queryEntities(request1); - expect(response1.items).toEqual([entityFrom('G'), entityFrom('F')]); + expect(entitiesResponseToObjects(response1.items)).toEqual([ + entityFrom('G'), + entityFrom('F'), + ]); expect(response1.pageInfo.nextCursor).toBeDefined(); expect(response1.pageInfo.prevCursor).toBeUndefined(); expect(response1.totalItems).toBe(names.length); @@ -1062,7 +1088,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response2 = await catalog.queryEntities(request2); - expect(response2.items).toEqual([entityFrom('E'), entityFrom('D')]); + expect(entitiesResponseToObjects(response2.items)).toEqual([ + entityFrom('E'), + entityFrom('D'), + ]); expect(response2.pageInfo.nextCursor).toBeDefined(); expect(response2.pageInfo.prevCursor).toBeDefined(); expect(response2.totalItems).toBe(names.length); @@ -1074,7 +1103,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response3 = await catalog.queryEntities(request3); - expect(response3.items).toEqual([entityFrom('C'), entityFrom('B')]); + expect(entitiesResponseToObjects(response3.items)).toEqual([ + entityFrom('C'), + entityFrom('B'), + ]); expect(response3.pageInfo.nextCursor).toBeDefined(); expect(response3.pageInfo.prevCursor).toBeDefined(); expect(response3.totalItems).toBe(names.length); @@ -1087,7 +1119,10 @@ describe('DefaultEntitiesCatalog', () => { }; const response4 = await catalog.queryEntities(request4); - expect(response4.items).toEqual([entityFrom('E'), entityFrom('D')]); + expect(entitiesResponseToObjects(response4.items)).toEqual([ + entityFrom('E'), + entityFrom('D'), + ]); expect(response4.pageInfo.nextCursor).toBeDefined(); expect(response4.pageInfo.prevCursor).toBeDefined(); expect(response4.totalItems).toBe(names.length); @@ -1099,7 +1134,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response5 = await catalog.queryEntities(request5); - expect(response5.items).toEqual([entityFrom('G'), entityFrom('F')]); + expect(entitiesResponseToObjects(response5.items)).toEqual([ + entityFrom('G'), + entityFrom('F'), + ]); expect(response5.pageInfo.nextCursor).toBeDefined(); expect(response5.pageInfo.prevCursor).toBeUndefined(); expect(response5.totalItems).toBe(names.length); @@ -1111,7 +1149,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response6 = await catalog.queryEntities(request6); - expect(response6.items).toEqual([entityFrom('E'), entityFrom('D')]); + expect(entitiesResponseToObjects(response6.items)).toEqual([ + entityFrom('E'), + entityFrom('D'), + ]); expect(response6.pageInfo.nextCursor).toBeDefined(); expect(response6.pageInfo.prevCursor).toBeDefined(); expect(response6.totalItems).toBe(names.length); @@ -1123,7 +1164,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response7 = await catalog.queryEntities(request7); - expect(response7.items).toEqual([entityFrom('C'), entityFrom('B')]); + expect(entitiesResponseToObjects(response7.items)).toEqual([ + entityFrom('C'), + entityFrom('B'), + ]); expect(response7.pageInfo.nextCursor).toBeDefined(); expect(response7.pageInfo.prevCursor).toBeDefined(); expect(response7.totalItems).toBe(names.length); @@ -1135,7 +1179,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response7bis = await catalog.queryEntities(request7bis); - expect(response7bis.items).toEqual([ + expect(entitiesResponseToObjects(response7bis.items)).toEqual([ entityFrom('C'), entityFrom('B'), entityFrom('A'), @@ -1151,7 +1195,9 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response8 = await catalog.queryEntities(request8); - expect(response8.items).toEqual([entityFrom('A')]); + expect(entitiesResponseToObjects(response8.items)).toEqual([ + entityFrom('A'), + ]); expect(response8.pageInfo.nextCursor).toBeUndefined(); expect(response8.pageInfo.prevCursor).toBeDefined(); expect(response8.totalItems).toBe(names.length); @@ -1203,7 +1249,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response = await catalog.queryEntities(request); - expect(response.items).toEqual([ + expect(entitiesResponseToObjects(response.items)).toEqual([ entityFrom('atcatss'), entityFrom('cat'), entityFrom('dogcat'), @@ -1261,7 +1307,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response = await catalog.queryEntities(request); - expect(response.items).toEqual(entities); + expect(entitiesResponseToObjects(response.items)).toEqual(entities); expect(response.pageInfo.nextCursor).toBeUndefined(); expect(response.pageInfo.prevCursor).toBeUndefined(); expect(response.totalItems).toBe(1); @@ -1316,7 +1362,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response = await catalog.queryEntities(request); - expect(response.items).toEqual([ + expect(entitiesResponseToObjects(response.items)).toEqual([ entityFrom('1', { uid: 'id1', title: 'cat' }), entityFrom('2', { uid: 'id2', title: 'atcatss' }), entityFrom('4', { uid: 'id4', title: 'dogcat' }), @@ -1329,7 +1375,7 @@ describe('DefaultEntitiesCatalog', () => { ...request, limit: 2, }); - expect(paginatedResponse.items).toEqual([ + expect(entitiesResponseToObjects(paginatedResponse.items)).toEqual([ entityFrom('1', { uid: 'id1', title: 'cat' }), entityFrom('2', { uid: 'id2', title: 'atcatss' }), ]); @@ -1341,7 +1387,7 @@ describe('DefaultEntitiesCatalog', () => { cursor: paginatedResponse.pageInfo.nextCursor!, credentials: mockCredentials.none(), }); - expect(paginatedResponseNext.items).toEqual([ + expect(entitiesResponseToObjects(paginatedResponseNext.items)).toEqual([ entityFrom('4', { uid: 'id4', title: 'dogcat' }), ]); expect(paginatedResponseNext.pageInfo.nextCursor).toBeUndefined(); @@ -1419,7 +1465,7 @@ describe('DefaultEntitiesCatalog', () => { }; const response = await catalog.queryEntities(request); - expect(response.items).toEqual([ + expect(entitiesResponseToObjects(response.items)).toEqual([ entityFrom('KingOfTheJungle', { uid: 'id0', title: 'lion' }), entityFrom('NotKingOfTheJungle', { uid: 'id1', title: 'cat' }), entityFrom('NotACatKing', { uid: 'id2', title: 'atcatss' }), @@ -1433,7 +1479,7 @@ describe('DefaultEntitiesCatalog', () => { ...request, limit: 2, }); - expect(paginatedResponse.items).toEqual([ + expect(entitiesResponseToObjects(paginatedResponse.items)).toEqual([ entityFrom('KingOfTheJungle', { uid: 'id0', title: 'lion' }), entityFrom('NotKingOfTheJungle', { uid: 'id1', title: 'cat' }), ]); @@ -1445,7 +1491,7 @@ describe('DefaultEntitiesCatalog', () => { cursor: paginatedResponse.pageInfo.nextCursor!, credentials: mockCredentials.none(), }); - expect(paginatedResponseNext.items).toEqual([ + expect(entitiesResponseToObjects(paginatedResponseNext.items)).toEqual([ entityFrom('NotACatKing', { uid: 'id2', title: 'atcatss' }), entityFrom('123', { uid: 'id3', title: 'king' }), ]); @@ -1489,7 +1535,11 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response = await catalog.queryEntities(request); - expect(response).toEqual({ totalItems: 20, items: [], pageInfo: {} }); + expect(response).toEqual({ + totalItems: 20, + items: { type: 'raw', entities: [] }, + pageInfo: {}, + }); }, ); @@ -1524,7 +1574,10 @@ describe('DefaultEntitiesCatalog', () => { let response = await catalog.queryEntities(request); expect(response).toEqual({ totalItems: 0, - items: expect.objectContaining({ length: 10 }), + items: { + type: 'raw', + entities: expect.objectContaining({ length: 10 }), + }, pageInfo: { nextCursor: expect.anything() }, }); response = await catalog.queryEntities({ @@ -1533,7 +1586,10 @@ describe('DefaultEntitiesCatalog', () => { }); expect(response).toEqual({ totalItems: 0, - items: expect.objectContaining({ length: 5 }), + items: { + type: 'raw', + entities: expect.objectContaining({ length: 5 }), + }, pageInfo: { prevCursor: expect.anything() }, }); }, @@ -1568,7 +1624,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response1 = await catalog.queryEntities(request1); - expect(response1.items).toMatchObject([ + expect(entitiesResponseToObjects(response1.items)).toMatchObject([ entityFrom('AA'), entityFrom('AA'), ]); @@ -1583,7 +1639,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response2 = await catalog.queryEntities(request2); - expect(response2.items).toMatchObject([ + expect(entitiesResponseToObjects(response2.items)).toMatchObject([ entityFrom('AA'), entityFrom('AA'), ]); @@ -1598,7 +1654,10 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response3 = await catalog.queryEntities(request3); - expect(response3.items).toEqual([entityFrom('CC'), entityFrom('DD')]); + expect(entitiesResponseToObjects(response3.items)).toEqual([ + entityFrom('CC'), + entityFrom('DD'), + ]); expect(response3.pageInfo.nextCursor).toBeUndefined(); expect(response3.pageInfo.prevCursor).toBeDefined(); expect(response3.totalItems).toBe(6); @@ -1610,7 +1669,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response4 = await catalog.queryEntities(request4); - expect(response4.items).toMatchObject([ + expect(entitiesResponseToObjects(response4.items)).toMatchObject([ entityFrom('AA'), entityFrom('AA'), ]); @@ -1625,7 +1684,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response5 = await catalog.queryEntities(request5); - expect(response5.items).toMatchObject([ + expect(entitiesResponseToObjects(response5.items)).toMatchObject([ entityFrom('AA'), entityFrom('AA'), ]); @@ -1693,7 +1752,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response1 = await catalog.queryEntities(request1); - expect(response1.items).toMatchObject([ + expect(entitiesResponseToObjects(response1.items)).toMatchObject([ entityFrom('AA', { uid: '1', kind: 'included' }), entityFrom('AA', { uid: '2', kind: 'included' }), ]); @@ -1708,7 +1767,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response2 = await catalog.queryEntities(request2); - expect(response2.items).toMatchObject([ + expect(entitiesResponseToObjects(response2.items)).toMatchObject([ entityFrom('AA', { uid: '4', kind: 'included' }), entityFrom('AA', { uid: '5', kind: 'included' }), ]); @@ -1752,7 +1811,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response1 = await catalog.queryEntities(request1); - expect(response1.items).toMatchObject([ + expect(entitiesResponseToObjects(response1.items)).toMatchObject([ entityFrom('AA'), entityFrom('CC'), ]); @@ -1767,7 +1826,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response2 = await catalog.queryEntities(request2); - expect(response2.items).toMatchObject([ + expect(entitiesResponseToObjects(response2.items)).toMatchObject([ entityFrom('DD'), entityFrom('AA', { namespace: 'namespace2' }), ]); @@ -1782,7 +1841,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response3 = await catalog.queryEntities(request3); - expect(response3.items).toMatchObject([ + expect(entitiesResponseToObjects(response3.items)).toMatchObject([ entityFrom('AA', { namespace: 'namespace3' }), entityFrom('AA', { namespace: 'namespace4' }), ]); @@ -1797,7 +1856,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response4 = await catalog.queryEntities(request4); - expect(response4.items).toMatchObject([ + expect(entitiesResponseToObjects(response4.items)).toMatchObject([ entityFrom('DD'), entityFrom('AA', { namespace: 'namespace2' }), ]); @@ -1812,7 +1871,7 @@ describe('DefaultEntitiesCatalog', () => { credentials: mockCredentials.none(), }; const response5 = await catalog.queryEntities(request5); - expect(response5.items).toMatchObject([ + expect(entitiesResponseToObjects(response5.items)).toMatchObject([ entityFrom('AA'), entityFrom('CC'), ]); From 8955923488c768a549b4cd508292c868b452fb41 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 12 Dec 2024 14:43:18 +0100 Subject: [PATCH 24/28] catalog-backend: update AuthorizedEntitiesCatalog tests with new response structure Signed-off-by: Patrik Oldsberg --- .../service/AuthorizedEntitiesCatalog.test.ts | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts index 2207802859..894949a691 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts @@ -64,7 +64,7 @@ describe('AuthorizedEntitiesCatalog', () => { credentials: mockCredentials.none(), }), ).toEqual({ - entities: [], + entities: { type: 'object', entities: [] }, pageInfo: { hasNextPage: false }, }); }); @@ -113,7 +113,7 @@ describe('AuthorizedEntitiesCatalog', () => { credentials: mockCredentials.none(), }), ).resolves.toEqual({ - items: [null], + items: { type: 'object', entities: [null] }, }); expect(fakeCatalog.entitiesBatch).not.toHaveBeenCalled(); @@ -174,7 +174,7 @@ describe('AuthorizedEntitiesCatalog', () => { filter: { key: 'kind', values: ['b'] }, }), ).resolves.toEqual({ - items: [], + items: { type: 'object', entities: [] }, pageInfo: {}, totalItems: 0, }); @@ -226,7 +226,7 @@ describe('AuthorizedEntitiesCatalog', () => { ]; fakeCatalog.queryEntities.mockResolvedValue({ - items: { type: 'objects', entities }, + items: { type: 'object', entities }, pageInfo: { nextCursor: { isPrevious: false, @@ -254,7 +254,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); expect(response).toEqual({ - items: entities, + items: { type: 'object', entities: entities }, totalItems: 4, pageInfo: { nextCursor: { @@ -290,7 +290,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); expect(response).toEqual({ - items: entities, + items: { type: 'object', entities: entities }, totalItems: 4, pageInfo: { nextCursor: { @@ -338,7 +338,9 @@ describe('AuthorizedEntitiesCatalog', () => { conditions: { rule: 'IS_ENTITY_KIND', params: { kinds: ['b'] } }, }, ]); - fakeCatalog.entities.mockResolvedValue({ entities: [] }); + fakeCatalog.entities.mockResolvedValue({ + entities: { type: 'object', entities: [] }, + }); const catalog = new AuthorizedEntitiesCatalog( fakeCatalog, fakePermissionApi, @@ -360,7 +362,10 @@ describe('AuthorizedEntitiesCatalog', () => { }, ]); fakeCatalog.entities.mockResolvedValue({ - entities: [{ kind: 'b', namespace: 'default', name: 'my-component' }], + entities: { + type: 'object', + entities: [{ kind: 'b', namespace: 'default', name: 'my-component' }], + }, }); const catalog = new AuthorizedEntitiesCatalog( fakeCatalog, From 14ce4026ed3d0753cccd7c06f4ed3090ea6f3605 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 13 Dec 2024 00:53:50 +0100 Subject: [PATCH 25/28] catalog-backend: rename enableRawJsonRespone -> disableRelationsCompatibility Signed-off-by: Patrik Oldsberg --- .changeset/curly-teachers-marry.md | 2 +- plugins/catalog-backend/config.d.ts | 10 ++++++++++ plugins/catalog-backend/src/service/CatalogBuilder.ts | 8 ++++---- .../src/service/DefaultEntitiesCatalog.ts | 10 ++++++---- .../catalog-backend/src/service/createRouter.test.ts | 2 +- plugins/catalog-backend/src/service/createRouter.ts | 6 +++--- plugins/catalog-backend/src/tests/integration.test.ts | 6 +++--- 7 files changed, 28 insertions(+), 16 deletions(-) diff --git a/.changeset/curly-teachers-marry.md b/.changeset/curly-teachers-marry.md index 21f41da291..ad22613612 100644 --- a/.changeset/curly-teachers-marry.md +++ b/.changeset/curly-teachers-marry.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-backend': minor --- -Added a new `catalog.enableRawJsonResponse` configuration option that avoids JSON deserialization and serialization if possible when reading entities. This can significantly improve the overall performance of the catalog, but it removes the backwards compatibility processing that ensures that both `entity.relation[].target` and `entity.relation[].targetRef` are present in returned entities. +Added a new `catalog.disableRelationsCompatibility` configuration option that avoids JSON deserialization and serialization if possible when reading entities. This can significantly improve the overall performance of the catalog, but it removes the backwards compatibility processing that ensures that both `entity.relation[].target` and `entity.relation[].targetRef` are present in returned entities. diff --git a/plugins/catalog-backend/config.d.ts b/plugins/catalog-backend/config.d.ts index 8dd5e50480..be6104ac84 100644 --- a/plugins/catalog-backend/config.d.ts +++ b/plugins/catalog-backend/config.d.ts @@ -138,6 +138,16 @@ export interface Config { }>; }>; + /** + * Disables the compatibility layer for relations in returned entities that + * ensures that all relations objects have both `target` and `targetRef`. + * + * Enabling this option can very significantly improve the performance of + * the catalog, but may break consumers that rely on the existence of + * `target` in the relations objects. + */ + disableRelationsCompatibility?: boolean; + /** * The strategy to use for entities that are orphaned, i.e. no longer have * any other entities or providers referencing them. The default value is diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index fd052bf474..7ce308e796 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -486,8 +486,8 @@ export class CatalogBuilder { discovery, }); - const enableRawJson = config.getOptionalBoolean( - 'catalog.enableRawJsonResponses', + const disableRelationsCompatibility = config.getOptionalBoolean( + 'catalog.disableRelationsCompatibility', ); const policy = this.buildEntityPolicy(); @@ -526,7 +526,7 @@ export class CatalogBuilder { database: dbClient, logger, stitcher, - enableRawJson, + disableRelationsCompatibility, }); let permissionsService: PermissionsService; @@ -638,7 +638,7 @@ export class CatalogBuilder { auth, httpAuth, permissionsService, - enableRawJson, + disableRelationsCompatibility, }); await connectEntityProviders(providerDatabase, entityProviders); diff --git a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts index ee9b46ce55..f0e0667bef 100644 --- a/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/DefaultEntitiesCatalog.ts @@ -105,18 +105,20 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { private readonly database: Knex; private readonly logger: LoggerService; private readonly stitcher: Stitcher; - private readonly enableRawJson: boolean; + private readonly disableRelationsCompatibility: boolean; constructor(options: { database: Knex; logger: LoggerService; stitcher: Stitcher; - enableRawJson?: boolean; + disableRelationsCompatibility?: boolean; }) { this.database = options.database; this.logger = options.logger; this.stitcher = options.stitcher; - this.enableRawJson = Boolean(options.enableRawJson); + this.disableRelationsCompatibility = Boolean( + options.disableRelationsCompatibility, + ); } async entities(request?: EntitiesRequest): Promise { @@ -197,7 +199,7 @@ export class DefaultEntitiesCatalog implements EntitiesCatalog { return { entities: processRawEntitiesResult( rows.map(r => r.final_entity!), - this.enableRawJson + this.disableRelationsCompatibility ? request?.fields : e => { expandLegacyCompoundRelationsInEntity(e); diff --git a/plugins/catalog-backend/src/service/createRouter.test.ts b/plugins/catalog-backend/src/service/createRouter.test.ts index 312900bcf1..53f831d320 100644 --- a/plugins/catalog-backend/src/service/createRouter.test.ts +++ b/plugins/catalog-backend/src/service/createRouter.test.ts @@ -883,7 +883,7 @@ describe('createRouter readonly and raw json enabled', () => { getLocationByEntity: jest.fn(), }; const router = await createRouter({ - enableRawJson: true, + disableRelationsCompatibility: true, entitiesCatalog, locationService, logger: mockServices.logger.mock(), diff --git a/plugins/catalog-backend/src/service/createRouter.ts b/plugins/catalog-backend/src/service/createRouter.ts index 6ea9ea1c13..84826267f2 100644 --- a/plugins/catalog-backend/src/service/createRouter.ts +++ b/plugins/catalog-backend/src/service/createRouter.ts @@ -85,7 +85,7 @@ export interface RouterOptions { auth: AuthService; httpAuth: HttpAuthService; permissionsService: PermissionsService; - enableRawJson?: boolean; + disableRelationsCompatibility?: boolean; } /** @@ -113,7 +113,7 @@ export async function createRouter( permissionsService, auth, httpAuth, - enableRawJson = false, + disableRelationsCompatibility = false, } = options; const readonlyEnabled = @@ -215,7 +215,7 @@ export async function createRouter( signal.throwIfAborted(); - if (!enableRawJson) { + if (!disableRelationsCompatibility) { processEntitiesResponseItems( result.items, expandLegacyCompoundRelationsInEntity, diff --git a/plugins/catalog-backend/src/tests/integration.test.ts b/plugins/catalog-backend/src/tests/integration.test.ts index 32116ace0d..e85ebe48ce 100644 --- a/plugins/catalog-backend/src/tests/integration.test.ts +++ b/plugins/catalog-backend/src/tests/integration.test.ts @@ -198,7 +198,7 @@ class TestHarness { readonly #proxyProgressTracker: ProxyProgressTracker; static async create(options?: { - enableRawJson?: boolean; + disableRelationsCompatibility?: boolean; logger?: LoggerService; db?: Knex; permissions?: PermissionEvaluator; @@ -277,7 +277,7 @@ class TestHarness { database: db, logger, stitcher, - enableRawJson: options?.enableRawJson, + disableRelationsCompatibility: options?.disableRelationsCompatibility, }); const proxyProgressTracker = new ProxyProgressTracker( new NoopProgressTracker(), @@ -788,7 +788,7 @@ describe('Catalog Backend Integration', () => { it('should return valid responses in raw JSON mode', async () => { const harness = await TestHarness.create({ - enableRawJson: true, + disableRelationsCompatibility: true, }); const entityA = { From 38436041d3158a6b5249e848d2a68af68589b315 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 13 Dec 2024 10:50:09 +0100 Subject: [PATCH 26/28] catalog-backend: add test for response writing and properly stop on close Signed-off-by: Patrik Oldsberg --- .../src/service/response/write.test.ts | 36 +++++++++++++++++++ .../src/service/response/write.ts | 24 ++++++++----- 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/plugins/catalog-backend/src/service/response/write.test.ts b/plugins/catalog-backend/src/service/response/write.test.ts index b1a688b48d..fdca874904 100644 --- a/plugins/catalog-backend/src/service/response/write.test.ts +++ b/plugins/catalog-backend/src/service/response/write.test.ts @@ -277,5 +277,41 @@ describe('writeEntitiesResponse', () => { totalItems: 1337, }); }); + + it('should write a large wrapped response', async () => { + const entityMock = JSON.stringify({ + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'my-component', + namespace: 'default', + annotations: { + 'backstage.io/managed-by-location': 'url:https://example.com', + }, + }, + spec: { + type: 'service', + owner: 'me', + lifecycle: 'production', + }, + }); + const res = await request(app) + .get('/wrapped') + .send({ + type: 'raw', + entities: Array(300).fill(entityMock), + }); + + expect(res.status).toBe(200); + expect(res.type).toBe('application/json'); + expect(res.header['content-type']).toBe( + 'application/json; charset=utf-8', + ); + expect(res.body).toEqual({ + page: 1, + items: expect.objectContaining({ length: 300 }), + totalItems: 1337, + }); + }); }); }); diff --git a/plugins/catalog-backend/src/service/response/write.ts b/plugins/catalog-backend/src/service/response/write.ts index 0962ee15d1..c2bca0953a 100644 --- a/plugins/catalog-backend/src/service/response/write.ts +++ b/plugins/catalog-backend/src/service/response/write.ts @@ -80,15 +80,23 @@ export async function writeEntitiesResponse( const needsDrain = !res.write(prefix + entity, 'utf8'); if (needsDrain) { - await new Promise(resolve => { - const cont = () => { - res.off('drain', cont); - res.off('close', cont); - resolve(); - }; - res.on('drain', cont); - res.on('close', cont); + const closed = await new Promise(resolve => { + function onContinue() { + res.off('drain', onContinue); + res.off('close', onClose); + resolve(false); + } + function onClose() { + res.off('drain', onContinue); + res.off('close', onClose); + resolve(true); + } + res.on('drain', onContinue); + res.on('close', onClose); }); + if (closed) { + return; + } } } res.end(`${first ? '[' : ''}]${trailing}`); From a86d259430a7c8d83a87454fe19df621ad17a6b4 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 13 Dec 2024 11:58:09 +0100 Subject: [PATCH 27/28] catalog-backend: fix relations compat flag not being applied Signed-off-by: Patrik Oldsberg --- plugins/catalog-backend/src/service/createRouter.ts | 2 +- plugins/catalog-backend/src/service/response/process.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/catalog-backend/src/service/createRouter.ts b/plugins/catalog-backend/src/service/createRouter.ts index 84826267f2..601b0b1098 100644 --- a/plugins/catalog-backend/src/service/createRouter.ts +++ b/plugins/catalog-backend/src/service/createRouter.ts @@ -216,7 +216,7 @@ export async function createRouter( signal.throwIfAborted(); if (!disableRelationsCompatibility) { - processEntitiesResponseItems( + result.items = processEntitiesResponseItems( result.items, expandLegacyCompoundRelationsInEntity, ); diff --git a/plugins/catalog-backend/src/service/response/process.ts b/plugins/catalog-backend/src/service/response/process.ts index 7e7730d697..fa07063f5b 100644 --- a/plugins/catalog-backend/src/service/response/process.ts +++ b/plugins/catalog-backend/src/service/response/process.ts @@ -39,7 +39,7 @@ export function processRawEntitiesResult( export function processEntitiesResponseItems( response: EntitiesResponseItems, transform?: (entity: Entity) => Entity, -) { +): EntitiesResponseItems { if (!transform) { return response; } From 5dcb0f3b7d975046c5ab40711fc6eb5c74ab99cd Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 13 Dec 2024 13:30:07 +0100 Subject: [PATCH 28/28] catalog-backend: update description of disableRelationsCompatibility post testing Signed-off-by: Patrik Oldsberg --- .changeset/curly-teachers-marry.md | 2 +- plugins/catalog-backend/config.d.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.changeset/curly-teachers-marry.md b/.changeset/curly-teachers-marry.md index ad22613612..9bf771f4d1 100644 --- a/.changeset/curly-teachers-marry.md +++ b/.changeset/curly-teachers-marry.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-backend': minor --- -Added a new `catalog.disableRelationsCompatibility` configuration option that avoids JSON deserialization and serialization if possible when reading entities. This can significantly improve the overall performance of the catalog, but it removes the backwards compatibility processing that ensures that both `entity.relation[].target` and `entity.relation[].targetRef` are present in returned entities. +Added a new `catalog.disableRelationsCompatibility` configuration option that avoids JSON deserialization and serialization if possible when reading entities. This significantly reduces the memory usage of the catalog, and slightly increases performance, but it removes the backwards compatibility processing that ensures that both `entity.relation[].target` and `entity.relation[].targetRef` are present in returned entities. diff --git a/plugins/catalog-backend/config.d.ts b/plugins/catalog-backend/config.d.ts index be6104ac84..384765f92e 100644 --- a/plugins/catalog-backend/config.d.ts +++ b/plugins/catalog-backend/config.d.ts @@ -142,9 +142,9 @@ export interface Config { * Disables the compatibility layer for relations in returned entities that * ensures that all relations objects have both `target` and `targetRef`. * - * Enabling this option can very significantly improve the performance of - * the catalog, but may break consumers that rely on the existence of - * `target` in the relations objects. + * Enabling this option significantly reduces the memory usage of the + * catalog, and slightly increases performance, but may break consumers that + * rely on the existence of `target` in the relations objects. */ disableRelationsCompatibility?: boolean;