Give a clearer error message when payload doesn't match with provided content-type

[khiav223577]

Let we are not support application/json content-type.
When user send a request with wrong content-type, the response message is clear:

{
  "error" => "The provided content-type 'application/json' is not supported"
}

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 not json),
the response message will be strange:

{
  "error" => "Empty input (after ) at line 1, column 1 [parse.c:1072] in '......"
}

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:

grape/lib/grape/middleware/formatter.rb

Lines 115 to 116 in f5d9831

	rescue StandardError => e
	throw :error, status: 400, message: e.message, backtrace: e.backtrace, original_exception: e

grape/lib/grape/middleware/error.rb

Lines 38 to 40 in f5d9831

	error_response(catch(:error) do
	return @app.call(@env)
	end)

[dm1try]

it looks like JSON parser used in your example does not raise ParseError for some reason otherwise the error would be handled here:

grape/lib/grape/parser/json.rb

Lines 8 to 11 in edc6abe

	::Grape::Json.load(object)
	rescue ::Grape::Json::ParseError
	# handle JSON parsing errors via the rescue handlers or provide error message
	raise Grape::Exceptions::InvalidMessageBody.new('application/json')

could you ensure that the parser raises the error mentioned above?

[khiav223577]

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

[dblock]

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:

1. Document how to swap JSON parser for OJ (I found others doing it like so for example).
2. Add integration tests that use oj.
3. Reproduce the issue here (the wrong error message) in a test.
4. Discuss how we can fix it. I think an explicit rescue would probably be easier to grok vs. the proposal above.

WDYT?

[dm1try]

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 :).

[dblock]

Documentation and tests, I like it!

😎

[khiav223577]

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 :)

[dblock]

@khiav223577 Care to contribute to the README?

[khiav223577]

@khiav223577 Care to contribute to the README?

I'm willing to :)

[khiav223577]

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

[dm1try]

@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!