Should we throw when parsing a template with segments that contain default values followed by non default segments?

[javiercn]

Currently we can write a template like the following and we will parse it correctly, does it make sense to have default value parameters before non default value parameters?
{parameter1=1}/{parameter2=2}/{parameter3}/{parameter4=4}
I don't think templates like these make sense, so we should ensure that when we find a segment with a default value, the remaining segments are either catch all, optional or have default values.

[javiercn]

@rynowak @kichalla

[rynowak]

This seems like a regression. I know there are some tests for this kind of thing.

[Eilon]

Throwing an exception would be a breaking change at this time. But we could certainly detect this and log it, and then in 2.0.0 start throwing an exception.

Is that a reasonable approach here?

[rynowak]

I don't believe that it's possible for this route to match anything or generate links. If it currently today is functionally useless in every way can we just make it throw now?

[Eilon]

It's not functionally useless it all, it just has extraneous info in it.

This route:

{parameter1=1}/{parameter2=2}/{parameter3}/{parameter4=4}

Is functionally equivalent to this route:

{parameter1=1}/{parameter2=2}/{parameter3}/{parameter4}

In other words, it's just that the last parameter isn't really optional, despite the annotation attempting to indicate that it is.

[rynowak]

It's not functionally useless it all, it just has extraneous info in it.

So you tried it and it works?

[Eilon]

I did not, but if it doesn't, I'd say that's also a regression from past similar behavior.

[javiercn]

I think that the current behavior might work on some scenarios but not with the behavior that you intend (basically any default parameter before the last ones gets ignored as you need to provide values for them anyways) and there's a chance that routes like / and /something will match because we shortcut when we see an optional parameter and we don't have more values on the url. I would say that make it throw + announcement would be the right thing to do.

[Eilon]

Oh BTW sorry my earlier comment was incorrect regarding which parameters were required. I should have said:

It's not functionally useless it all, it just has extraneous info in it.

This route:

{parameter1=1}/{parameter2=2}/{parameter3}/{parameter4=4}

Is functionally equivalent to this route:

{parameter1}/{parameter2}/{parameter3}/{parameter4=4}

In other words, it's just that the first two parameters aren't really optional, despite the annotations attempting to indicate that they are.

[javiercn]

Not just that, I haven't checked but I'm inclined to think that /something and / will probably also match that route due to the way we build the attribute route tree, which is more problematic. I'll double check that this is actually possible in an E2E scenario (I discovered it on a unit/integration test) and check if the routes I suggest match it.

[Eilon]

If that's the case then that's really, really bad...

[Eilon]

Ok this does look totally broken with attribute routing. With conventional routing it's as I described: the route is perfectly valid, it just doesn't do exactly what it might seem like it does.

But with attribute routing, it appears to be broken in lots of scenarios.

With this route:

        [Route("hi/{parameter1=1}/{parameter2=2}/{parameter3}/{parameter4=4}")]

All 4 parameters are required! It will only match the URL hi/a/b/c/d. It will not match hi/a/b/c or anything else shorter.

Even more bizarrely, with this route (all 4 optional - a totally legitimate route):

        [Route("hi/{parameter1=1}/{parameter2=2}/{parameter3=3}/{parameter4=4}")]

Also all 4 parameters are required! It will also only match the URL hi/a/b/c/d. It will not match hi/a/b/c or anything else shorter.

Am I missing something here? I'm quite baffled how all these cases don't seem to work. Do we just not properly support optional parameters in attribute routes?

[javiercn]

@Eilon That was one of the bugs that we fixed [#324], but didn't make it to 1.0.0. It's already fixed in 1.1.0, I found this bug too when I was doing the fix for the original bug. The reason why I want to throw is that our algorithms make assumptions on the shape of the tree that we build for doing attribute routing efficiently. (Like when to stop recursion, etc.) And they assume only correct route templates at the time the tree is being built. So if that's not the case we build an incorrect (and unknown behavior route tree) and the matching and link generation algorithms don't work appropriately.

My opinion is that given the random behaviors that we are going to provide with incorrect route templates, it's better to fix this bug by throwing when the user tries to define an incorrect template.

[Eilon]

I contend that the route template isn't wrong; it just has extraneous info in it. And throwing is a breaking change.

It sounds like we have bad logic in some algorithm we implemented - let's fix the algorithm.

[javiercn]

The route template is wrong, we only intend to support/allow optional parameters as the last parameter, I can check back on MVC5, but i'm pretty sure those routes throw.

[javiercn]

I retract my comment, I tried this out and both MVC and Web API allow it, although it's useless in both scenarios. Maybe we can throw in 2.0? I really think that when you run into this situation is because of an error and that there is no scenario for this and for that reason, throwing gives an early warning to users instead of leaving them wondering why their routes don't work.

As per fixing our algorithms, they work fine with what we consider correct routes and exhibit an undefined behavior (we haven't thoroughly tested these incorrect routes) but I don't think nothing bad will happen, (other than your routes not matching what you would think they match)

[Eilon]

I'm ok if we consider throwing in 2.0.0.

Are there any remaining issues to fix in 1.0.1 / 1.1.0? Or did we already fix all the major issues around this?

[Eilon]

We did a review of all the breaking changes (proposed or done) and with @DamianEdwards and others we felt that the value here wasn't sufficient to warrant this breaking change. It's a fairly harmless behavior, which apparently hasn't bothered any customers, so closing this as Won't Fix.