Add investment instrument discovery search API (#510, slice 1)#512
Merged
Conversation
First increment of issue #510: a user-facing instrument discovery search over OpenFIGI and Alpha Vantage, kept separate from the admin asset CRUD API. - InstrumentDiscoveryResultDto (Domain): rich listing-level result with identity (FIGI/ISIN), exchange MIC/name, trading currency, provider symbol, source, confidence score and warnings. Distinct from the existing InstrumentSearchResultDto (which searches existing DB listings). - IInvestmentInstrumentDiscoveryService + implementation (Application): detects ISIN vs ticker/name queries, fans out to OpenFIGI and Alpha Vantage, prefers OpenFIGI for identity, attaches the AV symbol as a price candidate, merges/de-duplicates, scores confidence, and emits warnings for ambiguous exchange mappings, GBX/minor-unit quoting, missing symbols and currency mismatches. Reuses the existing BrokerSymbol venue mapping; results cached 15 minutes. Provider failures are swallowed (no raw exceptions to the UI). - GET /api/investments/instruments/search endpoint. - Unit tests for ISIN, ticker/name, OpenFIGI-only, AV-only, combined, GBX multiplier warning, ambiguous-exchange warning, and caching. Import-preview/import (with symbol validation) and the Blazor search/import UI are intentionally deferred to later slices. Claude-Session: https://claude.ai/code/session_01NiXYVP3Hth2rmGTPs7A6Xw
|
Warning Review limit reached
More reviews will be available in 16 minutes and 3 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds an authenticated investment instrument discovery endpoint, a discovery service that queries OpenFIGI and Alpha Vantage with caching and result normalization, a shared result DTO, DI registration, and unit tests covering search paths and warnings. ChangesInvestment instrument discovery
Sequence Diagram(s)sequenceDiagram
participant InvestmentInstrumentDiscoveryController
participant InvestmentInstrumentDiscoveryService
participant IMemoryCache
participant IOpenFigiClient
participant IAlphaVantageClient
InvestmentInstrumentDiscoveryController->>InvestmentInstrumentDiscoveryService: SearchAsync(query, cancellationToken)
InvestmentInstrumentDiscoveryService->>IMemoryCache: TryGetValue(cacheKey)
alt ISIN query
InvestmentInstrumentDiscoveryService->>IOpenFigiClient: MapByIsinAsync(query, ct)
else ticker or name query
InvestmentInstrumentDiscoveryService->>IOpenFigiClient: MapByTickerAsync(baseTicker, exchangeCode, ct)
InvestmentInstrumentDiscoveryService->>IAlphaVantageClient: SearchSymbolsAsync(query, ct)
end
InvestmentInstrumentDiscoveryService-->>InvestmentInstrumentDiscoveryController: IReadOnlyList<InstrumentDiscoveryResultDto>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@code/FinanceManager.Application/FinancialAccounts/Investments/Discovery/InvestmentInstrumentDiscoveryService.cs`:
- Around line 111-117: The matching logic in
InvestmentInstrumentDiscoveryService is reusing the same Alpha Vantage symbol
across multiple OpenFIGI listings because FindMatchingAv still searches the full
avMatches set instead of excluding already-used entries. Update the matching
flow in the loop that builds each DTO, and in the other affected block, so it
filters out symbols already present in consumedAvSymbols before selecting a
match; this should ensure each AV symbol is attached to at most one listing and
prevent duplicate “Combined” confidence assignments.
- Around line 45-61: The outer catch in InvestmentInstrumentDiscoveryService’s
discovery flow is swallowing non-provider bugs and turning them into empty
results. Remove or narrow the general Exception handler around the
SearchByIsinAsync/SearchByTickerOrNameAsync path so only expected provider
failures are degraded, and let errors from Merge, ResolveVenue, DTO
construction, or caching propagate normally; keep the OperationCanceledException
handling intact and preserve the existing SafeOpenFigi/SafeAlphaVantage fallback
behavior.
- Around line 204-223: Normalize the Alpha Vantage fallback in
BuildFromAlphaVantage so AV-only matches return canonical instrument fields
instead of raw provider values. Use BrokerSymbol.TryLookupByVenueToken (via the
av.Region alias) to derive the venue metadata and populate ExchangeMic, and
separate the provider symbol from the display ticker so Ticker is normalized
while ProviderSymbol keeps av.Symbol. Keep the existing confidence/warnings
behavior, but ensure the returned InstrumentDiscoveryResultDto includes the
resolved exchange identity rather than leaving ExchangeMic unset.
In `@code/FinanceManager.Domain/Assets/Discovery/InstrumentDiscoveryResultDto.cs`:
- Around line 57-58: The InstrumentDiscoveryResultDto.Warnings property is
mutable, which allows callers to alter cached discovery results reused by
InvestmentInstrumentDiscoveryService.SearchAsync. Change Warnings to an
immutable/read-only type such as IReadOnlyList<string> or an immutable
collection, and update any construction or assignment code in
InstrumentDiscoveryResultDto and its consumers to produce the read-only value
without exposing a mutable List<string>.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0539c93d-389a-46ca-ac27-e43b5fbcc5c8
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/*.md
📒 Files selected for processing (6)
code/FinanceManager.Api/Controllers/Accounts/InvestmentInstrumentDiscoveryController.cscode/FinanceManager.Application/FinancialAccounts/Investments/Discovery/IInvestmentInstrumentDiscoveryService.cscode/FinanceManager.Application/FinancialAccounts/Investments/Discovery/InvestmentInstrumentDiscoveryService.cscode/FinanceManager.Application/FinancialAccounts/Registration.cscode/FinanceManager.Domain/Assets/Discovery/InstrumentDiscoveryResultDto.cscode/FinanceManager.Tests.Unit/Application/Services/Discovery/InvestmentInstrumentDiscoveryServiceTests.cs
…#510 Fixes the architecture test (EveryApiControllerHasIntegrationTests) that required InvestmentInstrumentDiscoveryController to have integration coverage, and applies CodeRabbit review feedback on the discovery service. - Add InvestmentInstrumentDiscoveryControllerTests (integration): unauthorized, empty-query (no provider calls), and ticker search with mocked OpenFIGI/Alpha Vantage clients (no real provider calls). - FindMatchingAv now skips Alpha Vantage symbols already consumed by an earlier listing, so one AV symbol is never attached to multiple listings (and no duplicate "Combined" results). Covered by a new unit test. - Normalize AV-only results: resolve venue identity from the AV region, keep the base ticker as the display Ticker, and preserve the raw provider symbol on ProviderSymbol. - Remove the broad outer catch in SearchAsync: provider failures are already degraded by SafeOpenFigi/SafeAlphaVantage, so only genuine bugs would have been swallowed into empty results. Cancellation still propagates. - Make InstrumentDiscoveryResultDto.Warnings an IReadOnlyList<string> so cached results can't be mutated by callers. Claude-Session: https://claude.ai/code/session_01NiXYVP3Hth2rmGTPs7A6Xw
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First, conservative increment of #510 (Add investment instrument discovery and import using OpenFIGI and Alpha Vantage). Issue #510 is size/XL, so per the repo's CLAUDE.md guidance ("if XL, split / implement incrementally") this PR delivers only the search foundation — no master-data is created and no UI is touched yet.
This is part of #510, not a full resolution, so there is intentionally no auto-close keyword.
What's included
InstrumentDiscoveryResultDto(Domain, newAssets/Discoverynamespace): rich listing-level result —Source,DisplayName,Ticker,ExchangeMic/ExchangeCode/ExchangeName,TradingCurrency,ListingFigi/ShareClassFigi/CompositeFigi,Isin,SecurityType/MarketSector,ProviderSymbol,ConfidenceScore,Warnings. Deliberately distinct from the existingInstrumentSearchResultDto(which searches listings already in the DB) to avoid breaking the current transaction-form autocomplete.IInvestmentInstrumentDiscoveryService+InvestmentInstrumentDiscoveryService(Application):MapByIsinAsync(listing-level results);1.0, OpenFIGI-only0.7, AV-only≤0.6) and emits warnings for ambiguous/unknown exchange mappings, minor-unit (GBX) quoting, missing price symbols, and AV/listing currency mismatches;BrokerSymbolvenue mapping (no new EF table / migration in this slice);GET /api/investments/instruments/search?query=— new user-facing endpoint, separate from the admin asset CRUD API.AddFinancialAccountsApplication.InvestmentInstrumentDiscoveryServiceTests): empty query, ISIN path, ticker/name merge → Combined, OpenFIGI-only normalization + missing-symbol warning, AV-only handling, GBX multiplier warning, ambiguous-exchange warning, and result caching.Deferred to later slices
ExchangeMappingDB table/service/seed (this slice reuses the in-codeBrokerSymbolmapping).IOpenFigiClient).Validation
dotnet build— succeeds, 0 warnings (warnings-as-errors).dotnet format— clean.dotnet test(unit) — 493 passed, 0 failed..razor/UI changes in this slice, so no screenshots.https://claude.ai/code/session_01NiXYVP3Hth2rmGTPs7A6Xw
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests