Ian/misc - #74
Conversation
commit: |
erquhart
left a comment
erquhart
left a comment
There was a problem hiding this comment.
One blocking issue and we should drop the remaining workpool references and dependency.
| const userId = | ||
| "id" in args.event.data | ||
| ? args.event.data.id | ||
| : "userId" in args.event.data | ||
| ? args.event.data.userId | ||
| : undefined; |
There was a problem hiding this comment.
Because we support handling all events and not just user events, this might be an id from a non-user data object. It could also be null.
There was a problem hiding this comment.
Understood. I'm fine with false negatives here. If an org id happens to match a userId that'd be surprising. The index here would just be used to delete things for a given user. But maybe it should be named more generically to delete by dataId or something.
My guess is that users with data retention policies will want ways to delete all data related to a user within a time frame. One way to satisfy it is to delete all events within that time frame. Another is to track which events come from which users.
Why don't we decouple this change from the rest and follow up on the simpler time-bound vacuuming first?
There was a problem hiding this comment.
I don't think we need to decouple, the fixes here are simple - check for userId first and filter out nulls. I'll push a commit
There was a problem hiding this comment.
sounds good, updated
Address PR feedback on #74: - Derive an event's userId from `data.userId` first, falling back to `data.id`, and only record it when it's a string. Events for non-user objects can carry a null id, which the schema rejected outright, and a session-style event's own id was previously preferred over the userId it references. Add tests for all three cases. - Import `vResultValidator` from `@convex-dev/workflow` (which re-exports it) and drop the unused `@convex-dev/workpool/test` imports left over from the removed event workpool, so `@convex-dev/workpool` is no longer a direct dependency.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
2 participants
Follow-up to #73
You can review as separate commits, main changes are:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.