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

Serializer pulls relations when belongs_to declared but not included #1552

Closed
youroff opened this issue Mar 3, 2016 · 18 comments · Fixed by #2026
Closed

Serializer pulls relations when belongs_to declared but not included #1552

youroff opened this issue Mar 3, 2016 · 18 comments · Fixed by #2026

Comments

@youroff
Copy link
Contributor

youroff commented Mar 3, 2016

Expected behavior vs actual behavior

As long as belongs_to relationship is not declared in include parameter, it shouldn't be loaded during serialization. Currently it's loaded.

Steps to reproduce

class MembershipSerializer < ActiveModel::Serializer
  attributes :id, :position

  belongs_to :organization
  belongs_to :user
end

And in controller:

respond_with @memberships, meta: meta_data

Environment

ActiveModelSerializers 0.10.0.rc4
ruby 2.3.0p0 (2015-12-25 revision 53290) [x86_64-darwin15]
Rails 4.2.4

@NullVoxPopuli
Copy link
Contributor

for ember, I know it uses those ids to get the associations... but I've been reading that it's better to use links for associations -- so that would mean the associations defined in the serializer would only be used with the include option.

@rails-api/ams Do you think disabling auto-getting ids for the relationships hash should be toggleable via some option?

@youroff
Copy link
Contributor Author

youroff commented Mar 9, 2016

Providing the ids is good, because right, ember-data uses that, but in case of 'belongs_to' association it doesn't require pulling whole related objects. We have ids right on a parent model and probably their class (to generate type attribute) can be taken from reflection, I guess.

@NullVoxPopuli
Copy link
Contributor

Well, what was mentioned in #1555 is that if you have a ton of related records via has-many, it would throw that ton of ids into your model without a choice.

I think the broader issue here is that ids are always pulled in to the resulting json, but aren't always desired.

@youroff
Copy link
Contributor Author

youroff commented Mar 9, 2016

Well, yeah... maybe fields param could be useful here? Or any other mean to control what will be included in resulting json. As for pulling ids when they are needed, it's not a problem, as active_record does pretty good job with include. And if I wanted better performance for this whole thing, I would generate json right out of the database, like stored procedures in Postgresql.

@bf4
Copy link
Member

bf4 commented Mar 9, 2016

I think the bug here is same as #1555 (comment) that relationships in jsonapi should not be called when not included

@NullVoxPopuli
Copy link
Contributor

Ok, will I can confirm that this is the behavior that is currently happening, cause I see it in my ember app.

@bf4
Copy link
Member

bf4 commented Mar 9, 2016

They're not being rendered, right? Just queried?

On Wed, Mar 9, 2016 at 8:43 AM L. Preston Sego III notifications@github.com
wrote:

Ok, will I can confirm that this is the behavior that is currently
happening, cause I see it in my ember app.


Reply to this email directly or view it on GitHub
#1552 (comment)
.

@NullVoxPopuli
Copy link
Contributor

not the whole model, no. just the relationship list of ids and types per relationship.

@youroff
Copy link
Contributor Author

youroff commented Mar 9, 2016

Which is correct, I assume. The only problem that it makes unnecessary queries to db.

@NullVoxPopuli
Copy link
Contributor

yeah, @youroff at least for belongs_to -- it shoudl be capable of getting the id without querying. has_many can get pretty crazy though

@NullVoxPopuli
Copy link
Contributor

just for summary for a future PR

  • belongs_to: include id - shouldn't make query
  • has_many: should not include ids, unless relationship is present in the include option

yeah?

I think that could be two separate PRs

@youroff
Copy link
Contributor Author

youroff commented Mar 9, 2016

Not sure how it would fit current API, but would be really awesome to do something like:

  1. Always render ids and types for both belongs_to and has_many, unless there's something like fields param (or except) that says which fields are required and vice versa.
  2. Render included objects for relations if they are in include option.
  3. Collect ids for belongs_to without query. And probably some optimization can be made to has_many keys collection. Not sure about exact ways to do that, but can propose something later, or even come with PR.
    This would cover all the cases and be consistent, I think.

@beauby
Copy link
Contributor

beauby commented Mar 10, 2016

@youroff I gave a thought about 3) a while ago, and the main issue is that we somehow need to consistently infer the type of such related resource. Back in the days, the codebase didn't really allow it, but I suppose now we could simply inspect the model's associations to get the class name, then lookup the corresponding serializer, and finally get the type. The only issue is with non-ActiveRecord models.

@youroff
Copy link
Contributor Author

youroff commented Mar 10, 2016

@beauby Yeah, I played a bit and it seemed to be accessible through AR reflections. For non-ar I agree, they probably should be treated differently.

@bf4
Copy link
Member

bf4 commented Jul 26, 2016

Related:

Related code:

in JSONAPI::Resources, the relationship object has a foreign_key_on option that may be self.

docs

The relationship methods (relationship, has_one, and has_many) support the following options:

  • foreign_key - the method on the resource used to fetch the related resource. Defaults to <resource_name>_id for has_one and <resource_name>_ids for has_many relationships.
    to_one relationships support the additional option:
  • foreign_key_on - defaults to :self. To indicate that the foreign key is on the related resource specify :related.
    Examples:
class ExpenseEntryResource < JSONAPI::Resource
  has_one :currency, class_name: 'Currency', foreign_key: 'currency_code'
end

see https://github.com/cerebris/jsonapi-resources/blob/master/lib/jsonapi/relationship.rb for full code

module JSONAPI
  class Relationship
    attr_reader :foreign_key

    def initialize(name, options = {})
      @foreign_key = options[:foreign_key] ? options[:foreign_key].to_sym : nil
    end

    def belongs_to?
      false
    end

    class ToOne < Relationship
      attr_reader :foreign_key_on

      def initialize(name, options = {})
        super
        @foreign_key ||= "#{name}_id".to_sym
        @foreign_key_on = options.fetch(:foreign_key_on, :self)
      end

      def belongs_to?
        foreign_key_on == :self
      end

    class ToMany < Relationship
      def initialize(name, options = {})
        super
        @foreign_key ||= "#{name.to_s.singularize}_ids".to_sym
      end
    end
  end
end
Customizing base records for finder methods

If you need to change the base records on which find and find_by_key operate, you can override the records method on the resource class.
For example to allow a user to only retrieve his own expense_entries you can customize self.records:

  def self.records(options = {})
   context = options[:context]
   context[:current_user].expense_entries
 end

When you create a relationship, a method is created to fetch record(s) for that relationship, using the relation name
for the relationship.

  has_one :currency
 # def record_for_currency
 #   relation_name = relationship.relation_name(context: @context)
 #   records_for(relation_name)
 # end

@ajunB7
Copy link

ajunB7 commented Mar 21, 2019

Anyone else hitting this issue again in the latest release? We had no problems in 0.10.6 but are hitting this exact issue in 0.10.9.

Edit: Tested with 0.10.7-0.10.9 all experience this issue while 0.10.6 is fine

@bf4
Copy link
Member

bf4 commented Mar 21, 2019

@ajuni7 please open a new issue. I appreciate the work you've done so far to isolate the version. I'll help you fix it (discuss there) if you're up for it.

@wasifhossain
Copy link
Member

@ajuni7 can you please confirm if setting the config option introduced in v0.10.7 would solve the case?

ActiveModelSerializers.config.jsonapi_use_foreign_key_on_belongs_to_relationship = true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants