Add include_data :via_include_parameter #1797

richmolj · 2016-06-11T16:21:02Z

@NullVoxPopuli @bf4, taking a final stab at simplifying the code from #1720. I created a separate PR since the original solves more issues and I didn't want to blow away the tests if you wanted to use them. Feel free to just close this PR if you still want to go in another direction, don't want this to seem pushy 😄 I believe this is the simplest possible solution, so maybe helpful to understand the problem if nothing else.

This now adds only one new variable name, include_slice. This is the slice of the include directive hash being processed by the current serializer. All the changes to lib/active_model/serializer/associations.rb and lib/active_model_serializers/adapter/json_api.rb are just passing that slice around so we can access it when we finally decide whether or not to have the data key in the response. The changes to lib/active_model/serializer/reflection.rb access that variable to make the decision.

Why do we need the current slice instead of the whole directive? Because multiple entities can have the same relationship, e.g. /blogs/?include=posts.tags,tags should include both blog tags and post tags, but /blogs/?include=posts.tags should only include post tags

Users now opt-in to this pattern using the current DSL. The current DSL conditionally includes the data key in the response via include_data true/false:

class PostSerializer < ActiveModel::Serializer
  has_many :comments do
    include_data false
  end
end

We now add another option :via_include_parameter. This means we only include the data key when the relationship is being sideloaded:

class PostSerializer < ActiveModel::Serializer
  has_many :comments do
    include_data :via_include_parameter
  end
end

bf4 · 2016-06-14T03:31:02Z

lib/active_model_serializers/adapter/json_api.rb

@@ -235,17 +235,17 @@ def resource_objects_for(serializers)
        @primary = []
        @included = []
        @resource_identifiers = Set.new
-        serializers.each { |serializer| process_resource(serializer, true) }
+        serializers.each { |serializer| process_resource(serializer, true, @include_directive) }


here it begins

bf4 · 2016-06-14T14:32:58Z

I feel like we shouldn't need to introduce the include_slice directive (i.e have two include directives) for this to work. Otherwise, the public interface change is good.

bf4 · 2016-06-14T14:34:10Z

test/adapter/json_api/include_param_test.rb

+            )
+          end
+
+          def test_relationship_not_loaded_when_not_included


I think this test is the central feature in this PR, at least for the JSONAPI adapter

richmolj · 2016-06-14T18:25:24Z

I feel like we shouldn't need to introduce the include_slice directive (i.e have two include directives) for this to work. Otherwise, the public interface change is good.

I feel you, but not sure how to avoid it. If you have an include_directive like { posts: { comments: :author }, reviews: { comments: {} }, and you only ever know about the entire directive and not the slice currently being processed, how would CommentSerializer know if author data should be populated? It's conditional depending on the nesting level.

bf4 · 2016-06-14T18:53:11Z

It should be modifying the include and fields params as each serializer calls the next, no?

B mobile phone

On Jun 14, 2016, at 1:25 PM, Lee Richmond notifications@github.com wrote:

I feel like we shouldn't need to introduce the include_slice directive (i.e have two include directives) for this to work. Otherwise, the public interface change is good.

I feel you, but not sure how to avoid it. If you have an include_directive like { posts: { comments: :author }, reviews: { comments: {} }, and you only ever know about the entire directive and not the slice currently being processed, how would CommentSerializer know if author data should be populated? It's conditional depending on the nesting level.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

richmolj · 2016-06-14T23:56:54Z

It should be modifying the include and fields params as each serializer calls the next, no?

@bf4 This may be a matter of phrasing, but to be clear - since fields and include live on the adapter, the adapter recursively iterates through associations (one serializer doesn't call the next - I wish they did, the issue would likely be avoided). During iteration, since we're only dealing with a slice of the overall directive, I renamed the parameter named include_slice to make this more obvious.

I feel like the current PR is already doing what you are suggesting, so let me try to rephrase - I think the confusion is with the current codebase, not this PR. include_directive means 3 different things in the current codebase:

The @include_directive - the overall directive passed to render
The include_directive in method parameters that I now refer to as include_slice. This is the slice of the @include_directive we're currently iterating over.
The include_directive in #relationships_for is a totally separate concept, derived from the fields param and used to iterate over associations.

IOW, you could actually refer to it as include_slice or include_directive everywhere...except the current implementation of #relationships_for screws us up. This introduces another variable named 'include_directive' that is not the include directive we've been dealing with this whole time. It's a separate directive derived from the 'fields' parameter. It uses this new directive to iterate over the associations - and we can't pass only the current slice (you'll get a ton of test failures as we often need to process relationships even when include is not present). But we need that current slice down the line in reflection.rb, which is why we end up referring to both include_directive and include_slice in the same method and confusion ensues.

Hope this helps - as always, your time and effort here are much appreciated.

NullVoxPopuli · 2016-07-11T10:59:20Z

Some thoughts:

I don't think fields should be recursive, unless we added the same notation that we do for include ('field1,field2.subfield1).

What if the API was this:

# current behavior
has_many :comments


has_many :comments do
  # must explicitly be included
  # or maybe something related to the resource identifier object?
  # http://jsonapi.org/format/#document-resource-identifier-objects
  skip_unless_included
end

I feel like include_data is a little ambiguous, as what we want to avoid is the Resource Identifier object isn't it?

I don't like the additional include_ variable.

bf4 · 2016-07-11T12:11:46Z

Hmm, fields_directive...

B mobile phone

On Jul 11, 2016, at 5:59 AM, L. Preston Sego III notifications@github.com wrote:

Some thoughts:

I don't think fields should be recursive, unless we added the same notation that we do for include ('field1,field2.subfield1).

What if the API was this:

current behavior

has_many :comments

has_many :comments do

must explicitly be included

or maybe something related to the resource identifier object?

http://jsonapi.org/format/#document-resource-identifier-objects

skip_unless_included
end
I feel like include_data is a little ambiguous, as what we want to avoid is the Resource Identifier object isn't it?

I don't like the additional include_ variable.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

NullVoxPopuli · 2016-07-11T12:17:07Z

Hmm, fields_directive...

at least initially, we could just have fields span attributes and relationships, flatly. No need for a fields directive, I don't think. at least.. not yet.

richmolj · 2016-07-11T14:14:16Z

@NullVoxPopuli correct me if I'm wrong, but fields is not currently recursive. I'm actually not sure how/why it would ever be - JSONAPI fields is flat and namespaced, e.g. /articles?include=author&fields[articles]=title,body&fields[people]=name. Something like field1.subfield1 would deviate from the spec AFAIK. #process_relationships is the source of recursion here.

skip_unless_included

👍 to include_data being ambiguous, but keep in mind the current behavior of this gem uses include_data false independent of the ?include parameter. So replacing with skip_unless_included probably doesn't match well for the current use cases you guys are supporting.

I don't like the additional include_ variable.

I think that is something everyone, including me, agrees on 😂 My thing is: what's the alternative? I can't see how you avoid it without fundamentally changing how adapter/json_api.rb works (avoiding the recursion and switching to a serializers-call-serializers pattern).

NullVoxPopuli · 2016-07-11T14:24:52Z

JSONAPI fields is flat and namespaced

I think we're on the same page here.

So replacing with skip_unless_included probably doesn't match well for the current use cases you guys are supporting.

how do you mean?

avoiding the recursion and switching to a serializers-call-serializers pattern

ah, I see. that is obnoxious then.

bf4 · 2016-07-11T14:25:07Z

skip_unless_included should be default for what jsonapi passes into serializer 'attributes'

bf4 · 2016-07-11T14:26:57Z

i.e.

# https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer.rb
    def serializable_hash(adapter_options = nil, options = {}, adapter_instance = self.class.serialization_adapter_instance)
      resource = fetch_attributes(options[:fields], cached_attributes, adapter_instance)
# https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/attributes.rb
        def attributes(requested_attrs = nil, reload = false)
 next if attr.excluded?(self)
            next unless requested_attrs.nil? || requested_attrs.include?(key)

means that the JSON API adapter should pass in options[:fields] = {} to a serializer when no fields are specified

richmolj · 2016-07-11T14:29:12Z

@NullVoxPopuli regarding skip_unless_included and the current behavior of the gem: Assuming people are using include_data false right now with the current gem, they are excluding the data key independent of the ?include param. So renaming this option something specific to the ?include param (skip_unless_included) seems off to me. That said, I agree with your general thought and for my personal use of AMS I'd be 👍

NullVoxPopuli · 2016-07-11T14:36:04Z

skip_unless_included should be default for what jsonapi passes into serializer 'attributes'

@bf4, do you mean we shouldn't include the id/type for relationships (by default) (no options passed)

Assuming people are using include_data false right now with the current gem, they are excluding the data key

@richmolj I actually didn't know that was a thing. ha

richmolj · 2016-07-11T14:40:44Z

@bf4 while I agree skip_unless_included should be a default, I am not sure 'attributes' is the place for it, for two reasons:

One - I think the payload output would be something like:

{
  relationships: {
    author: {
      data: {
        id: 1, type: 'people', attributes: {}
      }
    }
  }
}

Which means the resource identifiers would be included, a DB hit is still happening, and we haven't solved the issue.

That said, even if we got the payload to look like this:

{
  relationships: {
    author: {
      data: {}
    }
  }
}

It's still a no-go. This is telling clients "this object does not have an author", versus "this object may or may not have an author, we're just not including author information in the response".

bradens · 2016-07-25T22:57:00Z

I need this so bad, is there anything I can do to help get something to solve this problem released?

/edit Great work @richmolj 👍

richmolj · 2016-09-05T21:27:24Z

@bf4 - per our discussion yesterday:

Renamed via_include_parameter to if_sideloaded, so it now reads include_data :if_sideloaded.
Added ActiveModel::Serializer.config.include_data_default configuration option.

Though we both agree this should be the default, I didn't change the current default out of backwards compatibility concerns. If you're OK with it I'm OK with it, though.

bf4 · 2016-09-05T21:55:57Z

lib/active_model/serializer/concerns/configuration.rb

@@ -29,6 +29,7 @@ def config.array_serializer
        # Make JSON API top-level jsonapi member opt-in
        # ref: http://jsonapi.org/format/#document-top-level
        config.jsonapi_include_toplevel_object = false
+        config.include_data_default = true


jsonapi adapter should default to false, right?

current default is true. per the PR description, I vote we change that default but up to you guys

For JSONAPI, `include_data` currently means, "should we populate the 'data'" key for this relationship. Current options are true/false. This adds the `:if_sideloaded` option. This means "only populate the 'data' key when we are sideloading this relationship." This is because 'data' is often only relevant to sideloading, and causes a database hit. Addresses rails-api#1555

kevintyll · 2016-09-16T16:28:28Z

Is there any concern for ensuring that the relationship object contains at least of of data, links, or meta elements?
Per the spec, one is required, but if the association is set up with no link, or meta defined, and include_data :if_sideloaded, then the result will not be compliant?

Looking forward to this feature BTW. I just implemented my own monkey patch to do exactly this. This solution is better.

richmolj · 2016-09-16T16:45:30Z

@kevintyll - definitely agree. If the relation is not influded you should see an empty meta key, eg 'comments: { meta: {} }'. I'm on mobile right now but I should add a test asserting that is the case.

Addresses rails-api#1911 (comment)

ApplicationSerializer could exist, but not be loaded. So, we check existence by looking at the filesystem instead of defined?. Fixes rails-api#1890

Add docs for links Add docs for links Add docs for links Add docs for links Add docs for links Add controller info Grammar fixing Improve docs some small wording changes Add pr to changelog

While this same document provides details on how to override the serializer_for, not all users will realize this could be used to set the sterilizers for polymorphic relationships. This change just adds a link to that documentation and makes that point obvious.

* Update ember-and-json-api.md Removed ember-data adapter change to support include directives, as it's now built-in. Updated the documentation for how to add include directives to ember store queries, and added documentation covering how to tell Rails that we are consuming and generating jsonapi data * Fix up format for parameter restrictions * Update ember-and-json-api.md Updates to address comments; explain why Rails should know what format we are consuming/generating, reword introduction to include: examples, and fix render statement to specify jsonapi instead of json.

bf4 · 2016-09-16T18:21:17Z

@richmolj issue title /description should also be updated, I think, no?

richmolj · 2016-09-16T18:44:03Z

@bf4 agreed, will close this and submit a new one tonight.

richmolj · 2016-09-16T19:03:22Z

Moved to #1931

bf4 reviewed Jun 14, 2016
View reviewed changes

bf4 added C: Feature V: 0.10.x S: Needs Team Discussion C: JSON API C: Relationships C: Field Selection labels Jun 14, 2016

bf4 reviewed Jun 14, 2016
View reviewed changes

This was referenced Jul 26, 2016

Serializer pulls relations when belongs_to declared but not included #1552

Closed

Smarter association id lookup-- no db hit on belongs_to for id-only #1857

Merged

NullVoxPopuli mentioned this pull request Aug 16, 2016

Add an ability to manually include relationships #1882

Closed

richmolj force-pushed the include_data_via_include_param branch from 0ab4c89 to 49a87bb Compare August 25, 2016 22:41

richmolj force-pushed the include_data_via_include_param branch from 49a87bb to 474322b Compare September 4, 2016 14:28

richmolj force-pushed the include_data_via_include_param branch 2 times, most recently from 25d2ca0 to 5023df6 Compare September 5, 2016 21:23

bf4 reviewed Sep 5, 2016
View reviewed changes

richmolj force-pushed the include_data_via_include_param branch from 5023df6 to bb797b8 Compare September 5, 2016 22:05

Lee Richmond and others added 10 commits September 16, 2016 14:08

Add helpful testing gems

81983df

Addresses rails-api#1911 (comment)

Ensure generator picks up ApplicationSerializer

6c59dac

ApplicationSerializer could exist, but not be loaded. So, we check existence by looking at the filesystem instead of defined?. Fixes rails-api#1890

Fix typo (rails-api#1916)

fed2557

Disable pagination links via config

f3fd3d8

Add docs for links (rails-api#1909)

ee91d72

Add docs for links Add docs for links Add docs for links Add docs for links Add docs for links Add controller info Grammar fixing Improve docs some small wording changes Add pr to changelog

Make railties an optional dependency

b823499

Add chengelog entry about the issue

833f9e8

add bm_adapter (rails-api#1914)

fb288d5

richmolj mentioned this pull request Sep 16, 2016

Ensure valid jsonapi when blank relationship #1930

Merged

richmolj closed this Sep 16, 2016

richmolj mentioned this pull request Sep 16, 2016

Add include_data :if_sideloaded #1931

Merged

bf4 mentioned this pull request Oct 9, 2016

:only option on has_many not working #1943

Open

bf4 mentioned this pull request Jan 21, 2017

Refactor Association to make it eval reflection JIT #2026

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add include_data :via_include_parameter #1797

Add include_data :via_include_parameter #1797

richmolj commented Jun 11, 2016

bf4 Jun 14, 2016

bf4 commented Jun 14, 2016

bf4 Jun 14, 2016

richmolj commented Jun 14, 2016

bf4 commented Jun 14, 2016

richmolj commented Jun 14, 2016 •

edited

Loading

NullVoxPopuli commented Jul 11, 2016

bf4 commented Jul 11, 2016

current behavior

must explicitly be included

or maybe something related to the resource identifier object?

http://jsonapi.org/format/#document-resource-identifier-objects

NullVoxPopuli commented Jul 11, 2016

richmolj commented Jul 11, 2016

NullVoxPopuli commented Jul 11, 2016

bf4 commented Jul 11, 2016

bf4 commented Jul 11, 2016 •

edited

Loading

richmolj commented Jul 11, 2016

NullVoxPopuli commented Jul 11, 2016

richmolj commented Jul 11, 2016

bradens commented Jul 25, 2016 •

edited

Loading

richmolj commented Sep 5, 2016

bf4 Sep 5, 2016

richmolj Sep 5, 2016

kevintyll commented Sep 16, 2016

richmolj commented Sep 16, 2016

bf4 commented Sep 16, 2016

richmolj commented Sep 16, 2016

richmolj commented Sep 16, 2016

Add include_data :via_include_parameter #1797

Add include_data :via_include_parameter #1797

Conversation

richmolj commented Jun 11, 2016

bf4 Jun 14, 2016

Choose a reason for hiding this comment

bf4 commented Jun 14, 2016

bf4 Jun 14, 2016

Choose a reason for hiding this comment

richmolj commented Jun 14, 2016

bf4 commented Jun 14, 2016

richmolj commented Jun 14, 2016 • edited Loading

NullVoxPopuli commented Jul 11, 2016

bf4 commented Jul 11, 2016

current behavior

must explicitly be included

or maybe something related to the resource identifier object?

http://jsonapi.org/format/#document-resource-identifier-objects

NullVoxPopuli commented Jul 11, 2016

richmolj commented Jul 11, 2016

NullVoxPopuli commented Jul 11, 2016

bf4 commented Jul 11, 2016

bf4 commented Jul 11, 2016 • edited Loading

richmolj commented Jul 11, 2016

NullVoxPopuli commented Jul 11, 2016

richmolj commented Jul 11, 2016

bradens commented Jul 25, 2016 • edited Loading

richmolj commented Sep 5, 2016

bf4 Sep 5, 2016

Choose a reason for hiding this comment

richmolj Sep 5, 2016

Choose a reason for hiding this comment

kevintyll commented Sep 16, 2016

richmolj commented Sep 16, 2016

bf4 commented Sep 16, 2016

richmolj commented Sep 16, 2016

richmolj commented Sep 16, 2016

richmolj commented Jun 14, 2016 •

edited

Loading

bf4 commented Jul 11, 2016 •

edited

Loading

bradens commented Jul 25, 2016 •

edited

Loading