Fix NRT errors and enable TreatWarningsAsErrors by niemyjski · Pull Request #46 · FoundatioFx/Foundatio.Aliyun

niemyjski · 2026-04-10T03:07:03Z

Summary

Enabled <TreatWarningsAsErrors>true\ in build configuration
Fixed nullable reference annotations in Aliyun storage provider

Test plan

\dotnet build Foundatio.All.slnx\ — 0 warnings, 0 errors

- Enable TreatWarningsAsErrors in build - Fix nullable reference annotations in Aliyun storage implementation

Copilot

Pull request overview

Updates the Aliyun storage provider and build configuration to support stricter compilation (warnings as errors) by addressing nullable-reference-type (NRT) warnings and aligning dependency versions.

Changes:

Bumped Foundatio / Foundatio.TestHarness package references to 13.0.0-beta3.35.
Adjusted Aliyun storage/provider option and connection-string builder nullability annotations.
Minor internal refactors related to nullability initialization.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/Directory.Build.props	Updates `Foundatio.TestHarness` package version.
src/Foundatio.Aliyun/Foundatio.Aliyun.csproj	Updates `Foundatio` package version.
src/Foundatio.Aliyun/Storage/AliyunFileStorageOptions.cs	Changes `ConnectionString` nullability/initialization.
src/Foundatio.Aliyun/AliyunConnectionStringBuilder.cs	Changes connection-string parts (`Endpoint`, `AccessKey`, `SecretKey`) nullability + ctor signature.
src/Foundatio.Aliyun/Storage/AliyunFileStorageConnectionStringBuilder.cs	Changes `_bucket` init and ctor signature nullability.
src/Foundatio.Aliyun/Storage/AliyunFileStorage.cs	Updates `SearchCriteria.Prefix` initialization for NRT.

Comments suppressed due to low confidence (1)

src/Foundatio.Aliyun/AliyunConnectionStringBuilder.cs:20

Changing the constructor parameter from string? to string is a source-breaking nullability change for any consumers/derivers that may currently pass null (even though it throws at runtime). If you want to preserve compatibility while still validating, consider keeping the string? signature (or adding an overload) and continuing to throw for null/empty input.

    protected AliyunConnectionStringBuilder() { }

    protected AliyunConnectionStringBuilder(string connectionString)
    {
        if (String.IsNullOrEmpty(connectionString))
            throw new ArgumentNullException(nameof(connectionString));
        Parse(connectionString);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Foundatio.Aliyun/AliyunConnectionStringBuilder.cs

+    public string Endpoint { get; set; } = null!;

-    public string? AccessKey { get; set; }
+    public string AccessKey { get; set; } = null!;

-    public string? SecretKey { get; set; }
+    public string SecretKey { get; set; } = null!;


src/Foundatio.Aliyun/Storage/AliyunFileStorageOptions.cs

 public class AliyunFileStorageOptions : SharedOptions
 {
-    public string? ConnectionString { get; set; }
+    public string ConnectionString { get; set; } = null!;


src/Foundatio.Aliyun/Storage/AliyunFileStorageConnectionStringBuilder.cs

    }

-    public AliyunFileStorageConnectionStringBuilder(string? connectionString) : base(connectionString)
+    public AliyunFileStorageConnectionStringBuilder(string connectionString) : base(connectionString)


Address PR review comments: properties annotated as non-nullable string were initialized with null!, making the nullability contract inaccurate. Initialize with string.Empty instead so the non-nullable annotation is truthful at runtime.

niemyjski

@copilot review

…ring.Empty Tests assert null for unset properties. Using string.Empty changed the semantic meaning of not-set from null to empty, breaking 3 tests. Properly nullable properties preserve original behavior.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T03:51:24Z

src/Foundatio.Aliyun/Storage/AliyunFileStorage.cs

        _logger = options.LoggerFactory?.CreateLogger(GetType()) ?? NullLogger.Instance;

+        if (String.IsNullOrEmpty(options.ConnectionString))
+            throw new ArgumentException("ConnectionString is required.", nameof(options));


The ArgumentException uses nameof(options) as the parameter name, but the invalid value is options.ConnectionString. Using nameof(options.ConnectionString) will make the exception’s ParamName accurate and improve diagnosability.

Suggested change

throw new ArgumentException("ConnectionString is required.", nameof(options));

throw new ArgumentException("ConnectionString is required.", nameof(options.ConnectionString));

Copilot · 2026-04-10T03:51:24Z

src/Foundatio.Aliyun/Storage/AliyunFileStorage.cs

    private class SearchCriteria
    {
-        public string Prefix { get; set; } = String.Empty;
+        public string Prefix { get; set; } = null!;


Prefix is initialized with null!, which makes the non-nullable contract less truthful and can allow a null value if SearchCriteria is ever instantiated without setting Prefix. Initializing it to String.Empty (as before) avoids the null-forgiving operator and keeps it safe by default.

Suggested change

public string Prefix { get; set; } = null!;

public string Prefix { get; set; } = String.Empty;

Fix NRT errors and enable TreatWarningsAsErrors

2aa0953

- Enable TreatWarningsAsErrors in build - Fix nullable reference annotations in Aliyun storage implementation

niemyjski requested a review from Copilot April 10, 2026 03:17

Copilot started reviewing on behalf of niemyjski April 10, 2026 03:18 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

niemyjski commented Apr 10, 2026

View reviewed changes

Copilot stopped work on behalf of niemyjski due to an error April 10, 2026 03:36
Before you can use Copilot cloud agent, you need to pick a "Usage billed to" option in your Copilot settings. Update your settings at https://github.com/settings/copilot/features, then try again.

Fix connection string builder properties to use string? instead of st…

f83cc4d

…ring.Empty Tests assert null for unset properties. Using string.Empty changed the semantic meaning of not-set from null to empty, breaking 3 tests. Properly nullable properties preserve original behavior.

niemyjski requested a review from Copilot April 10, 2026 03:49

Copilot started reviewing on behalf of niemyjski April 10, 2026 03:50 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix NRT errors and enable TreatWarningsAsErrors#46

Fix NRT errors and enable TreatWarningsAsErrors#46
niemyjski wants to merge 3 commits intomainfrom
nrt/treat-warnings-as-errors

niemyjski commented Apr 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

niemyjski left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 10, 2026

Uh oh!

Copilot AI Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	throw new ArgumentException("ConnectionString is required.", nameof(options));
	throw new ArgumentException("ConnectionString is required.", nameof(options.ConnectionString));

	public string Prefix { get; set; } = null!;
	public string Prefix { get; set; } = String.Empty;

Uh oh!

Conversation

niemyjski commented Apr 10, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

niemyjski left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants