Infinite loop in circular associations #211

Closed
heartsentwined opened this Issue Feb 20, 2013 · 72 comments
@heartsentwined

active_model_serializers can handle self-referencing associations fine, e.g. categories, but it will run into an infinite loop if you try to sideload circular associations. An example is the twitter-esque follower-followed association pair.

class BaseSerializer < ActiveModel::Serializer
  embed :ids, include: true
end

class FollowerSerializer < BaseSerializer
  has_many :followeds
end

class FollowedSerializer < BaseSerializer
  has_many :followers
end

Let's say you start by requesting a follower. active_model_serializers will traverse its followeds, sideload the followed models by initiating a new FollowedSerializer for each, and if any followed points back to the original follower via its own has_many :followers association, we have an infinite loop.

The current workaround is, unfortunately, disable sideloading entirely for the offending associations.

class BaseSerializer < ActiveModel::Serializer
  embed :ids, include: true
end

class FollowerSerializer < BaseSerializer
  has_many :followeds, embed: :ids, include: false
end

class FollowedSerializer < BaseSerializer
  has_many :followers, embed: :ids, include: false
end

Perhaps keeping track of the model + primary key while loading associations (recursively) can solve this issue? This way the same model will only be serialized once.

@darthdeus

Funny, I ran into this exact issue about 2 hours ago, using the same exact follower/following structure :)

@heartsentwined

Can I +1 myself? :-)

@samuil

It seems that as of 0.7.0 this issue is not anymore present.

@steveklabnik

Thanks!

@heartsentwined

Confirmation: it has indeed been fixed in 0.7.0. Kudos to whoever that patched the issue!

@codezomb

This is still an issue, the following results in a stack level too deep error. This is using version 0.7.0.

target_serializer.rb

class TargetSerializer < ActiveModel::Serializer
  attributes :email, :first_name, :last_name
  has_many :lists
end

list_serializer.rb

class ListSerializer < ActiveModel::Serializer
  attributes :name
  has_many :targets
end
@steveklabnik

@gitt Can you try against master, please? I plan on releasing over the weekend.

@codezomb
@steveklabnik

Thanks. <3

@codezomb

@steveklabnik Same response stack level too deep.

SystemStackError - stack level too deep:
(gem) actionpack-3.2.13/lib/action_dispatch/middleware/reloader.rb:70:in `'

65       response = @app.call(env)
66       response[2] = ActionDispatch::BodyProxy.new(response[2]) { cleanup! }
67       response
68     rescue Exception
69       cleanup!
70       raise
71     end
72 
73     def prepare! #:nodoc:
74       run_callbacks :prepare if validated?
75     end
@codezomb

Just tested with embded: :ids, include: false. This works. Without include: false, it fails.

class ListSerializer < ActiveModel::Serializer
  attributes :name
  has_many :targets, :children, embed: :ids, include: false
end

class TargetSerializer < ActiveModel::Serializer
  attributes :email, :first_name, :last_name
  has_many :lists, embed: :ids, include: false
end
@steveklabnik steveklabnik reopened this May 3, 2013
@steveklabnik

Word, thanks. I'll try to look into it...

@searls searls added a commit to searls/active_model_serializers that referenced this issue May 4, 2013
@searls searls A test to try to replicate @gitt's issue in #211
Notice that `#test_embed_ids_include_true` does actually pass when
side-loading (using `:embed => :ids & :include => true`)

I also included `#test_default_circular_nesting`, which fails 
by raising `SystemStackError` when naively using default nesting
on a circular reference. Is this behavior expected?
7bdf87f
@ianpetzer

+1 to stay updated

@sethiele

+1
I have the same problem

@Foxandxss

Hello, I used a workaround to this problem, but is not scalable, but in the meantime could be useful:

The problem is that serializers can't reference each other so I created two serializers per side.

So, in the typical post - categories, N-M, I have this:

class PostSerializer < ActiveModel::Serializer
  attributes :id, :title, :body

  has_many :categories, serializer: CategoriesForPostSerializer
  embed :ids, include: true
end

class CategoriesForPostSerializer < ActiveModel::Serializer
  attributes :id, :name

  has_many :posts, embed: :ids
end

So, I have this side covered, I have a list of posts which have every Category sideloaded and every category has the posts ids. Notice I'm not using the Categories serializer. Now, I need to fetch the categories with the posts (which have the categories ids).

class CategorySerializer < ActiveModel::Serializer
  attributes :id, :name

  has_many :posts, serializer: PostsForCategorySerializer
  embed :ids, include: true
end

class PostsForCategorySerializer < ActiveModel::Serializer
  attributes :id, :title, :body

  has_many :categories, embed: :ids
end

So, for every side, I have two serializers, that prevents the cross references and it won't break.

So far this works for me in Ember but It will get out of control soon.

@localredhead

I experience this as well while modeling a product catalogue with categories

@davidcelis

Happens in my app too, for a HABTM where I'd like to embed the objects themselves on both sides rather than IDs.

@codezomb

Just stopping by to see if there's a resolution for this coming. We'd like to use AMS, but having to use 2 serializers just isn't a feasible option.

@joeellis

Also ran into this. As @codezomb mentioned, has_many :lists, embed: :ids, include: false seems to work, but has_many :lists, embed: :objects, include: false causes the recursive loop.

@Addiction2Code

I used a solution similar to @Foxandxss, as long as you specify a serializer for both models in question and you make sure you list your attributes, excluding the model / model_id column, you should be good to go. This is an annoyance, but at-least there's a workaround. Hopefully a fix comes soon.

@bruce

Also a problem for me; the two serializers approach is a pretty clunky workaround

@ajbraus

+1

@ajbraus ajbraus referenced this issue in Sutto/rocket_pants Sep 28, 2013
Closed

rocket_pants hanging #76

@ajbraus

The real solution here is to use a more granular JSON generator like RABL

@davidcelis

The real solution here is never to embed. Just make a separate API request using your _links. It takes what, 3ms?

@mateusmaso

@davidcelis is right, try to avoid deep relationships when nesting serializers for now. Lazy loading is the way to go.

@codezomb

The real solution here is never to embed. Just make a separate API request using your _links. It takes what, 3ms?

This is not always feasible. There may be situations where you don't want an API endpoint to that object, or you may need to build an object from multiple model pieces.

@arthurnn arthurnn was assigned Oct 21, 2013
@jackquack

I don't really understand how this was ever fixed. I get this error with 0.8.1 with a very simple

// UserSerializer.rb

has_one :user_profile

// UserProfileSerializer.rb

has_one :user

That's maybe the most basic implementation.

I see in the stack trace that there is a call to

check_for_circular_references <ActiveSupport::JSON::Encoding::Encoder#check_for_circular_references(value)>

Is there a way to get that to work as expected?

@jackquack

I found a way to avoid circular associations: jackquack@92847af

I add the parent association to the :except option object passed in to the child association serializer. I have not tested this extensively, it was just a first shot at it. Can anyone think of any potential flaws with the idea as implemented?

Update 1: It will not work with has_many associations.
Update 2: I have hacked my fix some more to have it work with has_many associations, but there is still an issue. There are apt to be problems in the case where a model has two associations that are of the same model. For instance has_many :users AND belongs_to :user at the same time. If anyone knows a better way to get the association type between a parent and a child association, let me know. jackquack@4e9471a

@ezranuite

I found a temporary fix to be

class ObjectaSerializer < ActiveModel::Serializer
  self.root = false

  attributes :objectb

  def objectb
    ObjectbSerializer.new(object.objectb, {except: :objecta})
  end
end

And vice-versa. This avoids having to write two serializers, but may have other side-effects I'm not aware of and may have a simpler implementation.

@jackquack

@ezranuite That looks pretty solid, but I'm really no expert at all.

The one edge case that your solution isn't resistant to, is a case in which a circular reference is created that involves 3 objects

objectA -> objectB -> objectC -> objectA

In this case, objectC receives the instruction to not include objectB. So, including objectA again is acceptable, and so an infinite loop occurs.
Now, you could bust that by passing along an array of models to exclude, like so:

def objectb
  ObjectbSerializer.new(object.objectb, {except: options[:except].push(:objecta)})
end

So, in that situation, an array is built up and passed alone at each level of nesting. But, even that solution has a problem with some cases, like:

objectA -> objectB -> objectC -> objectA'

In my second example, objectA' is another instance which is the same time as objectA. So, the inclusion of objectA' in the final JSON structure is legitimate (not representing a loop), but it would be prevented with the array solution.
This is a very distant edge case, but I would argue that any really good solution should take in to account all these cases.

I suppose I should just read up on how to prevent cycles in direction graphs, but :(

@ezranuite
@shekibobo

Is there a way to figure out if my serializer is being called from another serializer? For instance, I only want to include certain embedded associations if this serializer isn't already being embedded in another serializer. I think this would help pare down these infinite loops, at least on a case-by-case basis.

Something like this:

class MeetingSerializer < ApplicationSerializer
  attributes  :id, :title, :notes, :starts_at, :ends_at, :created_at, :updated_at

  has_many :users, embed: :ids, embed_in_root: (self.top.class != UserSerializer)
end

Where top is the top-level serializer that was called in the controller.

This could also help with excessive side-loading associations (prevent loading the full object graph unless you really want to).

@joefiorini joefiorini added a commit to joefiorini/reproduce_ams_issue that referenced this issue Jan 21, 2014
@joefiorini joefiorini Reproduced issue in rails-api/active_model_serializers#211 ed5b3c4
@joefiorini

I feel this issue is defeating me at the moment. Our architecture is so tied to AMS that I really would like to help fix this, but right now I have no idea where to start. Here's what I do know:

I get stack level too deep errors anytime I try to express a standard has_many/belongs_to relationship in my app.

This was actually really easy to reproduce. You can clone a failing example at https://github.com/joefiorini/reproduce_ams_issue. The serializers are basically:

class PostSerializer < BaseSerializer
  has_many :comments

end
class CommentSerializer < BaseSerializer
  has_one :post

end

Any thoughts?

@stevenharman
@jimmycuadra

It essentially does, since AMS doesn't distinguish between belongs_to and has_one. If you want to have an endpoint that returns a comment and its related post, this makes sense.

@stevenharman
@jackquack

@joefiorini

The solution that works for me, is the following: Make changes to the associations.rb Associations module method called find_serializable like so:

    def find_serializable(object)
      if target_serializer
        target_serializer.new(object, source_serializer.options)
      elsif object.respond_to?(:active_model_serializer) && (ams = object.active_model_serializer)
        source_association = source_serializer.object.class.to_s.underscore.downcase
        plural_source_association = source_association+'s'
        except = ams._associations.keys.map(&:to_s).find do |association|
          association == source_association || association == plural_source_association
        end
        _options = source_serializer.options
        _options[:except] = Array(_options[:except])
        if not except.nil?
          if not _options[:except].include? except.to_sym
            _options[:except].push(except.to_sym)
          end
        end
        ams.new(object, _options)
      else
        object
      end
    end
  end

This will prevent circular associations in a bit of a hacky way. Model A simply tells its associations to exclude it from their associations. So, A will include B, but B is prevented from including A. There are edge cases where it doesn't work, but it works for us in production.

@MikeSilvis

@jackquack +1 for this. Please create a PR with this change.

@jackquack

@MikeSilvis I would love to, especially because I use my fix in production and I really like AMS.

But the problem is that my solution isn't perfect. It has 1 edge case: Model A including Model B, which in turn wants to include a different instance of Model A (call it A'). So, B should be able to include A! if it wants to, but would be prevented by my 'fix'.

The edge case that my 'fix' fails is much harder to explain, reproduce, discover and work around. I think it would cause deeper issues were anyone to use it without understanding it. Infinite loop in circular associations is actually pretty straightforward and has simple (but verbose!) workarounds.

P.S.
While writing this, I thought of a potential fix for my edge case: I think if my fix were to actually use references to the model instances themselves, instead of using the model's class names, it might solve my edge case. I'll try to find some time to test.

@tibbon

+1 on a fix here. Hitting this problem currently

@KurtRMueller

+1. Is there a guide for how ams should handle many-to-many relationships?

Trying to get get into Ember and I'm currently stuck at this point.

@timurvafin

Guys,

I would like to introduce custom solution we used for AMS right now.

We decided to include in the generated JSON only associations declared in the serializer_includes option. So we could manually avoid circular references.

Here is a basic example from https://github.com/fs/rails-base-api/tree/examples

class PostSerializer < BaseSerializer
  attributes :id, :title, :text

  has_many :comments
end

class CommentSerializer < BaseSerializer
  attributes :id, :title, :text

  has_one :post
end

class PostsController < ApplicationController
  def index
    respond_with(
      posts,
      serializer_includes: {
        post: [:comments],  # enable "comments" association for PostSerializer
      }
    )
  end
end

class CommentsController < ApplicationController
  def index
    respond_with(
      comments,
      serializer_includes: {
        comment: [:post] # enable "post" association for CommentSerializer
      }
    )
  end
end

@steveklabnik if you find this reasonable I could create PR for that.

@steveklabnik steveklabnik removed the accepted label Aug 22, 2014
@steveklabnik

@timurvafin no, it should handle this without manual intervention.

@brancusi

+1. I'm stuck on this as well. I did a bundle update and it introduced this bug. Not sure what happened.

Hope there is a fix soon.

Thanks for the great Gem.

@dinsley

+1 for this, is this being poked at currently?

@steveklabnik

I am not actively working on the 0.9 branch, I don't know what others are doing. This is an important bug, for sure.

@dinsley

is this being fixed for 0.10? if no ones working on it, i can look into it, we'll need this fairly soon so i'll have some time to spend on it. just want to know where I should branch off of.

@steveklabnik

0.10 is a re-write, and it's not far enough to have the problem yet 😉

@chrise86

Have there been any developments on this? It's quite a big problem 😔

@jackquack

I have an idea for a generic solution if someone wants to code it up. If a graph object can be build (while the serialization is happening) that represents the associations between objects, then at each association, the graph could be checked to make sure there aren't any cycles. If traversing to the association would result in a cycle, then that association is skipped because a cycle would result in an infinite loop.

@colby-swandale colby-swandale pushed a commit that referenced this issue Sep 11, 2014
Colby Swandale circular bug fix, see: #211 2e791b6
@opsb

Been using the fork from @colby-swandale, it's working great, many thanks!

@colby-swandale

@opsb the code was taken from @jackquack so credit where it's due, and also note that commit is on the 0.8 branch so some documented features may not be available.

@baruchlubinsky

I know this isn't exactly the same bug. But I also ran into a infinite loop by having a field named options that is a Postgres json type field. More information about my code on stackoverflow. Hopefully this can be fixed at the same time, or else it should be documented as a limitation of the gem.

@mrloop mrloop added a commit to mrloop/active_model_serializers that referenced this issue Dec 20, 2014
@mrloop mrloop reverts f7f5e29
Makes AMS usable for me with rails 4.2 and current serializer which have
circular dependences.
rails-api#211

Longer term plan is to load all ember data associations asychronously
and then no need to embed associated models.
a1ca2af
@caspyin

Does #722 fix this same issue? It changes lib/active_model/serializer/adapter/json_api.rb where @colby-swandale changes lib/active_model/serializer/associations.rb. I'm not familiar enough with the inner workings of this project to know for sure.

#722 has already been pulled into master so if it does fix this issue, it would be nice to have it applied to 0.8.x and 0.9.x branches.

@kurko @ggordon

@ggordon

@caspyin #722 is specifically for the json-api adapter in master so it's not applicable to 0.8 or 0.9 branches.

@caspyin

@ggordon Ah ok, thanks for clarifying.

@kurko
rails-api member

@caspyin could you open a PR? I'd me happy to merge :)

@caspyin

@kurko You might look at @colby-swandale. He has a fork with a solution 2e791b6 for this already. @colby-swandale consider updating your fork and issuing a PR?

@evolve2k

Wow, we're almost at a two year anniversary on this issue, having been open back on 20 Feb 2013.
It's obviously affecting a lot of people, Im sure it's hairy, but would be awesome if it could be resolved.

I'm laying down the gauntlet, the challenge is to get this resolved before it hits two years old on 20 February 2015.

It's a 20 day challenge. Massive props to anyone who can help get this sorted.

@colby-swandale

I spent a few hours today to do a bit of an analysis to see if i could get any progress on this. There are a number problems with the design of the gem that is going to make this difficult to fix.

The biggest problem is keeping track of what associations are made with what serializes. I have a patch that makes every serializer have a reference to the instance of its parent serializer so i have a trace of what serializers are related to what, but i cannot do this for the first serializer. Its initialized differently and its the only roadblock i have from having a good solution to this problem. Is anyone else working on this? maybe we could work together on this.

@j4rs

Why don't just use the relationship-attribute to access the object it belongs to?. In the example a comment accesses the post it belongs to using the attribute post avoiding the has_one:

class PostSerializer < ActiveModel::Serializer
  attributes :id, :title, :text
  has_many :comments
end

class CommentSerializer < ActiveModel::Serializer
  attributes :id, :title, :text, :post
end
@chrisccerami

Crazy this still hasn't been solved. I'm using the multiple serializers workaround, but I'd still love to see this fixed as I think overall AMS is a great tool.

@daino3

+1 for @j4rs solution which works for me, but feels... awkward...

@saneshark

I did something like this in 0.9 branch:

  def initialize(object, options={})
    self.class.has_one(:event) unless options[:exclude_event]
    super
  end

But technically you could do object.association(:association_name).loaded? I would think just as well.

@wallaceicy06

I just wanted to chime in and say I had a very difficult time dealing with this issue. I understand why stack depth becomes a problem for serializers that include their collections from their many-to-many relationships. What I don't understand is why the stack explodes when I choose to not include the collections at all.

For example:

class JackSerializer < ActiveModel::Serializer
  attributes :id, :name
  has_many :users, include: false, embed: :id
end

class UserSerializer < ActiveModel::Serializer
  attributes :id, :username, :first_name, :last_name
  has_many :jacks, include: false, embed: :id
end

These associations cause an infinite recursion loop, and I don't understand why that should be a problem. If neither of these is embedding the other (except for the id), why would the serializer feel the need to make a recursive serialization call to the other? The generated JSON for one of these would look something like the following

{
  user: {
    id: 1
    username: "joeblow"
    first_name: "Joe"
    last_name: "Blow"
    jack_ids: [1]
      0:  1
  }
}

Nowhere in that JSON does the serializer need to make a recursive serialization step to serialize the "jack" object. Yet the library still appears to be doing this.

@joaomdmoura
rails-api member

@wallaceicy06 we have being really focused on the new 0.10.x version. btw I would totally recommend you to check it out and give it a try, it is being wrote from the scratch. We already have a RC2 and you can also give master a try 😄

We know there are some important issues on older versions of AMS, and we want to help you to fix it, but more important than that we also want to finish the new version.

Could you described which version of AMS, Ruby and Rails are you using? It will help us when trying to replicate the error in the future.

@wallaceicy06

@joaomdmoura I actually have a workaround using a custom serializer like mentioned above, so it's "fixed". I will check out master for sure. Keep up the great work, as this is an awesome gem!

I am on Rails 4.2.1 and AMS 0.9.3.

@joaomdmoura
rails-api member

Awesome! Great, in case of any problems just let us know, you can also use the new docs, it's not that big yet, but we are working on it.

@NullVoxPopuli

this is solved on master, via a new include options, where you define the nested associations.
Side loading is coming later this week.

@NullVoxPopuli

Closing, as using include prevents infinite looping.

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