Receiving a PATCH with an Array inside the "data"-key should be valid

[ddefrenne]

Expected behavior vs actual behavior

• Expected: Receiving a PATCH with an Array inside the "data"-key is valid, according to http://jsonapi.org/format/#crud-updating-relationships
• Actual: It throws an error ActiveModelSerializers::Adapter::JsonApi::Deserialization::InvalidDocument: Invalid payload ({:data => "Expected Hash"})

Steps to reproduce

PATCH to /articles/1/relationships/comments with

{
  "data": [
    { "type": "comments", "id": "2" },
    { "type": "comments", "id": "3" }
  ]
}

Environment

ActiveModelSerializers Version (commit ref if not on tag): 0.10.2

Output of ruby -e "puts RUBY_DESCRIPTION": ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-darwin15]

OS Type & Version: Mac OS X 10.11.6

Integrated application and version (e.g., Rails, Grape, etc): Rails 4.2.7.1

Backtrace

(e.g., provide any applicable backtraces from your application)

Additonal helpful information

(e.g., Gemfile.lock, configurations, PR containing a failing test, git bisect results)

[NullVoxPopuli]

IMO, document validation shouldn't be the responsibility of AMS. @beauby this would be a great opportunity to add a native extension to your jsonapi gem :-)

But anyway, @ddefrenne, do you have time to submit a failing test as a PR?

[richmolj]

I'd actually like a little clarification around expected versus actual behavior. It sounds like an invalid payload is being submitted, and we raise an error saying an invalid payload was submitted. This sounds like the correct behavior to me, is the suggestion we accomodate invalid jsonapi by not throwing an error?

[NullVoxPopuli]

@richmolj according to the jsonapi link the given patch payload is valid json api. It's just not a resource or document.

[richmolj]

Oh, my mistake, thanks for correcting me. You gotta scroll up from the given link to find that example.

Is there a reason we don't rely on https://github.com/beauby/jsonapi for this functionality?

[NullVoxPopuli]

oh hey: he already has a validation function https://github.com/beauby/jsonapi/blob/master/lib/jsonapi/document.rb#L37

we should use that.

[richmolj]

@NullVoxPopuli agreed.

Though separately @ddefrenne could you send us a backtrace? It looks like there is already a passing test for this and I can't tell what would throw that error.

Are you sure you aren't passing a raw array that is not nested under data? eg

{
  relationships: {
    comments: []
  }
}

versus

{
  relationships: {
    comments: { data: [] }
  }
}

[richmolj]

@NullVoxPopuli #1927 to use jsonapi gem instead

[ddefrenne]

@richmolj The data Array I supplied is not one that is to be nested in "relationships", but is at the top level. When you send a request to an endpoint for a specific relationship to a resource ({resource}/{id}/relationships/{relationship-resource}), it should allow sending an Array of them (in this case comments).

[ddefrenne]

This is the part of the spec I'm refering to:

[NullVoxPopuli]

There is opportunity here to make a gem that automatically adds relationship endpoints to every resource. :-)

[richmolj]

@ddefrenne thanks for pointing me in the right direction, we're on the same page now.

Here's a failing test for this. Though @NullVoxPopuli I still favor moving this to the jsonapi gem.

[richmolj]

@ddefrenne could you verify the expected behavior would be a parsed document like:

[ { id: 2 }, { id: 3 } ]

[NullVoxPopuli]

@richmolj if you change the validation code to use the jsonapi gem's validation? does that test pass?

[ddefrenne]

@richmolj Looks ok. Is there a reason why the "type" gets ignored? You can deduce it from the route, but I like to see it as an extra check that users are accessing the right endpoint.

[richmolj]

@NullVoxPopuli the validation will not fail, but I think the actual parsing code would. Would the parsing fail if it, too, were moved to jsonapi gem? Unsure. They work slightly differently so I'd have to revisit this later.

@ddefrenne I'm just following the existing behavior. That said, I think you could make the case that the existing behavior is not good enough when dealing with arrays. The problem is enhancing the existing behavior would be a breaking change...

[NullVoxPopuli]

@richmolj parsing checks if the document is valid, so parsing should if the validation check is changed.

[richmolj]

Unless maybe we added something like ActiveModelSerializers::Adapter::JsonApi::Deserialization.parse(document, root: true)?

[NullVoxPopuli]

no more roots!

[richmolj]

parsing checks if the document is valid, so parsing should if the validation check is changed.

Well, we're changing the expected contract of the parse code, so it would no longer work. Same as if we changed the validation check to be an empty method. Here's where the failure would happen (code expects a hash but given an array)