3580: AVRO-4214: [C#] Ensure nuget packages expose SPDX license expression

[martin-augment]

3580: To review by AI

[coderabbitai]

Walkthrough

This pull request modifies C# project packaging configuration and adds comprehensive NuGet package validation tests. The packaging metadata in common.props is updated to use PackageLicenseExpression instead of PackageLicenseFile. A new test suite is introduced in NuGetPackageTests.cs containing NUnit tests that validate package metadata for Apache Avro packages, specifically verifying SPDX license expressions, LICENSE file presence, and license file content. Helper methods extract package information from .nupkg archives.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pr-3580-2025-11-27-10-06-01

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[augmentcode]

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

[claude]

Code Review Summary

This PR implements AVRO-4214 to expose SPDX license expressions in NuGet packages. The changes are well-structured and include comprehensive test coverage.

Positive Aspects

1. Proper License Metadata: The change from PackageLicenseFile to PackageLicenseExpression aligns with modern NuGet packaging best practices and provides better machine-readable license information.

2. Comprehensive Test Coverage: The test file includes three test methods covering SPDX license expression verification, LICENSE file presence validation, and LICENSE file content verification. All six Apache.Avro packages are tested via TestCaseSource.

3. Consistent License Header: The new test file properly includes the Apache License header (lang/csharp/src/apache/test/NuGetPackageTests.cs:1-17).

4. Smart Package Discovery: The FindPackageInBuildOutput method intelligently searches for Release builds and orders by modification time (lang/csharp/src/apache/test/NuGetPackageTests.cs:120-133).

5. Graceful Handling: Uses Assert.Inconclusive when packages are not found, providing clear guidance to run the build first.

Potential Issues and Suggestions

1. License File Still Included (By Design)
The common.props file still includes the LICENSE file in packages (line 59). This appears intentional - having both the SPDX expression AND the license file provides maximum compatibility. This is good practice.

2. Test Discovery Path Complexity
The path navigation in FindPackageInBuildOutput uses multiple parent directory traversals. This is brittle if the test directory structure changes. However, given the stable structure of the Avro project, this is acceptable. The comment explaining the logic helps maintainability.

3. Minor: Test Organization
The test class is in the Avro.Test.Utils namespace but tests NuGet package metadata. Consider if Avro.Test.Packaging or Avro.Test.NuGet would be more semantically appropriate. Current naming is acceptable.

Security Considerations

• No security concerns identified: The code only reads and validates package contents, no external inputs or dangerous operations.

Performance Considerations

• File system operations are appropriate. The tests search for packages once per test case, which is reasonable.
• ZIP file operations are efficient using ZipFile.OpenRead with using statements ensuring proper resource disposal.
• Minor optimization opportunity: The FindPackageInBuildOutput method is called three times per package. Consider caching package paths at the test fixture level with OneTimeSetUp if performance becomes a concern.

Test Coverage

Excellent test coverage with three complementary test methods:

1. Metadata validation - Ensures SPDX expression is correct
2. File presence - Verifies LICENSE file exists
3. Content validation - Confirms LICENSE contains Apache License text

All six packages are tested, providing comprehensive validation.

Recommendations

1. Approve and merge - The code quality is high and meets best practices
2. Consider adding a comment in common.props line 59 explaining why LICENSE file is still included despite having SPDX expression
3. Optional: Add OneTimeSetUp fixture method to cache package discovery if test execution time becomes a concern

Overall Assessment

LGTM - This is a well-implemented change with proper test coverage. The code follows C# and NuGet best practices, includes proper error handling, and thoroughly validates the license metadata changes.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

lang/csharp/src/apache/test/NuGetPackageTests.cs (1)

121-135: Path discovery works, but comment/name could be clearer

FindPackageInBuildOutput’s strategy of walking up from TestContext.CurrentContext.TestDirectory and scanning SearchOption.AllDirectories for Release .nupkg files is pragmatic and should work across TFMs. However, the comment says “Find the lang/csharp root” while the .. chain actually lands at the src level, and the variable name csharpRoot doesn’t reflect that precisely. Consider tightening the comment (and optionally the variable name) to match the actual anchor directory.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 158cbc5 and cf6f2a1.

📒 Files selected for processing (2)

• lang/csharp/common.props (1 hunks)
• lang/csharp/src/apache/test/NuGetPackageTests.cs (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Cursor Bugbot
• GitHub Check: claude-review

🔇 Additional comments (6)

lang/csharp/common.props (1)

47-56: SPDX license expression configuration looks correct

Using <PackageLicenseExpression>Apache-2.0</PackageLicenseExpression> alongside the packed LICENSE file aligns with NuGet’s SPDX guidance and matches what the new tests expect. No issues from my side here.

lang/csharp/src/apache/test/NuGetPackageTests.cs (5)

31-39: Good coverage over all relevant package IDs

The static PackageIds array cleanly enumerates all Avro-related packages you care about, and drives the tests via TestCaseSource, which keeps the suite easy to extend.

---

41-67: SPDX license expression assertion is solid and namespace‑safe

The nuspec parsing correctly handles XML namespaces and asserts both type="expression" and value "Apache-2.0", with a clear inconclusive path when the package is missing. This gives strong validation of the new packaging metadata without being brittle.

---

69-90: LICENSE presence and non‑empty content checks are appropriate

Opening the .nupkg via ZipFile.OpenRead and asserting there is a non‑empty root entry named LICENSE matches how the packaging is configured and should catch accidental removal or renaming of the license file.

---

92-119: Text checks for Apache 2.0 are pragmatic and low‑maintenance

Reading the LICENSE file and asserting that it contains both "Apache License" and "Version 2.0" is a simple, robust way to guard against incorrect or truncated license content without overfitting to exact wording.

---

137-154: Nuspec extraction helper is clean and correctly scoped

ExtractNuspecFromPackage cleanly encapsulates the zip handling, guards on missing .nuspec with Assert.Fail, and disposes streams correctly via using. This keeps the test methods focused on assertions rather than I/O details.