ENG-2067 Refactor publishNodesToGroups to avoid database access in loops

apps/roam/src/utils/publishNodesToGroups.ts

@@ -2,26 +2,26 @@ import { CrossAppNode } from "@repo/database/crossAppContracts";
 import type { DGSupabaseClient } from "@repo/database/lib/client";
 import { getAvailableGroupIds } from "@repo/database/lib/groups";
 import { nodeUidsWithTypeToCrossApp } from "./roamToCrossAppConverters";
+import { ensurePartialSpaceAccess } from "@repo/database/lib/groups";
+import { isIgnorableUpsertError } from "@repo/database/lib/contextFunctions";
+import { difference, intersection } from "@repo/utils/setOperations";
+import internalError from "./internalError";
 
 export type NodeUidWithType = {
   uid: string;
   type: string;
 };
 
+const onlyStrings = (values: (string | null)[]): string[] =>
+  values.filter((value): value is string => typeof value === "string");
+
 type PublishNodesResult = {
   publishedNodeUids: string[];
   skippedUnsyncedUids: string[];
   okGroupIds: string[];
   failedGroupIds: string[];
 };
 
-// 23505 = unique_violation: the grant already exists, which counts as success.
-const isIgnorableUpsertError = (error: { code?: string } | null): boolean =>
-  !error || error.code === "23505";
-
-const onlyStrings = (values: (string | null)[]): string[] =>
-  values.filter((value): value is string => typeof value === "string");
-
 // Grants a group access to already-synced discourse nodes by mirroring the
 // Obsidian publish-to-group access model (SpaceAccess + ResourceAccess),
 // without its file/frontmatter/relation/asset coupling.
@@ -50,83 +50,66 @@ export const publishNodesToGroups = async ({
   if (nodes.length === 0 || groupIds.length === 0) return result;
 
   const availableGroupIds = new Set(await getAvailableGroupIds(client));
-  const requestedGroupIds = [...new Set(groupIds)];
-  const targetGroupIds = requestedGroupIds.filter((groupId) =>
-    availableGroupIds.has(groupId),
-  );
-  result.failedGroupIds = requestedGroupIds.filter(
-    (groupId) => !availableGroupIds.has(groupId),
-  );
-  if (targetGroupIds.length === 0) return result;
+  const requestedGroupIds = new Set(groupIds);
+  groupIds = [...intersection(requestedGroupIds, availableGroupIds)];
+  result.failedGroupIds = [...difference(requestedGroupIds, availableGroupIds)];
+  if (groupIds.length === 0) return result;
+
+  const { existing: existingSpaceAccess, missing: missingSpaceAccess } =
+    await ensurePartialSpaceAccess({
+      client,
+      groupIds,
+      spaceId,
+    });
+  if (missingSpaceAccess && Object.keys(missingSpaceAccess).length) {
+    result.failedGroupIds = [
+      ...result.failedGroupIds,
+      ...groupIds.filter((id) => !(id in existingSpaceAccess)),
+    ];
+    groupIds = Object.keys(existingSpaceAccess);
+  }
+  if (groupIds.length === 0) return result;
 
-  const uids = [...new Set(nodes.map((node) => node.localId))];
+  let nodeUids = [...new Set(nodes.map((node) => node.localId))];
 
   const syncedRes = await client
     .from("my_concepts")
     .select("source_local_id")
     .eq("space_id", spaceId)
-    .eq("is_schema", false)
-    .in("source_local_id", uids);
-  if (syncedRes.error) throw syncedRes.error;
+    .in("source_local_id", nodeUids);
+  if (syncedRes.error) {
+    internalError({ error: syncedRes.error });
+    return result;
+  }
   const syncedUids = new Set(
     onlyStrings((syncedRes.data ?? []).map((row) => row.source_local_id)),
   );
+  result.skippedUnsyncedUids = nodeUids.filter((uid) => !syncedUids.has(uid));
+  nodeUids = [...intersection(syncedUids, new Set(nodeUids))];
 
-  result.skippedUnsyncedUids = uids.filter((uid) => !syncedUids.has(uid));
-  const syncedNodeUids = uids.filter((uid) => syncedUids.has(uid));
-  if (syncedNodeUids.length === 0) return result;
-
-  // Required dependency: the node-type schema concept, when it is synced too.
-  const types = [
-    ...new Set(
-      nodes
-        .filter((node) => syncedUids.has(node.localId))
-        .map((node) => node.nodeType),
-    ),
-  ];
-  const schemaRes = await client
-    .from("my_concepts")
-    .select("source_local_id")
-    .eq("space_id", spaceId)
-    .eq("is_schema", true)
-    .in("source_local_id", types);
-  if (schemaRes.error) throw schemaRes.error;
-  const syncedSchemaIds = onlyStrings(
-    (schemaRes.data ?? []).map((row) => row.source_local_id),
-  );
-
-  const resourceIds = [...syncedNodeUids, ...syncedSchemaIds];
-
-  for (const groupId of targetGroupIds) {
-    // Existing reader/editor access is broader than partial, so leave it intact.
-    const spaceAccessRes = await client
-      .from("SpaceAccess")
-      .upsert(
-        { account_uid: groupId, space_id: spaceId, permissions: "partial" },
-        { ignoreDuplicates: true },
-      );
-    if (!isIgnorableUpsertError(spaceAccessRes.error)) {
-      result.failedGroupIds.push(groupId);
-      continue;
-    }
-
-    const grantRes = await client.from("ResourceAccess").upsert(
-      resourceIds.map((sourceLocalId) => ({
+  const resourceAccesses = [];
+  for (const groupId of groupIds) {
+    resourceAccesses.push(
+      ...nodeUids.map((sourceLocalId) => ({
         account_uid: groupId,
         source_local_id: sourceLocalId,
         space_id: spaceId,
       })),
-      { ignoreDuplicates: true },
     );
-    if (!isIgnorableUpsertError(grantRes.error)) {
-      result.failedGroupIds.push(groupId);
-      continue;
-    }
+  }
 
-    result.okGroupIds.push(groupId);
+  const grantRes = await client
+    .from("ResourceAccess")
+    .upsert(resourceAccesses, { ignoreDuplicates: true });
+  if (!isIgnorableUpsertError(grantRes.error)) {
+    internalError({ error: grantRes.error });
+    result.failedGroupIds.push(...groupIds);
+    return result;
   }
 
-  result.publishedNodeUids = result.okGroupIds.length > 0 ? syncedNodeUids : [];
+  result.okGroupIds = groupIds;
+  result.publishedNodeUids = nodeUids;
+
   return result;
 };
 

packages/database/src/lib/contextFunctions.ts

@@ -47,6 +47,11 @@ export const asPostgrestFailure = (
 
 export class FatalError extends Error {}
 
+// 23505 = unique_violation: the grant already exists, which counts as success.
+export const isIgnorableUpsertError = (
+  error: { code?: string } | null,
+): boolean => !error || error.code === "23505";
+
 export const spaceValidator = (space: SpaceCreationInput): string | null => {
   if (!space || typeof space !== "object")
     return "Invalid request body: expected a JSON object.";

packages/database/src/lib/groups.ts

@@ -1,4 +1,6 @@
 import type { DGSupabaseClient } from "./client";
+import type { Tables, Enums } from "../dbTypes";
+import { isIgnorableUpsertError } from "./contextFunctions";
 
 export type MyGroup = {
   id: string;
@@ -47,3 +49,57 @@ export const getMyGroups = async (
       name: row.my_groups.name ?? row.group_id,
     }));
 };
+
+type SpaceAccessPermissions = Enums<"SpaceAccessPermissions">;
+
+export const ensurePartialSpaceAccess = async ({
+  client,
+  groupIds,
+  spaceId,
+}: {
+  client: DGSupabaseClient;
+  groupIds: string[];
+  spaceId: number;
+}): Promise<{
+  existing: Record<number, SpaceAccessPermissions>;
+  missing?: Record<number, SpaceAccessPermissions>;
+}> => {
+  const existingAccessResult = await client
+    .from("SpaceAccess")
+    .select()
+    .eq("space_id", spaceId)
+    .in("account_uid", groupIds);
+  const existingAccessByGroupId = existingAccessResult.data
+    ? Object.fromEntries(
+        existingAccessResult.data.map((sa) => [sa.account_uid, sa.permissions]),
+      )
+    : {};
+  const missingAccess: Tables<"SpaceAccess">[] = [];
+  for (const groupId of groupIds) {
+    if (existingAccessByGroupId[groupId] === undefined) {
+      missingAccess.push({
+        space_id: spaceId,
+        permissions: "partial",
+        account_uid: groupId,
+      });
+    }
+  }
+  if (missingAccess.length > 0) {
+    const upsertAccessResult = await client
+      .from("SpaceAccess")
+      .upsert(missingAccess, { ignoreDuplicates: true });
+    if (!isIgnorableUpsertError(upsertAccessResult.error)) {
+      // allow partial results
+      return {
+        existing: existingAccessByGroupId,
+        missing: Object.fromEntries(
+          missingAccess.map((a) => [a.account_uid, a.permissions]),
+        ),
+      };
+    }
+  }
+  missingAccess.forEach((a) => {
+    existingAccessByGroupId[a.account_uid] = "partial";
+  });
+  return { existing: existingAccessByGroupId };
+};