name clash with halite

[qszhu]

both defined a status_message:

• in onyx
• in halite

How is this usually resolved in Crystal?

[vladfaust]

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]

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:

Details

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]

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]

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]

@qszhu,

Should I report this to halite?

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

[vladfaust]

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]

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]

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.