Access data of incoming message over the context only

[danielmarbach]

When doing the API walkthrough with @indualagarsamy @mikeminutillo and @udidahan, we detected that we have inconsistency in how we expose data of the incoming message over the context. Here are a few examples

    public class TransportReceivePhase : Behavior<ITransportReceiveContext>
    {
        public override async Task Invoke(ITransportReceiveContext context, Func<Task> next)
        {
            var headers = context.Message.Headers;
            var messageId = context.Message.MessageId;
            var body = context.Message.Body;
            // the only way to access the incoming message
        }
    }

    public class IncomingPhysicalMessagePhase : Behavior<IIncomingPhysicalMessageContext>
    {
        public override async Task Invoke(IIncomingPhysicalMessageContext context, Func<Task> next)
        {
            var headers = context.Message.Headers;
            var messageId = context.Message.MessageId;
            var body = context.Message.Body;

            messageId = context.MessageId;
            headers = context.MessageHeaders;
            // Two ways of accessing the same data, magically works by reference passing for headers
        }
    }

    public class IncomingLogicalMessagePhase : Behavior<IIncomingLogicalMessageContext>
    {
        public override async Task Invoke(IIncomingLogicalMessageContext context, Func<Task> next)
        {
            // Here is is now the logical message
            var logicalMessage = context.Message;

            // Two ways to access the headers
            var headers = context.Headers;
            var readonlyHeaders = context.MessageHeaders;
            var messageId = context.MessageId;
        }
    }

The API is not concise and clear because we have multiple ways of accessing the message id and the headers. There are two ways we can achieve this:

• Always expose .Message on the context and only access data over that property, or
• Expose everything directly on the context and remove .Message

Since the context is already the advanced object containing the strongly typed data for a given stage, I'd say we should just expose the necessary data on the context directly. So the code would only allow to do the following:

        var messageId = context.MessageId;
        var headers = context.Headers;
        var body = context.Body;

based on the stage. Better clarity and discoverability of the API. We would no longer confuse the users with two ways of achieving the same thing and therefore, remove questions like

Should I access and modify the headers over the context.Message instance or context.Headers or even both?

for our users.

Questions

• On the outgoing pipeline should we just expose methods to access the incoming message headers or a tuple of MessageId and Headers?
• RevertToOriginalBodyIfNeeded needs to be redundantly implemented both on transport receive and incoming physical. Is that OK?
• I still don't like that we have MessageHeaders and Headers exposed inside the pipeline only for the purpose to make the user access to headers expose IReadonlyDictionary. I'm contemplating to streamline it and just expose Dictionary<string, string> but make a copy of the headers. Thoughts?
• CustomRetryPolicy would expose an ITransportReceiveContext instead of IncomingMessage. Ok?
• Should we still keep around IncomingMessage as an internal class to wrap the body and reverting of the body? This might reduce code redundancy on the contexts described above. Thoughts?

  Outgoing part

https://github.com/Particular/NServiceBus/issues/3535

Spike

API exploration impact

https://github.com/Particular/NServiceBus/pull/3529

[danielmarbach]

@Particular/nservicebus-maintainers would like to flag this as Core V6 and get the squad to approve it. Thoughts?

[andreasohlund]

:+1: to fix this

[timbussmann]

:+1:

[danielmarbach]

@Particular/platform-dev-squad I would like to flag this V6

[andreasohlund]

I think the change is good but since this touch the API only exposed to advanced users I propose we focus on

https://github.com/Particular/NServiceBus/pull/3355

and

https://github.com/Particular/PlatformDevelopment/issues/621

and look at this one once we have a decision on the above? (since they improve the API exposed to all users and therefor have much bigger impact)

[danielmarbach]

:+1:

On Tuesday, March 8, 2016, Andreas Öhlund notifications@github.com wrote:

I think the change is good but since this touch the API only exposed to advanced users I propose we focus on

3355 https://github.com/Particular/NServiceBus/pull/3355

and

Particular/PlatformDevelopment#621 https://github.com/Particular/PlatformDevelopment/issues/621

and look at this one once we have a decision on the above? (since they improve the API exposed to all users and therefor have much bigger impact)

— Reply to this email directly or view it on GitHub https://github.com/Particular/NServiceBus/issues/3534#issuecomment-193692374 .

Don't miss the Async/Await Webinar Series • Learn more http://go.particular.net/l/27442/2016-02-15/681bjl →

[danielmarbach]

Talked to @timbussmann and @andreasohlund today and we came to the conclusion that we cannot justify the value of this change since it really only affects a very small portion of users. We will live with this inconsistency and deal with it in a future release.