Skip to content
This repository was archived by the owner on Jan 24, 2021. It is now read-only.

Int Route Constraint Still Matching for Long #1565

Closed
RPM1984 opened this issue Jun 12, 2014 · 35 comments
Closed

Int Route Constraint Still Matching for Long #1565

RPM1984 opened this issue Jun 12, 2014 · 35 comments
Assignees

Comments

@RPM1984
Copy link

RPM1984 commented Jun 12, 2014

I have a route like this:
Get["/details/{value:int}"]

But if i pass a value like this:
/details/1111111111111111111

It still matches, and errors with the following message:
Value was either too large or too small for an Int32

Which is valid. BUT, it shouldn't have matched in the first place, according to the documentation here

@RPM1984
Copy link
Author

RPM1984 commented Jun 12, 2014

@PureKrome - cc'd you in

@phillip-haydon
Copy link
Member

At what point does it error out?

int is just an alias for Int32

But we aren't dictating the size of the int when used in routing, so we're using the maximum value available in .NET which is Int64 aka a long.

Get["/details/{value:int}"] = _ =>
{
    long value = (long)_.value;

    return value;
}

This is perfectly value.

@RPM1984
Copy link
Author

RPM1984 commented Jun 13, 2014

It errors when i try and pass the dynamic param to my method, e.g:
Get["/details/{value:int}"] = parameters => Details(parameters.value);
where the Details method expects an int

Fair enough, so the doco here is a bit misleading..
int - Allows only integer values.

I assumed int in C# (aka Int32, like you said).

I only want int32 values, so i guess i'll have to use:
Get["/details/{value:range(1, 2147483647)}"]

bit messy...

@phillip-haydon
Copy link
Member

Maybe the other @NancyFx/most-valued-minions can take a look, see if they think this is something that should be changed i.e:

  • value:long
  • value:int
  • value:short

Or if doco needs to be updated.

@jchannon
Copy link
Member

If you specify int as a route constraint and the number is larger it should fail IMO with a helpful error message

@PureKrome
Copy link
Contributor

so @jchannon you're saying that value: int should therefore be value:int32 explicitly?

and not value:int == any number.. which means int and long.

@phillip-haydon
Copy link
Member

@PureKrome - we would name them their .NET alias's

int / long / short

@khellang
Copy link
Member

Yeah, I wondered why that was when I first saw it, but didn't change it. It uses long.TryParse for the IntRouteSegmentConstraint here. Assigning a long to an int might of course fail.

I guess we should ask @mat-mcloughlin why?

I suggest we introduce LongRouteSegmentConstraint and change the current logic to int.TryParse.

@khellang
Copy link
Member

Looks like MVC also has two different constraints, one for int and one for long.

@jchannon
Copy link
Member

We can still use value:int but the logic must parse it as an int

@khellang
Copy link
Member

I suggest we introduce LongRouteSegmentConstraint and change the current logic to int.TryParse.

😉

@phillip-haydon
Copy link
Member

Correct @jchannon but we should add all 4 primitive number types. byte/short/int/long that all map to the implementation in .NET.

@jchannon
Copy link
Member

@phillip-haydon

@phillip-haydon phillip-haydon self-assigned this Jun 13, 2014
@phillip-haydon
Copy link
Member

ITS ASSIGNED TO ME!

@PureKrome
Copy link
Contributor

wohoo

@khellang khellang added this to the 1.0Alpha milestone Jun 13, 2014
@mat-mcloughlin
Copy link
Contributor

Passing the buck on this one. After discussions it was decided that there wasn't much need for both int and long so under advisement just had the int constraint parsed with long. You want me change?

@phillip-haydon
Copy link
Member

I can change it, I don't mind. But yeah I remember you put int, and i decided to make it long and just leave it at that, I didn't think people would want to constrain to actual length of the int types.

@mat-mcloughlin
Copy link
Contributor

Yeah me neither ;) and stop stealing the easy PR's trying to get more involved ;)

@phillip-haydon
Copy link
Member

If you want to do it, go head :) I was just taking responsibility because it was me who said go with long.

@mat-mcloughlin
Copy link
Contributor

Tbh I still don't see a business need to distinguish

@jchannon
Copy link
Member

Make it value:number then parse it as long then that should encompass all
of them

@mat-mcloughlin
Copy link
Contributor

Breaking change right there

@jchannon
Copy link
Member

Yup

On Friday, 13 June 2014, mat-mcloughlin [email protected] wrote:

Breaking change right there

Reply to this email directly or view it on GitHub
#1565 (comment).

@damianh
Copy link
Member

damianh commented Jun 13, 2014

Lol busted
On 13 Jun 2014 09:08, "mat-mcloughlin" [email protected] wrote:

Yeah me neither ;) and stop stealing the easy PR's trying to get more
involved ;)


Reply to this email directly or view it on GitHub
#1565 (comment).

@albertjan
Copy link
Contributor

I agree with you @jchannon number would be fine. Adding byte, short, ushort, int, uint, long, ulong is a bit overkill imho.

@phillip-haydon
Copy link
Member

I'm not suggesting we do uint/ulong. Just the 4 signed types.

@albertjan
Copy link
Contributor

😄 Yes I know I was just exaggerating I thought of adding Complex to the list too. I'd like to keep the current impl and rename it to number and create an alias to int, and deprecate that. I don't like the idea of leaking the .net type system to my api.

@khellang
Copy link
Member

Eeeh, @jchannon how would that fix anything? The problem is that when you parse something as long, you'll stil loose data when assigning to int and get

Value was either too large or too small for an Int32

@mat-mcloughlin
Copy link
Contributor

So what types we going with?

@phillip-haydon
Copy link
Member

Something that will fit @jchannon's mum.

@jchannon
Copy link
Member

@khellang realised that after i said it. we either have onevalue:number and its a long to encompass everything or we do all signed types such as int, short, long etc so that when it comes to a route and a method in the route is called like the OP example it'll be ok

@phillip-haydon
Copy link
Member

Maybe we just do int and long...

How often do people ever use short and byte considering we only deal with bytes when dealing with files or the likes of. And short, I don't remember the last time I used it...

@jchannon
Copy link
Member

agree

@mat-mcloughlin
Copy link
Contributor

ok, I'll add in long

@thecodejunkie
Copy link
Member

Resolved by #1569

@khellang khellang removed this from the 1.0 Alpha milestone Jan 27, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants