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

Adjusts Deserializer relationship parsing to allow links/meta keys #2193

Open
wants to merge 1 commit into
base: 0-10-stable
Choose a base branch
from

Conversation

nikz
Copy link

@nikz nikz commented Sep 18, 2017

Purpose

The JSON API spec allows for the relationships key to include one of data, links or meta.

The current parsing logic for the deserialiser raises an exception unless a data key is present (this may have been changed after the code was written 🚄), which means that we're getting an exception on a valid JSON:API document 😭

Changes

  • Adjusts the document validation inside the Deserializer to allow for one of data, links, or meta as per the spec.
  • Ignores the links or meta keys when parsing the relationship into something to be used by Rails.

Caveats

It may not be the best behaviour to ignore the keys silently - however, I'm not really sure what a useful thing to do here would be. Perhaps print a warning?

Related GitHub issues

None 😺

Additional helpful information

Thanks for the 💎, and your time reviewing this folks!

* Allows specifying links/meta keys as per spec
* Ignores those keys when parsing the relationship
@bf4
Copy link
Member

bf4 commented Sep 19, 2017

@beauby any objections?

@bf4 bf4 closed this Nov 19, 2017
@nikz
Copy link
Author

nikz commented Nov 20, 2017

@bf4 I think this still a change that's needed - is there a reason why you closed the PR? I'm not sure the test failures are related to the changes I've made but I could update to latest 0-10-stable and attempt to get them passing?

@bf4
Copy link
Member

bf4 commented Nov 20, 2017

@nikz Not sure how or why I closed it

@bf4 bf4 reopened this Nov 20, 2017
@nikz
Copy link
Author

nikz commented Nov 22, 2017

@bf4 slip of the mouse! No worries at all. Is there anything you need me to do on this patch?

@bf4
Copy link
Member

bf4 commented Nov 26, 2017

I think I'd rather replace our deserializer with the jsonapi-deserializer. There are prs for it. See #1928 (comment)

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