diff --git a/.changeset/stupid-flies-shake.md b/.changeset/stupid-flies-shake.md new file mode 100644 index 0000000000..c9e138272e --- /dev/null +++ b/.changeset/stupid-flies-shake.md @@ -0,0 +1,7 @@ +--- +'@backstage/plugin-user-settings-backend': minor +'@backstage/plugin-user-settings-common': minor +'@backstage/plugin-user-settings': minor +--- + +User-settings will now use DataLoader to batch consecutive calls into one API call to improve performance diff --git a/plugins/user-settings-backend/package.json b/plugins/user-settings-backend/package.json index 75e74bfc55..e1f91a0abe 100644 --- a/plugins/user-settings-backend/package.json +++ b/plugins/user-settings-backend/package.json @@ -55,9 +55,12 @@ "@backstage/plugin-signals-node": "workspace:^", "@backstage/plugin-user-settings-common": "workspace:^", "@backstage/types": "workspace:^", + "already": "^2.2.1", "express": "^4.22.0", "express-promise-router": "^4.1.0", - "knex": "^3.0.0" + "knex": "^3.0.0", + "p-limit": "^3.1.0", + "zod": "^3.25.76" }, "devDependencies": { "@backstage/backend-defaults": "workspace:^", diff --git a/plugins/user-settings-backend/src/database/DatabaseUserSettingsStore.test.ts b/plugins/user-settings-backend/src/database/DatabaseUserSettingsStore.test.ts index 2d9ea5b23e..5fb2dde23f 100644 --- a/plugins/user-settings-backend/src/database/DatabaseUserSettingsStore.test.ts +++ b/plugins/user-settings-backend/src/database/DatabaseUserSettingsStore.test.ts @@ -107,6 +107,81 @@ describe.each(databases.eachSupportedId())( }); }); + describe('multiget', () => { + it('should return empty', async () => { + await expect( + storage.multiget({ + userEntityRef: 'user-1', + items: [ + { + bucket: 'bucket-c', + key: 'key-c', + }, + ], + }), + ).resolves.toEqual([null]); + }); + + it('should handle large inputs and return existing values', async () => { + // Create 30 buckets with 300 keys in each. + const BUCKETS = 30; + const KEYS_PER_BUCKET = 300; + + const buckets = Array.from(Array(BUCKETS)).map( + (_, i) => `mget-bucket-${i}`, + ); + + const items = buckets.flatMap(bucket => { + const keys = Array.from(Array(KEYS_PER_BUCKET)).map( + (_, i) => `mget-key-${i}`, + ); + return keys.map(key => ({ bucket, key })); + }); + + const chunkify = (keys: Array, size: number): T[][] => { + const chunks: T[][] = []; + for (let i = 0; i < keys.length; i += size) { + chunks.push(keys.slice(i, i + size)); + } + return chunks; + }; + + const valueOfKey = (bucket: string, key: string) => ({ + theValue: `Value of ${bucket} / ${key}`, + }); + + const chunkedItems = chunkify(items, 100); + for (const chunk of chunkedItems) { + await insert( + chunk.map(({ bucket, key }) => ({ + user_entity_ref: 'user-1', + bucket, + key, + value: JSON.stringify(valueOfKey(bucket, key)), + })), + ); + } + + const result = await storage.multiget({ + userEntityRef: 'user-1', + // Include a missing key which shouldn't exist in the result + items: [...items, { bucket: 'missing', key: 'missing' }], + }); + expect(result).toEqual([ + ...items.map(({ bucket, key }) => ({ + value: valueOfKey(bucket, key), + })), + null, // The missing key + ]); + + const rows = await knex('user_settings') + .where('user_entity_ref', 'user-1') + .count<{ count: string }[]>('* as count'); + const savedRows = Number(rows[0].count); + expect(savedRows).toEqual(BUCKETS * KEYS_PER_BUCKET); + }); + }); + describe('set', () => { it('should insert a new setting', async () => { await storage.set({ diff --git a/plugins/user-settings-backend/src/database/DatabaseUserSettingsStore.ts b/plugins/user-settings-backend/src/database/DatabaseUserSettingsStore.ts index 8caa62c4cd..c631227ae3 100644 --- a/plugins/user-settings-backend/src/database/DatabaseUserSettingsStore.ts +++ b/plugins/user-settings-backend/src/database/DatabaseUserSettingsStore.ts @@ -21,6 +21,7 @@ import { import { NotFoundError } from '@backstage/errors'; import { JsonValue } from '@backstage/types'; import { Knex } from 'knex'; +import pLimit from 'p-limit'; import { UserSettingsStore, type UserSetting } from './UserSettingsStore'; const migrationsDir = resolvePackagePath( @@ -28,6 +29,8 @@ const migrationsDir = resolvePackagePath( 'migrations', ); +const dbLimit = pLimit(10); + /** * @public */ @@ -91,6 +94,75 @@ export class DatabaseUserSettingsStore implements UserSettingsStore { }; } + async multiget(options: { + userEntityRef: string; + items: Array<{ bucket: string; key: string }>; + }): Promise<({ value: JsonValue } | null)[]> { + if (options.items.length === 0) { + return []; + } + + // Split the items into a map of bucket -> keys + const bucketMap = new Map>(); + for (const item of options.items) { + let keys = bucketMap.get(item.bucket); + if (!keys) { + keys = new Set(); + bucketMap.set(item.bucket, keys); + } + keys.add(item.key); + } + + // Chunks the keys per bucket to avoid hitting SQL parameter limits + const chunkKeys = (keys: Array, size: number): string[][] => { + const chunks = []; + for (let i = 0; i < keys.length; i += size) { + chunks.push(keys.slice(i, i + size)); + } + return chunks; + }; + + // Store the database content into a map of bucket -> {key -> value} + const resultsMap = new Map>(); + + await Promise.all( + Array.from(bucketMap.entries()).map(([bucket, keySet]) => + dbLimit(async (): Promise => { + const keyMap = new Map(); + resultsMap.set(bucket, keyMap); + + const keyChunks = chunkKeys(Array.from(keySet), 100); + + for (const keys of keyChunks) { + const rows = await this.db('user_settings') + .where({ + user_entity_ref: options.userEntityRef, + bucket, + }) + .whereIn('key', keys) + .select(['bucket', 'key', 'value']); + + for (const row of rows) { + keyMap.set(row.key, JSON.parse(row.value)); + } + } + }), + ), + ); + + // For each exact bucket/key requested, return either the value or null if + // not found + return options.items.map(({ bucket, key }) => { + const value = resultsMap.get(bucket)?.get(key); + + if (typeof value === 'undefined') { + return null; + } + + return { value }; + }); + } + async set(options: { userEntityRef: string; bucket: string; diff --git a/plugins/user-settings-backend/src/database/UserSettingsStore.ts b/plugins/user-settings-backend/src/database/UserSettingsStore.ts index 289c37523e..9c0a5ebf5b 100644 --- a/plugins/user-settings-backend/src/database/UserSettingsStore.ts +++ b/plugins/user-settings-backend/src/database/UserSettingsStore.ts @@ -35,6 +35,16 @@ export interface UserSettingsStore { key: string; }): Promise; + /** + * Get multiple user settings at once. The result is an array corresponding + * index by index to the elements of the input, where the elements are either + * `null` (not found), or an object `{ value: JsonValue }`. + */ + multiget(options: { + userEntityRef: string; + items: Array<{ bucket: string; key: string }>; + }): Promise<({ value: JsonValue } | null)[]>; + set(options: { userEntityRef: string; bucket: string; diff --git a/plugins/user-settings-backend/src/service/router.test.ts b/plugins/user-settings-backend/src/service/router.test.ts index 01277a9fd3..3503ba3c80 100644 --- a/plugins/user-settings-backend/src/service/router.test.ts +++ b/plugins/user-settings-backend/src/service/router.test.ts @@ -24,9 +24,11 @@ import { mockServices, mockErrorHandler, } from '@backstage/backend-test-utils'; +import { JsonValue } from '@backstage/types'; describe('createRouter', () => { const userSettingsStore: jest.Mocked = { + multiget: jest.fn(), get: jest.fn(), set: jest.fn(), delete: jest.fn(), @@ -108,6 +110,124 @@ describe('createRouter', () => { }); }); + describe('GET /multi', () => { + const mockMultiget = (store: Record>) => { + userSettingsStore.multiget.mockImplementation(async ({ items }) => { + return items.map(({ bucket, key }) => { + const value = store[bucket]?.[key]; + if (typeof value !== 'undefined') { + return { value }; + } + return null; + }); + }); + }; + + it('returns single value', async () => { + const values = { 'key-1': 'a' }; + + mockMultiget({ 'my-bucket': values }); + + const responses = await request(app) + .post('/multiget') + .send({ + items: Object.keys(values).map(key => ({ + bucket: 'my-bucket', + key, + })), + }); + + expect(responses.status).toEqual(200); + expect(responses.body).toEqual({ items: [{ value: 'a' }] }); + + expect(userSettingsStore.multiget).toHaveBeenCalledTimes(1); + expect(userSettingsStore.multiget).toHaveBeenCalledWith({ + userEntityRef: mockUserRef, + items: [ + { + bucket: 'my-bucket', + key: 'key-1', + }, + ], + }); + }); + + it('returns single missing', async () => { + const values = { 'key-1': 'a' }; + + mockMultiget({ 'my-bucket': values }); + + const responses = await request(app) + .post('/multiget') + .send({ + items: [{ bucket: 'my-bucket', key: 'missing-key' }], + }); + + expect(responses.status).toEqual(200); + expect(responses.body).toEqual({ + items: [null], + }); + + expect(userSettingsStore.multiget).toHaveBeenCalledTimes(1); + expect(userSettingsStore.multiget).toHaveBeenCalledWith({ + userEntityRef: mockUserRef, + items: [ + { + bucket: 'my-bucket', + key: 'missing-key', + }, + ], + }); + }); + + it('returns existing and missing mixed', async () => { + const values = { + 'key-1': 'a', + 'key-2': 'b', + }; + + mockMultiget({ 'my-bucket': values }); + + const responses = await request(app) + .post('/multiget') + .send({ + items: [ + ...Object.keys(values).map(key => ({ bucket: 'my-bucket', key })), + { bucket: 'my-bucket', key: 'missing-key' }, + ], + }); + + expect(responses.status).toEqual(200); + expect(responses.body).toEqual({ + items: [{ value: 'a' }, { value: 'b' }, null], + }); + + expect(userSettingsStore.multiget).toHaveBeenCalledTimes(1); + expect(userSettingsStore.multiget).toHaveBeenCalledWith({ + userEntityRef: mockUserRef, + items: [ + ...Object.keys(values).map(key => ({ + bucket: 'my-bucket', + key, + })), + { + bucket: 'my-bucket', + key: 'missing-key', + }, + ], + }); + }); + + it('returns an error if the Authorization header is missing', async () => { + const responses = await request(app) + .get('/buckets/my-bucket/keys/my-key') + .set('Authorization', mockCredentials.none.header()); + + expect(responses.status).toEqual(401); + expect(userSettingsStore.get).not.toHaveBeenCalled(); + }); + }); + describe('DELETE /buckets/:bucket/keys/:key', () => { it('returns ok', async () => { userSettingsStore.delete.mockResolvedValue(); diff --git a/plugins/user-settings-backend/src/service/router.ts b/plugins/user-settings-backend/src/service/router.ts index 3a0de7054c..c9971bbb4d 100644 --- a/plugins/user-settings-backend/src/service/router.ts +++ b/plugins/user-settings-backend/src/service/router.ts @@ -17,9 +17,13 @@ import { InputError } from '@backstage/errors'; import express, { Request } from 'express'; import Router from 'express-promise-router'; +import { z } from 'zod'; import { UserSettingsStore } from '../database/UserSettingsStore'; import { SignalsService } from '@backstage/plugin-signals-node'; -import { UserSettingsSignal } from '@backstage/plugin-user-settings-common'; +import { + MultiGetResponse, + UserSettingsSignal, +} from '@backstage/plugin-user-settings-common'; import { HttpAuthService } from '@backstage/backend-plugin-api'; export async function createRouter(options: { @@ -30,6 +34,10 @@ export async function createRouter(options: { const router = Router(); router.use(express.json()); + const multiGetRequestSchema = z.object({ + items: z.array(z.object({ bucket: z.string(), key: z.string() })), + }); + /** * Helper method to extract the userEntityRef from the request. */ @@ -40,6 +48,20 @@ export async function createRouter(options: { return credentials.principal.userEntityRef; }; + // get multiple values + router.post('/multiget', async (req, res) => { + const userEntityRef = await getUserEntityRef(req); + + const bucketsAndKeys = multiGetRequestSchema.parse(req.body).items; + + const items = await options.userSettingsStore.multiget({ + userEntityRef, + items: bucketsAndKeys, + }); + + res.json({ items } satisfies MultiGetResponse); + }); + // get a single value router.get('/buckets/:bucket/keys/:key(*)', async (req, res) => { const userEntityRef = await getUserEntityRef(req); diff --git a/plugins/user-settings-common/package.json b/plugins/user-settings-common/package.json index 82c5db84f1..92a5b84ecd 100644 --- a/plugins/user-settings-common/package.json +++ b/plugins/user-settings-common/package.json @@ -37,6 +37,9 @@ "postpack": "backstage-cli package postpack", "test": "backstage-cli package test" }, + "dependencies": { + "@backstage/types": "workspace:^" + }, "devDependencies": { "@backstage/cli": "workspace:^" } diff --git a/plugins/user-settings-common/report.api.md b/plugins/user-settings-common/report.api.md index b69f4a4190..42b9bd3f04 100644 --- a/plugins/user-settings-common/report.api.md +++ b/plugins/user-settings-common/report.api.md @@ -3,6 +3,15 @@ > Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/). ```ts +import type { JsonValue } from '@backstage/types'; + +// @public +export type MultiGetResponse = { + items: ({ + value: JsonValue; + } | null)[]; +}; + // @public (undocumented) export type UserSettingsSignal = { type: 'key-changed' | 'key-deleted'; diff --git a/plugins/user-settings-common/src/types.ts b/plugins/user-settings-common/src/types.ts index 6a49a72c93..437d0c8e1c 100644 --- a/plugins/user-settings-common/src/types.ts +++ b/plugins/user-settings-common/src/types.ts @@ -14,8 +14,17 @@ * limitations under the License. */ +import type { JsonValue } from '@backstage/types'; + /** @public */ export type UserSettingsSignal = { type: 'key-changed' | 'key-deleted'; key: string; }; + +/** + * Response type from /multiget + * + * @public + */ +export type MultiGetResponse = { items: ({ value: JsonValue } | null)[] }; diff --git a/plugins/user-settings/package.json b/plugins/user-settings/package.json index 4702b8cbeb..8cfaf74b41 100644 --- a/plugins/user-settings/package.json +++ b/plugins/user-settings/package.json @@ -69,6 +69,7 @@ "@material-ui/core": "^4.12.2", "@material-ui/icons": "^4.9.1", "@material-ui/lab": "4.0.0-alpha.61", + "dataloader": "^2.0.0", "react-use": "^17.2.4", "zen-observable": "^0.10.0" }, diff --git a/plugins/user-settings/src/apis/StorageApi/CacheMap.ts b/plugins/user-settings/src/apis/StorageApi/CacheMap.ts new file mode 100644 index 0000000000..271ee93f67 --- /dev/null +++ b/plugins/user-settings/src/apis/StorageApi/CacheMap.ts @@ -0,0 +1,56 @@ +/* + * Copyright 2025 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * A Map with TTL (time-to-live) support, for the Dataloader of user settings. + */ +export class CacheMap extends Map { + #ttlMs: number; + #timestamps: Map = new Map(); + + constructor(ttlMs: number) { + super(); + this.#ttlMs = ttlMs; + } + + set(key: K, value: V) { + const result = super.set(key, value); + this.#timestamps.set(key, Date.now()); + return result; + } + + get(key: K) { + if (!this.has(key)) { + return undefined; + } + const timestamp = this.#timestamps.get(key)!; + if (Date.now() - timestamp > this.#ttlMs) { + this.delete(key); + return undefined; + } + return super.get(key); + } + + delete(key: K) { + this.#timestamps.delete(key); + return super.delete(key); + } + + clear() { + this.#timestamps.clear(); + return super.clear(); + } +} diff --git a/plugins/user-settings/src/apis/StorageApi/UserSettingsStorage.test.ts b/plugins/user-settings/src/apis/StorageApi/UserSettingsStorage.test.ts index e54ac36624..0dd8aa4c36 100644 --- a/plugins/user-settings/src/apis/StorageApi/UserSettingsStorage.test.ts +++ b/plugins/user-settings/src/apis/StorageApi/UserSettingsStorage.test.ts @@ -25,6 +25,7 @@ import { mockApis, registerMswTestHooks, } from '@backstage/test-utils'; +import { createDeferred } from '@backstage/types'; import { rest } from 'msw'; import { setupServer } from 'msw/node'; import { UserSettingsStorage } from './UserSettingsStorage'; @@ -41,14 +42,12 @@ describe('Persistent Storage API', () => { const mockIdentityApi = mockApis.identity({ token: 'a-token' }); const mockIdentityApiFallback = mockApis.identity(); - const createPersistentStorage = ( - args?: Partial<{ - fetchApi: FetchApi; - discoveryApi: DiscoveryApi; - errorApi: ErrorApi; - namespace?: string; - }>, - ): StorageApi => { + const createPersistentStorage = (args: { + fetchApi?: FetchApi; + discoveryApi?: DiscoveryApi; + errorApi?: ErrorApi; + namespace: string; + }): StorageApi => { return UserSettingsStorage.create({ errorApi: mockErrorApi, fetchApi: new MockFetchApi(), @@ -58,14 +57,12 @@ describe('Persistent Storage API', () => { }); }; - const createPersistentStorageFallback = ( - args?: Partial<{ - fetchApi: FetchApi; - discoveryApi: DiscoveryApi; - errorApi: ErrorApi; - namespace?: string; - }>, - ): StorageApi => { + const createPersistentStorageFallback = (args: { + fetchApi?: FetchApi; + discoveryApi?: DiscoveryApi; + errorApi?: ErrorApi; + namespace: string; + }): StorageApi => { return UserSettingsStorage.create({ errorApi: mockErrorApi, fetchApi: new MockFetchApi(), @@ -75,21 +72,17 @@ describe('Persistent Storage API', () => { }); }; - afterEach(() => { + afterEach(async () => { + // Wait for server callbacks to settle before clearing the handlers. + // DataLoader delay is 10ms, so this should be plenty. + await new Promise(resolve => setTimeout(resolve, 100)); + jest.clearAllMocks(); + server.resetHandlers(); }); it('should return undefined for values which are unset', async () => { - const storage = createPersistentStorage(); - - server.use( - rest.get( - `${mockBaseUrl}/buckets/:bucket/keys/:key`, - async (_req, res, ctx) => { - return res(ctx.json({ value: 'a' })); - }, - ), - ); + const storage = createPersistentStorage({ namespace: 'undefined' }); expect(storage.snapshot('myfakekey').value).toBeUndefined(); expect(storage.snapshot('myfakekey')).toEqual({ @@ -100,7 +93,7 @@ describe('Persistent Storage API', () => { }); it('should allow setting of a simple data structure', async () => { - const storage = createPersistentStorage(); + const storage = createPersistentStorage({ namespace: 'simple' }); const dummyValue = 'a'; server.use( @@ -121,7 +114,7 @@ describe('Persistent Storage API', () => { }); it('should allow setting of a complex data structure', async () => { - const storage = createPersistentStorage(); + const storage = createPersistentStorage({ namespace: 'complex' }); const dummyValue = { some: 'nice data', with: { nested: 'values', nice: true }, @@ -144,7 +137,9 @@ describe('Persistent Storage API', () => { }); it('should fallback set when user not logged in', async () => { - const storage = createPersistentStorageFallback(); + const storage = createPersistentStorageFallback({ + namespace: 'not-logged-in', + }); const selectedKeyNextHandler = jest.fn(); const dummyValue = 'my-value'; @@ -171,12 +166,14 @@ describe('Persistent Storage API', () => { }); it('should subscribe to key changes when setting a new value', async () => { - const storage = createPersistentStorage(); + const storage = createPersistentStorage({ namespace: 'key-change-set' }); const wrongKeyNextHandler = jest.fn(); const selectedKeyNextHandler = jest.fn(); const mockData = { hello: 'im a great new value' }; + const serverCall = createDeferred(); + server.use( rest.put( `${mockBaseUrl}/buckets/:bucket/keys/:key`, @@ -188,6 +185,10 @@ describe('Persistent Storage API', () => { return res(ctx.json(data)); }, ), + rest.post(`${mockBaseUrl}/multiget`, async (_req, res, ctx) => { + serverCall.resolve(); + return res(ctx.json([])); + }), ); await new Promise(resolve => { @@ -212,14 +213,18 @@ describe('Persistent Storage API', () => { presence: 'present', value: mockData, }); + + await serverCall; }); it('should subscribe to key changes when deleting a value', async () => { - const storage = createPersistentStorage(); + const storage = createPersistentStorage({ namespace: 'key-change-delete' }); const wrongKeyNextHandler = jest.fn(); const selectedKeyNextHandler = jest.fn(); + const serverCall = createDeferred(); + server.use( rest.delete( `${mockBaseUrl}/buckets/:bucket/keys/:key`, @@ -227,6 +232,10 @@ describe('Persistent Storage API', () => { return res(ctx.status(204)); }, ), + rest.post(`${mockBaseUrl}/multiget`, async (_req, res, ctx) => { + serverCall.resolve(); + return res(ctx.json([])); + }), ); await new Promise(resolve => { @@ -251,10 +260,12 @@ describe('Persistent Storage API', () => { presence: 'absent', value: undefined, }); + + await serverCall; }); it('should not clash with other namespaces when creating buckets', async () => { - const rootStorage = createPersistentStorage(); + const rootStorage = createPersistentStorage({ namespace: 'clash' }); const selectedKeyNextHandler = jest.fn(); server.use( @@ -264,23 +275,21 @@ describe('Persistent Storage API', () => { const { bucket, key } = req.params; const { value } = await req.json(); - expect(bucket).toEqual('default.profile.something.deep'); + expect(bucket).toEqual('clash.profile.something.deep'); expect(key).toEqual('test2'); return res(ctx.json({ value })); }, ), - rest.get( - `${mockBaseUrl}/buckets/:bucket/keys/:key`, - async (req, res, ctx) => { - const { bucket, key } = req.params; + rest.post(`${mockBaseUrl}/multiget`, async (req, res, ctx) => { + const payload = await req.json(); + const { bucket, key } = payload.items[0]; - expect(bucket).toEqual('default.profile/something'); - expect(key).toEqual('deep/test2'); + expect(bucket).toEqual('clash.profile/something'); + expect(key).toEqual('deep/test2'); - return res(ctx.status(404)); - }, - ), + return res(ctx.status(404)); + }), ); // when getting key test2 it will translate to default.profile.something.deep/test2 @@ -321,15 +330,13 @@ describe('Persistent Storage API', () => { }); server.use( - rest.get( - `${mockBaseUrl}/buckets/:bucket/keys/:key`, - async (req, res, ctx) => { - const { bucket, key } = req.params; - expect(bucket).toEqual('Test.Mock.Thing'); - expect(key).toEqual('key'); - return res(ctx.text('{ invalid: json string }')); - }, - ), + rest.post(`${mockBaseUrl}/multiget`, async (req, res, ctx) => { + const payload = await req.json(); + expect(payload).toEqual({ + items: [{ bucket: 'Test.Mock.Thing', key: 'key' }], + }); + return res(ctx.text('{ invalid: json string }')); + }), ); await new Promise(resolve => { @@ -353,17 +360,14 @@ describe('Persistent Storage API', () => { }); it('should freeze the snapshot value', async () => { - const storage = createPersistentStorage(); + const storage = createPersistentStorage({ namespace: 'freeze' }); const selectedKeyNextHandler = jest.fn(); const data = { foo: 'bar', baz: [{ foo: 'bar' }] }; server.use( - rest.get( - `${mockBaseUrl}/buckets/:bucket/keys/:key`, - async (_req, res, ctx) => { - return res(ctx.json({ value: data })); - }, - ), + rest.post(`${mockBaseUrl}/multiget`, async (_req, res, ctx) => { + return res(ctx.json({ items: [{ value: data }] })); + }), ); await new Promise(resolve => { @@ -396,4 +400,112 @@ describe('Persistent Storage API', () => { snapshot.value.baz.push({ foo: 'buzz' }); }).toThrow(/Cannot add property 1, object is not extensible/); }); + + it('should batch multiple calls into one', async () => { + const storage = createPersistentStorage({ namespace: 'multiget' }); + const selectedKeyNextHandler = jest.fn(); + const selectedKeyNextHandlerCached = jest.fn(); + const data1 = { foo: 'bar1', baz: [{ foo: 'bar1' }] }; + const data2 = { foo: 'bar2', baz: [{ foo: 'bar2' }] }; + + let serverCalls = 0; + + server.use( + rest.post(`${mockBaseUrl}/multiget`, async (req, res, ctx) => { + ++serverCalls; + const payload = (await req.json()) as { + items: { bucket: string; key: string }[]; + }; + const result = payload.items.map(item => { + if (item.key === 'key1') { + return { value: data1 }; + } else if (item.key === 'key2') { + return { value: data2 }; + } + return null; + }); + + return res(ctx.json({ items: result })); + }), + ); + + await Promise.all([ + new Promise(resolve => { + storage.observe$('key1').subscribe({ + next: snapshot => { + selectedKeyNextHandler(snapshot); + if (snapshot.presence === 'present') { + resolve(); + } + }, + }); + + storage.snapshot('key1'); + }), + + new Promise(resolve => { + storage.observe$('missing-key').subscribe({ + next: snapshot => { + selectedKeyNextHandler(snapshot); + if (snapshot.presence === 'absent') { + resolve(); + } + }, + }); + + storage.snapshot('missing-key'); + }), + + new Promise(resolve => { + storage.observe$('key2').subscribe({ + next: snapshot => { + selectedKeyNextHandler(snapshot); + if (snapshot.presence === 'present') { + resolve(); + } + }, + }); + + storage.snapshot('key2'); + }), + ]); + + expect(selectedKeyNextHandler).toHaveBeenCalledWith({ + key: 'key1', + presence: 'present', + value: data1, + }); + expect(selectedKeyNextHandler).toHaveBeenCalledWith({ + key: 'key2', + presence: 'present', + value: data2, + }); + expect(selectedKeyNextHandler).toHaveBeenCalledWith({ + key: 'missing-key', + presence: 'absent', + }); + expect(mockErrorApi.post).not.toHaveBeenCalled(); + expect(serverCalls).toBe(1); + + // Get key1 again, should use cached value + await new Promise(resolve => { + storage.observe$('key1').subscribe({ + next: snapshot => { + selectedKeyNextHandlerCached(snapshot); + if (snapshot.presence === 'present') { + resolve(); + } + }, + }); + + storage.snapshot('key1'); + }); + + expect(selectedKeyNextHandlerCached).toHaveBeenCalledWith({ + key: 'key1', + presence: 'present', + value: data1, + }); + expect(serverCalls).toBe(1); + }); }); diff --git a/plugins/user-settings/src/apis/StorageApi/UserSettingsStorage.ts b/plugins/user-settings/src/apis/StorageApi/UserSettingsStorage.ts index aa3080345c..9a5a0f22ed 100644 --- a/plugins/user-settings/src/apis/StorageApi/UserSettingsStorage.ts +++ b/plugins/user-settings/src/apis/StorageApi/UserSettingsStorage.ts @@ -27,7 +27,12 @@ import { ResponseError } from '@backstage/errors'; import { JsonValue, Observable } from '@backstage/types'; import { SignalApi, SignalSubscriber } from '@backstage/plugin-signals-react'; import ObservableImpl from 'zen-observable'; -import { UserSettingsSignal } from '@backstage/plugin-user-settings-common'; +import { + MultiGetResponse, + UserSettingsSignal, +} from '@backstage/plugin-user-settings-common'; +import DataLoader from 'dataloader'; +import { CacheMap } from './CacheMap'; const JSON_HEADERS = { 'Content-Type': 'application/json; charset=utf-8', @@ -36,6 +41,15 @@ const JSON_HEADERS = { const buckets = new Map(); +const DATALOADER_CACHE_TTL_MS = 2 * 1000; // 2 seconds cache +const DATALOADER_WINDOW_MS = 10; // 10 ms + +type DataLoaderType = DataLoader< + { bucket: string; key: string }, + StorageValueSnapshot, + string +>; + /** * An implementation of the storage API, that uses the user-settings backend to * persist the data in the DB. @@ -59,6 +73,7 @@ export class UserSettingsStorage implements StorageApi { private readonly identityApi: IdentityApi; private readonly fallback: WebStorage; private readonly signalApi?: SignalApi; + private readonly userSettingsLoader: DataLoaderType; private constructor( namespace: string, @@ -68,6 +83,7 @@ export class UserSettingsStorage implements StorageApi { identityApi: IdentityApi, fallback: WebStorage, signalApi?: SignalApi, + userSettingsLoader?: DataLoaderType, ) { this.namespace = namespace; this.fetchApi = fetchApi; @@ -76,6 +92,35 @@ export class UserSettingsStorage implements StorageApi { this.identityApi = identityApi; this.fallback = fallback; this.signalApi = signalApi; + + this.userSettingsLoader = + userSettingsLoader ?? + new DataLoader( + async bucketAndKeyList => this.getMulti(bucketAndKeyList), + { + name: 'UserSettingsStorage.userSettingsLoader', + cacheMap: new CacheMap< + string, + Promise> + >(DATALOADER_CACHE_TTL_MS), + cacheKeyFn: bucketAndKey => this.stringifyDataLoaderKey(bucketAndKey), + maxBatchSize: 100, + batchScheduleFn: cb => setTimeout(cb, DATALOADER_WINDOW_MS), + }, + ); + } + + private stringifyDataLoaderKey({ + bucket, + key, + }: { + bucket: string; + key: string; + }) { + return `${encodeURIComponent(bucket)}/${encodeURIComponent(key)}`; + } + private clearCacheKey(key: string) { + this.userSettingsLoader.clear({ bucket: this.namespace, key }); } static create(options: { @@ -114,6 +159,8 @@ export class UserSettingsStorage implements StorageApi { this.errorApi, this.identityApi, this.fallback, + this.signalApi, + this.userSettingsLoader, ), ); } @@ -132,6 +179,8 @@ export class UserSettingsStorage implements StorageApi { throw await ResponseError.fromResponse(response); } + this.clearCacheKey(key); + this.notifyChanges({ key, presence: 'absent' }); } @@ -139,9 +188,12 @@ export class UserSettingsStorage implements StorageApi { if (!(await this.isSignedIn())) { await this.fallback.set(key, data); this.notifyChanges({ key, presence: 'present', value: data }); + this.clearCacheKey(key); return; } + this.clearCacheKey(key); + const fetchUrl = await this.getFetchUrl(key); const response = await this.fetchApi.fetch(fetchUrl, { @@ -156,6 +208,15 @@ export class UserSettingsStorage implements StorageApi { const { value } = await response.json(); + this.userSettingsLoader.prime( + { bucket: this.namespace, key }, + { + key, + presence: 'present', + value, + }, + ); + this.notifyChanges({ key, value, presence: 'present' }); } @@ -171,7 +232,9 @@ export class UserSettingsStorage implements StorageApi { const updateSnapshot = () => { Promise.resolve() - .then(() => this.get(key)) + .then(() => + this.userSettingsLoader.load({ bucket: this.namespace, key }), + ) .then(snapshot => subscriber.next(snapshot)) .catch(error => this.errorApi.post(error)); }; @@ -206,38 +269,62 @@ export class UserSettingsStorage implements StorageApi { return { key, presence: 'unknown' }; } - private async get( - key: string, - ): Promise> { + private async getMulti( + bucketAndKeyList: readonly { bucket: string; key: string }[], + ): Promise[]> { + if (bucketAndKeyList.length === 0) return []; + if (!(await this.isSignedIn())) { // This explicitly uses WebStorage, which we know is synchronous and doesn't return presence: unknown - return this.fallback.snapshot(key); - } - - const fetchUrl = await this.getFetchUrl(key); - const response = await this.fetchApi.fetch(fetchUrl); - - if (response.status === 404) { - return { key, presence: 'absent' }; - } - - if (!response.ok) { - throw await ResponseError.fromResponse(response); + return bucketAndKeyList.map(bucketAndKey => + this.fallback.snapshot(bucketAndKey.key), + ); } try { - const { value: rawValue } = await response.json(); - const value = JSON.parse(JSON.stringify(rawValue), (_key, val) => { - if (typeof val === 'object' && val !== null) { - Object.freeze(val); - } - return val; + const baseUrl = await this.discoveryApi.getBaseUrl('user-settings'); + const response = await this.fetchApi.fetch(`${baseUrl}/multiget`, { + method: 'POST', + headers: JSON_HEADERS, + body: JSON.stringify({ items: bucketAndKeyList }), }); - return { key, presence: 'present', value }; - } catch { - // If the value is not valid JSON, we return an unknown presence. This should never happen - return { key, presence: 'absent' }; + if (response.status === 404) { + return bucketAndKeyList.map(bucketAndKey => ({ + key: bucketAndKey.key, + presence: 'absent', + })); + } + + if (!response.ok) { + throw await ResponseError.fromResponse(response); + } + + const { items: values } = (await response.json()) as MultiGetResponse; + + return bucketAndKeyList.map( + ({ key }, i): StorageValueSnapshot => { + if (!values[i]) { + return { key, presence: 'absent' }; + } + return { + key, + presence: 'present', + value: JSON.parse(JSON.stringify(values[i].value), (_key, val) => { + if (typeof val === 'object' && val !== null) { + Object.freeze(val); + } + return val; + }), + }; + }, + ); + } catch (e) { + this.errorApi.post(new Error(`Failed to fetch user settings, ${e}`)); + return bucketAndKeyList.map(bucketAndKey => ({ + key: bucketAndKey.key, + presence: 'absent', + })); } } diff --git a/yarn.lock b/yarn.lock index 9b84ee2cbb..a8ae398d6a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7784,10 +7784,13 @@ __metadata: "@backstage/types": "workspace:^" "@types/express": "npm:^4.17.6" "@types/supertest": "npm:^2.0.8" + already: "npm:^2.2.1" express: "npm:^4.22.0" express-promise-router: "npm:^4.1.0" knex: "npm:^3.0.0" + p-limit: "npm:^3.1.0" supertest: "npm:^7.0.0" + zod: "npm:^3.25.76" languageName: unknown linkType: soft @@ -7796,6 +7799,7 @@ __metadata: resolution: "@backstage/plugin-user-settings-common@workspace:plugins/user-settings-common" dependencies: "@backstage/cli": "workspace:^" + "@backstage/types": "workspace:^" languageName: unknown linkType: soft @@ -7826,6 +7830,7 @@ __metadata: "@testing-library/react": "npm:^16.0.0" "@testing-library/user-event": "npm:^14.0.0" "@types/react": "npm:^18.0.0" + dataloader: "npm:^2.0.0" msw: "npm:^1.0.0" react: "npm:^18.0.2" react-dom: "npm:^18.0.2" @@ -24494,6 +24499,13 @@ __metadata: languageName: node linkType: hard +"already@npm:^2.2.1": + version: 2.2.1 + resolution: "already@npm:2.2.1" + checksum: 10/1c55b50667c3dbe9d40716454d4a870f5758143061a0e39c0a7077eab2c6dbec116edf081796afb6f441462096bf68ef72a4daad074843a0d970527f29037ffe + languageName: node + linkType: hard + "anser@npm:^2.1.1": version: 2.3.3 resolution: "anser@npm:2.3.3"