Skip to content
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

Access data of outgoing message over the context only. #3535

Open
danielmarbach opened this issue Mar 1, 2016 · 9 comments
Open

Access data of outgoing message over the context only. #3535

danielmarbach opened this issue Mar 1, 2016 · 9 comments

Comments

@danielmarbach
Copy link
Contributor

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

 public class OutgoingPhysicalPhase : Behavior<IOutgoingPhysicalMessageContext>
    {
        public override async Task Invoke(IOutgoingPhysicalMessageContext context, Func<Task> next)
        {
            var body = context.Body;
            var headers = context.Headers;
            var messageId = context.MessageId;
            // The only way to access the data
        }
    }

    public class OutgoingLogicalPhase : Behavior<IOutgoingLogicalMessageContext>
    {
        public override async Task Invoke(IOutgoingLogicalMessageContext context, Func<Task> next)
        {
            var logicalMessage = context.Message;

            var headers = context.Headers;
            var messageId = context.MessageId;
            // The only way to access the data
        }
    }


    public class InvokeHandlerContet : Behavior<IInvokeHandlerContext>
    {
        public override async Task Invoke(IInvokeHandlerContext context, Func<Task> next)
        {
            // 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 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.

Incoming part

#3534

@danielmarbach
Copy link
Contributor Author

This is less convoluted than the incoming part. But still @Particular/nservicebus-maintainers would like to flag this as Core V6 and get the squad to approve it. Thoughts?

@danielmarbach
Copy link
Contributor Author

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

@andreasohlund
Copy link
Member

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

#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
Copy link
Contributor Author

👍

On Tuesday, March 8, 2016, Andreas Öhlund [email protected] wrote:

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

#3355 #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
#3535 (comment)
.

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

@adamralph
Copy link
Contributor

if API changes are being proposed, then this cannot be a refactoring.

@timbussmann
Copy link
Contributor

if API changes are being proposed, then this cannot be a refactoring

@adamralph if the functionality stays the same, why can't it be refactoring? Is that label and it's meaning documented somewhere?

@adamralph
Copy link
Contributor

@timbussmann the label is defined at https://github.com/Particular/PlatformDevelopment/blob/master/IssueLabels.md#type-refactoring

An API change is something users will see so I would say it has to be relevant to them, and would have to form part of the release notes.

@timbussmann
Copy link
Contributor

@adamralph thanks for the link 👍 Since the whole pipeline changed drastically from v5, there isn't really an "API" change which would affect users other than it already did before. But since those label don't really imply anything right now, I don't really mind.

@danielmarbach
Copy link
Contributor Author

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.

@andreasohlund andreasohlund modified the milestone: Future Oct 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants