fixed cloning by utilizing Service Bus .Clone(stream) instead of .Clone(string)

[c-lombardi]

…one() for Streams

Regarding:
#245

This appears to be everything necessary in order to resolve the issue I was having.

I am uncertain of all functionality of ServiceBusExplorer to QA this change, but this is basically what I found will meet the needs of properly cloning a stream.

[SeanFeldman]

✅ Build ServiceBusExplorer 4.1.0-PullRequest0247.264.build.459 completed (commit bf3ecabd98 by @)

[SeanFeldman]

✅ Build ServiceBusExplorer 4.1.0-PullRequest0247.265.build.460 completed (commit 299dfacc86 by @)

[c-lombardi]

Although I think .Clone() is simpler to utilize, I implemented this with the .Clone(Stream) static helper function as per @paolosalvatori 's recommendation in: fixes #245

[SeanFeldman]

✅ Build ServiceBusExplorer 4.1.0-PullRequest0247.266.build.461 completed (commit 7376945d2f by @)

[SeanFeldman]

✅ Build ServiceBusExplorer 4.1.0-PullRequest0247.267.build.462 completed (commit 85c51de352 by @)

[ErikMogensen]

Hi @c-lombardi, have you tested this with other message formats too, such non-WCF Stream, ByteArray and String?

[SeanFeldman]

The more I think about it the more I'm convinced we should have test cases to cover this surface.
That way, if changes @c-lombardi is proposing are causing a regression, he'd know w/o us interfering in his PR 🙂

@c-lombardi while it's not part of your work, would you like to take a stab at it?

[c-lombardi]

I was itching to write some tests.

I'd be happy to write a few to lock-down these changes when I can find some more time. It took me almost a week to get back to this for just the few lines of changes I have made.

There is a lot of mutability going on in this class, including 7 tiers deep of inheritance and a 62 cyclomatic complexity rating (metrics calculated by Visual Studio).

I'm not certain about the non-WCF, ByteArray or String implementations. The application that I develop at my day-job utilizes ServiceBus only with Streams and that's where this became a problem for us to re-queue dead-lettered messages.

Your tool is great by the way. It was really what our Devs, QAs, and Operations teams needed to gain valuable insight into our use of Service Bus.

Until now, it's been more of a theoretical black-box.

Thanks for developing it and for responding so quickly to all of my comments and concerns!

I'll update this PR with the tests when I can.

[paolosalvatori]

Hi @c-lombardi thanks for the fix. Have you tested your code with all message formats? The reason why a few years ago I created an extension to clone the message it's because the original Clone method did not clone the body of the message, but only its properties. I don't know if the implementation of the BrokeredMessage.Clone() has changed in the meantime.

[c-lombardi]

That makes a lot of sense, thanks for explaining your perspective. The BrokeredMessage's Clone function does include the message's body into the cloned object.

I have not tested with other message formats. The scope of my change was meant to only support the Stream message format that was not working.

[paolosalvatori]

@c-lombardi please try out all the formats and if you confirm that this fixes the issue without impacting the other formats, I'll approve the pull request, thanks!

[SeanFeldman]

@paolosalvatori I've tried it and it works.

Would you go and merge a PR based on what I've said?

Hope you haven't done it yet and got to this line. Because I'm a human and could make a mistake 😉
So it @c-lombardi. Hence my comment here: #247 (comment).

[paolosalvatori]

Thanks @SeanFeldman, of course I trust you of course, should I merge the PR? ;)

[SeanFeldman]

We need to have some tests/verification prior to merging this. Ideally automated.
Adding blocked until resolved.

[rhythmnewt]

Can someone take a look at this PR and merge it? It solves my problem with resubmitting messages with binary message body (custom compressed stream).

[SeanFeldman]

@rhythmnewt, not to be disrespectful but everyone is working full time that is not this tool. The change needs to be validated and, ideally, have a test. The author (@c-lombardi) did not have a chance to test all the formats or commit a test. If you do have time and this change is important to you, would you be interested in contributing?

[paolosalvatori]

@c-lombardi @SeanFeldman @ErikMogensen did you have the chance to try out the various formats? Can I proceed and approve this PR? Please let me know asap, I usually have time once a week to review issues and PRs. Before approving and merging the PR I want to be 100% sure that this does not introduce any regression bug in the tool thanks!

[ErikMogensen]

Paolo, as far as know no one has tested this. I do think GetBody() is the right way to handle it. I heard someone had problems downloading messages with Korean text.

[paolosalvatori]

@SeanFeldman @ErikMogensen I approved. I need a second approver