Skip to content

Embedded structs v3 #100

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

Merged
merged 15 commits into from
Jul 27, 2017

Conversation

skimata
Copy link

@skimata skimata commented Jul 19, 2017

this is an update to #78. I found it cleaner and more convenient to just cherry-pick my changes in the latest google/jsonapi.

All comments made to the last PR should be addressed here. Addressing the last comment in that PR required some refactor/enhancements, hence took some time.

I think some additional tests should be added to demonstrate what should happen when fields are overlapping. Ie both have a field w/ primary annotation or an attr with the same name has been used.

I added these additional examples:

  • response_test.go:1135: running scenario: Model embeds Thing, models have no annotation overlaps
  • response_test.go:1135: running scenario: Model embeds Thing, overlap Buzz attribute
  • response_test.go:1135: running scenario: Model embeds Thing, attribute, and primary annotation overlap
  • response_test.go:1135: running scenario: Model embeds Thing, but is annotated w/ ignore

The expected behavior is that it will behave in the "expected" composition behavior; it will handle the top-level struct fields first, then handle any embedded structs after that. So in the scenario where there is a top-level struct that has a primary annotation, that embeds another struct also containing a primary annotation, the top-level attribute wins. (In the below example, the expected jsonapi payload should have a data/resource type of "posts")

type Post struct {
	Blog
	ID            uint64     `jsonapi:"primary,posts"`
	BlogID        int        `jsonapi:"attr,blog_id"`
	ClientID      string     `jsonapi:"client-id"`
	Title         string     `jsonapi:"attr,title"`
	Body          string     `jsonapi:"attr,body"`
	Comments      []*Comment `jsonapi:"relation,comments"`
	LatestComment *Comment   `jsonapi:"relation,latest_comment"`
}

Lastly, I felt it was getting complex and a bit messy, so I started splitting out unmarshalNode() into smaller functions for better readability.

Happy to add something to the readme.md if you think that would be useful

@aren55555 aren55555 mentioned this pull request Jul 20, 2017
@skimata skimata mentioned this pull request Jul 26, 2017
@aren55555 aren55555 changed the base branch from master to feature/embeded-structs July 27, 2017 18:15
@aren55555 aren55555 merged commit a6ac768 into google:feature/embeded-structs Jul 27, 2017
})
}
for _, scenario := range scenarios {
t.Logf("running scenario: %s\n", scenario.name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skimata please submit pr to use t.Run

marc-lebourdais pushed a commit to marc-lebourdais/jsonapi that referenced this pull request Mar 10, 2018
* working version

* fix text

* combine test files

* move private funcs to bottom

* ErrInvalidType should ignore interfaces

* replace MarshalOnePayload w/ MarshalPayload; fix bug w/ node merge()

* minor tweaks; address a couple comments

* decompose unmarshalNode() to smaller funcs; unmarshal should go from top-level to embedded

* deep copy the node when passing relation/sideloaded notes to unmarshal()

* add some comments and do some additional cleanup

* add test uses annotationIgnore

* implement support for struct fields that implement json.Marshaler/Unmarshaler

* add additional test that compares marshal/unmarshal behavior w/ standard json library

* add support for pointer embedded structs

* Revert "implement support for struct fields that implement json.Marshaler/Unmarshaler"

This reverts commit deeffb7.
@fredericbirke
Copy link

Is there a reason, that this MR got merged into google:feature/embeded-structs and that the feature branch has been lying there since, without getting merged/touched for more than a year? I would like to use the jsonapi in production, but would prefer if embedded-structs were supported.

@ghaniswara
Copy link

it looks like this will never be supported huh ?

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.

5 participants