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

niemyjski · 2026-04-10T03:06:42Z

Summary

Replaced ineffective <WarningsAsErrors>true\ with correct <TreatWarningsAsErrors>true\
Fixed nullable reference warnings in queue implementation and tests

Changes

build/common.props: \WarningsAsErrors\ → \TreatWarningsAsErrors\
AzureStorageQueue.cs: Null guard after envelope deserialization
Tests: Assert.NotNull guards before Value dereference
Samples: Conditional Properties assignment

Test plan

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

- Replace ineffective WarningsAsErrors=true with TreatWarningsAsErrors=true - Add null guard after deserializing queue message envelope - Fix nullable Properties assignment in sample - Add Assert.NotNull for queue entry Value in tests

Copilot

Pull request overview

This PR tightens build-time correctness by enabling nullable reference type analysis and treating warnings as errors, then updates Azure Storage queue/file storage code plus tests/samples to satisfy the new nullability requirements.

Changes:

Enable <Nullable>enable</Nullable> and enforce <TreatWarningsAsErrors>true</TreatWarningsAsErrors> at the common build props level.
Align Azure Storage queue/file storage APIs and internal models with nullable annotations (including safer handling around message envelope deserialization).
Update queue/storage tests and sample apps to avoid nullable dereferences and to set optional properties conditionally.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/Foundatio.AzureStorage.Tests/Storage/AzureStorageTests.cs	Adjusts connection string nullability and suppresses NRT warnings when returning null storage in test setup.
tests/Foundatio.AzureStorage.Tests/Queues/LegacyAzureStorageQueueTests.cs	Makes optional parameters nullable and avoids passing nulls into option builder methods.
tests/Foundatio.AzureStorage.Tests/Queues/AzureStorageQueueTests.cs	Adds null guards in tests and minor connection string typing tweaks.
tests/Directory.Build.props	Bumps `Foundatio.TestHarness` package version.
src/Foundatio.AzureStorage/Storage/AzureFileStorageOptions.cs	Initializes required options and marks optional retry configuration nullable.
src/Foundatio.AzureStorage/Storage/AzureFileStorage.cs	Updates return types/parameters for nullability (e.g., file-not-found returning null) and propagates nullable searchPattern handling.
src/Foundatio.AzureStorage/Queues/AzureStorageQueueOptions.cs	Initializes required options and marks optional retry configuration nullable.
src/Foundatio.AzureStorage/Queues/AzureStorageQueueEntry.cs	Makes correlationId/properties nullable in the entry constructor to match actual usage.
src/Foundatio.AzureStorage/Queues/AzureStorageQueue.cs	Makes enqueue/dequeue return types nullable and adds a null guard/fallback around envelope deserialization.
src/Foundatio.AzureStorage/Foundatio.AzureStorage.csproj	Bumps `Foundatio` package version.
samples/Foundatio.AzureStorage.Enqueue/Program.cs	Adjusts nullability at call sites and sets `QueueEntryOptions.Properties` conditionally.
samples/Foundatio.AzureStorage.Dequeue/Program.cs	Adjusts nullability at call sites for queue name.
build/common.props	Enables nullable, enforces warnings-as-errors, and updates SourceLink package version.

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

niemyjski

@copilot review

entry.Value can be null for queue entries; use null-conditional access.

Copilot

Pull request overview

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

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

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

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

- ConnectionString properties: null! -> string? (validated at construction) - SearchCriteria.Prefix: null! -> String.Empty - QueueMessageEnvelope.Data: null! -> required modifier - Test GetQueue/GetStorage: return null! -> nullable return type + null - Samples: string.Empty/string.Join -> String.Empty/String.Join

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

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

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

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

- Change connection string validation from ThrowIfNullOrEmpty to ThrowIfNullOrWhiteSpace

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/Foundatio.AzureStorage/Queues/AzureStorageQueueOptions.cs:85

AzureStorageQueueOptionsBuilder.ConnectionString now rejects whitespace-only values, but AzureStorageQueue’s constructor still only checks String.IsNullOrEmpty(options.ConnectionString). If options are instantiated/bound without the builder, a whitespace connection string will slip through and fail later inside the Azure SDK. Consider aligning the runtime validation to IsNullOrWhiteSpace (and throwing a clear ArgumentException) so both configuration paths behave consistently.

public class AzureStorageQueueOptionsBuilder<T> : SharedQueueOptionsBuilder<T, AzureStorageQueueOptions<T>, AzureStorageQueueOptionsBuilder<T>> where T : class
{
    public AzureStorageQueueOptionsBuilder<T> ConnectionString(string connectionString)
    {
        ArgumentException.ThrowIfNullOrWhiteSpace(connectionString);

        Target.ConnectionString = connectionString;
        return this;
    }

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

- Revert Microsoft.SourceLink.GitHub from 10.0.200 back to 10.0.201 - Use ThrowIfNullOrWhiteSpace for ConnectionString validation in queue and storage constructors to align with builder-side validation - Add missing ConnectionString validation in AzureFileStorage constructor - Use ThrowIfNullOrWhiteSpace for ContainerName in storage builder

Copilot

Pull request overview

Copilot reviewed 14 out of 14 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-10T19:55:35Z

    <NoWarn>$(NoWarn);CS1591;NU1701</NoWarn>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
    <LangVersion>latest</LangVersion>
+    <Nullable>enable</Nullable>
    <GenerateDocumentationFile>true</GenerateDocumentationFile>


PR description mentions replacing <WarningsAsErrors> with <TreatWarningsAsErrors>, but this repo doesn’t appear to contain <WarningsAsErrors> anywhere, and TreatWarningsAsErrors is already present here. Please update the PR description (or include the missing change) so it accurately reflects what’s actually being modified (e.g., enabling <Nullable>enable</Nullable> in this file).

niemyjski · 2026-04-10T20:04:47Z

    Console.WriteLine();

-    await DequeueMessages(connectionString, queueName, mode, count);
+    await DequeueMessages(connectionString, queueName!, mode, count);


shouldn't the console error out if these are not specified and not get this far?

Fixed: Sample now uses null coalescing with default values instead of null-forgiving operators, eliminating the ! hack.

niemyjski · 2026-04-10T20:08:25Z

    {
        // Arrange - Inject a raw (non-envelope) message simulating legacy format
-        string connectionString = Configuration.GetConnectionString("AzureStorageConnectionString");
+        var connectionString = Configuration.GetConnectionString("AzureStorageConnectionString");


always prefer built in types. this should be string?

Fixed: Changed to \string?\ (built-in type alias).

niemyjski · 2026-04-10T20:08:34Z

    public async Task DequeueAsync_WithCorruptMessage_MovesToDeadletterAsync()
    {
-        string connectionString = Configuration.GetConnectionString("AzureStorageConnectionString");
+        var connectionString = Configuration.GetConnectionString("AzureStorageConnectionString");


Suggested change

var connectionString = Configuration.GetConnectionString("AzureStorageConnectionString");

string? connectionString = Configuration.GetConnectionString("AzureStorageConnectionString");

Fixed: Using \string?\ for connectionString declarations.

niemyjski · 2026-04-10T20:08:50Z

+            if (timeProvider is not null)
+                builder.TimeProvider(timeProvider);
+            if (serializer is not null)
+                builder.Serializer(serializer);


just chain it like before this is automatically handled

Fixed: Reverted to original chaining pattern.

… comments, use guard clauses, fix nullability - Implement deserializeException pattern matching SQS/AzureServiceBus providers - Restore removed code comments (envelope unwrap, legacy fallback, legacy mode) - NormalizePath now accepts/returns string? per reviewer request - SearchCriteria.Prefix uses required property instead of String.Empty - SampleMessage uses required properties instead of String.Empty defaults - Sample programs use null coalescing instead of null-forgiving operators - Revert test GetQueue methods to original chaining pattern - Use string? (built-in alias) for connectionString in tests

niemyjski · 2026-04-10T21:10:33Z

                          EmulatorConnectionString;

-    var queueName = parseResult.GetValue(queueOption);
+    var queueName = parseResult.GetValue(queueOption) ?? "sample-queue";


shouldn't this always be specified since there is a default specified? Otherwise I'd expect some kind of error

Fixed: Added early validation for connection string and queue name — prints error and returns exit code 1 if missing.

niemyjski · 2026-04-10T21:11:12Z

+    var message = parseResult.GetValue(messageOption) ?? "Hello World";
    var correlationId = parseResult.GetValue(correlationIdOption);
-    var properties = parseResult.GetValue(propertiesOption);
+    var properties = parseResult.GetValue(propertiesOption) ?? Array.Empty<string>();


Suggested change

var properties = parseResult.GetValue(propertiesOption) ?? Array.Empty<string>();

var properties = parseResult.GetValue(propertiesOption) ?? [];

why isn't these defaults from the parser options itself?

Fixed: Using ?? []\ syntax. Removed redundant null coalescing defaults that the parser already provides via \DefaultValueFactory. Also removed the now-redundant \if (properties != null)\ check.

niemyjski · 2026-04-10T21:12:06Z

+            _logger.LogWarning(deserializeException, "Error deserializing message {MessageId} (attempt {DequeueCount}), abandoning for retry", message.MessageId, message.DequeueCount);

-            var poisonEntry = new AzureStorageQueueEntry<T>(message, null, null, null, this);
+            var poisonEntry = new AzureStorageQueueEntry<T>(message, null, null, default!, this);


WHY did this need to change, just pass null?

Fixed: Reverted to pass
ull\ directly. Updated \AzureStorageQueueEntry\ constructor to accept \T?\ so null is valid without \default!.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

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

Copilot · 2026-04-10T21:13:39Z

+    var message = parseResult.GetValue(messageOption) ?? "Hello World";
    var correlationId = parseResult.GetValue(correlationIdOption);
-    var properties = parseResult.GetValue(propertiesOption);
+    var properties = parseResult.GetValue(propertiesOption) ?? Array.Empty<string>();


propertiesOption already provides a non-null default (Array.Empty) and this handler now also coalesces to Array.Empty<string>(), so properties is guaranteed non-null. The subsequent if (properties != null) check inside EnqueueMessages is now redundant—consider removing the null check and treating properties as always non-null throughout the sample to simplify the code path.

Suggested change

var properties = parseResult.GetValue(propertiesOption) ?? Array.Empty<string>();

var properties = parseResult.GetValue(propertiesOption);

…lidation, normalize path handling, pass null for poison entry

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

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

Copilot · 2026-04-11T02:07:34Z


        // Assert
-        Assert.NotNull(entry);
+        Assert.NotNull(entry?.Value);


Assert.NotNull(entry?.Value) makes it unclear whether the failure is because entry is null or because entry.Value is null, and it also doesn’t reliably help nullable flow analysis for subsequent entry.Value / entry.CompleteAsync() usages. Prefer asserting entry is non-null first (and then entry.Value if needed) so the intent and nullability narrowing are explicit.

Suggested change

Assert.NotNull(entry?.Value);

Assert.NotNull(entry);

Assert.NotNull(entry.Value);

Copilot · 2026-04-11T02:07:35Z

+    public async Task<Stream?> GetFileStreamAsync(string path, StreamMode streamMode, CancellationToken cancellationToken = default)
    {
        ArgumentException.ThrowIfNullOrEmpty(path);

-        string normalizedPath = NormalizePath(path);
+        string normalizedPath = NormalizePath(path)!;
        _logger.LogTrace("Getting file stream for {Path}", normalizedPath);



NormalizePath was made nullable (string?) which forces a proliferation of null-forgiveness (!) at call sites even when inputs have already been validated as non-null/non-empty. Consider keeping NormalizePath non-nullable for required paths (or adding an overload) so callers don’t need ! and nullability stays precise.

Copilot · 2026-04-11T02:07:35Z

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


The constructor validation uses options?.ConnectionString even though options is expected to be non-null (and is immediately dereferenced later). Using the null-conditional here can mask an unexpected null options (changing the failure mode) and is inconsistent with the AzureFileStorage constructor validation. Prefer validating options.ConnectionString directly after ensuring options is non-null.

Suggested change

ArgumentException.ThrowIfNullOrWhiteSpace(options?.ConnectionString, nameof(options.ConnectionString));

ArgumentNullException.ThrowIfNull(options);

ArgumentException.ThrowIfNullOrWhiteSpace(options.ConnectionString, nameof(options.ConnectionString));

Copilot · 2026-04-11T02:07:35Z

+
+        if (data is null)
+        {
+            _logger.LogWarning(deserializeException, "Error deserializing message {MessageId} (attempt {DequeueCount}), abandoning for retry", message.MessageId, message.DequeueCount);


When data ends up null but no exception was thrown during deserialization (e.g., payload is literal null or an envelope deserializes with Data=null), deserializeException remains null and the log message still says "Error deserializing". Consider handling the deserializeException == null case separately (e.g., log that the payload was null) so operational logs accurately describe why the message is being abandoned/dead-lettered.

Suggested change

_logger.LogWarning(deserializeException, "Error deserializing message {MessageId} (attempt {DequeueCount}), abandoning for retry", message.MessageId, message.DequeueCount);

if (deserializeException is null)

_logger.LogWarning("Message {MessageId} deserialized to null payload (attempt {DequeueCount}), abandoning for retry", message.MessageId, message.DequeueCount);

else

_logger.LogWarning(deserializeException, "Error deserializing message {MessageId} (attempt {DequeueCount}), abandoning for retry", message.MessageId, message.DequeueCount);

This is invalid feedback, it's fine to pass null for the exception here and it's what the logging api's do internally.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

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

…on-nullable

niemyjski · 2026-04-11T15:52:23Z

@copilot review

Fix NRT errors and enable TreatWarningsAsErrors

c0e1e9e

- Replace ineffective WarningsAsErrors=true with TreatWarningsAsErrors=true - Add null guard after deserializing queue message envelope - Fix nullable Properties assignment in sample - Add Assert.NotNull for queue entry Value in tests

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

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