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

Router Conditions #403

Open
MuhammetDilmac opened this issue Sep 25, 2017 · 8 comments
Open

Router Conditions #403

MuhammetDilmac opened this issue Sep 25, 2017 · 8 comments
Labels

Comments

@MuhammetDilmac
Copy link

MuhammetDilmac commented Sep 25, 2017

Maybe support some conditions on route definitions.

Like sinatra; http://www.sinatrarb.com/intro.html#conditions

Example;

get "/", host_name: "api.example.com" do
  "api.example.com"
end
@sdogruyol
Copy link
Member

sdogruyol commented Sep 25, 2017

Not sure about this, an alternative and more decoupled of doing this would be using a middleware.

class SubdomainHandler
  only ["/"]

  def call(env)
    if only_match?(env)
    ..... # your_logic
    end
    call_next(env)
  end
end

add_handler SubdomainHandler.new

get "/" do |env|
  "api.example.com"
end

@crisward
Copy link
Contributor

crisward commented Oct 4, 2017

The problem with the middleware solution is it's completely invisible to the developer working on the route. I think middleware makes more sense when more general, perhaps adding content-json headers to everything with a '/api/*' route.

For a single route, express js has route specific middleware, which you could attach as another parameter within the route declaration. The 2nd parameter can actually take an array of middlewares.

eg

var is_admin = function (req, res, next) {
  //do something
  next()
})

var is_over18 = function (req, res, next) {
  //do something else
  next()
})

app.get('/example', [is_admin, is_over18], function (req, res) {
  res.send("You're and admin and over 18")
})

This is much more declarative, anyone looking at the route can see whats happening.
In kemal we could replicate this with procs?

def id_admin(ctx)
  # return if ok, or throw
end

def is_over18(ctx)
  # return if ok, or throw
end

get "/example",[->is_admin(Ctx),->is_over18(Ctx)] do |ctx|
  "You're and admin and over 18"
end

Or with standard middleware handlers

class Is_Admin
  include HTTP::Handler
  def call(context)
    # do something
    call_next(context)
  end
end

class Is_Over_18
  include HTTP::Handler
  def call(context)
    # do something
    call_next(context)
  end
end

get "/example",[Is_Admin.new,Is_Over_18.new] do |ctx|
  "You're and admin and over 18"
end

WDYT?

@sdogruyol
Copy link
Member

I have some ideas about middlewares but it's still in flux. How about creating a new issue for this proposal (RFC) @crisward ?

@crisward
Copy link
Contributor

crisward commented Oct 5, 2017

Ok. Any preference on the above. On reflection I think I prefer 2 (standard middleware handlers), especially as you could use existing middleware like this.

The definition is more verbose, but most middleware is going to contain a handful of lines of code anyway.

@sdogruyol
Copy link
Member

@crisward let's consider both, however my preference tends to be in favor for options 2

@Blacksmoke16
Copy link
Contributor

Blacksmoke16 commented Jul 4, 2018

Been a while since last comment on this but what would your thoughts be on giving routes an optional config hash/namedtuple. My reasoning is there doesn't seem to be a way to nicely handle route specific options, besides using a Handler with an only [] macro. That is totally doable but is a little bit of a pain imo as that array could get pretty big after a while.

Was thinking this could work like:

  get "/user/:id", {resolve: User} do
    "Hello World!"
  end

  # Wouldn't trigger resolve handler nor auth handler
  get "/settings/:id", do
    "Hello World!"
  end

  post "/user/:id", {auth: true, requirements: [Is_Admin.new,Is_Over_18.new]} do
    "Hello World!"
  end
  class ResolveHandler < Kemal::Handler
    def call(context)
      if context.config && context.config[:resolve]
        # Query User model for record with given id
        # Set given record to context (or env.set?) as well maybe so could be accessible within route.
      end
      call_next context
    end
  end
  class AuthHandler < Kemal::Handler
    def call(context)
      if context.config && context.config[:authed]
        # Do your auth logic here
      end
      call_next context
    end
  end

Edit overall i think this adds quite a bit of flexibility as the same handler can be used for multiple routes, but the config can slightly change the logic of each handler, reducing the amount of handlers required.

@straight-shoota
Copy link
Contributor

I wouldn't put too much logic in the routing layer, that's what handlers are for.

@Blacksmoke16
Copy link
Contributor

Blacksmoke16 commented Jul 4, 2018

The main logic would still live in the handler. The config by itself doesn't do anything unless a handler is setup to utilize it. Just adds some added flexibility without having to keep an array up to date or a handler specific to one senario.

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

No branches or pull requests

5 participants