Skip to content

Commit a8ccf2c

Browse files
DevJonnyclaude
andcommitted
test: address review feedback — drop Fragile trait, document converter contract, pin fallback
Three issues from claude[bot]'s latest review: 1. The new ASB unit test carried [Trait("Fragile", "CI")], copied by mistake from the live-ASB integration tests. The CI pipeline uses "Fragile!=CI" as its filter, so the trait skipped the only transport-level pin for local-header filtering. Drop it. 2. GetProducerLookupTopic uses `producerTopic is string topic`. Concern was that persistent outboxes round-trip Header.Bag through JSON, so values would come back as JsonElement and the cast would silently fail — reproducing the bug the PR fixes. Investigation shows Brighter's bag round-trip uses JsonSerialisationOptions.Options, which composes DictionaryStringObjectJsonConverter with ObjectToInferredTypesConverter to preserve string runtime types. Verified RelationDatabaseOutbox (covers MsSql, PostgreSQL, MySql, Sqlite, Spanner), MongoDb, and DynamoDB all use those options on deserialise. Document the contract on GetProducerLookupTopic and add BagStringValueRoundTripTests to pin the converter behaviour. If a future change drops one of the converters, the round-trip test fails loudly instead of GetProducerLookupTopic regressing silently. 3. Add WrapMatchingPublicationTopicTests to pin the fallback path: when a mapper does NOT override Header.Topic, no ProducerTopic bag entry is written and producer lookup falls back to Header.Topic. The existing test trio (reply-mapper-overrides, null-publication-topic, matching-topic) now covers all three branches of WrapPipeline.Wrap. Co-Authored-By: Claude (claude-opus-4-7) <noreply@anthropic.com>
1 parent 53996d4 commit a8ccf2c

4 files changed

Lines changed: 87 additions & 1 deletion

File tree

src/Paramore.Brighter/OutboxProducerMediator.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -781,6 +781,14 @@ private static RoutingKey GetProducerLookupTopic(Message message)
781781
// dynamic reply address. Normal publications don't carry the bag entry, so we
782782
// fall back to Header.Topic — and a null/empty Header.Topic remains a lookup
783783
// failure, matching pre-fix behaviour.
784+
//
785+
// The `is string` cast is safe across persistent outboxes (SQL family,
786+
// Mongo, DynamoDB) because Brighter's bag round-trip uses
787+
// JsonSerialisationOptions.Options, which registers DictionaryStringObjectJsonConverter
788+
// + ObjectToInferredTypesConverter — together they preserve the string runtime
789+
// type through serialise/deserialise rather than handing back JsonElement.
790+
// See When_Bag_String_Values_Round_Trip_Through_Brighter_Json_Options for
791+
// a regression pin on that contract.
784792
if (message.Header.Bag.TryGetValue(Message.ProducerTopicHeaderName, out var producerTopic)
785793
&& producerTopic is string topic)
786794
{

tests/Paramore.Brighter.AzureServiceBus.Tests/MessagingGateway/When_Converting_A_Message_With_Local_Headers.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
namespace Paramore.Brighter.AzureServiceBus.Tests.MessagingGateway;
66

77
[Trait("Category", "ASB")]
8-
[Trait("Fragile", "CI")]
98
public class AzureServiceBusMessagePublisherLocalHeaderTests
109
{
1110
[Fact]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
using System.Collections.Generic;
2+
using System.Text.Json;
3+
using Paramore.Brighter.JsonConverters;
4+
using Xunit;
5+
6+
namespace Paramore.Brighter.Core.Tests.MessageSerialisation;
7+
8+
// Regression pin for the contract that OutboxProducerMediator.GetProducerLookupTopic
9+
// relies on: when Header.Bag is round-tripped through Brighter's JsonSerialisationOptions
10+
// the values come back as their runtime types (string here), NOT as JsonElement.
11+
// If a future change drops DictionaryStringObjectJsonConverter or
12+
// ObjectToInferredTypesConverter from the options, the `producerTopic is string` cast in
13+
// GetProducerLookupTopic would silently fail for persistent outboxes (SQL, Mongo,
14+
// DynamoDB) and reply-message dispatch would regress to the bug this PR fixes.
15+
public class BagStringValueRoundTripTests
16+
{
17+
[Fact]
18+
public void When_Round_Tripping_A_Bag_String_Value_It_Survives_As_String_Not_JsonElement()
19+
{
20+
var bag = new Dictionary<string, object>
21+
{
22+
[Message.ProducerTopicHeaderName] = "the.publication.topic",
23+
["customer.header"] = "customer.value"
24+
};
25+
26+
var json = JsonSerializer.Serialize(bag, JsonSerialisationOptions.Options);
27+
var roundTripped = JsonSerializer.Deserialize<Dictionary<string, object>>(json, JsonSerialisationOptions.Options);
28+
29+
Assert.NotNull(roundTripped);
30+
Assert.IsType<string>(roundTripped![Message.ProducerTopicHeaderName]);
31+
Assert.Equal("the.publication.topic", roundTripped[Message.ProducerTopicHeaderName]);
32+
Assert.IsType<string>(roundTripped["customer.header"]);
33+
}
34+
}
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
using Paramore.Brighter.Core.Tests.CommandProcessors.TestDoubles;
2+
using Xunit;
3+
4+
namespace Paramore.Brighter.Core.Tests.MessageSerialisation;
5+
6+
// Fallback path for OutboxProducerMediator.GetProducerLookupTopic: when a mapper does
7+
// NOT override Header.Topic (the normal-publication case), no ProducerTopic bag entry
8+
// is written. Producer lookup then falls back to Header.Topic. Without this pin, a
9+
// future change that always wrote the bag entry would leave persistent outbox rows
10+
// across a rolling upgrade with stale producer hints — exactly the failure mode the
11+
// pre-fix bug had, just inverted.
12+
public class WrapMatchingPublicationTopicTests
13+
{
14+
[Fact]
15+
public void When_The_Mapper_Topic_Matches_The_Publication_No_Producer_Topic_Bag_Entry_Is_Written()
16+
{
17+
TransformPipelineBuilder.ClearPipelineCache();
18+
19+
var mapperRegistry = new MessageMapperRegistry(
20+
new SimpleMessageMapperFactory(_ => new MyCommandMessageMapper()),
21+
null);
22+
mapperRegistry.Register<MyCommand, MyCommandMessageMapper>();
23+
24+
var publicationTopic = new RoutingKey("normal.publication.topic");
25+
var publication = new Publication
26+
{
27+
Topic = publicationTopic,
28+
RequestType = typeof(MyCommand)
29+
};
30+
31+
var pipelineBuilder = new TransformPipelineBuilder(
32+
mapperRegistry,
33+
new SimpleMessageTransformerFactory(_ => null));
34+
35+
var message = pipelineBuilder
36+
.BuildWrapPipeline<MyCommand>()
37+
.Wrap(new MyCommand(), new RequestContext(), publication);
38+
39+
// mapper produced a topic identical to publication.Topic — fallback path
40+
Assert.Equal(publicationTopic, message.Header.Topic);
41+
42+
// no bag entry → producer lookup falls back to Header.Topic
43+
Assert.False(message.Header.Bag.ContainsKey(Message.ProducerTopicHeaderName));
44+
}
45+
}

0 commit comments

Comments
 (0)