http-rs / route-recognizer

Recognizes URL patterns with support for dynamic and wildcard segments
MIT License
100 stars 38 forks source link

Route (metadata) ordering is incorrect. #20

Closed Parakleta closed 5 years ago

Parakleta commented 8 years ago

The implementation of prioritising routes based on the specificity of their pattern introduced in e5047b8afbbc32b64305ad95331c6d58aee0869c is nice, but I believe the ordering is the reverse of what it should be.

I think if you have a mixture of static, dynamic and star components in the path the most specific path should be preferred (i.e. /index.html > /:segment > /*path). Simple cases like this work (as does the test case provided in the referenced commit) but not because of the ordering conditions but rather because in this simple case the higher priority paths contain none of the lower priority components.

Consider instead /archive/*path vs /*path and you see that the ordering constraints as currently implemented do the reverse in that /*path (the least static) is the preferred path.

I believe instead the ordering should be as follows:

    if self.statics > other.statics {
        Ordering::Greater
    } else if self.statics < other.statics {
        Ordering::Less
    } else if self.dynamics > other.dynamics {
        Ordering::Greater
    } else if self.dynamics < other.dynamics {
        Ordering::Less
    } else if self.stars > other.stars {
        Ordering::Greater
    } else if self.stars < other.stars {
        Ordering::Less
    } else {
        Ordering::Equal
    }

The ordering of the final stars comparison could go either way. As I have proposed it here, the path /*pre/and_then/*post is preferred over /archive/*path because the latter is least specific (containing only two components versus three). An alternative would be to ignore the number of stars in the ordering and fall back to the default ordering so long as it is deterministic.

I believe the default ordering currently is such that if two matches are equal (based on metadata ordering) then the first defined is chosen (please correct me if I am wrong). A deterministic default ordering (or else an explicit one) is required because I don't believe there is any sensible way to automatically decide between /archive/*path and /*path/meta

jadencarver commented 8 years ago

+1

wycats commented 5 years ago

As the original author of this code, I think I made a mistake in the original implementation. Unless I'm misunderstanding, I agree with this proposed change.