Open qszhu opened 5 years ago
vladfaust
commented
5 years ago 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
commented
5 years ago 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.listenwhen compiling:
It seems the status_message in onyx prevented the type of status_message in halite to be inferred.
vladfaust
commented
5 years ago Some wild guesses from me:
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
endYou 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 :wink:
qszhu
commented
5 years ago 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
endStill, this feels hacky. Should I report this to halite?
P.S. Nice trick about the <details> tag. 😄
vladfaust
commented
5 years ago @qszhu,
Should I report this to halite?
No, I don't think so. It's Onyx's issue.
vladfaust
commented
5 years ago 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
endThis 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
commented
5 years ago 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
endI think a scoped (within Onyx's namespace) exception class is better, so that it doesn't interfere with other classes with a same name.
both defined a
status_message:How is this usually resolved in Crystal?