Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

route parameters with type constraints #1127

Closed
SysPete opened this issue Feb 5, 2016 · 18 comments
Closed

route parameters with type constraints #1127

SysPete opened this issue Feb 5, 2016 · 18 comments
Assignees

Comments

@SysPete
Copy link
Member

SysPete commented Feb 5, 2016

Would be nice to have type constraints for route parameters as suggested by @shadowcat-mst

<mst> get '/user/:id(Int)/'
<mst> or similar
<mst> which would default to Types::Standard
<SysPete> absolutely
<mst> but then you can create a MyApp::Types to declare your own in
<mst> using Type::Library + inheritance
<mst> in Catalyst we tried doing it via getting people to import the type constants, but that gets disappointingly crappy fast
<mst> whereas
<mst> set type_library => 'MyApp::Types';
<mst> is relatively nice
<SysPete> yup
<mst> note: this design may also have problems. but my last attempt definitely did so :)
@shumphrey
Copy link
Contributor

This would be neat.

@SysPete
Copy link
Member Author

SysPete commented Feb 6, 2016

@shumphrey I have a branch which works for simple type declarations for types available in Dancer::Core::Types (from my Type::Tiny branch so includes Types::Standard). Only works for simple types atm but could do more. Do you have any expectations?

@bigpresh
Copy link
Member

bigpresh commented Feb 7, 2016

Definitely sounds like a cool feature. @SysPete, you saying your branch already provides simple support for this? Is there a link look at it for the lazy?

@SysPete
Copy link
Member Author

SysPete commented Feb 7, 2016

@bigpresh this is the commit with my initial implementation: https://github.com/SysPete/Dancer2/commit/ccdffaddaa30fb3b2ccc553cfc3561731d1ae370. No pod updates and no tests pushed yet but that will happen in the next day or so. Changes are really small.

@SysPete
Copy link
Member Author

SysPete commented Feb 7, 2016

@bigpresh this feature depends on my Type::Tiny PR (hint, hint) and is not feature-complete: needs to support user-specified type library for starters.

@shumphrey
Copy link
Contributor

@SysPete No expectations. I like the idea of the route matching happening on arbitrary types. Currently, if we do a regex match, and the route doesn't match, it goes on to the next one. This allows two routes, one for a string, one for an int.
This removes the need for me to write if/else logic etc as this is handled by D2.
Extending this with types, would further allow routes to be distinguished without the need for conditional logic.
In general, I think the regex routes cover the majority of cases, where route definitions conflict, but adding types would just make this much more flexible.

@SysPete
Copy link
Member Author

SysPete commented Feb 8, 2016

@SysPete
Copy link
Member Author

SysPete commented Feb 8, 2016

@bigpresh use of eval has been removed in favour of Type::Registry->lookup which restricts type libraries to being built on Type::Tiny but I don't see this as a major issue.

@bigpresh
Copy link
Member

bigpresh commented Feb 8, 2016

Excellent :) 👍

@xsawyerx
Copy link
Member

I'd like to hear from more people on the core team.

@veryrusty @mickeyn @yanick @racke @cromedome anyone else?

@yanick
Copy link
Contributor

yanick commented Mar 29, 2016

Interesting venue to explore. We could even do something like (off-the-cuff pseudo-code below)

subtype UserId => as Int => validate { };  # don't remember the real Types::Tiny syntax

get '/api/user/!UserId' => sub { 
  ...;
  # dancer magic would use the type name to populate the right param name
  param('UserId'); 
};

@SysPete
Copy link
Member Author

SysPete commented Mar 29, 2016

As long as we still have the option for something like '/foo/:id[Int]/:bar[Int]'as per my current implementation I like this idea. I just don't want to be forced to create new types.

@xsawyerx
Copy link
Member

This might be a silly question (that's my specialty!) but what about paths that have parenthesis in them or brackets? (Depending on which one we choose.)

@xsawyerx
Copy link
Member

According to RFC 1738, some characters are "unsafe" and perhaps we could/should use them in this, because they're expected not to exist in the URL itself:

   Unsafe:

   Characters can be unsafe for a number of reasons.  The space
   character is unsafe because significant spaces may disappear and
   insignificant spaces may be introduced when URLs are transcribed or
   typeset or subjected to the treatment of word-processing programs.
   The characters "<" and ">" are unsafe because they are used as the
   delimiters around URLs in free text; the quote mark (""") is used to
   delimit URLs in some systems.  The character "#" is unsafe and should
   always be encoded because it is used in World Wide Web and in other
   systems to delimit a URL from a fragment/anchor identifier that might
   follow it.  The character "%" is unsafe because it is used for
   encodings of other characters.  Other characters are unsafe because
   gateways and other transport agents are known to sometimes modify
   such characters. These characters are "{", "}", "|", "\", "^", "~",
   "[", "]", and "`".

   All unsafe characters must always be encoded within a URL. For
   example, the character "#" must be encoded within URLs even in
   systems that do not normally deal with fragment or anchor
   identifiers, so that if the URL is copied into another system that
   does use them, it will not be necessary to change the URL encoding.

Parenthesis aren't covered there, but square brackets, angle brackets, and curly braces are all unsafe and should be encoded.

@yanick
Copy link
Contributor

yanick commented Mar 29, 2016

(parens) Plus, I have to check, but if I remember correctly :stuff matches everything up to the next /, so using /blah/:foo(String)/blah outside of the type context wouldn't be parsed right anyway.

And yeah, if [] and {} and not considered valid in urls, all the more reasons to use them. ==b o.o

@xsawyerx
Copy link
Member

xsawyerx commented Apr 3, 2016

@yanick++ for the extra tidbit! :)

@cromedome
Copy link
Contributor

So this has been sitting here a while. Type::Tiny has come to pass... any chance you want to revisit this, @SysPete? This would be a great addition!

@cromedome
Copy link
Contributor

Resolved by #1476. Thank you!

cromedome added a commit that referenced this issue Dec 24, 2019
    [ BUG FIXES ]
    * None

    [ ENHANCEMENTS ]
    * GH #1127, GH #1476: Route parameters with types (Peter Mottram -
      SysPete)

    [ DOCUMENTATION ]
    * None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants