Add account details to queryStatuses #3236
Conversation
matthieusieben
commented
Dec 12, 2024
•
matthieusieben
commented
- Add support for pre-existing view indexes in the migration (allowing the views and indexes to be created manually instead of through the migration)
- Refresh materialized views from the worker (note: refresh order matters!)
- Determine which columns to expose as filter
- For those: add an index on their respective materialized views
- Adapt the code to support the filtering
matthieusieben
force-pushed
the
msi/ozone-subject-stats
branch
2 times, most recently
from
3a88fb1 to
9401fc2
Compare
matthieusieben
force-pushed
the
msi/ozone-subject-stats
branch
from
7bbec5a to
3f7412c
Compare
matthieusieben
force-pushed
the
msi/ozone-subject-stats
branch
from
d86cf08 to
da81bb8
Compare
| } | ||
| }, | ||
| "accountStats": { | ||
| "description": "Statistics about a particular account subject on the labeller", |
There was a problem hiding this comment.
nit, but generally we refer to these as "mod service" instead of labeler since it's duties are wider than just labeling. alternately, could just leave off the "on the labeler"
| ) | ||
| .execute() | ||
|
|
||
| // TODO try/catch to ignore existing |
There was a problem hiding this comment.
migrations are meant to be run once and nearly all of our migrations will fail if re-run (for instance createTable)
basically, i don't think you need to worry about a try/catch here
There was a problem hiding this comment.
Because I manually created those view in the prod db. But Devin tells me that migrations are manually performed anyways so this does not matter indeed.
| export const moderationSubjectStatusQueryBuilder = (db: DatabaseSchema) => { | ||
| return db | ||
| .selectFrom('moderation_subject_status as mss') | ||
| .selectAll('mss') |
There was a problem hiding this comment.
I do get the appeal of the shortened versions of these table names, but they leak out into the calling functions and I find it hard to balance the mental state of them. Generally I try to only use abbreviated names like this when the variable is confined to some scoped function & the context is contained within a dozen or two lines. Even though it's wordier, I'm inclined to just use the table names for these.
| @@ -312,19 +313,6 @@ export class ModerationService { | |||
| .executeTakeFirst() | |||
| } | |||
|
|
|||
| async getCurrentStatus( | |||
There was a problem hiding this comment.
This work might just be in-progress, but I think this method is still needed. Specifically, I see it used in createReport
There was a problem hiding this comment.
This is a mistake. will revert 👍
|
|
||
| const builder = moderationSubjectStatusQueryBuilder(this.db.db).where( | ||
| (clause) => { | ||
| return parsedSubjects.reduce( |
There was a problem hiding this comment.
This is kind've badass, and I'm a fan of using reduce in most places. But in the context of postgres subqueries, I lean more towards a straight-foward/imperative approach that makes it a bit easier to reason about.
I do really appreciate getting rid of the index check on this which i think is similarly unclear
There was a problem hiding this comment.
Ideally we would use this pattern but that requires updating Kysely (which is non trivial).
But reading the docs of the current version we use, we can see:
Even the first
wherecan be anorWhere. This is useful if you are looping through a set of conditions:
So we don't actually need the index check.
| const applySubjectFilter = filterForSubject( | ||
| subject.did, | ||
| subject.recordPath, | ||
| if (!subjects.length) return new Map() |
There was a problem hiding this comment.
nice! looks so much simpler!
