feat(occurrences): Preprocess redirects#109616
Conversation
github-actions
bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| # Only need to update in the "new matches found" case. | ||
| if len(all_group_ids) > len(group_ids): | ||
| pieces.append(f"group_id:[{', '.join(str(gid) for gid in all_group_ids)}]") |
There was a problem hiding this comment.
Non-deterministic set iteration produces unstable query output
Medium Severity
get_all_merged_group_ids returns a set, and iterating over it with ', '.join(str(gid) for gid in all_group_ids) produces group IDs in a non-deterministic order. The tests assert exact string equality (e.g., == "group_id:[1, 2, 3]"), which only passes due to CPython's implementation-specific hash ordering for small integers. With real (large) group IDs, the output order will vary across runs, making behavior unpredictable and tests flaky.
Additional Locations (1)
There was a problem hiding this comment.
Query behavior should not depend on array ordering and the tests can keep using small numbers.
We do not update EAP in the event of a group merge. Instead, we track the merge in our "Group Redirect" table. This PR preprocesses EAP queries to account for the redirects, so users can always query and get all their relevant groups.
thetruecpaul
force-pushed
the
cpaul/eap_occurrences/redirect_preprocess
branch
from
908fc04 to
b9b1950
Compare
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.
| all_group_ids = get_all_merged_group_ids({int(gid) for gid in group_ids}) | ||
|
|
||
| # Only need to update in the "new matches found" case. | ||
| if len(all_group_ids) > len(group_ids): |
There was a problem hiding this comment.
List-vs-set length comparison can skip needed updates
Low Severity
The guard len(all_group_ids) > len(group_ids) compares the returned set length against the length of the original string list. Since the input to get_all_merged_group_ids is deduplicated via a set comprehension, if the original list had duplicates, len(group_ids) (list) can exceed len(all_group_ids) (set) even when new merged IDs were found, causing the update to be incorrectly skipped.
There was a problem hiding this comment.
There shouldn't be duplicates in the passed-in input.
1 participant
We do not update EAP in the event of a group merge. Instead, we track the merge in our "Group Redirect" table.
This PR preprocesses EAP queries to account for the redirects, so users can always query and get all their relevant groups.