diff --git a/app/models/market_data_importer.rb b/app/models/market_data_importer.rb index 9eaf5964..d0ef723f 100644 --- a/app/models/market_data_importer.rb +++ b/app/models/market_data_importer.rb @@ -22,7 +22,8 @@ class MarketDataImporter return end - Security.where.not(exchange_operating_mic: nil).find_each do |security| + # Import all securities that aren't marked as "offline" (i.e. they're available from the provider) + Security.where.not(offline: true).find_each do |security| security.import_provider_prices( start_date: get_first_required_price_date(security), end_date: end_date, diff --git a/app/models/provider/synth.rb b/app/models/provider/synth.rb index 8d76fc72..1723da48 100644 --- a/app/models/provider/synth.rb +++ b/app/models/provider/synth.rb @@ -94,7 +94,8 @@ class Provider::Synth < Provider req.params["name"] = symbol req.params["dataset"] = "limited" req.params["country_code"] = country_code if country_code.present? - req.params["exchange_operating_mic"] = exchange_operating_mic if exchange_operating_mic.present? + # Synth uses mic_code, which encompasses both exchange_mic AND exchange_operating_mic (union) + req.params["mic_code"] = exchange_operating_mic if exchange_operating_mic.present? req.params["limit"] = 25 end @@ -131,7 +132,7 @@ class Provider::Synth < Provider end end - def fetch_security_price(symbol:, exchange_operating_mic:, date:) + def fetch_security_price(symbol:, exchange_operating_mic: nil, date:) with_provider_response do historical_data = fetch_security_prices(symbol:, exchange_operating_mic:, start_date: date, end_date: date) @@ -141,13 +142,13 @@ class Provider::Synth < Provider end end - def fetch_security_prices(symbol:, exchange_operating_mic:, start_date:, end_date:) + def fetch_security_prices(symbol:, exchange_operating_mic: nil, start_date:, end_date:) with_provider_response do params = { start_date: start_date, end_date: end_date, operating_mic_code: exchange_operating_mic - } + }.compact data = paginate( "#{base_url}/tickers/#{symbol}/open-close", diff --git a/app/models/security.rb b/app/models/security.rb index 6115aa4c..ad28c61c 100644 --- a/app/models/security.rb +++ b/app/models/security.rb @@ -24,10 +24,6 @@ class Security < ApplicationRecord ) end - def has_prices? - exchange_operating_mic.present? - end - private def upcase_symbols self.ticker = ticker.upcase diff --git a/app/models/security/health_checker.rb b/app/models/security/health_checker.rb index 7fb090c6..16778fda 100644 --- a/app/models/security/health_checker.rb +++ b/app/models/security/health_checker.rb @@ -2,21 +2,28 @@ # Due to the always-changing nature of the market, the health checker is responsible # for periodically checking active securities to ensure we can still fetch prices for them. # -# Securities that cannot fetch prices are marked "offline" and will not run price updates. +# Each security goes through some basic health checks. If failed, this class is responsible for: +# - Marking failed attempts and incrementing the failed attempts counter +# - Marking the security offline if enough consecutive failed checks occur +# - When we move a security "offline", delete all prices for that security as we assume they are bad data # -# The health checker is run daily through SecurityHealthCheckJob (see config/schedule.yml) +# The health checker is run daily through SecurityHealthCheckJob (see config/schedule.yml), but not all +# securities will be checked every day (we run in batches) class Security::HealthChecker - HEALTH_CHECK_INTERVAL = 30.days + MAX_CONSECUTIVE_FAILURES = 5 + HEALTH_CHECK_INTERVAL = 7.days DAILY_BATCH_SIZE = 1000 + class << self def check_all - # All securities that have never been checked run, regardless of daily batch size + # No daily limit for unchecked securities (they are prioritized) never_checked_scope.find_each do |security| new(security).run_check end - due_for_check_scope.find_each do |security| + # Daily limit for checked securities + due_for_check_scope.limit(DAILY_BATCH_SIZE).find_each do |security| new(security).run_check end end @@ -33,7 +40,6 @@ class Security::HealthChecker def due_for_check_scope Security.where(last_health_check_at: ..HEALTH_CHECK_INTERVAL.ago) .order(last_health_check_at: :asc) - .limit(DAILY_BATCH_SIZE) end end @@ -42,17 +48,70 @@ class Security::HealthChecker end def run_check + if latest_provider_price + handle_success + else + handle_failure + end + rescue => e + Sentry.capture_exception(e) do |scope| + scope.set_tags(security_id: @security.id) + end + ensure + security.update!(last_health_check_at: Time.current) end private - def scope - Security.where(last_health_check_at: nil) - .or(Security.where(last_health_check_at: ..7.days.ago)) + attr_reader :security + + def provider + Security.provider end - def can_fetch_from_provider? + def latest_provider_price + response = provider.fetch_security_price( + symbol: security.ticker, + exchange_operating_mic: security.exchange_operating_mic, + date: Date.current + ) + + return nil unless response.success? + + response.data.price end - def has_daily_prices? + # On success, reset any failure counters and ensure it is "online" + def handle_success + security.update!( + offline: false, + failed_fetch_count: 0, + failed_fetch_at: nil + ) + end + + def handle_failure + new_failure_count = security.failed_fetch_count.to_i + 1 + new_failure_at = Time.current + + if new_failure_count > MAX_CONSECUTIVE_FAILURES + convert_to_offline_security! + else + security.update!( + failed_fetch_count: new_failure_count, + failed_fetch_at: new_failure_at + ) + end + end + + # The "offline" state tells our MarketDataImporter (daily cron) to skip this security when fetching prices + def convert_to_offline_security! + Security.transaction do + security.update!( + offline: true, + failed_fetch_count: MAX_CONSECUTIVE_FAILURES + 1, + failed_fetch_at: Time.current + ) + security.prices.delete_all + end end end diff --git a/app/models/security/provided.rb b/app/models/security/provided.rb index 7927d6e6..6a264081 100644 --- a/app/models/security/provided.rb +++ b/app/models/security/provided.rb @@ -10,7 +10,7 @@ module Security::Provided end def search_provider(symbol, country_code: nil, exchange_operating_mic: nil) - return [] if symbol.blank? || symbol.length < 2 + return [] if provider.nil? || symbol.blank? response = provider.search_securities(symbol, country_code: country_code, exchange_operating_mic: exchange_operating_mic) @@ -37,7 +37,11 @@ module Security::Provided # Make sure we have a data provider before fetching return nil unless provider.present? - response = provider.fetch_security_price(self, date: date) + response = provider.fetch_security_price( + symbol: ticker, + exchange_operating_mic: exchange_operating_mic, + date: date + ) return nil unless response.success? # Provider error diff --git a/app/models/security/resolver.rb b/app/models/security/resolver.rb index 31c3f40b..295dccec 100644 --- a/app/models/security/resolver.rb +++ b/app/models/security/resolver.rb @@ -5,13 +5,146 @@ class Security::Resolver @country_code = country_code end + # Attempts several paths to resolve a security: + # 1. Exact match in DB + # 2. Search provider for an exact match + # 3. Search provider for close match, ranked by relevance + # 4. Create offline security if no match is found in either DB or provider def resolve + return nil if symbol.blank? + + exact_match_from_db || + exact_match_from_provider || + close_match_from_provider || + offline_security end private attr_reader :symbol, :exchange_operating_mic, :country_code - def provider - Security.provider + def offline_security + security = Security.find_or_initialize_by( + ticker: symbol, + exchange_operating_mic: exchange_operating_mic, + ) + + security.assign_attributes( + country_code: country_code, + offline: true # This tells us that we shouldn't try to fetch prices later + ) + + security.save! + + security + end + + def exact_match_from_db + Security.find_by( + { + ticker: symbol, + exchange_operating_mic: exchange_operating_mic, + country_code: country_code.presence + }.compact + ) + end + + # If provided a ticker + exchange (and optionally, a country code), we can find exact matches + def exact_match_from_provider + # Without an exchange, we can never know if we have an exact match + return nil unless exchange_operating_mic.present? + + match = provider_search_result.find do |s| + ticker_matches = s.ticker.upcase.to_s == symbol.upcase.to_s + exchange_matches = s.exchange_operating_mic.upcase.to_s == exchange_operating_mic.upcase.to_s + + if country_code && exchange_operating_mic + ticker_matches && exchange_matches && s.country_code.upcase.to_s == country_code.upcase.to_s + else + ticker_matches && exchange_matches + end + end + + return nil unless match + + find_or_create_provider_match!(match) + end + + def close_match_from_provider + filtered_candidates = provider_search_result + + # If a country code is specified, we MUST find a match with the same code + if country_code.present? + filtered_candidates = filtered_candidates.select { |s| s.country_code.upcase.to_s == country_code.upcase.to_s } + end + + # 1. Prefer exact exchange_operating_mic matches (if one was provided) + # 2. Rank by country relevance (lower index in the list is more relevant) + # 3. Rank by exchange_operating_mic relevance (lower index in the list is more relevant) + sorted_candidates = filtered_candidates.sort_by do |s| + [ + exchange_operating_mic.present? && s.exchange_operating_mic.upcase.to_s == exchange_operating_mic.upcase.to_s ? 0 : 1, + sorted_country_codes_by_relevance.index(s.country_code&.upcase.to_s) || sorted_country_codes_by_relevance.length, + sorted_exchange_operating_mics_by_relevance.index(s.exchange_operating_mic&.upcase.to_s) || sorted_exchange_operating_mics_by_relevance.length + ] + end + + match = sorted_candidates.first + + return nil unless match + + find_or_create_provider_match!(match) + end + + def find_or_create_provider_match!(match) + security = Security.find_or_initialize_by( + ticker: match.ticker, + exchange_operating_mic: match.exchange_operating_mic, + ) + + security.country_code = match.country_code + security.save! + + security + end + + def provider_search_result + @provider_search_result ||= Security.search_provider( + symbol, + exchange_operating_mic: exchange_operating_mic, + country_code: country_code + ) + end + + # Non-exhaustive list of common country codes for help in choosing "close" matches + # These are generally sorted by market cap. + def sorted_country_codes_by_relevance + %w[US CN JP IN GB CA FR DE CH SA TW AU NL SE KR IE ES AE IT HK BR DK SG MX RU IL ID BE TH NO] + end + + # Non-exhaustive list of common exchange operating MICs for help in choosing "close" matches + # This is very US-centric since our prices provider and user base is a majority US-based + def sorted_exchange_operating_mics_by_relevance + [ + "XNYS", # New York Stock Exchange + "XNAS", # NASDAQ Stock Market + "XOTC", # OTC Markets Group (OTC Link) + "OTCM", # OTC Markets Group + "OTCN", # OTC Bulletin Board + "OTCI", # OTC International + "OPRA", # Options Price Reporting Authority + "MEMX", # Members Exchange + "IEXA", # IEX All-Market + "IEXG", # IEX Growth Market + "EDXM", # Cboe EDGX Exchange (Equities) + "XCME", # CME Group (Derivatives) + "XCBT", # Chicago Board of Trade + "XPUS", # Nasdaq PSX (U.S.) + "XPSE", # Nasdaq PHLX (U.S.) + "XTRD", # Nasdaq TRF (Trade Reporting Facility) + "XTXD", # FINRA TRACE (Trade Reporting) + "XARC", # NYSE Arca + "XBOX", # BOX Options Exchange + "XBXO" # BZX Options (Cboe) + ] end end diff --git a/test/models/security/health_checker_test.rb b/test/models/security/health_checker_test.rb index 61acab15..a8efe8f7 100644 --- a/test/models/security/health_checker_test.rb +++ b/test/models/security/health_checker_test.rb @@ -1,42 +1,158 @@ require "test_helper" class Security::HealthCheckerTest < ActiveSupport::TestCase - test "checks all securities that don't have a health check" do - # Setup - unchecked_security = Security.create!(ticker: "AAA") - recent_security = Security.create!(ticker: "BBB", last_health_check_at: 5.days.ago) - due_security = Security.create!(ticker: "CCC", last_health_check_at: Security::HealthChecker::HEALTH_CHECK_INTERVAL.ago - 1.day) + include ProviderTestHelper - scope = Security::HealthChecker.send(:never_checked_scope) + setup do + # Clean slate + Holding.destroy_all + Trade.destroy_all + Security::Price.delete_all + Security.delete_all - assert_includes scope, unchecked_security - refute_includes scope, recent_security - refute_includes scope, due_security + @provider = mock + Security.stubs(:provider).returns(@provider) + + # Brand new, no health check has been run yet + @new_security = Security.create!( + ticker: "NEW", + offline: false, + last_health_check_at: nil + ) + + # New security, offline + # This will be checked, but unless it gets a price, we keep it offline + @new_offline_security = Security.create!( + ticker: "NEW_OFFLINE", + offline: true, + last_health_check_at: nil + ) + + # Online, recently checked, healthy + @healthy_security = Security.create!( + ticker: "HEALTHY", + offline: false, + last_health_check_at: 2.hours.ago + ) + + # Online, due for a health check + @due_for_check_security = Security.create!( + ticker: "DUE", + offline: false, + last_health_check_at: Security::HealthChecker::HEALTH_CHECK_INTERVAL.ago - 1.day + ) + + # Offline, recently checked (keep offline, don't check) + @offline_security = Security.create!( + ticker: "OFFLINE", + offline: true, + last_health_check_at: 20.days.ago + ) + + # Currently offline, but has had no health check and actually has prices (needs to convert to "online") + @offline_never_checked_with_prices = Security.create!( + ticker: "OFFLINE_NEVER_CHECKED", + offline: true, + last_health_check_at: nil + ) end - # We don't intend to check all securities every day - test "checks oldest DAILY_BATCH_SIZE securities that haven't been checked in HEALTH_CHECK_INTERVAL days" do - batch_size = Security::HealthChecker::DAILY_BATCH_SIZE + test "any security without a health check runs" do + to_check = Security.where(last_health_check_at: nil).or(Security.where(last_health_check_at: ..Security::HealthChecker::HEALTH_CHECK_INTERVAL.ago)) + Security::HealthChecker.any_instance.expects(:run_check).times(to_check.count) + Security::HealthChecker.check_all + end - # Create batch_size + 2 securities that are all past the health check interval so that - # the scope needs to apply the LIMIT and ordering. - (batch_size + 2).times do |i| - # Spread the dates so we can assert ordering (older first) - days_past_interval = Security::HealthChecker::HEALTH_CHECK_INTERVAL + i.days + 1.day - Security.create!(ticker: "SEC#{i}", last_health_check_at: days_past_interval.ago) + test "offline security with no health check that fails stays offline" do + hc = Security::HealthChecker.new(@new_offline_security) + + @provider.expects(:fetch_security_price) + .with( + symbol: @new_offline_security.ticker, + exchange_operating_mic: @new_offline_security.exchange_operating_mic, + date: Date.current + ) + .returns( + provider_error_response(StandardError.new("No prices found")) + ) + .once + + hc.run_check + + assert_equal 1, @new_offline_security.failed_fetch_count + assert @new_offline_security.offline? + end + + test "after enough consecutive health check failures, security goes offline and prices are deleted" do + # Create one test price + Security::Price.create!( + security: @due_for_check_security, + date: Date.current, + price: 100, + currency: "USD" + ) + + hc = Security::HealthChecker.new(@due_for_check_security) + + @provider.expects(:fetch_security_price) + .with( + symbol: @due_for_check_security.ticker, + exchange_operating_mic: @due_for_check_security.exchange_operating_mic, + date: Date.current + ) + .returns(provider_error_response(StandardError.new("No prices found"))) + .times(Security::HealthChecker::MAX_CONSECUTIVE_FAILURES + 1) + + Security::HealthChecker::MAX_CONSECUTIVE_FAILURES.times do + hc.run_check end - scoped = Security::HealthChecker.send(:due_for_check_scope).to_a + refute @due_for_check_security.offline? + assert_equal 1, @due_for_check_security.prices.count - # 1. Only DAILY_BATCH_SIZE records are returned - assert_equal batch_size, scoped.size + # We've now exceeded the max consecutive failures, so the security should be marked offline + hc.run_check + assert @due_for_check_security.offline? + assert_equal 0, @due_for_check_security.prices.count + end - # 2. Records are ordered oldest -> newest by last_health_check_at - ordered_dates = scoped.map(&:last_health_check_at) - assert_equal ordered_dates.sort, ordered_dates, "due_for_check_scope should return oldest records first" + test "failure incrementor increases for each health check failure" do + hc = Security::HealthChecker.new(@due_for_check_security) - # 3. The newest (least old) security should have been excluded due to the LIMIT - newest_excluded_date = Security.order(last_health_check_at: :desc).where(last_health_check_at: ..Security::HealthChecker::HEALTH_CHECK_INTERVAL.ago).first.last_health_check_at - refute_includes scoped.map(&:last_health_check_at), newest_excluded_date + @provider.expects(:fetch_security_price) + .with( + symbol: @due_for_check_security.ticker, + exchange_operating_mic: @due_for_check_security.exchange_operating_mic, + date: Date.current + ) + .returns(provider_error_response(StandardError.new("No prices found"))) + .twice + + hc.run_check + assert_equal 1, @due_for_check_security.failed_fetch_count + + hc.run_check + assert_equal 2, @due_for_check_security.failed_fetch_count + end + + test "failure incrementor resets to 0 when health check succeeds" do + hc = Security::HealthChecker.new(@offline_never_checked_with_prices) + + @provider.expects(:fetch_security_price) + .with( + symbol: @offline_never_checked_with_prices.ticker, + exchange_operating_mic: @offline_never_checked_with_prices.exchange_operating_mic, + date: Date.current + ) + .returns(provider_success_response(OpenStruct.new(price: 100, date: Date.current, currency: "USD"))) + .once + + assert @offline_never_checked_with_prices.offline? + + hc.run_check + + refute @offline_never_checked_with_prices.offline? + assert_equal 0, @offline_never_checked_with_prices.failed_fetch_count + assert_nil @offline_never_checked_with_prices.failed_fetch_at end end diff --git a/test/models/security/price_test.rb b/test/models/security/price_test.rb index 14c022fe..6e8e49da 100644 --- a/test/models/security/price_test.rb +++ b/test/models/security/price_test.rb @@ -43,7 +43,7 @@ class Security::PriceTest < ActiveSupport::TestCase with_provider_response = provider_error_response(StandardError.new("Test error")) @provider.expects(:fetch_security_price) - .with(security, date: Date.current) + .with(symbol: security.ticker, exchange_operating_mic: security.exchange_operating_mic, date: Date.current) .returns(with_provider_response) assert_not @security.find_or_fetch_price(date: Date.current) @@ -52,7 +52,7 @@ class Security::PriceTest < ActiveSupport::TestCase private def expect_provider_price(security:, price:, date:) @provider.expects(:fetch_security_price) - .with(security, date: date) + .with(symbol: security.ticker, exchange_operating_mic: security.exchange_operating_mic, date: date) .returns(provider_success_response(price)) end diff --git a/test/models/security/resolver_test.rb b/test/models/security/resolver_test.rb new file mode 100644 index 00000000..5599037b --- /dev/null +++ b/test/models/security/resolver_test.rb @@ -0,0 +1,79 @@ +require "test_helper" + +class Security::ResolverTest < ActiveSupport::TestCase + setup do + @provider = mock + Security.stubs(:provider).returns(@provider) + end + + test "resolves DB security" do + # Given an existing security in the DB that exactly matches the lookup params + db_security = Security.create!(ticker: "TSLA", exchange_operating_mic: "XNAS", country_code: "US") + + # The resolver should return the DB record and never hit the provider + Security.expects(:search_provider).never + + resolved = Security::Resolver.new("TSLA", exchange_operating_mic: "XNAS", country_code: "US").resolve + + assert_equal db_security, resolved + end + + test "resolves exact provider match" do + # Provider returns multiple results, one of which exactly matches symbol + exchange (and country) + exact_match = Security.new(ticker: "NVDA", exchange_operating_mic: "XNAS", country_code: "US") + near_miss = Security.new(ticker: "NVDA", exchange_operating_mic: "XNYS", country_code: "US") + + Security.expects(:search_provider) + .with("NVDA", exchange_operating_mic: "XNAS", country_code: "US") + .returns([ near_miss, exact_match ]) + + assert_difference "Security.count", 1 do + resolved = Security::Resolver.new("NVDA", exchange_operating_mic: "XNAS", country_code: "US").resolve + + assert resolved.persisted? + assert_equal "NVDA", resolved.ticker + assert_equal "XNAS", resolved.exchange_operating_mic + assert_equal "US", resolved.country_code + refute resolved.offline, "Exact provider matches should not be marked offline" + end + end + + test "resolves close provider match" do + # No exact match – resolver should choose the most relevant close match based on exchange + country ranking + preferred = Security.new(ticker: "TEST1", exchange_operating_mic: "XNAS", country_code: "US") + other = Security.new(ticker: "TEST2", exchange_operating_mic: "XNYS", country_code: "GB") + + # Return in reverse-priority order to prove the sorter works + Security.expects(:search_provider) + .with("TEST", exchange_operating_mic: "XNAS", country_code: nil) + .returns([ other, preferred ]) + + assert_difference "Security.count", 1 do + resolved = Security::Resolver.new("TEST", exchange_operating_mic: "XNAS").resolve + + assert resolved.persisted? + assert_equal "TEST1", resolved.ticker + assert_equal "XNAS", resolved.exchange_operating_mic + assert_equal "US", resolved.country_code + refute resolved.offline, "Provider matches should not be marked offline" + end + end + + test "resolves offline security" do + Security.expects(:search_provider).returns([]) + + assert_difference "Security.count", 1 do + resolved = Security::Resolver.new("FOO").resolve + + assert resolved.persisted?, "Offline security should be saved" + assert_equal "FOO", resolved.ticker + assert resolved.offline, "Offline securities should be flagged offline" + end + end + + test "returns nil when symbol blank" do + assert_no_difference "Security.count" do + assert_nil Security::Resolver.new(nil).resolve + end + end +end