name clash with halite #78

qszhu · 2019-05-24T10:48:17Z

both defined a status_message:

How is this usually resolved in Crystal?

The text was updated successfully, but these errors were encountered:

vladfaust · 2019-05-24T11:02:02Z

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 · 2019-05-24T14:17:10Z

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 · 2019-05-24T16:30:32Z

Some wild guesses from me:

Try requiring halite before onyx/http

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 · 2019-05-26T11:39:26Z

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 · 2019-05-27T16:59:19Z

@qszhu,

Should I report this to halite?

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

vladfaust · 2019-05-28T20:49:02Z

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 · 2019-05-28T20:50:25Z

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 · 2019-05-29T12:55:25Z

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.

vladfaust added the design flaw Something's poorly designed label May 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

name clash with halite #78

name clash with halite #78

qszhu commented May 24, 2019

vladfaust commented May 24, 2019 •

edited

Loading

qszhu commented May 24, 2019 •

edited by vladfaust

Loading

vladfaust commented May 24, 2019 •

edited

Loading

qszhu commented May 26, 2019

vladfaust commented May 27, 2019

vladfaust commented May 28, 2019

vladfaust commented May 28, 2019

qszhu commented May 29, 2019

name clash with halite #78

name clash with halite #78

Comments

qszhu commented May 24, 2019

vladfaust commented May 24, 2019 • edited Loading

qszhu commented May 24, 2019 • edited by vladfaust Loading

vladfaust commented May 24, 2019 • edited Loading

qszhu commented May 26, 2019

vladfaust commented May 27, 2019

vladfaust commented May 28, 2019

vladfaust commented May 28, 2019

qszhu commented May 29, 2019

vladfaust commented May 24, 2019 •

edited

Loading

qszhu commented May 24, 2019 •

edited by vladfaust

Loading

vladfaust commented May 24, 2019 •

edited

Loading