From fd10f12aca7478c4155528ac3142ee4ab70f4d75 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Fri, 16 May 2025 12:42:44 -0400 Subject: [PATCH] Fix issues with provider API --- app/jobs/sync_market_data_job.rb | 8 +- app/models/market_data_syncer.rb | 155 ++++++++++-------- app/models/provider/synth.rb | 6 +- app/models/security/price/syncer.rb | 3 +- app/models/security/provided.rb | 16 +- config/initializers/sidekiq.rb | 12 +- config/schedule.yml | 6 +- .../exchange_rate_provider_interface_test.rb | 2 + .../security_provider_interface_test.rb | 1 + test/models/market_data_syncer_test.rb | 140 ++++++++-------- test/models/security/price/syncer_test.rb | 12 +- 11 files changed, 202 insertions(+), 159 deletions(-) diff --git a/app/jobs/sync_market_data_job.rb b/app/jobs/sync_market_data_job.rb index d7bdc15d..db34a41a 100644 --- a/app/jobs/sync_market_data_job.rb +++ b/app/jobs/sync_market_data_job.rb @@ -10,7 +10,11 @@ class SyncMarketDataJob < ApplicationJob queue_as :scheduled - def perform(mode: :full, clear_cache: false) - MarketDataSyncer.new.sync_all(mode: mode, clear_cache: clear_cache) + def perform(opts) + opts = opts.symbolize_keys + mode = opts.fetch(:mode, :full) + clear_cache = opts.fetch(:clear_cache, false) + + MarketDataSyncer.new(mode: mode, clear_cache: clear_cache).sync end end diff --git a/app/models/market_data_syncer.rb b/app/models/market_data_syncer.rb index 8881880d..70d60b75 100644 --- a/app/models/market_data_syncer.rb +++ b/app/models/market_data_syncer.rb @@ -1,45 +1,51 @@ class MarketDataSyncer - DEFAULT_HISTORY_DAYS = 30 - RATE_PROVIDER_NAME = :synth - PRICE_PROVIDER_NAME = :synth + # By default, our graphs show 1M as the view, so by fetching 31 days, + # we ensure we can always show an accurate default graph + SNAPSHOT_DAYS = 31 - # Syncer can optionally be scoped. Otherwise, it syncs all user data - def initialize(family: nil, account: nil) - @family = family - @account = account + InvalidModeError = Class.new(StandardError) + + def initialize(mode: :full, clear_cache: false) + @mode = set_mode!(mode) + @clear_cache = clear_cache end - def sync_all(full_history: false, clear_cache: false) - sync_exchange_rates(full_history: full_history, clear_cache: clear_cache) - sync_prices(full_history: full_history, clear_cache: clear_cache) + def sync + sync_prices + sync_exchange_rates end - def sync_exchange_rates(full_history: false, clear_cache: false) - unless rate_provider - Rails.logger.warn("No rate provider configured for MarketDataSyncer.sync_exchange_rates, skipping sync") + # Syncs historical security prices (and details) + def sync_prices + unless Security.provider + Rails.logger.warn("No provider configured for MarketDataSyncer.sync_prices, skipping sync") return end - # Finds distinct currency pairs - entry_pairs = entries_scope.joins(:account) - .where.not("entries.currency = accounts.currency") - .select("entries.currency as source, accounts.currency as target") - .distinct + Security.where.not(exchange_operating_mic: nil).find_each do |security| + security.sync_provider_prices( + start_date: get_first_required_price_date(security), + end_date: end_date, + clear_cache: clear_cache + ) - # All accounts in currency not equal to the family currency require exchange rates to show a normalized historical graph - account_pairs = accounts_scope.joins(:family) - .where.not("families.currency = accounts.currency") - .select("accounts.currency as source, families.currency as target") - .distinct + security.sync_provider_details(clear_cache: clear_cache) + end + end - pairs = (entry_pairs + account_pairs).uniq + def sync_exchange_rates + unless ExchangeRate.provider + Rails.logger.warn("No provider configured for MarketDataSyncer.sync_exchange_rates, skipping sync") + return + end - pairs.each do |pair| - start_date = full_history ? find_oldest_required_rate(from_currency: pair.source) : default_start_date + required_exchange_rate_pairs.each do |pair| + # pair is a Hash with keys :source, :target, and :start_date + start_date = snapshot? ? default_start_date : pair[:start_date] ExchangeRate.sync_provider_rates( - from: pair.source, - to: pair.target, + from: pair[:source], + to: pair[:target], start_date: start_date, end_date: end_date, clear_cache: clear_cache @@ -47,61 +53,80 @@ class MarketDataSyncer end end - def sync_prices(full_history: false, clear_cache: false) - unless price_provider - Rails.logger.warn("No price provider configured for MarketDataSyncer.sync_prices, skipping sync") - nil - end - - securities_scope.each do |security| - start_date = full_history ? find_oldest_required_price(security: security) : default_start_date - - security.sync_provider_prices(start_date: start_date, end_date: end_date, clear_cache: clear_cache) - security.sync_provider_details(clear_cache: clear_cache) - end - end - private - attr_reader :family, :account + attr_reader :mode, :clear_cache - def accounts_scope - return Account.where(id: account.id) if account - return family.accounts if family - Account.all + def snapshot? + mode.to_sym == :snapshot end - def entries_scope - account&.entries || family&.entries || Entry.all - end + # Builds a unique list of currency pairs with the earliest date we need + # exchange rates for. + # + # Returns: Array of Hashes – [{ source:, target:, start_date: }, ...] + def required_exchange_rate_pairs + pair_dates = {} # { [source, target] => earliest_date } - def securities_scope - if account - account.trades.joins(:security).where.not(securities: { exchange_operating_mic: nil }) - elsif family - family.trades.joins(:security).where.not(securities: { exchange_operating_mic: nil }) - else - Security.where.not(exchange_operating_mic: nil) + # 1. ENTRY-BASED PAIRS – we need rates from the first entry date + Entry.joins(:account) + .where.not("entries.currency = accounts.currency") + .group("entries.currency", "accounts.currency") + .minimum("entries.date") + .each do |(source, target), date| + key = [ source, target ] + pair_dates[key] = [ pair_dates[key], date ].compact.min + end + + # 2. ACCOUNT-BASED PAIRS – use the account's oldest entry date + account_first_entry_dates = Entry.group(:account_id).minimum(:date) + + Account.joins(:family) + .where.not("families.currency = accounts.currency") + .select("accounts.id, accounts.currency AS source, families.currency AS target") + .find_each do |account| + earliest_entry_date = account_first_entry_dates[account.id] + + chosen_date = [ earliest_entry_date, default_start_date ].compact.min + + key = [ account.source, account.target ] + pair_dates[key] = [ pair_dates[key], chosen_date ].compact.min + end + + # Convert to array of hashes for ease of use + pair_dates.map do |(source, target), date| + { source: source, target: target, start_date: date } end end - def rate_provider - Provider::Registry.for_concept(:exchange_rates).get_provider(RATE_PROVIDER_NAME) + def get_first_required_price_date(security) + return default_start_date if snapshot? + + Trade.with_entry.where(security: security).minimum(:date) end - def price_provider - Provider::Registry.for_concept(:securities).get_provider(PRICE_PROVIDER_NAME) - end + # An approximation that grabs more than we likely need, but simplifies the logic + def get_first_required_exchange_rate_date(from_currency:) + return default_start_date if snapshot? - def find_oldest_required_rate(from_currency:) - entries_scope.where(currency: from_currency).minimum(:date) || default_start_date + Entry.where(currency: from_currency).minimum(:date) end def default_start_date - DEFAULT_HISTORY_DAYS.days.ago.to_date + SNAPSHOT_DAYS.days.ago.to_date end # Since we're querying market data from a US-based API, end date should always be today (EST) def end_date Date.current.in_time_zone("America/New_York").to_date end + + def set_mode!(mode) + valid_modes = [ :full, :snapshot ] + + unless valid_modes.include?(mode.to_sym) + raise InvalidModeError, "Invalid mode for MarketDataSyncer, can only be :full or :snapshot, but was #{mode}" + end + + mode.to_sym + end end diff --git a/app/models/provider/synth.rb b/app/models/provider/synth.rb index be35b6f8..fee3a236 100644 --- a/app/models/provider/synth.rb +++ b/app/models/provider/synth.rb @@ -50,7 +50,7 @@ class Provider::Synth < Provider rates = JSON.parse(response.body).dig("data", "rates") - Rate.new(date:, from:, to:, rate: rates.dig(to)) + Rate.new(date: date.to_date, from:, to:, rate: rates.dig(to)) end end @@ -77,7 +77,7 @@ class Provider::Synth < Provider next end - Rate.new(date:, from:, to:, rate:) + Rate.new(date: date.to_date, from:, to:, rate:) end.compact end end @@ -170,7 +170,7 @@ class Provider::Synth < Provider Price.new( symbol: symbol, - date: date, + date: date.to_date, price: price, currency: currency, exchange_operating_mic: exchange_operating_mic diff --git a/app/models/security/price/syncer.rb b/app/models/security/price/syncer.rb index f1aae018..dbdf0831 100644 --- a/app/models/security/price/syncer.rb +++ b/app/models/security/price/syncer.rb @@ -66,7 +66,8 @@ class Security::Price::Syncer provider_fetch_start_date = effective_start_date - 5.days response = security_provider.fetch_security_prices( - security, + symbol: security.ticker, + exchange_operating_mic: security.exchange_operating_mic, start_date: provider_fetch_start_date, end_date: end_date ) diff --git a/app/models/security/provided.rb b/app/models/security/provided.rb index 5c573161..2214ccfa 100644 --- a/app/models/security/provided.rb +++ b/app/models/security/provided.rb @@ -59,15 +59,21 @@ module Security::Provided return end - details = provider.fetch_security_info( + response = provider.fetch_security_info( symbol: ticker, exchange_operating_mic: exchange_operating_mic ) - update( - name: details.name, - logo_url: details.logo_url, - ) + if response.success? + update( + name: response.data.name, + logo_url: response.data.logo_url, + ) + else + err = StandardError.new("Failed to fetch security info for #{ticker} from #{provider.class.name}: #{response.error.message}") + Rails.logger.warn(err.message) + Sentry.capture_exception(err, level: :warning) + end end def sync_provider_prices(start_date:, end_date:, clear_cache: false) diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index 9040f864..70a6e476 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -1,11 +1,13 @@ require "sidekiq/web" -Sidekiq::Web.use(Rack::Auth::Basic) do |username, password| - configured_username = ::Digest::SHA256.hexdigest(ENV.fetch("SIDEKIQ_WEB_USERNAME", "maybe")) - configured_password = ::Digest::SHA256.hexdigest(ENV.fetch("SIDEKIQ_WEB_PASSWORD", "maybe")) +if Rails.env.production? + Sidekiq::Web.use(Rack::Auth::Basic) do |username, password| + configured_username = ::Digest::SHA256.hexdigest(ENV.fetch("SIDEKIQ_WEB_USERNAME", "maybe")) + configured_password = ::Digest::SHA256.hexdigest(ENV.fetch("SIDEKIQ_WEB_PASSWORD", "maybe")) - ActiveSupport::SecurityUtils.secure_compare(::Digest::SHA256.hexdigest(username), configured_username) && - ActiveSupport::SecurityUtils.secure_compare(::Digest::SHA256.hexdigest(password), configured_password) + ActiveSupport::SecurityUtils.secure_compare(::Digest::SHA256.hexdigest(username), configured_username) && + ActiveSupport::SecurityUtils.secure_compare(::Digest::SHA256.hexdigest(password), configured_password) + end end Sidekiq::Cron.configure do |config| diff --git a/config/schedule.yml b/config/schedule.yml index 6e0c4f52..04335fb0 100644 --- a/config/schedule.yml +++ b/config/schedule.yml @@ -1,5 +1,9 @@ sync_market_data: - cron: "0 22 * * 1-5" # 5:00 PM EST / 6:00 PM EDT (NY time) + # cron: "0 22 * * 1-5" # 5:00 PM EST / 6:00 PM EDT (NY time) + cron: "* * * * *" # every minute class: "SyncMarketDataJob" queue: "scheduled" description: "Syncs market data daily at 5:00 PM EST (1 hour after market close)" + args: + mode: "full" + clear_cache: false diff --git a/test/interfaces/exchange_rate_provider_interface_test.rb b/test/interfaces/exchange_rate_provider_interface_test.rb index 9293c4d9..3716f6fc 100644 --- a/test/interfaces/exchange_rate_provider_interface_test.rb +++ b/test/interfaces/exchange_rate_provider_interface_test.rb @@ -15,6 +15,7 @@ module ExchangeRateProviderInterfaceTest assert_equal "USD", rate.from assert_equal "GBP", rate.to + assert rate.date.is_a?(Date) assert_in_delta 0.78, rate.rate, 0.01 end end @@ -26,6 +27,7 @@ module ExchangeRateProviderInterfaceTest ) assert_equal 213, response.data.count # 213 days between 01.01.2024 and 31.07.2024 + assert response.data.first.date.is_a?(Date) end end diff --git a/test/interfaces/security_provider_interface_test.rb b/test/interfaces/security_provider_interface_test.rb index 56bf88b0..a994bb73 100644 --- a/test/interfaces/security_provider_interface_test.rb +++ b/test/interfaces/security_provider_interface_test.rb @@ -26,6 +26,7 @@ module SecurityProviderInterfaceTest ) assert response.success? + assert response.data.first.date.is_a?(Date) assert_equal 147, response.data.count # Synth won't return prices on weekends / holidays, so less than total day count of 213 end end diff --git a/test/models/market_data_syncer_test.rb b/test/models/market_data_syncer_test.rb index bd2142e6..8a9db1f5 100644 --- a/test/models/market_data_syncer_test.rb +++ b/test/models/market_data_syncer_test.rb @@ -2,90 +2,84 @@ require "test_helper" require "ostruct" class MarketDataSyncerTest < ActiveSupport::TestCase - include EntriesTestHelper, ProviderTestHelper + include ProviderTestHelper - test "syncs exchange rates with upsert" do - empty_db + SNAPSHOT_START_DATE = MarketDataSyncer::SNAPSHOT_DAYS.days.ago.to_date + PROVIDER_BUFFER = 5.days - family1 = Family.create!(name: "Family 1", currency: "USD") - account1 = family1.accounts.create!(name: "Account 1", currency: "USD", balance: 100, accountable: Depository.new) - account2 = family1.accounts.create!(name: "Account 2", currency: "CAD", balance: 100, accountable: Depository.new) + setup do + Security::Price.delete_all + ExchangeRate.delete_all + Trade.delete_all + Holding.delete_all + Security.delete_all - family2 = Family.create!(name: "Family 2", currency: "EUR") - account3 = family2.accounts.create!(name: "Account 3", currency: "EUR", balance: 100, accountable: Depository.new) - account4 = family2.accounts.create!(name: "Account 4", currency: "USD", balance: 100, accountable: Depository.new) - - mock_provider = mock - Provider::Registry.any_instance.expects(:get_provider).with(:synth).returns(mock_provider).at_least_once - - start_date = 1.month.ago.to_date - end_date = Date.current.in_time_zone("America/New_York").to_date - - # Put an existing rate in DB to test upsert - ExchangeRate.create!(from_currency: "CAD", to_currency: "USD", date: start_date, rate: 2.0) - - # The individual syncers fetch with a 5-day buffer to ensure we have a «starting» price/rate - provider_start_date = get_provider_fetch_start_date(start_date) - provider_start_date_for_cad_usd = get_provider_fetch_start_date(start_date + 1.day) # first missing date is +1 day - - mock_provider.expects(:fetch_exchange_rates) - .with(from: "CAD", to: "USD", start_date: provider_start_date_for_cad_usd, end_date: end_date) - .returns(provider_success_response([ OpenStruct.new(from: "CAD", to: "USD", date: start_date, rate: 1.0) ])) - - mock_provider.expects(:fetch_exchange_rates) - .with(from: "USD", to: "EUR", start_date: provider_start_date, end_date: end_date) - .returns(provider_success_response([ OpenStruct.new(from: "USD", to: "EUR", date: start_date, rate: 1.0) ])) - - before_count = ExchangeRate.count - MarketDataSyncer.new.sync_exchange_rates - after_count = ExchangeRate.count - - assert_operator after_count, :>, before_count, "Expected at least one new exchange-rate row to be inserted" - - # The original CAD→USD rate on start_date should remain (no clear_cache), so value stays 2.0 - assert_equal 2.0, ExchangeRate.where(from_currency: "CAD", to_currency: "USD", date: start_date).first.rate + @provider = mock("provider") + Provider::Registry.any_instance + .stubs(:get_provider) + .with(:synth) + .returns(@provider) end - test "syncs security prices with upsert" do - empty_db + test "syncs required exchange rates" do + family = Family.create!(name: "Smith", currency: "USD") + family.accounts.create!(name: "Chequing", + currency: "CAD", + balance: 100, + accountable: Depository.new) - aapl = Security.create!(ticker: "AAPL", exchange_operating_mic: "XNAS") + # Seed stale rate so only the next missing day is fetched + ExchangeRate.create!(from_currency: "CAD", + to_currency: "USD", + date: SNAPSHOT_START_DATE, + rate: 2.0) - family = Family.create!(name: "Family 1", currency: "USD") - account = family.accounts.create!(name: "Account 1", currency: "USD", balance: 100, accountable: Investment.new) + expected_start_date = (SNAPSHOT_START_DATE + 1.day) - PROVIDER_BUFFER + end_date = Date.current.in_time_zone("America/New_York").to_date - mock_provider = mock - Provider::Registry.any_instance.expects(:get_provider).with(:synth).returns(mock_provider).at_least_once + @provider.expects(:fetch_exchange_rates) + .with(from: "CAD", + to: "USD", + start_date: expected_start_date, + end_date: end_date) + .returns(provider_success_response([ + OpenStruct.new(from: "CAD", to: "USD", date: SNAPSHOT_START_DATE, rate: 1.5) + ])) - start_date = 1.month.ago.to_date - end_date = Date.current.in_time_zone("America/New_York").to_date + before = ExchangeRate.count + MarketDataSyncer.new(mode: :snapshot).sync_exchange_rates + after = ExchangeRate.count - # The individual syncers fetch with a 5-day buffer to ensure we have a «starting» price/rate - provider_start_date = get_provider_fetch_start_date(start_date) - - mock_provider.expects(:fetch_security_prices) - .with(aapl, start_date: provider_start_date, end_date: end_date) - .returns(provider_success_response([ OpenStruct.new(security: aapl, date: start_date, price: 100, currency: "USD") ])) - - # The syncer also enriches security details, so stub that out as well - mock_provider.stubs(:fetch_security_info) - .with(symbol: "AAPL", exchange_operating_mic: "XNAS") - .returns(OpenStruct.new(name: "Apple", logo_url: "logo")) - - MarketDataSyncer.new.sync_prices - - assert_equal 1, Security::Price.where(security: aapl, date: start_date).count + assert_operator after, :>, before, "Should insert at least one new exchange-rate row" end - private - def empty_db - Invitation.destroy_all - Family.destroy_all - Security.destroy_all - end + test "syncs security prices" do + security = Security.create!(ticker: "AAPL", exchange_operating_mic: "XNAS") - # Match the internal syncer logic of adding a 5-day buffer before provider calls - def get_provider_fetch_start_date(start_date) - start_date - 5.days - end + expected_start_date = SNAPSHOT_START_DATE - PROVIDER_BUFFER + end_date = Date.current.in_time_zone("America/New_York").to_date + + @provider.expects(:fetch_security_prices) + .with(symbol: security.ticker, + exchange_operating_mic: security.exchange_operating_mic, + start_date: expected_start_date, + end_date: end_date) + .returns(provider_success_response([ + OpenStruct.new(security: security, + date: SNAPSHOT_START_DATE, + price: 100, + currency: "USD") + ])) + + @provider.stubs(:fetch_security_info) + .with(symbol: "AAPL", exchange_operating_mic: "XNAS") + .returns(provider_success_response(OpenStruct.new(name: "Apple", logo_url: "logo"))) + + # Ignore exchange rate calls for this test + @provider.stubs(:fetch_exchange_rates).returns(provider_success_response([])) + + MarketDataSyncer.new(mode: :snapshot).sync_prices + + assert_equal 1, Security::Price.where(security: security, date: SNAPSHOT_START_DATE).count + end end diff --git a/test/models/security/price/syncer_test.rb b/test/models/security/price/syncer_test.rb index ff5610d0..25a3f14c 100644 --- a/test/models/security/price/syncer_test.rb +++ b/test/models/security/price/syncer_test.rb @@ -19,7 +19,8 @@ class Security::Price::SyncerTest < ActiveSupport::TestCase ]) @provider.expects(:fetch_security_prices) - .with(@security, start_date: get_provider_fetch_start_date(2.days.ago.to_date), end_date: Date.current) + .with(symbol: @security.ticker, exchange_operating_mic: @security.exchange_operating_mic, + start_date: get_provider_fetch_start_date(2.days.ago.to_date), end_date: Date.current) .returns(provider_response) Security::Price::Syncer.new( @@ -47,7 +48,8 @@ class Security::Price::SyncerTest < ActiveSupport::TestCase ]) @provider.expects(:fetch_security_prices) - .with(@security, start_date: get_provider_fetch_start_date(1.day.ago.to_date), end_date: Date.current) + .with(symbol: @security.ticker, exchange_operating_mic: @security.exchange_operating_mic, + start_date: get_provider_fetch_start_date(1.day.ago.to_date), end_date: Date.current) .returns(provider_response) Security::Price::Syncer.new( @@ -94,7 +96,8 @@ class Security::Price::SyncerTest < ActiveSupport::TestCase ]) @provider.expects(:fetch_security_prices) - .with(@security, start_date: get_provider_fetch_start_date(2.days.ago.to_date), end_date: Date.current) + .with(symbol: @security.ticker, exchange_operating_mic: @security.exchange_operating_mic, + start_date: get_provider_fetch_start_date(2.days.ago.to_date), end_date: Date.current) .returns(provider_response) Security::Price::Syncer.new( @@ -119,7 +122,8 @@ class Security::Price::SyncerTest < ActiveSupport::TestCase ]) @provider.expects(:fetch_security_prices) - .with(@security, start_date: get_provider_fetch_start_date(Date.current), end_date: Date.current) + .with(symbol: @security.ticker, exchange_operating_mic: @security.exchange_operating_mic, + start_date: get_provider_fetch_start_date(Date.current), end_date: Date.current) .returns(provider_response) Security::Price::Syncer.new(