update nuget packages#539
Merged
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughProject-wide dependency bumps, C# primary-constructor refactors across many types, SPDX/license helper adjustments and minor null/version-safety checks, plus test wiring and pragma/string-literal cleanups. ChangesDependency Version Updates
Primary-constructor / refactor and behavior changes
Tests and small cleanups
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NuGetLicense/NuGetLicense.csproj (1)
23-26:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate NuGet.ProjectModel and NuGet.Frameworks to a released version.
NuGet 7.6.0 is currently in preview and has not been officially released. Using unreleased versions in production dependencies poses significant risks including potential instability, lack of support, and incomplete API documentation. Update these packages to the latest released version (currently 7.5.x) or document justification for using preview software.
Note: The comment references
src/NuGetLicense/NuGetLicense.csprojbut the packages in question are insrc/NuGetUtility/NuGetUtility.csprojat lines 23-24.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NuGetLicense/NuGetLicense.csproj` around lines 23 - 26, The csproj is referencing preview NuGet packages; update the PackageReference entries for NuGet.ProjectModel and NuGet.Frameworks in the NuGetUtility project (look for the PackageReference symbols NuGet.ProjectModel and NuGet.Frameworks in src/NuGetUtility/NuGetUtility.csproj) to a released version (e.g., change the Version attribute to the latest 7.5.x stable release without any preview suffix), run dotnet restore/build to validate, and if the preview was intentional add a short justification comment in the csproj and raise it in the PR instead of keeping an unreleased dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/NuGetUtility/NuGetUtility.csproj`:
- Around line 23-26: The project references `NuGet.ProjectModel` and
`NuGet.Frameworks` at version 7.6.0 (preview); change these PackageReference
entries to a stable 7.5.x release (e.g., set Version="7.5.0" or the latest
7.5.*) in the NuGetUtility.csproj to avoid using unreleased preview packages, or
if you must keep 7.6.0, add a clear justification comment and a short note in
project documentation explaining why a preview is required and the associated
risk acceptance; update the `PackageReference` versions for `NuGet.ProjectModel`
and `NuGet.Frameworks` (or add the justification) accordingly.
---
Outside diff comments:
In `@src/NuGetLicense/NuGetLicense.csproj`:
- Around line 23-26: The csproj is referencing preview NuGet packages; update
the PackageReference entries for NuGet.ProjectModel and NuGet.Frameworks in the
NuGetUtility project (look for the PackageReference symbols NuGet.ProjectModel
and NuGet.Frameworks in src/NuGetUtility/NuGetUtility.csproj) to a released
version (e.g., change the Version attribute to the latest 7.5.x stable release
without any preview suffix), run dotnet restore/build to validate, and if the
preview was intentional add a short justification comment in the csproj and
raise it in the PR instead of keeping an unreleased dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38d73e87-da08-4c4a-a9f2-215bd8c91689
📒 Files selected for processing (9)
src/NuGetLicense/NuGetLicense.csprojsrc/NuGetUtility/NuGetUtility.csprojtests/FileLicenseMatcher.Test/FileLicenseMatcher.Test.csprojtests/NuGetLicense.Test/NuGetLicense.Test.csprojtests/NuGetUtility.Test/NuGetUtility.Test.csprojtests/NuGetUtility.UrlToLicenseMapping.Test/NuGetUtility.UrlToLicenseMapping.Test.csprojtests/targets/MultiTargetProjectWithDifferentDependencies/MultiTargetProjectWithDifferentDependencies.csprojtests/targets/PackageReferenceProject/PackageReferenceProject.csprojtests/targets/VersionRangesProject/VersionRangesProject.csproj
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/FileLicenseMatcher/Compare/LicenseMatcher.cs`:
- Around line 11-12: Add XML documentation to the public primary-constructor
declaration for the LicenseMatcher class: add a <summary> describing the purpose
of the LicenseMatcher type and its behavior, and add <param> tags for the
constructor parameters named fileSystem (IFileSystem) and fileLicenseMap
(IDictionary<string,string>) describing each parameter's role; ensure the XML
appears immediately above the primary-constructor declaration "public class
LicenseMatcher(IFileSystem fileSystem, IDictionary<string, string>
fileLicenseMap) : IFileLicenseMatcher" and follows the project's XML doc style
conventions.
In `@src/NuGetLicense/Output/Csv/CsvOutputFormatter.cs`:
- Line 9: Add XML documentation to the public primary-constructor record/class
CsvOutputFormatter describing its purpose and include <summary> plus <param>
tags for the constructor parameters printErrorsOnly and skipIgnoredPackages;
place the /// <summary> above the CsvOutputFormatter declaration and add ///
<param name="printErrorsOnly"> and /// <param name="skipIgnoredPackages">
entries explaining what each flag controls so the public API is properly
documented.
In `@src/NuGetLicense/Output/Json/JsonOutputFormatter.cs`:
- Around line 11-12: Add XML documentation to the public primary-constructor
class JsonOutputFormatter: add a <summary> describing the formatter's purpose
and behavior, and add <param> tags for the constructor parameters prettyPrint,
printErrorsOnly, and skipIgnoredPackages explaining what each flag controls;
ensure the XML comment is placed directly above the class declaration "public
class JsonOutputFormatter(bool prettyPrint, bool printErrorsOnly, bool
skipIgnoredPackages) : IOutputFormatter".
In `@src/NuGetUtility/ReferencedPackagesReader/ProjectsCollector.cs`:
- Around line 13-15: The check using
fileSystem.Path.GetExtension(inputPath).StartsWith(".sln") should be changed to
an exact, case-insensitive equality check to avoid matching extensions like
".slnf" or different case; replace the StartsWith call with a comparison using
Equals(".sln", StringComparison.OrdinalIgnoreCase) (or string.Equals with
StringComparison.OrdinalIgnoreCase) so the branch that calls
solutionPersistence.GetProjectsFromSolutionAsync and the fallback that returns
fileSystem.Path.GetFullPath(inputPath) behave correctly.
In `@src/NuGetUtility/Wrapper/HttpClientWrapper/FileDownloader.cs`:
- Around line 52-55: In TryDownload (FileDownloader.cs) you’re creating an
HttpRequestMessage and HttpResponseMessage that aren’t disposed and can leak
connections during the retry loop; wrap the HttpRequestMessage and the
HttpResponseMessage returned by client.SendAsync in using blocks (or explicit
try/finally Dispose calls) so both are always disposed after use, making sure to
read or copy the response content you need before disposing the response (e.g.,
use response.Content.ReadAsStreamAsync()/CopyToAsync into a MemoryStream if you
need the body after disposing). Replace the existing direct creations of request
and response around client.SendAsync with using (var request = new
HttpRequestMessage(...)) { var response = await client.SendAsync(...); using
(response) { ... } } (or equivalent try/finally) to ensure no HttpRequestMessage
or HttpResponseMessage instances are leaked across retries.
In
`@src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/GlobalPackagesFolderUtility.cs`:
- Around line 34-36: The guard in GlobalPackagesFolderUtility.cs incorrectly
returns null when the manifest.Metadata.Version equals the package identity,
rejecting valid packages; in the method containing the check on
manifest.Metadata.Version, change the condition so it returns null only when
metadataVersion is missing or DOES NOT equal identity.Version (i.e., invert the
equality check using !new
WrappedNuGetVersion(metadataVersion).Equals(identity.Version) or compare
versions for inequality) so valid cached packages are accepted (refer to
manifest.Metadata.Version, metadataVersion, WrappedNuGetVersion, and
identity.Version).
In `@src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/WrappedPackageMetadata.cs`:
- Around line 15-16: The ArgumentNullException calls in the PackageIdentity
initializer use incorrect parameter names ("Id" and "Version"); update the two
throws so they pass the actual parameter name nameof(metadata) instead of
nameof(metadata.Id) and nameof(metadata.Version) in the Identity property
initializer (the PackageIdentity Identity = new(...) expression) so the
exception reports the correct parameter and removes analyzer warnings.
In
`@src/NuGetUtility/Wrapper/SolutionPersistenceWrapper/SolutionPersistenceWrapper.cs`:
- Line 10: The public class SolutionPersistenceWrapper (implementing
ISolutionPersistenceWrapper) lacks XML documentation; add an XML doc comment on
the public primary-constructor type declaration that includes a <summary>
describing the class responsibility and a <param> tag for each constructor
parameter (use the exact parameter names from the SolutionPersistenceWrapper
constructor), and include any relevant <returns> or <exception> tags if the
class exposes methods that require them; place the comment immediately above the
"public class SolutionPersistenceWrapper : ISolutionPersistenceWrapper"
declaration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76277ff7-b516-41dd-b962-fa205e097c40
📒 Files selected for processing (67)
src/FileLicenseMatcher/Combine/LicenseMatcher.cssrc/FileLicenseMatcher/Compare/LicenseMatcher.cssrc/FileLicenseMatcher/SPDX/FastLicenseMatcher.cssrc/FileLicenseMatcher/SPDX/JavaCore/LicenseTemplateRule.cssrc/FileLicenseMatcher/SPDX/JavaCore/LicenseTextHelper.cssrc/FileLicenseMatcher/SPDX/JavaCore/SpdxLicenseTemplateHelper.cssrc/FileLicenseMatcher/SPDX/JavaLibrary/CompareTemplateOutputHandler.cssrc/FileLicenseMatcher/SPDX/JavaLibrary/LicenseCompareHelper.cssrc/FileLicenseMatcher/SPDX/JavaLibrary/ParseInstruction.cssrc/NuGetLicense/CommandLineOptionsParser.cssrc/NuGetLicense/LicenseValidationOrchestrator.cssrc/NuGetLicense/LicenseValidator/LicenseDownloadException.cssrc/NuGetLicense/LicenseValidator/LicenseValidationResult.cssrc/NuGetLicense/LicenseValidator/LicenseValidator.cssrc/NuGetLicense/Output/Csv/CsvOutputFormatter.cssrc/NuGetLicense/Output/Json/JsonOutputFormatter.cssrc/NuGetLicense/Output/Table/TableOutputFormatter.cssrc/NuGetLicense/Program.cssrc/NuGetLicense/Serialization/ValidatedLicenseJsonConverterWithOmittingEmptyErrorList.cssrc/NuGetUtility/Output/Table/TablePrinter.cssrc/NuGetUtility/PackageInformationReader/PackageInformationReader.cssrc/NuGetUtility/PackageInformationReader/PackageMetadata.cssrc/NuGetUtility/ReferencedPackagesReader/ProjectsCollector.cssrc/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cssrc/NuGetUtility/Wrapper/HttpClientWrapper/DownloadFailedException.cssrc/NuGetUtility/Wrapper/HttpClientWrapper/FileDownloader.cssrc/NuGetUtility/Wrapper/MsBuildWrapper/ProjectWrapper.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Frameworks/WrappedNuGetFramework.cssrc/NuGetUtility/Wrapper/NuGetWrapper/NugetWrapperException.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/PackagesConfigReaderException.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/WrappedPackageDependency.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/AssetsPackageDependencyReader.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLibraryDependency.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFile.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFileLibrary.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFileTarget.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedPackageSpec.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedTargetFrameworkInformation.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Protocol/Core/Types/CachingDisposableSourceRepository.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Protocol/Core/Types/CachingPackageMetadataResource.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Protocol/GlobalPackagesFolderUtility.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Protocol/WrappedPackageMetadata.cssrc/NuGetUtility/Wrapper/SolutionPersistenceWrapper/ISolutionPersistenceWrapper.cssrc/NuGetUtility/Wrapper/SolutionPersistenceWrapper/SolutionPersistenceWrapper.cstests/FileLicenseMatcher.Test/Combine/LicenseMatcherTest.cstests/FileLicenseMatcher.Test/Compare/LicenseMatcherTest.cstests/FileLicenseMatcher.Test/SPDX/LicenseMatcherTest.cstests/NuGetLicense.Test/CommandLineOptionsParserTest.cstests/NuGetLicense.Test/LicenseValidationOrchestratorTest.cstests/NuGetLicense.Test/Output/Csv/CsvOutputFormatterTests.cstests/NuGetLicense.Test/Output/Helper/NuGetVersion.cstests/NuGetLicense.Test/Output/Json/JsonOutputFormatterTest.cstests/NuGetLicense.Test/Output/Table/MarkdownTableOutputFormatterTest.cstests/NuGetLicense.Test/Output/Table/TableOutputFormatterTest.cstests/NuGetUtility.Test.Extensions/Helper/AsyncEnumerableExtension/AsyncEnumerable.cstests/NuGetUtility.Test.Extensions/Helper/AsyncEnumerableExtension/AsyncEnumerator.cstests/NuGetUtility.Test.Extensions/Helper/AutoFixture/NuGet/Versioning/NuGetVersionBuilder.cstests/NuGetUtility.Test/Architecture/ArchitectureTest.cstests/NuGetUtility.Test/Architecture/ConditionsExtensions.cstests/NuGetUtility.Test/Architecture/Rules/NugetAbstractionTest.cstests/NuGetUtility.Test/Extensions/HashSetExtensionsTest.cstests/NuGetUtility.Test/Extensions/ProjectExtensionsTest.cstests/NuGetUtility.Test/PackageInformationReader/PackageInformationReaderTest.cstests/NuGetUtility.Test/ProjectFiltering/ProjectFiltererTest.cstests/NuGetUtility.Test/ReferencedPackagesReader/ProjectsCollectorTest.cstests/NuGetUtility.Test/ReferencedPackagesReader/ReferencedPackageReaderTest.cstests/NuGetUtility.UrlToLicenseMapping.Test/UrlToLicenseMappingTest.cs
💤 Files with no reviewable changes (1)
- tests/NuGetUtility.Test/Architecture/Rules/NugetAbstractionTest.cs
✅ Files skipped from review due to trivial changes (17)
- src/NuGetUtility/Wrapper/SolutionPersistenceWrapper/ISolutionPersistenceWrapper.cs
- src/NuGetUtility/Wrapper/HttpClientWrapper/DownloadFailedException.cs
- src/NuGetUtility/Wrapper/NuGetWrapper/NugetWrapperException.cs
- src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedTargetFrameworkInformation.cs
- tests/NuGetUtility.Test/PackageInformationReader/PackageInformationReaderTest.cs
- src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/AssetsPackageDependencyReader.cs
- src/NuGetLicense/Serialization/ValidatedLicenseJsonConverterWithOmittingEmptyErrorList.cs
- src/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/PackagesConfigReaderException.cs
- tests/NuGetUtility.UrlToLicenseMapping.Test/UrlToLicenseMappingTest.cs
- tests/NuGetLicense.Test/CommandLineOptionsParserTest.cs
- src/NuGetLicense/LicenseValidator/LicenseValidationResult.cs
- src/NuGetUtility/PackageInformationReader/PackageMetadata.cs
- src/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/WrappedPackageDependency.cs
- src/NuGetLicense/LicenseValidator/LicenseDownloadException.cs
- src/FileLicenseMatcher/SPDX/FastLicenseMatcher.cs
- src/FileLicenseMatcher/SPDX/JavaCore/LicenseTemplateRule.cs
- tests/NuGetUtility.Test/ReferencedPackagesReader/ReferencedPackageReaderTest.cs
sensslen
force-pushed
the
simon/update-nuget-packages
branch
2 times, most recently
from
352916f to
7dd6316
Compare
sensslen
force-pushed
the
simon/update-nuget-packages
branch
from
7dd6316 to
a5a0fd9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/WrappedPackageMetadata.cs (1)
17-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the actual parameter name in
ArgumentNullExceptionconstructors.
nameof(metadata.Id)evaluates to"Id"andnameof(metadata.Version)evaluates to"Version", but the constructor parameter ismetadata. This produces analyzer warnings (CA1507) and misleading exception messages when thrown. Usenameof(metadata)for both exceptions to match the actual parameter name.🔧 Proposed fix
- public WrappedPackageMetadata(ManifestMetadata metadata) - { - Identity = new PackageIdentity(metadata.Id ?? throw new ArgumentNullException(nameof(metadata.Id)), - new WrappedNuGetVersion(metadata.Version ?? throw new ArgumentNullException(nameof(metadata.Version)))); + public WrappedPackageMetadata(ManifestMetadata metadata) + { + Identity = new PackageIdentity(metadata.Id ?? throw new ArgumentNullException(nameof(metadata)), + new WrappedNuGetVersion(metadata.Version ?? throw new ArgumentNullException(nameof(metadata))));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/WrappedPackageMetadata.cs` around lines 17 - 18, The ArgumentNullException usage in WrappedPackageMetadata's constructor uses nameof(metadata.Id) and nameof(metadata.Version) which produces wrong parameter names; update the two throws so they pass nameof(metadata) instead (the null-checked parameter) when constructing Identity (PackageIdentity and WrappedNuGetVersion) to eliminate CA1507 warnings and produce correct exception messages.src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/GlobalPackagesFolderUtility.cs (1)
37-37:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInvert the version comparison to accept matching packages.
Line 37 returns
nullwhen the manifest version equalsidentity.Version, which rejects valid cached packages. The condition should returnnullonly when the versions do not match. Add a negation operator before theEqualscall.🐛 Proposed fix
if (manifest.Metadata.Version is not { } manifestVersion || - new WrappedNuGetVersion(manifestVersion).Equals(identity.Version) || + !new WrappedNuGetVersion(manifestVersion).Equals(identity.Version) || manifest.Metadata.LicenseMetadata is not { } manifestLicenseMetadata) { return null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/GlobalPackagesFolderUtility.cs` at line 37, The code in GlobalPackagesFolderUtility is currently returning null when WrappedNuGetVersion(manifestVersion).Equals(identity.Version) is true, which incorrectly rejects valid cached packages; change the conditional to return null only when the versions do not match by negating the comparison (i.e., use !WrappedNuGetVersion(manifestVersion).Equals(identity.Version)) so the method (in GlobalPackagesFolderUtility) accepts packages when the manifest version equals identity.Version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/GlobalPackagesFolderUtility.cs`:
- Line 37: The code in GlobalPackagesFolderUtility is currently returning null
when WrappedNuGetVersion(manifestVersion).Equals(identity.Version) is true,
which incorrectly rejects valid cached packages; change the conditional to
return null only when the versions do not match by negating the comparison
(i.e., use !WrappedNuGetVersion(manifestVersion).Equals(identity.Version)) so
the method (in GlobalPackagesFolderUtility) accepts packages when the manifest
version equals identity.Version.
In `@src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/WrappedPackageMetadata.cs`:
- Around line 17-18: The ArgumentNullException usage in WrappedPackageMetadata's
constructor uses nameof(metadata.Id) and nameof(metadata.Version) which produces
wrong parameter names; update the two throws so they pass nameof(metadata)
instead (the null-checked parameter) when constructing Identity (PackageIdentity
and WrappedNuGetVersion) to eliminate CA1507 warnings and produce correct
exception messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c2074ed-1e3c-46ff-9196-071a8a29db67
📒 Files selected for processing (5)
src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/GlobalPackagesFolderUtility.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Protocol/WrappedPackageMetadata.cstests/FileLicenseMatcher.Test/Compare/LicenseMatcherTest.cstests/NuGetLicense.Test/CommandLineOptionsParserTest.cstests/NuGetUtility.UrlToLicenseMapping.Test/UrlToLicenseMappingTest.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/NuGetLicense.Test/CommandLineOptionsParserTest.cs
- tests/FileLicenseMatcher.Test/Compare/LicenseMatcherTest.cs
sensslen
force-pushed
the
simon/update-nuget-packages
branch
from
a5a0fd9 to
ef6d6f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/NuGetUtility/Wrapper/HttpClientWrapper/FileDownloader.cs (1)
53-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDispose request and response objects in
TryDownload.Both objects are created per retry attempt and currently not disposed, which can exhaust HTTP resources.
💡 Proposed fix
- var request = new HttpRequestMessage(HttpMethod.Get, url); - - HttpResponseMessage response = await client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, token); + using HttpRequestMessage request = new(HttpMethod.Get, url); + using HttpResponseMessage response = await client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, token);Do HttpRequestMessage and HttpResponseMessage implement IDisposable, and is explicit disposal recommended after SendAsync in retry loops?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NuGetUtility/Wrapper/HttpClientWrapper/FileDownloader.cs` around lines 53 - 56, In TryDownload, each retry creates an HttpRequestMessage and an HttpResponseMessage but never disposes them; wrap the per-attempt HttpRequestMessage and the HttpResponseMessage returned by client.SendAsync in proper disposal (e.g., using or try/finally) so both are disposed after the attempt finishes or before a retry; ensure you do not dispose the response until any needed response.Content/stream is consumed or handed off, and dispose them on all code paths (success, error, and cancellation) to avoid exhausting HTTP resources.src/NuGetUtility/ReferencedPackagesReader/ProjectsCollector.cs (1)
13-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse case-insensitive extension matching for solution detection.
Contains(extension)is case-sensitive here, so uppercase solution extensions won’t be recognized and won’t go through solution expansion.💡 Proposed fix
- return new[] { ".sln", ".slnx" }.Contains(extension) + return new[] { ".sln", ".slnx" }.Contains(extension, StringComparer.OrdinalIgnoreCase) ? (await solutionPersistence.GetProjectsFromSolutionAsync(fileSystem.Path.GetFullPath(inputPath))).Where(fileSystem.File.Exists).Select(fileSystem.Path.GetFullPath) : [fileSystem.Path.GetFullPath(inputPath)];In .NET, does Enumerable.Contains(string) without an IEqualityComparer perform case-sensitive comparison?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NuGetUtility/ReferencedPackagesReader/ProjectsCollector.cs` around lines 13 - 15, The extension comparison is case-sensitive so uppercase extensions like ".SLN" are missed; update the conditional that checks extension (the extension variable used in ProjectsCollector.cs before calling solutionPersistence.GetProjectsFromSolutionAsync) to perform a case-insensitive comparison (e.g., use Enumerable.Contains with StringComparer.OrdinalIgnoreCase or otherwise compare with OrdinalIgnoreCase) so solution files are recognized regardless of casing before invoking GetProjectsFromSolutionAsync and filtering with fileSystem.File.Exists.
🧹 Nitpick comments (2)
src/NuGetLicense/Output/Table/TableOutputFormatter.cs (1)
9-10: ⚡ Quick winAdd XML docs for the public primary constructor.
Please add
<summary>and<param>documentation forprintErrorsOnly,skipIgnoredPackages, andprintMarkdown.As per coding guidelines, "Public APIs in C# should have XML documentation comments using
<summary>,<param>,<returns>,<exception>tags as appropriate".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NuGetLicense/Output/Table/TableOutputFormatter.cs` around lines 9 - 10, Add XML documentation comments for the public primary constructor of TableOutputFormatter: add a <summary> describing the constructor's purpose and three <param> tags documenting printErrorsOnly (whether to only print error entries), skipIgnoredPackages (whether to omit ignored packages from output), and printMarkdown (whether to render output in Markdown format). Place the XML docs immediately above the primary constructor declaration for TableOutputFormatter(bool printErrorsOnly, bool skipIgnoredPackages, bool printMarkdown = false) and use concise, clear descriptions following the project's documentation style.src/NuGetUtility/Wrapper/SolutionPersistenceWrapper/ISolutionPersistenceWrapper.cs (1)
6-8: ⚡ Quick winAdd XML documentation to this public interface and method.
This public API is missing XML docs (
<summary>,<param>,<returns>), which makes the contract less discoverable.Proposed patch
+ /// <summary> + /// Provides access to project paths contained in a solution file. + /// </summary> public interface ISolutionPersistenceWrapper { + /// <summary> + /// Gets project file paths from the specified solution. + /// </summary> + /// <param name="inputPath">Path to the solution file.</param> + /// <returns>A collection of project file paths.</returns> Task<IEnumerable<string>> GetProjectsFromSolutionAsync(string inputPath); }As per coding guidelines "Public APIs in C# should have XML documentation comments using
<summary>,<param>,<returns>,<exception>tags as appropriate".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NuGetUtility/Wrapper/SolutionPersistenceWrapper/ISolutionPersistenceWrapper.cs` around lines 6 - 8, Add XML documentation to the public interface ISolutionPersistenceWrapper and its method GetProjectsFromSolutionAsync: add a <summary> describing the purpose of the interface and the method, a <param name="inputPath"> explaining the expected solution path, and a <returns> describing the returned Task<IEnumerable<string>> (what the strings represent). If the method can throw specific exceptions, include <exception> tags naming them. Ensure the doc comments are placed immediately above the interface declaration and the GetProjectsFromSolutionAsync method signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/NuGetLicense/LicenseValidationOrchestrator.cs`:
- Around line 90-93: The net472 build fails because
LicenseValidationOrchestrator.cs calls await os.DisposeAsync() when
shouldDisposeStream is true; change this to synchronous disposal compatible with
.NET Framework by replacing await os.DisposeAsync() with os.Dispose() (remove
the await), keeping the surrounding conditional and variable names
(shouldDisposeStream and os) intact so the stream is disposed synchronously.
In `@src/NuGetLicense/LicenseValidator/LicenseValidator.cs`:
- Around line 168-173: The AddOrUpdateLicense call in the "not allowed"
file-license path is recording info.LicenseMetadata.License even though
validation checked matchedLicense; change the argument passed to
AddOrUpdateLicense so it uses matchedLicense (the actual resolved/checked
license ID) instead of info.LicenseMetadata.License, keep the other parameters
(result, info, LicenseInformationOrigin.File, new
ValidationError(GetLicenseNotAllowedMessage(matchedLicense), context)) unchanged
so the stored license matches the message produced by
GetLicenseNotAllowedMessage.
In `@src/NuGetUtility/Wrapper/HttpClientWrapper/DownloadFailedException.cs`:
- Line 6: The declaration "public class DownloadFailedException(Uri url) :
Exception($"Download failed for URL: {url");" is invalid C# syntax; change the
declaration of DownloadFailedException into a normal class with a body and a
constructor: declare "public class DownloadFailedException : Exception" with an
explicit public constructor "public DownloadFailedException(Uri url) :
base($\"Download failed for URL: {url}\") { }" (optionally add serialization
ctor if needed) so the class has braces and a proper base Exception call.
In
`@src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/GlobalPackagesFolderUtility.cs`:
- Around line 38-41: Guard against a null or empty License string before calling
PathUtility.GetPathWithDirectorySeparator: inside the block that checks
result.LicenseMetadata?.Type == Packaging.LicenseType.File, first verify
result.LicenseMetadata?.License is not null or whitespace (e.g., if
(string.IsNullOrWhiteSpace(result.LicenseMetadata.License)) { /* skip/log and
continue */ }), then compute normalizedPath via
NuGet.Common.PathUtility.GetPathWithDirectorySeparator(result.LicenseMetadata.License)
and call pkgStream.GetStream(normalizedPath); this prevents
GetPathWithDirectorySeparator and pkgStream.GetStream from throwing on malformed
nuspec metadata.
In `@tests/NuGetUtility.Test/ProjectFiltering/ProjectFiltererTest.cs`:
- Around line 13-21: The test expectations for includeSharedProjects=true are
wrong: update the Arguments entries in
ProjectFiltererTest.FilterProjects_ExcludesSharedProjects_WhenIncludeSharedProjectsIsFalse
so that for the four cases where includeSharedProjects is true (the last four
[Arguments] lines) the isFiltered boolean is false; this aligns the test with
ProjectFilter.FilterProjects (which returns all projects when
includeSharedProjects is true) and ensures the test data matches the behavior.
---
Duplicate comments:
In `@src/NuGetUtility/ReferencedPackagesReader/ProjectsCollector.cs`:
- Around line 13-15: The extension comparison is case-sensitive so uppercase
extensions like ".SLN" are missed; update the conditional that checks extension
(the extension variable used in ProjectsCollector.cs before calling
solutionPersistence.GetProjectsFromSolutionAsync) to perform a case-insensitive
comparison (e.g., use Enumerable.Contains with StringComparer.OrdinalIgnoreCase
or otherwise compare with OrdinalIgnoreCase) so solution files are recognized
regardless of casing before invoking GetProjectsFromSolutionAsync and filtering
with fileSystem.File.Exists.
In `@src/NuGetUtility/Wrapper/HttpClientWrapper/FileDownloader.cs`:
- Around line 53-56: In TryDownload, each retry creates an HttpRequestMessage
and an HttpResponseMessage but never disposes them; wrap the per-attempt
HttpRequestMessage and the HttpResponseMessage returned by client.SendAsync in
proper disposal (e.g., using or try/finally) so both are disposed after the
attempt finishes or before a retry; ensure you do not dispose the response until
any needed response.Content/stream is consumed or handed off, and dispose them
on all code paths (success, error, and cancellation) to avoid exhausting HTTP
resources.
---
Nitpick comments:
In `@src/NuGetLicense/Output/Table/TableOutputFormatter.cs`:
- Around line 9-10: Add XML documentation comments for the public primary
constructor of TableOutputFormatter: add a <summary> describing the
constructor's purpose and three <param> tags documenting printErrorsOnly
(whether to only print error entries), skipIgnoredPackages (whether to omit
ignored packages from output), and printMarkdown (whether to render output in
Markdown format). Place the XML docs immediately above the primary constructor
declaration for TableOutputFormatter(bool printErrorsOnly, bool
skipIgnoredPackages, bool printMarkdown = false) and use concise, clear
descriptions following the project's documentation style.
In
`@src/NuGetUtility/Wrapper/SolutionPersistenceWrapper/ISolutionPersistenceWrapper.cs`:
- Around line 6-8: Add XML documentation to the public interface
ISolutionPersistenceWrapper and its method GetProjectsFromSolutionAsync: add a
<summary> describing the purpose of the interface and the method, a <param
name="inputPath"> explaining the expected solution path, and a <returns>
describing the returned Task<IEnumerable<string>> (what the strings represent).
If the method can throw specific exceptions, include <exception> tags naming
them. Ensure the doc comments are placed immediately above the interface
declaration and the GetProjectsFromSolutionAsync method signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3942ec4-2593-4d28-b66a-cc194db2a8ee
📒 Files selected for processing (63)
.coderabbit.yamlsrc/FileLicenseMatcher/Combine/LicenseMatcher.cssrc/FileLicenseMatcher/Compare/LicenseMatcher.cssrc/FileLicenseMatcher/SPDX/FastLicenseMatcher.cssrc/FileLicenseMatcher/SPDX/JavaCore/LicenseTemplateRule.cssrc/FileLicenseMatcher/SPDX/JavaCore/LicenseTextHelper.cssrc/FileLicenseMatcher/SPDX/JavaCore/SpdxLicenseTemplateHelper.cssrc/FileLicenseMatcher/SPDX/JavaLibrary/CompareTemplateOutputHandler.cssrc/FileLicenseMatcher/SPDX/JavaLibrary/LicenseCompareHelper.cssrc/FileLicenseMatcher/SPDX/JavaLibrary/ParseInstruction.cssrc/NuGetLicense/CommandLineOptionsParser.cssrc/NuGetLicense/LicenseValidationOrchestrator.cssrc/NuGetLicense/LicenseValidator/LicenseDownloadException.cssrc/NuGetLicense/LicenseValidator/LicenseValidationResult.cssrc/NuGetLicense/LicenseValidator/LicenseValidator.cssrc/NuGetLicense/Output/Csv/CsvOutputFormatter.cssrc/NuGetLicense/Output/Json/JsonOutputFormatter.cssrc/NuGetLicense/Output/Table/TableOutputFormatter.cssrc/NuGetLicense/Program.cssrc/NuGetUtility/Output/Table/TablePrinter.cssrc/NuGetUtility/PackageInformationReader/PackageInformationReader.cssrc/NuGetUtility/ReferencedPackagesReader/ProjectsCollector.cssrc/NuGetUtility/ReferencedPackagesReader/ReferencedPackageReader.cssrc/NuGetUtility/Wrapper/HttpClientWrapper/DownloadFailedException.cssrc/NuGetUtility/Wrapper/HttpClientWrapper/FileDownloader.cssrc/NuGetUtility/Wrapper/MsBuildWrapper/ProjectWrapper.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Frameworks/WrappedNuGetFramework.cssrc/NuGetUtility/Wrapper/NuGetWrapper/NugetWrapperException.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/PackagesConfigReaderException.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/WrappedPackageDependency.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/AssetsPackageDependencyReader.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLibraryDependency.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFile.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFileLibrary.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedLockFileTarget.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedPackageSpec.cssrc/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedTargetFrameworkInformation.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Protocol/Core/Types/CachingDisposableSourceRepository.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Protocol/Core/Types/CachingPackageMetadataResource.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Protocol/GlobalPackagesFolderUtility.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Protocol/LicenseAugmentedPackageMetadata.cssrc/NuGetUtility/Wrapper/NuGetWrapper/Protocol/WrappedPackageMetadata.cssrc/NuGetUtility/Wrapper/SolutionPersistenceWrapper/ISolutionPersistenceWrapper.cssrc/NuGetUtility/Wrapper/SolutionPersistenceWrapper/SolutionPersistenceWrapper.cstests/FileLicenseMatcher.Test/Combine/LicenseMatcherTest.cstests/FileLicenseMatcher.Test/SPDX/LicenseMatcherTest.cstests/NuGetLicense.Test/LicenseValidationOrchestratorTest.cstests/NuGetLicense.Test/Output/Csv/CsvOutputFormatterTests.cstests/NuGetLicense.Test/Output/Helper/NuGetVersion.cstests/NuGetLicense.Test/Output/Json/JsonOutputFormatterTest.cstests/NuGetLicense.Test/Output/Table/MarkdownTableOutputFormatterTest.cstests/NuGetLicense.Test/Output/Table/TableOutputFormatterTest.cstests/NuGetUtility.Test.Extensions/Helper/AsyncEnumerableExtension/AsyncEnumerable.cstests/NuGetUtility.Test.Extensions/Helper/AsyncEnumerableExtension/AsyncEnumerator.cstests/NuGetUtility.Test.Extensions/Helper/AutoFixture/NuGet/Versioning/NuGetVersionBuilder.cstests/NuGetUtility.Test/Architecture/ArchitectureTest.cstests/NuGetUtility.Test/Architecture/ConditionsExtensions.cstests/NuGetUtility.Test/Extensions/HashSetExtensionsTest.cstests/NuGetUtility.Test/Extensions/ProjectExtensionsTest.cstests/NuGetUtility.Test/PackageInformationReader/PackageInformationReaderTest.cstests/NuGetUtility.Test/ProjectFiltering/ProjectFiltererTest.cstests/NuGetUtility.Test/ReferencedPackagesReader/ProjectsCollectorTest.cstests/NuGetUtility.Test/ReferencedPackagesReader/ReferencedPackageReaderTest.cs
✅ Files skipped from review due to trivial changes (12)
- src/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/PackagesConfigReaderException.cs
- src/NuGetUtility/Wrapper/NuGetWrapper/NugetWrapperException.cs
- tests/NuGetUtility.Test/PackageInformationReader/PackageInformationReaderTest.cs
- src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/LicenseAugmentedPackageMetadata.cs
- src/FileLicenseMatcher/Combine/LicenseMatcher.cs
- .coderabbit.yaml
- src/NuGetUtility/Wrapper/NuGetWrapper/Packaging/Core/WrappedPackageDependency.cs
- src/NuGetUtility/Output/Table/TablePrinter.cs
- src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/WrappedTargetFrameworkInformation.cs
- src/NuGetUtility/Wrapper/NuGetWrapper/ProjectModel/AssetsPackageDependencyReader.cs
- src/NuGetLicense/Program.cs
- tests/NuGetUtility.Test/ReferencedPackagesReader/ReferencedPackageReaderTest.cs
Comment on lines
38
to
41
| if (result.LicenseMetadata?.Type == Packaging.LicenseType.File) | ||
| { | ||
| string normalizedPath = NuGet.Common.PathUtility.GetPathWithDirectorySeparator(manifest.Metadata.LicenseMetadata.License); | ||
| string normalizedPath = NuGet.Common.PathUtility.GetPathWithDirectorySeparator(result.LicenseMetadata.License); | ||
| using Stream licenseStream = pkgStream.GetStream(normalizedPath); |
There was a problem hiding this comment.
Guard LicenseMetadata.License before path normalization.
Type == File does not guarantee License is non-null/non-empty. Line 40 can throw on malformed nuspec metadata and fail package inspection.
Proposed fix
- if (result.LicenseMetadata?.Type == Packaging.LicenseType.File)
+ if (result.LicenseMetadata?.Type == Packaging.LicenseType.File &&
+ !string.IsNullOrWhiteSpace(result.LicenseMetadata.License))
{
string normalizedPath = NuGet.Common.PathUtility.GetPathWithDirectorySeparator(result.LicenseMetadata.License);
using Stream licenseStream = pkgStream.GetStream(normalizedPath);
using var reader = new StreamReader(licenseStream);
return new LicenseAugmentedPackageMetadata(result, reader.ReadToEnd());
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/NuGetUtility/Wrapper/NuGetWrapper/Protocol/GlobalPackagesFolderUtility.cs`
around lines 38 - 41, Guard against a null or empty License string before
calling PathUtility.GetPathWithDirectorySeparator: inside the block that checks
result.LicenseMetadata?.Type == Packaging.LicenseType.File, first verify
result.LicenseMetadata?.License is not null or whitespace (e.g., if
(string.IsNullOrWhiteSpace(result.LicenseMetadata.License)) { /* skip/log and
continue */ }), then compute normalizedPath via
NuGet.Common.PathUtility.GetPathWithDirectorySeparator(result.LicenseMetadata.License)
and call pkgStream.GetStream(normalizedPath); this prevents
GetPathWithDirectorySeparator and pkgStream.GetStream from throwing on malformed
nuspec metadata.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Chores