Adopt ANcpLua.NET.Sdk 3.4.27 across .NET projects and refactor JS SDK usage#17
Adopt ANcpLua.NET.Sdk 3.4.27 across .NET projects and refactor JS SDK usage#17ANcpLua wants to merge 2 commits into
Conversation
Wire each .NET project to the appropriate ANcpLua.NET.Sdk flavor (3.4.27) through global.json's msbuild-sdks block, mirroring qyl's setup: - Paperless.Contracts → ANcpLua.NET.Sdk (library) - PaperlessREST, PaperlessUI.Blazor → ANcpLua.NET.Sdk.Web - PaperlessServices → ANcpLua.NET.Sdk (worker; UseMicrosoftTestingPlatform=false, explicit OutputType=Exe + M.E.X usings replace Microsoft.NET.Sdk.Worker's implicit defaults) - PaperlessREST.Tests → ANcpLua.NET.Sdk.Web + IsTestProject=true (WebApplicationFactory needs Web SDK; IsTestProject opts in Tests.targets) - PaperlessServices.Tests → ANcpLua.NET.Sdk.Test - Pipeline/Build.csproj stays on Microsoft.NET.Sdk by design (NUKE bootstrap self-contained, opts out of root CPVM) What the SDK brings (Common.props): AnalysisLevel=latest-all, EnforceCodeStyleInBuild, Features=strict, Deterministic, embedded source link, CentralPackageTransitivePinningEnabled, NuGetAudit=all+low, TreatWarningsAsErrors in CI/Release, and 9 auto-injected editorconfigs (Global, CodingStyle, Compiler, GeneratedFiles, NamingConvention, per-analyzer for NetAnalyzers/BannedApiAnalyzers/xunit/AwesomeAssertions/ANcpLua). Test projects also auto-receive AwesomeAssertions + xunit.v3.mtp-v2. JS frontends opt into the Debug solution config via <Build Solution="Debug|*"/> in Paperless.slnx and bump Microsoft.VisualStudio.JavaScript.Sdk to 1.0.5483906 (latest on nuget.org); <ShouldRunNpmInstall>false</ShouldRunNpmInstall> keeps dotnet build inert for them so pnpm-lock.yaml is preserved (CI builds them via their own pnpm jobs per CLAUDE.md). Security: pin NuGet.Packaging 7.3.1 and System.Security.Cryptography.Xml 10.0.7 directly on Pipeline/Build.csproj (closes GHSA-g4vj-cjjj-v7hg, GHSA-37gx-xxp4-5rgx, GHSA-w3x6-4m5h-cxqf transitive through Nuke.Common); the same pins live in Directory.Packages.props so Paperless projects under CPVM get them via transitive pinning. Genuine bugs surfaced by latest-all are fixed in-tree: - xUnit1051: cancellation tokens threaded through Task.Delay calls in DocumentRepositoryIntegrationTests + ErrorOr<>.UploadDocumentAsync calls in DocumentServiceErrorMappingTests - CS1573: Document.CreateFromUpload XML doc gains <param name="timeProvider"> - IDE0052 + IDE1006: unused TypedResults.NotFound()/NoContent() statics in TypedErrorOrAsyncExtensions inlined at use sites - IDE1006: ReportProcessor.Serializer renamed to s_serializer; SInitLock / SInitializedIndices renamed to s_initLock / s_initializedIndices in SearchIndexService - IDE0370: unneeded null-forgiving operators dropped on DocumentService TryMapStorageException return and ServiceCollectionExtensions.AddPostgres connection string Style/migration warnings (AL0025/AL0026/AL0039/RS0030 BannedSymbols, CA2000, CA1034, etc. — 371 sites mostly in test code) are surfaced as warnings but carved out of TreatWarningsAsErrors at Directory.Build.props via WarningsNotAsErrors, scheduled for follow-up cleanup PRs per qyl's pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace both PaperlessUI.{Angular,React}.esproj projects with the qyl
hand-rolled MSBuild pattern (qyl/services/qyl.dashboard/qyl.dashboard.esproj),
adapted for pnpm. No more <Project Sdk=...>; custom Restore/Build/Test/Clean
targets shell to pnpm only when RunFrontendOnBuild=true, defaulting to no-op
so CI's dedicated frontend jobs remain the single source of truth.
Removes the entire shim stack that existed only to neutralize the JS SDK:
- <Project Sdk="Microsoft.VisualStudio.JavaScript.Sdk/1.0.5483906">
- <ShouldRunBuildScript>false</ShouldRunBuildScript>
- <ShouldRunNpmInstall>false</ShouldRunNpmInstall>
- <JavaScriptTestRoot>src\</JavaScriptTestRoot> (VS-Windows only)
- <JavaScriptTestFramework>Jasmine|Vitest</...> (VS-Windows only)
- <BuildOutputFolder>$(MSBuildProjectDirectory)\dist\...> (backslashes leaked
literal 'obj\Debug' folders on macOS/Linux every time dotnet build ran)
- <Build Solution="Debug|*"/> opt-in in Paperless.slnx (only needed because
the JS SDK ignored projects that weren't explicitly selected per config)
The bootstrap commit (02ce2b3, today 01:34 UTC+2) hand-typed SDK version
1.0.1641815, which is a Visual Studio Windows internal build number from the
IDE's private NuGet feed and never published to nuget.org — that's why
dotnet build outside VS-Windows failed to resolve it. Dropping the SDK
entirely removes the version-pinning fragility for good.
Both projects still surface their source trees to Rider's solution explorer
via <None Include="src/**/*"/> etc., and 'dotnet build Paperless.slnx'
remains green (0 errors). Real frontend builds keep happening in the pnpm CI
jobs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (19)
📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.props⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
Summary by CodeRabbitChores
WalkthroughDie Änderung aktualisiert zentrale MSBuild-Eigenschaften: ChangesZentrale Build-Konfiguration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 101 |
| Duplication | 15 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull request overview
This PR modernizes the repo’s .NET build/dependency setup by adopting ANcpLua.NET.Sdk v3.4.27 across projects, centralizing package/analyzer management, and decoupling the frontend .esproj files from the fragile Microsoft.VisualStudio.JavaScript.Sdk packaging.
Changes:
- Adopt
ANcpLua.NET.Sdk/.Web/.Test(3.4.27) across .NET projects and adjust project properties accordingly. - Enable central transitive package pinning and add explicit security override versions for
NuGet.PackagingandSystem.Security.Cryptography.Xml. - Replace VS JavaScript SDK-based
.esprojfiles with hand-rolled MSBuild projects that are CI-friendly and keepdotnet buildworking.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Pipeline/Build.csproj | Adds explicit package overrides to address vulnerable transitive dependencies in the NUKE toolchain. |
| PaperlessUI.React/PaperlessUI.React.esproj | Replaces VS JavaScript SDK usage with a minimal MSBuild project shell (no-op by default). |
| PaperlessUI.Blazor/PaperlessUI.Blazor.csproj | Switches to ANcpLua.NET.Sdk.Web and updates packaging settings. |
| PaperlessUI.Angular/PaperlessUI.Angular.esproj | Replaces VS JavaScript SDK usage with a minimal MSBuild project shell (no-op by default). |
| PaperlessServices/PaperlessServices.csproj | Migrates worker project to ANcpLua.NET.Sdk and compensates for lost Worker-SDK implicit usings. |
| PaperlessServices/Features/OcrProcessing/Infrastructure/Search/SearchIndexService.cs | Renames private static fields to s_ prefix for consistency. |
| PaperlessServices.Tests/PaperlessServices.Tests.csproj | Migrates test project to ANcpLua.NET.Sdk.Test and relies on SDK-injected test stack. |
| PaperlessREST/PaperlessREST.csproj | Migrates REST API project to ANcpLua.NET.Sdk.Web and adjusts packaging settings. |
| PaperlessREST/Host/Extensions/TypedErrorOrAsyncExtensions.cs | Simplifies result conversion by using TypedResults directly instead of cached static results. |
| PaperlessREST/Host/Extensions/ServiceCollectionExtensions.cs | Tweaks Postgres configuration wiring (notably connection string retrieval). |
| PaperlessREST/Features/DocumentManagement/Application/DocumentService.cs | Minor cleanup in exception-to-domain-error mapping (null! → null). |
| PaperlessREST/Features/DocumentManagement/Application/Document.cs | Documentation update to clarify injected TimeProvider usage for testability. |
| PaperlessREST/Features/BatchProcessing/Application/ReportProcessor.cs | Renames serializer static field to match s_ convention. |
| PaperlessREST.Tests/Unit/DocumentServiceErrorMappingTests.cs | Updates tests to pass CancellationToken through service calls. |
| PaperlessREST.Tests/PaperlessREST.Tests.csproj | Migrates test project SDK usage and relies on SDK-injected AwesomeAssertions/analyzers via IsTestProject. |
| PaperlessREST.Tests/Integration/DocumentRepositoryIntegrationTests.cs | Passes CancellationToken to Task.Delay calls for better cancellation behavior. |
| Paperless.Contracts/Paperless.Contracts.csproj | Migrates contracts project to ANcpLua.NET.Sdk. |
| global.json | Pins ANcpLua.NET.* MSBuild SDK versions to 3.4.27. |
| Directory.Packages.props | Enables transitive pinning and centralizes security override versions; removes direct AwesomeAssertions version management. |
| Directory.Build.props | Adds WarningsNotAsErrors carve-outs for incremental analyzer cleanup. |
| .nuke/build.schema.json | Adds a Verify target to the NUKE schema. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private IServiceCollection AddPostgres(IConfiguration config) | ||
| { | ||
| NpgsqlDataSource dataSource = new NpgsqlDataSourceBuilder(config.GetConnectionString("PaperlessDb")!) | ||
| NpgsqlDataSource dataSource = new NpgsqlDataSourceBuilder(config.GetConnectionString("PaperlessDb")) |
| <Target Name="Build" DependsOnTargets="Restore"> | ||
| <Message Text="[PaperlessUI.React] Build is a no-op; pnpm run build runs in the CI frontend job." Importance="low"/> | ||
| <Exec Command="$(PnpmExe) install --frozen-lockfile=false" WorkingDirectory="$(FrontendRoot)" Condition=" '$(RunFrontendOnBuild)' == 'true' "/> | ||
| <Exec Command="$(PnpmExe) run build" WorkingDirectory="$(FrontendRoot)" Condition=" '$(RunFrontendOnBuild)' == 'true' "/> | ||
| </Target> |
|
|
||
| <Target Name="Build" DependsOnTargets="Restore"> | ||
| <Message Text="[PaperlessUI.Angular] Build is a no-op; pnpm run build runs in the CI frontend job." Importance="low"/> | ||
| <Exec Command="$(PnpmExe) install --frozen-lockfile=false" WorkingDirectory="$(FrontendRoot)" Condition=" '$(RunFrontendOnBuild)' == 'true' "/> |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces several infrastructure improvements, but contains critical errors that will prevent a successful build. Specifically, the package versions specified in Directory.Packages.props (7.3.1 and 10.0.7) are invalid and do not exist on the public NuGet gallery.
Furthermore, there is a major misalignment between the PR metadata and the code changes: the title and description mention 'refactoring JS SDK usage' and switching project-level SDKs in .csproj files, but no such changes appear in the diff. Beyond these blockers, there are logic issues regarding potential ArgumentNullException in the database setup and several quality findings related to high method complexity and incomplete assertions in integration tests.
About this PR
- The description claims project files (.csproj) were switched from standard Microsoft SDKs to custom ones, but the diff only shows SDK version updates in global.json. If project-level changes were intended, they appear to be missing from this commit.
- The PR title and description mention refactoring 'JS SDK usage', but no JavaScript files, npm packages, or JS-related logic are included in the code changes. Please ensure the PR description accurately reflects the scope of the changes.
3 comments outside of the diff
PaperlessREST.Tests/Integration/DocumentRepositoryIntegrationTests.cs
line 325-38111🟡 MEDIUM RISK
The local variables 'hasMore' and 'hasMoreSecond' are unused. Add assertions to verify these flags or use a discard if the values are intentionally ignored.
PaperlessREST/Features/BatchProcessing/Application/ReportProcessor.cs
line 65🟡 MEDIUM RISK
This method has high cyclomatic complexity and length. Consider extracting the XML schema validation and the deserialization logic into dedicated private methods to improve testability and readability.
PaperlessREST/Host/Extensions/ServiceCollectionExtensions.cs
line 123🟡 MEDIUM RISK
Suggestion: This dependency injection registration method is too large (168 lines). It is recommended to break this down into logical feature-based extension methods such as 'AddDatabaseServices', 'AddMessaging', or 'AddDocumentFeature'.
Test suggestions
- Verify CentralPackageTransitivePinningEnabled is set to true to ensure security overrides propagate.
- Verify that Task.Delay calls in DocumentRepositoryIntegrationTests receive the CancellationToken from TestContext.
- Verify that private static fields in ReportProcessor and SearchIndexService follow the 's_' naming convention.
- Verify the 'Verify' target is present in the NUKE build schema.
- Verify that DocumentService.UploadDocumentAsync calls in tests propagate the CancellationToken.
- Assert the value of 'hasMore' in DocumentRepositoryIntegrationTests.cs to verify pagination completeness.
- Assert the value of 'hasMoreSecond' in DocumentRepositoryIntegrationTests.cs to verify multi-page pagination.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Assert the value of 'hasMore' in DocumentRepositoryIntegrationTests.cs to verify pagination completeness.
2. Assert the value of 'hasMoreSecond' in DocumentRepositoryIntegrationTests.cs to verify multi-page pagination.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| <PackageVersion Include="NuGet.Packaging" Version="7.3.1" /> | ||
| <PackageVersion Include="System.Security.Cryptography.Xml" Version="10.0.7" /> |
There was a problem hiding this comment.
🔴 HIGH RISK
Update the package versions to match the stable releases mentioned in the comment to ensure a successful build. The versions 7.3.1 and 10.0.7 do not exist for these packages.
| <PackageVersion Include="NuGet.Packaging" Version="7.3.1" /> | |
| <PackageVersion Include="System.Security.Cryptography.Xml" Version="10.0.7" /> | |
| <PackageVersion Include="NuGet.Packaging" Version="6.12.1" /> | |
| <PackageVersion Include="System.Security.Cryptography.Xml" Version="9.0.1" /> |
| private IServiceCollection AddPostgres(IConfiguration config) | ||
| { | ||
| NpgsqlDataSource dataSource = new NpgsqlDataSourceBuilder(config.GetConnectionString("PaperlessDb")!) | ||
| NpgsqlDataSource dataSource = new NpgsqlDataSourceBuilder(config.GetConnectionString("PaperlessDb")) |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Add a guard clause to handle missing connection strings explicitly, which improves error clarity and satisfies the compiler when NRT is enabled.
| NpgsqlDataSource dataSource = new NpgsqlDataSourceBuilder(config.GetConnectionString("PaperlessDb")) | |
| string connectionString = config.GetConnectionString("PaperlessDb") ?? throw new InvalidOperationException("Connection string 'PaperlessDb' not found."); | |
| NpgsqlDataSource dataSource = new NpgsqlDataSourceBuilder(connectionString) |
| private static readonly NotFound NotFound = TypedResults.NotFound(); | ||
| private static readonly NoContent NoContent = TypedResults.NoContent(); | ||
|
|
||
| private static ValidationProblem CreateValidationProblem(IReadOnlyList<Error> errors) => |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Re-introduce the cached static instances for NotFound and NoContent results to maintain performance and reduce allocations in high-traffic scenarios.
There was a problem hiding this comment.
Pull Request Overview
This PR successfully adopts ANcpLua.NET.Sdk 3.4.27 and implements central package pinning to address transitive security vulnerabilities. While the codebase is generally up to standards according to Codacy, there are critical discrepancies and logic gaps that must be addressed before merging.
Most importantly, there is a temporal inconsistency in the Document entity where TimeProvider is ignored during GUID v7 generation, which will lead to non-deterministic behavior in tests. Additionally, the PR title and description mention refactors (JS SDK and Directory.Build.props) that are completely absent from the file diff, suggesting a partial commit or misleading metadata. Finally, thread-safety concerns in ReportProcessor and excessive complexity in XML parsing should be resolved to ensure system stability.
About this PR
- The PR title references 'refactor JS SDK usage' and the description mentions 'Directory.Build.props' configuration, but neither of these changes are present in the submitted files. Please verify if some files were missed in the commit.
4 comments outside of the diff
PaperlessREST.Tests/Integration/DocumentRepositoryIntegrationTests.cs
line 325🟡 MEDIUM RISK
The local variable 'hasMore' is declared but never used. Replacing it with a discard improves code clarity:(List<Document> results, _) = await _repository.
line 381🟡 MEDIUM RISK
The local variable 'hasMoreSecond' is not used in this test. Consider using a discard to simplify the assignment:(List<Document> secondPage, _) = await _repository.
PaperlessREST/Features/BatchProcessing/Application/ReportProcessor.cs
line 65🟡 MEDIUM RISK
This method is doing too much and is difficult to test in isolation. Consider extracting theXmlReaderSettingsconfiguration and theAccessReportDtovalidation logic into separate private methods. Specifically, extractGetXmlReaderSettingsand aValidateReportDtomethod to reduce cyclomatic complexity and improve readability.
PaperlessREST/Features/DocumentManagement/Application/Document.cs
line 104🟡 MEDIUM RISK
ThetimeProvideris ignored when generating the Version 7 GUID. In tests, this leads to a mismatch between the document'sCreatedAttimestamp (mocked) and the timestamp embedded in theId(real system clock). Since .NET'sGuid.CreateVersion7()does not currently support a custom timestamp override, this creates a temporal inconsistency in the domain entity metadata.
Test suggestions
- Verify that Document.CreateFromUpload correctly uses the injected TimeProvider for CreatedAt and StoragePath generation.
- Verify that SearchIndexService correctly handles concurrent initialization using the renamed s_initLock.
- Verify that ReportProcessor correctly deserializes reports using the renamed s_serializer.
- Verify that DocumentRepositoryIntegrationTests successfully cancel operations when a CancellationToken is triggered during Task.Delay.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that Document.CreateFromUpload correctly uses the injected TimeProvider for CreatedAt and StoragePath generation.
2. Verify that SearchIndexService correctly handles concurrent initialization using the renamed s_initLock.
3. Verify that ReportProcessor correctly deserializes reports using the renamed s_serializer.
Low confidence findings
- While central package pinning is enabled in Directory.Packages.props, there is no verification that the build process correctly resolves to these pinned versions. Consider adding a build-time check or confirming via logs.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| private static readonly XmlSerializer Serializer = new(typeof(AccessReportDto)); | ||
| private static readonly XmlSerializer s_serializer = new(typeof(AccessReportDto)); | ||
|
|
||
| private XmlSchemaSet Schemas => field ??= LoadSchemas(); |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Lazy initialization with field ??= is not thread-safe. Concurrent requests may cause LoadSchemas() to run multiple times. Consider using LazyInitializer.EnsureInitialized or a Lazy<XmlSchemaSet> field to ensure the schema set is built only once in a thread-safe manner.
|
Superseded — the substantive changes here landed via PR #10 ( Branch |
This pull request introduces several infrastructure and codebase updates to modernize the build system, improve dependency management, and enhance code quality and consistency. The most significant changes involve switching project SDKs to custom ANcpLua.NET SDKs, centralizing dependency and analyzer management, addressing security vulnerabilities in transitive dependencies, and improving code clarity and testability.
Build and Dependency Management Modernization:
.csproj) from standard Microsoft SDKs to customANcpLua.NET.SdkandANcpLua.NET.Sdk.Web/TestSDKs, enabling centralized test stack injection, improved analyzer management, and streamlined test configuration. [1] [2] [3] [4]NuGet.Packaging,System.Security.Cryptography.Xml) to address known GitHub security advisories. [1] [2]AwesomeAssertionsand its analyzers from test projects, as these are now injected by the custom SDK. [1] [2] [3]Build Process and Analyzer Configuration:
.nuke/build.schema.jsonto add aVerifystep to the build pipeline.Directory.Build.propsto treat most warnings as errors, but carved out specific analyzer rule IDs for deferred cleanup, improving code quality while allowing incremental adoption.Code Quality and Consistency Improvements:
s_prefix for private static fields inReportProcessorandSearchIndexService. [1] [2]TypedErrorOrAsyncExtensionsby removing unnecessary static fields and usingTypedResultsdirectly. [1] [2] [3]nullinstead ofnull!where appropriate and removing redundant code in build scripts.Test Improvements and Reliability:
CancellationTokentoTask.Delayand service methods, improving test reliability and cancellation support. [1] [2] [3] [4] [5] [6] [7]Other Notable Changes:
These changes collectively modernize the build and dependency management, improve security, and enhance codebase maintainability.