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

Modify Plugs.DataToAttributes to follow JSON API sideposting draft #1303

Open
begedin opened this issue Dec 18, 2017 · 1 comment
Open

Modify Plugs.DataToAttributes to follow JSON API sideposting draft #1303

begedin opened this issue Dec 18, 2017 · 1 comment
Labels

Comments

@begedin
Copy link
Contributor

begedin commented Dec 18, 2017

Problem

From https://github.com/json-api/json-api/pull/1197/files?short_path=571f6ce#diff-571f6cee9a492e0e177801c21a4b2b2a

Our current approach with included records is to simply assume they are related to the main record.

Based on the draft

  • included record payloads should have temp-id properties if they are to be created.
  • the main record payload should have a relationship where the identifier is %{data: [%{"temp-id" => "foo", "type" => "bar"}
  • the plug should get those identifiers, then replace the identifier payloads with the associated full payloads matched in included

Subtasks

  • instead of just taking any included records and assuming they're relationships in the resulting params map, DataToAttributes should parse the relationships map first
    • the relationship where data is a map is a belongs to and should be left alone. it's already handled by JaSerializer.Params.to_attributes
    • the relationship where data is an array should be checked for identifiers with temp-id keys
      • each temp-id should be found in the included section and added under the pluralized relationship key in the resulting map

An advantage of this is that we now are able to conclude it's definitely a has_many, so we do not need to explicitly specify this anymore.

References

The code this needs to modify is part of #1301 so this is blocked until that is merged.

@joshsmith
Copy link
Contributor

To be clear, I do think that temp-id is not going to be the final, and the draft may not be acceptable to use as of right now as a starting point. The temp-id, e.g., is proposed to change to tid.

It may be best to hold off on this until more movement is made there.

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

No branches or pull requests

2 participants