Open khiav223577 opened 2 years ago
it looks like JSON parser used in your example does not raise ParseError
for some reason otherwise the error would be handled here:
https://github.com/ruby-grape/grape/blob/edc6abef69054d61fdcd77ee7e054c8afc4f4c5e/lib/grape/parser/json.rb#L8-L11
could you ensure that the parser raises the error mentioned above?
Thanks @dm1try.
I found out that we assign Oj
to Grape::Json
in our project and we had set the default mode as compat
.
In this mode, it will raise EncodingError
instead of Oj::ParseError
when the format of string is wrong.
Oj.load('d:', mode: :object)
# => Oj::ParseError (unexpected character (after ) at line 1, column 1 [parse.c:804])
Oj.load('d:', mode: :compat)
# => EncodingError (Empty input (after ) at line 1, column 1 [parse.c:1116] in 'd:)
Could we define ParseError
constant under Grape
namespace instead of Grape::Json
namespace? So that we can have better control of which exception should be caught, and we will not have to define constant under other gems' namespace which may pollute them and encounter name collision. Take Oj
for example, Oj::ParseError
have already been defined, we cannot redefine it as EncodingError
.
Pseudo code:
module Grape
if Object.const_defined? :MultiJson
Json = ::MultiJson
+ JsonParseError = Json::ParseError
else
Json = ::JSON
- Json::ParseError = Json::ParserError
+ JsonParseError = Json::ParserError
end
end
module Grape
module Parser
module Json
class << self
def call(object, _env)
::Grape::Json.load(object)
- rescue ::Grape::Json::ParseError
+ rescue ::Grape::JsonParseError
# handle JSON parsing errors via the rescue handlers or provide error message
raise Grape::Exceptions::InvalidMessageBody.new('application/json')
end
end
end
end
end
For someone who may want to replace the parser with Oj
, they can redefine the constant:
module Grape
Json = ::Oj
JsonParseError = ::EncodingError
end
Moving ::Grape::Json::ParseError
iinto ::Grape::ParseError
would be a breaking change and would likely need to have some backwards compat. Not sure it would be worth it. I propose a few things:
rescue
would probably be easier to grok vs. the proposal above. WDYT?
Documentation and tests, I like it!
@dblock agreed, the issue is about how to swap JSON parser and do not break anything(btw, the example you provided is about formatter but not a parser).
In the issue above a new parser was set implicitly by assigning Oj
to Grape::Json
relying on Grape
internals. But Grape::Json
is some adapter around multijson
and standard json
, so I do not think that we need to touch its interface at all. There is the documentation about custom parsers here, so I would start with it in mind:
class OjCompactParser
def self.call(object, _env)
Oj.load(object, mode: :compat)
rescue EncodingError
raise Grape::Exceptions::InvalidMessageBody.new('application/json')
end
end
class API < Grape::API
parser :json, OjCompactParser
...
end
@khiav223577 I'm not sure how EncodingError
is different from ParseError
but if you will not able to use a custom parser for some reason you can use some wrapper around Oj that mimics a needed interface:
class OjCompact
def self.load(object)
Oj.load(object, mode: :compat)
rescue EncodingError
raise Oj::ParseError
end
end
Grape::Json = OjCompact
But I would stick with a public interface in the first example(you should try if it works :).
Documentation and tests, I like it!
😎
class OjCompactParser def self.call(object, _env) Oj.load(object, mode: :compat) rescue EncodingError raise Grape::Exceptions::InvalidMessageBody.new('application/json') end end class API < Grape::API parser :json, OjCompactParser ... end
@dm1try thanks for your reply. The first example looks great, I'll try and see if it works or not :)
@khiav223577 Care to contribute to the README?
@khiav223577 Care to contribute to the README?
I'm willing to :)
Hi @dm1try,
Since there are many other places besides formatter that use Grape::Json.load
and Grape::Json.parse
, the first example doesn't satisfy all of our needs. The following way is more general. How do you think?
module CustomGrapeParser
ParseError = ::EncodingError
class << self
def load(object)
Oj.load(object, mode: :compat)
end
def dump(object)
Oj.dump(object, mode: :compat)
end
end
end
Grape::Json = CustomGrapeParser
@khiav223577
In that case, I think your example is the most suitable solution.
But we know that such interface hasn't been documented yet and it looks like it does not have any specs, so it can be broken easily. Feel free to add the example to README
and/or add some integration spec :)
Thanks!
Let we are not support
application/json
content-type. When user send a request with wrong content-type, the response message is clear:Let we support
application/json
content-type. When user send a request with correct content-type, but with wrong payload (the format of it is notjson
), the response message will be strange:It will be nice if we can provide clearer error message. But it seems to be impossible to add custom messages without overriding codes in the gem.
The error is throwed and rescued inside:
https://github.com/ruby-grape/grape/blob/f5d9831bac2e2dd439d0f3901797995a91139690/lib/grape/middleware/formatter.rb#L115-L116 https://github.com/ruby-grape/grape/blob/f5d9831bac2e2dd439d0f3901797995a91139690/lib/grape/middleware/error.rb#L38-L40