Conversation
Introduced models, mappers, and schemas to handle crypto ordinal spot price data, including enhanced serialization and DTO support. Updated repository methods and refactored interfaces to accommodate IMarketData changes. Improved CosmosDB query and save operations with error handling.
Updated schema versioning, added JSON schema validation, and restructured version handling in `VersionManager` and `MarketDataRepository`. Introduced `JsonSchemaValidator` and `JsonSchemaValidatorRegistry` for schema validation. Modified domain models to dynamically compute unique IDs based on new schema rules.
Converted all string parameters to lowercase to ensure case-insensitivity during queries. Updated schemas and DTOs to handle null values and added default timestamps for missing data. Enhanced logging for validation and deserialization errors.
Renamed `SaveAsync` to `SaveMarketDataAsync` for better description, updated calling methods, and adjusted null checks on `feedIterator`. Enhanced logging in `ProcessActionResult` to capture both success and error messages. Updated HTTP request examples with corrected field values and renamed assets for consistency.
|
Caution Review failedThe pull request is closed. """ WalkthroughThis update introduces support for saving and validating multiple market data types—specifically FX spot prices and crypto ordinal spot prices—via a unified API endpoint. It adds new domain models, DTOs, mapping logic, JSON schemas, and schema validation infrastructure. The repository and related infrastructure are refactored for richer error handling and detailed result reporting. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SaveDocumentToDb (API)
participant JsonSchemaValidatorRegistry
participant Mapper
participant MarketDataRepository
participant CosmosDB
Client->>SaveDocumentToDb (API): POST /SaveDocumentToDb?datatype=...&assetclass=...&schemaversion=...
SaveDocumentToDb (API)->>JsonSchemaValidatorRegistry: Validate(payload, schema params)
alt Schema valid
SaveDocumentToDb (API)->>Mapper: Deserialize and map DTO to domain model
SaveDocumentToDb (API)->>MarketDataRepository: SaveMarketDataAsync(domain model)
MarketDataRepository->>CosmosDB: Save document
CosmosDB-->>MarketDataRepository: Save result
MarketDataRepository-->>SaveDocumentToDb (API): SaveMarketDataResult
SaveDocumentToDb (API)-->>Client: 200 OK / error based on result
else Schema invalid
SaveDocumentToDb (API)-->>Client: 400 Bad Request (validation errors)
end
Possibly related PRs
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (4)
src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs (2)
50-58: 🛠️ Refactor suggestionAdd
[JsonConstructor]to keep constructor–deserializer behaviour predictable
Newtonsoft.Jsonwill now prefer the (new) parameter-less ctor and set properties via reflection.
If any callers relied on the 12-parameter ctor for population (e.g.,BaseMarketDataMapper.ToDto), they will silently stop working unless you decorate that ctor with[JsonConstructor].@@ -public BaseMarketDataDto(string id, string schemaVersion, int? version, string assetId, string assetClass, +[JsonConstructor] +public BaseMarketDataDto(string id, string schemaVersion, int? version, string assetId, string assetClass,
54-70:⚠️ Potential issueCollection-expression
[]requires C# 12 – fall back tonew List<string>()for wider compatibility
Tags = tags ?? [];compiles only under C# 12/ .NET 8.0+.
If the project targets earlier versions (many Azure Functions projects are still on .NET 6/7) this line will not build.- Tags = tags ?? []; + Tags = tags ?? new List<string>();src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs (1)
146-159: 🛠️ Refactor suggestion
CalculateId()should guard against an unsetAsOfDateWhen
AsOfDateis left at its default (0001-01-01), the generated ID becomes misleading and may collide across instruments.- if (string.IsNullOrEmpty(DataType) || string.IsNullOrEmpty(AssetClass) || string.IsNullOrEmpty(AssetId) || - string.IsNullOrEmpty(Region) || string.IsNullOrEmpty(DocumentType)) + if (string.IsNullOrEmpty(DataType) + || string.IsNullOrEmpty(AssetClass) + || string.IsNullOrEmpty(AssetId) + || string.IsNullOrEmpty(Region) + || string.IsNullOrEmpty(DocumentType) + || AsOfDate == default) { return string.Empty; }src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (1)
100-108:⚠️ Potential issueKeep the return-type contract consistent for version-conflict scenarios
SaveMarketDataAsyncis designed to never throw to the caller but to wrap any error inSaveMarketDataResult.
However, in theConflictbranch the method still throws when the max-retry threshold is exceeded, breaking the new contract and forcing callers to addtry/catchagain.- if (attempt == maxRetries) - throw; + if (attempt == maxRetries) + return new SaveMarketDataResult + { + Success = false, + Message = $"Version conflict after {maxRetries} retries.", + Exception = ex + };Aligning all branches with the same result pattern makes the API simpler and avoids an unexpected exception path.
🧹 Nitpick comments (17)
src/Phoenix.MarketData.Infrastructure/Serialization/PriceSideDto.cs (1)
5-15: Add XML documentation to the enumThe enum is well-structured with proper EnumMember attributes for serialization. Consider adding XML documentation to explain its purpose and the meaning of each value, which would improve code maintainability and API documentation.
namespace Phoenix.MarketData.Infrastructure.Serialization; +/// <summary> +/// Represents the side of a price quote for serialization purposes. +/// </summary> public enum PriceSideDto { + /// <summary> + /// Represents the middle price between bid and ask. + /// </summary> [EnumMember(Value = "Mid")] Mid = 0, + /// <summary> + /// Represents the price at which a buyer is willing to purchase. + /// </summary> [EnumMember(Value = "Bid")] Bid = 1, + /// <summary> + /// Represents the price at which a seller is willing to sell. + /// </summary> [EnumMember(Value = "Ask")] Ask = 2, }Phoenix.MarketData.Functions/HttpRequests/SaveMarketData.http (1)
1-40: Update HTTP file with server variables for better maintainabilityConsider using HTTP file variables to make the examples more maintainable, especially if the same host/port is used in multiple requests.
+@host = localhost:7006 + ### Save fx spot market data to cosmos db example -POST localhost:7006/api/SaveDocumentToDb?datatype=price.spot&assetClass=fx&schemaversion=1.0.0 +POST {{host}}/api/SaveDocumentToDb?datatype=price.spot&assetClass=fx&schemaversion=1.0.0 Content-Type: application/json ... ### Save crypto ordinal spot price data to cosmos db example -POST localhost:7006/api/SaveDocumentToDb?datatype=price.ordinals.spot&assetClass=crypto&schemaversion=1.0.0 +POST {{host}}/api/SaveDocumentToDb?datatype=price.ordinals.spot&assetClass=crypto&schemaversion=1.0.0 Content-Type: application/jsonsrc/Phoenix.MarketData.Infrastructure/Schemas/FxSpotPriceData_v1.0.0.schema.json (1)
69-71: Consider case-insensitive pattern for side propertyThe pattern currently enforces lowercase values ("bid|mid|ask"), which might lead to validation errors if the client sends mixed-case values like "Bid" or "MID". Consider using a case-insensitive pattern or adding a description note about the required case.
- "pattern": "^(bid|mid|ask)$", + "pattern": "^(?i)(bid|mid|ask)$", "description": "The side of the price quote, e.g., mid, bid, or ask"Alternatively, document the case requirement:
"pattern": "^(bid|mid|ask)$", - "description": "The side of the price quote, e.g., mid, bid, or ask" + "description": "The side of the price quote, lowercase only: mid, bid, or ask"src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidator.cs (1)
6-7: Consider implementing a more modern result pattern.The class returns a boolean with an out parameter for error messages, which works but isn't aligned with modern C# patterns.
Consider implementing a result pattern that encapsulates both success and error information:
+ public class ValidationResult + { + public bool IsValid { get; } + public string ErrorMessage { get; } + + private ValidationResult(bool isValid, string errorMessage = null) + { + IsValid = isValid; + ErrorMessage = errorMessage; + } + + public static ValidationResult Success() => new ValidationResult(true); + public static ValidationResult Failure(string errorMessage) => new ValidationResult(false, errorMessage); + } public class JsonSchemaValidator { // ... existing code ... + public ValidationResult Validate(string jsonPayload) + { + try + { + var element = JsonDocument.Parse(jsonPayload).RootElement; + var result = _schema.Evaluate(element, new EvaluationOptions { OutputFormat = OutputFormat.Hierarchical }); + + if (result.IsValid) + { + return ValidationResult.Success(); + } + + return ValidationResult.Failure(FormatValidationErrors(result)); + } + catch (JsonException ex) + { + return ValidationResult.Failure($"Invalid JSON format: {ex.Message}"); + } + } }You can keep the existing method for backward compatibility if needed.
src/Phoenix.MarketData.Infrastructure/Mapping/CryptoOrdinalSpotPriceDataMapper.cs (1)
7-19: Add XML documentation for class and methods.The class and methods lack XML documentation, which would improve API discoverability and usage.
Consider adding XML documentation:
+ /// <summary> + /// Provides mapping functionality between <see cref="CryptoOrdinalSpotPriceData"/> domain model and + /// <see cref="CryptoOrdinalSpotPriceDataDto"/> data transfer object. + /// </summary> public static class CryptoOrdinalSpotPriceDataMapper { + /// <summary> + /// Converts a domain model to a DTO. + /// </summary> + /// <param name="domain">The domain model to convert. Cannot be null.</param> + /// <returns>A new DTO with values from the domain model.</returns> + /// <exception cref="ArgumentNullException">Thrown when domain is null.</exception> public static CryptoOrdinalSpotPriceDataDto ToDto(CryptoOrdinalSpotPriceData domain) { ArgumentNullException.ThrowIfNull(domain); var dto = new CryptoOrdinalSpotPriceDataDto(domain.Id, domain.SchemaVersion, domain.Version, domain.AssetId, domain.AssetClass, domain.DataType, domain.Region, domain.DocumentType, domain.CreateTimestamp, domain.AsOfDate, domain.AsOfTime, domain.Tags, domain.Price, domain.Side, domain.CollectionName, domain.ParentInscriptionId, domain.InscriptionId, domain.InscriptionNumber); return dto; }src/Phoenix.MarketData.Infrastructure/Schemas/CryptoOrdinalSpotPriceData_v1.0.0.schema.json (2)
2-5: Consider adding additionalProperties constraint.The schema doesn't specify whether additional properties are allowed. This could lead to unexpected fields being present in the data.
{ "$schema": "http://json-schema.org/draft-07/schema#", "title": "OrdinalPrice - Crypto", "description": "Schema for spot prices for ordinal assets (e.g., quantum cats).", "type": "object", + "additionalProperties": false, "required": ["assetId", "dataType", "assetClass", "asOfDate", "price", "collectionName", "parentInscriptionId", "inscriptionId", "inscriptionNumber"],
64-67: Update price property description.The price property description mentions "FX instrument" which is incorrect for crypto ordinals.
"price": { "type": "number", - "description": "The spot price of the FX instrument (e.g., 1.0985 for EUR/USD)." + "description": "The spot price of the crypto ordinal (e.g., 10.5 BTC for a specific ordinal)." }src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidatorRegistry.cs (2)
17-18: Mark the dictionary asreadonlyto avoid accidental reassignmentThe reference to
_validatorPathsshould not change after construction.
Changing only the mutability of the reference (not the contents) improves safety without blocking normalAdd/Removeoperations.- private Dictionary<SchemaKey, string> _validatorPaths = new Dictionary<SchemaKey, string>(); + private readonly Dictionary<SchemaKey, string> _validatorPaths = new();
36-38: Avoid re-reading schema files on every call – cacheJsonSchemaValidatorinstances
new JsonSchemaValidator(path)reloads the schema from disk for every validation, which is I/O-heavy and becomes a bottleneck under load.
StoreJsonSchemaValidatorinstances in the dictionary (or a second cache) after first creation.src/Phoenix.MarketData.Infrastructure/Serialization/CryptoOrdinalSpotPriceDataDto.cs (1)
30-49: Guard against negative prices and empty identifiersDTOs are often the first defence line. A negative
priceor missing inscription IDs most likely indicates bad upstream data.- Price = price; + if (price < 0) + throw new ArgumentOutOfRangeException(nameof(price), "Price must be non-negative."); + Price = price; - ParentInscriptionId = parentInscriptionId; - InscriptionId = inscriptionId; + ParentInscriptionId = string.IsNullOrWhiteSpace(parentInscriptionId) + ? throw new ArgumentException("ParentInscriptionId is required", nameof(parentInscriptionId)) + : parentInscriptionId; + InscriptionId = string.IsNullOrWhiteSpace(inscriptionId) + ? throw new ArgumentException("InscriptionId is required", nameof(inscriptionId)) + : inscriptionId;Phoenix.MarketData.Functions/SaveDocumentToDb.cs (2)
24-27: Restrict the trigger to POST – GET requests have no bodyThe function accepts
"get"but immediately tries to read the request body.
Unless you intend to support query-only GET requests (which are not implemented), drop"get"to prevent confusing 400 responses.- [Function("SaveDocumentToDb")] - public async Task<IActionResult> Run([HttpTrigger(AuthorizationLevel.Function, "get", "post")] HttpRequest req) + [Function("SaveDocumentToDb")] + public async Task<IActionResult> Run([HttpTrigger(AuthorizationLevel.Function, "post")] HttpRequest req)
105-121: Null-safe message handling
result.Messagecan benull, but the code compares it tostring.Empty, leading to a possibleNullReferenceException.- var msg = result.Message != string.Empty + var msg = !string.IsNullOrWhiteSpace(result.Message) ? result.Message : $"Document saved successfully to {result.Id}."; @@ - var message = result.Message != string.Empty ? result.Message : $"Error saving document to {result.Id}."; + var message = !string.IsNullOrWhiteSpace(result.Message) + ? result.Message + : $"Error saving document to {result.Id}.";src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs (2)
11-13:Sidecan be non-nullable – it always gets a valueBecause the ctor always assigns
SideaPriceSideDtovalue (defaulting toMid), callers will never observe anull.
Declaring it as nullable (PriceSideDto?) only complicates downstream null-checks.- public PriceSideDto? Side { get; set; } + public PriceSideDto Side { get; set; } = PriceSideDto.Mid;
14-17: Mark parameter-less ctor asinternalif it is serializer-onlyIf you added the ctor solely for
Newtonsoft.Json, consider restricting the surface area:- public FxSpotPriceDataDto() + internal FxSpotPriceDataDto()It prevents accidental misuse while keeping the serializer happy.
src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs (1)
24-134: Consider extracting the “lower-case-and-recalculate-id” pattern to reduce duplicationEach required property setter repeats the same three steps.
A small helper (e.g.,SetAndInvalidate(ref field, value)) orOnPropertyChangedpattern would:
- remove ~90 lines of boilerplate,
- reduce the chance of forgetting the
ToLowerInvariant()or_id = CalculateId()in the future.This is a readability / maintainability win.
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (2)
236-247: Guard against unsupported domain types earlier
MapToDtoreturnsobject, deferring type mismatch detection until runtime.
Consider making it generic to return the specific DTO type or restrictingTwith a common base DTO interface to get compile-time safety.Alternatively, at least bubble up the unsupported type information:
_ => throw new NotSupportedException( - $"Unsupported market data type: {typeof(T).Name}") + $"Unsupported market data type {typeof(T).Name}. Ensure a mapper is added in MapToDto().")This tiny change makes troubleshooting far easier when new models are introduced.
40-53: Optional: supportCancellationTokenfor better caller controlAll public async methods omit
CancellationToken.
Cosmos SDK fully supports it, and long-running IO calls (writes & queries) benefit from allowing the caller to cancel (e.g., API request aborted, graceful shutdown).-public async Task<SaveMarketDataResult> SaveMarketDataAsync<T>(T marketData, bool saveNextVersion = true) +public async Task<SaveMarketDataResult> SaveMarketDataAsync<T>( + T marketData, + bool saveNextVersion = true, + CancellationToken cancellationToken = default)Propagate the token to
GetNextVersionAsync,CreateItemAsync,ReadNextAsync, etc.
This is a non-breaking additive improvement for current callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
Phoenix.MarketData.Functions/HttpRequests/SaveMarketData.http(1 hunks)Phoenix.MarketData.Functions/SaveDocumentToDb.cs(2 hunks)Phoenix.MarketDataPlatform.sln(3 hunks)src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs(3 hunks)src/Phoenix.MarketData.Domain/Models/CryptoOrdinalSpotPriceData.cs(1 hunks)src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs(1 hunks)src/Phoenix.MarketData.Domain/Models/Interfaces/IMarketData.cs(1 hunks)src/Phoenix.MarketData.Domain/PriceSide.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs(5 hunks)src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/CryptoOrdinalSpotPriceDataMapper.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs(2 hunks)src/Phoenix.MarketData.Infrastructure/Phoenix.MarketData.Infrastructure.csproj(2 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/CryptoOrdinalSpotPriceData_v1.0.0.schema.json(1 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/FxSpotPriceData_v1.0.0.schema.json(2 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidator.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidatorRegistry.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/SchemaVersions.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs(3 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/CryptoOrdinalSpotPriceDataDto.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs(2 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/PriceSideDto.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/Phoenix.MarketData.Infrastructure/Mapping/CryptoOrdinalSpotPriceDataMapper.cs (3)
src/Phoenix.MarketData.Infrastructure/Serialization/CryptoOrdinalSpotPriceDataDto.cs (3)
CryptoOrdinalSpotPriceDataDto(6-50)CryptoOrdinalSpotPriceDataDto(26-28)CryptoOrdinalSpotPriceDataDto(30-49)src/Phoenix.MarketData.Domain/Models/CryptoOrdinalSpotPriceData.cs (1)
CryptoOrdinalSpotPriceData(3-34)src/Phoenix.MarketData.Domain/AssetClass.cs (1)
AssetClass(6-14)
src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidatorRegistry.cs (2)
src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidator.cs (3)
Validate(16-33)JsonSchemaValidator(6-34)JsonSchemaValidator(10-14)src/Phoenix.MarketData.Domain/AssetClass.cs (1)
AssetClass(6-14)
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs (3)
src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs (1)
FxSpotPriceData(7-22)src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs (3)
FxSpotPriceDataDto(6-32)FxSpotPriceDataDto(14-16)FxSpotPriceDataDto(18-31)src/Phoenix.MarketData.Domain/AssetClass.cs (1)
AssetClass(6-14)
src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs (1)
src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs (1)
BaseMarketDataDto(8-15)
src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs (1)
src/Phoenix.MarketData.Domain/AssetClass.cs (1)
AssetClass(6-14)
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (7)
src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs (3)
VersionManager(5-28)VersionManager(9-12)Task(14-27)src/Phoenix.MarketData.Infrastructure/Cosmos/CosmosClientFactory.cs (1)
CosmosClient(9-16)src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/DateOnlyJsonConverter.cs (1)
DateOnly(25-29)src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs (2)
FxSpotPriceData(20-48)FxSpotPriceDataMapper(7-49)src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs (1)
FxSpotPriceData(7-22)src/Phoenix.MarketData.Infrastructure/Mapping/CryptoOrdinalSpotPriceDataMapper.cs (2)
CryptoOrdinalSpotPriceData(21-53)CryptoOrdinalSpotPriceDataMapper(7-54)src/Phoenix.MarketData.Domain/Models/CryptoOrdinalSpotPriceData.cs (1)
CryptoOrdinalSpotPriceData(3-34)
🔇 Additional comments (18)
src/Phoenix.MarketData.Domain/Models/Interfaces/IMarketData.cs (1)
6-6: Interface renamed to better align with domain terminology.The interface name change from
IMarketDataObjecttoIMarketDataimproves consistency across the domain models and infrastructure layers. This renaming clarifies the purpose of the interface without changing its functionality.src/Phoenix.MarketData.Infrastructure/Phoenix.MarketData.Infrastructure.csproj (2)
10-10: Added JSON schema validation capability.Good addition of the JsonSchema.Net package, providing the infrastructure needed for schema validation of market data objects.
19-26: Schema files added for new market data types.The addition of JSON schema files for Crypto Ordinal and FX Spot price data types with proper output directory configuration enables runtime validation of these market data formats.
src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs (1)
32-32: Improved null handling for CreateTimestamp.Good enhancement to handle null timestamps by using the current UTC time as a fallback. This makes the mapping more robust and prevents potential null reference issues downstream.
src/Phoenix.MarketData.Domain/PriceSide.cs (1)
1-15: Well-defined enum for price side representation.This new enum is a good addition to the domain model, providing a type-safe way to represent different price sides (mid, bid, ask) commonly used in market data. The use of Description attributes helps with display and serialization concerns.
src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs (3)
9-14: Well-documented propertyThe XML documentation for the Price property is comprehensive and clearly explains its purpose within the domain model.
16-21: Properly documented new Side property with appropriate defaultThe addition of the Side property with a default value of PriceSide.Mid is well-implemented and thoroughly documented.
7-22:Details
❓ Verification inconclusive
Verify the BaseMarketData inheritance is properly handled
Ensure the new Side property doesn't conflict with any properties or methods in the BaseMarketData class and that any required serialization/mapping logic is updated accordingly.
🏁 Script executed:
#!/bin/bash # Check if BaseMarketData has any conflicting properties or methods rg "public\s+(virtual\s+)?(required\s+)?(abstract\s+)?(\w+\s+)?Side(\s*\{|\s*=)" -t cs src/Phoenix.MarketData.Domain/Length of output: 236
FxSpotPriceData – BaseMarketData inheritance checked
NoSidemember exists onBaseMarketData, so there’s no inheritance conflict.• Now confirm that any serialization or mapping layers (e.g. AutoMapper profiles, DTOs, JSON converters) have been updated to include the new
Sideproperty so it’s correctly persisted and transmitted.src/Phoenix.MarketData.Infrastructure/Schemas/FxSpotPriceData_v1.0.0.schema.json (1)
31-32: Updated dataType to be more specificThe change from "price" to "price.spot" in the dataType enum makes the schema more precise and aligns with the domain model.
src/Phoenix.MarketData.Infrastructure/Schemas/SchemaVersions.cs (1)
12-12: Schema version correctly updated to V1The schema version has been properly updated from V0 to V1, aligning with the introduction of new JSON schemas for version "1.0.0" that support multiple market data types. The supported versions collection has been updated accordingly, and the placeholder for future versions now correctly references V2.
Also applies to: 15-15, 22-22
Phoenix.MarketDataPlatform.sln (1)
11-14: Test project added properlyThe test project structure has been correctly added to the solution, including proper nesting under the tests folder and appropriate build configurations. This supports the expanded testing coverage for the enhanced market data infrastructure.
Also applies to: 24-24, 39-42
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs (3)
1-2: Imports added for domain modelsThese imports correctly support the reference to domain models and the PriceSide enum used in the updated mapper implementation.
15-15: Side property added to DTO constructorThe ToDto method now correctly maps the new Side property from the domain model to the DTO.
20-48: Method replaced with improved implementationThe previous
ApplyToDomainmethod has been replaced with a betterToDomainmethod that:
- Creates a new domain object instead of modifying an existing one
- Properly maps all properties including the new Side property
- Uses appropriate null handling with fallbacks
- Handles enum conversion with a comprehensive switch expression
This change follows better immutability practices and aligns with the enhanced domain model.
src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs (2)
15-15: Interface constraint updatedThe type constraint has been correctly updated from
IMarketDataObjecttoIMarketDatato align with the renamed interface in the domain model.
22-23: Repository method and result handling updatedThe code now calls the updated repository method
GetMarketDataByLatestVersionAsyncand correctly handles the richer result type by accessing theResultproperty. The version increment logic remains properly implemented.Also applies to: 26-26
src/Phoenix.MarketData.Domain/Models/CryptoOrdinalSpotPriceData.cs (1)
3-34: LGTM! The domain model is well-structured with clear documentation.The CryptoOrdinalSpotPriceData class appropriately inherits from BaseMarketData and provides all necessary properties with XML documentation. The required modifier is used correctly for most properties.
src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs (1)
39-41: NullableCreateTimestampmismatches non-nullable ctor argument
CreateTimestampis now nullable, yet the ctor enforces a non-nullable argument.
Either make the parameter nullable or keep the property non-nullable to avoid confusion and double-null-checking elsewhere.
| "side": { | ||
| "type": "string", | ||
| "pattern": "^(bid|mid|ask)$", | ||
| "description": "The side of the price quote, e.g., mid, bid, or ask" | ||
| } |
There was a problem hiding this comment.
Properly defined side property with pattern validation
The addition of the "side" property with pattern validation ensures only valid values ("bid", "mid", "ask") are accepted. However, it's not included in the required properties list despite being a necessary field.
- "required": ["assetId", "dataType", "assetClass", "asOfDate", "price"],
+ "required": ["assetId", "dataType", "assetClass", "asOfDate", "price", "side"],📝 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.
| "side": { | |
| "type": "string", | |
| "pattern": "^(bid|mid|ask)$", | |
| "description": "The side of the price quote, e.g., mid, bid, or ask" | |
| } | |
| "required": ["assetId", "dataType", "assetClass", "asOfDate", "price", "side"], |
| public bool Validate(string jsonPayload, out string errorMessage) | ||
| { | ||
| var element = JsonDocument.Parse(jsonPayload).RootElement; | ||
| var result = _schema.Evaluate(element, new EvaluationOptions { OutputFormat = OutputFormat.Hierarchical }); | ||
|
|
||
| if (result.IsValid) | ||
| { | ||
| errorMessage = null; | ||
| return true; | ||
| } | ||
|
|
||
| // Failed to validate | ||
| errorMessage = string.Join("; ", result.Details.Select( | ||
| e => e.InstanceLocation + ": " + (e.HasErrors | ||
| ? string.Join("\n", e.Errors?.Select(kvp => kvp.Key.ToString() + " - " + kvp.Value.ToString()) ?? Array.Empty<string>()) | ||
| : ""))); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JSON parsing and improve error message readability.
The Validate method doesn't handle exceptions from JsonDocument.Parse, and the error message formatting is complex.
Consider adding try-catch for JSON parsing and extracting the error formatting logic to a separate method:
public bool Validate(string jsonPayload, out string errorMessage)
{
+ try
+ {
var element = JsonDocument.Parse(jsonPayload).RootElement;
var result = _schema.Evaluate(element, new EvaluationOptions { OutputFormat = OutputFormat.Hierarchical });
if (result.IsValid)
{
errorMessage = null;
return true;
}
// Failed to validate
- errorMessage = string.Join("; ", result.Details.Select(
- e => e.InstanceLocation + ": " + (e.HasErrors
- ? string.Join("\n", e.Errors?.Select(kvp => kvp.Key.ToString() + " - " + kvp.Value.ToString()) ?? Array.Empty<string>())
- : "")));
+ errorMessage = FormatValidationErrors(result);
return false;
+ }
+ catch (JsonException ex)
+ {
+ errorMessage = $"Invalid JSON format: {ex.Message}";
+ return false;
+ }
}
+ private static string FormatValidationErrors(EvaluationResults result)
+ {
+ return string.Join("; ", result.Details.Select(
+ e => e.InstanceLocation + ": " + (e.HasErrors
+ ? string.Join("\n", e.Errors?.Select(kvp => kvp.Key.ToString() + " - " + kvp.Value.ToString()) ?? Array.Empty<string>())
+ : "")));
+ }📝 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.
| public bool Validate(string jsonPayload, out string errorMessage) | |
| { | |
| var element = JsonDocument.Parse(jsonPayload).RootElement; | |
| var result = _schema.Evaluate(element, new EvaluationOptions { OutputFormat = OutputFormat.Hierarchical }); | |
| if (result.IsValid) | |
| { | |
| errorMessage = null; | |
| return true; | |
| } | |
| // Failed to validate | |
| errorMessage = string.Join("; ", result.Details.Select( | |
| e => e.InstanceLocation + ": " + (e.HasErrors | |
| ? string.Join("\n", e.Errors?.Select(kvp => kvp.Key.ToString() + " - " + kvp.Value.ToString()) ?? Array.Empty<string>()) | |
| : ""))); | |
| return false; | |
| } | |
| public bool Validate(string jsonPayload, out string errorMessage) | |
| { | |
| try | |
| { | |
| var element = JsonDocument.Parse(jsonPayload).RootElement; | |
| var result = _schema.Evaluate(element, new EvaluationOptions { OutputFormat = OutputFormat.Hierarchical }); | |
| if (result.IsValid) | |
| { | |
| errorMessage = null; | |
| return true; | |
| } | |
| // Failed to validate | |
| errorMessage = FormatValidationErrors(result); | |
| return false; | |
| } | |
| catch (JsonException ex) | |
| { | |
| errorMessage = $"Invalid JSON format: {ex.Message}"; | |
| return false; | |
| } | |
| } | |
| private static string FormatValidationErrors(EvaluationResults result) | |
| { | |
| return string.Join("; ", result.Details.Select( | |
| e => e.InstanceLocation + ": " + (e.HasErrors | |
| ? string.Join("\n", e.Errors?.Select(kvp => kvp.Key.ToString() + " - " + kvp.Value.ToString()) | |
| ?? Array.Empty<string>()) | |
| : ""))); | |
| } |
src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidatorRegistry.cs
Show resolved
Hide resolved
| if (string.IsNullOrWhiteSpace(dataType) || string.IsNullOrWhiteSpace(assetClass) || string.IsNullOrEmpty("schemaversion")) | ||
| { | ||
| return new BadRequestObjectResult("Please provide 'datatype', 'assetclass' and 'schema' as query parameters."); | ||
| } |
There was a problem hiding this comment.
Bug: schemaVersion check is against a string literal – validation always passes
string.IsNullOrEmpty("schemaversion") is always false, so the function never verifies that the caller actually supplied the schemaversion parameter.
- if (string.IsNullOrWhiteSpace(dataType) || string.IsNullOrWhiteSpace(assetClass) || string.IsNullOrEmpty("schemaversion"))
+ if (string.IsNullOrWhiteSpace(dataType) || string.IsNullOrWhiteSpace(assetClass) || string.IsNullOrWhiteSpace(schemaVersion))| if (dataType == "price.ordinals.spot" && assetClass == "crypto" && schemaVersion != string.Empty) | ||
| { | ||
| var requestBody = await new StreamReader(req.Body).ReadToEndAsync(); | ||
| CryptoOrdinalSpotPriceDataDto? requestData; | ||
|
|
||
| try | ||
| { | ||
| requestData = System.Text.Json.JsonSerializer.Deserialize<CryptoOrdinalSpotPriceDataDto>(requestBody, new System.Text.Json.JsonSerializerOptions | ||
| { | ||
| PropertyNameCaseInsensitive = true | ||
| }); | ||
|
|
||
| if (requestData == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(requestData)); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error deserializing request body for fx spot price."); | ||
| return new BadRequestObjectResult("Invalid request body."); | ||
| } | ||
|
|
||
| var result = await _repository.SaveMarketDataAsync(CryptoOrdinalSpotPriceDataMapper.ToDomain(requestData)); | ||
| return ProcessActionResult(result); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add schema validation + correct logging for the crypto branch
The crypto branch skips JSON-schema validation and logs the wrong asset class on deserialization errors.
- var requestBody = await new StreamReader(req.Body).ReadToEndAsync();
+ var requestBody = await new StreamReader(req.Body).ReadToEndAsync();
CryptoOrdinalSpotPriceDataDto? requestData;
try
{
+ var validated = JsonSchemaValidatorRegistry.Instance.Validate(
+ dataType!, assetClass!, schemaVersion!, requestBody, out var errorMsg);
+ if (!validated)
+ {
+ _logger.LogError($"Error validating request body for crypto ordinal spot price. {errorMsg}");
+ return new BadRequestObjectResult("Could not validate request body against schema.");
+ }
+
requestData = System.Text.Json.JsonSerializer.Deserialize<CryptoOrdinalSpotPriceDataDto>(requestBody, new System.Text.Json.JsonSerializerOptions
{
PropertyNameCaseInsensitive = true
});
@@
- catch (Exception ex)
+ catch (Exception ex)
{
- _logger.LogError(ex, "Error deserializing request body for fx spot price.");
+ _logger.LogError(ex, "Error deserializing request body for crypto ordinal spot price.");
return new BadRequestObjectResult("Invalid request body.");
}📝 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.
| if (dataType == "price.ordinals.spot" && assetClass == "crypto" && schemaVersion != string.Empty) | |
| { | |
| var requestBody = await new StreamReader(req.Body).ReadToEndAsync(); | |
| CryptoOrdinalSpotPriceDataDto? requestData; | |
| try | |
| { | |
| requestData = System.Text.Json.JsonSerializer.Deserialize<CryptoOrdinalSpotPriceDataDto>(requestBody, new System.Text.Json.JsonSerializerOptions | |
| { | |
| PropertyNameCaseInsensitive = true | |
| }); | |
| if (requestData == null) | |
| { | |
| throw new ArgumentNullException(nameof(requestData)); | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Error deserializing request body for fx spot price."); | |
| return new BadRequestObjectResult("Invalid request body."); | |
| } | |
| var result = await _repository.SaveMarketDataAsync(CryptoOrdinalSpotPriceDataMapper.ToDomain(requestData)); | |
| return ProcessActionResult(result); | |
| } | |
| if (dataType == "price.ordinals.spot" && assetClass == "crypto" && schemaVersion != string.Empty) | |
| { | |
| var requestBody = await new StreamReader(req.Body).ReadToEndAsync(); | |
| CryptoOrdinalSpotPriceDataDto? requestData; | |
| try | |
| { | |
| // JSON‐schema validation for crypto branch | |
| var validated = JsonSchemaValidatorRegistry.Instance.Validate( | |
| dataType!, assetClass!, schemaVersion!, requestBody, out var errorMsg); | |
| if (!validated) | |
| { | |
| _logger.LogError($"Error validating request body for crypto ordinal spot price. {errorMsg}"); | |
| return new BadRequestObjectResult("Could not validate request body against schema."); | |
| } | |
| requestData = System.Text.Json.JsonSerializer.Deserialize<CryptoOrdinalSpotPriceDataDto>(requestBody, new System.Text.Json.JsonSerializerOptions | |
| { | |
| PropertyNameCaseInsensitive = true | |
| }); | |
| if (requestData == null) | |
| { | |
| throw new ArgumentNullException(nameof(requestData)); | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Error deserializing request body for crypto ordinal spot price."); | |
| return new BadRequestObjectResult("Invalid request body."); | |
| } | |
| var result = await _repository.SaveMarketDataAsync(CryptoOrdinalSpotPriceDataMapper.ToDomain(requestData)); | |
| return ProcessActionResult(result); | |
| } |
…tor.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ory.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ation Introduced a `currency` field to the `CryptoOrdinalSpotPriceData` model and schema for enhanced data representation. Enhanced request validation against schemas using a centralized validator logic. Added unit tests for repository operations and implemented lowercasing for query parameters to ensure consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (2)
196-205: LGTM! Fixed empty result handlingThe implementation now correctly sets Success=false when no document is found, making the API more consistent for consumers.
306-310:⚠️ Potential issueNormalize query parameters in extension method for consistency
Other queries lower-case parameters to match the normalised casing strategy in
BaseMarketData.
The extension method omits this, which may cause partition-key or filter mismatches.-.WithParameter("@assetId", assetId) -.WithParameter("@assetClass", assetClass) -.WithParameter("@region", region) -.WithParameter("@dataType", dataType) -.WithParameter("@documentType", documentType); +.WithParameter("@assetId", assetId.ToLowerInvariant()) +.WithParameter("@assetClass", assetClass.ToLowerInvariant()) +.WithParameter("@region", region.ToLowerInvariant()) +.WithParameter("@dataType", dataType.ToLowerInvariant()) +.WithParameter("@documentType", documentType.ToLowerInvariant());Aligning casing avoids hard-to-trace "document not found" issues.
🧹 Nitpick comments (2)
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (2)
5-5: Remove unnecessary self-importThis namespace import is redundant since the file is already in the
Phoenix.MarketData.Infrastructure.Cosmosnamespace (defined on line 9).-using Phoenix.MarketData.Infrastructure.Cosmos;
263-270: Consider removing redundant initializers for nullable propertiesThe properties are already declared as nullable with the
?modifier, so initializing them with default values is redundant.-public string? Id { get; init; } = string.Empty; +public string? Id { get; init; } -public string? Message { get; init; } = string.Empty; +public string? Message { get; init; }Apply the same change to the
Messageproperty inLoadMarketDataResult<T>on line 280.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs(5 hunks)
🔇 Additional comments (4)
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (4)
22-26: LGTM! Improved testability with additional constructorAdding a constructor that accepts a Container directly improves testability and dependency injection capabilities.
58-111: Strong improvement with structured error handlingThe enhanced exception handling with specific error cases and detailed messages is excellent. This approach of returning structured error results instead of throwing exceptions provides better control for calling code and improves observability.
135-139: LGTM! Parameter normalization applied consistentlyGood implementation of the string normalization using ToLowerInvariant() for all query parameters. This ensures consistency with the normalized casing in the base market data model.
244-250: LGTM! Support added for CryptoOrdinalSpotPriceDataCorrectly extended the mapping logic to handle the new CryptoOrdinalSpotPriceData type.
…torRegistry.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Added checks to handle null results in `MarketDataRepository` queries, ensuring proper error messages are returned. Enhanced `JsonSchemaValidator` with input validation and better exception handling for file reading and JSON parsing.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
src/Phoenix.MarketData.Infrastructure/Schemas/CryptoOrdinalSpotPriceData_v1.0.0.schema.json (2)
24-26: Fix incorrect description for assetClass.The description still incorrectly states this is for 'fx' instruments, but this is a crypto ordinal schema.
"assetClass": { "type": "string", "enum": ["crypto"], - "description": "Specifies the asset class; fixed as 'fx' for foreign exchange instruments." + "description": "Specifies the asset class; fixed as 'crypto' for cryptocurrency instruments." }
88-91: Add minimum constraint to inscriptionNumber.The inscriptionNumber is defined as a required field that should be a number, but there's no minimum value constraint. It could potentially be 0 or negative, which may not be valid.
"inscriptionNumber": { "type": "number", + "minimum": 0, "description": "The inscription number of the asset." }src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidator.cs (1)
28-46: Add error handling for JSON parsing and improve error message readability.The
Validatemethod doesn't handle exceptions fromJsonDocument.Parse, and the error message formatting is complex.Consider adding try-catch for JSON parsing and extracting the error formatting logic to a separate method:
public ValidationResult Validate(string jsonPayload) { + try + { var element = JsonDocument.Parse(jsonPayload).RootElement; var result = _schema.Evaluate(element, new EvaluationOptions { OutputFormat = OutputFormat.Hierarchical, ValidateAgainstMetaSchema = true }); if (result.IsValid) return ValidationResult.Success(); // Failed to validate - var errorMessage = string.Join("; ", result.Details.Select( - e => e.InstanceLocation + ": " + (e.HasErrors - ? string.Join("\n", e.Errors?.Select(kvp => kvp.Key.ToString() + " - " + kvp.Value.ToString()) ?? Array.Empty<string>()) - : ""))); + var errorMessage = FormatValidationErrors(result); return ValidationResult.Failure(errorMessage); + } + catch (JsonException ex) + { + return ValidationResult.Failure($"Invalid JSON format: {ex.Message}"); + } } + private static string FormatValidationErrors(EvaluationResults result) + { + return string.Join("; ", result.Details.Select( + e => e.InstanceLocation + ": " + (e.HasErrors + ? string.Join("\n", e.Errors?.Select(kvp => kvp.Key.ToString() + " - " + kvp.Value.ToString()) ?? Array.Empty<string>()) + : ""))); + }src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (2)
139-143: Parameter normalization implemented.Great implementation of the previously suggested normalization of query parameters to lowercase, which aligns with the normalized casing strategy in BaseMarketData.
185-241: Centralized query execution with improved error handling.The method includes comprehensive error handling for different exception types and returns meaningful error messages with the appropriate context.
The implementation also correctly addresses the previous suggestion to return failure when no documents are found, improving the semantics of the Success flag.
🧹 Nitpick comments (4)
src/Phoenix.MarketData.Infrastructure/Schemas/CryptoOrdinalSpotPriceData_v1.0.0.schema.json (1)
65-65: Fix typo in price description.There's a double period at the end of the description.
- "description": "The spot price of the crypto ordinal (e.g., 0.015 in BTC for a specific ordinal).." + "description": "The spot price of the crypto ordinal (e.g., 0.015 in BTC for a specific ordinal)."src/Phoenix.MarketData.Infrastructure/Serialization/CryptoOrdinalSpotPriceDataDto.cs (1)
33-39: Improve constructor readability and parameter organization.The constructor parameters are spread across multiple lines in a way that makes it difficult to read and maintain. Consider reformatting for better readability.
[JsonConstructor] public CryptoOrdinalSpotPriceDataDto(string id, string schemaVersion, int? version, string assetId, string assetClass, string dataType, string region, string documentType, DateTimeOffset createTimestamp, DateOnly asOfDate, TimeOnly? asOfTime, List<string> tags, - decimal price, PriceSide? side, string collectionName, string parentInscriptionId, - string inscriptionId, int inscriptionNumber, string currency) : base(id, schemaVersion, version, assetId, assetClass, dataType, - region, documentType, createTimestamp, asOfDate, asOfTime, tags) + decimal price, PriceSide? side, string collectionName, + string parentInscriptionId, string inscriptionId, + int inscriptionNumber, string currency) + : base(id, schemaVersion, version, assetId, assetClass, dataType, + region, documentType, createTimestamp, asOfDate, asOfTime, tags)src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (2)
5-5: Unnecessary namespace import.
Phoenix.MarketData.Infrastructure.Cosmosis redundant since the file is already in this namespace.-using Phoenix.MarketData.Infrastructure.Cosmos;
197-203: Potential redundant null check.The check for
feedIterator == nullis likely unnecessary as GetItemQueryIterator shouldn't return null based on Cosmos DB SDK behavior.- if (feedIterator == null || !feedIterator.HasMoreResults) + if (!feedIterator.HasMoreResults) return new LoadMarketDataResult<T>{ Success = false, Message = "Market data not found."};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Phoenix.MarketData.Functions/HttpRequests/SaveMarketData.http(1 hunks)Phoenix.MarketData.Functions/SaveDocumentToDb.cs(2 hunks)Phoenix.MarketData.Infrastructure.Tests/MarketDataRepositoryTests.cs(1 hunks)Phoenix.MarketData.Infrastructure.Tests/Phoenix.MarketData.Infrastructure.Tests.csproj(1 hunks)src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs(2 hunks)src/Phoenix.MarketData.Domain/Models/CryptoOrdinalSpotPriceData.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs(4 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/CryptoOrdinalSpotPriceDataMapper.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/CryptoOrdinalSpotPriceData_v1.0.0.schema.json(1 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/FxSpotPriceData_v1.0.0.schema.json(2 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidator.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidatorRegistry.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs(3 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/CryptoOrdinalSpotPriceDataDto.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs(2 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/PriceSideDto.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Phoenix.MarketData.Infrastructure.Tests/Phoenix.MarketData.Infrastructure.Tests.csproj
🚧 Files skipped from review as they are similar to previous changes (10)
- Phoenix.MarketData.Functions/HttpRequests/SaveMarketData.http
- src/Phoenix.MarketData.Infrastructure/Mapping/CryptoOrdinalSpotPriceDataMapper.cs
- src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidatorRegistry.cs
- src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs
- src/Phoenix.MarketData.Infrastructure/Serialization/PriceSideDto.cs
- Phoenix.MarketData.Functions/SaveDocumentToDb.cs
- src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs
- src/Phoenix.MarketData.Domain/Models/CryptoOrdinalSpotPriceData.cs
- src/Phoenix.MarketData.Infrastructure/Schemas/FxSpotPriceData_v1.0.0.schema.json
- src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidator.cs (1)
src/Phoenix.MarketData.Infrastructure/Schemas/JsonSchemaValidatorRegistry.cs (1)
Validate(29-47)
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (6)
src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs (3)
VersionManager(5-28)VersionManager(9-12)Task(14-27)src/Phoenix.MarketData.Infrastructure/Cosmos/CosmosClientFactory.cs (1)
CosmosClient(9-16)src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs (2)
FxSpotPriceData(20-48)FxSpotPriceDataMapper(7-49)src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs (1)
FxSpotPriceData(7-22)src/Phoenix.MarketData.Domain/Models/CryptoOrdinalSpotPriceData.cs (1)
CryptoOrdinalSpotPriceData(3-39)src/Phoenix.MarketData.Infrastructure/Mapping/CryptoOrdinalSpotPriceDataMapper.cs (2)
CryptoOrdinalSpotPriceData(36-69)CryptoOrdinalSpotPriceDataMapper(12-70)
🔇 Additional comments (11)
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (11)
21-25: Good addition: Constructor with direct Container injection.This new constructor allows for easier testing and dependency injection by accepting a pre-configured Container directly.
35-35: Interface change from IMarketDataObject to IMarketData.The method signature has been updated to use IMarketData instead of IMarketDataObject, which aligns with the domain model changes.
57-118: Comprehensive error handling with structured result object.The improved error handling now returns detailed SaveMarketDataResult objects instead of throwing exceptions, making it easier for callers to handle different error scenarios appropriately.
146-146: Reuse of query execution logic.Extracting the query execution to a separate method improves code maintainability and reduces duplication.
161-176: Added method to retrieve specific version.This new method enhances the API by allowing retrieval of a specific version of market data, while maintaining the pattern of normalized parameters and structured results.
243-243: Updated generic constraint to IMarketData.The MapToDto method has been updated to accept IMarketData instead of IMarketDataObject, aligning with domain interface changes.
248-248: Support for new CryptoOrdinalSpotPriceData type.The mapper now handles the newly added CryptoOrdinalSpotPriceData domain model, ensuring DTOs can be properly created for this data type.
258-269: Well-designed result class for save operations.The SaveMarketDataResult class provides a structured way to communicate success/failure and includes relevant metadata about the operation.
271-280: Well-designed generic result class for load operations.The LoadMarketDataResult class provides a structured way to communicate success/failure and includes the loaded data when successful.
283-313: Useful extension method for retrieving most recent market data.This extension method provides a convenient way to get the most recent data by date, addressing a common use case. The implementation correctly normalizes query parameters and reuses the ExecuteMarketDataFetchQuery method.
190-195: Efficient query configuration.The query options with MaxBufferedItemCount and MaxItemCount set to 1 optimize the query execution for single-item retrieval.
src/Phoenix.MarketData.Infrastructure/Serialization/CryptoOrdinalSpotPriceDataDto.cs
Show resolved
Hide resolved
| // [Fact] | ||
| // public async Task GetMarketDataAsync_ShouldReturnNull_WhenItemDoesNotExist() | ||
| // { | ||
| // // Arrange | ||
| // var id = "non-existent-id"; | ||
| // var partitionKey = "partition"; | ||
| // | ||
| // _mockContainer | ||
| // .Setup(c => c.ReadItemAsync<MarketData>( | ||
| // It.IsAny<string>(), | ||
| // It.IsAny<PartitionKey>(), | ||
| // It.IsAny<ItemRequestOptions>(), | ||
| // It.IsAny<CancellationToken>())) | ||
| // .ThrowsAsync(new CosmosException("Not Found", HttpStatusCode.NotFound, 0, "", 0)); | ||
| // | ||
| // // Act | ||
| // var result = await _repository.GetMarketDataAsync(id, partitionKey); | ||
| // | ||
| // // Assert | ||
| // Assert.Null(result); | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uncomment and complete the test for non-existent items.
The test for the scenario when an item doesn't exist is commented out. This is an important test case to ensure the repository correctly handles missing data.
Either uncomment and update this test to work with the current implementation, or remove it if it's no longer relevant. If you decide to keep it, update it to use the correct return type and assertions.
| //Assert.NotNull(result); | ||
| //Assert.Equal("Test Data", result.Data); | ||
| //Assert.Equal(id, result.Id); |
There was a problem hiding this comment.
Uncomment and complete the assertions.
The test is missing proper assertions. Uncomment and update these to verify the correct data is returned.
// Assert
-//Assert.NotNull(result);
-//Assert.Equal("Test Data", result.Data);
-//Assert.Equal(id, result.Id);
+Assert.NotNull(result);
+Assert.True(result.IsSuccess);
+Assert.Equal(_marketData.Price, result.Data.Price);
+Assert.Equal(_marketData.AssetId, result.Data.AssetId);📝 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.
| //Assert.NotNull(result); | |
| //Assert.Equal("Test Data", result.Data); | |
| //Assert.Equal(id, result.Id); | |
| // Assert | |
| Assert.NotNull(result); | |
| Assert.True(result.IsSuccess); | |
| Assert.Equal(_marketData.Price, result.Data.Price); | |
| Assert.Equal(_marketData.AssetId, result.Data.AssetId); |
…nalSpotPriceDataDto.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…sts.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Updated test assertions to use correct property names, ensuring consistency with the updated data model. This change improves test accuracy and prevents potential false positives in validation.
1 participant
➕ What does this PR do?
🔨 Changes
✅ Checklist
🗒 Notes for reviewer
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores