Skip to content

Filter checkins by date in Mongo instead of in Ruby - #875

Open
suttondemlong wants to merge 1 commit into
rubyforgood:masterfrom
suttondemlong:fix/checkin-index-query
Open

Filter checkins by date in Mongo instead of in Ruby#875
suttondemlong wants to merge 1 commit into
rubyforgood:masterfrom
suttondemlong:fix/checkin-index-query

Conversation

@suttondemlong

Copy link
Copy Markdown
Collaborator

Closes #874.

The bug

CheckinsController#index called .select with a block on the criteria. That is Enumerable#select, not Mongoid::Criteria#select, so it forced the criteria to load: every check-in the user had ever created came back from Mongo, along with the eager loaded associations for all of them, and everything outside the requested day was then thrown away in Ruby.

This is the endpoint the web and native clients hit to render a single day, so the cost grew with account age on one of the hottest paths in the app — and it grew fastest for the longest-tenured users.

The fix

Use the by_date scope that already exists on Checkin over the requested day's bounds, so the query is served by the existing index(date: 1, encrypted_user_id: 1). Both the scope and the index were already there and simply were not being used here.

The resulting query:

{"find"=>"checkins",
 "filter"=>{"encrypted_user_id"=>"…",
            "date"=>{"$gte"=>2016-01-06 00:00:00 UTC,
                     "$lte"=>2016-01-06 23:59:59.999999999 UTC}}}

Why the .to_a

It looks like a no-op and isn't, which is why it carries a comment. Rendering the bare criteria makes AMS enumerate it more than once, re-running both the query and the eager loads. Measured against a user with 60 days of history, counting actual driver commands:

Variant Mongo commands checkins finds Filter in DB? Docs returned
select {} (before) 14 1 no 60
by_date criteria 20 2 yes 1
by_date + to_a 14 1 yes 1

So to_a keeps the command count identical to the original while moving the filter into the database.

Test

The existing specs passed against the buggy code — the output was correct, just expensively obtained — which is how this survived. A behaviour-only test cannot catch it, so spec/support/mongo_commands.rb adds a small driver-monitoring subscriber, and the new example asserts there is exactly one checkins find and that it carries a $gte/$lte date filter. It fails if Ruby-side filtering returns or the to_a is dropped.

Behaviour note

Date.parse used to be called inside the block, so an invalid date param returned 200 [] for a user with no check-ins but raised for everyone else. It now raises consistently (422 in production via rescue_from "Exception"). More consistent, but it is a change, so worth calling out.

Verification

Full backend suite green (316 examples, 0 failures), standardrb clean, erblint --lint-all clean.

One unrelated thing noticed while verifying: spec/models/food_spec.rb:35 is flaky on master (1 in 20 full-suite runs for me). Food.fts orders by ts_rank_cd(...) DESC with no tie-breaker, and the spec asserts an exact order for two rows that rank equal. It passes 25/25 in isolation and only varies in full-suite runs. Not touched here and not caused by this change — measured at 1/20 on master and 0/20 on this branch — but mentioning it in case CI goes red on it.

CheckinsController#index called .select with a block on the criteria,
which is Enumerable#select rather than Mongoid::Criteria#select. That
loaded every checkin the user had ever created, along with the eager
loaded associations for all of them, and then threw away everything
outside the requested day. Cost grew with account age on the endpoint
the clients hit to render a single day.

Use the existing by_date scope over the day's bounds so the query is
served by the existing index(date: 1, encrypted_user_id: 1), and
materialise with to_a so the criteria is not enumerated once per
serializer pass.

Measured with a user holding 60 days of checkins: before, one unfiltered
find returning 60 documents; after, one filtered find returning 1, with
the same total number of Mongo commands.

Fixes rubyforgood#874

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

CheckinsController#index loads a user's entire check-in history to render one day

1 participant