onyxframework / http

An opinionated framework for scalable web 🌎
https://onyxframework.com/http
MIT License
142 stars 11 forks source link

included params macro not merged? #81

Closed qszhu closed 5 years ago

qszhu commented 5 years ago

I'm trying to do some check in each request:

require "onyx/http"

struct Details
  include Onyx::HTTP::Endpoint

  params do
    path do
      type id : String
    end

    query do
      type token : String
    end
  end

  def call
    pp params.query.token
    # do auth with token
    # ...
  end
end

Onyx::HTTP.get "/item/:id", Details

Onyx::HTTP.listen

To check:

$ http localhost:5000/item/1?token=test

This works. To reuse some code, I extracted a module:

require "onyx/http"

module Auth
  macro included
    params do
      query do
        type token : String
      end
    end

    before do
      pp params.query.token
      # do auth with token
    end
  end
end

struct Details
  include Onyx::HTTP::Endpoint
  include Auth

  params do
    path do
      type id : String
    end
  end

  def call
    # ...
  end
end

Onyx::HTTP.get "/item/:id", Details

Onyx::HTTP.listen

This one crashes.

Output ``` Invalid memory access (signal 11) at address 0x4 [0x10d1af66b] *CallStack::print_backtrace:Int32 +107 [0x10d183375] __crystal_sigfault_handler +181 [0x7fff5f894b5d] _sigtramp +29 [0x10d18d375] *String#empty?:Bool +5 [0x10d19415e] *String#pretty_print:Nil +222 [0x10d26fe23] *PrettyPrint::format:IO::FileDescriptor +99 [0x10d26fdad] *PrettyPrint::format:IO::FileDescriptor +61 [0x10d1886a2] *pp:String +34 [0x10d26ea5d] *Details#before:String +13 [0x10d26e8dd] *Details::call:Nil +77 [0x10d188354] ~procProc(HTTP::Server::Context, Nil)@lib/onyx-http/src/onyx-http/middleware/router.cr:170 +20 [0x10d188726] ~procProc(HTTP::Server::Context, Nil)@lib/onyx-http/src/onyx-http/middleware/router.cr:150 +70 [0x10d265bdd] *Onyx::HTTP::Middleware::Router#call:(Array(String) | Bool | IO+ | Int32 | Nil) +749 [0x10d28a6f6] *Onyx::HTTP::Middleware::Rescuer::Silent(Onyx::HTTP::Error(Code))+@HTTP::Handler#call_next:(Array(String) | Bool | IO+ | Int32 | Nil) +1686 [0x10d28996c] *Onyx::HTTP::Middleware::Rescuer::Silent(Onyx::HTTP::Error(Code))+@Onyx::HTTP::Middleware::Rescuer(T)#call:(Array(String) | Bool | IO+ | Int32 | Nil) +44 [0x10d281cf8] *Onyx::HTTP::Middleware::Rescuer::Standard(Exception+)@HTTP::Handler#call_next:(Array(String) | Bool | IO+ | Int32 | Nil) +1192 [0x10d28111c] *Onyx::HTTP::Middleware::Rescuer::Standard(Exception+)@Onyx::HTTP::Middleware::Rescuer(T)#call:(Array(String) | Bool | IO+ | Int32 | Nil) +44 [0x10d280be2] *Onyx::HTTP::Middleware::CORS@HTTP::Handler#call_next:(Array(String) | Bool | IO+ | Int32 | Nil) +1106 [0x10d2804a9] *Onyx::HTTP::Middleware::CORS#call:(Array(String) | Bool | IO+ | Int32 | Nil) +345 [0x10d27fb85] *Onyx::HTTP::Middleware::Logger@HTTP::Handler#call_next:(Array(String) | Bool | IO+ | Int32 | Nil) +693 [0x10d27e55a] *Onyx::HTTP::Middleware::Logger#call:(Array(String) | Bool | IO+ | Int32 | Nil) +2170 [0x10d27d703] *Onyx::HTTP::Middleware::RequestID@HTTP::Handler#call_next:(Array(String) | Bool | IO+ | Int32 | Nil) +771 [0x10d27d3d4] *Onyx::HTTP::Middleware::RequestID#call:(Array(String) | Bool | IO+ | Int32 | Nil) +244 [0x10d27ce28] *Onyx::HTTP::Middleware::ResponseTime@HTTP::Handler#call_next:(Array(String) | Bool | IO+ | Int32 | Nil) +936 [0x10d27c9f2] *Onyx::HTTP::Middleware::ResponseTime#call:Array(String) +66 [0x10d28d2ad] *HTTP::Server::RequestProcessor#process:Nil +3053 [0x10d28c6b5] *HTTP::Server::RequestProcessor#process:Nil +53 [0x10d283644] *Onyx::HTTP::Server@HTTP::Server#handle_client:Nil +292 [0x10d18996b] ~procProc(Nil)@/usr/local/Cellar/crystal/0.28.0/src/http/server.cr:428 +27 [0x10d1c6b1f] *Fiber#run:(IO::FileDescriptor | Nil) +95 [0x10d182b29] ~proc2Proc(Fiber, (IO::FileDescriptor | Nil))@/usr/local/Cellar/crystal/0.28.0/src/fiber.cr:47 +9 ```

Is this a limitation of Crystal? Or am I just doing it the wrong way?

vladfaust commented 5 years ago

It's just not designed for that. You have to define token param explicitly in every endpoint. However, shared before block should work even with params.query.token in it.

qszhu commented 5 years ago

I used context.request.query as a workaround for now. What I was thinking is that such "middleware"s should take care of their interested params respectively, so that these params don't clutter with the ones required by real business logic.

On a second thought, I think what you said is also beneficial. Listing all the params explicitly in an endpoint could make maintenance much easier.