Route paramter constraints

[wout]

We've touched on this in the Discord: adding constraints for dynamic parameters in routes. Possible API approaches could be either with an annotation:

@[Lucky::RouteConstraint(locale: /\A[a-z]{2}\z/)]
class About::Index < BrowserAction
  get "/about", "/:locale/about" do
    html About::IndexPage
  end
end

With another argument:

class About::Index < BrowserAction
  get "/about", "/:locale/about", constraints: { locale: /\A[a-z]{2}\z/ } do
    html About::IndexPage
  end
end

Or like route_prefix with a dedicated macro:

class About::Index < BrowserAction
  route_constraints locale: /\A[a-z]{2}\z/

  get "/about", "/:locale/about" do
    html About::IndexPage
  end
end

Aside from a regex, we could also make it accept a type:

class Product::Index < BrowserAction
  route_constraints uuid: UUID

  get "/product/:uuid" do
    html Product::ShowPage
  end
end

And test with UUID.parse?. The supported types can be limited to the most commonly used and expand later.

Or even use a proc:

route_constraints handle: ->(handle : String) { ProfileQuery.new.handle(handle).first? }

Considerations

I've been looking at the code to find the best way to implement this. My primary concern here is the speed of the router. Ideally I would rather not touch LuckyRouter at all. And this brings me to the first consideration:

Do we want to allow the same path, but with different constraints, and route each variant to a different action?
→ So /some/:route with /\A\d+\z/ would route to action A, and /some/:route with constraint /\A[a-z]+\z to action B.

1. If the answer is YES, then we'd need to update LuckyRouter to test a matched path to the regex and continue lookup if it doesn't match.
2. If the answer is NO, we could just discard the matched route Lucky::RouteHandler and continue to the following handlers, which are static files in the default setup. This is also the easiest and least intrusive approach.

My feeling is, start with option 2 because it gives us safer and more controllable routing without a possible performance hit, and implement option 1 if there's a need for it (YAGNI).

Typed params

Another thing we briefly discussed is casting matched params to their respective types. I think this will be difficult to achieve without braking things because params.get(:uuid) will then suddenly return all kinds of unions. I think a better approach would be adding a method overload for Lucky::Params#get:

params.get(:uuid, UUID)
# => UUID(4ceb2290-050f-48ad-8958-75499261046e)

If there's a constraint on the path param, you can be 100% sure that the cast to UUID won't fail. But even then we could add the safe variant:

params.get?(:uuid, UUID)
# => UUID(4ceb2290-050f-48ad-8958-75499261046e) | Nil

This would then test the existence of the value and test if it matches the required format, and return Nil id it doesn't match.

Thoughts?

[jwoertink]

I definitely like the annotation or the route_constraints macro options over adding extra args to the route itself. I was originally thinking the annotation would be a pretty clean option, but you do have a point that route_constraints lines up with a few other features Lucky already has.

As for your consideration points, I don't think you'd ever want to have 2 separate action classes that use the same route path and different constraints... unless maybe they're different HTTP methods? I feel like that would be strange, but they would still match different actions anyway, so I say we probably don't want the same path with different constraints to match different actions. So options 2 is probably the best way.

Related to typed params, when I use params.get this is generally an escape hatch to pulling out specific values. I think generally you'd want to just pass your params in to operations, or if you're pulling from query params, then you're already getting typed params with param uuid : UUID anyway. My thought on the type added to route constrains was purely for path params only in which case a method is generated for you

here

lucky/src/lucky/routable.cr

Line 196 in 5922ff8

def {{ part }} : String

and here

lucky/src/lucky/routable.cr

Line 206 in 5922ff8

def {{ part }} : String?

where these are statically set to String and String? respectively. By adding the route constraint type for these, then we could optionally set them to say UUID or whatever type was declared. Of course, we'd have to know how to convert and cast to that type, but a first round option could just be something simple like a case statement when UUID ... when Int64 .... Then if the type isn't listed, it's just a compile-time error for now. We could get smarter with it later.

Thanks for putting this all together and getting the discussion started!

[wout]

Yeah, same here. I like annotations but the route_constraints macro feels more in line with other existing methods.

I'll have to look deeper into the whole params story though but I think casting with a case statement is a good start. There are not too many options there anyway so a case statement won't hurt.