-
Notifications
You must be signed in to change notification settings - Fork 403
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
Lexicon: declaration, assertion/confirmation, follow app migration #664
Lexicon: declaration, assertion/confirmation, follow app migration #664
Conversation
validate: false, | ||
}) | ||
} catch { | ||
const del = prepareDelete({ |
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.
just curious, how many hit this in trial runs?
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.
There were 7, I think. I looked at them all and they were indeed malformed in some way (either a handle rather than a did, or empty did).
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 both looks really great 🙌
Would you be up for adding some quick sanity checks at the end, that throw if they fail? like "count of follows = count before migration - count deleted" & "no declaration/assertion/confirmation left in records?
// Should be 4-5k records | ||
const recordsToDelete = await tx.db | ||
.selectFrom('record') | ||
.innerJoin('repo_root', 'repo_root.did', 'record.did') // Ignore any records not in a repo |
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.
did you encounter any of these in your tests?
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.
I didn't check actually, but I could do a run without this and find out.
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.
Okay this was overly paranoid, it just doesn't happen on any environment 😆 Will remove.
}), | ||
) | ||
|
||
await repoTx.processWrites(did, updates, now) |
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.
ah probably wanna ensure updates.length > 0
@dholms should be good for another look! |
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 great 👍
Two app migrations contained here:
subject
is a did rather than an actor ref. Invalid follow records are deleted.In both we use the repo service, which takes care of updating repositories plus the generic
record
index. If any app-view related changes are needed, those are handled at the end of the migrations— that would include updates toduplicate_record
andfollow
tables. A no-opmessageDispatcher
is used rather than a real one in order to avoid app-view indexing logic from kicking in.