Skip to content
This repository has been archived by the owner on Nov 27, 2018. It is now read-only.

{id:int?} should act as optional #123

Closed
yishaigalatzer opened this issue Nov 3, 2014 · 9 comments
Closed

{id:int?} should act as optional #123

yishaigalatzer opened this issue Nov 3, 2014 · 9 comments
Assignees
Milestone

Comments

@yishaigalatzer
Copy link
Contributor

No description provided.

@rynowak
Copy link
Member

rynowak commented Nov 4, 2014

What's the behavior in MVC5? I don't think any of our constraints know about optionality.

@yishaigalatzer
Copy link
Contributor Author

Interesting. I was thinking optionallity could trump constraint for the same key.

@kichalla
Copy link
Member

kichalla commented Nov 4, 2014

In MVC5 (MVC and Web API attribute routing) the integer constraint only gets evaluated when there is a value in this case, so I think this is a bug...

@rynowak
Copy link
Member

rynowak commented Nov 4, 2014

In MVC5 (MVC and Web API attribute routing) the integer constraint only gets evaluated when there is a value in this case, so I think this is a bug...

If this were true, then global constraints via attribute routing wouldn't work at all. (there are constraints for which there is never a route value).

If I try the following definition in MVC, I can get to my action with /Home/Index/5, but not /Home/Index:

            routes.MapRoute(
                "default",
                "{controller}/{action}/{id}",
                new { id = UrlParameter.Optional, },
                new { id = new IntRouteConstraint(), });

@kichalla
Copy link
Member

kichalla commented Nov 4, 2014

Good point!, Ryan...will investigate to see why it works in case of attribute routing...

@kichalla
Copy link
Member

kichalla commented Nov 4, 2014

So it seems like in case of attribute routing in MVC5, for the above scenario, the inline route template parser wraps the IntRouteConstraint inside an OptionalRouteConstraint...

InlineRouteRemplateParser
http://aspnetwebstack.codeplex.com/SourceControl/latest#src/Common/Routing/InlineRouteTemplateParser.cs
Code of interest

if (parameterConstraints.Count > 0)
{
    var constraint = parameterConstraints.Count == 1 ?
        parameterConstraints[0] :
        new CompoundRouteConstraint(parameterConstraints);

    if (isOptional)
    {
        // Constraints should match RouteParameter.Optional if the parameter is optional
        // This prevents contraining when there's no value specified
        constraint = new OptionalRouteConstraint(constraint);
    }

    return constraint;
}

OptionalRouteConstraint
http://aspnetwebstack.codeplex.com/SourceControl/latest#src/Common/Routing/Constraints/OptionalRouteConstraint.cs

@rynowak
Copy link
Member

rynowak commented Nov 4, 2014

That's pretty cool, I didn't know we dealt with that. If we want to fix this we should do it for conventional and attribute routes.

@yishaigalatzer
Copy link
Contributor Author

@rynowak you are on it, or pick a victim.

There is one wierdness around the Required constraint. But you got yourself in it if you wrote it, and it's not that interesting anyways.

@yishaigalatzer yishaigalatzer added this to the 1.0.0-beta2 milestone Nov 5, 2014
@kulmugdha kulmugdha assigned kulmugdha and unassigned rynowak Nov 6, 2014
kulmugdha added a commit that referenced this issue Nov 18, 2014
kulmugdha added a commit that referenced this issue Nov 18, 2014
kulmugdha added a commit that referenced this issue Nov 19, 2014
kulmugdha added a commit that referenced this issue Dec 4, 2014
Added OptionalRouteConstraint class to take care of optional inline parameter. It will create the OptionalRouteConstraint for a inline parameter that is optional with real constraint on the parameter as inner constraint of OptionalRouteConstraint.
@kulmugdha
Copy link
Contributor

f549a55 -- this fixes the routing part.
ba8cf3ca4617ee4b5a15ac45a88c2bb36418b3e1 -- this fixes the MVC part

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants