Make failingendpoint address non-nullable to match usage - #5822
Conversation
|
|
||
| public Dictionary<string, object> MessageMetadata { get; set; } | ||
| public FailureDetails FailureDetails { get; set; } = new(); | ||
| public FailureDetails FailureDetails { get; set; } = new() { AddressOfFailingEndpoint = string.Empty }; |
There was a problem hiding this comment.
Should FailureDetails rather be made required and not initialise the failing endpoint to an empty string? This sorta keeps it honest then but not sure what the downstream effect is
There was a problem hiding this comment.
The blast radius of making that property required kinda sucks!
because the failed message attempts are synthesized they don't actually have failuredetails, we only actually consume the last one (so that one works correctly) but the model still contains a list.
It's a much bigger change to get rid of that and requires a bunch of Raven changes, or it requires us storing all the historical failures in EF persisters which is also quite a change.
There was a problem hiding this comment.
Is there a consequence of going from a null address to an empty address? Im just thinking if something is checking for a null in a query, but now its an empty string instead.
Problem
FailingEndpointAddress(EF entity) andAddressOfFailingEndpointwere markedstring?, but the ingestion path (FailedMessageFactory.ParseFailureDetails)already guarantees a non-null value — it throws if the NServiceBus.FailedQ header is missing. The nullable annotation was propagated from the mutable FailureDetails DTO rather than
reflecting actual usage, leading to a nullable column, a dedicated NullableEndpointAddress migration, and defensive != null guards scattered across the EF query layer.
Changes
Made the types non-nullable with required:
FailureDetails.AddressOfFailingEndpoint→required string(wasstring?)FailedMessageEntity.FailingEndpointAddress→ required string (wasstring?)RecordedFailedProcessingAttempt.FailingEndpointAddress→required string(wasstring?)StagingMessage.FailingEndpointAddress→stringrecord parameter (wasstring?)Refactored
FailedMessageFactory.ParseFailureDetailsto check the FailedQ header before constructing, using an object initializer to satisfy required.Updated
FailedMessage.ProcessingAttempt.FailureDetailsdefault from= new()to= new() { AddressOfFailingEndpoint = string.Empty }so placeholder attempts satisfy the requiredmember.
Removed defensive null checks that existed only because of the nullable:
FailedMessageQueryFilters.FilterByQueueAddress— droppedFailingEndpointAddress != null &&guardFailedMessageRetryDataStore.GetRetryPendingMessages— replaced two-branch null match with simple comparisonAdded EF migration MakeFailingEndpointAddressNonNull (SQL Server + PostgreSQL) to make the column NOT NULL with defaultValue: "" to safely handle any stray nulls. Updated model
snapshots and designer files.
Fixed 5 test files that constructed
FailureDetailswithout settingAddressOfFailingEndpoint.What's not affected