Merge pull request #241 from DestinyItemManager/loadout-shares-stately

Change loadout shares to prefer Stately, no longer dual write

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,

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) {

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;
 

api/stately/loadout-share-queries.ts

@@ -48,6 +48,8 @@ export async function addLoadoutShare(
 ): Promise<void> {
   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