From dd6b9225f03000f036fc59f97ae47eca5db408b4 Mon Sep 17 00:00:00 2001 From: Blayne Chard Date: Thu, 14 Jul 2022 17:16:36 +1200 Subject: [PATCH 1/2] feat(config): ensure ids are prefixed before querying with them --- .../dynamo/__tests__/config.dynamo.test.ts | 137 ++++++++++++++++++ .../dynamo/__tests__/config.imagery.test.ts | 38 +++-- .../config/src/dynamo/dynamo.config.base.ts | 17 ++- .../config/src/dynamo/dynamo.config.cached.ts | 14 +- 4 files changed, 187 insertions(+), 19 deletions(-) create mode 100644 packages/config/src/dynamo/__tests__/config.dynamo.test.ts diff --git a/packages/config/src/dynamo/__tests__/config.dynamo.test.ts b/packages/config/src/dynamo/__tests__/config.dynamo.test.ts new file mode 100644 index 000000000..50b4a0c48 --- /dev/null +++ b/packages/config/src/dynamo/__tests__/config.dynamo.test.ts @@ -0,0 +1,137 @@ +import DynamoDB from 'aws-sdk/clients/dynamodb.js'; +import o from 'ospec'; +import sinon from 'sinon'; +import { Config } from '../../base.config.js'; +import { ConfigImagery } from '../../config/imagery.js'; +import { ConfigTileSet } from '../../config/tile.set.js'; +import { ConfigDynamoCached } from '../dynamo.config.cached.js'; +import { ConfigProviderDynamo } from '../dynamo.config.js'; + +const sandbox = sinon.createSandbox(); + +class FakeDynamoDb { + values: Map> = new Map(); + get: unknown[] = []; + getAll: { RequestItems: { Foo: { Keys: { id: { S: string } }[] } } }[] = []; + getItem(req: any): unknown { + this.get.push(req); + const reqId = req.Key.id.S; + const val = this.values.get(reqId); + return { + promise(): Promise { + if (val) return Promise.resolve({ Item: DynamoDB.Converter.marshall(val) }); + return Promise.resolve(null); + }, + }; + } + + batchGetItem(req: any): unknown { + this.getAll.push(req); + const keys = req.RequestItems.Foo.Keys.map((c: any) => DynamoDB.Converter.unmarshall(c).id); + const output = keys.map((c: string) => this.values.get(c)).filter((f: unknown) => f != null); + return { + promise(): Promise { + if (output.length === 0) return Promise.resolve({ Responses: {} }); + return Promise.resolve({ Responses: { Foo: output.map((c: any) => DynamoDB.Converter.marshall(c)) } }); + }, + }; + } +} + +o.spec('ConfigDynamo', () => { + let provider: ConfigProviderDynamo; + let fakeDynamo: FakeDynamoDb; + + o.beforeEach(() => { + provider = new ConfigProviderDynamo('Foo'); + Config.setConfigProvider(provider); + fakeDynamo = new FakeDynamoDb(); + provider.dynamo = fakeDynamo as any; + }); + + o.afterEach(() => sandbox.restore()); + + o('should not get if missing', async () => { + const ret = await Config.TileSet.get('ts_abc123'); + + o(fakeDynamo.get).deepEquals([{ Key: { id: { S: 'ts_abc123' } }, TableName: 'Foo' }]); + o(ret).equals(null); + }); + + o('should get', async () => { + fakeDynamo.values.set('ts_abc123', { id: 'ts_abc123' }); + const ret = await Config.TileSet.get('ts_abc123'); + + o(fakeDynamo.get).deepEquals([{ Key: { id: { S: 'ts_abc123' } }, TableName: 'Foo' }]); + o(ret).deepEquals({ id: 'ts_abc123' } as ConfigTileSet); + }); + + o('should get-all', async () => { + fakeDynamo.values.set('ts_abc123', { id: 'ts_abc123' }); + fakeDynamo.values.set('ts_abc456', { id: 'ts_abc456' }); + const ret = await Config.TileSet.getAll(new Set(fakeDynamo.values.keys())); + + o(fakeDynamo.getAll[0].RequestItems.Foo.Keys).deepEquals([{ id: { S: 'ts_abc123' } }, { id: { S: 'ts_abc456' } }]); + o([...ret.values()]).deepEquals([...fakeDynamo.values.values()] as any); + }); + + o('should get without prefix', async () => { + fakeDynamo.values.set('ts_abc123', { id: 'ts_abc123' }); + const ret = await Config.TileSet.get('abc123'); + + o(fakeDynamo.get).deepEquals([{ Key: { id: { S: 'ts_abc123' } }, TableName: 'Foo' }]); + o(ret).deepEquals({ id: 'ts_abc123' } as ConfigTileSet); + }); + + o('should get-all without prefix', async () => { + fakeDynamo.values.set('ts_abc123', { id: 'ts_abc123' }); + fakeDynamo.values.set('ts_abc456', { id: 'ts_abc456' }); + const ret = await Config.TileSet.getAll(new Set(['abc123', 'ts_abc456'])); + + o(fakeDynamo.getAll[0].RequestItems.Foo.Keys).deepEquals([{ id: { S: 'ts_abc123' } }, { id: { S: 'ts_abc456' } }]); + o([...ret.values()]).deepEquals([...fakeDynamo.values.values()] as any); + }); + + o('should get-all partial', async () => { + fakeDynamo.values.set('ts_abc123', { id: 'ts_abc123' }); + const ret = await Config.TileSet.getAll(new Set(['abc123', 'ts_abc456'])); + o(fakeDynamo.getAll[0].RequestItems.Foo.Keys).deepEquals([{ id: { S: 'ts_abc123' } }, { id: { S: 'ts_abc456' } }]); + o([...ret.values()]).deepEquals([...fakeDynamo.values.values()] as any); + }); + + o('should force a prefix even if prefix is defined', async () => { + const ret = await Config.TileSet.get('im_abc123'); + + o(fakeDynamo.get).deepEquals([{ Key: { id: { S: 'ts_im_abc123' } }, TableName: 'Foo' }]); + o(ret).deepEquals(null); + }); + + o.spec('DynamoCached', () => { + o('should get-all with cache', async () => { + fakeDynamo.values.set('im_abc123', { id: 'im_abc123' }); + fakeDynamo.values.set('im_abc456', { id: 'im_abc456' }); + + const cached = Config.Imagery as ConfigDynamoCached; + cached.cache.set('im_abc123', { id: 'im_abc123' } as ConfigImagery); + const ret = await Config.Imagery.getAll(new Set(['abc123', 'im_abc456'])); + + o(fakeDynamo.getAll[0].RequestItems.Foo.Keys).deepEquals([{ id: { S: 'im_abc456' } }]); + o([...ret.values()]).deepEquals([...fakeDynamo.values.values()] as any); + }); + + o('should get with cache', async () => { + fakeDynamo.values.set('im_abc123', { id: 'im_abc123' }); + + const cached = Config.Imagery as ConfigDynamoCached; + cached.cache.set('im_abc123', { id: 'im_abc123' } as ConfigImagery); + const ret = await Config.Imagery.get('abc123'); + + o(fakeDynamo.get).deepEquals([]); + o(ret).deepEquals({ id: 'im_abc123' } as ConfigImagery); + + const retPrefixed = await Config.Imagery.get('im_abc123'); + o(fakeDynamo.get).deepEquals([]); + o(retPrefixed).deepEquals({ id: 'im_abc123' } as ConfigImagery); + }); + }); +}); diff --git a/packages/config/src/dynamo/__tests__/config.imagery.test.ts b/packages/config/src/dynamo/__tests__/config.imagery.test.ts index 7e9d0b5cd..1549c2494 100644 --- a/packages/config/src/dynamo/__tests__/config.imagery.test.ts +++ b/packages/config/src/dynamo/__tests__/config.imagery.test.ts @@ -1,4 +1,4 @@ -import { Epsg, EpsgCode, NamedBounds, QuadKey, TileMatrixSet, TileMatrixSets } from '@basemaps/geo'; +import { Epsg } from '@basemaps/geo'; import DynamoDB from 'aws-sdk/clients/dynamodb.js'; import o from 'ospec'; import sinon from 'sinon'; @@ -9,14 +9,6 @@ import { ConfigProviderDynamo } from '../dynamo.config.js'; const sandbox = sinon.createSandbox(); -export function qkToNamedBounds(quadKeys: string[]): NamedBounds[] { - const tms = TileMatrixSets.get(EpsgCode.Google); - return quadKeys.map((qk) => ({ - name: TileMatrixSet.tileToName(QuadKey.toTile(qk)), - ...tms.tileToSourceBounds(QuadKey.toTile(qk)), - })); -} - o.spec('ConfigProvider.Imagery', () => { const provider = new ConfigProviderDynamo('Foo'); @@ -73,6 +65,34 @@ o.spec('ConfigProvider.Imagery', () => { o(result.get('im_foo4')).deepEquals(item); }); + o('should get un-prefixed', async () => { + const get = sandbox.stub(provider.dynamo, 'getItem').returns({ promise: () => Promise.resolve(null) } as any); + await provider.Imagery.get('foo1'); + o(get.callCount).equals(1); + o(get.firstCall.args[0]).deepEquals({ Key: { id: { S: 'im_foo1' } }, TableName: 'Foo' } as any); + }); + + o('should getall un-prefixed', async () => { + const keys = ['foo1', 'im_foo2', 'foo3']; + const get = sandbox.stub(provider.dynamo, 'batchGetItem').returns({ + promise: () => + Promise.resolve({ + Responses: { + [provider.tableName]: keys + .map((c) => Config.prefix(ConfigPrefix.Imagery, c)) + .map((id) => { + return DynamoDB.Converter.marshall({ id }); + }), + }, + }), + } as any); + await provider.Imagery.getAll(new Set(keys)); + o(get.callCount).equals(1); + o(get.firstCall.args[0]).deepEquals({ + RequestItems: { Foo: { Keys: [{ id: { S: 'im_foo1' } }, { id: { S: 'im_foo2' } }, { id: { S: 'im_foo3' } }] } }, + } as any); + }); + o('should handle unprocessed keys', async () => { const bulk = sandbox.stub(provider.dynamo, 'batchGetItem').callsFake((req: any) => { const keys = req.RequestItems[provider.tableName].Keys; diff --git a/packages/config/src/dynamo/dynamo.config.base.ts b/packages/config/src/dynamo/dynamo.config.base.ts index c8acea07f..61c6c4358 100644 --- a/packages/config/src/dynamo/dynamo.config.base.ts +++ b/packages/config/src/dynamo/dynamo.config.base.ts @@ -4,7 +4,8 @@ import { BaseConfig } from '../config/base.js'; import { ConfigPrefix } from '../config/prefix.js'; import { ConfigProviderDynamo } from './dynamo.config.js'; -function toId(id: string): { id: { S: string } } { +export type IdQuery = { id: { S: string } }; +function toId(id: string): IdQuery { return { id: { S: id } }; } @@ -16,6 +17,11 @@ export class ConfigDynamoBase extends Basemap this.cfg = cfg; } + /** Ensure the ID is prefixed before querying */ + ensureId(id: string): string { + if (id.startsWith(this.prefix)) return id; + return `${this.prefix}_${id}`; + } private get db(): DynamoDB { return this.cfg.dynamo; } @@ -29,7 +35,9 @@ export class ConfigDynamoBase extends Basemap } public async get(key: string): Promise { - const item = await this.db.getItem({ Key: { id: { S: key } }, TableName: this.cfg.tableName }).promise(); + const item = await this.db + .getItem({ Key: { id: { S: this.ensureId(key) } }, TableName: this.cfg.tableName }) + .promise(); if (item == null || item.Item == null) return null; const obj = DynamoDB.Converter.unmarshall(item.Item) as BaseConfig; if (this.is(obj)) return obj; @@ -38,7 +46,8 @@ export class ConfigDynamoBase extends Basemap /** Get all records with the id */ public async getAll(keys: Set): Promise> { - let mappedKeys = Array.from(keys, toId); + let mappedKeys: IdQuery[] = []; + for (const key of keys) mappedKeys.push(toId(this.ensureId(key))); const output: Map = new Map(); @@ -52,7 +61,7 @@ export class ConfigDynamoBase extends Basemap const items = await this.db.batchGetItem({ RequestItems }).promise(); const metadataItems = items.Responses?.[this.cfg.tableName]; - if (metadataItems == null) throw new Error('Failed to fetch metadata from ' + this.cfg.tableName); + if (metadataItems == null) throw new Error('Failed to fetch from ' + this.cfg.tableName); for (const row of metadataItems) { const item = DynamoDB.Converter.unmarshall(row) as BaseConfig; diff --git a/packages/config/src/dynamo/dynamo.config.cached.ts b/packages/config/src/dynamo/dynamo.config.cached.ts index a15612497..4bb7f22e7 100644 --- a/packages/config/src/dynamo/dynamo.config.cached.ts +++ b/packages/config/src/dynamo/dynamo.config.cached.ts @@ -5,11 +5,12 @@ export class ConfigDynamoCached extends ConfigDynamoBase = new Map(); public async get(id: string): Promise { - let existing: T | null | undefined = this.cache.get(id); + const queryKey = this.ensureId(id); + let existing: T | null | undefined = this.cache.get(queryKey); if (existing == null) { - existing = await super.get(id); + existing = await super.get(queryKey); if (existing == null) return null; - this.cache.set(id, existing); + this.cache.set(queryKey, existing); } return existing; @@ -20,11 +21,12 @@ export class ConfigDynamoCached extends ConfigDynamoBase(); for (const id of ids) { - const existing = this.cache.get(id); + const queryKey = this.ensureId(id); + const existing = this.cache.get(queryKey); if (existing == null) { - toFetch.add(id); + toFetch.add(queryKey); } else { - output.set(id, existing); + output.set(queryKey, existing); } } From 2db057ca3632c6a18b52af9e7c1f8e04f0fb7b0e Mon Sep 17 00:00:00 2001 From: Blayne Chard Date: Thu, 14 Jul 2022 18:19:35 +1200 Subject: [PATCH 2/2] refactor: throw on bad keys --- .../dynamo/__tests__/config.dynamo.test.ts | 41 ++++++++----------- .../dynamo/__tests__/config.imagery.test.ts | 28 ------------- .../config/src/dynamo/dynamo.config.base.ts | 5 ++- 3 files changed, 21 insertions(+), 53 deletions(-) diff --git a/packages/config/src/dynamo/__tests__/config.dynamo.test.ts b/packages/config/src/dynamo/__tests__/config.dynamo.test.ts index 50b4a0c48..4d720a3be 100644 --- a/packages/config/src/dynamo/__tests__/config.dynamo.test.ts +++ b/packages/config/src/dynamo/__tests__/config.dynamo.test.ts @@ -75,35 +75,34 @@ o.spec('ConfigDynamo', () => { o([...ret.values()]).deepEquals([...fakeDynamo.values.values()] as any); }); - o('should get without prefix', async () => { + o('should throw without prefix', async () => { fakeDynamo.values.set('ts_abc123', { id: 'ts_abc123' }); - const ret = await Config.TileSet.get('abc123'); + const ret = await Config.TileSet.get('abc123').catch((e) => e); - o(fakeDynamo.get).deepEquals([{ Key: { id: { S: 'ts_abc123' } }, TableName: 'Foo' }]); - o(ret).deepEquals({ id: 'ts_abc123' } as ConfigTileSet); + o(fakeDynamo.get).deepEquals([]); + o(String(ret)).deepEquals('Error: Trying to query "abc123" expected prefix of ts'); }); - o('should get-all without prefix', async () => { + o('should get-all partial', async () => { fakeDynamo.values.set('ts_abc123', { id: 'ts_abc123' }); - fakeDynamo.values.set('ts_abc456', { id: 'ts_abc456' }); - const ret = await Config.TileSet.getAll(new Set(['abc123', 'ts_abc456'])); - + const ret = await Config.TileSet.getAll(new Set(['ts_abc123', 'ts_abc456'])); o(fakeDynamo.getAll[0].RequestItems.Foo.Keys).deepEquals([{ id: { S: 'ts_abc123' } }, { id: { S: 'ts_abc456' } }]); o([...ret.values()]).deepEquals([...fakeDynamo.values.values()] as any); }); - o('should get-all partial', async () => { - fakeDynamo.values.set('ts_abc123', { id: 'ts_abc123' }); - const ret = await Config.TileSet.getAll(new Set(['abc123', 'ts_abc456'])); - o(fakeDynamo.getAll[0].RequestItems.Foo.Keys).deepEquals([{ id: { S: 'ts_abc123' } }, { id: { S: 'ts_abc456' } }]); - o([...ret.values()]).deepEquals([...fakeDynamo.values.values()] as any); + o('should throw if on wrong prefix', async () => { + const ret = await Config.TileSet.get('im_abc123').catch((e) => e); + o(fakeDynamo.get).deepEquals([]); + o(String(ret)).deepEquals('Error: Trying to query "im_abc123" expected prefix of ts'); }); - o('should force a prefix even if prefix is defined', async () => { - const ret = await Config.TileSet.get('im_abc123'); + o('should throw on prefixed and un-prefixed', async () => { + fakeDynamo.values.set('ts_abc123', { id: 'ts_abc123' }); - o(fakeDynamo.get).deepEquals([{ Key: { id: { S: 'ts_im_abc123' } }, TableName: 'Foo' }]); - o(ret).deepEquals(null); + const ret = Config.TileSet.getAll(new Set(['abc123', 'ts_abc123'])); + const err = await ret.then(() => null).catch((e) => e); + o(String(err)).equals('Error: Trying to query "abc123" expected prefix of ts'); + o(fakeDynamo.getAll).deepEquals([]); }); o.spec('DynamoCached', () => { @@ -113,7 +112,7 @@ o.spec('ConfigDynamo', () => { const cached = Config.Imagery as ConfigDynamoCached; cached.cache.set('im_abc123', { id: 'im_abc123' } as ConfigImagery); - const ret = await Config.Imagery.getAll(new Set(['abc123', 'im_abc456'])); + const ret = await Config.Imagery.getAll(new Set(['im_abc123', 'im_abc456'])); o(fakeDynamo.getAll[0].RequestItems.Foo.Keys).deepEquals([{ id: { S: 'im_abc456' } }]); o([...ret.values()]).deepEquals([...fakeDynamo.values.values()] as any); @@ -124,14 +123,10 @@ o.spec('ConfigDynamo', () => { const cached = Config.Imagery as ConfigDynamoCached; cached.cache.set('im_abc123', { id: 'im_abc123' } as ConfigImagery); - const ret = await Config.Imagery.get('abc123'); + const ret = await Config.Imagery.get('im_abc123'); o(fakeDynamo.get).deepEquals([]); o(ret).deepEquals({ id: 'im_abc123' } as ConfigImagery); - - const retPrefixed = await Config.Imagery.get('im_abc123'); - o(fakeDynamo.get).deepEquals([]); - o(retPrefixed).deepEquals({ id: 'im_abc123' } as ConfigImagery); }); }); }); diff --git a/packages/config/src/dynamo/__tests__/config.imagery.test.ts b/packages/config/src/dynamo/__tests__/config.imagery.test.ts index 1549c2494..4a53c0e00 100644 --- a/packages/config/src/dynamo/__tests__/config.imagery.test.ts +++ b/packages/config/src/dynamo/__tests__/config.imagery.test.ts @@ -65,34 +65,6 @@ o.spec('ConfigProvider.Imagery', () => { o(result.get('im_foo4')).deepEquals(item); }); - o('should get un-prefixed', async () => { - const get = sandbox.stub(provider.dynamo, 'getItem').returns({ promise: () => Promise.resolve(null) } as any); - await provider.Imagery.get('foo1'); - o(get.callCount).equals(1); - o(get.firstCall.args[0]).deepEquals({ Key: { id: { S: 'im_foo1' } }, TableName: 'Foo' } as any); - }); - - o('should getall un-prefixed', async () => { - const keys = ['foo1', 'im_foo2', 'foo3']; - const get = sandbox.stub(provider.dynamo, 'batchGetItem').returns({ - promise: () => - Promise.resolve({ - Responses: { - [provider.tableName]: keys - .map((c) => Config.prefix(ConfigPrefix.Imagery, c)) - .map((id) => { - return DynamoDB.Converter.marshall({ id }); - }), - }, - }), - } as any); - await provider.Imagery.getAll(new Set(keys)); - o(get.callCount).equals(1); - o(get.firstCall.args[0]).deepEquals({ - RequestItems: { Foo: { Keys: [{ id: { S: 'im_foo1' } }, { id: { S: 'im_foo2' } }, { id: { S: 'im_foo3' } }] } }, - } as any); - }); - o('should handle unprocessed keys', async () => { const bulk = sandbox.stub(provider.dynamo, 'batchGetItem').callsFake((req: any) => { const keys = req.RequestItems[provider.tableName].Keys; diff --git a/packages/config/src/dynamo/dynamo.config.base.ts b/packages/config/src/dynamo/dynamo.config.base.ts index 61c6c4358..c3b5aff08 100644 --- a/packages/config/src/dynamo/dynamo.config.base.ts +++ b/packages/config/src/dynamo/dynamo.config.base.ts @@ -19,9 +19,10 @@ export class ConfigDynamoBase extends Basemap /** Ensure the ID is prefixed before querying */ ensureId(id: string): string { - if (id.startsWith(this.prefix)) return id; - return `${this.prefix}_${id}`; + if (id.startsWith(this.prefix + '_')) return id; + throw new Error(`Trying to query "${id}" expected prefix of ${this.prefix}`); } + private get db(): DynamoDB { return this.cfg.dynamo; }