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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃帀 Support for `included` query param in JsonApiSerializer #424

Merged
merged 13 commits into from Dec 19, 2015

Conversation

Projects
None yet
2 participants
@lolmaus
Copy link
Contributor

commented Dec 10, 2015

1. Updated JsonApiSerializer to use dasherized types

JSONAPI recommends to use dasherized types. Though not a strict requirement, that's what Ember uses. Ember ignores camel-cased types.

2. Updated serializer relationships to use dasherized strings rather than camelized

You said the convention is to use camel case in property keys and dashes in strings. It sucked to write relationships: ['fooBar']. I refactored the serializer layer to work with relationships: ['foo-bar'].

3. Support for included query param in JsonApiSerializer

Now JsonApiSerializer combines relationship names from serializer.relationships and request.queryParams.include.

4. serializer.relationships is changed to serializer.include

According to jsonapi.org, a resource object MAY contain a relationships object. However, members of the relationships object ("relationships") are said to "represent references from the resource object in which it's defined to other resource objects". I. e. it is not said that relationships are allowed to be omitted.

Rails' JSONAPI::Resources serializes all defined relationships unconditionally.

Also, I can't think of a use case where serialization of a relationship is undesired.

Thus, I renamed serializer.relationships to serializer.include. This property now only controls which resources should be sideloaded by default. As for relationships within resource objects, they are all serialized.

Tests updated accordingly.


Notes:

  1. It might be more informative to look at changes per commit. I'm sorry it's all in one PR, but each subsequent commit is dependent on the previous one, so I was unable to file them separately against origin/master.
  2. There's a fixup commit which I was unable to squash (there were changes already that disallowed changing the order of commits).
  3. Everything is tested with automated tests. Existing tests updated, new tests written.
  4. Tested in a real app.
  5. There's repetetive use of camelize(key). Might make sense to extract it into a method.
  6. I had to assert against empty relationships: {'blog-posts': { data: [] }} into some tests. That's optional output, so making it a requirement is not very good. But that's what the serializer was returning, and I didn't bother to refactor propEqual into a more elaborate assertion that allows for optional empty relationships.

@lolmaus lolmaus changed the title :tada: Support for `included` query param in JsonApiSerializer 馃帀 Support for `included` query param in JsonApiSerializer Dec 10, 2015

@samselikoff samselikoff added this to the 0.2.0 milestone Dec 13, 2015

@samselikoff

This comment has been minimized.

Copy link
Owner

commented Dec 13, 2015

I've pushed my wip branch so we can compare what I was doing with what you've done here. There's a lot going on in both branches.

  1. There's a lot of similar code in both PRs, which is fine - means it needs to get in there!

  2. As part of my PR, I renamed model.type to model.modelTypeKey, since .type was too common and could interfere with properties people have on their models. I think we should move forward with getting your PR merged, then I can rebase mine off of master and make the modelTypeKey changes. Please don't add that in here, since there's already a lot going on. Let's try to get this merged with the minimum number of changes.

  3. The ?include support is awesome! I originally thought I'd add this to the GetRouteHandler, but it makes so much more sense to have it in the JSONApiSerializer. I believe your implementation is technically partial support for ?include as per the spec, since the spec supports dot paths. This is fine, we'll just need to make sure to document it.

  4. I had some changes I wanted to make for the default Serializer, but again let's not worry about those here. I may update those in my branch after we get this merged.

  5. Ok, the only big thing I see here is the relationships: ['kebab-case']. I understand it's weird to write relationships: ['camelCase'] but the point is that these reference model properties that point to relationships, not the relationship types themselves. For example, given the model

    export default Model.extend({
    someAdmins: hasMany('fine-user')
    });

    the way you would tell your serializer to include this relationships should be by referencing someAdmins, rather than the type of fine-user:

    export default Serializer.extend({
    relationships: ['someAdmins']
    });

    so changing the API so you specify relationships: ['some-admins'] feels a bit too magical and unconventional to me. I know someAdmins is a string here, but it's supposed to directly represent which property you want to include.

    Note that this is currently how attributes work as well. If your model has a firstName property, you'd whitelist it in your serializer like so:

    export default Serializer.extend({
    attributes: ['firstName']
    });

    I know this is all kind of a bummer, but for now I think it's the best we have. Can you think of a better API? I'm not sure how we can list properties without using strings. If you can't think of something better, we'll leave it like this for now.


Ok, if you agree + revert the kebab-casing of the relationships array, I think this is good to go!

@lolmaus lolmaus force-pushed the lolmaus:feature-included branch from 44e35d8 to 7e40b83 Dec 14, 2015

@lolmaus

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2015

Done.

@@ -29,7 +29,7 @@ module('Integration | Serializer | ActiveModelSerializer', {
application: ActiveModelSerializer,
wordSmith: ActiveModelSerializer.extend({
attrs: ['id', 'name'],
relationships: ['blogPosts']
relationships: ['blog-posts']

This comment has been minimized.

Copy link
@samselikoff

samselikoff Dec 14, 2015

Owner

@lolmaus I meant all of these pieces - like I said, it seems weird writing camelCase in string, but they directly correspond to model properties (just like the attrs hash). So, if we can't think of a better API (and I don't think referencing camelCased properties with kebab-case is better), I'd like to keep these camelCased.

@samselikoff

This comment has been minimized.

Copy link
Owner

commented Dec 14, 2015

Camelizing the readModules bit was fine, see my inline comment above for clarification about what I meant in point 5 of my comment.

JsonApiSerializer now adds all relationships to all resource objects.鈥
鈥 `serializer.relationships` is renamed to `serializer.include` and controls which resources to sideload by default.
@lolmaus

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2015

4. serializer.relationships is changed to serializer.include

According to jsonapi.org, a resource object MAY contain a relationships object. However, members of the relationships object ("relationships") are said to "represent references from the resource object in which it's defined to other resource objects". I. e. it is not said that relationships are allowed to be omitted.

Rails' JSONAPI::Resources serializes all defined relationships unconditionally.

Also, I can't think of a use case where serialization of a relationship is undesired.

Thus, I renamed serializer.relationships to serializer.include. This property now only controls which resources should be sideloaded by default. As for relationships within resource objects, they are all serialized.

Tests updated accordingly.

@samselikoff

This comment has been minimized.

Copy link
Owner

commented Dec 15, 2015

Thanks for the update! Some comments:

  • Regarding default serialization of resources, http://jsonapi.org/format/#fetching-includes states

    An endpoint MAY return resources related to the primary data by default.

    Also, JR does not serialize all relationships unconditionally. Specifying a relationship on a resource in JR adds its link to the relationships.links key of the parent's resource object, but it does not serialize and include that relationship in the payload. Serializing every relationship by default would lead to returning a user's entire object graph when making a request to /users/1, which is definitely not what we want to have happen by default. Relationships should absolutely be opt-in.

  • I'm okay with changing relationships to include, since I'm in favor of pushing the language of the JSON:API spec, but if we change it for the JSONAPISerializer we should change for all serializers too. After all, it does the same thing, regardless of which serializer you're using: specifies the server's default includes.

  • Regarding queryParam support, queryParam includes need to override any default includes specified by the serializer, rather than merge with them:

    If an endpoint supports the include parameter and a client supplies it, the server MUST NOT include unrequested resource objects in the included section of the compound document. source

    This is one reason I wanted to hold off on queryParam support - it's nontrivial to write a conforming implementation.


This PR is now very large, and it's taken me a long time to review both of the bigger sets of changes you've made to it. The dasherize issues addressed here are at the top of my queue, and they're a blocker for the next set of issues I need to tackle.

I'm going to ask that you revert the includes queryParam changes here (you should check out a new branch so you can rebase once we get this merged, so you don't lose your work, and if you want to continue working on that feature in the future), as well as the serialization-by-default issues mentioned above, so we can merge this in. Otherwise, I'm going to need to close this, so I can finish my implementation at #428 and press on towards 0.2.0.

@lolmaus lolmaus force-pushed the lolmaus:feature-included branch from 3e2a347 to 1d4295e Dec 15, 2015

@lolmaus

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2015

I've updated tests to use camelized serializer.includes and... they didn't break! It uses camelize where needed, so turns out you can path both camelized and dasherized resource names.

@lolmaus

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2015

Ready for review and merge.

@samselikoff

This comment has been minimized.

Copy link
Owner

commented Dec 17, 2015

I reviewed everything, great work (again)! I'm glad you made the change to json:api so all relationships are serialized, even for association not included - this is how json:api resources does it too (I believe that's what you were saying originally, but I didn't understand you).

The one thing that remains is, to standardize names across JSONAPISerializer and BaseSerializer. You changed JSONAPISerializer to include: [], but Base still uses relationships: []. I'd like these to be named the same, and I think include is the best name.

I'm happy to merge this, then make the change myself, if you'd like to just get this in, so we can be done with it! Or, you can make the change. What do you think?

@lolmaus

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2015

  • Updated base serializer to use the same include key as JsonApiSerializer now does
  • Merged with master to resolve a conflict in package.json
@samselikoff

This comment has been minimized.

Copy link
Owner

commented Dec 19, 2015

Looks perfect! 馃挜

Thanks again!

samselikoff added a commit that referenced this pull request Dec 19, 2015

Merge pull request #424 from lolmaus/feature-included
馃帀 Support for `included` query param in JsonApiSerializer

@samselikoff samselikoff merged commit 3edf933 into samselikoff:master Dec 19, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lolmaus

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2015

Yay! 馃帄 馃懐 馃嵒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.