Repository navigation
Added entity for currency - #158
Conversation
WalkthroughIntroduces persistent, database-backed currency collections and an HTTP currency fetcher. Currency entities move to Domain, the currency service now caches and stores daily collections, DI and background job names updated, EF Core mapping and migration added, and tests adapted to the new flow. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant CurrencyService
participant MemCache as "Memory Cache"
participant DB as Database
participant HttpService as "ICurrenciesHttpService / CurrenciesHttpService"
participant External as "External API"
rect rgb(200,220,240)
Note over Client,CurrencyService: First request (cache miss, no DB)
Client->>CurrencyService: GetAllCurrenciesAsync()
CurrencyService->>MemCache: Try get cached collection
MemCache-->>CurrencyService: null
CurrencyService->>DB: Query latest CurrenciesCollection
DB-->>CurrencyService: null
CurrencyService->>HttpService: GetCurrenciesAsync()
HttpService->>External: HTTP fetch (XML)
External-->>HttpService: XML response
HttpService-->>CurrencyService: Dictionary<Currency, CurrencyContent>
CurrencyService->>DB: Save new CurrenciesCollection
DB-->>CurrencyService: persisted entity
CurrencyService->>MemCache: Cache collection
CurrencyService-->>Client: Return currencies
end
rect rgb(220,240,200)
Note over Client,MemCache: Subsequent request (cache hit)
Client->>CurrencyService: GetAllCurrenciesAsync()
CurrencyService->>MemCache: Get cached collection
MemCache-->>CurrencyService: CurrenciesCollection
CurrencyService-->>Client: Return cached currencies
end
rect rgb(240,220,200)
Note over Scheduler,CurrencyService: Scheduled refetch
Scheduler->>CurrencyService: RefetchServiceCurrenciesAsync()
CurrencyService->>HttpService: GetCurrenciesAsync()
HttpService->>External: HTTP fetch
External-->>HttpService: XML response
HttpService-->>CurrencyService: Dictionary<Currency, CurrencyContent>
CurrencyService->>DB: Save new CurrenciesCollection (new date)
CurrencyService->>MemCache: Clear cache
CurrencyService-->>Scheduler: Complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Domain/Entities/Currencies/CurrencyContent.cs (1)
20-35: Fix ArgumentException parameter order.The
ArgumentExceptionconstructor parameters are reversed. The correct signature isArgumentException(string message, string paramName), but the code passes the invalid value as the message and the property name as paramName.🔎 Proposed fix
public CurrencyContent( string value, string currency, string pubDate) : this( !string.IsNullOrEmpty(value) ? double.Parse(value, CultureInfo.InvariantCulture) : - throw new ArgumentException(value, nameof(Value)), + throw new ArgumentException($"Value cannot be null or empty.", nameof(value)), !string.IsNullOrEmpty(currency) ? currency.ToEnum<Currency>() : - throw new ArgumentException(currency, nameof(currency)), + throw new ArgumentException($"Currency cannot be null or empty.", nameof(currency)), !string.IsNullOrEmpty(pubDate) ? DateTime.ParseExact(pubDate, "dd.MM.yyyy", CultureInfo.InvariantCulture) : - throw new ArgumentException(pubDate, nameof(PubDate))) + throw new ArgumentException($"PubDate cannot be null or empty.", nameof(pubDate))) { }
🧹 Nitpick comments (8)
CLAUDE.md (2)
5-7: Format bare URL with markdown link syntax.Line 7 contains a bare URL that should be wrapped in markdown link syntax for proper rendering and linting compliance.
🔎 Proposed fix
-**Repository:** https://github.com/techinterview-space/web-api +**Repository:** [github.com/techinterview-space/web-api](https://github.com/techinterview-space/web-api)
65-79: Minor wording refinement for formality.The phrase "fix the issues" on line 78 could be more formally expressed as "resolve the issues" for a more professional tone in the documentation, though "fix" is commonly acceptable in technical guidelines.
🔎 Proposed refinement
-If the build fails, fix the issues before considering the task complete. +If the build fails, resolve the issues before considering the task complete.src/TestUtils/Services/CurrencyServiceFake.cs (1)
50-53: Align test fake behavior with CurrenciesServiceFake.The method throws
NotImplementedException, which will cause test failures if any test calls this method. The similar fake inCurrenciesServiceFake.cs(lines 65-69) returnsTask.CompletedTaskinstead, which is more appropriate for a test double.🔎 Proposed fix to align with CurrenciesServiceFake
public Task RefetchServiceCurrenciesAsync(CancellationToken cancellationToken) { - throw new System.NotImplementedException(); + return Task.CompletedTask; }src/Infrastructure/Currencies/ICurrenciesHttpService.cs (1)
1-9: LGTM!The interface is well-designed with a clear single responsibility for fetching currency data via HTTP. The method signature appropriately accepts a
CancellationTokenand returns a list of currency content.Consider adding XML documentation comments to describe the interface and method purpose:
namespace Infrastructure.Currencies; +/// <summary> +/// Service for fetching currency exchange rates from external HTTP sources. +/// </summary> public interface ICurrenciesHttpService { + /// <summary> + /// Retrieves the current currency exchange rates from the external service. + /// </summary> + /// <param name="cancellationToken">Cancellation token for the async operation.</param> + /// <returns>A list of currency content with exchange rates.</returns> Task<List<CurrencyContent>> GetCurrenciesAsync( CancellationToken cancellationToken); }src/InfrastructureTests/Services/CurrenciesHttpServiceTests.cs (1)
399-413: Consider verifying currency values, not just presence.The test validates that the correct currency types are returned but doesn't verify the actual exchange rate values parsed from the XML. Consider adding assertions to check that at least one currency has the expected value to ensure the XML parsing logic is working correctly.
Example enhancement
Assert.Equal(existingCurrencies.Count, currencies.Count); foreach (var currency in currencies) { Assert.Contains(currency.Currency, existingCurrencies); } + + // Verify at least one currency value is parsed correctly + var usdCurrency = currencies.First(c => c.Currency == Currency.USD); + Assert.Equal(446.89, usdCurrency.Value);src/TestUtils/Fakes/CurrenciesCollectionFake.cs (1)
36-42: Remove unnecessary cast in Please method.On Line 39, the explicit cast to
(CurrenciesCollection)thisis unnecessary sinceCurrenciesCollectionFakealready inherits fromCurrenciesCollection. The type is implicitly compatible.🔎 Proposed fix
public CurrenciesCollection Please( InMemoryDatabaseContext context) { - var entry = context.Add((CurrenciesCollection)this); + var entry = context.Add(this); context.SaveChanges(); return entry.Entity; }src/Domain/Entities/Currencies/CurrenciesCollection.cs (1)
26-26: Consider using OnCreate() method from HasDatesBase.Line 26 directly sets
CreatedAtandUpdatedAt, bypassing theOnCreate()method defined inHasDatesBase. This may be intentional, but using the base class method would be more consistent with the inheritance pattern and easier to maintain if timestamp logic changes.Alternative approach
Id = Guid.NewGuid(); CurrencyDate = currencies[0].PubDate; - CreatedAt = UpdatedAt = DateTimeOffset.UtcNow; + OnCreate(DateTimeOffset.UtcNow);src/Infrastructure/Currencies/CurrencyService.cs (1)
85-118: Consider adding exception handling for HTTP service failures.If
_currenciesHttpService.GetCurrenciesAsyncthrows (network timeout, service unavailable), the exception propagates to callers. WhileGetAllCurrenciesAsynchas a fallback path,RefetchServiceCurrenciesAsyncwill fail entirely, potentially causing the background job to crash.Consider wrapping the HTTP call or documenting that callers must handle exceptions:
🔎 Proposed enhancement with exception handling
if (existingEntity is null) { - var currencies = await _currenciesHttpService.GetCurrenciesAsync(cancellationToken); - if (currencies.Count == 0) + List<CurrencyContent> currencies; + try + { + currencies = await _currenciesHttpService.GetCurrenciesAsync(cancellationToken); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Failed to fetch currencies from HTTP service"); + return null; + } + + if (currencies.Count == 0) { return null; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/Infrastructure/Migrations/20260102102556_CurrencyCollectionEntityAdded.Designer.csis excluded by!**/*.Designer.cs,!**/Migrations/*.Designer.cs
📒 Files selected for processing (25)
CLAUDE.mdsrc/Domain/Entities/Currencies/CurrenciesCollection.cssrc/Domain/Entities/Currencies/CurrencyContent.cssrc/Domain/Entities/Currencies/KztCurrencyContent.cssrc/Infrastructure/Currencies/Contracts/ICurrencyService.cssrc/Infrastructure/Currencies/CurrenciesHttpService.cssrc/Infrastructure/Currencies/CurrencyService.cssrc/Infrastructure/Currencies/ICurrenciesHttpService.cssrc/Infrastructure/Database/Config/CurrenciesCollectionEntityConfiguration.cssrc/Infrastructure/Database/DatabaseContext.cssrc/Infrastructure/Migrations/20260102102556_CurrencyCollectionEntityAdded.cssrc/Infrastructure/Migrations/DatabaseContextModelSnapshot.cssrc/InfrastructureTests/Mocks/HttpClientFactoryMock.cssrc/InfrastructureTests/Services/CurrenciesHttpServiceTests.cssrc/InfrastructureTests/Services/CurrencyServiceTests.cssrc/TestUtils/Fakes/CurrenciesCollectionFake.cssrc/TestUtils/Mocks/CurrenciesServiceFake.cssrc/TestUtils/Services/CurrencyServiceFake.cssrc/Web.Api/Features/Admin/AdminToolsController.cssrc/Web.Api/Features/BackgroundJobs/RefetchServiceCurrenciesJob.cssrc/Web.Api/Features/Salaries/GetSalariesChart/Charts/SalariesChartResponse.cssrc/Web.Api/Features/Salaries/GetSalariesChart/GetSalariesChartHandler.cssrc/Web.Api/Setup/ScheduleConfig.cssrc/Web.Api/Setup/ServiceRegistration.cssrc/techinterview.sln
🧰 Additional context used
📓 Path-based instructions (4)
**/*.cs
⚙️ CodeRabbit configuration file
**/*.cs: Review C# code for:
- SOLID principles and clean architecture adherence
- Proper async/await usage and avoiding async void
- Null safety and proper use of nullable reference types
- Dependency injection best practices
- Exception handling patterns
- Memory management and IDisposable implementation
- LINQ query optimization
Files:
src/Web.Api/Features/Salaries/GetSalariesChart/GetSalariesChartHandler.cssrc/Infrastructure/Currencies/ICurrenciesHttpService.cssrc/Web.Api/Setup/ServiceRegistration.cssrc/Web.Api/Features/Salaries/GetSalariesChart/Charts/SalariesChartResponse.cssrc/Web.Api/Features/Admin/AdminToolsController.cssrc/TestUtils/Services/CurrencyServiceFake.cssrc/Infrastructure/Currencies/CurrenciesHttpService.cssrc/InfrastructureTests/Mocks/HttpClientFactoryMock.cssrc/Infrastructure/Migrations/20260102102556_CurrencyCollectionEntityAdded.cssrc/Infrastructure/Database/DatabaseContext.cssrc/InfrastructureTests/Services/CurrenciesHttpServiceTests.cssrc/TestUtils/Fakes/CurrenciesCollectionFake.cssrc/Domain/Entities/Currencies/KztCurrencyContent.cssrc/Infrastructure/Database/Config/CurrenciesCollectionEntityConfiguration.cssrc/Web.Api/Features/BackgroundJobs/RefetchServiceCurrenciesJob.cssrc/Web.Api/Setup/ScheduleConfig.cssrc/Infrastructure/Currencies/CurrencyService.cssrc/Infrastructure/Currencies/Contracts/ICurrencyService.cssrc/Infrastructure/Migrations/DatabaseContextModelSnapshot.cssrc/TestUtils/Mocks/CurrenciesServiceFake.cssrc/InfrastructureTests/Services/CurrencyServiceTests.cssrc/Domain/Entities/Currencies/CurrenciesCollection.cssrc/Domain/Entities/Currencies/CurrencyContent.cs
**/Services/**/*.cs
⚙️ CodeRabbit configuration file
**/Services/**/*.cs: Review service layer for:
- Single responsibility principle
- Proper abstraction through interfaces
- Transaction management
- Business logic encapsulation
Files:
src/TestUtils/Services/CurrencyServiceFake.cssrc/InfrastructureTests/Services/CurrenciesHttpServiceTests.cssrc/InfrastructureTests/Services/CurrencyServiceTests.cs
**/Migrations/**/*.cs
⚙️ CodeRabbit configuration file
**/Migrations/**/*.cs: Review EF Core migrations for:
- Data integrity preservation
- Proper index creation
- Reversible migrations where possible
Files:
src/Infrastructure/Migrations/20260102102556_CurrencyCollectionEntityAdded.cssrc/Infrastructure/Migrations/DatabaseContextModelSnapshot.cs
**/*Tests.cs
⚙️ CodeRabbit configuration file
**/*Tests.cs: Review test files for:
- Adequate test coverage
- Proper use of xUnit/NUnit/MSTest assertions
- Mock objects with Moq or NSubstitute
- Arrange-Act-Assert pattern
- Integration test isolation
Files:
src/InfrastructureTests/Services/CurrenciesHttpServiceTests.cssrc/InfrastructureTests/Services/CurrencyServiceTests.cs
🧬 Code graph analysis (10)
src/Web.Api/Features/Salaries/GetSalariesChart/GetSalariesChartHandler.cs (2)
src/Web.Api/Features/Salaries/GetSalaries/GetSalariesPaginatedQuery.cs (1)
Domain(37-48)src/Web.Api/Features/Salaries/SalariesChartQueryParamsBase.cs (1)
Domain(37-48)
src/TestUtils/Services/CurrencyServiceFake.cs (3)
src/Infrastructure/Currencies/Contracts/ICurrencyService.cs (3)
Task(8-10)Task(12-13)Task(15-16)src/Infrastructure/Currencies/CurrencyService.cs (4)
Task(32-39)Task(41-70)Task(72-83)Task(85-118)src/TestUtils/Mocks/CurrenciesServiceFake.cs (3)
Task(42-48)Task(50-57)Task(59-63)
src/InfrastructureTests/Services/CurrenciesHttpServiceTests.cs (3)
src/Infrastructure/Currencies/CurrenciesHttpService.cs (3)
Task(26-57)CurrenciesHttpService(10-58)CurrenciesHttpService(16-24)src/Infrastructure/Currencies/ICurrenciesHttpService.cs (1)
Task(7-8)src/Domain/Extensions/EnumHelper.cs (1)
EnumHelper(11-133)
src/TestUtils/Fakes/CurrenciesCollectionFake.cs (4)
src/Domain/Entities/Currencies/CurrenciesCollection.cs (4)
CurrenciesCollection(8-56)CurrenciesCollection(16-32)CurrenciesCollection(53-55)List(34-51)src/Domain/Entities/Currencies/CurrencyContent.cs (3)
CurrencyContent(11-18)CurrencyContent(20-35)CurrencyContent(37-45)src/Domain/Entities/Currencies/KztCurrencyContent.cs (1)
KztCurrencyContent(8-15)src/Infrastructure/Database/DatabaseContext.cs (1)
SaveChanges(183-187)
src/Infrastructure/Database/Config/CurrenciesCollectionEntityConfiguration.cs (1)
src/Domain/Entities/Currencies/CurrenciesCollection.cs (3)
CurrenciesCollection(8-56)CurrenciesCollection(16-32)CurrenciesCollection(53-55)
src/Web.Api/Setup/ScheduleConfig.cs (1)
src/Web.Api/Features/BackgroundJobs/RefetchServiceCurrenciesJob.cs (2)
RefetchServiceCurrenciesJob(8-25)RefetchServiceCurrenciesJob(12-18)
src/Infrastructure/Currencies/CurrencyService.cs (5)
src/Infrastructure/Database/DatabaseContext.cs (8)
DatabaseContext(25-236)DatabaseContext(27-30)Task(102-114)Task(116-128)Task(130-136)Task(148-159)Task(161-181)Task(189-194)src/Infrastructure/Currencies/Contracts/ICurrencyService.cs (3)
Task(8-10)Task(12-13)Task(15-16)src/Domain/Entities/Currencies/CurrencyContent.cs (3)
CurrencyContent(11-18)CurrencyContent(20-35)CurrencyContent(37-45)src/Domain/Entities/Currencies/CurrenciesCollection.cs (4)
CurrenciesCollection(8-56)CurrenciesCollection(16-32)CurrenciesCollection(53-55)List(34-51)src/Domain/Entities/Currencies/KztCurrencyContent.cs (1)
KztCurrencyContent(8-15)
src/TestUtils/Mocks/CurrenciesServiceFake.cs (3)
src/Infrastructure/Currencies/Contracts/ICurrencyService.cs (3)
Task(8-10)Task(12-13)Task(15-16)src/Infrastructure/Currencies/CurrencyService.cs (4)
Task(32-39)Task(41-70)Task(72-83)Task(85-118)src/TestUtils/Services/CurrencyServiceFake.cs (3)
Task(29-35)Task(37-43)Task(45-48)
src/InfrastructureTests/Services/CurrencyServiceTests.cs (4)
src/Infrastructure/Currencies/Contracts/ICurrencyService.cs (3)
Task(8-10)Task(12-13)Task(15-16)src/Infrastructure/Currencies/CurrencyService.cs (7)
Task(32-39)Task(41-70)Task(72-83)Task(85-118)List(120-126)CurrencyService(11-127)CurrencyService(20-30)src/TestUtils/Fakes/CurrenciesCollectionFake.cs (3)
List(19-34)CurrenciesCollectionFake(9-43)CurrenciesCollectionFake(11-17)src/Domain/Entities/Currencies/CurrencyContent.cs (3)
CurrencyContent(11-18)CurrencyContent(20-35)CurrencyContent(37-45)
src/Domain/Entities/Currencies/CurrenciesCollection.cs (5)
src/TestUtils/Fakes/CurrenciesCollectionFake.cs (2)
CurrenciesCollection(36-42)List(19-34)src/Domain/Entities/HasDatesBase.cs (1)
HasDatesBase(6-21)src/Infrastructure/Currencies/CurrencyService.cs (1)
List(120-126)src/Domain/Entities/Currencies/CurrencyContent.cs (3)
CurrencyContent(11-18)CurrencyContent(20-35)CurrencyContent(37-45)src/Domain/Entities/Currencies/KztCurrencyContent.cs (1)
KztCurrencyContent(8-15)
🪛 LanguageTool
CLAUDE.md
[style] ~78-~78: Consider using a different verb for a more formal wording.
Context: ...piles successfully If the build fails, fix the issues before considering the task ...
(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
7-7: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (30)
src/Web.Api/Features/Salaries/GetSalariesChart/GetSalariesChartHandler.cs (1)
6-6: Import correctly aligns with PR changes.The added using directive is necessary to resolve the
CurrencyContentandCurrencytypes instantiated inGetCurrenciesAsync. The import placement follows C# conventions and integrates cleanly with the existing structure.CLAUDE.md (1)
82-276: Well-structured and comprehensive AI agent guidelines.This documentation provides clear, actionable guidance across all key areas: migrations (appropriately restricting EF modifications), testing patterns (xUnit, AAA structure, test naming), test utilities (mocks/fakes/builders), and project-specific notes. The summary checklist is a helpful verification aid. Excellent addition to the repository.
src/techinterview.sln (1)
28-28: LGTM!The CLAUDE.md reference is properly added to SolutionItems with correct relative path syntax, consistent with existing solution item entries.
src/InfrastructureTests/Mocks/HttpClientFactoryMock.cs (1)
28-31: LGTM! Clean variable rename.The variable rename from
httpClienttoclientis a minor refactoring with no functional impact. The mock setup remains correct.src/Domain/Entities/Currencies/KztCurrencyContent.cs (1)
1-4: LGTM! Proper layering with Domain entities.The namespace change from
Infrastructure.Currencies.ContractstoDomain.Entities.Currenciescorrectly moves the entity to the Domain layer, adhering to Clean Architecture principles. Theusing System;directive is required for theDateTimeparameter.src/Web.Api/Features/Admin/AdminToolsController.cs (1)
5-5: LGTM! Necessary using directive for Domain entities.The added
using Domain.Entities.Currencies;directive is required for theCurrencyContentreturn type on line 39. This correctly maintains separation between Domain entities and Infrastructure service contracts.src/Web.Api/Features/Salaries/GetSalariesChart/Charts/SalariesChartResponse.cs (1)
4-4: LGTM! Required for Domain entity types.The
using Domain.Entities.Currencies;directive is necessary for theCurrencyContenttype used throughout this response model (lines 30, 65, 190, 210). This maintains proper separation between Domain entities and Infrastructure services.src/Web.Api/Setup/ServiceRegistration.cs (1)
59-59: DI registration is correctly implemented.The
CurrencyServiceconstructor has been properly updated to depend onICurrenciesHttpService(line 21), and the transient lifetime registration is appropriate for HTTP services. The interface contract is well-defined with proper async/await patterns and CancellationToken handling.src/Infrastructure/Database/DatabaseContext.cs (2)
4-4: LGTM!The using directive is appropriately added to support the new CurrenciesCollection entity.
100-101: LGTM!The DbSet is properly declared and follows the existing conventions in the codebase. The configuration will be automatically applied through
ApplyConfigurationsFromAssemblyon line 199.src/TestUtils/Services/CurrencyServiceFake.cs (1)
6-6: LGTM!The using directive is appropriately added to support the CurrencyContent type that has been moved to Domain.Entities.Currencies.
src/TestUtils/Mocks/CurrenciesServiceFake.cs (2)
6-6: LGTM!The using directive is appropriately added to support the CurrencyContent type that has been moved to Domain.Entities.Currencies.
65-69: LGTM!The method is appropriately renamed to match the interface change, and the implementation correctly returns
Task.CompletedTask, which is the proper approach for a test fake.src/Infrastructure/Currencies/Contracts/ICurrencyService.cs (2)
1-2: LGTM!The using directives are appropriately added to support the currency-related types used in the interface methods.
15-16: LGTM!The method rename from
ResetCacheAsynctoRefetchServiceCurrenciesAsyncis more descriptive and better communicates the intent. The removal ofGetCurrenciesAsync(List<Currency>)aligns with the new collection-based caching approach described in the PR objectives, where currency data is retrieved throughGetAllCurrenciesAsyncwith subsequent filtering.src/Web.Api/Setup/ScheduleConfig.cs (1)
19-19: LGTM! Proper job registration and scheduling configuration.The transient registration and scheduling configuration are appropriate. The use of
RunOnceAtStart()ensures currency data is available immediately on application startup, andPreventOverlapping()prevents concurrent executions.Also applies to: 37-40
src/Infrastructure/Database/Config/CurrenciesCollectionEntityConfiguration.cs (1)
19-21: Excellent use of unique index for daily caching.The unique index on
CurrencyDateeffectively enforces the business rule of one currency collection per day, preventing duplicate entries and supporting the daily caching strategy.src/Infrastructure/Migrations/20260102102556_CurrencyCollectionEntityAdded.cs (1)
14-34: LGTM! Well-structured and reversible migration.The migration properly creates the table with appropriate column types, primary key, and unique index. The use of
timestamp with time zonefor date fields follows PostgreSQL best practices. The migration is fully reversible with a properDownmethod.src/Infrastructure/Migrations/DatabaseContextModelSnapshot.cs (1)
246-270: LGTM! Model snapshot correctly reflects the new entity.The snapshot accurately captures the
CurrenciesCollectionentity configuration, including the unique index onCurrencyDateand proper column mappings. This auto-generated file correctly represents the database schema state.src/Web.Api/Features/BackgroundJobs/RefetchServiceCurrenciesJob.cs (1)
8-24: LGTM! Job refactoring aligns with PR objectives.The renamed class and updated method call properly reflect the refactored currency fetching architecture. The lack of exception handling in
ExecuteAsyncis acceptable as exceptions will propagate to the job framework for retry/failure handling.src/Infrastructure/Currencies/CurrenciesHttpService.cs (1)
38-38: No compilation issues—project targets .NET 10.0.The project targets
net10.0, which fully supports theGetStringAsync(string, CancellationToken)overload introduced in .NET 5. This code will compile and run without issues.src/InfrastructureTests/Services/CurrencyServiceTests.cs (4)
19-66: LGTM!The test correctly verifies that when no database records exist,
GetCurrencyOrNullAsyncfetches currencies via HTTP, persists them, and returns the requested currency. The mock verification and assertions are comprehensive.
122-161: LGTM!The test correctly validates that
RefetchServiceCurrenciesAsynccreates a new database record when the HTTP service returns currencies. Assertions verify record creation and HTTP service invocation count.
163-194: LGTM!Correctly verifies that no database record is created when the HTTP service returns an empty list.
244-287: LGTM!Correctly verifies that when a record for today already exists, no new record is created and the HTTP service is not called.
src/Infrastructure/Currencies/CurrencyService.cs (5)
11-30: LGTM!Constructor properly implements dependency injection with all dependencies assigned to readonly fields. Using
DatabaseContextdirectly is acceptable for EF Core patterns.
32-39: LGTM!Clean delegation pattern. The method correctly returns
nullwhen the requested currency is not found, matching the method name contract.
41-70: LGTM!The caching strategy with 6-hour expiration and fallback chain (DB → HTTP → default) is well-designed. The cache factory correctly handles all scenarios.
72-83: LGTM!The order of operations (recreate → clear cache) is correct. If recreation fails with an exception, the cache remains valid. Cache invalidation after successful recreation ensures subsequent reads get fresh data.
120-126: LGTM!Simple fallback providing KZT as the default currency when no data is available from DB or HTTP.
| public CurrenciesCollection( | ||
| List<CurrencyContent> currencies) | ||
| { | ||
| if (currencies.Count == 0) | ||
| { | ||
| throw new InvalidOperationException("Currencies collection must have at least one currency content."); | ||
| } |
There was a problem hiding this comment.
Add null check for currencies parameter.
The constructor validates that the list is not empty but doesn't check for null. This could result in a NullReferenceException instead of a clear ArgumentNullException.
🔎 Proposed fix
public CurrenciesCollection(
List<CurrencyContent> currencies)
{
+ if (currencies == null)
+ {
+ throw new ArgumentNullException(nameof(currencies));
+ }
+
if (currencies.Count == 0)
{
throw new InvalidOperationException("Currencies collection must have at least one currency content.");
}As per coding guidelines for null safety.
🤖 Prompt for AI Agents
In src/Domain/Entities/Currencies/CurrenciesCollection.cs around lines 16 to 22,
the constructor checks for an empty list but does not validate the currencies
parameter for null; add an explicit null check at the start of the constructor
and throw an ArgumentNullException when currencies is null, then keep the
existing empty-list validation (or combine both checks) so callers receive a
clear, typed exception for null inputs.
| } | ||
|
|
||
| Id = Guid.NewGuid(); | ||
| CurrencyDate = currencies[0].PubDate; |
There was a problem hiding this comment.
Validate consistent PubDate across all currencies.
Line 25 assumes all currencies in the list share the same PubDate by using only the first element's date. If the input contains currencies with different dates, this could lead to data inconsistency. Consider validating that all currencies have the same PubDate or documenting this assumption clearly.
🔎 Proposed fix
Id = Guid.NewGuid();
CurrencyDate = currencies[0].PubDate;
+
+ if (currencies.Any(c => c.PubDate != CurrencyDate))
+ {
+ throw new InvalidOperationException("All currencies must have the same publication date.");
+ }
+
CreatedAt = UpdatedAt = DateTimeOffset.UtcNow;🤖 Prompt for AI Agents
In src/Domain/Entities/Currencies/CurrenciesCollection.cs around line 25, the
code currently assigns CurrencyDate = currencies[0].PubDate which assumes all
items share the same PubDate; instead validate that every currency.PubDate
equals currencies[0].PubDate and if any differ throw an ArgumentException (or a
domain-specific exception) with a clear message, or if differing dates are
acceptable decide and document the chosen behavior (e.g., use the
earliest/latest date) and implement that deterministic selection; ensure the
validation runs before setting CurrencyDate and include a unit test to cover
mixed-date input.
| catch (Exception e) | ||
| { | ||
| _logger.LogError( | ||
| e, | ||
| "Error while getting currencies from {Url}. Message {Message}", | ||
| currenciesUrl, | ||
| e.Message); | ||
|
|
||
| return new List<CurrencyContent>(0); | ||
| } |
There was a problem hiding this comment.
Reconsider silent failure pattern for external service calls.
Catching all exceptions and returning an empty list masks failures from consumers. This makes it difficult to distinguish between "no currencies available" and "service failure". Consider:
- Letting exceptions propagate for transient failures (network issues, timeouts) so retry logic can be applied at a higher level
- Returning a result type that distinguishes success from failure
- At minimum, including a more specific return value or throwing a custom exception for non-transient errors
This is especially important for background jobs that should retry on transient failures.
Based on coding guidelines for exception handling patterns.
🤖 Prompt for AI Agents
In src/Infrastructure/Currencies/CurrenciesHttpService.cs around lines 47–56,
the current catch-all swallows all exceptions and returns an empty list which
hides failures; instead, log the error and rethrow (or throw a specific
ExternalServiceException wrapping the original exception) so callers can handle
retries for transient failures, or change the method signature to return a
Result/Outcome type that clearly distinguishes success from failure; implement
one of these approaches: remove the blanket return new List, keep the logging,
and rethrow the original exception (or throw a custom exception with context) so
higher-level retry/handling can occur, or convert the method to return a
Result<T> and return a failed result with the logged error.
| builder | ||
| .Property(x => x.Currencies) | ||
| .HasJsonConversion(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider non-nullable constraint for Currencies column.
The Currencies property is stored as JSON and appears nullable in the database schema. Since the domain entity's constructor ensures at least one currency is present and throws if the collection is empty, consider adding a non-nullable constraint to enforce data integrity at the database level.
🔎 Proposed fix
builder
.Property(x => x.Currencies)
+ .IsRequired()
.HasJsonConversion();📝 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.
| builder | |
| .Property(x => x.Currencies) | |
| .HasJsonConversion(); | |
| builder | |
| .Property(x => x.Currencies) | |
| .IsRequired() | |
| .HasJsonConversion(); |
🤖 Prompt for AI Agents
In src/Infrastructure/Database/Config/CurrenciesCollectionEntityConfiguration.cs
around lines 15 to 17, the Currencies JSON column is currently configured as
nullable but the domain guarantees at least one currency; make the column
non-nullable by updating the EF configuration to mark the property as required
(e.g., append IsRequired() to the HasJsonConversion chain or equivalent), add a
migration to change the column to NOT NULL (handling existing nulls by
backfilling them to a valid JSON array or failing the migration with a clear
remediation step), and run the migration so the database enforces the invariant.
| { "Currencies:Url", "https://currencies.com/rates_all.xml" }, | ||
| }); | ||
|
|
||
| using var mockedCache = Create.MockedMemoryCache(); |
There was a problem hiding this comment.
Remove unused mocked cache variable.
The mockedCache variable is created but never used in the test. Since CurrenciesHttpService doesn't appear to use IMemoryCache in its constructor, this is dead code that should be removed.
🔎 Proposed fix
- using var mockedCache = Create.MockedMemoryCache();
var mockHttpClientFactory = new HttpClientFactoryMock(CurrenciesXml);📝 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.
| using var mockedCache = Create.MockedMemoryCache(); | |
| var mockHttpClientFactory = new HttpClientFactoryMock(CurrenciesXml); |
🤖 Prompt for AI Agents
In src/InfrastructureTests/Services/CurrenciesHttpServiceTests.cs around line
391, the test declares and assigns using var mockedCache =
Create.MockedMemoryCache(); but never uses it; remove this unused mockedCache
declaration (the using line) so the test no longer contains dead code and
compiler warnings.
| [Fact] | ||
| public async Task GetAllCurrenciesAsync_NoSavedDatabaseRecord_ReturnsAllCurrencies() | ||
| { | ||
| await using var context = new InMemoryDatabaseContext(); | ||
| var now = DateTime.UtcNow; | ||
| var currenciesHttpService = new Mock<ICurrenciesHttpService>(); | ||
| currenciesHttpService | ||
| .Setup(x => x.GetCurrenciesAsync(It.IsAny<CancellationToken>())) | ||
| .ReturnsAsync(new List<Domain.Entities.Currencies.CurrencyContent> | ||
| { | ||
| new ( | ||
| 400, | ||
| Currency.USD, | ||
| now), | ||
| new ( | ||
| 500, | ||
| Currency.EUR, | ||
| now), | ||
| }); | ||
|
|
||
| using var mockedCache = Create.MockedMemoryCache(); | ||
|
|
||
| var currencyService = new CurrencyService( | ||
| currenciesHttpService.Object, | ||
| mockedCache, | ||
| new Mock<ILogger<CurrencyService>>().Object, | ||
| context); | ||
|
|
||
| var collection1 = new CurrenciesCollectionFake(DateTime.UtcNow.AddDays(-2)).Please(context); | ||
| var collection2 = new CurrenciesCollectionFake(DateTime.UtcNow.AddDays(-3)).Please(context); | ||
| var collection3 = new CurrenciesCollectionFake(DateTime.UtcNow.AddDays(-4)).Please(context); | ||
|
|
||
| Assert.Equal(3, context.CurrencyCollections.Count()); | ||
|
|
||
| context.ChangeTracker.Clear(); | ||
| var currencies = await currencyService.GetAllCurrenciesAsync(default); | ||
|
|
||
| Assert.NotNull(currencies); | ||
| Assert.Equal(3, currencies.Count); | ||
| Assert.Contains(currencies, x => x.Currency == Currency.USD); | ||
| Assert.Contains(currencies, x => x.Currency == Currency.EUR); | ||
| Assert.Contains(currencies, x => x.Currency == Currency.KZT); | ||
|
|
||
| Assert.Equal(collection1.CurrencyDate, currencies[0].PubDate); | ||
| Assert.NotEqual(collection2.CurrencyDate, currencies[0].PubDate); | ||
| Assert.NotEqual(collection3.CurrencyDate, currencies[0].PubDate); | ||
|
|
||
| // Assert | ||
| currenciesHttpService | ||
| .Verify( | ||
| x => x.GetCurrenciesAsync(It.IsAny<CancellationToken>()), | ||
| Times.Never); | ||
| } |
There was a problem hiding this comment.
Misleading test name: test seeds records but name says "NoSavedDatabaseRecord".
The test name GetAllCurrenciesAsync_NoSavedDatabaseRecord_ReturnsAllCurrencies implies no database records exist, but lines 96-98 seed three CurrenciesCollectionFake records. The test actually verifies that existing DB records are returned without calling the HTTP service.
Consider renaming to reflect the actual scenario:
🔎 Proposed fix
[Fact]
- public async Task GetAllCurrenciesAsync_NoSavedDatabaseRecord_ReturnsAllCurrencies()
+ public async Task GetAllCurrenciesAsync_ExistingDatabaseRecords_ReturnsLatestCollectionWithoutHttpCall()
{📝 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.
| [Fact] | |
| public async Task GetAllCurrenciesAsync_NoSavedDatabaseRecord_ReturnsAllCurrencies() | |
| { | |
| await using var context = new InMemoryDatabaseContext(); | |
| var now = DateTime.UtcNow; | |
| var currenciesHttpService = new Mock<ICurrenciesHttpService>(); | |
| currenciesHttpService | |
| .Setup(x => x.GetCurrenciesAsync(It.IsAny<CancellationToken>())) | |
| .ReturnsAsync(new List<Domain.Entities.Currencies.CurrencyContent> | |
| { | |
| new ( | |
| 400, | |
| Currency.USD, | |
| now), | |
| new ( | |
| 500, | |
| Currency.EUR, | |
| now), | |
| }); | |
| using var mockedCache = Create.MockedMemoryCache(); | |
| var currencyService = new CurrencyService( | |
| currenciesHttpService.Object, | |
| mockedCache, | |
| new Mock<ILogger<CurrencyService>>().Object, | |
| context); | |
| var collection1 = new CurrenciesCollectionFake(DateTime.UtcNow.AddDays(-2)).Please(context); | |
| var collection2 = new CurrenciesCollectionFake(DateTime.UtcNow.AddDays(-3)).Please(context); | |
| var collection3 = new CurrenciesCollectionFake(DateTime.UtcNow.AddDays(-4)).Please(context); | |
| Assert.Equal(3, context.CurrencyCollections.Count()); | |
| context.ChangeTracker.Clear(); | |
| var currencies = await currencyService.GetAllCurrenciesAsync(default); | |
| Assert.NotNull(currencies); | |
| Assert.Equal(3, currencies.Count); | |
| Assert.Contains(currencies, x => x.Currency == Currency.USD); | |
| Assert.Contains(currencies, x => x.Currency == Currency.EUR); | |
| Assert.Contains(currencies, x => x.Currency == Currency.KZT); | |
| Assert.Equal(collection1.CurrencyDate, currencies[0].PubDate); | |
| Assert.NotEqual(collection2.CurrencyDate, currencies[0].PubDate); | |
| Assert.NotEqual(collection3.CurrencyDate, currencies[0].PubDate); | |
| // Assert | |
| currenciesHttpService | |
| .Verify( | |
| x => x.GetCurrenciesAsync(It.IsAny<CancellationToken>()), | |
| Times.Never); | |
| } | |
| [Fact] | |
| public async Task GetAllCurrenciesAsync_ExistingDatabaseRecords_ReturnsLatestCollectionWithoutHttpCall() | |
| { | |
| await using var context = new InMemoryDatabaseContext(); | |
| var now = DateTime.UtcNow; | |
| var currenciesHttpService = new Mock<ICurrenciesHttpService>(); | |
| currenciesHttpService | |
| .Setup(x => x.GetCurrenciesAsync(It.IsAny<CancellationToken>())) | |
| .ReturnsAsync(new List<Domain.Entities.Currencies.CurrencyContent> | |
| { | |
| new ( | |
| 400, | |
| Currency.USD, | |
| now), | |
| new ( | |
| 500, | |
| Currency.EUR, | |
| now), | |
| }); | |
| using var mockedCache = Create.MockedMemoryCache(); | |
| var currencyService = new CurrencyService( | |
| currenciesHttpService.Object, | |
| mockedCache, | |
| new Mock<ILogger<CurrencyService>>().Object, | |
| context); | |
| var collection1 = new CurrenciesCollectionFake(DateTime.UtcNow.AddDays(-2)).Please(context); | |
| var collection2 = new CurrenciesCollectionFake(DateTime.UtcNow.AddDays(-3)).Please(context); | |
| var collection3 = new CurrenciesCollectionFake(DateTime.UtcNow.AddDays(-4)).Please(context); | |
| Assert.Equal(3, context.CurrencyCollections.Count()); | |
| context.ChangeTracker.Clear(); | |
| var currencies = await currencyService.GetAllCurrenciesAsync(default); | |
| Assert.NotNull(currencies); | |
| Assert.Equal(3, currencies.Count); | |
| Assert.Contains(currencies, x => x.Currency == Currency.USD); | |
| Assert.Contains(currencies, x => x.Currency == Currency.EUR); | |
| Assert.Contains(currencies, x => x.Currency == Currency.KZT); | |
| Assert.Equal(collection1.CurrencyDate, currencies[0].PubDate); | |
| Assert.NotEqual(collection2.CurrencyDate, currencies[0].PubDate); | |
| Assert.NotEqual(collection3.CurrencyDate, currencies[0].PubDate); | |
| // Assert | |
| currenciesHttpService | |
| .Verify( | |
| x => x.GetCurrenciesAsync(It.IsAny<CancellationToken>()), | |
| Times.Never); | |
| } |
🤖 Prompt for AI Agents
In src/InfrastructureTests/Services/CurrencyServiceTests.cs around lines 68 to
120, the test method is misnamed — it seeds three CurrencyCollectionFake records
but is called GetAllCurrenciesAsync_NoSavedDatabaseRecord_ReturnsAllCurrencies
which implies an empty DB; rename the test to reflect the actual scenario (for
example
GetAllCurrenciesAsync_WithSavedDatabaseRecords_ReturnsAllCurrenciesAndDoesNotCallHttpService)
and update the method name declaration accordingly so the name matches the
seeded data and the assertion that the HTTP service is never called.
| [Fact] | ||
| public async Task RefetchServiceCurrenciesAsync_SomeDbRecordsForPrevousDate_NewRecordCreatedInDatabase() | ||
| { | ||
| // Arrange | ||
| await using var context = new InMemoryDatabaseContext(); | ||
| var now = DateTime.UtcNow; | ||
|
|
||
| var record1 = new CurrenciesCollectionFake(now.Date.AddDays(-1)).Please(context); | ||
| var record2 = new CurrenciesCollectionFake(now.Date.AddDays(-2)).Please(context); | ||
|
|
||
| var currenciesHttpService = new Mock<ICurrenciesHttpService>(); | ||
| currenciesHttpService | ||
| .Setup(x => x.GetCurrenciesAsync(It.IsAny<CancellationToken>())) | ||
| .ReturnsAsync(new List<Domain.Entities.Currencies.CurrencyContent> | ||
| { | ||
| new (400, Currency.USD, now), | ||
| new (500, Currency.EUR, now), | ||
| }); | ||
|
|
||
| using var mockedCache = Create.MockedMemoryCache(); | ||
|
|
||
| var currencyService = new CurrencyService( | ||
| currenciesHttpService.Object, | ||
| mockedCache, | ||
| new Mock<ILogger<CurrencyService>>().Object, | ||
| context); | ||
|
|
||
| Assert.Equal(2, context.CurrencyCollections.Count()); | ||
|
|
||
| // Act | ||
| await currencyService.RefetchServiceCurrenciesAsync(CancellationToken.None); | ||
|
|
||
| // Assert | ||
| Assert.Equal(3, context.CurrencyCollections.Count()); | ||
| var savedCollection = context.CurrencyCollections.Last(); | ||
|
|
||
| Assert.NotEqual(record1.CurrencyDate, savedCollection.CurrencyDate); | ||
| Assert.NotEqual(record2.CurrencyDate, savedCollection.CurrencyDate); | ||
|
|
||
| Assert.Equal(now.Date, savedCollection.CurrencyDate.Date); | ||
| Assert.Equal(2, savedCollection.Currencies.Count); | ||
|
|
||
| currenciesHttpService | ||
| .Verify( | ||
| x => x.GetCurrenciesAsync(It.IsAny<CancellationToken>()), | ||
| Times.Once); | ||
| } |
There was a problem hiding this comment.
Minor typo in test name.
Prevous should be Previous.
🔎 Proposed fix
[Fact]
- public async Task RefetchServiceCurrenciesAsync_SomeDbRecordsForPrevousDate_NewRecordCreatedInDatabase()
+ public async Task RefetchServiceCurrenciesAsync_SomeDbRecordsForPreviousDate_NewRecordCreatedInDatabase()
{🤖 Prompt for AI Agents
In src/InfrastructureTests/Services/CurrencyServiceTests.cs around lines 196 to
242, the test method name contains a typo: "Prevous" should be "Previous";
rename the method
RefetchServiceCurrenciesAsync_SomeDbRecordsForPrevousDate_NewRecordCreatedInDatabase
to
RefetchServiceCurrenciesAsync_SomeDbRecordsForPreviousDate_NewRecordCreatedInDatabase
and update any references/calls to that test name (if any) to match the new
spelling, ensuring the file compiles and tests run.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/InfrastructureTests/Services/CurrenciesHttpServiceTests.cs (1)
391-391: Remove unused mocked cache variable.The
mockedCachevariable is created but never used.CurrenciesHttpServicedoesn't useIMemoryCachein its constructor, so this is dead code.🔎 Proposed fix
- using var mockedCache = Create.MockedMemoryCache(); var mockHttpClientFactory = new HttpClientFactoryMock(CurrenciesXml);src/Infrastructure/Currencies/CurrenciesHttpService.cs (1)
48-57: Reconsider silent failure pattern for external service calls.Catching all exceptions and returning an empty dictionary masks failures from consumers. This prevents distinguishing between "no currencies available" and "service failure", and blocks retry logic for transient failures in background jobs.
Consider:
- Letting exceptions propagate for transient failures so retry logic can be applied at a higher level
- Returning a result type that distinguishes success from failure
- At minimum, throwing a custom exception for non-transient errors after logging
As per coding guidelines for exception handling patterns.
src/InfrastructureTests/Services/CurrencyServiceTests.cs (2)
75-134: Misleading test name: test seeds records but name says "NoSavedDatabaseRecord".The test name implies no database records exist, but lines 110-112 seed three
CurrenciesCollectionFakerecords. The test actually verifies that existing DB records are returned without calling the HTTP service.🔎 Proposed fix
[Fact] - public async Task GetAllCurrenciesAsync_NoSavedDatabaseRecord_ReturnsAllCurrencies() + public async Task GetAllCurrenciesAsync_ExistingDatabaseRecords_ReturnsLatestCollectionWithoutHttpCall() {
222-280: Minor typo in test name.
Prevousshould bePrevious.🔎 Proposed fix
[Fact] - public async Task RefetchServiceCurrenciesAsync_SomeDbRecordsForPrevousDate_NewRecordCreatedInDatabase() + public async Task RefetchServiceCurrenciesAsync_SomeDbRecordsForPreviousDate_NewRecordCreatedInDatabase() {src/Domain/Entities/Currencies/CurrenciesCollection.cs (2)
16-22: Add null check for currencies parameter.The constructor validates that the dictionary is not empty but doesn't check for null. This could result in a
NullReferenceExceptioninstead of a clearArgumentNullException.🔎 Proposed fix
public CurrenciesCollection( Dictionary<Currency, CurrencyContent> currencies) { + if (currencies == null) + { + throw new ArgumentNullException(nameof(currencies)); + } + if (currencies.Count == 0) { throw new InvalidOperationException("Currencies collection must have at least one currency content."); }As per coding guidelines for null safety.
25-25: Validate consistent PubDate across all currencies.Line 25 assumes all currencies share the same
PubDateby using only the first element's date. If the input contains currencies with different dates, this could lead to data inconsistency.🔎 Proposed fix
Id = Guid.NewGuid(); CurrencyDate = currencies.First().Value.PubDate; + + if (currencies.Values.Any(c => c.PubDate.Date != CurrencyDate.Date)) + { + throw new InvalidOperationException("All currencies must have the same publication date."); + } + CreatedAt = UpdatedAt = DateTimeOffset.UtcNow;
🧹 Nitpick comments (5)
src/Infrastructure/Services/AiServices/Custom/CustomOpenAiService.cs (3)
48-49: Consider removing redundant BaseAddress assignment.Setting
BaseAddressand then passing the full URL to theHttpRequestMessageis redundant. You can simplify by removing line 48, since line 49 already uses the full URL.🔎 Proposed simplification
- client.BaseAddress = new Uri(apiUrl); - var request = new HttpRequestMessage(HttpMethod.Post, apiUrl); + var request = new HttpRequestMessage(HttpMethod.Post, apiUrl);
46-56: Consider adding timeout for resilience.For external API calls, setting an explicit timeout prevents indefinite hangs and improves resilience. While the
CancellationTokenprovides some control, an explicit timeout is a recommended best practice.🔎 Example timeout configuration
var client = _httpClientFactory.CreateClient(); + client.Timeout = TimeSpan.FromSeconds(30); - client.BaseAddress = new Uri(apiUrl); var request = new HttpRequestMessage(HttpMethod.Post, apiUrl);
49-62: Consider usingusingstatements for proper resource disposal.Both
HttpRequestMessageandHttpResponseMessageimplementIDisposableand should be disposed to ensure proper cleanup of resources.🔎 Proposed resource management improvement
- var request = new HttpRequestMessage(HttpMethod.Post, apiUrl); - request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", GetBearer()); - request.Content = new StringContent( - JsonSerializer.Serialize(report), - Encoding.UTF8, - "application/json"); - - var response = await client.SendAsync(request, cancellationToken); - - responseContent = await response.Content.ReadAsStringAsync(cancellationToken); - if (response.IsSuccessStatusCode) - { - return responseContent; - } - - _logger.LogError( - "Failed request to OpenAI {Url}. Status {Status}, Response {Response}", - apiUrl, - response.StatusCode, - responseContent); + using var request = new HttpRequestMessage(HttpMethod.Post, apiUrl); + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", GetBearer()); + request.Content = new StringContent( + JsonSerializer.Serialize(report), + Encoding.UTF8, + "application/json"); + + using var response = await client.SendAsync(request, cancellationToken); + + responseContent = await response.Content.ReadAsStringAsync(cancellationToken); + if (response.IsSuccessStatusCode) + { + return responseContent; + } + + _logger.LogError( + "Failed request to OpenAI {Url}. Status {Status}, Response {Response}", + apiUrl, + response.StatusCode, + responseContent);src/InfrastructureTests/Services/CurrenciesHttpServiceTests.cs (1)
18-380: Consider externalizing the large XML constant.The 363-line XML constant reduces test readability. Consider moving it to an embedded resource file or a separate test data file, then loading it at runtime.
Example approach
Create
TestData/CurrenciesResponse.xmland load it:- public const string CurrenciesXml = @"<?xml version=""1.0"" encoding=""utf-8""?> -<rss version=""2.0""> -... -</rss>"; + private static string CurrenciesXml => + File.ReadAllText(Path.Combine("TestData", "CurrenciesResponse.xml"));Or use an embedded resource with
Assembly.GetManifestResourceStream().src/Infrastructure/Currencies/CurrenciesHttpService.cs (1)
29-33: Consider validating configuration at construction time.The URL validation runs on every
GetCurrenciesAsynccall. Moving this check to the constructor would provide fail-fast behavior at service registration time, making configuration issues immediately visible at startup.🔎 Proposed refactor
+ private readonly string _currenciesUrl; + public CurrenciesHttpService( IHttpClientFactory httpClientFactory, IConfiguration configuration, ILogger<CurrenciesHttpService> logger) { _httpClientFactory = httpClientFactory; _configuration = configuration; _logger = logger; + + _currenciesUrl = configuration["Currencies:Url"]; + if (string.IsNullOrEmpty(_currenciesUrl)) + { + throw new InvalidOperationException("Currencies url is not set"); + } } public async Task<Dictionary<Currency, CurrencyContent>> GetCurrenciesAsync( CancellationToken cancellationToken) { - var currenciesUrl = _configuration["Currencies:Url"]; - if (string.IsNullOrEmpty(currenciesUrl)) - { - throw new InvalidOperationException("Currencies url is not set"); - } - try { var client = _httpClientFactory.CreateClient(); - var xmlContent = await client.GetStringAsync(currenciesUrl, cancellationToken); + var xmlContent = await client.GetStringAsync(_currenciesUrl, cancellationToken);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Domain/Entities/Currencies/CurrenciesCollection.cssrc/Infrastructure/Currencies/CurrenciesHttpService.cssrc/Infrastructure/Currencies/ICurrenciesHttpService.cssrc/Infrastructure/Services/AiServices/Custom/CustomOpenAiService.cssrc/InfrastructureTests/Services/CurrenciesHttpServiceTests.cssrc/InfrastructureTests/Services/CurrencyServiceTests.cssrc/TestUtils/Fakes/CurrenciesCollectionFake.cs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
⚙️ CodeRabbit configuration file
**/*.cs: Review C# code for:
- SOLID principles and clean architecture adherence
- Proper async/await usage and avoiding async void
- Null safety and proper use of nullable reference types
- Dependency injection best practices
- Exception handling patterns
- Memory management and IDisposable implementation
- LINQ query optimization
Files:
src/Infrastructure/Services/AiServices/Custom/CustomOpenAiService.cssrc/InfrastructureTests/Services/CurrencyServiceTests.cssrc/Domain/Entities/Currencies/CurrenciesCollection.cssrc/Infrastructure/Currencies/ICurrenciesHttpService.cssrc/InfrastructureTests/Services/CurrenciesHttpServiceTests.cssrc/TestUtils/Fakes/CurrenciesCollectionFake.cssrc/Infrastructure/Currencies/CurrenciesHttpService.cs
**/Services/**/*.cs
⚙️ CodeRabbit configuration file
**/Services/**/*.cs: Review service layer for:
- Single responsibility principle
- Proper abstraction through interfaces
- Transaction management
- Business logic encapsulation
Files:
src/Infrastructure/Services/AiServices/Custom/CustomOpenAiService.cssrc/InfrastructureTests/Services/CurrencyServiceTests.cssrc/InfrastructureTests/Services/CurrenciesHttpServiceTests.cs
**/*Tests.cs
⚙️ CodeRabbit configuration file
**/*Tests.cs: Review test files for:
- Adequate test coverage
- Proper use of xUnit/NUnit/MSTest assertions
- Mock objects with Moq or NSubstitute
- Arrange-Act-Assert pattern
- Integration test isolation
Files:
src/InfrastructureTests/Services/CurrencyServiceTests.cssrc/InfrastructureTests/Services/CurrenciesHttpServiceTests.cs
🧬 Code graph analysis (3)
src/Infrastructure/Currencies/ICurrenciesHttpService.cs (2)
src/TestUtils/Fakes/CurrenciesCollectionFake.cs (1)
Dictionary(19-43)src/Domain/Entities/Currencies/CurrencyContent.cs (3)
CurrencyContent(11-18)CurrencyContent(20-35)CurrencyContent(37-45)
src/TestUtils/Fakes/CurrenciesCollectionFake.cs (4)
src/Domain/Entities/Currencies/CurrenciesCollection.cs (3)
CurrenciesCollection(8-56)CurrenciesCollection(16-32)CurrenciesCollection(53-55)src/Domain/Entities/Currencies/CurrencyContent.cs (3)
CurrencyContent(11-18)CurrencyContent(20-35)CurrencyContent(37-45)src/Domain/Entities/Currencies/KztCurrencyContent.cs (1)
KztCurrencyContent(8-15)src/Infrastructure/Database/DatabaseContext.cs (1)
SaveChanges(183-187)
src/Infrastructure/Currencies/CurrenciesHttpService.cs (3)
src/Infrastructure/Currencies/ICurrenciesHttpService.cs (1)
Task(8-9)src/Domain/Entities/Currencies/CurrencyContent.cs (3)
CurrencyContent(11-18)CurrencyContent(20-35)CurrencyContent(37-45)src/Domain/Extensions/EnumHelper.cs (1)
EnumHelper(11-133)
🔇 Additional comments (13)
src/Infrastructure/Services/AiServices/Custom/CustomOpenAiService.cs (1)
46-46: LGTM! Correct IHttpClientFactory pattern.Removing the
usingdeclaration is the correct approach when usingIHttpClientFactory. The factory manages the lifecycle of the underlyingHttpMessageHandler, and the returnedHttpClientinstances should not be disposed manually.src/Infrastructure/Currencies/ICurrenciesHttpService.cs (1)
6-10: LGTM! Clean interface design.The interface follows best practices with a single, well-defined responsibility and proper async/await patterns including
CancellationTokensupport for cooperative cancellation.src/Infrastructure/Currencies/CurrenciesHttpService.cs (2)
16-24: LGTM! Proper dependency injection.The constructor follows DI best practices by accepting interface dependencies, making the service testable and adhering to SOLID principles.
39-46: LGTM! Robust XML parsing and filtering.The parsing logic correctly handles the XML structure, filters to supported currencies, and uses
GroupBy().First()to handle potential duplicates in the source data elegantly.src/InfrastructureTests/Services/CurrencyServiceTests.cs (4)
20-73: LGTM! Well-structured test with clear assertions.The test correctly validates the initial state (no collections), the service behavior (creates collection on first call), and the dependency interaction (HTTP service called once).
136-187: LGTM! Test correctly validates collection creation from HTTP response.The test follows AAA pattern and validates that
RefetchServiceCurrenciesAsynccreates a new database record when the HTTP service returns currencies.
189-220: LGTM! Validates proper handling of empty HTTP response.The test correctly verifies that no database record is created when the HTTP service returns an empty currency collection.
282-337: LGTM! Validates idempotent behavior for same-date collections.The test correctly verifies that
RefetchServiceCurrenciesAsyncdoesn't create duplicates or make unnecessary HTTP calls when a collection for today's date already exists.src/Domain/Entities/Currencies/CurrenciesCollection.cs (2)
34-51: LGTM! Proper reconstruction with KZT special handling.The method correctly reconstructs
CurrencyContentinstances from the stored dictionary and ensures KZT (presumably the local currency) is always present. The logic is sound.
53-55: LGTM! Required for EF Core entity materialization.The protected parameterless constructor follows EF Core best practices, allowing the ORM to materialize entities while preventing direct instantiation in domain code.
src/TestUtils/Fakes/CurrenciesCollectionFake.cs (3)
11-17: LGTM! Clean constructor design.The constructor properly delegates to the base class with sensible defaults. The null-coalescing operator ensures that
CreateDefaultCurrenciesprovides a valid non-empty dictionary when none is supplied, satisfying the base constructor's validation requirements.
19-43: LGTM! Provides consistent test data.The method creates a well-structured default currency collection suitable for testing scenarios. The hardcoded exchange rates (USD=400, EUR=500, KZT=1) provide predictable test data, and proper usage of
KztCurrencyContentensures consistent behavior with the domain model.
45-51: LGTM! Well-designed persistence helper.The
Pleasemethod follows a fluent API pattern common in test utilities. The explicit cast toCurrenciesCollectionon line 48 is intentional—it ensures theEntityEntry<T>generic type matches the method's return type, avoiding the need for a cast on the return statement.The synchronous
SaveChanges()call is appropriate for in-memory test contexts, and the method correctly returns the persisted entity for potential further test setup.
Summary by CodeRabbit
Documentation
Refactor
New Features
Migrations
Tests
✏️ Tip: You can customize this high-level summary in your review settings.