routing key bug fix and wild card support #95
Conversation
…ldCard support for RoutingKeyValue.
…Value with a wildcard.
|
|
||
| [Subscribe] | ||
| /// <summary> | ||
| /// This Subscription should only get SampleEvents who messages end in a |
There was a problem hiding this comment.
This comment is incomplete.
| /// </summary> | ||
| /// <param name="sampleEvent"></param> | ||
| /// <returns></returns> | ||
| [Subscribe(routingKeyName: "Message", routingKeyValue: "*1")] |
There was a problem hiding this comment.
I was not thinking of separating the name and value in the Subscribe attribute, but just splitting it in the ReferenceWrapper constructor. This way breaks backwards compatibility, which I would like to avoid. The only files that should need to change (besides tests and examples) is ReferenceWrapper.
| protected ReferenceWrapper(string routingKey = null) | ||
| /// <param name="routingKeyName">Optional Message property path used for routing</param> | ||
| /// <param name="routingKeyValue">Optional value to match with message payload content used for routing.</param> | ||
| protected ReferenceWrapper(string routingKeyName = null, string routingKeyValue = null) |
There was a problem hiding this comment.
Revert this to accept a single parameter 'routingKey'. Split on '=' and validate as it was doing before. Then assign RoutingKeyName and RoutingKeyValue instead of RoutingKey.
There was a problem hiding this comment.
If you want to go the regex route should we still have a RoutingKeyValue property that is a string or should we just create the regex in the constructor?
| @@ -54,5 +54,246 @@ public void WhenDeterminingShouldDeliverMessageToActorWithUnmatchingPayload_Then | |||
| bool shouldDeliver = actorRef.ShouldDeliverMessage(messageWrapper); | |||
| Assert.IsFalse(shouldDeliver); | |||
| } | |||
| [TestMethod] | |||
| public void WhenDeterminingShouldDeliverMessageToActorWithUnmatchingPayload_ThenReturnsTrue() | |||
There was a problem hiding this comment.
Test name should be "WhenDeterminingShouldDeliverMessageToActorWithMatchingPayload_ThenReturnsTrue".
| @@ -54,5 +54,246 @@ public void WhenDeterminingShouldDeliverMessageToActorWithUnmatchingPayload_Then | |||
| bool shouldDeliver = actorRef.ShouldDeliverMessage(messageWrapper); | |||
| Assert.IsFalse(shouldDeliver); | |||
| } | |||
| [TestMethod] | |||
There was a problem hiding this comment.
Add a space between the methods.
| Assert.IsFalse(shouldDeliver); | ||
| } | ||
| [TestMethod] | ||
| public void WhenDeterminingShouldDeliverMessageToActorWithUnmatchingPayloadWithStartingWildCard__ThenReturnsTrue() |
There was a problem hiding this comment.
Test name should be "WhenDeterminingShouldDeliverMessageToActorWithMatchingPayloadWithStartingWildCard__ThenReturnsTrue".
| bool shouldDeliver = actorRef.ShouldDeliverMessage(messageWrapper); | ||
| Assert.IsFalse(shouldDeliver); | ||
| } | ||
| [TestMethod] |
There was a problem hiding this comment.
Add an empty line between methods.
| Assert.IsFalse(shouldDeliver); | ||
| } | ||
| [TestMethod] | ||
| public void WhenDeterminingShouldDeliverMessageToActorWithUnmatchingPayloadWithEndingWildCard__ThenReturnsTrue() |
There was a problem hiding this comment.
Test name should be "WhenDeterminingShouldDeliverMessageToActorWithMatchingPayloadWithEndingWildCard__ThenReturnsTrue".
| Assert.IsFalse(shouldDeliver); | ||
| } | ||
| [TestMethod] | ||
| public void WhenDeterminingShouldDeliverMessageToActorWithUnmatchingPayloadWithMultipleWildCard__ThenReturnsTrue() |
There was a problem hiding this comment.
Test name should be "WhenDeterminingShouldDeliverMessageToActorWithMatchingPayloadWithMultipleWildCard__ThenReturnsTrue".
| string value = (string)token.SelectToken(RoutingKeyName); | ||
| if(value is null) | ||
| return false; | ||
| var regex = new Regex("^" + Regex.Escape(RoutingKeyValue).Replace(@"\*", ".*") + "$"); |
There was a problem hiding this comment.
This makes me think we might as well just go all the way and support any regex for the RoutingKey instead of inventing a custom and less flexible wildcard scheme.
There was a problem hiding this comment.
That is good idea but it would be a breaking change that would only show up while running which sounds dangerous.
There was a problem hiding this comment.
What are your thoughts on adding a new bool to the Subscribe Attribute, something like UseRegexForRouting.
if (UseRegexForRouting) {
// regex compare
} else {
// string.equals
}
This wouldn't prevent any breaking changes, if someone had used regex reserved characters in there RoutingKey.
|
Since most of changes requested are just undoing the changes that I made, it is easier or better if I make a new branch and a new PR? |
|
Whatever is easiest for you. Either way works for me. |
|
Closing this PR. I will submit a new one. |
#94
Replace RoutingKey with RoutingKeyName and RoutingKeyValue. If RoutingKeyValue is not null but RoutingKeyName is null, a invalid argument exception is thrown. This seems like the best option. The only other thing that I thought of was maybe using a tuple instead of two different properties.
#93
Added wildcard support for RoutingKeyValue by using a regex in ShouldDeliverMessage(). Added test methods for wildcard support.