MQTTnet icon indicating copy to clipboard operation
MQTTnet copied to clipboard

MqttTopicFilterCompareResult suggests validation of topic and filter, but doesn't

Open d5UtQvp8QQU9 opened this issue 3 years ago • 3 comments

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.

d5UtQvp8QQU9 avatar Jul 05 '22 12:07 d5UtQvp8QQU9

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.

chkr1011 avatar Jul 19 '22 15:07 chkr1011

Don't you think that the possible return values might be misleading?

d5UtQvp8QQU9 avatar Jul 21 '22 09:07 d5UtQvp8QQU9

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.

chkr1011 avatar Jul 23 '22 08:07 chkr1011