diff --git a/app/models/account/provider_import_adapter.rb b/app/models/account/provider_import_adapter.rb index 95e7915a9..4403a1167 100644 --- a/app/models/account/provider_import_adapter.rb +++ b/app/models/account/provider_import_adapter.rb @@ -143,20 +143,27 @@ class Account::ProviderImportAdapter holding = account.holdings.find_by(external_id: external_id) unless holding - # Fallback path: match by (security, date, currency) to respect schemas - # where a unique index exists on these columns. This allows us to "claim" - # an existing row created by another import path (e.g., fixtures or a prior provider) - holding = account.holdings.find_by( + # Fallback path: match by (security, date, currency) — and when provided, + # also scope by account_provider_id to avoid cross‑provider claiming. + # This keeps behavior symmetric with deletion logic below which filters + # by account_provider_id when present. + find_by_attrs = { security: security, date: date, currency: currency - ) + } + if account_provider_id.present? + find_by_attrs[:account_provider_id] = account_provider_id + end + + holding = account.holdings.find_by(find_by_attrs) end holding ||= account.holdings.new( security: security, date: date, - currency: currency + currency: currency, + account_provider_id: account_provider_id ) else holding = account.holdings.find_or_initialize_by( @@ -166,6 +173,26 @@ class Account::ProviderImportAdapter ) end + # Early cross-provider composite-key conflict guard: avoid attempting a write + # that would violate a unique index on (account_id, security_id, date, currency). + if external_id.present? + existing_composite = account.holdings.find_by( + security: security, + date: date, + currency: currency + ) + + if existing_composite && + account_provider_id.present? && + existing_composite.account_provider_id.present? && + existing_composite.account_provider_id != account_provider_id + Rails.logger.warn( + "ProviderImportAdapter: cross-provider holding collision for account=#{account.id} security=#{security.id} date=#{date} currency=#{currency}; returning existing id=#{existing_composite.id}" + ) + return existing_composite + end + end + holding.assign_attributes( security: security, date: date, @@ -178,7 +205,45 @@ class Account::ProviderImportAdapter external_id: external_id ) - holding.save! + begin + Holding.transaction(requires_new: true) do + holding.save! + end + rescue ActiveRecord::RecordNotUnique => e + # Handle unique index collisions on (account_id, security_id, date, currency) + # that can occur when another provider (or concurrent import) already + # created a row for this composite key. Use the existing row and keep + # the outer transaction valid by isolating the error in a savepoint. + existing = account.holdings.find_by( + security: security, + date: date, + currency: currency + ) + + if existing + # If an existing row belongs to a different provider, do NOT claim it. + # Keep cross-provider isolation symmetrical with deletion logic. + if account_provider_id.present? && existing.account_provider_id.present? && existing.account_provider_id != account_provider_id + Rails.logger.warn( + "ProviderImportAdapter: cross-provider holding collision for account=#{account.id} security=#{security.id} date=#{date} currency=#{currency}; returning existing id=#{existing.id}" + ) + holding = existing + else + # Same provider (or unowned). Optionally set external_id if blank to ensure idempotency. + if external_id.present? && existing.external_id.blank? + begin + existing.update_columns(external_id: external_id) + rescue => _ + # Best-effort only; avoid raising in collision handler + end + end + holding = existing + end + else + # Could not find an existing row; re-raise original error + raise e + end + end # Optionally delete future holdings for this security (Plaid behavior) # Only delete if ALL providers allow deletion (cross-provider check) diff --git a/lib/tasks/simplefin_holdings_backfill.rake b/lib/tasks/simplefin_holdings_backfill.rake index 7e33fb18d..1129866e1 100644 --- a/lib/tasks/simplefin_holdings_backfill.rake +++ b/lib/tasks/simplefin_holdings_backfill.rake @@ -96,7 +96,6 @@ namespace :sure do sfa = ap&.provider || SimplefinAccount.find_by(account: acct) sfas = Array.wrap(sfa).compact else - success = errors.empty? puts({ ok: false, error: "usage", message: "Provide one of item_id, account_id, or account_name" }.to_json) exit 1 end diff --git a/test/models/account/provider_import_adapter_cross_provider_test.rb b/test/models/account/provider_import_adapter_cross_provider_test.rb new file mode 100644 index 000000000..e78f55237 --- /dev/null +++ b/test/models/account/provider_import_adapter_cross_provider_test.rb @@ -0,0 +1,95 @@ +require "test_helper" + +# Ensures a provider cannot "claim" another provider's holdings when external_id lookup misses +# and a (security, date, currency) match exists. The fallback path must be scoped by account_provider_id. +class Account::ProviderImportAdapterCrossProviderTest < ActiveSupport::TestCase + test "does not claim holdings from a different provider when external_id is present and fallback kicks in" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + security = securities(:aapl) + + # Create two different account providers for the SAME account + # Provider A (e.g., Plaid) + ap_a = AccountProvider.create!( + account: investment_account, + provider: plaid_accounts(:one) + ) + + # Provider B (e.g., SimpleFin) + item = SimplefinItem.create!(family: families(:dylan_family), name: "SF Conn", access_url: "https://example.com/access") + sfa_b = SimplefinAccount.create!( + simplefin_item: item, + name: "SF Invest", + account_id: "sf_inv_cross_provider", + currency: "USD", + account_type: "investment", + current_balance: 1000 + ) + ap_b = AccountProvider.create!( + account: investment_account, + provider: sfa_b + ) + + # Use a date that will not collide with existing fixture holdings for this account + holding_date = Date.today - 3.days + + # Existing holding created by Provider A for (security, date, currency) + existing_a = investment_account.holdings.create!( + security: security, + date: holding_date, + qty: 1, + price: 100, + amount: 100, + currency: "USD", + account_provider_id: ap_a.id + ) + + # Now import for Provider B with an external_id that doesn't exist yet. + # Fallback should NOT "claim" Provider A's row because account_provider_id differs. + # Attempt import for Provider B with a conflicting composite key. + # Policy: do NOT create a duplicate row and do NOT claim Provider A's row. + assert_no_difference "investment_account.holdings.count" do + @result_b = adapter.import_holding( + security: security, + quantity: 2, + amount: 220, + currency: "USD", + date: holding_date, + price: 110, + cost_basis: nil, + external_id: "ext-b-1", + source: "simplefin", + account_provider_id: ap_b.id, + delete_future_holdings: false + ) + end + + # Provider A's holding remains unclaimed (no external_id added) and still owned by A + existing_a.reload + assert_nil existing_a.external_id + assert_equal ap_a.id, existing_a.account_provider_id + + # Adapter returns the existing A row for transparency + assert_equal existing_a.id, @result_b.id + + # Idempotency: importing again with the same external_id should not create another row + assert_no_difference "investment_account.holdings.count" do + again = adapter.import_holding( + security: security, + quantity: 2, + amount: 220, + currency: "USD", + date: holding_date, + price: 110, + cost_basis: nil, + external_id: "ext-b-1", + source: "simplefin", + account_provider_id: ap_b.id, + delete_future_holdings: false + ) + assert_equal existing_a.id, again.id + # Ensure external_id was NOT attached to A's row (no cross-provider claim) + assert_nil existing_a.reload.external_id + end + end +end