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

Ensuring "Path" returns the right destination in the case of via-sender #6941

Merged
merged 3 commits into from
Jul 17, 2019

Conversation

nemakam
Copy link
Contributor

@nemakam nemakam commented Jul 16, 2019

Fixing #6031
In the case of Via-Sender defined by
viaSender = new MessageSender(connection, "path", "via"),

Assert.Equal("path", viaSender.Path);
Assert.Equal("path", viaSender.TransferDestinationPath);
Assert.Equal("via", viaSender.ViaEntityPath);

EDIT
Alternatively, to keep things from breaking and causing further confusion, keeping Path as-is and updating the xmldoc.

  1. Path will now point to wherever the amqp-link is created to.
  2. viaEntityPath will point to via entity.
  3. TransferDestinationPath will point to the final destination of the message

@nemakam nemakam self-assigned this Jul 16, 2019
…sion, keeping Path as-is and updating the xmldoc.

1. Path will now point to wherever the amqp-link is created to.
2. viaEntityPath will point to via entity.
3. TransferDestinationPath will point to the final destination of the message
@nemakam nemakam merged commit 9d63b5a into Azure:master Jul 17, 2019
@nemakam nemakam deleted the sendviapath branch July 17, 2019 00:40
@SeanFeldman
Copy link
Contributor

With this change, when send-via is used, TransferDestinationPath is the real "path" and Path with SendViaPath are the same. Not sure this is less confusing. Personally, I find that confusing enough to warrant a change in a new major.

This would need to be released as a minor given a new property news added.

@nemakam
Copy link
Contributor Author

nemakam commented Jul 17, 2019

@SeanFeldman, yes, this will be a new minor. Will take care of that during release time.
But regarding the output of Path, it becomes confusing either way. I ran by this scenario with multiple teammates of mine, and had mixed opinions on what the real value of Path should be. Is it the entity where the message is being sent to (i.e., the target of the link) or the actual message itself? Lets say there was another feature where you could just specify the real desitnation of the message within the message.To field and just use the amqp-link to send it to a temporary routing path. How does it look like then? Same AMQP link can be used to send to multiple entities. You cannot change Path based on different destinations. And, also, in the case of send-via, its best if users understand that the message is in fact being sent to the "via" entity right now and lazily being forwarded to "destinationEntity". Having the Path variable point to "via" does make sense in that way.
The cherry on top of it is that in the older client, people had the same discussion and decided to stick to Path being "via", and if the new client has a different behavior, that may break users unknowingly who are shifting from old client to new client.
In short, any value of Path would be confusing. Hence the new variable called ViaEntity and TransferDestination which should keep things clear.

@SeanFeldman
Copy link
Contributor

The cherry on top predates my participation 😉
Would be good to see doco PRs linked to this issues/PR to ensure it's documented when minor is going out.
/cc @jfggdl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants