diff --git a/api/db/item-annotations-queries.ts b/api/db/item-annotations-queries.ts index 0c0f912..c5695b3 100644 --- a/api/db/item-annotations-queries.ts +++ b/api/db/item-annotations-queries.ts @@ -76,7 +76,7 @@ function convertItemAnnotation(row: ItemAnnotationRow): ItemAnnotation { } /** - * Insert or update (upsert) a single item annotation. Loadouts are totally replaced when updated. + * Insert or update (upsert) a single item annotation. */ export async function updateItemAnnotation( client: ClientBase, diff --git a/api/routes/loadout-share.ts b/api/routes/loadout-share.ts index 32ec32f..6558c73 100644 --- a/api/routes/loadout-share.ts +++ b/api/routes/loadout-share.ts @@ -1,9 +1,8 @@ import crypto from 'crypto'; import asyncHandler from 'express-async-handler'; import base32 from 'hi-base32'; -import { DatabaseError } from 'pg-protocol'; import { transaction } from '../db/index.js'; -import { addLoadoutShare, getLoadoutShare, recordAccess } from '../db/loadout-share-queries.js'; +import { getLoadoutShare, recordAccess } from '../db/loadout-share-queries.js'; import { metrics } from '../metrics/index.js'; import { ApiApp } from '../shapes/app.js'; import { @@ -16,6 +15,7 @@ import { UserInfo } from '../shapes/user.js'; import { addLoadoutShare as addLoadoutShareStately, getLoadoutShare as getLoadoutShareStately, + LoadoutShareCollision, recordAccess as recordAccessStately, } from '../stately/loadout-share-queries.js'; import slugify from './slugify.js'; @@ -29,14 +29,11 @@ const getShareURL = (loadout: Loadout, shareId: string) => { return `https://dim.gg/${shareId}/${titleSlug}`; }; -// Turn this on to save all loadout shares to StatelyDB as well as Postgres -const saveToStately = true; - /** * Save a loadout to be shared via a dim.gg link. */ export const loadoutShareHandler = asyncHandler(async (req, res) => { - const { bungieMembershipId } = req.user as UserInfo; + const { profileIds } = req.user as UserInfo; const { id: appId } = req.dimApp as ApiApp; metrics.increment(`loadout_share.app.${appId}`, 1); const request = req.body as LoadoutShareRequest; @@ -49,6 +46,12 @@ export const loadoutShareHandler = asyncHandler(async (req, res) => { message: 'Loadouts require platform membership ID to be set', }); return; + } else if (!profileIds.includes(platformMembershipId)) { + metrics.increment('loadout_share.validation.platformMembershipIdMismatch.count'); + res.status(400).send({ + status: 'InvalidArgument', + message: 'Loadouts must be for the currently authenticated user', + }); } const validationResult = validateLoadout('loadout_share', loadout); @@ -57,28 +60,22 @@ export const loadoutShareHandler = asyncHandler(async (req, res) => { return; } - const shareId = await pgSaveLoadoutShare( - bungieMembershipId, - appId, - platformMembershipId, - loadout, - ); - if (shareId === 'ran-out') { - metrics.increment('loadout_share.ranOutOfAttempts.count'); - res.status(500).send({ - status: 'RanOutOfIDs', - message: "We couldn't generate a share URL", - }); - return; - } - - if (saveToStately) { + let shareId = ''; + let attempts = 0; + // We'll make three attempts to guess a random non-colliding number + while (attempts < 4) { + shareId = generateRandomShareId(); try { await addLoadoutShareStately(platformMembershipId, shareId, loadout); } catch (e) { - metrics.increment('loadout_share.statelyFailure.count'); - console.error('Failed to save loadout share to Stately', e); + // This is a unique constraint violation, generate another random share ID + if (e instanceof LoadoutShareCollision) { + // try again! + } else { + throw e; + } } + attempts++; } const result: LoadoutShareResponse = { @@ -88,41 +85,6 @@ export const loadoutShareHandler = asyncHandler(async (req, res) => { res.send(result); }); -async function pgSaveLoadoutShare( - bungieMembershipId: number, - appId: string, - platformMembershipId: string, - loadout: Loadout, -) { - return transaction(async (client) => { - let attempts = 0; - // We'll make three attempts to guess a random non-colliding number - while (attempts < 4) { - const shareId = generateRandomShareId(); - try { - await addLoadoutShare( - client, - appId, - bungieMembershipId, - platformMembershipId, - shareId, - loadout, - ); - return shareId; - } catch (e) { - // This is a unique constraint violation, generate another random share ID - if (e instanceof DatabaseError && e.code === '23505') { - // try again! - } else { - throw e; - } - } - attempts++; - } - return 'ran-out'; - }); -} - /** * Generate 4 random bytes (32 bits) and encode to base32url, which will yield 7 characters. * @@ -154,20 +116,19 @@ export const getLoadoutShareHandler = asyncHandler(async (req, res) => { }); export async function loadLoadoutShare(shareId: string) { - if (saveToStately) { - // This is just dual-reading to Stately for now - try { - const loadout = await getLoadoutShareStately(shareId); - if (loadout) { - // Record when this was viewed and increment the view counter. Not using it much for now but I'd like to know. - await recordAccessStately(shareId); - } - } catch (e) { - console.error('Failed to load loadout share from Stately', e); + // First look in Stately + try { + const loadout = await getLoadoutShareStately(shareId); + if (loadout) { + // Record when this was viewed and increment the view counter. Not using it much for now but I'd like to know. + await recordAccessStately(shareId); + return loadout; } + } catch (e) { + console.error('Failed to load loadout share from Stately', e); } - // Always read from Postgres + // Fall back to Postgres return transaction(async (client) => { const loadout = await getLoadoutShare(client, shareId); if (loadout) { diff --git a/api/routes/update.ts b/api/routes/update.ts index 20ba27d..b70d35c 100644 --- a/api/routes/update.ts +++ b/api/routes/update.ts @@ -193,6 +193,10 @@ export const updateHandler = asyncHandler(async (req, res) => { }); }); +// TODO: For ease of porting, I made each update a separate transaction. But it +// would be more efficient to batch them all into one transaction. That said, +// aside from bulk-tagging, it's most likely that only one update will be sent +// at a time. async function statelyUpdate( req: express.Request, updates: ProfileUpdate[], @@ -203,6 +207,9 @@ async function statelyUpdate( ) { const results: ProfileUpdateResult[] = []; + // TODO: batch these up - one phase for validation, then do all the reads, + // then all the updates, then all the deletes + for (const update of updates) { let result: ProfileUpdateResult; diff --git a/api/stately/loadout-share-queries.ts b/api/stately/loadout-share-queries.ts index 3362ddc..3a4582e 100644 --- a/api/stately/loadout-share-queries.ts +++ b/api/stately/loadout-share-queries.ts @@ -48,6 +48,8 @@ export async function addLoadoutShare( ): Promise { const loadoutShare = convertLoadoutShareToStately(loadout, platformMembershipId, shareId); + // TODO: This would be a nice place for Stately's initialValue option which + // would guarantee uniqueness. But it'd have to support our weird shareIDs.s await client.transaction(async (txn) => { const existing = await txn.get('LoadoutShare', keyFor(shareId)); // We do not want to overwrite an existing share! This is another place