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

name clash with halite #78

Open
qszhu opened this issue May 24, 2019 · 8 comments
Open

name clash with halite #78

qszhu opened this issue May 24, 2019 · 8 comments
Labels
design flaw Something's poorly designed

Comments

@qszhu
Copy link

qszhu commented May 24, 2019

both defined a status_message:

How is this usually resolved in Crystal?

@vladfaust
Copy link
Member

vladfaust commented May 24, 2019

Did it cause any troubles? Halite's APIError should have it's own status_message method, which would override Exception's.

Anyway, I think that Onyx should not monkey-patch Exception in this manner.

@qszhu
Copy link
Author

qszhu commented May 24, 2019

to reproduce:

require "onyx/http"
require "halite"

Onyx::HTTP.get "/" do |env|
  env.response << "Hello, Onyx!"
end

Onyx::HTTP.get "/proxy" do |env|
  r = Halite.get("http://localhost:5000")
  env.response << r.body
end

Onyx::HTTP.listen

when compiling:

Error in test.cr:13: expanding macro

Onyx::HTTP.listen
           ^~~~~~

in test.cr:13: expanding macro

Onyx::HTTP.listen
^

in macro 'listen' /Users/qinsi/dev/wechat/tracking/lib/onyx/src/onyx/http.cr:67, line 5:

   1.     handlers = Onyx::HTTP::Singleton.instance.handlers()
   2.     
   3.     server = Onyx::HTTP::Server.new(handlers, logger: Onyx.logger)
   4.     server.bind_tcp("127.0.0.1", 5000, reuse_port: false)
>  5.     server.listen
   6.   

instantiating 'Onyx::HTTP::Server#listen()'
in lib/onyx-http/src/onyx-http/server.cr:71: instantiating 'super()'

      super
      ^~~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server.cr:415: instantiating 'Array(Socket::Server)#each()'

    @sockets.each do |socket|
             ^~~~

in /usr/local/Cellar/crystal/0.28.0/src/indexable.cr:187: instantiating 'each_index()'

    each_index do |i|
    ^~~~~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/indexable.cr:187: instantiating 'each_index()'

    each_index do |i|
    ^~~~~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server.cr:415: instantiating 'Array(Socket::Server)#each()'

    @sockets.each do |socket|
             ^~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server.cr:428: expanding macro

            spawn handle_client(_io)
            ^

in macro 'spawn' /usr/local/Cellar/crystal/0.28.0/src/concurrent.cr:97, line 11:

   1.   
   2. 
   3.   
   4.     ->(
   5.       
   6.         __arg0 : typeof(_io),
   7.       
   8.       
   9.       ) {
  10.       spawn(name: nil) do
> 11.         handle_client(
  12.           
  13.             __arg0,
  14.           
  15.           
  16.         )
  17.       end
  18.     
  19.       }.call(_io)
  20.     
  21.   

instantiating 'handle_client(IO+)'
in /usr/local/Cellar/crystal/0.28.0/src/http/server.cr:462: instantiating 'HTTP::Server::RequestProcessor#process(IO+, IO+)'

    @processor.process(io, io)
               ^~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/request_processor.cr:16: instantiating 'process(IO+, IO+, IO::FileDescriptor)'

  def process(input, output, error = STDERR)
  ^

in /usr/local/Cellar/crystal/0.28.0/src/http/server/request_processor.cr:39: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'

          @handler.call(context)
                   ^~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handlers/compress_handler.cr:12: expanding macro

    {% if flag?(:without_zlib) %}
    ^

in macro 'macro_4625847840' /usr/local/Cellar/crystal/0.28.0/src/http/server/handlers/compress_handler.cr:12, line 12:

   1. 
   2.       request_headers = context.request.headers
   3. 
   4.       if request_headers.includes_word?("Accept-Encoding", "gzip")
   5.         context.response.headers["Content-Encoding"] = "gzip"
   6.         context.response.output = Gzip::Writer.new(context.response.output, sync_close: true)
   7.       elsif request_headers.includes_word?("Accept-Encoding", "deflate")
   8.         context.response.headers["Content-Encoding"] = "deflate"
   9.         context.response.output = Flate::Writer.new(context.response.output, sync_close: true)
  10.       end
  11. 
> 12.       call_next(context)
  13.     

instantiating 'call_next(HTTP::Server::Context)'
in /usr/local/Cellar/crystal/0.28.0/src/http/server/handler.cr:26: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'

      next_handler.call(context)
                   ^~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handlers/error_handler.cr:15: instantiating 'call_next(HTTP::Server::Context)'

      call_next(context)
      ^~~~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handler.cr:26: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'

      next_handler.call(context)
                   ^~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handlers/log_handler.cr:11: instantiating 'Time.class#measure()'

    elapsed = Time.measure { call_next(context) }
                   ^~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handlers/log_handler.cr:11: instantiating 'Time.class#measure()'

    elapsed = Time.measure { call_next(context) }
                   ^~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handlers/log_handler.cr:11: instantiating 'call_next(HTTP::Server::Context)'

    elapsed = Time.measure { call_next(context) }
                             ^~~~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handler.cr:26: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'

      next_handler.call(context)
                   ^~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handlers/static_file_handler.cr:30: instantiating 'call_next(HTTP::Server::Context)'

        call_next(context)
        ^~~~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handler.cr:26: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'

      next_handler.call(context)
                   ^~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handlers/websocket_handler.cr:47: instantiating 'call_next(HTTP::Server::Context)'

      call_next(context)
      ^~~~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handler.cr:26: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'

      next_handler.call(context)
                   ^~~~

in lib/onyx-http/src/onyx-http/middleware/cors.cr:62: instantiating 'call_next(HTTP::Server::Context)'

          call_next(context)
          ^~~~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handler.cr:26: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'

      next_handler.call(context)
                   ^~~~

in lib/onyx-http/src/onyx-http/middleware/logger.cr:69: instantiating 'call_next(HTTP::Server::Context)'

        call_next(context)
        ^~~~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/http/server/handler.cr:26: instantiating '(HTTP::Handler | Proc(HTTP::Server::Context, Nil))#call(HTTP::Server::Context)'

      next_handler.call(context)
                   ^~~~

in lib/onyx-http/src/onyx-http/middleware/renderer.cr:36: instantiating 'Array(MIME::MediaType)#each()'

            accept.each do |a|
                   ^~~~

in /usr/local/Cellar/crystal/0.28.0/src/indexable.cr:187: instantiating 'each_index()'

    each_index do |i|
    ^~~~~~~~~~

in /usr/local/Cellar/crystal/0.28.0/src/indexable.cr:187: instantiating 'each_index()'

    each_index do |i|
    ^~~~~~~~~~

in lib/onyx-http/src/onyx-http/middleware/renderer.cr:36: instantiating 'Array(MIME::MediaType)#each()'

            accept.each do |a|
                   ^~~~

in lib/onyx-http/src/onyx-http/middleware/renderer.cr:42: instantiating 'render_json_error(HTTP::Server::Context, Exception+)'

                render_json_error(context, error)
                ^~~~~~~~~~~~~~~~~

in lib/onyx-http/src/onyx-http/middleware/renderer.cr:130: instantiating 'Exception+#status_message()'

            name:    error.status_message,
                           ^~~~~~~~~~~~~~

in lib/halite/src/halite/error.cr:52: expanding macro

      getter status_message
      ^

in macro 'getter' expanded macro: macro_4459236080:117, line 5:

   1.       
   2.         
   3.           
   4.             def status_message
>  5.               @status_message
   6.             end
   7.           
   8.         
   9.       
  10.     

Can't infer the type of instance variable '@status_message' of Halite::Exception::ServerError

The type of a instance variable, if not declared explicitly with
`@status_message : Type`, is inferred from assignments to it across
the whole program.

The assignments must look like this:

  1. `@status_message = 1` (or other literals), inferred to the literal's type
  2. `@status_message = Type.new`, type is inferred to be Type
  3. `@status_message = Type.method`, where `method` has a return type
     annotation, type is inferred from it
  4. `@status_message = arg`, with 'arg' being a method argument with a
     type restriction 'Type', type is inferred to be Type
  5. `@status_message = arg`, with 'arg' being a method argument with a
     default value, type is inferred using rules 1, 2 and 3 from it
  6. `@status_message = uninitialized Type`, type is inferred to be Type
  7. `@status_message = LibSome.func`, and `LibSome` is a `lib`, type
     is inferred from that fun.
  8. `LibSome.func(out @status_message)`, and `LibSome` is a `lib`, type
     is inferred from that fun argument.

Other assignments have no effect on its type.

Can't infer the type of instance variable '@status_message' of Halite::Exception::ServerError

It seems the status_message in onyx prevented the type of status_message in halite to be inferred.

@vladfaust
Copy link
Member

vladfaust commented May 24, 2019

Some wild guesses from me:

  1. Try requiring halite before onyx/http

  2. Monkey patch Halite's error so @status_message has type:

    module Halite
      module Exception
        class APIError < ResponseError
          getter status_message : String
        end
      end
    end  

    or

    module Halite
      module Exception
        class APIError < ResponseError
          @status_message : String
        end
      end
    end

    You could also experiment with the order — it should be possible to monkey-patch Halite before actually requiring it.

P.S: You can use <details> tag to hide big chunks of code in your comments 😉

@qszhu
Copy link
Author

qszhu commented May 26, 2019

Changing require order doesn't work. Monkey-patching works, but have to define it as nil-able:

module Halite
  module Exception
    class APIError < ResponseError
      getter status_message : String | Nil
    end
  end
end

Still, this feels hacky. Should I report this to halite?

P.S. Nice trick about the <details> tag. 😄

@vladfaust
Copy link
Member

@qszhu,

Should I report this to halite?

No, I don't think so. It's Onyx's issue.

@vladfaust vladfaust added the design flaw Something's poorly designed label May 27, 2019
@vladfaust
Copy link
Member

It's not that simple.

Onyx rescues all Exceptions. If server is in verbose mode, the exception name is printed into a response, formatted. For example, Errors::MyCustomError is printed as 500 My Custom Error, and IndexOutOfBounds as 500 Index Out Of Bounds.

This is the extension used in Onyx:

class Exception
  # The status message for this error. Returns its class name decorated as
  # an HTTP status message, for example `"User Not Found"` for
  # `MyEndpoint::UserNotFound` error.
  def status_message
    {{@type.name.split("::").last.underscore.split("_").map(&.capitalize).join(" ")}}
  end
end

This way the exception's formatted name is baked into the binary. There are plenty of places where error.status_message is used, and replacing the baked string with runtime manipulation would affect performance.

It seems like a good idea to rename the method to http_status_message, but there is no guarantee of clash absence in the future.

@vladfaust
Copy link
Member

Another option would be wrapping all rescued exceptions into a generic class:

class Onyx::HTTP::Exception(T)
  # The status message for this exception. Returns `T` class name decorated as
  # an HTTP status message, for example `"User Not Found"` for
  # `Onyx::HTTP::Exception(MyEndpoint::UserNotFound)`.
  def status_message
    {{T.name.split("::").last.underscore.split("_").map(&.capitalize).join(" ")}}
  end
end

class HTTP::Server::Response
  # A rescued error which is likely to be put into the response output.
  property error : Onyx::HTTP::Exception?

  def reset
    previous_def
    @error = nil
  end
end

@qszhu
Copy link
Author

qszhu commented May 29, 2019

I think a scoped (within Onyx's namespace) exception class is better, so that it doesn't interfere with other classes with a same name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design flaw Something's poorly designed
Projects
None yet
Development

No branches or pull requests

2 participants