perf(workflows): Batch Action fetching in WorkflowEngineRuleSerializer#111945
Conversation
github-actions
Bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
saponifi3d
left a comment
saponifi3d
left a comment
There was a problem hiding this comment.
lgtm - might be also nice to include a test to illustrate the reduction in queries and prevent it from going back up.
| .exclude( | ||
| action__status__in=[ | ||
| ObjectStatus.DELETION_IN_PROGRESS, | ||
| ObjectStatus.PENDING_DELETION, | ||
| ], | ||
| ) |
There was a problem hiding this comment.
for a future PR... should we update the DataConditionGroupAction (and probably most of the models) to have a manager that will not the pending / deleted objects? (I think we did this for some of the models, but can't remember).
|
I feel ok about not adding a query counting test here, just because it is a design bug when we do O(N) queries in get_attr (or serialize) anyway, so it does require a human mistake (or strategic choice) to cause this sort of problem, and we have detectors that flag N+1s (this one was flagged), so the risk of doing it accidentally and not noticing isn't huge. But also, query counting tests can be a bit brittle. This is maybe the 4th N+1 querying pattern I've fixed here this week, though, so maybe there's something to be said for trying to have serializer tests that count queries for N items, then N*2, and check that the query count hasn't gone up significantly.. Added ISWF-2360 to track. |
2 participants
Fixes SENTRY-5MD5.