Use DTO for uv.lock parsing#1851
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors uv.lock parsing to use Tomlyn’s typed model binding (DTOs) instead of manually traversing TOML model objects, simplifying parsing logic while intentionally reducing tolerance for malformed uv.lock structures.
Changes:
- Replaced manual TOML table/array parsing in
UvLockwithToml.ToModel<UvLock>and a normalization pass. - Introduced DTOs/attributes for uv.lock structures (
UvPackage,UvDependency,UvSource, and newUvMetadata) to support typed binding. - Updated and reduced unit tests to match the new parsing behavior (e.g., malformed shapes now throw instead of being partially ignored).
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.ComponentDetection.Detectors.Tests/UvLockTests.cs | Updates tests to align with DTO-based parsing and new exception behavior for malformed TOML shapes. |
| src/Microsoft.ComponentDetection.Detectors/uv/UvSource.cs | Adds TOML binding attributes for source fields (registry, virtual, git). |
| src/Microsoft.ComponentDetection.Detectors/uv/UvPackage.cs | Converts package model to DTO-friendly shape and adds normalization/filtering plus metadata modeling. |
| src/Microsoft.ComponentDetection.Detectors/uv/UvMetadata.cs | New DTO to represent and normalize [package.metadata] (requires-dist, requires-dev). |
| src/Microsoft.ComponentDetection.Detectors/uv/UvLock.cs | Simplifies parsing to typed Tomlyn model binding and normalizes parsed packages. |
| src/Microsoft.ComponentDetection.Detectors/uv/UvDependency.cs | Adds TOML binding attributes and DTO-friendly property setters/defaults. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Low
|
|
||
| // Metadata dependencies (requires-dist) | ||
| public List<UvDependency> MetadataRequiresDist { get; set; } = []; | ||
| [DataMember(Name = "metadata")] |
There was a problem hiding this comment.
Why DataMember? I have seen some "JsonPropertyName" used in the repository, is there any particular reason we still had to use it in this package? Probably nothing will break but for my own understanding of the current state of things with our serialization libraries since I know some has migrated but not all of it yet.
There was a problem hiding this comment.
I was following the pattern used by other TOML detectors.
Although interestingly Tomlyn has explicit support for JsonPropertyName and I can't find any evidence in their repo that they pay attention to [DataMember] so you're likely right, or more generally the default name transformations are sufficient without explicit serialization attributes needed.
|
👋 Hi! It looks like you modified some files in the
If none of the above scenarios apply, feel free to ignore this comment 🙂 |
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
src/Microsoft.ComponentDetection.Detectors/uv/UvPackage.cs:30
- If switching TOML DTOs away from System.Text.Json attributes, the computed properties should use [IgnoreDataMember] instead of [JsonIgnore] to keep serialization/binding attributes consistent.
[JsonIgnore]
public List<UvDependency> MetadataRequiresDist => this.Metadata?.RequiresDist ?? [];
[JsonIgnore]
public List<UvDependency> MetadataRequiresDev => this.Metadata?.RequiresDev?.Values
- Files reviewed: 6/6 changed files
- Comments generated: 5
- Review effort level: Low
This simplifies the parsing of uv.lock files by just using the built-in model parser. It comes at the expense of some resiliency to malformed files, but given that these are generated files and the reliability of data from a manually edited file is questionable that seems either preferable or like an acceptable tradeoff to me.