Do not throw on invalid ReplyTo address

[ig-sinicyn]

Proposed Changes

Do not throw ArgumentNullException from ReplyToAddress getter and PublicationAddress.Parse() call. See #827 for more details.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

• Bug fix (non-breaking change which fixes issue Rabbitmq 6.0, ReplyToAddress getter throws ArgumentNullException #827 )
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• Documentation improvements (corrections, new content, etc)
• Cosmetic change (whitespace, formatting, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories (no such changes)

Further Comments

I've decided to add fix into the PublicationAddress.Parse() method as ReplyToAddress documentation states

/// Returns null if ReplyTo property cannot be parsed by PublicationAddress.Parse

[bording]

I'm not sure it's a good idea to change the behavior of the Parse method like this. It's pretty standard practice in in the .NET space for Parse methods to throw for invalid input, and there also being a non-throwing TryParse method that returns a bool to indicate success or failure.

If non-throwing behavior is needed because this method is called internally, I think adding a TryParse method and then modifying the internal calls to use it instead is a better approach.

[ig-sinicyn]

@bording

Well, documentation states that the property "returns null if ReplyTo property cannot be parsed by PublicationAddress.Parse". So, behavior of var x = properties.ReplyToAddress must be in sync with var x = PublicationAddress.Parse(properties.ReplyTo).

Adding a new TryParse method will result in two methods with almost same behavior. The only difference is, Parse() throws on null arg while TryParse() does not.

[michaelklishin]

Thank you!

[michaelklishin]

Here's my thinking: the method in question is a getter and it would make more sense to return a null if it cannot compute its result.

[lukebakken]

My assumption is that this fix requires a 6.1.0 version.

[bording]

I'm still not sure this was the correct approach, and technically this is a breaking behavior change.

The actual code in this PR wasn't changing a property getter, it was changing a public method on the PublicationAddress class. The getter logic could be changed without altering the behavior of a separate public type.

That's why I was suggesting adding a TryParse method with the non-throwing behavior. The getter code could be updated to use that instead (which would then fix it based on the documented behavior), and then there would be no public behavior change to a separate, unrelated method.

And it also fits into the standard Parse/TryParse convention you see for .NET parsing methods.

[michaelklishin]

@ig-sinicyn would you be up for another PR that introduces a new TryParse method and switches the gtter to it?

[ig-sinicyn]

@michaelklishin
As it is for now, the only difference between Parse() and TryParse() methods will be null value handling. I'm not sure if it is worth to add a new method for such minor change.

As an option, the Parse() method logic may be changed to throw on parsing failures. Personally, I do not like the idea as it will be a breaking change.

[lukebakken]

I'm not sure if it is worth to add a new method for such minor change

As @bording points out here, Parse / TryParse is a very common idiom in .NET. We should do it here.

[ig-sinicyn]

Well, try-parse pattern is not recommended for all scenarios. See Framework Design Guidelines for more.

CONSIDER the TryParse pattern for members that may throw exceptions in common scenarios to avoid performance problems related to exceptions.
...
DO provide an exception-throwing member for each member using the Try-Parse Pattern.

(c) FDG

Currently the Parse() method does not throw exceptions for invalid input value, and therefore have no performance penalty. So, the try-parse pattern is not applicable here.

[ig-sinicyn]

P.S. Note the CONSIDER tag. In FDG terms, it means that recommendation is not a mandatory thing and should be used with caution.

[michaelklishin]

It's great that there is an entire book written on .NET patterns and idioms but what the book says and what the community does can and likely will diverge over time. I'll tweak #828 to do what @bording suggests because introducing a new method is fairly trivial. The whole feature is obscure and rarely used but I'd rather do what prominent contributors suggest than spend mental energy on arguing about idioms. Either option is fine with me but one is safer.