MQTTnet
MQTTnet copied to clipboard
MqttTopicFilterCompareResult suggests validation of topic and filter, but doesn't
Describe the feature request
At first glance, the results MqttTopicFilterCompareResult.FilterInvalid and MqttTopicFilterCompareResult.TopicInvalid suggest that there is a validation happening. Looking at the source code, this is not the case. The arguments are only checked for string.IsNullOrEmpty(x). My tests (see below) support this.
Has this been done to avoid a performance penalty?
Which project is your feature request related to?
- Generic
Describe the solution you'd like
I expect the method to throw an exception if the argument is malformed, but not fully validated. Alternatively, I expect the method to perform a full validation and return one of the four enumerated values.
Describe alternatives you've considered
none
Additional context
Test1:
[Theory]
[InlineData("sport/tennis#", "sport/tennis")]
[InlineData("sport/tennis/#/ranking", "sport/tennis")]
public void TopicFilter_ShouldFailFilterInvalid(string topicFilter, string topicName)
{
// Act
var result = MqttTopicFilterComparer.Compare(topicName, topicFilter);
// Assert
Assert.Equal(MqttTopicFilterCompareResult.FilterInvalid, result);
}
Test2:
[Theory]
[InlineData("sport/tennis", "sport/tennis#")]
[InlineData("sport/tennis", "sport/tennis/#/ranking")]
[InlineData("sport/tennis", "sport/tennis/+/ranking")]
public void TopicFilter_ShouldFailTopicInvalid(string topicFilter, string topicName)
{
// Act
var result = MqttTopicFilterComparer.Compare(topicName, topicFilter);
// Assert
Assert.Equal(MqttTopicFilterCompareResult.TopicInvalid, result);
}
All tests fail with the actual result being MqttTopicFilterCompareResult.NoMatch.
Validation is an intended part of the comparison check. But this should not affect performance so it does not cover all possible cases. If it is an easy check like if there is some more chars available after the # char, the proper enum should be returned. But when evaluation requires more checks etc. I prefer to avoid that in favor of performance.
Don't you think that the possible return values might be misleading?
Yes probably I will remove these values and basically return "match" or "no match". For now I will take your unit tests and improve the code because I assume this will produce not much overhead.