Error response behaviour change in the JSON formatter

[fredwu]

Hi,

As I noted in this commit, there are some behavioural changes that are not very obvious and perhaps should either be reverted or documented.

Essentially, in ErrorFormatter::Json, the old behaviour (our app was on grape v0.10.1) was to:

• pass on a Hash as it is, or;
• wrap the error object (can be a string or array, etc - anything other than a hash) with { error: original_error_object }

The commit linked has unfortunately changed this behaviour to be:

• wrap the error object with { error: original_error_object } ONLY IF the error object is a String, or;
• anything else (a hash or an array, etc) will be passed on as they are

I have checked the test changes in the same commit, as well as the changelog, and I could not see any obvious reference to this behavioural change.

I would love to know more about the intention behind this change, and see we either revert or document this change accordingly.

Hope that makes sense. Thanks everyone!

[fredwu]

Forgot to tag the original PR. #889

[fredwu]

cc @dblock @sunnyrjuneja :)

[dblock]

I believe the behavior is intentional, we want to pass through things as much as possible. This is internal, but I'd be OK to document it in UPGRADING, please PR.

[fredwu]

@dblock Hi, thanks for the prompt response again.

Interestingly, after the newly released v0.15.0 (released yesterday shortly after this discussion), we noticed the behaviour has changed yet again, with no mention in the upgrading doc. 😿

To recap and summarise the different behaviours:

Original (circa 0.10.1) link

• pass on error object as it is:
  • Hash
• wrap error object with error key:
  • anything other than Hash

Changed (circa 0.14.0) link

• pass on error object as it is:
  • anything other than String
• wrap error object with error key:
  • String

New (0.15.0) link

• pass on error object as it is:
  • Hash or Exceptions::ValidationErrors
• wrap error object with error key:
  • anything other than Hash or Exceptions::ValidationErrors

So the funny side is that after v0.15.0 we no longer needed to money patch the JSON error formatter as our Array-based error objects are now working again. 😉 But what's concerning to me is the fact that it was mentioned that the behavioural changes were intentional, when in fact it wasn't according to this commit which was done way before this whole discussion.

[dblock]

I am not sure what you're trying to say @fredwu. For internal APIs we don't really discuss changes the same way as for public APIs. The issue you link fixed a bug, specifically #1037 and had nothing to do with the internal format of anything. What am I missing?

[fredwu]

@dblock I understand the formatter itself is internal, but unless I'm missing something - the fact that the final output json is different between different grape versions means that it has material impact to users, right? And #1037 is considered a bug for exactly this reason, no? :)

[dblock]

Yes, you're right @fredwu. Help us get to the bottom of it.

[wleeper]

With the change in #1037 now if you pass in

error!(
  {
    somekey: 'some value',
    with: Entities::Errors
  }, 500)

It wraps the resulting message formatted with Entities::Errors with error: { result of Entity Model }.

This is because the if statement is looking at .is_a(Hash) and apparently (unverified) the response from present method being compared to is_a(Hash) is not in fact a Hash, but most likely some Grape::Entity class.

[dblock]

What do we want to do with this? @fredwu?