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

Adds a configuration option belongs_to_uses_id_on_self #2140

Closed
wants to merge 2 commits into from
Closed

Adds a configuration option belongs_to_uses_id_on_self #2140

wants to merge 2 commits into from

Conversation

cassiascheffer
Copy link
Contributor

Purpose

Adds a configuration option belongs_to_uses_id_on_self to allow for automatic serializer lookup. This solves an issue where the custom type attribute was not being used as the type was inferred from the associated object.

Changes

  • Adds ActiveModelSerializers.config.belongs_to_uses_id_on_self
  • Adds documentation for usage

Caveats

  • Setting belongs_to_uses_id_on_self to true will result in a lookup on the associated object,

Related GitHub issues

Closes #2125

Additional helpful information

- adding documentation and changelog
- setting belongs_to_uses_id_on_self to false by default
@cassiascheffer cassiascheffer changed the title Ck 2125 type attribute on bleongs to Adds a configuration option belongs_to_uses_id_on_self May 22, 2017
Copy link
Member

@bf4 bf4 left a comment

Choose a reason for hiding this comment

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

Looks good. Some discussion on improving examples.

@@ -28,6 +28,31 @@ Possible values:

When `false`, serializers must be explicitly specified.

##### belongs_to_uses_id_on_self

Enable automatic serializer lookup when using a `belongs_to` without `has_many`.
Copy link
Member

Choose a reason for hiding this comment

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

Tells the JSON:API adapter to build belongs_to relationship id and type using the foreign key on self, rather than loading the association.
For example, a Comment that belongs to a user will have a user_id attribute.
This means the relationship will the call object.user_id instead of object.user.id, which can prevent unneeded queries.
IMPORTANT: Because the association is no longer be loaded, any type must come from the belongs_to options as type: :comment_users or serializer: CommentUserSerializer (which would call CommentUserSerializer._type, i.e. no instance methods)

or something to that effect. Examples below are good.


```ruby
class PostSerializer < ActiveModel::Serializer
belongs_to :foo, serializer: FooSerializer
Copy link
Member

Choose a reason for hiding this comment

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

examples should both be belongs to :blog, but one uses the type option and one uses the serializer option, and then show how they differ in their serialized relationship given a Post with blog_id = 1 and different BlogSerializer configurations, and what happens when both are given, or if the relationship is included...

end
class BelongsToBlogWithCustomTypeModelSerializer < ActiveModel::Serializer
type :posts
belongs_to :blog_with_custom_type
Copy link
Member

Choose a reason for hiding this comment

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

also good to include an instance method json_key to show it's not used

belongs_to :blog_with_custom_type
end

def test_belongs_to_allows_type_overwriting
Copy link
Member

Choose a reason for hiding this comment

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

test_belongs_to_doesnt_load_record should also change, no, since it requires belongs_to_uses_id_on_self to be true, no?

@@ -43,7 +43,8 @@ def data_for(association)
end

def data_for_one(association)
if association.belongs_to? &&
if !ActiveModelSerializers.config.belongs_to_uses_id_on_self &&
Copy link
Member

Choose a reason for hiding this comment

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

should be

-          if !ActiveModelSerializers.config.belongs_to_uses_id_on_self &&
+          if ActiveModelSerializers.config.belongs_to_uses_id_on_self &&

@cassiascheffer
Copy link
Contributor Author

@bf4 Just to clarify, do we wantbelongs_to_uses_id_on_self to be false` by default? If it's false by default, the less performant behaviour will be the default.

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.

2 participants