Skip to content

Update disavowed event lookup to load one record#10372

Merged
aduth merged 2 commits intomainfrom
aduth-event-find-one
Apr 5, 2024
Merged

Update disavowed event lookup to load one record#10372
aduth merged 2 commits intomainfrom
aduth-event-find-one

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Apr 5, 2024

🛠 Summary of changes

Updates EventDisavowal::FindDisavowedEvent#event to avoid loading all events matching token fingerprints.

This is the result of a discussion with @jmhooper stemming from surprise around performance characteristics noted in #2855. While #first would still be advisable to avoid here due to built-in ordering ("if no order is defined it will order by primary key"), #find_by will locate at most one record without imposing any order. #take would have also worked.

The previous implementation was likely fine enough since the logic producing fingerprints would likely only ever produce 0 or 1 database results, but these revisions are simpler and more semantic, and should hopefully avoid future confusion around unoptimized LIMIT 1 queries.

📜 Testing Plan

Existing tests provide coverage for (a) missing, (b) matched, (c) matched old key. Verify they pass:

rspec spec/services/event_disavowal/find_disavowed_event_spec.rb

changelog: Internal, Database, Optimize event disavowal query to load single record into memory
@aduth aduth requested a review from jmhooper April 5, 2024 15:29
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@aduth aduth merged commit df546b6 into main Apr 5, 2024
@aduth aduth deleted the aduth-event-find-one branch April 5, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants