Populate originator property on saga creation

[timbussmann]

Fixes #426 by ensuring that the Saga.Originator property is always assigned.

Note that this slightly deviates from NServiceBus.Core which doesn't populate the property when there is no ReplyTo header on the message starting a saga. Given this header is almost always sent when sending messages with NServiceBus, and the testable handler context has a default value for the context.ReplyTo property, using a default value seems valid. Not setting the value when the header is missing might also be a valid alternative to have a more accurate simulation though.

[DavidBoike]

Setting the values in the ReplyMessage is only useful for the asserts, and not required to make ReplyToOriginator work in this case, correct?

[timbussmann]

yes, I've added that to have a simple way to verify the value of Data.Originator outside the usage of ReplyToOriginator. That property is also publicly accessible.

[danielmarbach]

Just a question out of curiousity. Why didn't you implement it this way?

Not setting the value when the header is missing might also be a valid alternative to have a more accurate simulation though.

Given the TestableSaga is about simulating saga behavior, that would sound like the more logical conclusion to me unless I misunderstood the design of TestableSaga (which is very likely since I wasn't involved)

[timbussmann]

@danielmarbach there's an unfortunate duality of the replyToAddress. The NServiceBus code seems to retrieve the value from the headers but the pipeline context also contains a ReplyToAddress property (see https://github.com/Particular/NServiceBus/blob/master/src/NServiceBus.Core/IMessageProcessingContext.cs#L20) which is filled by core based on the replyTo header value when invoking the pipeline. But this property is also always set to a default value in the testable contexts so it's not reflecting the header values of the incoming message correctly.

Now it might seem perfectly valid for a user to configure the property on the context and expect that to influence the originator of the saga data (or other replyto related code) while simulating the core behavior it actually wouldn't. If better "simulation" is preferred, we shouldn't set that property value by default IMO (actually it looks like we should completely remove that property from the context).

Does that thought process make sense?

[danielmarbach]

If I understood you correctly, then essentially the existence of the header is the "driver" for the property to be set. So in all cases when the header is not present, the property would be empty. So in that sense I guess if we would aim for simulation then the property should by default be empty unless such a header is around, would you agree under the assumption we strive for "simulating" the core behavior?

The question is though, I guess, do we want to simulate the core behavior?

[DavidBoike]

We don't necessarily want to simulate core exactly. A scenario test is never going to comprise multiple endpoints. All the handlers get called right there in memory. So the mechanics of routing largely don't apply, yet have to just work as far as any important assertions that would be made.

[danielmarbach]

I like that because that means having a default value on the Reply Address is good and better than having to go through "oh yeah when I set the header it will be there otherwise it won't" make the test setup way more complicated while all you potentially want to do is make sure some of your code that happens to read the reply address has a value around.

[timbussmann]

Thanks for the discussion and thoughts. I've also looked at changing the default implementation of the ReplyToAddress property in the TestableMessageProcessingContext class to be something like this:

public string ReplyToAddress => MessageHeaders.TryGetValue(Headers.ReplyToAddress, out var replyAddress) ? replyAddress : "reply address";

this would also be perfectly valid IMO but it comes with the downside that there are possible scenarios where the property would actually be null in core, in case the header wouldn't be set on an incoming message (e.g. send-only endpoints or third party integration scenarios). With the approach mentioned above, this wouldn't be reproducible anymore. So I think there are two reasonable approaches:

• Replicating core behavior and not setting the value, forcing users to provide the actual header value if they really depend on it (also a potentially breaking change for the testing library)
• Keep the behavior suggested in this PR which provides some default value for more convenient testing but allows full control depending on what their code needs (using the headers directly or using the context property)

Happy to further discuss in case you feel we should change the approach.