Add region and asOfTime support to market data objects#1
Conversation
Introduced new properties `region` and `asOfTime` across domain models, schemas, and repository methods for better regional and timestamp granularity in market data handling. Refactored `FxSpotPriceObject` to inherit from a new `BaseMarketDataObject`, consolidating shared logic. Updated schema versioning with a centralized version management system.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (1)
38-49:⚠️ Potential issueTwo query gotchas:
DateOnlyserialisation & lexicographicalORDER BYonversion
Parameter
@asOfDatewill be serialised to JSON using the defaultDateOnlyconverter (none), resulting in an empty string. Combined with the earlier note, this filter will never match until custom converters are added.
versionis stored as a string and ordered lexicographically, so"10"comes before"2".
Convert tointin the query:ORDER BY CAST(c.version AS int) DESC(Cosmos SQL supports
CASTtoint).Together these issues can lead to incorrect “latest” retrieval.
src/Phoenix.MarketData.Domain/Models/FxSpotPriceObject.cs (1)
8-15: 🛠️ Refactor suggestionConsider using
decimalfor monetary values
Priceis declared asdouble, which can introduce floating-point rounding errors (e.g.,0.1 + 0.2 != 0.3).
For FX spot prices 4–8 decimal places of precision are common and deterministic arithmetic is required.-[JsonProperty("price")] -public required double Price { get; set; } +[JsonProperty("price")] +public required decimal Price { get; set; }If large numbers or sub-pip precision are expected, configure the JSON serializer with
FloatParseHandling.Decimal.
🧹 Nitpick comments (7)
Phoenix.MarketData.Functions/SaveDocumentToDb.cs (1)
33-33: Consider using constants for region and standardizing time formatThe region value "NY" is hardcoded as a string. Consider using constants or enums for regions to avoid typos and ensure consistency across the codebase. Also, for
AsOfTime, consider standardizing the seconds parameter format by removing the leading zero (using 5 instead of 05).- Region = "NY", + Region = RegionConstants.NewYork, // Use a constant - AsOfTime = new TimeOnly(15, 30, 05), + AsOfTime = new TimeOnly(15, 30, 5),Also applies to: 37-37
src/Phoenix.MarketData.Domain/Schemas/SchemaVersions.cs (1)
7-12: Good implementation of centralized schema versioningThe centralization of schema versions is a good practice. Consider enhancing this class with:
- Methods for version comparison (IsNewerThan, IsCompatibleWith)
- Additional constants for future versions (V1, V2)
- Documentation on versioning strategy and upgrade paths
public static class SchemaVersions { public const string V0 = "0.0.0"; // Ready for future versions // public const string V1 = "1.0.0"; public static readonly HashSet<string> Supported = [V0]; /// <summary> /// Checks if a schema version is supported /// </summary> public static bool IsSupported(string version) => Supported.Contains(version); /// <summary> /// Compares two versions and returns true if version1 is newer than version2 /// </summary> public static bool IsNewerThan(string version1, string version2) { // Implementation for semantic version comparison return false; // Placeholder } }src/Phoenix.MarketData.Domain/Schemas/FxSpotPriceSchema.json (1)
59-63: Consider documenting time zone implications for asOfTimeThe
asOfTimeproperty is well-defined with a pattern for HH:mm or HH:mm:ss, but there's no mention of the time zone context. Consider documenting whether this time should be interpreted in UTC, local time, or the time zone of the specified region."asOfTime": { "type": "string", "pattern": "^[0-9]{2}:[0-9]{2}(:[0-9]{2})?$", - "description": "The specific time (HH:mm or HH:mm:ss) associated with the asOfDate, representing the moment the data was recorded. Example: '15:30' or '15:30:45'." + "description": "The specific time (HH:mm or HH:mm:ss) associated with the asOfDate, representing the moment the data was recorded. Example: '15:30' or '15:30:45'. Times are interpreted in the timezone of the specified region." },src/Phoenix.MarketData.Domain/Models/Interfaces/IMarketDataObject.cs (2)
9-11: Confirm that the sample ID format matchesBaseMarketDataObject.CalculateId()The sample value now includes the new
regioncomponent, which is great.
However, the example still prefixes the string with[dataType].[assetclass](using a dot) whereasBaseMarketDataObject.CalculateId()joins every segment with a double-underscore (__).
If consumers copy this comment literally they may create malformed IDs when querying.Consider either:
- Removing the dot in the comment, or
- Adding a second example that reflects the exact output of
CalculateId().
40-46: Region should be a constrained value (enum or constant list)
Regionis currently an unconstrainedstring, which makes downstream aggregation difficult ("EU"vs"eu"vs"EUROPE").
If only a finite set of regions is supported, expose them through:public static class Regions { public const string Global = "global"; public const string Europe = "eu"; // … }or an
enumwith a custom converter. This prevents typo-driven data fragmentation.src/Phoenix.MarketData.Domain/Models/BaseMarketDataObject.cs (2)
26-37: Consider adding a guard clause to prevent unnecessary Id recalculation.When setting the Version property, the Id is always recalculated regardless of whether the value actually changed.
public string? Version { get => _version; set { + if (_version == value) + return; _version = value; // Invalidate the ID if the Version changes _id = CalculateId(); } }
11-79: Consider adding XML documentation for public properties.While the class has good XML documentation, adding documentation for individual properties would improve code readability and maintainability, especially for properties like Region and AsOfTime that have specific business meanings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Phoenix.MarketData.Functions/SaveDocumentToDb.cs(2 hunks)src/Phoenix.MarketData.Domain/Models/BaseMarketDataObject.cs(1 hunks)src/Phoenix.MarketData.Domain/Models/FxSpotPriceObject.cs(1 hunks)src/Phoenix.MarketData.Domain/Models/Interfaces/IMarketDataObject.cs(3 hunks)src/Phoenix.MarketData.Domain/Schemas/FxSpotPriceSchema.json(3 hunks)src/Phoenix.MarketData.Domain/Schemas/SchemaVersions.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
Phoenix.MarketData.Functions/SaveDocumentToDb.cs (1)
src/Phoenix.MarketData.Domain/Schemas/SchemaVersions.cs (1)
SchemaVersions(7-12)
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (1)
src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs (1)
Task(14-25)
src/Phoenix.MarketData.Domain/Models/FxSpotPriceObject.cs (1)
src/Phoenix.MarketData.Domain/Models/BaseMarketDataObject.cs (1)
BaseMarketDataObject(10-79)
🔇 Additional comments (9)
Phoenix.MarketData.Functions/SaveDocumentToDb.cs (1)
6-6: Schema versioning updated to use centralized version managementGood improvement to use the
SchemaVersions.V0constant instead of a hardcoded string. This aligns with the centralized version management approach.Also applies to: 29-29
src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs (1)
14-16:Details
✅ Verification successful
Ensure all callers of GetNextVersionAsync have been updated
This method signature change adds a required parameter without a default value. Ensure all callers have been updated to provide the region parameter to prevent runtime errors.
🏁 Script executed:
#!/bin/bash # Find all callers of GetNextVersionAsync that might not have been updated echo "Searching for calls to GetNextVersionAsync that might not include the region parameter..." rg "GetNextVersionAsync\s*<.*>\s*\(\s*.*,\s*.*,\s*.*,\s*\w+\s*,\s*\w+\s*\)" -g "*.cs" --multiline --ignore-caseLength of output: 302
Let’s broaden the search to catch every usage of
GetNextVersionAsync, including calls without explicit type arguments:
🏁 Script executed:
#!/bin/bash # Search for all references to GetNextVersionAsync in C# files echo "Searching for all occurrences of GetNextVersionAsync in .cs files..." rg "GetNextVersionAsync" -g "*.cs" -n -C2Length of output: 1399
All callers updated – no further action required
Verified that the only usage of
GetNextVersionAsyncinMarketDataRepository.csnow supplies the newregionargument (marketData.Region). No other calls exist, so no runtime issues should occur from this signature change.src/Phoenix.MarketData.Domain/Schemas/FxSpotPriceSchema.json (1)
44-47: Good implementation of region and asOfTime propertiesThe addition of region and asOfTime properties with proper descriptions and validation is well-implemented. Making region optional maintains backward compatibility with existing data.
Also applies to: 59-63
src/Phoenix.MarketData.Domain/Models/BaseMarketDataObject.cs (6)
1-10: Good implementation of a base class for market data objects.The abstract class design with XML documentation effectively centralizes core market data functionality and implements the
IMarketDataObjectinterface. This is a clean approach that will reduce code duplication across concrete market data classes.
12-14: Good use of backing fields for lazy initialization.The backing fields for
Id,CreatedTimestamp, andVersionprovide proper control over these properties. The nullable types are appropriate for this context.
16-21: Effective implementation of Id property with lazy initialization.The Id property is well-implemented with lazy calculation and a private setter that prevents external modification while allowing deserialization.
48-49: New Region property aligns with PR objectives.The required Region property properly implements the PR objective of enhancing regional data support.
67-68: New AsOfTime property aligns with PR objectives.The nullable TimeOnly property for AsOfTime appropriately implements the PR objective of adding timestamp accuracy to market data objects.
70-78:Details
❓ Verification inconclusive
Verify that all implementations use the same ID format.
The
CalculateIdmethod now incorporates the Region property into the ID format. Ensure this format is consistently used across all repositories and services that may construct these IDs independently.
🏁 Script executed:
#!/bin/bash # Search for other ID calculation implementations in the codebase rg "string\.Join\(\s*\"__\"\s*,\s*new\s*\[\]" --type csLength of output: 159
No additional ID generators found in the codebase
Only theCalculateIdimplementation in
src/Phoenix.MarketData.Domain/Models/BaseMarketDataObject.csremains, so there are no other in‐repo overrides to update.
Please verify that any external repositories or services which independently build these IDs have been updated to include the newRegionsegment. Consider adding or updating documentation (or a shared utility) to ensure consistency across all consumers.
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs
Outdated
Show resolved
Hide resolved
Replaced `FxSpotPriceObject` and `FxVolSurfaceObject` with `FxSpotPriceData` using a new DTO-based structure for better separation of concerns. Introduced mappings for serialization and a retry mechanism in `MarketDataRepository` to handle version conflicts. Standardized region definitions and updated schemas to clarify UTC timestamps.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src/Phoenix.MarketData.Domain/Schemas/SchemaVersions.cs (1)
1-33: Consider adding comprehensive version management capabilitiesThe class provides basic version support but lacks functionality that will likely be needed as the system evolves.
Consider enhancing this class with additional functionality:
- Add validation for semantic version format
- Implement methods to get the latest supported version
- Add capability to check version compatibility (e.g., backward compatibility)
- Include methods to convert between versions if needed
Example for getting the latest version:
/// <summary> /// Gets the latest supported schema version /// </summary> public static string GetLatestVersion() { if (Supported.Count == 0) return string.Empty; return Supported.OrderByDescending(v => Version.Parse(v)).First(); }src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs (4)
15-19: Consider more robust Id calculation logic.The lazy Id calculation is a good approach, but it might be safer to include checks to ensure all required properties are initialized before calculating.
public string Id { - get => _id ??= CalculateId(); // If not set, calculate it + get + { + if (_id == null) + { + // Check if required properties are initialized before calculating + if (string.IsNullOrEmpty(DataType) || string.IsNullOrEmpty(AssetClass) || + string.IsNullOrEmpty(AssetId) || string.IsNullOrEmpty(Region) || + string.IsNullOrEmpty(DocumentType)) + { + throw new InvalidOperationException("Cannot calculate Id: required properties are not initialized."); + } + _id = CalculateId(); + } + return _id; + } private set => _id = value; // Can only be set during deserialization }
44-44: Consider validating Region property.The Region property is marked as required, but there's no validation against a known set of valid regions.
-public required string Region { get; set; } +private string _region; +public required string Region +{ + get => _region; + set + { + // Consider validation against known regions + if (string.IsNullOrWhiteSpace(value)) + throw new ArgumentException("Region cannot be null or empty", nameof(value)); + + _region = value; + + // Invalidate ID if Region changes + if (_id != null) + _id = null; + } +}
50-54: Add clarification about UTC default for CreateTimestamp.The CreateTimestamp property defaulting to UTC now might not be clear to users. Consider adding a comment or documentation.
public DateTimeOffset CreateTimestamp { - get => _createTimeStamp ??= DateTimeOffset.UtcNow; + // Defaults to UTC time when accessed if not explicitly set + get => _createTimeStamp ??= DateTimeOffset.UtcNow; set => _createTimeStamp = value; }
60-68: Optimize Id calculation for potential null or empty Region.The Id calculation should be resilient against uninitialized properties, particularly the new Region field.
private string CalculateId() { + // Validate that required properties are initialized + if (string.IsNullOrEmpty(DataType) || string.IsNullOrEmpty(AssetClass) || + string.IsNullOrEmpty(AssetId) || string.IsNullOrEmpty(Region) || + string.IsNullOrEmpty(DocumentType)) + { + throw new InvalidOperationException("Cannot calculate Id: required properties are not initialized."); + } + var id = string.Join("__", new[] { DataType, AssetClass, AssetId, Region, AsOfDate.ToString("yyyyMMdd"), DocumentType}); if (Version != null) id += $"__{Version}"; return id; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Phoenix.MarketData.Functions/SaveDocumentToDb.cs(2 hunks)src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs(1 hunks)src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs(1 hunks)src/Phoenix.MarketData.Domain/Models/FxSpotPriceObject.cs(0 hunks)src/Phoenix.MarketData.Domain/Models/FxVolSurfaceData.cs(1 hunks)src/Phoenix.MarketData.Domain/Models/FxVolSurfaceObject.cs(0 hunks)src/Phoenix.MarketData.Domain/Models/Interfaces/IMarketDataObject.cs(3 hunks)src/Phoenix.MarketData.Domain/Regions.cs(1 hunks)src/Phoenix.MarketData.Domain/Schemas/FxSpotPriceSchema.json(3 hunks)src/Phoenix.MarketData.Domain/Schemas/SchemaVersions.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs(3 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDto.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceMapper.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataObjectDto.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceObjectDto.cs(1 hunks)
💤 Files with no reviewable changes (2)
- src/Phoenix.MarketData.Domain/Models/FxVolSurfaceObject.cs
- src/Phoenix.MarketData.Domain/Models/FxSpotPriceObject.cs
✅ Files skipped from review due to trivial changes (5)
- src/Phoenix.MarketData.Domain/Models/FxVolSurfaceData.cs
- src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs
- src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceObjectDto.cs
- src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDto.cs
- src/Phoenix.MarketData.Domain/Regions.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Phoenix.MarketData.Domain/Schemas/FxSpotPriceSchema.json
- Phoenix.MarketData.Functions/SaveDocumentToDb.cs
- src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs
- src/Phoenix.MarketData.Domain/Models/Interfaces/IMarketDataObject.cs
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceMapper.cs (4)
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDto.cs (1)
FxSpotPriceDto(6-10)src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs (1)
FxSpotPriceData(7-11)src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs (1)
BaseMarketDataMapper(6-41)src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataObjectDto.cs (1)
CopyBasePropertiesFrom(50-64)
src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs (2)
src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataObjectDto.cs (1)
BaseMarketDataDto(10-65)src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs (1)
BaseMarketData(9-69)
src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataObjectDto.cs (1)
src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs (1)
BaseMarketDataDto(8-25)
🔇 Additional comments (10)
src/Phoenix.MarketData.Domain/Schemas/SchemaVersions.cs (3)
9-11: Good start with semantic versioningThe implementation follows semantic versioning principles with clear version constants. The commented placeholder for V1 shows foresight for future versioning.
18-18: Collection initialization style is modern and cleanUsing the collection initialization syntax with square brackets (introduced in C# 12) is concise and readable.
23-23: Efficient implementation of version checkingThe
IsSupportedmethod implementation is clean and efficient using the HashSet for O(1) lookups.src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs (2)
1-3: Appropriate imports for the mapper class.The imports are correctly including both the domain models and serialization namespaces needed for the mapping operations.
6-7: Good use of static class for mapper functionality.Using a static class is appropriate for this stateless mapping functionality, following common mapper pattern practices in C#.
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceMapper.cs (1)
5-6: Class design follows consistent mapping pattern.The static class design is consistent with
BaseMarketDataMapper, providing a clean approach for mapping operations.src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataObjectDto.cs (1)
6-9: Appropriate XML documentation for the base DTO class.The documentation clearly explains the purpose and functionality of the base DTO class.
src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs (3)
5-10: Well-documented abstract base class.The abstract class definition and XML documentation clearly explain the purpose and role of this base class in the domain model hierarchy.
11-14: Good use of backing fields for special property behavior.The backing fields support the lazy initialization and special behavior of the Id, CreateTimestamp, and Version properties.
23-36: Version property correctly invalidates Id.The Version property setter correctly invalidates the Id when the version changes, maintaining consistency.
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceMapper.cs
Outdated
Show resolved
Hide resolved
| using Phoenix.MarketData.Domain.Models; | ||
|
|
||
| namespace Phoenix.MarketData.Infrastructure.Mapping; |
There was a problem hiding this comment.
Add missing import for FxSpotPriceDto.
The import for Phoenix.MarketData.Infrastructure.Serialization namespace is missing, which contains the FxSpotPriceDto class.
using Phoenix.MarketData.Domain.Models;
+using Phoenix.MarketData.Infrastructure.Serialization;
namespace Phoenix.MarketData.Infrastructure.Mapping;Committable suggestion skipped: line range outside the PR's diff.
src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataObjectDto.cs
Outdated
Show resolved
Hide resolved
| public class BaseMarketDataDto | ||
| { | ||
| [JsonProperty("id")] // Ensures this property is deserialized from JSON | ||
| public string Id { get; set; } | ||
|
|
||
| [JsonProperty("schemaVersion")] | ||
| public string SchemaVersion { get; set; } | ||
|
|
||
| [JsonProperty("version")] | ||
| public string? Version { get; set; } | ||
|
|
||
| [JsonProperty("assetId")] | ||
| public string AssetId { get; set; } | ||
|
|
||
| [JsonProperty("assetClass")] | ||
| public string AssetClass { get; set; } | ||
|
|
||
| [JsonProperty("dataType")] | ||
| public string DataType { get; set; } | ||
|
|
||
| [JsonProperty("region")] | ||
| public string Region { get; set; } | ||
|
|
||
| [JsonProperty("tags")] | ||
| public List<string> Tags { get; set; } | ||
|
|
||
| [JsonProperty("documentType")] | ||
| public string DocumentType { get; set; } | ||
|
|
||
| [JsonProperty("createTimestamp")] | ||
| public DateTimeOffset CreateTimestamp { get; set; } | ||
|
|
||
| [JsonProperty("asOfDate")] | ||
| [JsonConverter(typeof(DateOnlyConverter))] | ||
| public DateOnly AsOfDate { get; set; } | ||
|
|
||
| [JsonProperty("asOfTime")] | ||
| [JsonConverter(typeof(TimeOnlyConverter))] | ||
| public TimeOnly? AsOfTime { get; set; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize collections and consider required properties.
The class definition properly includes all necessary market data properties with appropriate JSON attributes. However, the Tags property doesn't have an initializer which could lead to NullReferenceExceptions, and many properties could benefit from the required modifier.
public class BaseMarketDataDto
{
[JsonProperty("id")] // Ensures this property is deserialized from JSON
public string Id { get; set; }
[JsonProperty("schemaVersion")]
- public string SchemaVersion { get; set; }
+ public required string SchemaVersion { get; set; }
[JsonProperty("version")]
public string? Version { get; set; }
[JsonProperty("assetId")]
- public string AssetId { get; set; }
+ public required string AssetId { get; set; }
[JsonProperty("assetClass")]
- public string AssetClass { get; set; }
+ public required string AssetClass { get; set; }
[JsonProperty("dataType")]
- public string DataType { get; set; }
+ public required string DataType { get; set; }
[JsonProperty("region")]
- public string Region { get; set; }
+ public required string Region { get; set; }
[JsonProperty("tags")]
- public List<string> Tags { get; set; }
+ public List<string> Tags { get; set; } = new();
[JsonProperty("documentType")]
- public string DocumentType { get; set; }
+ public required string DocumentType { get; set; }
// Other properties...📝 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 class BaseMarketDataDto | |
| { | |
| [JsonProperty("id")] // Ensures this property is deserialized from JSON | |
| public string Id { get; set; } | |
| [JsonProperty("schemaVersion")] | |
| public string SchemaVersion { get; set; } | |
| [JsonProperty("version")] | |
| public string? Version { get; set; } | |
| [JsonProperty("assetId")] | |
| public string AssetId { get; set; } | |
| [JsonProperty("assetClass")] | |
| public string AssetClass { get; set; } | |
| [JsonProperty("dataType")] | |
| public string DataType { get; set; } | |
| [JsonProperty("region")] | |
| public string Region { get; set; } | |
| [JsonProperty("tags")] | |
| public List<string> Tags { get; set; } | |
| [JsonProperty("documentType")] | |
| public string DocumentType { get; set; } | |
| [JsonProperty("createTimestamp")] | |
| public DateTimeOffset CreateTimestamp { get; set; } | |
| [JsonProperty("asOfDate")] | |
| [JsonConverter(typeof(DateOnlyConverter))] | |
| public DateOnly AsOfDate { get; set; } | |
| [JsonProperty("asOfTime")] | |
| [JsonConverter(typeof(TimeOnlyConverter))] | |
| public TimeOnly? AsOfTime { get; set; } | |
| public class BaseMarketDataDto | |
| { | |
| [JsonProperty("id")] // Ensures this property is deserialized from JSON | |
| public string Id { get; set; } | |
| [JsonProperty("schemaVersion")] | |
| public required string SchemaVersion { get; set; } | |
| [JsonProperty("version")] | |
| public string? Version { get; set; } | |
| [JsonProperty("assetId")] | |
| public required string AssetId { get; set; } | |
| [JsonProperty("assetClass")] | |
| public required string AssetClass { get; set; } | |
| [JsonProperty("dataType")] | |
| public required string DataType { get; set; } | |
| [JsonProperty("region")] | |
| public required string Region { get; set; } | |
| [JsonProperty("tags")] | |
| public List<string> Tags { get; set; } = new(); | |
| [JsonProperty("documentType")] | |
| public required string DocumentType { get; set; } | |
| [JsonProperty("createTimestamp")] | |
| public DateTimeOffset CreateTimestamp { get; set; } | |
| [JsonProperty("asOfDate")] | |
| [JsonConverter(typeof(DateOnlyConverter))] | |
| public DateOnly AsOfDate { get; set; } | |
| [JsonProperty("asOfTime")] | |
| [JsonConverter(typeof(TimeOnlyConverter))] | |
| public TimeOnly? AsOfTime { get; set; } | |
| } |
Added standardized null checks across mappers and DTO methods for robustness. Introduced property validation for lists and enhanced semantic version comparison logic. Moved schema files to the appropriate infrastructure layer for better organization.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceMapper.cs (1)
1-2:⚠️ Potential issueMissing import for FxSpotPriceDto.
The code is missing an import for the
FxSpotPriceDtoclass from thePhoenix.MarketData.Infrastructure.Serializationnamespace. Without this import, the compiler will not be able to resolve references to this type.using Phoenix.MarketData.Domain.Models; +using Phoenix.MarketData.Infrastructure.Serialization;
🧹 Nitpick comments (2)
src/Phoenix.MarketData.Infrastructure/Schemas/SchemaVersions.cs (2)
9-11: Consider using semantic versioning constantsThe versioning approach seems to align with semantic versioning (SemVer), but this isn't explicitly stated. Making this explicit would improve code clarity and set expectations for future version additions.
You could enhance the documentation and add a more descriptive constant naming:
- public const string V0 = "0.0.0"; - // Ready for future versions - // public const string V1 = "1.0.0"; + /// <summary> + /// Initial schema version (SemVer: Major.Minor.Patch) + /// </summary> + public const string V0 = "0.0.0"; + + // Ready for future versions + // public const string V1 = "1.0.0";
18-18: Consider adding a method to retrieve the latest supported versionThe class maintains a set of supported versions but doesn't provide a way to easily determine the latest one, which could be useful in many scenarios.
Add a method to retrieve the latest supported version:
public static readonly HashSet<string> Supported = [V0]; +/// <summary> +/// Returns the latest supported schema version. +/// </summary> +/// <returns>The latest supported schema version.</returns> +public static string GetLatestVersion() +{ + return Supported.Count > 0 + ? Supported.OrderBy(v => Version.Parse(v)).Last() + : string.Empty; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs(1 hunks)src/Phoenix.MarketData.Domain/Phoenix.MarketData.Domain.csproj(0 hunks)src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceMapper.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Phoenix.MarketData.Infrastructure.csproj(1 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/FxSpotPriceSchema.json(3 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/SchemaVersions.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataObjectDto.cs(1 hunks)
💤 Files with no reviewable changes (1)
- src/Phoenix.MarketData.Domain/Phoenix.MarketData.Domain.csproj
✅ Files skipped from review due to trivial changes (2)
- src/Phoenix.MarketData.Infrastructure/Phoenix.MarketData.Infrastructure.csproj
- src/Phoenix.MarketData.Infrastructure/Schemas/FxSpotPriceSchema.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs
- src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs
- src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs
- src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataObjectDto.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceMapper.cs (4)
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDto.cs (1)
FxSpotPriceDto(6-10)src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs (1)
FxSpotPriceData(7-11)src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs (2)
BaseMarketDataMapper(6-46)ApplyToDomain(29-45)src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataObjectDto.cs (1)
CopyBasePropertiesFrom(50-65)
🔇 Additional comments (4)
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceMapper.cs (3)
7-21: Well-implemented ToDto method with proper null checking.Good implementation with proper null validation using
ArgumentNullException.ThrowIfNull(). The method correctly maps thePriceproperty and leverages the base mapper to handle common properties.
23-31: Well-implemented bidirectional mapping support.The
ApplyToDomainmethod properly implements the missing bidirectional mapping functionality. It includes appropriate null validation for both parameters and correctly maps specific and base properties.
5-32:Details
✅ Verification successful
Verify integration with repository layer.
This mapper class is a key part of the infrastructure for the new market data model with region and asOfTime support. Ensure it's properly integrated with the repository layer that consumes it.
🏁 Script executed:
#!/bin/bash # Search for repository classes that use this mapper rg -A 5 "FxSpotPriceMapper" --type csharpLength of output: 1468
Integration Verified: FxSpotPriceMapper is wired into the repository layer
We confirmed that
FxSpotPriceMapper.ToDtois invoked inMarketDataRepository.cswhen projectingFxSpotPriceDatatoFxSpotPriceDto. Base properties (including region and asOfTime) flow throughBaseMarketDataMapper. No further integration gaps were found.• Location of usage:
- src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (switch mapping to
FxSpotPriceMapper.ToDto)src/Phoenix.MarketData.Infrastructure/Schemas/SchemaVersions.cs (1)
1-38: Overall implementation is clean and well-structuredThe SchemaVersions class provides a clear, centralized approach to schema version management with good documentation and a clean API. The use of a static class with constants and utility methods is appropriate for this use case.
src/Phoenix.MarketData.Infrastructure/Schemas/SchemaVersions.cs
Outdated
Show resolved
Hide resolved
Replaced string-based versioning with integer-based for consistency, updated serialization classes, and introduced custom JSON converters for DateOnly and TimeOnly types. Removed redundant DTO, renamed and reorganized mapping and serialization classes, aligning with new conventions.
WalkthroughThis update refactors the market data domain and infrastructure to introduce new base and FX spot price data models, region and time granularity support, and schema version management. It adds robust mapping and serialization logic, updates repository and versioning interfaces, and removes obsolete FX object classes. JSON serialization is globally configured for custom date and time handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Repository as MarketDataRepository
participant VersionMgr as VersionManager
participant Mapper
participant CosmosDB
Caller->>Repository: SaveAsync(marketData, saveNextVersion)
alt saveNextVersion = true
Repository->>VersionMgr: GetNextVersionAsync(..., region, ...)
VersionMgr->>Repository: GetLatestAsync(..., region, ...)
Repository-->>VersionMgr: latestVersion
VersionMgr-->>Repository: nextVersion (int)
Repository->>marketData: set Version
end
Repository->>Mapper: MapToDto(marketData)
Mapper-->>Repository: DTO
Repository->>CosmosDB: Insert DTO
alt Conflict (409)
Repository->>Repository: Retry up to 3 times with backoff
end
CosmosDB-->>Repository: Success/Failure
Repository-->>Caller: Task Complete / Exception
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (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 (
|
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs (1)
1-11: Domain Model for FX Spot Price
TheFxSpotPriceDataclass correctly inherits fromBaseMarketDataand introduces a requiredPriceproperty. Consider adding an XML doc comment for thePriceproperty to maintain consistency with the class summary and improve documentation.src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs (1)
1-9: DTO for FX Spot Price Serialization
The DTO extendsBaseMarketDataDtoand properly decorates thePriceproperty for JSON serialization. For consistency with the domain layer, consider adding XML comments to the class and thePriceproperty.src/Phoenix.MarketData.Domain/Regions.cs (1)
3-9: Well-structured constants class for region supportThe
Regionsclass provides a clean implementation of region constants that will help standardize region references throughout the codebase. This aligns well with the PR objective to add region support to market data objects.Consider adding XML documentation to the class and each constant to provide more context about their usage and meaning, especially for less obvious abbreviations like "ldn" and "jhb".
src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs (1)
22-26: Consider adding culture specification to WriteJson for consistencyFor consistency with the
DateOnlyJsonConverterimplementation shown in the context snippets, consider specifying the culture when formatting the TimeOnly value.- writer.WriteValue(value.ToString(Format)); + writer.WriteValue(value.ToString(Format, System.Globalization.CultureInfo.InvariantCulture));src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs (1)
8-22: Consider optimizing the ToDto method to avoid intermediate object creationThe current implementation creates an unnecessary intermediate BaseMarketDataDto object that's only used for property copying, which is inefficient.
public static FxSpotPriceDataDto ToDto(FxSpotPriceData domain) { ArgumentNullException.ThrowIfNull(domain); var dto = new FxSpotPriceDataDto { Price = domain.Price, - }; - - // Apply base properties - var baseDto = BaseMarketDataMapper.ToDto(domain); - dto.CopyBasePropertiesFrom(baseDto); + // Apply base properties directly + Id = domain.Id, + SchemaVersion = domain.SchemaVersion, + Version = domain.Version, + AssetId = domain.AssetId, + AssetClass = domain.AssetClass, + DataType = domain.DataType, + Region = domain.Region, + DocumentType = domain.DocumentType, + CreateTimestamp = domain.CreateTimestamp, + AsOfDate = domain.AsOfDate, + AsOfTime = domain.AsOfTime, + Tags = domain.Tags + }; return dto; }Alternatively, if you want to maintain consistency with the current pattern, consider adding a method to BaseMarketDataMapper that populates an existing DTO instead of creating a new one.
src/Phoenix.MarketData.Infrastructure/Schemas/SchemaVersions.cs (1)
32-48: Improve exception handling inIsNewerThanmethod.The current implementation catches
FormatExceptiononly to throw a new one with a more descriptive message. This creates an unnecessary exception chaining and loses the original exception details.Consider including the original exception as the inner exception when rethrowing:
catch (FormatException ex) { - throw new FormatException("Could not parse version string when trying to compare versions."); + throw new FormatException("Could not parse version string when trying to compare versions.", ex); }src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs (1)
11-13: Inconsistent casing in private backing field name.The naming convention for private fields appears inconsistent. The
_createTimeStampfield uses camel case with an uppercase 'S' while other fields use consistent camel casing.- private DateTimeOffset? _createTimeStamp; // Backing field for CreatedTimestamp + private DateTimeOffset? _createTimestamp; // Backing field for CreateTimestampsrc/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (1)
90-101: Consider implementation timeline for commented mapper placeholders.The MapToDto method effectively uses pattern matching to handle different market data types, but contains commented placeholders for future mappers.
return domain switch { FxSpotPriceData fxSpotData => FxSpotPriceDataMapper.ToDto(fxSpotData), - //Add other mappers... - //ForwardPriceData f => ForwardPriceMapper.ToDto(f), - //YieldCurveData y => YieldCurveMapper.ToDto(y), - //VolatilitySurfaceData v => VolatilitySurfaceMapper.ToDto(v), + // TODO: Implement additional mappers in future PRs - TICKET-123 _ => throw new NotSupportedException($"Unsupported market data type: {typeof(T).Name}") };If these additional mappers are planned for future work rather than this PR, consider replacing the commented code with a single TODO comment that references a ticket number for better tracking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
Phoenix.MarketData.Functions/Program.cs(2 hunks)Phoenix.MarketData.Functions/SaveDocumentToDb.cs(2 hunks)src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs(1 hunks)src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs(1 hunks)src/Phoenix.MarketData.Domain/Models/FxSpotPriceObject.cs(0 hunks)src/Phoenix.MarketData.Domain/Models/FxVolSurfaceData.cs(1 hunks)src/Phoenix.MarketData.Domain/Models/FxVolSurfaceObject.cs(0 hunks)src/Phoenix.MarketData.Domain/Models/Interfaces/IMarketDataObject.cs(4 hunks)src/Phoenix.MarketData.Domain/Phoenix.MarketData.Domain.csproj(0 hunks)src/Phoenix.MarketData.Domain/Regions.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs(3 hunks)src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Phoenix.MarketData.Infrastructure.csproj(1 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/FxSpotPriceSchema.json(3 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/SchemaVersions.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/DateOnlyJsonConverter.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs(1 hunks)
💤 Files with no reviewable changes (3)
- src/Phoenix.MarketData.Domain/Phoenix.MarketData.Domain.csproj
- src/Phoenix.MarketData.Domain/Models/FxVolSurfaceObject.cs
- src/Phoenix.MarketData.Domain/Models/FxSpotPriceObject.cs
🧰 Additional context used
🧬 Code Graph Analysis (9)
Phoenix.MarketData.Functions/Program.cs (2)
src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs (1)
TimeOnlyJsonConverter(18-34)src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/DateOnlyJsonConverter.cs (1)
DateOnlyJsonConverter(16-30)
src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs (2)
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs (1)
FxSpotPriceDataDto(8-22)src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs (1)
BaseMarketDataDto(10-66)
src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs (1)
src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/DateOnlyJsonConverter.cs (1)
WriteJson(20-23)
src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs (1)
src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs (1)
BaseMarketData(9-74)
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs (4)
src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs (1)
FxSpotPriceDataDto(5-9)src/Phoenix.MarketData.Domain/Models/FxSpotPriceData.cs (1)
FxSpotPriceData(7-11)src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs (1)
BaseMarketDataMapper(6-46)src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs (1)
CopyBasePropertiesFrom(50-65)
src/Phoenix.MarketData.Domain/Models/Interfaces/IMarketDataObject.cs (2)
src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/DateOnlyJsonConverter.cs (1)
DateOnly(25-29)src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs (1)
TimeOnly(28-33)
src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/DateOnlyJsonConverter.cs (1)
src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs (1)
WriteJson(22-26)
src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs (1)
src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (2)
Task(28-63)Task(66-88)
src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs (2)
src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/DateOnlyJsonConverter.cs (2)
DateOnlyJsonConverter(16-30)DateOnly(25-29)src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs (2)
TimeOnlyJsonConverter(18-34)TimeOnly(28-33)
🔇 Additional comments (16)
src/Phoenix.MarketData.Infrastructure/Phoenix.MarketData.Infrastructure.csproj (1)
11-11: Centralize JSON Dependency in Infrastructure
Adding theNewtonsoft.Jsonpackage here aligns with the goal of consolidating serialization logic. Ensure this version (13.0.3) is compatible with all downstream projects and check for any known vulnerabilities (e.g., via a security advisory scan).Phoenix.MarketData.Functions/Program.cs (1)
9-11: Global JSON Converter Registration
RegisteringTimeOnlyJsonConverterandDateOnlyJsonConverterviaJsonConvert.DefaultSettingsensures consistent serialization ofTimeOnlyandDateOnlytypes across the Functions project. Verify this executes before any JSON operations and consider extending the settings (e.g.,NullValueHandling,Culture) if future requirements arise.Also applies to: 35-41
Phoenix.MarketData.Functions/SaveDocumentToDb.cs (2)
5-8: LGTM: Added necessary imports for new domain typesThe added imports correctly reference the new domain namespace and schema version constants needed for the updated implementation.
28-40: Correctly implemented market data object with new region and time propertiesThe code successfully transitions from using the old
FxSpotPriceObjectto the newFxSpotPriceDataclass with proper initialization of all required properties, including the newly addedRegionandAsOfTime. The use ofSchemaVersions.V0aligns with the centralized version management approach mentioned in the PR objectives.src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs (1)
5-18: Good documentation with comprehensive XML commentsThe XML documentation clearly explains the purpose, behavior, and usage of the converter, which will be valuable for other developers working with the code.
src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs (1)
24-32: LGTM: ApplyToDomain implementation is clean and effectiveThe method correctly applies the specific property (Price) before delegating to the base mapper for common properties. The null checks are properly implemented using ArgumentNullException.ThrowIfNull.
src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs (1)
1-47: Well-designed mapper implementation for BaseMarketDataThe implementation provides clean mapping between the domain model and DTO, including proper null validation and defensive copying of collections. Good use of
ArgumentNullException.ThrowIfNull()for parameter validation and proper handling of the Tags collection.src/Phoenix.MarketData.Infrastructure/Schemas/FxSpotPriceSchema.json (3)
6-6: Update of required fields aligns with domain model changesThe schema correctly updates the required fields to use
asOfDateinstead oftimestamp, matching the domain model changes that replace timestamp with more granular date representation.
44-47: Region property added correctlyGood addition of the region property with appropriate description explaining its purpose for geographic market segmentation.
59-63: Time representation implemented with clear formatThe asOfTime property is well-defined with an appropriate regex pattern that validates both HH:mm and HH:mm:ss formats. The description clearly explains the UTC time interpretation.
src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs (1)
14-27: Version management updated correctly for region supportThe method has been properly updated to include region support with appropriate validation. The return type change from
Task<string>toTask<int>aligns with the domain model update where Version is now anint?instead of a string.src/Phoenix.MarketData.Domain/Models/Interfaces/IMarketDataObject.cs (1)
76-81:Details
✅ Verification successful
Ensure TimeOnly serialization is properly configured across the application.
The new
TimeOnly?property requires proper JSON serialization. I see the converters are defined, but confirming they're registered in the DI container is essential.
🏁 Script executed:
#!/bin/bash # Check if JsonConverters are registered in the application echo "Checking for JsonConverter registration..." rg -p "AddJsonOptions|JsonConverter|configure.*json" --type csharpLength of output: 1193
🏁 Script executed:
#!/bin/bash # Verify JSON converter registration in Program.cs echo "Searching for AddJsonOptions usage in Program.cs..." rg -n "AddJsonOptions" src/Phoenix.MarketData.Functions/Program.cs echo "Inspecting converter additions in Program.cs..." rg -n "Converters.Add" src/Phoenix.MarketData.Functions/Program.csLength of output: 519
🏁 Script executed:
#!/bin/bash # Locate all Program.cs files in the repo echo "Locating Program.cs files..." find . -type f -name Program.cs # Search for JSON converter registration patterns across all C# files echo "Searching for converter registrations..." rg -n "Converters\.Add" --type cs || true rg -n "AddJsonOptions" --type cs || true rg -n "Configure<.*Json" --type cs || true rg -n "SerializerOptions" --type cs || trueLength of output: 403
🏁 Script executed:
#!/bin/bash # Search for TimeOnlyJsonConverter usage in Functions Program.cs echo "Locating converter usage..." rg -n "TimeOnlyJsonConverter" -C 3 Phoenix.MarketData.Functions/Program.cs || true # Print the top of Program.cs to inspect JSON configuration echo "Printing Program.cs head..." sed -n '1,80p' Phoenix.MarketData.Functions/Program.csLength of output: 2567
JSON Converter Registration Confirmed
I’ve verified that the
TimeOnlyJsonConverteris properly registered:
Global Newtonsoft setup in
Phoenix.MarketData.Functions/Program.cs
JsonConvert.DefaultSettings includesConverters = [ new TimeOnlyJsonConverter(), new DateOnlyJsonConverter() ]Property-level attributes in
Infrastructure/Serialization/BaseMarketDataDto.cs
Properties decorated with[JsonConverter(typeof(TimeOnlyJsonConverter))] [JsonConverter(typeof(DateOnlyJsonConverter))]No additional DI registration is required for Newtonsoft-based serialization.
src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs (1)
50-65: LGTM! The CopyBasePropertiesFrom method has proper null checking.The method includes appropriate null argument validation and defensive copying of the
Tagscollection, which are good practices for preventing bugs.src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs (3)
1-5: Appropriate imports added for new functionality.The additions of System.Net and Phoenix.MarketData.Infrastructure.Mapping namespaces are necessary to support the new error handling and mapping functionality.
30-63: Good implementation of retry logic addressing previous race condition concerns.The retry mechanism with backoff delay properly addresses the race condition mentioned in the past review comment. This implementation:
- Attempts to save with up to 3 retries
- Catches 409 Conflict exceptions specifically when attempting to save next version
- Uses a simple backoff strategy with increasing delay
- Throws an explicit exception if all retries fail
The addition of mapping through MapToDto also supports the domain model refactoring mentioned in the PR objectives.
66-76: Region parameter correctly added to query.The region parameter has been appropriately added to the method signature and SQL query, aligning with the PR objective of adding region support to market data objects.
| @@ -0,0 +1 @@ | |||
| No newline at end of file | |||
There was a problem hiding this comment.
Remove or Implement Placeholder File
The file is currently empty (only contains a BOM) and does not declare any class. If FxVolSurfaceData is intended for future use, please add at least a class stub or documentation; otherwise, remove this file to avoid confusion.
src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs
Show resolved
Hide resolved
| using System.Globalization; | ||
| using Newtonsoft.Json; | ||
|
|
||
| namespace Phoenix.MarketData.Infrastructure.Serialization.JsonConverters | ||
| { | ||
| /// <summary> | ||
| /// A custom JSON converter for serializing and deserializing <see cref="DateOnly"/> values. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This converter handles <see cref="DateOnly"/> types by formatting them as strings | ||
| /// in the "yyyy-MM-dd" format during serialization and deserialization. | ||
| /// </remarks> | ||
| /// <example> | ||
| /// Use this converter with <see cref="JsonConverterAttribute"/> to convert <see cref="DateOnly"/> properties in JSON. | ||
| /// </example> | ||
| public class DateOnlyJsonConverter : JsonConverter<DateOnly> | ||
| { | ||
| private const string Format = "yyyy-MM-dd"; | ||
|
|
||
| public override void WriteJson(JsonWriter writer, DateOnly value, JsonSerializer serializer) | ||
| { | ||
| writer.WriteValue(value.ToString(Format, CultureInfo.InvariantCulture)); | ||
| } | ||
|
|
||
| public override DateOnly ReadJson(JsonReader reader, Type objectType, DateOnly existingValue, bool hasExistingValue, JsonSerializer serializer) | ||
| { | ||
| var str = reader.Value?.ToString(); | ||
| return DateOnly.ParseExact(str!, Format, CultureInfo.InvariantCulture); | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Improve null handling in DateOnly JSON converter
The converter is well-implemented overall with good documentation and use of invariant culture. However, there's a potential issue in the ReadJson method.
The forced unwrap on line 28 (str!) will throw an exception if str is null. Consider adding proper null handling:
public override DateOnly ReadJson(JsonReader reader, Type objectType, DateOnly existingValue, bool hasExistingValue, JsonSerializer serializer)
{
var str = reader.Value?.ToString();
- return DateOnly.ParseExact(str!, Format, CultureInfo.InvariantCulture);
+ if (string.IsNullOrEmpty(str))
+ {
+ throw new JsonSerializationException("Cannot convert null or empty string to DateOnly.");
+ }
+
+ return DateOnly.ParseExact(str, Format, CultureInfo.InvariantCulture);
}This change provides more descriptive error messages and avoids potential null reference exceptions.
| /// <summary> | ||
| /// Represents the version of a market data object. This property is used to track | ||
| /// incremental updates or changes to the data over time. | ||
| /// </summary> | ||
| int? Version { get; set; } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Breaking change: Version property type changed from string to int.
Changing the Version property type from string? to int? is a breaking change that will require updates to all implementations and consumers of this interface.
🏁 Script executed:
#!/bin/bash
# Identify code that might be affected by the Version property type change
echo "Searching for Version property usage in implementations..."
rg -p "Version\s*{\s*get" --type csharp
echo "Searching for Version property assignments..."
rg -p "\.Version\s*=" --type csharp
echo "Searching for Version property access..."
rg -p "\.Version\b" --type csharp | grep -v "\.Version\s*="Length of output: 1377
Breaking Change: Version property type changed from string? to int?
The change of the IMarketDataObject.Version from string? → int? is breaking and requires updates to all implementations and consumers:
• src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs
– Line 16: Version = domain.Version;
– Line 35: domain.Version = dto.Version;
• src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs
– Line 55: public string? Version { get; set; } (needs to be int? or conversion logic)
• src/Phoenix.MarketData.Infrastructure/Cosmos/VersionManager.cs
– Line 23: if (latest == null || latest.Version == null)
– Line 26: return latest.Version.Value + 1;
• src/Phoenix.MarketData.Infrastructure/Cosmos/MarketDataRepository.cs
– Line 36: marketData.Version = await _versionManager.GetNextVersionAsync<T>(…);
Action items:
- Update DTO’s
Versionproperty type or add appropriateint⇄stringconversions. - Adjust mapping in
BaseMarketDataMapperto parse/format the version. - Review repository and version-management logic to ensure all numeric operations compile and behave as expected.
| /// in ISO 8601 format, e.g., "2023-10-19T15:23:00Z". | ||
| /// </summary> | ||
| DateTimeOffset CreatedTimestamp { get; } | ||
| DateTimeOffset CreateTimestamp { get; } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Breaking change: Property renamed from CreatedTimestamp to CreateTimestamp.
The property has been renamed from CreatedTimestamp to CreateTimestamp (removing the 'd'). This is a breaking change that will affect all implementations and consumers of this interface.
🏁 Script executed:
#!/bin/bash
# Identify code that might be affected by the timestamp property rename
echo "Searching for CreatedTimestamp property usage..."
rg -p "CreatedTimestamp" --type csharpLength of output: 287
Breaking Change: Update Implementation for CreateTimestamp
The interface property was renamed from CreatedTimestamp to CreateTimestamp. This requires updating any implementation or references to the old name.
• File: src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs (line 12)
- The backing-field comment still refers to
CreatedTimestampand should be updated toCreateTimestamp.
Suggested diff:
- private DateTimeOffset? _createTimeStamp; // Backing field for CreatedTimestamp
+ private DateTimeOffset? _createTimeStamp; // Backing field for CreateTimestampPlease verify there are no other lingering references (e.g., in tests or other consumers) to the old property name.
Committable suggestion skipped: line range outside the PR's diff.
| [JsonProperty("asOfTime")] | ||
| [JsonConverter(typeof(TimeOnlyJsonConverter))] | ||
| public TimeOnly? AsOfTime { get; set; } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
JSON converter implementation for TimeOnly appears incomplete.
The TimeOnlyJsonConverter being applied here uses TimeOnly.Parse() which doesn't enforce a specific format, potentially leading to inconsistent serialization/deserialization.
Looking at the converter implementation in the provided code snippet, I notice it uses TimeOnly.Parse(str!) without specifying a format. This is inconsistent with the DateOnlyJsonConverter which uses a specific format string.
🏁 Script executed:
#!/bin/bash
# Check how the TimeOnly converter is used elsewhere in the codebase
echo "Checking TimeOnly converter usage and implementation..."
rg -p "TimeOnly" --type csharp -A 5 -B 5Length of output: 3951
Enforce strict time format in TimeOnlyJsonConverter ReadJson
The converter’s WriteJson uses the "HH:mm:ss" constant, but ReadJson still calls TimeOnly.Parse(str!), which accepts multiple formats. This can lead to unexpected parsing behavior. Please update ReadJson to use TimeOnly.ParseExact with the same format (and specify CultureInfo.InvariantCulture).
- File: src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs
Location: ReadJson method (around line 32)
Suggested diff:
--- a/src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs
+++ b/src/Phoenix.MarketData.Infrastructure/Serialization/JsonConverters/TimeOnlyJsonConverter.cs
@@ -30,7 +30,9 @@ public class TimeOnlyJsonConverter : JsonConverter<TimeOnly>
var str = reader.Value?.ToString();
- return TimeOnly.Parse(str!);
+ // Enforce the same "HH:mm:ss" format on deserialization
+ return TimeOnly.ParseExact(str!, Format, CultureInfo.InvariantCulture);
}- Add
using System.Globalization;at the top of the file.
| public int? Version | ||
| { | ||
| get => _version; | ||
| set | ||
| { | ||
| if (_version == value) | ||
| return; | ||
|
|
||
| _version = value; | ||
|
|
||
| // Invalidate the ID if the Version changes | ||
| _id = CalculateId(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ID recalculation happens only when Version changes, not other ID components.
The Version property setter invalidates and recalculates the ID, but changes to other ID components (AssetId, Region, etc.) don't trigger recalculation.
Consider implementing similar logic for other properties that contribute to the ID, or use a more general solution that tracks changes to all relevant properties:
public required string AssetId { get; set; }
+ {
+ get => _assetId;
+ set
+ {
+ if (_assetId == value)
+ return;
+
+ _assetId = value;
+ _id = null; // Invalidate ID when component changes
+ }
+ }Alternatively, consider using C# property change notification or implementing a generalized solution that invalidates the ID whenever any of its components change.
Committable suggestion skipped: line range outside the PR's diff.
…ters/TimeOnlyJsonConverter.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Replaced property-by-property mapping with constructor-based initialization for DTOs, improving clarity and reducing boilerplate. Added validation for ID calculations and enhanced error handling for schema version parsing. Adjusted the JSON deserialization logic for TimeOnly to handle culture-specific formats.
Added checks for null, empty, and invalid string formats during deserialization. Introduced TryParse method for safer conversion and clearer
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs (2)
5-9: Consider making the Price property immutable.The Price property is currently mutable with both a getter and setter. Since DTOs are typically used for data transfer rather than data manipulation, consider making it immutable to prevent unintended modifications after construction.
- public decimal Price { get; set; } + public decimal Price { get; }
10-15: Long parameter list in constructor might benefit from a builder pattern.The constructor has a large number of parameters which can make it difficult to use correctly. Consider implementing a builder pattern or using record types if you're using C# 9.0+ to improve readability and maintainability.
For example, with C# records:
public record FxSpotPriceDataDto : BaseMarketDataDto { [JsonProperty("price")] public decimal Price { get; init; } public FxSpotPriceDataDto(BaseMarketDataDto baseData, decimal price) : base(baseData) { Price = price; } }This assumes that BaseMarketDataDto would be updated to support copying its properties.
src/Phoenix.MarketData.Infrastructure/Schemas/SchemaVersions.cs (2)
54-67: Exception handling implemented, but logging is missingThe
GetLatestVersionmethod now includes proper exception handling for malformed version strings, addressing a previous review suggestion. However, the comment on line 65 mentions "Log the error" but no actual logging is implemented.Consider implementing actual logging:
catch (FormatException ex) { // Log the error and return a fallback version or rethrow with better context + _logger.LogError(ex, "Invalid version format found in supported versions."); throw new FormatException("Invalid version format found in supported versions.", ex); }Or remove the logging comment if logging isn't needed:
catch (FormatException ex) { - // Log the error and return a fallback version or rethrow with better context throw new FormatException("Invalid version format found in supported versions.", ex); }
7-68: Consider adding version validation in static constructorWhile the class handles exceptions during runtime operations, it would be beneficial to validate the format of version strings when the class is initialized to catch configuration errors early.
Consider adding a static constructor to validate all predefined versions:
public static class SchemaVersions { /// <summary> /// Initial schema version (SemVer: Major.Minor.Patch) /// </summary> public const string V0 = "0.0.0"; // Ready for future versions // public const string V1 = "1.0.0"; /// <summary> /// Represents the collection of schema versions supported within the Phoenix MarketData domain. /// This set of versions is used to validate and check compatibility for schema versioning in /// operations and data handling. /// </summary> public static readonly HashSet<string> Supported = [V0]; + static SchemaVersions() + { + // Validate all supported versions at startup + foreach (var version in Supported) + { + try + { + _ = Version.Parse(version); + } + catch (FormatException ex) + { + throw new FormatException($"Invalid version format '{version}' in SchemaVersions.", ex); + } + } + } // Rest of the class... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Schemas/SchemaVersions.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs(1 hunks)src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Phoenix.MarketData.Infrastructure/Mapping/BaseMarketDataMapper.cs
- src/Phoenix.MarketData.Infrastructure/Mapping/FxSpotPriceDataMapper.cs
- src/Phoenix.MarketData.Infrastructure/Serialization/BaseMarketDataDto.cs
- src/Phoenix.MarketData.Domain/Models/BaseMarketData.cs
🔇 Additional comments (4)
src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs (1)
1-4: Appropriate imports and namespace.The inclusion of Newtonsoft.Json is appropriate for the JSON serialization needs of this DTO.
src/Phoenix.MarketData.Infrastructure/Schemas/SchemaVersions.cs (3)
1-1: Correct namespace implementationThe namespace now correctly matches the file location in the Infrastructure project, addressing the inconsistency noted in a previous review.
32-48: Exception handling properly implementedThe
IsNewerThanmethod now includes proper exception handling for malformed version strings, addressing the suggestion from a previous review.
22-22:Details
✅ Verification successful
Verify C# version compatibility
The code uses collection expressions
[V0]which is a C# 12.0 feature. Ensure your build pipeline and all development environments support this language version.
🏁 Script executed:
#!/bin/bash # Check project file for language version setting echo "Checking .csproj files for language version:" find src -name "*.csproj" -exec grep -l "<LangVersion>" {} \; -exec grep -A 1 "<LangVersion>" {} \; # Check global.json if it exists echo "Checking global.json for SDK version:" if [ -f global.json ]; then cat global.json fiLength of output: 439
C# 12 support confirmed via .NET 9.0 SDK
Project’s global.json pins the SDK at 9.0.0, which includes C# 12, and with no
<LangVersion>override the compiler defaults toLatestMajor. The collection expression syntax ([V0]) is therefore supported—no changes required.
src/Phoenix.MarketData.Infrastructure/Serialization/FxSpotPriceDataDto.cs
Show resolved
Hide resolved
Marked the `MarketDataRepository` field as `readonly` for immutability and consistency. Updated the ID calculation logic to return an empty string instead of throwing an exception when required properties are missing.
Introduced `AssetClass` and added descriptive comments for `Regions` to streamline market data classification. Updated references in `SaveDocumentToDb` to use the new `AssetClass.Fx` constant for better type safety and consistency.
1 participant
Introduced new properties
regionandasOfTimeacross domain models, schemas, and repository methods for better regional and timestamp granularity in market data handling. RefactoredFxSpotPriceObjectto inherit from a newBaseMarketDataObject, consolidating shared logic. Updated schema versioning with a centralized version management system.➕ What does this PR do?
🔨 Changes
✅ Checklist
🗒 Notes for reviewer
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores