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

bf4 · 2016-07-26T21:42:06Z

If the serializer has an association,

If adapter is not JSON API, serialize it.
else if it is JSON API and the related resource
- is not included, serialize just the ids
- is included, serialize the all fields, or the requested fields

The thing where it doesn't actually call the association is a
special case where AMS needs to know it can get the relationship id
from the resource itself, so that userland code doesn't need to do any fancy association preloading.

The way this has been solved in JSONAPI-Resources
is to have special rules for looking up association ids.
And see JSONAPI::Relationship

In brief:

association	foreign_key method	loads relation (i.e. when foreign key is on relation )
`has_many :authors`	`:author_ids`	true
`has_one :author`	`:author_id`	true
`belongs_to :author`	`:author_id`	false

Caveat:

This will only work on active record objects

Includes:

belongs_to causes unnecessary db hits #1100

Follows:

mention-bot · 2016-07-26T21:42:07Z

@bf4, thanks for your PR! By analyzing the annotation information on this pull request, we identified @beauby, @ehsanyousefi and @bolshakov to be potential reviewers

NullVoxPopuli · 2016-07-26T22:57:51Z

Do you plan to fix the bug, or is this just to demonstrate the problem? (either is cool)

bf4 · 2016-08-01T19:38:55Z

lib/active_model/serializer/reflection.rb

        reflection_options[:include_data] = @_include_data
+        reflection_options[:new_options] = {


hack to allow for refactoring...

I was just about to ask about this, thanks

bf4 · 2016-08-07T21:17:45Z

I think I got it working and all tests passing! Now time for review.

bf4 · 2016-08-07T21:20:54Z

test/adapter/json_api/relationship_test.rb

+        #   parent_serializer = AuthorSerializer.new(@author)
+        #   relationship = Relationship.new(parent_serializer, @serializer, nil, params)
+        #   assert_equal(expected, relationship.as_json)
+        # end


These tests, like much of the JSONAPI code, are tightly coupled to the implementation.

They are asserting a relationship between two serializers that are not necessarily actually related.

The impl in this PR is to pass in the parent serializer and the association.

In these tests, the association would need to be derived from the @serializer. I couldn't get this the api to this test file right, so am relying on the remainder of the test suite.

(DO NOT merge with this commented out... tbd what to do about it)

bf4 · 2016-08-08T01:14:18Z

cc @youroff @ssendev @sandstrom @richmolj @joaomdmoura @vasilakisfil @beauby

bf4 · 2016-08-08T01:15:19Z

(Test failure right now are regularly occurring but intermittent... tests pass though, and pr needs refactoring, so this is a good chance to look)

sandstrom · 2016-08-08T11:48:20Z

@bf4 this is a great feature!

Regarding "This will only work on active record objects", would it be possible to define an interface (+ default handling for ActiveRecord) such that other ORMs can use it too (or use some other mechanism, basically anything that'll make this useful for other ORMs that are similar to ActiveRecord). For example Mongoid.

bf4 · 2016-08-08T12:06:09Z

Well, I wrote it using respond_to, so if it quacks that way it will work. I'm not familiar with the interface needed by mongoid

B mobile phone

On Aug 8, 2016, at 6:48 AM, sandstrom notifications@github.com wrote:

@bf4 this is a great feature!

Regarding "This will only work on active record objects", would it be possible to define an interface (+ default handling for ActiveRecord) such that other ORMs can use it too (or use some other mechanism, basically anything that'll make this useful for other ORMs that are similar to ActiveRecord). For example Mongoid.

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

NullVoxPopuli · 2016-08-08T12:13:13Z

lib/active_model/serializer/collection_reflection.rb

@@ -2,6 +2,12 @@ module ActiveModel
  class Serializer
    # @api private
    class CollectionReflection < Reflection
+      def initialize(*)
+        super
+        @class_name = options.fetch(:class_name, name.to_s.camelize.singularize)


should these maybe be methods?

as oppose to in the initializer?

I'm just wary of too much logic in an initializer / any method

bf4 · 2016-08-17T21:33:46Z

test/adapter/json_api/relationship_test.rb

+          options[:new_options] = {
+            meta: test_options.delete(:meta),
+            links: test_options.delete(:links),
+            reflection: parent_serializer._reflections.to_a.first


I know this is broken (in the rebase off of master), but just wanted to push the work for the moment

bf4 · 2016-08-18T07:07:13Z

test/adapter/json_api/relationship_test.rb

+          @serializable_resource_options = {} # adapter.instance_options
+        end
+
+        def build_author_relationship(author_attributes, association_name)


I completely rewrote the test harness and assertion in this file.. could use a second pair of eyes, but at least I think it is straightforward enough to read the assertions and understand, and to read the harness and understand what it's doing (even if not so clear why it's so complicated, and how could it be simpler).

I think it's pretty straight forward, usage wise.

though, why do you need to reset the reflection stuff?
is that for isolation or something else?
my concern is that if something is happening to those normally that is being avoided here, it could reveal a hole in testing.

Just that normally these are created by the serializer using these as defaults in the reflection initializer, but here I am mutating them, which changes them for all the tests, and dup and deep_dup didn't work. So it's a hack to make the serializer behave in each test as if it were being defined for the first time.

B mobile phone

On Aug 18, 2016, at 6:53 AM, L. Preston Sego III notifications@github.com wrote:

In test/adapter/json_api/relationship_test.rb:

setup do

@blog = Blog.new(id: 1)

@author = Author.new(id: 1, name: 'Steve K.', blog: @blog)

@serializer = BlogSerializer.new(@blog)

ActionController::Base.cache_store.clear

@serializable_resource_options = {} # adapter.instance_options

end

def build_author_relationship(author_attributes, association_name)
I think it's pretty straight forward, usage wise.

though, why do you need to reset the reflection stuff?
is that for isolation or something else?
my concern is that if something is happening to those normally that is being avoided here, it could reveal a hole in testing.

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

NullVoxPopuli · 2016-08-31T02:49:30Z

An issue I noticed when playing around with this briefly:

sponsor should be of type organizations, and sponsored should be of type events

belongs_to associations now use the association_name for the model name, rather than the type of the corresponding serializer

bf4 · 2016-09-27T15:27:04Z

I just rebased and broke a lot of stuff... I'll need to review the diff for what changes we actually want at this point. So, yes, lots of failures now that need to be addressed, it is known.

furkanayhan · 2017-06-16T12:06:15Z

@bf4 Hey, thank you for this improvement.
But I think there is something wrong with polymorphic-belongs_to relationships, after this pr is merged.

Examle:

serializer:

class CommentSerializer < ActiveModel::Serializer
  # ....
  belongs_to :commentable
end

request:
/comments?include=commentable

result:

//...
"relationships": {
  "commentable": {
    "data": {
      "id": "1",
      "type": "commentables"
    }
  }
}
//...

Can you check it out?

andyhite · 2017-06-30T05:18:57Z

lib/active_model/serializer/reflection.rb

      def initialize(*)
        super
        options[:links] = {}
        options[:include_data_setting] = Serializer.config.include_data_default
        options[:meta] = nil
+        @type = options.fetch(:type) do
+          class_name = options.fetch(:class_name, name.to_s.camelize.singularize)


This line is causing the bug reported here: #2160 In particular, the use of the field's name if the class_name and type aren't specified breaks belongs_to associations.

bf4 mentioned this pull request Jul 27, 2016

Provide a clear way to include nested associations and fields #1849

Open

bf4 force-pushed the smarter_association_id_lookup branch 2 times, most recently from 3abc80b to c93e599 Compare July 29, 2016 03:44

bf4 mentioned this pull request Jul 29, 2016

#as_json option :only Not working #1851

Open

bf4 force-pushed the smarter_association_id_lookup branch from c93e599 to cb515f8 Compare August 1, 2016 19:29

bf4 reviewed Aug 1, 2016
View reviewed changes

bf4 mentioned this pull request Aug 3, 2016

Add tests for fields option demonstrating usage on both attributes and relationships #1839

Merged

bf4 added V: 0.10.x C: Relationships labels Aug 5, 2016

bf4 force-pushed the smarter_association_id_lookup branch from cb515f8 to 282ac72 Compare August 7, 2016 21:15

bf4 reviewed Aug 7, 2016
View reviewed changes

NullVoxPopuli reviewed Aug 8, 2016
View reviewed changes

bf4 reviewed Aug 17, 2016
View reviewed changes

bf4 force-pushed the smarter_association_id_lookup branch from 32dbfa9 to 93fa05b Compare August 18, 2016 07:04

bf4 reviewed Aug 18, 2016
View reviewed changes

This was referenced Aug 31, 2016

Refactor Association into Field like everything else #1897

Merged

Organize Serializer concerns into concerns/ #1898

Merged

bf4 force-pushed the smarter_association_id_lookup branch 2 times, most recently from ce5c4b8 to 919380e Compare September 27, 2016 15:26

bf4 force-pushed the smarter_association_id_lookup branch from ce5c4b8 to 919380e Compare September 27, 2016 15:27

bf4 mentioned this pull request Nov 22, 2016

Make test attributes explicit #1984

Merged

This was referenced Jan 13, 2017

Refactor Association to make it eval reflection JIT #2026

Merged

'data.relationships.data' has all attributes of relationship by default when it shouldn't #2027

Open

bf4 mentioned this pull request Mar 15, 2017

Make serializer interface more obvious #2074

Merged

ssendev and others added 2 commits April 30, 2017 18:11

belongs_to causes unnecessary db hit

273b7e7

Skip eval relationships object on belongs to

6e41528

bf4 force-pushed the smarter_association_id_lookup branch from ce5c4b8 to 6e41528 Compare April 30, 2017 23:32

bf4 merged commit 7d0f4e0 into rails-api:master Apr 30, 2017

bf4 deleted the smarter_association_id_lookup branch April 30, 2017 23:50

furkanayhan mentioned this pull request Jun 12, 2017

Implement setting for including belongs_to relationship resource iden… #2151

Closed

furkanayhan mentioned this pull request Jun 28, 2017

JSONAPI - Wrong "type" for belongs_to polymorphic relationships #2160

Closed

andyhite reviewed Jun 30, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bf4 commented Jul 26, 2016 •

edited

Loading

mention-bot commented Jul 26, 2016

NullVoxPopuli commented Jul 26, 2016

bf4 Aug 1, 2016

NullVoxPopuli Aug 7, 2016

bf4 commented Aug 7, 2016

bf4 Aug 7, 2016 •

edited

Loading

bf4 Aug 7, 2016

bf4 commented Aug 8, 2016

bf4 commented Aug 8, 2016

sandstrom commented Aug 8, 2016

bf4 commented Aug 8, 2016

NullVoxPopuli Aug 8, 2016

bf4 Aug 9, 2016

NullVoxPopuli Aug 9, 2016

bf4 Aug 17, 2016

bf4 Aug 18, 2016

NullVoxPopuli Aug 18, 2016

bf4 Aug 18, 2016

NullVoxPopuli commented Aug 31, 2016

bf4 commented Sep 27, 2016

furkanayhan commented Jun 16, 2017 •

edited

Loading

andyhite Jun 30, 2017

		reflection_options[:include_data] = @_include_data
		reflection_options[:new_options] = {

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

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

Conversation

bf4 commented Jul 26, 2016 • edited Loading

mention-bot commented Jul 26, 2016

NullVoxPopuli commented Jul 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Aug 7, 2016

bf4 Aug 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Aug 8, 2016

bf4 commented Aug 8, 2016

sandstrom commented Aug 8, 2016

bf4 commented Aug 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Aug 31, 2016

bf4 commented Sep 27, 2016

furkanayhan commented Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

bf4 commented Jul 26, 2016 •

edited

Loading

bf4 Aug 7, 2016 •

edited

Loading

furkanayhan commented Jun 16, 2017 •

edited

Loading