Fix: Unravel Factors Live Handle Null Value #80
Conversation
moconnell
added
bug
WalkthroughUpdated docs and VSCode launch config to .NET 10; API response model changed to nullable doubles, deserialization now validates and surfaces detailed errors, service layer maps nulls to NaN, tests expanded; numerous files had using-directive pruning and minor formatting changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (service)
participant Unravel as UnravelApiService
participant HTTP as HttpClientExtensions
participant API as Remote API
Caller->>Unravel: GetFactorsLiveAsync(tickers, ...)
Unravel->>HTTP: Send GET request
HTTP->>API: HTTP request
API-->>HTTP: HTTP 200 + JSON payload (Data: [double?, ...])
alt JSON deserializes and Data present
HTTP-->>Unravel: ApiResponse<T> with nullable doubles
Unravel->>Unravel: Map null -> double.NaN, align missing tickers, build DataFrame
Unravel-->>Caller: DataFrame (with NaN)
else Deserialization error or missing/empty Data
HTTP-->>Unravel: throws ApiException (detailed message + raw content)
Unravel-->>Caller: propagate ApiException
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
🧹 Nitpick comments (2)
src/YoloAbstractions/FactorDataFrame.cs (1)
245-320: Row/column counts cast to int are acceptable for expected sizesCasting
_dataFrame.Rows.Counttointforrowsand subsequent loops is fine given the practical data sizes in this domain; if you ever expect >2B rows, you’d want to revisit this to keep everything inlong.test/Unravel.Api.Test/UnravelApiServiceTest.cs (1)
345-389: Test validates null factor values are normalized to NaN
GivenMissingValues_WhenThrowOnMissingValueFalse_ShouldPopulateNaNexercises the case wheredatacontainsnullfor a present ticker and verifies that it comes through as NaN inFactorDataFrame, while non-null entries retain their numeric values. You might consider tweaking the// Missing ADAcomment to clarify that it’s a missing value, not a missing ticker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.vscode/launch.json(1 hunks)README.md(3 hunks)src/Unravel.Api/Data/FactorsLiveResponse.cs(1 hunks)src/Unravel.Api/UnravelApiService.cs(2 hunks)src/YoloAbstractions/Extensions/HttpClientExtensions.cs(1 hunks)src/YoloAbstractions/FactorDataFrame.cs(7 hunks)test/Unravel.Api.Test/UnravelApiServiceTest.cs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use C# 12+ features including nullable reference types (Nullable is enabled)
Follow implicit usings pattern (ImplicitUsings is enabled)
Maintain existing code style and conventions
Avoid adding comments unless they explain complex logic or match existing comment patterns
Files:
src/Unravel.Api/Data/FactorsLiveResponse.cssrc/Unravel.Api/UnravelApiService.cssrc/YoloAbstractions/Extensions/HttpClientExtensions.cssrc/YoloAbstractions/FactorDataFrame.cstest/Unravel.Api.Test/UnravelApiServiceTest.cs
test/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
test/**/*.cs: Use xUnit as the testing framework
Use Moq for creating mock objects in tests
Use Shouldly for fluent assertions in tests
Integration tests should be marked with [Trait("Category", "Integration")] attribute
Files:
test/Unravel.Api.Test/UnravelApiServiceTest.cs
🧠 Learnings (1)
📚 Learning: 2025-11-02T20:24:42.143Z
Learnt from: moconnell
Repo: moconnell/yolo PR: 37
File: src/YoloBroker/BrokerVolatilityFactorService.cs:26-30
Timestamp: 2025-11-02T20:24:42.143Z
Learning: In implementations of IGetFactors.GetFactorsAsync, the `factors` parameter represents factors that have already been obtained or computed, not a filter of factors to compute. Services should return empty or skip computation when their factor type is already present in this set to avoid duplicate work.
Applied to files:
src/Unravel.Api/Data/FactorsLiveResponse.cssrc/Unravel.Api/UnravelApiService.cstest/Unravel.Api.Test/UnravelApiServiceTest.cs
🧬 Code graph analysis (4)
src/Unravel.Api/Data/FactorsLiveResponse.cs (2)
src/Unravel.Api/Data/FactorsResponse.cs (1)
FactorsResponse(6-16)src/Unravel.Api/Interfaces/IUnravelApiService.cs (1)
IUnravelApiService(5-13)
src/Unravel.Api/UnravelApiService.cs (3)
src/Unravel.Api/Data/FactorsLiveResponse.cs (1)
FactorsLiveResponse(6-16)src/YoloAbstractions/FactorDataFrame.cs (3)
FactorDataFrame(15-23)FactorDataFrame(33-60)FactorDataFrame(126-164)src/Unravel.Api/Interfaces/IUnravelApiService.cs (1)
Task(7-10)
src/YoloAbstractions/FactorDataFrame.cs (1)
src/RobotWealth.Api/RobotWealthFactorService.cs (1)
FactorDataFrame(41-72)
test/Unravel.Api.Test/UnravelApiServiceTest.cs (2)
src/Unravel.Api/UnravelApiService.cs (4)
UnravelApiService(10-122)UnravelApiService(28-36)Task(38-102)Task(104-121)src/Unravel.Api/Interfaces/IUnravelApiService.cs (2)
Task(7-10)Task(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (13)
.vscode/launch.json (1)
27-41: Debug profile now targets net10.0 outputUpdating the debug
programpath tobin/Debug/net10.0/YoloKonsole.dllis consistent with the .NET 10 requirement; just ensure theYoloKonsoleproject actually targetsnet10.0so this path exists in all dev environments.README.md (2)
45-47: .NET 10 requirement wording is clearUpdating the requirement to “.NET 10 installation required” matches the launch configuration change; assuming the projects target
net10.0, this keeps docs and tooling aligned.
97-112: RebalanceMode example and values listing are clearerThe expanded tolerance-band example and explicit
Center/Edgevalues block read well and make the trade‑off much easier to understand without changing behavior.src/YoloAbstractions/FactorDataFrame.cs (4)
15-28: Ticker index and Tickers property remain consistentCasting
"Ticker"toStringDataFrameColumnfor_tickerIndexandTickerskeeps the column access strongly typed; these are style-only tweaks and preserve existing semantics.
62-74: Indexer now treats null factor values as NaNUsing
(double?)_dataFrame[factorType.ToString()][index]and returningval ?? double.NaNgives a consistent “missing == NaN” contract on the[FactorType, ticker]indexer, which matches the new null‑from‑API semantics and the tests that assert NaN for missing data.
137-164: Normalize keeps factor set via collection expressionThe cast to
DoubleDataFrameColumnandnew FactorDataFrame(normalizedDf, [.. FactorTypes])are idiomatic C# 12, preserving the exact factor set on the normalized frame without behavior change.
322-385: ToString uses int rowCount consistentlyThe explicit
(int)_dataFrame.Rows.CountforrowCountaligns with the usage patterns above and keeps formatting logic straightforward; no behavioral issues here.src/Unravel.Api/UnravelApiService.cs (1)
60-91: Nullable factors payload is handled correctly with NaN mappingSwitching to
_httpClient.GetAsync<FactorsLiveResponse, double?>(...)and projectingresponse.Dataviad ?? double.NaNin both the full‑match and missing‑ticker branches makes all missing values surface as NaNs inFactorDataFrame, which matches the new API contract and the added tests.src/Unravel.Api/Data/FactorsLiveResponse.cs (1)
6-15: DTO now correctly models nullable factor dataUpdating
FactorsLiveResponsetoIApiResponse<double?>withIReadOnlyList<double?> Dataaccurately represents nullable factor values from the API and lines up with the newGetAsync<..., double?>usage.src/YoloAbstractions/Extensions/HttpClientExtensions.cs (1)
41-56: Stricter deserialization and “no data” checks improve robustnessThe try/catch around
ReadFromJsonAsync, the explicit guard against null/emptyData, and the detailedApiException(including raw response) give much clearer failures for bad or empty payloads without masking intentional ApiExceptions.test/Unravel.Api.Test/UnravelApiServiceTest.cs (3)
299-343: Test covers NaN fill-in when tickers are missing but throwOnMissingValue is false
GivenMissingTickers_WhenThrowOnMissingValueFalse_ShouldPopulateNaNcorrectly mocks a response missingADAand asserts that the resultingFactorDataFramehas the requested ticker set with NaN forADAand the expected numeric values forBTC/ETH, matching the updated service behavior.
574-592: Empty-ticker argument validation uses modern collection syntaxCalling
GetFactorsLiveAsync([])inGivenEmptyTickers_WhenCalled_ShouldThrowArgumentExceptionis a nice use of collection expressions and clearly asserts the expected argument validation behavior.
594-611: Whitespace-only ticker validation is well-coveredThe
GivenWhitespaceOnlyTickers_WhenCalled_ShouldThrowArgumentExceptiontest, using[" ", "\t", ""], cleanly verifies that trimming/normalization eliminates such entries and that an empty effective ticker set results in anArgumentException.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/YoloTrades/TradeFactory.cs (1)
223-239: GuardCalcLimitPriceagainst null bid/ask to avoid runtime exceptions
CalcLimitPricedereferencesmarket.Ask!.Valueandmarket.Bid!.Value, but the rest of the code explicitly handlesBid/Askbeing null and keeps such markets inprojectedPositions. If a market has only one side populated (e.g., ask present, bid null),GetPricewill still return a non-null price for one side, and the code can reachCalcLimitPrice, whereBidorAskbeing null would throw at runtime.Consider defensively bailing out when either side is missing and letting the caller fall back to a market order or skip the trade:
- decimal? CalcLimitPrice() - { - var factor = isBuy ? SpreadSplit : 1 - SpreadSplit; - var spread = market.Ask!.Value - market.Bid!.Value; + decimal? CalcLimitPrice() + { + if (market.Ask is null || market.Bid is null) + return null; + + var factor = isBuy ? SpreadSplit : 1 - SpreadSplit; + var spread = market.Ask.Value - market.Bid.Value; var rawLimitPrice = market.Bid!.Value + spread * factor; var limitPriceSteps = rawLimitPrice / market.PriceStep; var limitPrice = limitPriceSteps is null ? null : (isBuy ? Math.Floor(limitPriceSteps.Value) : Math.Ceiling(limitPriceSteps.Value)) * market.PriceStep; return limitPrice; }And, if
CalcLimitPrice()returnsnull, confirm that downstream handling of a null limit price on theTradetype is acceptable or add a guard there as well.Also applies to: 252-265
🧹 Nitpick comments (1)
test/YoloAbstractions.Test/HttpClientExtensionsTest.cs (1)
138-148: Test does not verify cancellation token propagation.The assertion
cts.Token.IsCancellationRequested.ShouldBeFalse()only checks that the token wasn't cancelled—it doesn't verify the token was actually passed toSendAsync. This test will pass even if the implementation ignores the cancellation token entirely.Capture and verify the token in the mock:
[Fact] public async Task GetAsync_WithCancellationToken_PassesTokenToRequest() { var apiResponse = new TestApiResponse { Data = ["item1"] }; - var httpClient = CreateHttpClient(HttpStatusCode.OK, apiResponse); - var cts = new CancellationTokenSource(); + CancellationToken capturedToken = default; + var mockHandler = new Mock<HttpMessageHandler>(); + mockHandler.Protected() + .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.IsAny<HttpRequestMessage>(), ItExpr.IsAny<CancellationToken>()) + .ReturnsAsync((HttpRequestMessage _, CancellationToken ct) => + { + capturedToken = ct; + return new HttpResponseMessage + { + StatusCode = HttpStatusCode.OK, + Content = new StringContent(JsonSerializer.Serialize(apiResponse), Encoding.UTF8, "application/json") + }; + }); + var httpClient = new HttpClient(mockHandler.Object); + using var cts = new CancellationTokenSource(); await httpClient.GetAsync<TestApiResponse, string>("https://api.example.com/data", cancellationToken: cts.Token); - cts.Token.IsCancellationRequested.ShouldBeFalse(); + capturedToken.ShouldBe(cts.Token); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
src/RobotWealth.Api/Data/RwApiResponse.cs(0 hunks)src/RobotWealth.Api/Data/RwVolatility.cs(0 hunks)src/RobotWealth.Api/Data/RwWeight.cs(0 hunks)src/RobotWealth.Api/Interfaces/IRobotWealthApiService.cs(0 hunks)src/RobotWealth.Api/RobotWealthApiService.cs(0 hunks)src/RobotWealth.Api/RobotWealthFactorService.cs(0 hunks)src/YoloAbstractions/AssetPermissions.cs(0 hunks)src/YoloAbstractions/Config/YoloConfig.cs(0 hunks)src/YoloAbstractions/Exceptions/ApiException.cs(0 hunks)src/YoloAbstractions/Exceptions/ConfigException.cs(0 hunks)src/YoloAbstractions/Extensions/ConfigExtensions.cs(0 hunks)src/YoloAbstractions/Extensions/HttpClientExtensions.cs(1 hunks)src/YoloAbstractions/Extensions/Sum.cs(0 hunks)src/YoloAbstractions/Extensions/TickerExtensions.cs(0 hunks)src/YoloAbstractions/Extensions/ToCsv.cs(0 hunks)src/YoloAbstractions/Extensions/VectorExtensions.cs(0 hunks)src/YoloAbstractions/Extensions/VolatilityExtensions.cs(0 hunks)src/YoloAbstractions/FactorDataFrame.cs(7 hunks)src/YoloAbstractions/Interfaces/IApiResponse.cs(0 hunks)src/YoloAbstractions/Interfaces/ICalcWeights.cs(0 hunks)src/YoloAbstractions/Interfaces/IGetFactors.cs(0 hunks)src/YoloAbstractions/Interfaces/ITradeFactory.cs(0 hunks)src/YoloAbstractions/MarketInfo.cs(0 hunks)src/YoloAbstractions/Order.cs(0 hunks)src/YoloAbstractions/OrderManagementSettings.cs(0 hunks)src/YoloAbstractions/OrderUpdate.cs(0 hunks)src/YoloAbstractions/Price.cs(0 hunks)src/YoloAbstractions/Trade.cs(0 hunks)src/YoloBroker.Hyperliquid/Config/HyperliquidConfig.cs(0 hunks)src/YoloBroker.Hyperliquid/CustomSigning/Nethereum.cs(0 hunks)src/YoloBroker.Hyperliquid/Extensions/DecimalExtensions.cs(0 hunks)src/YoloBroker.Hyperliquid/Extensions/TypeConversionExtensions.cs(0 hunks)src/YoloBroker.Hyperliquid/HyperliquidBroker.cs(0 hunks)src/YoloBroker/BrokerVolatilityFactorService.cs(0 hunks)src/YoloBroker/Exceptions/BrokerException.cs(0 hunks)src/YoloBroker/Interface/IYoloBroker.cs(0 hunks)src/YoloBroker/TickerAliasService.cs(0 hunks)src/YoloKonsole/Extensions/LoggerExtensions.cs(0 hunks)src/YoloKonsole/Program.cs(1 hunks)src/YoloTrades/LoggerExtensions.cs(0 hunks)src/YoloTrades/PositionExtensions.cs(0 hunks)src/YoloTrades/PriceExtensions.cs(0 hunks)src/YoloTrades/ProjectedPosition.cs(0 hunks)src/YoloTrades/TradeFactory.cs(1 hunks)test/Robotwealth.Api.Test/RobotWealthFactorServiceTest.cs(0 hunks)test/YoloAbstractions.Test/HttpClientExtensionsTest.cs(1 hunks)test/YoloAbstractions.Test/YoloAbstractions.Test.csproj(2 hunks)test/YoloBroker.Hyperliquid.Test/Extensions/TypeConversionExtensionsTest.cs(0 hunks)
💤 Files with no reviewable changes (42)
- test/YoloBroker.Hyperliquid.Test/Extensions/TypeConversionExtensionsTest.cs
- src/YoloAbstractions/Extensions/ConfigExtensions.cs
- src/YoloTrades/LoggerExtensions.cs
- src/YoloAbstractions/Order.cs
- src/YoloBroker.Hyperliquid/Extensions/DecimalExtensions.cs
- test/Robotwealth.Api.Test/RobotWealthFactorServiceTest.cs
- src/YoloTrades/PositionExtensions.cs
- src/YoloAbstractions/Extensions/VectorExtensions.cs
- src/YoloAbstractions/AssetPermissions.cs
- src/YoloBroker.Hyperliquid/CustomSigning/Nethereum.cs
- src/YoloAbstractions/Extensions/TickerExtensions.cs
- src/YoloBroker.Hyperliquid/HyperliquidBroker.cs
- src/YoloAbstractions/Interfaces/ITradeFactory.cs
- src/YoloAbstractions/Trade.cs
- src/YoloAbstractions/Extensions/VolatilityExtensions.cs
- src/YoloKonsole/Extensions/LoggerExtensions.cs
- src/RobotWealth.Api/Data/RwApiResponse.cs
- src/YoloAbstractions/Extensions/ToCsv.cs
- src/YoloBroker.Hyperliquid/Config/HyperliquidConfig.cs
- src/YoloTrades/ProjectedPosition.cs
- src/RobotWealth.Api/RobotWealthFactorService.cs
- src/RobotWealth.Api/Data/RwVolatility.cs
- src/YoloAbstractions/Interfaces/ICalcWeights.cs
- src/YoloBroker/BrokerVolatilityFactorService.cs
- src/RobotWealth.Api/RobotWealthApiService.cs
- src/RobotWealth.Api/Interfaces/IRobotWealthApiService.cs
- src/YoloBroker.Hyperliquid/Extensions/TypeConversionExtensions.cs
- src/YoloAbstractions/MarketInfo.cs
- src/YoloBroker/Interface/IYoloBroker.cs
- src/YoloBroker/TickerAliasService.cs
- src/YoloAbstractions/Exceptions/ConfigException.cs
- src/YoloAbstractions/Interfaces/IGetFactors.cs
- src/YoloAbstractions/Exceptions/ApiException.cs
- src/YoloAbstractions/OrderUpdate.cs
- src/RobotWealth.Api/Data/RwWeight.cs
- src/YoloAbstractions/OrderManagementSettings.cs
- src/YoloAbstractions/Extensions/Sum.cs
- src/YoloAbstractions/Interfaces/IApiResponse.cs
- src/YoloBroker/Exceptions/BrokerException.cs
- src/YoloAbstractions/Config/YoloConfig.cs
- src/YoloAbstractions/Price.cs
- src/YoloTrades/PriceExtensions.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/YoloAbstractions/FactorDataFrame.cs
🧰 Additional context used
📓 Path-based instructions (4)
test/**/*.csproj
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Test projects must follow naming convention: [ProjectName].Test
Files:
test/YoloAbstractions.Test/YoloAbstractions.Test.csproj
**/*.csproj
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.csproj: All package versions are centrally managed in Directory.Packages.props; reference packages in .csproj files without version numbers
Use .NET 9.0 as the target framework for new projects
Enable nullable reference types and implicit usings in new .csproj projects
Files:
test/YoloAbstractions.Test/YoloAbstractions.Test.csproj
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use C# 12+ features including nullable reference types (Nullable is enabled)
Follow implicit usings pattern (ImplicitUsings is enabled)
Maintain existing code style and conventions
Avoid adding comments unless they explain complex logic or match existing comment patterns
Files:
src/YoloAbstractions/Extensions/HttpClientExtensions.cssrc/YoloTrades/TradeFactory.cstest/YoloAbstractions.Test/HttpClientExtensionsTest.cssrc/YoloKonsole/Program.cs
test/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
test/**/*.cs: Use xUnit as the testing framework
Use Moq for creating mock objects in tests
Use Shouldly for fluent assertions in tests
Integration tests should be marked with [Trait("Category", "Integration")] attribute
Files:
test/YoloAbstractions.Test/HttpClientExtensionsTest.cs
🧠 Learnings (4)
📚 Learning: 2025-11-28T10:36:40.819Z
Learnt from: CR
Repo: moconnell/yolo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T10:36:40.819Z
Learning: Applies to test/**/*.cs : Use Moq for creating mock objects in tests
Applied to files:
test/YoloAbstractions.Test/YoloAbstractions.Test.csprojtest/YoloAbstractions.Test/HttpClientExtensionsTest.cs
📚 Learning: 2025-11-28T10:36:40.819Z
Learnt from: CR
Repo: moconnell/yolo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T10:36:40.819Z
Learning: Applies to test/**/*.cs : Use Shouldly for fluent assertions in tests
Applied to files:
test/YoloAbstractions.Test/YoloAbstractions.Test.csprojtest/YoloAbstractions.Test/HttpClientExtensionsTest.cs
📚 Learning: 2025-11-28T10:36:40.819Z
Learnt from: CR
Repo: moconnell/yolo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T10:36:40.819Z
Learning: Applies to test/**/*.cs : Use xUnit as the testing framework
Applied to files:
test/YoloAbstractions.Test/YoloAbstractions.Test.csproj
📚 Learning: 2025-11-28T10:36:40.819Z
Learnt from: CR
Repo: moconnell/yolo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T10:36:40.819Z
Learning: Applies to **/*.csproj : Enable nullable reference types and implicit usings in new .csproj projects
Applied to files:
test/YoloAbstractions.Test/YoloAbstractions.Test.csproj
🧬 Code graph analysis (2)
test/YoloAbstractions.Test/YoloAbstractions.Test.csproj (3)
test/YoloWeights.Test/YoloWeightsServiceTest.cs (1)
YoloWeightsServiceTest(13-471)test/Unravel.Api.Test/UnravelFactorServiceTest.cs (1)
UnravelFactorServiceTest(9-115)test/Robotwealth.Api.Test/RobotWealthApiServiceTest.cs (1)
RobotWealthApiServiceTest(10-158)
src/YoloAbstractions/Extensions/HttpClientExtensions.cs (3)
src/YoloAbstractions/Exceptions/ApiException.cs (3)
ApiException(3-14)ApiException(5-8)ApiException(10-13)src/YoloAbstractions/Interfaces/IApiResponse.cs (1)
IApiResponse(5-8)src/Unravel.Api/UnravelApiService.cs (1)
Task(104-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (6)
src/YoloKonsole/Program.cs (1)
1-15: Using cleanup correctly relies on SDK implicit usingsThe removal of redundant
System*usings looks good. All referenced types (e.g.,Task,CancellationTokenSource,Path,Directory, LINQ extensions,Console,DateTime,TimeSpan,Math) are covered by the SDK’s implicit usings givenImplicitUsingsis enabled as per your guidelines, so there’s no missing-namespace risk and no behavioral change introduced.src/YoloTrades/TradeFactory.cs (1)
1-1: Using re-introduction looks correctRe-adding
Microsoft.Extensions.Loggingis appropriate here given the logger usage and extension methods throughout the class; no issues from this change.test/YoloAbstractions.Test/YoloAbstractions.Test.csproj (1)
6-6: LGTM!The Moq package reference and global using are correctly added without version specification (centrally managed per guidelines). This aligns with the learnings indicating Moq should be used for mocking in tests.
Also applies to: 24-24
test/YoloAbstractions.Test/HttpClientExtensionsTest.cs (3)
14-17: LGTM!The private test DTO correctly implements
IApiResponse<string>for testing purposes. Using collection expression[]as the default is idiomatic C# 12+.
19-124: Good test coverage for core functionality.The tests comprehensively cover:
- Successful deserialization with valid data
- Header propagation to requests
- Argument validation (null client, null/empty URL)
- HTTP error status handling
- Null response, null data, and empty data scenarios
- Invalid JSON with inner exception preservation
Uses Shouldly assertions and Moq as per coding guidelines.
150-172: Well-structured test helpers.The helper methods properly use
Moq.Protectedto mockHttpMessageHandler.SendAsyncand support request capture for verification. This pattern aligns with existing test infrastructure inRobotWealthApiServiceTest.
| try | ||
| { | ||
| var apiResponse = await response.Content.ReadFromJsonAsync<TResponse>(cancellationToken: cancellationToken); | ||
|
|
||
| if (apiResponse is null || apiResponse.Data is null || apiResponse.Data.Count == 0) | ||
| { | ||
| throw new ApiException($"No data returned from API ({url})"); | ||
| } | ||
|
|
||
| if (apiResponse is null || apiResponse.Data is null || apiResponse.Data.Count == 0) | ||
| return apiResponse; | ||
| } | ||
| catch (Exception ex) when (ex is not ApiException) | ||
| { | ||
| throw new ApiException($"No data returned from API ({url})"); | ||
| var responseContent = await response.Content.ReadAsStringAsync(cancellationToken); | ||
| throw new ApiException($"Error deserializing API response from ({url}): {ex.Message}\r\n{responseContent}", ex); | ||
| } |
There was a problem hiding this comment.
Response content stream already consumed before error handling.
After ReadFromJsonAsync (line 38) fails partway through, the response content stream has already been consumed or partially read. Since HttpContent streams are typically non-seekable, calling ReadAsStringAsync (line 49) will return an empty string or incomplete content, making the error message unhelpful.
Consider buffering the content first:
+ var responseContent = await response.Content.ReadAsStringAsync(cancellationToken);
try
{
- var apiResponse = await response.Content.ReadFromJsonAsync<TResponse>(cancellationToken: cancellationToken);
+ var apiResponse = JsonSerializer.Deserialize<TResponse>(responseContent);
if (apiResponse is null || apiResponse.Data is null || apiResponse.Data.Count == 0)
{
throw new ApiException($"No data returned from API ({url})");
}
return apiResponse;
}
catch (Exception ex) when (ex is not ApiException)
{
- var responseContent = await response.Content.ReadAsStringAsync(cancellationToken);
throw new ApiException($"Error deserializing API response from ({url}): {ex.Message}\r\n{responseContent}", ex);
}Alternatively, use LoadIntoBufferAsync() before deserialization to allow multiple reads.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try | |
| { | |
| var apiResponse = await response.Content.ReadFromJsonAsync<TResponse>(cancellationToken: cancellationToken); | |
| if (apiResponse is null || apiResponse.Data is null || apiResponse.Data.Count == 0) | |
| { | |
| throw new ApiException($"No data returned from API ({url})"); | |
| } | |
| if (apiResponse is null || apiResponse.Data is null || apiResponse.Data.Count == 0) | |
| return apiResponse; | |
| } | |
| catch (Exception ex) when (ex is not ApiException) | |
| { | |
| throw new ApiException($"No data returned from API ({url})"); | |
| var responseContent = await response.Content.ReadAsStringAsync(cancellationToken); | |
| throw new ApiException($"Error deserializing API response from ({url}): {ex.Message}\r\n{responseContent}", ex); | |
| } | |
| var responseContent = await response.Content.ReadAsStringAsync(cancellationToken); | |
| try | |
| { | |
| var apiResponse = JsonSerializer.Deserialize<TResponse>(responseContent); | |
| if (apiResponse is null || apiResponse.Data is null || apiResponse.Data.Count == 0) | |
| { | |
| throw new ApiException($"No data returned from API ({url})"); | |
| } | |
| return apiResponse; | |
| } | |
| catch (Exception ex) when (ex is not ApiException) | |
| { | |
| throw new ApiException($"Error deserializing API response from ({url}): {ex.Message}\r\n{responseContent}", ex); | |
| } |
🤖 Prompt for AI Agents
In src/YoloAbstractions/Extensions/HttpClientExtensions.cs around lines 36 to
51, the catch attempts to read response.Content after ReadFromJsonAsync may have
already consumed the non-seekable stream, so the fallback ReadAsStringAsync will
be empty; fix by buffering the content before deserialization (e.g., call
response.Content.LoadIntoBufferAsync() or read
response.Content.ReadAsStringAsync() into a local string variable and then
deserialize from that string) so the content can be inspected on error, and use
that buffered string in the ApiException message.
Summary by CodeRabbit
Chores
Documentation
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.