From 4944dda7fb992dd1a251557c86c28988a04343f9 Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Mon, 11 May 2020 19:12:39 +0300 Subject: [PATCH 1/9] Revert "Revert "Merge pull request #828 from ig-sinicyn/feature/message-properties-do-not-throw"" This reverts commit ed658549962a5aa984bf99a75e10d44aea4658a1. See the discussion in #827. We will slightly tweak the approach but at the very least the test added by @ig-sinicyn can be retained. --- .../client/api/PublicationAddress.cs | 8 +++++ projects/Unit/TestBasicProperties.cs | 31 +++++++++++++++++++ projects/Unit/TestPropertiesClone.cs | 17 +++++----- projects/Unit/TestPublicationAddress.cs | 1 + 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/projects/RabbitMQ.Client/client/api/PublicationAddress.cs b/projects/RabbitMQ.Client/client/api/PublicationAddress.cs index 60fdbd0c80..6778809a1a 100644 --- a/projects/RabbitMQ.Client/client/api/PublicationAddress.cs +++ b/projects/RabbitMQ.Client/client/api/PublicationAddress.cs @@ -106,6 +106,14 @@ public PublicationAddress(string exchangeType, string exchangeName, string routi /// public static PublicationAddress Parse(string uriLikeString) { + // Callers such as IBasicProperties.ReplyToAddress + // expect null result for invalid input. + // The regex.Match() throws on null arguments so we perform explicit check here + if (uriLikeString == null) + { + return null; + } + Match match = PSEUDO_URI_PARSER.Match(uriLikeString); if (match.Success) { diff --git a/projects/Unit/TestBasicProperties.cs b/projects/Unit/TestBasicProperties.cs index 3b7da08dc1..c206c799ed 100644 --- a/projects/Unit/TestBasicProperties.cs +++ b/projects/Unit/TestBasicProperties.cs @@ -120,5 +120,36 @@ public void TestNullableProperties_CanWrite( Assert.AreEqual(isCorrelationIdPresent, propertiesFromStream.IsCorrelationIdPresent()); Assert.AreEqual(isMessageIdPresent, propertiesFromStream.IsMessageIdPresent()); } + + [Test] + public void TestProperties_ReplyTo( + [Values(null, "foo_1", "fanout://name/key")] string replyTo + ) + { + // Arrange + var subject = new Framing.BasicProperties + { + + // Act + ReplyTo = replyTo, + }; + + // Assert + bool isReplyToPresent = replyTo != null; + string replyToAddress = PublicationAddress.Parse(replyTo)?.ToString(); + Assert.AreEqual(isReplyToPresent, subject.IsReplyToPresent()); + + var writer = new Impl.ContentHeaderPropertyWriter(new byte[1024]); + subject.WritePropertiesTo(ref writer); + + // Read from Stream + var propertiesFromStream = new Framing.BasicProperties(); + var reader = new Impl.ContentHeaderPropertyReader(writer.Memory.Slice(0, writer.Offset)); + propertiesFromStream.ReadPropertiesFrom(ref reader); + + Assert.AreEqual(replyTo, propertiesFromStream.ReplyTo); + Assert.AreEqual(isReplyToPresent, propertiesFromStream.IsReplyToPresent()); + Assert.AreEqual(replyToAddress, propertiesFromStream.ReplyToAddress?.ToString()); + } } } diff --git a/projects/Unit/TestPropertiesClone.cs b/projects/Unit/TestPropertiesClone.cs index f5827b5a0e..06be6fffea 100644 --- a/projects/Unit/TestPropertiesClone.cs +++ b/projects/Unit/TestPropertiesClone.cs @@ -68,10 +68,10 @@ private void TestBasicPropertiesClone(BasicProperties bp) bp.ContentType = "foo_1"; bp.ContentEncoding = "foo_2"; bp.Headers = new Dictionary - { - { "foo_3", "foo_4" }, - { "foo_5", "foo_6" } - }; + { + { "foo_3", "foo_4" }, + { "foo_5", "foo_6" } + }; bp.DeliveryMode = 2; // Persistent also changes DeliveryMode's value to 2 bp.Persistent = true; @@ -123,6 +123,7 @@ private void TestBasicPropertiesClone(BasicProperties bp) Assert.AreEqual(12, bpClone.Priority); Assert.AreEqual("foo_7", bpClone.CorrelationId); Assert.AreEqual("foo_8", bpClone.ReplyTo); + Assert.AreEqual(null, bpClone.ReplyToAddress); Assert.AreEqual("foo_9", bpClone.Expiration); Assert.AreEqual("foo_10", bpClone.MessageId); Assert.AreEqual(new AmqpTimestamp(123), bpClone.Timestamp); @@ -141,10 +142,10 @@ private void TestBasicPropertiesNoneClone(BasicProperties bp) bp.ContentType = "foo_1"; bp.ContentEncoding = "foo_2"; bp.Headers = new Dictionary - { - { "foo_3", "foo_4" }, - { "foo_5", "foo_6" } - }; + { + { "foo_3", "foo_4" }, + { "foo_5", "foo_6" } + }; bp.DeliveryMode = 2; // Persistent also changes DeliveryMode's value to 2 bp.Persistent = true; diff --git a/projects/Unit/TestPublicationAddress.cs b/projects/Unit/TestPublicationAddress.cs index 0d9b7ca924..7e9661e3e1 100644 --- a/projects/Unit/TestPublicationAddress.cs +++ b/projects/Unit/TestPublicationAddress.cs @@ -59,6 +59,7 @@ public void TestParseOk() [Test] public void TestParseFail() { + Assert.IsNull(PublicationAddress.Parse(null)); Assert.IsNull(PublicationAddress.Parse("not a valid uri")); } From 470dba7304a45f92b5bfb5ff2e8e6bb1a7345000 Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Mon, 11 May 2020 19:34:20 +0300 Subject: [PATCH 2/9] Amend #828 to introduce a new TryParse method instead of changing the behavior of an existing one. Per discussion with @bording and @ig-sinicyn in #827. --- .../client/api/PublicationAddress.cs | 24 ++++++++++++------- .../client/impl/BasicProperties.cs | 2 +- projects/Unit/TestPublicationAddress.cs | 19 ++++++++++++--- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/projects/RabbitMQ.Client/client/api/PublicationAddress.cs b/projects/RabbitMQ.Client/client/api/PublicationAddress.cs index 6778809a1a..0befcf620f 100644 --- a/projects/RabbitMQ.Client/client/api/PublicationAddress.cs +++ b/projects/RabbitMQ.Client/client/api/PublicationAddress.cs @@ -106,14 +106,6 @@ public PublicationAddress(string exchangeType, string exchangeName, string routi /// public static PublicationAddress Parse(string uriLikeString) { - // Callers such as IBasicProperties.ReplyToAddress - // expect null result for invalid input. - // The regex.Match() throws on null arguments so we perform explicit check here - if (uriLikeString == null) - { - return null; - } - Match match = PSEUDO_URI_PARSER.Match(uriLikeString); if (match.Success) { @@ -124,6 +116,22 @@ public static PublicationAddress Parse(string uriLikeString) return null; } + public static PublicationAddress TryParse(string uriLikeString) { + // Callers such as IBasicProperties.ReplyToAddress + // expect null result for invalid input. + // The regex.Match() throws on null arguments so we perform explicit check here + if (uriLikeString == null) + { + return null; + } else { + try { + return Parse(uriLikeString); + } catch { + return null; + } + } + } + /// /// Reconstruct the "uri" from its constituents. /// diff --git a/projects/RabbitMQ.Client/client/impl/BasicProperties.cs b/projects/RabbitMQ.Client/client/impl/BasicProperties.cs index b824801b83..a8946397f7 100644 --- a/projects/RabbitMQ.Client/client/impl/BasicProperties.cs +++ b/projects/RabbitMQ.Client/client/impl/BasicProperties.cs @@ -115,7 +115,7 @@ public bool Persistent /// public PublicationAddress ReplyToAddress { - get { return PublicationAddress.Parse(ReplyTo); } + get { return PublicationAddress.TryParse(ReplyTo); } set { ReplyTo = value.ToString(); } } diff --git a/projects/Unit/TestPublicationAddress.cs b/projects/Unit/TestPublicationAddress.cs index 7e9661e3e1..e1b4683155 100644 --- a/projects/Unit/TestPublicationAddress.cs +++ b/projects/Unit/TestPublicationAddress.cs @@ -57,10 +57,23 @@ public void TestParseOk() } [Test] - public void TestParseFail() + public void TestParseFailWithANE() { - Assert.IsNull(PublicationAddress.Parse(null)); - Assert.IsNull(PublicationAddress.Parse("not a valid uri")); + Assert.That(()=> PublicationAddress.Parse(null), Throws.ArgumentNullException); + } + + [Test] + public void TestParseFailWithUnparseableInput() + { + Assert.IsNull(PublicationAddress.Parse("not a valid URI")); + } + + [Test] + public void TestTryParseFail() + { + Assert.IsNull(PublicationAddress.TryParse(null)); + Assert.IsNull(PublicationAddress.TryParse("not a valid URI")); + Assert.IsNull(PublicationAddress.TryParse("}}}}}}}}")); } [Test] From 2592c1c4b28798a62ddd5bdb1843c09e48b91e02 Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Mon, 11 May 2020 19:40:50 +0300 Subject: [PATCH 3/9] Update the test from #828 --- projects/Unit/TestBasicProperties.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/projects/Unit/TestBasicProperties.cs b/projects/Unit/TestBasicProperties.cs index c206c799ed..0926b80f07 100644 --- a/projects/Unit/TestBasicProperties.cs +++ b/projects/Unit/TestBasicProperties.cs @@ -129,14 +129,13 @@ public void TestProperties_ReplyTo( // Arrange var subject = new Framing.BasicProperties { - // Act ReplyTo = replyTo, }; // Assert bool isReplyToPresent = replyTo != null; - string replyToAddress = PublicationAddress.Parse(replyTo)?.ToString(); + string replyToAddress = PublicationAddress.TryParse(replyTo)?.ToString(); Assert.AreEqual(isReplyToPresent, subject.IsReplyToPresent()); var writer = new Impl.ContentHeaderPropertyWriter(new byte[1024]); From 34ce6fd57b5317a49ca37334bd10ba5be779237e Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Mon, 11 May 2020 19:41:56 +0300 Subject: [PATCH 4/9] Code style --- projects/RabbitMQ.Client/client/api/PublicationAddress.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/projects/RabbitMQ.Client/client/api/PublicationAddress.cs b/projects/RabbitMQ.Client/client/api/PublicationAddress.cs index 0befcf620f..9c6cd80dc1 100644 --- a/projects/RabbitMQ.Client/client/api/PublicationAddress.cs +++ b/projects/RabbitMQ.Client/client/api/PublicationAddress.cs @@ -116,7 +116,8 @@ public static PublicationAddress Parse(string uriLikeString) return null; } - public static PublicationAddress TryParse(string uriLikeString) { + public static PublicationAddress TryParse(string uriLikeString) + { // Callers such as IBasicProperties.ReplyToAddress // expect null result for invalid input. // The regex.Match() throws on null arguments so we perform explicit check here From daaae07abc9d5fd15598fd7bab18ba206e86e811 Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Mon, 11 May 2020 19:44:32 +0300 Subject: [PATCH 5/9] Update documentation references --- projects/RabbitMQ.Client/client/api/IBasicProperties.cs | 4 ++-- projects/RabbitMQ.Client/client/impl/BasicProperties.cs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/projects/RabbitMQ.Client/client/api/IBasicProperties.cs b/projects/RabbitMQ.Client/client/api/IBasicProperties.cs index c1ccc74349..16b4e477c4 100644 --- a/projects/RabbitMQ.Client/client/api/IBasicProperties.cs +++ b/projects/RabbitMQ.Client/client/api/IBasicProperties.cs @@ -121,9 +121,9 @@ public interface IBasicProperties : IContentHeader string ReplyTo { get; set; } /// - /// Convenience property; parses property using , + /// Convenience property; parses property using , /// and serializes it using . - /// Returns null if property cannot be parsed by . + /// Returns null if property cannot be parsed by . /// PublicationAddress ReplyToAddress { get; set; } diff --git a/projects/RabbitMQ.Client/client/impl/BasicProperties.cs b/projects/RabbitMQ.Client/client/impl/BasicProperties.cs index a8946397f7..e1d761d7a8 100644 --- a/projects/RabbitMQ.Client/client/impl/BasicProperties.cs +++ b/projects/RabbitMQ.Client/client/impl/BasicProperties.cs @@ -109,9 +109,9 @@ public bool Persistent public abstract string ReplyTo { get; set; } /// - /// Convenience property; parses property using , + /// Convenience property; parses property using , /// and serializes it using . - /// Returns null if property cannot be parsed by . + /// Returns null if property cannot be parsed by . /// public PublicationAddress ReplyToAddress { From 9aeb7572365661482902b27472b5278c43448eb5 Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Mon, 11 May 2020 19:56:17 +0300 Subject: [PATCH 6/9] PublicationAddress.TryParse: return parsed value by reference --- .../RabbitMQ.Client/client/api/PublicationAddress.cs | 12 ++++++++---- .../RabbitMQ.Client/client/impl/BasicProperties.cs | 6 +++++- projects/Unit/TestBasicProperties.cs | 4 +++- projects/Unit/TestPublicationAddress.cs | 12 +++++++++--- 4 files changed, 25 insertions(+), 9 deletions(-) diff --git a/projects/RabbitMQ.Client/client/api/PublicationAddress.cs b/projects/RabbitMQ.Client/client/api/PublicationAddress.cs index 9c6cd80dc1..dbe8a10533 100644 --- a/projects/RabbitMQ.Client/client/api/PublicationAddress.cs +++ b/projects/RabbitMQ.Client/client/api/PublicationAddress.cs @@ -116,19 +116,23 @@ public static PublicationAddress Parse(string uriLikeString) return null; } - public static PublicationAddress TryParse(string uriLikeString) + public static bool TryParse(string uriLikeString, out PublicationAddress result) { // Callers such as IBasicProperties.ReplyToAddress // expect null result for invalid input. // The regex.Match() throws on null arguments so we perform explicit check here if (uriLikeString == null) { - return null; + result = null; + return false; } else { try { - return Parse(uriLikeString); + var res = Parse(uriLikeString); + result = res; + return true; } catch { - return null; + result = null; + return false; } } } diff --git a/projects/RabbitMQ.Client/client/impl/BasicProperties.cs b/projects/RabbitMQ.Client/client/impl/BasicProperties.cs index e1d761d7a8..0e0bc6e9fb 100644 --- a/projects/RabbitMQ.Client/client/impl/BasicProperties.cs +++ b/projects/RabbitMQ.Client/client/impl/BasicProperties.cs @@ -115,7 +115,11 @@ public bool Persistent /// public PublicationAddress ReplyToAddress { - get { return PublicationAddress.TryParse(ReplyTo); } + get { + PublicationAddress result; + PublicationAddress.TryParse(ReplyTo, out result); + return result; + } set { ReplyTo = value.ToString(); } } diff --git a/projects/Unit/TestBasicProperties.cs b/projects/Unit/TestBasicProperties.cs index 0926b80f07..1440acaad9 100644 --- a/projects/Unit/TestBasicProperties.cs +++ b/projects/Unit/TestBasicProperties.cs @@ -135,7 +135,9 @@ public void TestProperties_ReplyTo( // Assert bool isReplyToPresent = replyTo != null; - string replyToAddress = PublicationAddress.TryParse(replyTo)?.ToString(); + PublicationAddress result; + PublicationAddress.TryParse(replyTo, out result); + string replyToAddress = result?.ToString(); Assert.AreEqual(isReplyToPresent, subject.IsReplyToPresent()); var writer = new Impl.ContentHeaderPropertyWriter(new byte[1024]); diff --git a/projects/Unit/TestPublicationAddress.cs b/projects/Unit/TestPublicationAddress.cs index e1b4683155..7371959177 100644 --- a/projects/Unit/TestPublicationAddress.cs +++ b/projects/Unit/TestPublicationAddress.cs @@ -71,9 +71,15 @@ public void TestParseFailWithUnparseableInput() [Test] public void TestTryParseFail() { - Assert.IsNull(PublicationAddress.TryParse(null)); - Assert.IsNull(PublicationAddress.TryParse("not a valid URI")); - Assert.IsNull(PublicationAddress.TryParse("}}}}}}}}")); + PublicationAddress result; + PublicationAddress.TryParse(null, out result); + Assert.IsNull(result); + + PublicationAddress.TryParse("not a valid URI", out result); + Assert.IsNull(result); + + PublicationAddress.TryParse("}}}}}}}}", out result); + Assert.IsNull(result); } [Test] From d5f4cc396b85cad6399de43b752732d028e06254 Mon Sep 17 00:00:00 2001 From: Luke Bakken Date: Mon, 11 May 2020 11:27:24 -0700 Subject: [PATCH 7/9] Fix approval tests --- projects/Unit/APIApproval.Approve.verified.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/projects/Unit/APIApproval.Approve.verified.txt b/projects/Unit/APIApproval.Approve.verified.txt index f07a3050cd..3d8a9c9e03 100644 --- a/projects/Unit/APIApproval.Approve.verified.txt +++ b/projects/Unit/APIApproval.Approve.verified.txt @@ -483,6 +483,7 @@ namespace RabbitMQ.Client public string RoutingKey { get; } public override string ToString() { } public static RabbitMQ.Client.PublicationAddress Parse(string uriLikeString) { } + public static bool TryParse(string uriLikeString, out RabbitMQ.Client.PublicationAddress result) { } } public class QueueDeclareOk { From 56e716e18a280aab92a1e234df6ddb8d456c55c9 Mon Sep 17 00:00:00 2001 From: Luke Bakken Date: Mon, 11 May 2020 11:30:49 -0700 Subject: [PATCH 8/9] Fix formatting --- .../RabbitMQ.Client/client/api/PublicationAddress.cs | 11 ++++++++--- .../RabbitMQ.Client/client/impl/BasicProperties.cs | 3 ++- projects/Unit/TestBasicProperties.cs | 4 +--- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/projects/RabbitMQ.Client/client/api/PublicationAddress.cs b/projects/RabbitMQ.Client/client/api/PublicationAddress.cs index dbe8a10533..9f819f8e7f 100644 --- a/projects/RabbitMQ.Client/client/api/PublicationAddress.cs +++ b/projects/RabbitMQ.Client/client/api/PublicationAddress.cs @@ -125,12 +125,17 @@ public static bool TryParse(string uriLikeString, out PublicationAddress result) { result = null; return false; - } else { - try { + } + else + { + try + { var res = Parse(uriLikeString); result = res; return true; - } catch { + } + catch + { result = null; return false; } diff --git a/projects/RabbitMQ.Client/client/impl/BasicProperties.cs b/projects/RabbitMQ.Client/client/impl/BasicProperties.cs index 0e0bc6e9fb..d11bcd5d1b 100644 --- a/projects/RabbitMQ.Client/client/impl/BasicProperties.cs +++ b/projects/RabbitMQ.Client/client/impl/BasicProperties.cs @@ -115,7 +115,8 @@ public bool Persistent /// public PublicationAddress ReplyToAddress { - get { + get + { PublicationAddress result; PublicationAddress.TryParse(ReplyTo, out result); return result; diff --git a/projects/Unit/TestBasicProperties.cs b/projects/Unit/TestBasicProperties.cs index 1440acaad9..ce12f98615 100644 --- a/projects/Unit/TestBasicProperties.cs +++ b/projects/Unit/TestBasicProperties.cs @@ -122,9 +122,7 @@ public void TestNullableProperties_CanWrite( } [Test] - public void TestProperties_ReplyTo( - [Values(null, "foo_1", "fanout://name/key")] string replyTo - ) + public void TestProperties_ReplyTo([Values(null, "foo_1", "fanout://name/key")] string replyTo) { // Arrange var subject = new Framing.BasicProperties From 09bf7acb63deb3022781a7a4638156aea6bea65e Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Tue, 12 May 2020 13:24:54 +0300 Subject: [PATCH 9/9] Simplify --- projects/RabbitMQ.Client/client/impl/BasicProperties.cs | 6 ++---- projects/Unit/TestBasicProperties.cs | 1 - 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/projects/RabbitMQ.Client/client/impl/BasicProperties.cs b/projects/RabbitMQ.Client/client/impl/BasicProperties.cs index d11bcd5d1b..05ffda72a2 100644 --- a/projects/RabbitMQ.Client/client/impl/BasicProperties.cs +++ b/projects/RabbitMQ.Client/client/impl/BasicProperties.cs @@ -115,10 +115,8 @@ public bool Persistent /// public PublicationAddress ReplyToAddress { - get - { - PublicationAddress result; - PublicationAddress.TryParse(ReplyTo, out result); + get { + PublicationAddress.TryParse(ReplyTo, out var result); return result; } set { ReplyTo = value.ToString(); } diff --git a/projects/Unit/TestBasicProperties.cs b/projects/Unit/TestBasicProperties.cs index ce12f98615..3693309c8e 100644 --- a/projects/Unit/TestBasicProperties.cs +++ b/projects/Unit/TestBasicProperties.cs @@ -52,7 +52,6 @@ public void TestPersistentPropertyChangesDeliveryMode_PersistentTrueDelivery2() // Arrange var subject = new Framing.BasicProperties { - // Act Persistent = true };