Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include intermediate resources on compound documents #50

Merged
merged 1 commit into from
Feb 20, 2016

Conversation

jcarlos
Copy link
Contributor

@jcarlos jcarlos commented Feb 18, 2016

Please see #49

@fotinakis
Copy link
Owner

Looks good, except for the big blocks of unformatted hashes. :) See the current style and way that tests are structured in the file, it should be pretty easy to update near comment Only the author is included: to include other objects (something like Intermediates are included: long-comments, long-comments.post, and long-comments.post.author). If you could update the comments as well, that would be great.

@jcarlos
Copy link
Contributor Author

jcarlos commented Feb 18, 2016

No worries @fotinakis , I'll do the updates :)

Did you mean I should just format the hashes or to remove the big hashes and replace them with something like the code below?

expected_data = {
        'data' => serialize_primary(post, {serializer: MyApp::PostSerializer}),
        'included' => [
          serialize_primary(post.long_comments.first, {serializer: MyApp::LongCommentSerializer}),
          serialize_primary(post.long_comments.last, {serializer: MyApp::LongCommentSerializer}),   
          serialize_primary(post, { serializer: MyApp::PostSerializer }),
          serialize_primary(post.author, {serializer: MyAppOtherNamespace::UserSerializer})
        ],
      }

I have tried this option first and then I replaced it with the direct big hash because expected_data was missing the "data" part inside the relationship of each include, ex:

{
  "relationships"=> {
    "user"=> {
      "links"=> {
        "self"=>"/long-comments/2/relationships/user", 
         "related"=>"/long-comments/2/user"
      },
      "data"=> { # this data bit was missing for each included resource
        "type"=>"users", 
        "id"=>"2"
      }
    }
  }
}  

Maybe there is some special option that would include this "data" bit in there when using serialize_primary but I couldn't find it.

@jcarlos jcarlos force-pushed the issue-49 branch 2 times, most recently from b47fdb3 to b08f9da Compare February 18, 2016 12:07
@jcarlos
Copy link
Contributor Author

jcarlos commented Feb 18, 2016

By the way, I have already included the comment you asked, formatted the hash and squashed my commits in case that was the only thing missing :)

@fotinakis
Copy link
Owner

Sorry I meant getting it to use the serialize_primary method like the rest of the tests, not formatting the hash. The reasoning here is to to tightly control what the tests expect and make sure we know exactly what the format is supposed to be.

The data issue you hit was correct, because were now including all intermediates along the way, each included object needs to have data to provide full linkage. You can get serialize_primary to include data like this (from another test):

      serialize_primary(post, {
        serializer: MyApp::PostSerializer,
        include_linkages: ['author'],
      })

@jcarlos
Copy link
Contributor Author

jcarlos commented Feb 19, 2016

Got it, thanks for letting me know about the include_linkages. I have updated the specs accordingly, please have a look at the updated pull request @fotinakis

@fotinakis
Copy link
Owner

This looks great! 👍

I'll merge this tomorrow, just need to think through the correct way to notate the change and I want to test it with my own app. I think this is relatively safe because 1) it brings it into compatibility with the JSON:API spec, and 2) it's just adding more data to compound documents, not removing anything or changing a format, so I believe this is backwards compatible and we can classify it a "bugfix".

fotinakis added a commit that referenced this pull request Feb 20, 2016
Include intermediate resources on compound documents
@fotinakis fotinakis merged commit b013ab4 into fotinakis:master Feb 20, 2016
@fotinakis
Copy link
Owner

Released in gem v0.6.1, thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants