-
-
Notifications
You must be signed in to change notification settings - Fork 13
Fix NRT errors and enable TreatWarningsAsErrors #72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
c0e1e9e
68f2a83
b482b82
bfc0638
03b47fb
51506c7
f9dd5b5
8ec53c0
495f7f6
e795f95
63c518a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,7 +60,7 @@ | |
| Console.WriteLine("Press Ctrl+C to stop..."); | ||
| Console.WriteLine(); | ||
|
|
||
| await DequeueMessages(connectionString, queueName, mode, count); | ||
| await DequeueMessages(connectionString, queueName!, mode, count); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't the console error out if these are not specified and not get this far?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: Sample now uses null coalescing with default values instead of null-forgiving operators, eliminating the ! hack. |
||
| return 0; | ||
|
niemyjski marked this conversation as resolved.
Outdated
|
||
| }); | ||
|
|
||
|
|
@@ -120,14 +120,14 @@ static async Task DequeueMessages(string connectionString, string queueName, Azu | |
| processed++; | ||
|
|
||
| logger.LogInformation("Dequeued message {MessageId}: '{Message}' from '{Source}' at {Timestamp}", | ||
| entry.Id, entry.Value.Message, entry.Value.Source, entry.Value.Timestamp); | ||
| entry.Id, entry.Value?.Message, entry.Value?.Source, entry.Value?.Timestamp); | ||
|
|
||
| logger.LogInformation(" CorrelationId: '{CorrelationId}'", entry.CorrelationId ?? "<none>"); | ||
|
|
||
| if (entry.Properties != null && entry.Properties.Count > 0) | ||
| { | ||
| logger.LogInformation(" Properties: [{Properties}]", | ||
| string.Join(", ", entry.Properties.Select(p => $"{p.Key}={p.Value}"))); | ||
| String.Join(", ", entry.Properties.Select(p => $"{p.Key}={p.Value}"))); | ||
| } | ||
| else | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,14 +79,14 @@ | |
| Console.WriteLine($"Mode: {mode}"); | ||
| Console.WriteLine(); | ||
|
|
||
| await EnqueueMessages(connectionString, queueName, message, correlationId, properties, mode, count); | ||
| await EnqueueMessages(connectionString, queueName!, message!, correlationId, properties!, mode, count); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't the console error out if these are not specified and not get this far?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: Sample now uses null coalescing with default values instead of null-forgiving operators. |
||
| return 0; | ||
|
niemyjski marked this conversation as resolved.
Outdated
|
||
| }); | ||
|
|
||
| // Parse and invoke | ||
| return await rootCommand.Parse(args).InvokeAsync(); | ||
|
|
||
| static async Task EnqueueMessages(string connectionString, string queueName, string message, string correlationId, string[] properties, AzureStorageQueueCompatibilityMode mode, int count) | ||
| static async Task EnqueueMessages(string connectionString, string queueName, string message, string? correlationId, string[] properties, AzureStorageQueueCompatibilityMode mode, int count) | ||
| { | ||
| using var loggerFactory = LoggerFactory.Create(builder => builder.AddConsole().SetMinimumLevel(LogLevel.Information)); | ||
| var logger = loggerFactory.CreateLogger("Enqueue"); | ||
|
|
@@ -122,15 +122,17 @@ static async Task EnqueueMessages(string connectionString, string queueName, str | |
|
|
||
| var entryOptions = new QueueEntryOptions | ||
| { | ||
| CorrelationId = correlationId, | ||
| Properties = queueProperties.Count > 0 ? queueProperties : null | ||
| CorrelationId = correlationId | ||
| }; | ||
|
|
||
| if (queueProperties.Count > 0) | ||
| entryOptions.Properties = queueProperties; | ||
|
|
||
| var messageId = await queue.EnqueueAsync(sampleMessage, entryOptions); | ||
|
|
||
| logger.LogInformation("Enqueued message {MessageId}: '{Message}' with CorrelationId: '{CorrelationId}' Properties: [{Properties}]", | ||
| messageId, sampleMessage.Message, correlationId ?? "<none>", | ||
| string.Join(", ", queueProperties.Select(p => $"{p.Key}={p.Value}"))); | ||
| String.Join(", ", queueProperties.Select(p => $"{p.Key}={p.Value}"))); | ||
| } | ||
|
|
||
| logger.LogInformation("Successfully enqueued {Count} message(s)", count); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ namespace Foundatio.AzureStorage.Samples; | |
|
|
||
| public record SampleMessage | ||
| { | ||
| public string Message { get; init; } = string.Empty; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. feels like these should be made required and we should not be using String.Empty ever for defaults.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: SampleMessage now uses |
||
| public string Message { get; init; } = String.Empty; | ||
| public DateTime Timestamp { get; init; } = DateTime.UtcNow; | ||
| public string Source { get; init; } = string.Empty; | ||
| public string Source { get; init; } = String.Empty; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,7 +71,7 @@ await Task.WhenAll( | |
| } | ||
| } | ||
|
|
||
| protected override async Task<string> EnqueueImplAsync(T data, QueueEntryOptions options) | ||
| protected override async Task<string?> EnqueueImplAsync(T data, QueueEntryOptions options) | ||
| { | ||
| if (!await OnEnqueuingAsync(data, options).AnyContext()) | ||
| return null; | ||
|
|
@@ -110,7 +110,7 @@ protected override async Task<string> EnqueueImplAsync(T data, QueueEntryOptions | |
| return response.Value.MessageId; | ||
| } | ||
|
|
||
| protected override async Task<IQueueEntry<T>> DequeueImplAsync(CancellationToken linkedCancellationToken) | ||
| protected override async Task<IQueueEntry<T>?> DequeueImplAsync(CancellationToken linkedCancellationToken) | ||
| { | ||
| _logger.LogTrace("Checking for message: IsCancellationRequested={IsCancellationRequested} VisibilityTimeout={VisibilityTimeout}", linkedCancellationToken.IsCancellationRequested, _options.WorkItemTimeout); | ||
|
|
||
|
|
@@ -177,41 +177,48 @@ protected override async Task<IQueueEntry<T>> DequeueImplAsync(CancellationToken | |
| message.MessageId, insertedOn, nowUtc, queueTime.TotalMilliseconds, linkedCancellationToken.IsCancellationRequested); | ||
| Interlocked.Increment(ref _dequeuedCount); | ||
|
|
||
| T data; | ||
| string correlationId = null; | ||
| IDictionary<string, string> properties = null; | ||
| T? data; | ||
| string? correlationId = null; | ||
| IDictionary<string, string>? properties = null; | ||
|
|
||
| try | ||
| { | ||
| if (_options.CompatibilityMode == AzureStorageQueueCompatibilityMode.Default) | ||
| { | ||
| try | ||
| { | ||
| // Unwrap envelope to extract metadata | ||
| var envelope = _serializer.Deserialize<QueueMessageEnvelope<T>>(message.Body.ToArray()); | ||
| data = envelope.Data; | ||
| correlationId = envelope.CorrelationId; | ||
| properties = envelope.Properties; | ||
| if (envelope is not null) | ||
| { | ||
|
niemyjski marked this conversation as resolved.
|
||
| data = envelope.Data; | ||
| correlationId = envelope.CorrelationId; | ||
| properties = envelope.Properties; | ||
| } | ||
| else | ||
| { | ||
| data = _serializer.Deserialize<T>(message.Body.ToArray()); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| // Fallback: try legacy format (raw T) for messages written before the envelope format. | ||
| // If this also fails, let the exception propagate to the outer catch which will dead-letter it. | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these comments are valid, we should not be removing them.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: Restored the removed comments (envelope unwrap, legacy fallback, legacy mode). |
||
| _logger.LogWarning(ex, "Failed to deserialize message {MessageId} as envelope format, attempting legacy format fallback", message.MessageId); | ||
| data = _serializer.Deserialize<T>(message.Body.ToArray()); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // Legacy mode: deserialize data directly (no envelope) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these comments are valid, we should not be removing them.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: Restored the removed comments. |
||
| data = _serializer.Deserialize<T>(message.Body.ToArray()); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogWarning(ex, "Error deserializing message {MessageId} (attempt {DequeueCount}), abandoning for retry", message.MessageId, message.DequeueCount); | ||
|
niemyjski marked this conversation as resolved.
Outdated
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not following our deserializeException pattern!!!!! FIX ALL THE PROVIDERS COME ON.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: Now follows the \deserializeException\ pattern matching SQS and Azure Service Bus providers — exception is captured outside the try block and passed to the log message when \data is null. |
||
| data = null; | ||
| } | ||
|
|
||
| var poisonEntry = new AzureStorageQueueEntry<T>(message, null, null, null, this); | ||
| if (data is null) | ||
| { | ||
| var poisonEntry = new AzureStorageQueueEntry<T>(message, null, null, default!, this); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WHY did this need to change, just pass null?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: Reverted to pass |
||
| await AbandonAsync(poisonEntry).AnyContext(); | ||
|
niemyjski marked this conversation as resolved.
Outdated
|
||
| return null; | ||
| } | ||
|
|
@@ -419,7 +426,7 @@ protected override void StartWorkingImpl(Func<IQueueEntry<T>, CancellationToken, | |
| { | ||
| _logger.LogTrace("WorkerLoop Signaled {QueueName}", _options.Name); | ||
|
|
||
| IQueueEntry<T> queueEntry = null; | ||
| IQueueEntry<T>? queueEntry = null; | ||
| try | ||
| { | ||
| queueEntry = await DequeueImplAsync(linkedCancellationToken.Token).AnyContext(); | ||
|
|
@@ -498,15 +505,15 @@ internal record QueueMessageEnvelope<T> where T : class | |
| /// <summary> | ||
| /// Correlation ID for distributed tracing | ||
| /// </summary> | ||
| public string CorrelationId { get; init; } | ||
| public string? CorrelationId { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// Custom properties/metadata | ||
| /// </summary> | ||
| public IDictionary<string, string> Properties { get; init; } | ||
| public IDictionary<string, string>? Properties { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// The actual message payload | ||
| /// </summary> | ||
| public T Data { get; init; } | ||
| public required T Data { get; init; } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,10 +58,10 @@ public AzureFileStorage(Builder<AzureFileStorageOptionsBuilder, AzureFileStorage | |
| public BlobContainerClient Container => _container; | ||
|
|
||
| [Obsolete($"Use {nameof(GetFileStreamAsync)} with {nameof(StreamMode)} instead to define read or write behavior of stream")] | ||
| public Task<Stream> GetFileStreamAsync(string path, CancellationToken cancellationToken = default) | ||
| public Task<Stream?> GetFileStreamAsync(string path, CancellationToken cancellationToken = default) | ||
| => GetFileStreamAsync(path, StreamMode.Read, cancellationToken); | ||
|
|
||
| public async Task<Stream> GetFileStreamAsync(string path, StreamMode streamMode, CancellationToken cancellationToken = default) | ||
| public async Task<Stream?> GetFileStreamAsync(string path, StreamMode streamMode, CancellationToken cancellationToken = default) | ||
| { | ||
| ArgumentException.ThrowIfNullOrEmpty(path); | ||
|
|
||
|
|
@@ -91,7 +91,7 @@ public async Task<Stream> GetFileStreamAsync(string path, StreamMode streamMode, | |
| } | ||
| } | ||
|
|
||
| public async Task<FileSpec> GetFileInfoAsync(string path) | ||
| public async Task<FileSpec?> GetFileInfoAsync(string path) | ||
| { | ||
| ArgumentException.ThrowIfNullOrEmpty(path); | ||
|
|
||
|
niemyjski marked this conversation as resolved.
|
||
|
|
@@ -255,7 +255,7 @@ public async Task<bool> DeleteFileAsync(string path, CancellationToken cancellat | |
| } | ||
| } | ||
|
|
||
| public async Task<int> DeleteFilesAsync(string searchPattern = null, CancellationToken cancellationToken = default) | ||
| public async Task<int> DeleteFilesAsync(string? searchPattern = null, CancellationToken cancellationToken = default) | ||
| { | ||
| var files = await GetFileListAsync(searchPattern, cancellationToken: cancellationToken).AnyContext(); | ||
| int count = 0; | ||
|
|
@@ -273,7 +273,7 @@ public async Task<int> DeleteFilesAsync(string searchPattern = null, Cancellatio | |
| return count; | ||
| } | ||
|
|
||
| public async Task<PagedFileListResult> GetPagedFileListAsync(int pageSize = 100, string searchPattern = null, CancellationToken cancellationToken = default) | ||
| public async Task<PagedFileListResult> GetPagedFileListAsync(int pageSize = 100, string? searchPattern = null, CancellationToken cancellationToken = default) | ||
| { | ||
| if (pageSize <= 0) | ||
| return PagedFileListResult.Empty; | ||
|
|
@@ -283,7 +283,7 @@ public async Task<PagedFileListResult> GetPagedFileListAsync(int pageSize = 100, | |
| return result; | ||
| } | ||
|
|
||
| private async Task<NextPageResult> GetFiles(string searchPattern, int page, int pageSize, CancellationToken cancellationToken) | ||
| private async Task<NextPageResult> GetFiles(string? searchPattern, int page, int pageSize, CancellationToken cancellationToken) | ||
| { | ||
| int pagingLimit = pageSize; | ||
| int skip = (page - 1) * pagingLimit; | ||
|
|
@@ -307,7 +307,7 @@ private async Task<NextPageResult> GetFiles(string searchPattern, int page, int | |
| }; | ||
| } | ||
|
|
||
| private async Task<List<FileSpec>> GetFileListAsync(string searchPattern = null, int? limit = null, int? skip = null, CancellationToken cancellationToken = default) | ||
| private async Task<List<FileSpec>> GetFileListAsync(string? searchPattern = null, int? limit = null, int? skip = null, CancellationToken cancellationToken = default) | ||
| { | ||
| if (limit is <= 0) | ||
| return new List<FileSpec>(); | ||
|
|
@@ -368,16 +368,16 @@ private static FileSpec ToFileInfo(string path, BlobProperties properties) | |
|
|
||
| private static string NormalizePath(string path) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should return and take a string?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: \NormalizePath\ now accepts \string?\ and returns \string?.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could be wrong here this might just be string and not string? as long as we are sure every caller of this has argument validation.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping \NormalizePath\ as \string? -> string?\ since it accurately reflects the null-in/null-out behavior. All callers that pass validated (non-null) paths already guard with \ArgumentException.ThrowIfNullOrEmpty\ before calling, so the return is non-null in those paths.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in e795f95. Changed \NormalizePath\ to take and return non-nullable \string. All callers already validate null via \ArgumentException.ThrowIfNullOrEmpty\ before calling, and the one \string?\ caller in \GetRequestCriteria\ is guarded by the \String.IsNullOrEmpty\ early return. Removed all !\ null-forgiving operators from call sites. |
||
| { | ||
| return path?.Replace('\\', '/'); | ||
| return path.Replace('\\', '/'); | ||
| } | ||
|
|
||
| private record SearchCriteria | ||
| { | ||
| public string Prefix { get; set; } | ||
| public Regex Pattern { get; set; } | ||
| public string Prefix { get; set; } = String.Empty; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. required no string.empty hacks
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: \SearchCriteria.Prefix\ now uses |
||
| public Regex? Pattern { get; set; } | ||
| } | ||
|
|
||
| private SearchCriteria GetRequestCriteria(string searchPattern) | ||
| private SearchCriteria GetRequestCriteria(string? searchPattern) | ||
| { | ||
| if (String.IsNullOrEmpty(searchPattern)) | ||
| return new SearchCriteria { Prefix = String.Empty }; | ||
|
|
@@ -387,7 +387,7 @@ private SearchCriteria GetRequestCriteria(string searchPattern) | |
| bool hasWildcard = wildcardPos >= 0; | ||
|
|
||
| string prefix = normalizedSearchPattern; | ||
| Regex patternRegex = null; | ||
| Regex? patternRegex = null; | ||
|
|
||
| if (hasWildcard) | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description mentions replacing
<WarningsAsErrors>with<TreatWarningsAsErrors>, but this repo doesn’t appear to contain<WarningsAsErrors>anywhere, andTreatWarningsAsErrorsis 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).