✨ Ozone batch repo and record getters #2836

foysalit · 2024-09-27T14:42:27Z

This PR introduces 2 new endpoints on ozone tools.ozone.moderation.getRecords and tools.ozone.moderation.getRepos in order to allow fetching repo and record details in batches.

…atch-repo-and-record

dholms · 2024-09-30T22:01:58Z

packages/ozone/src/api/moderation/getRecord.ts

-      const [record, accountInfo] = await Promise.all([
-        ctx.modService(db).views.recordDetail(params, labelers),
+      const [records, accountInfos] = await Promise.all([
+        ctx.modService(db).views.recordDetail([params], labelers),


Can we pluralize everything that is taking a list & returning a map?

ie modService(db).views.recordDetails

and getPdsAccountInfos

dholms · 2024-09-30T22:03:29Z

packages/ozone/src/api/moderation/getRecords.ts

+
+      const results: (RecordViewDetail | RecordViewNotFound)[] = []
+
+      params.uris.forEach((uri) => {


a map() might be a bit clearer here

const results = params.uris.map((uri) => ...)

dholms · 2024-09-30T22:04:13Z

packages/ozone/src/api/moderation/getRepos.ts

+
+      const repos: (RepoViewDetail | RepoViewNotFound)[] = []
+
+      dids.forEach((did) => {


same thing here: a map() might be a bit clearer

dholms · 2024-09-30T22:07:24Z

packages/ozone/src/api/util.ts

@@ -30,17 +30,28 @@ import { ids } from '../lexicon/lexicons'

 export const getPdsAccountInfo = async (
  ctx: AppContext,
-  did: string,
-): Promise<AccountView | null> => {
+  did: string | string[],


I'm inclined to just have this always take a string[] if it's always returning a Map of account views. Just keeps the function a bit simpler

dholms · 2024-09-30T22:08:52Z

packages/ozone/src/mod-service/views.ts

@@ -224,24 +224,33 @@ export class ModerationViews {
  }

  async repoDetail(
-    did: string,
+    dids: string | string[],


similarly, i'm inclined to just have this always take an array

dholms · 2024-09-30T22:15:37Z

packages/ozone/src/mod-service/views.ts

+    repos.forEach((repo, did) => {
+      const labels = [
+        ...(localLabels.get(did) || []),
+        ...externalLabels.filter((label) => label.uri === did),


I wonder if getExternalLabels should return a Map<string, Label[]> instead of just a big unfiltered array. That way we don't need to repeatedly scan over the array to get our list of labels for each subject

dholms · 2024-09-30T22:19:13Z

packages/ozone/src/mod-service/views.ts

+
+    const results = new Map<string, RecordViewDetail>()
+
+    records.forEach(async (record, uri) => {


Pretty sure you lose track of the promise like this. I think you either need to use a for loop or map() with a Promise.all.

I'd be inclined to do the latter so you can grab the blob infos in parallel

dholms · 2024-09-30T22:19:39Z

packages/ozone/tests/get-records.test.ts

+import { forSnapshot } from './_util'
+import { ids } from '../src/lexicon/lexicons'
+
+describe('admin get record view', () => {


Suggested change

describe('admin get record view', () => {

describe('admin get records view', () => {

dholms · 2024-09-30T22:20:04Z

packages/ozone/tests/get-records.test.ts

+    })
+  })
+
+  it('gets a records by uris', async () => {


Suggested change

it('gets a records by uris', async () => {

it('get multiple records by uri', async () => {

dholms · 2024-09-30T22:22:12Z

packages/pds/src/account-manager/helpers/account.ts

@@ -64,6 +64,41 @@ export const getAccount = async (
  return found || null
 }

+export const getAccounts = async (
+  db: AccountDb,
+  handlesAndDids: string[],


since getAccountInfos only takes dids, you can probably simplify this by just taking an array of DIDs (no handles)

dholms · 2024-09-30T22:23:32Z

packages/pds/src/account-manager/helpers/account.ts

+
+  const accounts = await selectAccountQB(db, flags)
+    .if(handles.size > 0, (qb) =>
+      qb.where('actor.handle', 'in', Array.from(handles)),


I'm pretty sure these two .wheres strung together are going to turn into a WHERE ... AND ...

Kysely has an orWhere function, but sometimes the grouping around those can be a little bit tricky. Partially why I recommend just ditching the handles 😛

dholms · 2024-09-30T22:24:42Z

packages/pds/src/account-manager/helpers/account.ts

@@ -64,6 +64,41 @@ export const getAccount = async (
  return found || null
 }

+export const getAccounts = async (
+  db: AccountDb,
+  handlesAndDids: string[],


also kysely gets mad if you pass in an empty array, so the first thing I'd do in this function is check if the array length is 0 & if so then just return an empty map

dholms · 2024-10-01T00:09:08Z

packages/pds/src/account-manager/helpers/invite.ts

+): Promise<Map<string, CodeDetail[]>> => {
+  const results = new Map<string, CodeDetail[]>()
+  const res = await selectInviteCodesQb(db)
+    .where('forAccount', 'in', dids)


like above^^ make sure you don't pass in an empty array

dholms · 2024-10-01T00:10:01Z

packages/pds/src/account-manager/helpers/invite.ts

@@ -155,6 +155,30 @@ export const getAccountInviteCodes = async (
  }))
 }

+export const getAccountsInviteCodes = async (


Can dry this up a bit & use this function to implement getAccountInviteCodes?

decided to stick to the pattern of take an array and return a map and got rid of the singular getAccountInviteCodes in favor of getAccountsInviteCodes

dholms · 2024-10-01T00:10:52Z

packages/pds/src/api/com/atproto/admin/getAccountInfos.ts

+      const managesOwnInvites = !ctx.cfg.entryway
+      const infos: AccountView[] = []
+
+      accounts.forEach((account) => {


.map() might be a bit cozier here too

dholms · 2024-10-01T00:12:04Z

Overall looking pretty good! Some style tweaks and what not but nothing too major.

Main thing that is prone to errors is accidentally passing empty arrays into kysely where() clauses

…ror type

dholms

looking good ✨

foysalit added 6 commits September 27, 2024 15:47

✨ Add getRepos and getRecords endpoints for bulk fetching

700eba3

Merge branch 'main' of github.com:bluesky-social/atproto into ozone-b…

2fec6f6

…atch-repo-and-record

✨ Fix issues and add tests for get repos and get records

bdc0a58

✨ Use the right lxm

c957ea1

🐛 Revert changes in lockfile

bcdaff9

✨ Add getAccountInfos in PDS

f788fb7

foysalit marked this pull request as ready for review September 30, 2024 07:55

foysalit added 2 commits September 30, 2024 12:11

🐛 Fix type def for repo and record view detail

e966ff5

✅ Update snapshots

f0d5351

foysalit mentioned this pull request Sep 30, 2024

✨ Fetch workspace data in batches and add advanced filtering bluesky-social/ozone#206

Merged

✅ Update snapshots

2ab2ba4

foysalit requested review from dholms, devinivy and matthieusieben September 30, 2024 16:58

foysalit changed the title ~~Ozone batch repo and record getters~~ ✨ Ozone batch repo and record getters Sep 30, 2024

dholms reviewed Sep 30, 2024

View reviewed changes

dholms reviewed Oct 1, 2024

View reviewed changes

✨ Consolidate error type for com.atproto and tools.ozone getRecord er…

e5ec727

…ror type

foysalit force-pushed the ozone-batch-repo-and-record branch from aab44ab to e5ec727 Compare October 1, 2024 09:13

foysalit added 3 commits October 1, 2024 13:30

🧹 Cleanup

19bd23b

✅ Update snapshots

100f8c1

✅ Update snapshots

cda9843

foysalit requested a review from dholms October 1, 2024 12:15

✨ Changeset

e23c68a

dholms approved these changes Oct 1, 2024

View reviewed changes

foysalit merged commit a2bad97 into main Oct 1, 2024
10 checks passed

foysalit deleted the ozone-batch-repo-and-record branch October 1, 2024 17:37

github-actions bot mentioned this pull request Oct 1, 2024

Version packages #2846

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Ozone batch repo and record getters #2836

✨ Ozone batch repo and record getters #2836

foysalit commented Sep 27, 2024

dholms Sep 30, 2024

dholms Sep 30, 2024 •

edited

Loading

dholms Sep 30, 2024

dholms Sep 30, 2024

dholms Sep 30, 2024

dholms Sep 30, 2024

dholms Sep 30, 2024

dholms Sep 30, 2024

dholms Sep 30, 2024 •

edited

Loading

dholms Sep 30, 2024

dholms Sep 30, 2024

dholms Sep 30, 2024

dholms Oct 1, 2024

dholms Oct 1, 2024

foysalit Oct 1, 2024

dholms Oct 1, 2024

dholms commented Oct 1, 2024

dholms left a comment


		const results: (RecordViewDetail \| RecordViewNotFound)[] = []

		params.uris.forEach((uri) => {


		const repos: (RepoViewDetail \| RepoViewNotFound)[] = []

		dids.forEach((did) => {


		const results = new Map<string, RecordViewDetail>()

		records.forEach(async (record, uri) => {

	describe('admin get record view', () => {
	describe('admin get records view', () => {

	it('gets a records by uris', async () => {
	it('get multiple records by uri', async () => {

✨ Ozone batch repo and record getters #2836

✨ Ozone batch repo and record getters #2836

Conversation

foysalit commented Sep 27, 2024

Choose a reason for hiding this comment

dholms Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholms Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholms commented Oct 1, 2024

dholms left a comment

Choose a reason for hiding this comment

dholms Sep 30, 2024 •

edited

Loading

dholms Sep 30, 2024 •

edited

Loading