-
Notifications
You must be signed in to change notification settings - Fork 630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ozone label endpoints #2043
Ozone label endpoints #2043
Conversation
@@ -13,6 +13,9 @@ export const readEnv = (): OzoneEnvironment => { | |||
pdsDid: envStr('OZONE_PDS_DID'), | |||
dbPostgresUrl: envStr('OZONE_DB_POSTGRES_URL'), | |||
dbPostgresSchema: envStr('OZONE_DB_POSTGRES_SCHEMA'), | |||
dbPoolSize: envInt('OZONE_DB_POOL_SIZE'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these crept in cause I thought i was running into some connection pool issues in the test, but it turns out that wasn't the case
kept them around since it's just nice to have 👍
throw new InvalidRequestError(`invalid pattern: ${pattern}`) | ||
} | ||
const searchPattern = pattern.slice(0, -1) | ||
qb = qb.orWhere('uri', 'like', `${searchPattern}%`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be nice to sanitize these search patterns so that they can't use weird arbitrary like
pattern syntax.
Also just a note—I am pretty sure postgres can turn uri like '{prefix}%'
into a simple range scan if uri
is indexed 👍
export default function (server: Server, ctx: AppContext) { | ||
server.com.atproto.label.subscribeLabels(async function* ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tidy ✅
oc.columns(['src', 'uri', 'cid', 'val']).doUpdateSet({ | ||
id: sql`${excluded('id')}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! Slightly surprised this just works, cool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i was unsure if it was going to. But seems to work nicely & thinking it through, it make sense since the ID is taken from the sequence before the write actually occurs
@@ -858,12 +857,14 @@ export class ModerationService { | |||
neg: !!l.neg, | |||
})) | |||
const { ref } = this.db.db.dynamic | |||
await sql`notify ${ref(LabelChannel)}`.execute(this.db.db) // emitted transactionally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: the comment—does this need to be emitted transactionally? It seems like it probably doesn't need to be transactional, and I have a feeling this method often runs outside of a transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid—nice to be able to pinch the sequencer!
if (sources && sources.length > 0) { | ||
builder = builder.where('src', 'in', sources) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to support both multiple sources and multiple patterns w/ pagination? I think multiple patterns w/ pagination or multiple sources w/ pagination will probably be simple enough for postgres, but all three at once could put it in a tricky spot for a large result set.
Co-authored-by: devin ivy <[email protected]>
Adds endpoints for
Subscribe labels uses the sequencer/outbox model that we also used on the PDS, backed by Postgres & just routine polling.
Note on deploying this:
I added a new serial
id
column to the label table. Not exactly sure how we want to manage this one but we'll have to figure out something sneaky on it 🤔