Show actual observation dates on /v2/rates (#338)#339
Conversation
Broadens #338 from "latest path only" to all read paths. Range queries no longer carry forward — each pair appears on the days it actually published, with its own date. Pairs that did not publish during the range are absent rather than projected. CarryForward is now used only on the latest/single-date path. The module becomes a class: CarryForward.apply(rows, date: Date.today) Public attrs (rows, date, lookback) plus private helpers (cutoff, eligible_rows, key_for) that the apply method composes. The old range-based enrich method and RANGE_LOOKBACK_DAYS constant are gone; LATEST_LOOKBACK_DAYS becomes LOOKBACK_DAYS. Mixed-cadence providers (e.g. weekly publishers) now appear at their actual publication frequency rather than being projected daily.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates /v2/rates to report per-row observation dates (no more flattened batch date) and removes carry-forward behavior from range queries, while refactoring carry-forward into a single entry-point API.
Changes:
- Range queries now return only rates that actually published on each day (no carry-forward enrichment).
- Latest/single-date queries now stamp each row with its own observation date.
- Refactors
CarryForwardtoCarryForward.apply(rows, date:)and updates callers/tests accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/versions/v2/rate_query_spec.rb | Adds coverage for per-row dates on latest path and no carry-forward in range queries. |
| spec/rate_spec.rb | Updates tests to use CarryForward.apply. |
| spec/carry_forward_spec.rb | Renames API usage to .apply and removes range-enrichment tests. |
| lib/versions/v2/rate_query.rb | Removes range carry-forward and emits per-row observation dates. |
| lib/versions/v1/quote/end_of_day.rb | Updates v1 quote endpoint to use CarryForward.apply. |
| lib/versions/v1/currency_names.rb | Updates v1 currency names query to use CarryForward.apply. |
| lib/carry_forward.rb | Refactors carry-forward from module functions into a class with .apply. |
| CHANGELOG.md | Documents the API-visible change in observation date semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def apply | ||
| best = {} | ||
| eligible_rows.each do |row| | ||
| key = key_for(row) | ||
| best[key] = row if !best[key] || row[:date] > best[key][:date] | ||
| end | ||
|
|
||
| # Enriches each date in the target range with carried-forward rates. Returns { date => [rows] } | ||
| # where each date's rows include both same-day rates and each provider's most recent rate within | ||
| # the lookback window. Carried-forward rows keep their original dates so WeightedAverage can | ||
| # discount them by staleness. | ||
| def enrich(rows, range:, lookback: RANGE_LOOKBACK_DAYS) | ||
| by_date = rows.group_by { |r| r[:date] } | ||
| target_dates = by_date.keys.select { |d| range.cover?(d) }.sort | ||
|
|
||
| index = {} | ||
| rows.each do |row| | ||
| key = [row[:provider], row[:base], row[:quote]] | ||
| (index[key] ||= []) << row | ||
| end | ||
| index.each_value { |v| v.sort_by! { |r| r[:date] }.reverse! } | ||
|
|
||
| target_dates.to_h do |date| | ||
| cutoff = date - lookback | ||
| group = index.filter_map do |_, dated_rows| | ||
| dated_rows.find { |r| r[:date].between?(cutoff, date) } | ||
| end | ||
| [date, group] | ||
| end | ||
| end | ||
| best.values | ||
| end |
There was a problem hiding this comment.
eligible_rows allocates an intermediate array via select, which is avoidable since apply already iterates. You can iterate rows once inside apply and next rows outside the window; this reduces memory churn and speeds up processing for larger row sets.
There was a problem hiding this comment.
Deferring — the helper-method structure (with cutoff, eligible_rows, key_for) is intentional for readability. Row counts here are bounded by LOOKBACK_DAYS × providers (low thousands at most), so the extra allocation is negligible.
| def eligible_rows | ||
| rows.select { |r| r[:date].between?(cutoff, date) } | ||
| end |
There was a problem hiding this comment.
eligible_rows allocates an intermediate array via select, which is avoidable since apply already iterates. You can iterate rows once inside apply and next rows outside the window; this reduces memory churn and speeds up processing for larger row sets.
There was a problem hiding this comment.
Same as above — keeping eligible_rows as a named helper.
- rate_spec: deterministic max-date assertion instead of sample - rate_query_spec: filter RON rows by base+quote to guard against fixture changes
Closes #338.
Broadens the per-pair observation date fix from "latest path only" to all read paths. Range queries no longer carry forward — each pair appears on the days it actually published, with its own date.
Summary
CarryForwardrefactored to a class withapply(rows, date:)as the entry point. Used only on the latest/single-date path now. The oldenrichmethod andRANGE_LOOKBACK_DAYSconstant are gone;LATEST_LOOKBACK_DAYSbecomesLOOKBACK_DAYS.Editorial principle
Continues "surface uncertainty, don't impose judgement." Previously the response date erased staleness — a Friday rate carried into a Tuesday response was stamped Tuesday. Consumers had no signal that the data was three days old. Now each row reports when it was actually observed.
API impact
Visible change for clients of
/v2/rates:OpenAPI spec is unchanged — the schema doesn't promise uniform dates, and rows still have the same shape.
Test plan
bundle exec rakepasses (598 runs, 0 failures, no offenses)