Serialization issues for self referencing models

[kengreeff]

Hey Maximo,

I'm seeing a strange bug when using a self referencing model:

# app/models/comment.rb

class Comment < ApplicationRecord
  belongs_to :parent_comment, class_name: "Comment", optional: true

  has_many :replies, class_name: "Comment", foreign_key: "parent_comment_id", dependent: :destroy
end

A snippet of CommentSerializer looks like this:

# app/serializers/comment_serializer.rb

class CommentSerializer < BaseSerializer
  attribute :type do "Comment" end

  attributes(
    :body,
    :parent_comment_id,
  )

  attribute :replies, if: -> { expand.include?('replies') } do
    CommentSerializer.render(comment.replies)
  end
end

When using the same serializer here we end up with the parent comment and the reply both having the same attributes:

[
    {
        "id": 543,
        "type": "Comment",
        "parent_comment_id": null,
        "asset_files": [],
        "replies": [
            {
                "id": 544,
                "type": "Comment",
                "parent_comment_id": 543,
                "body": "Test reply"
            }
        ],
        "body": "Test reply"
    }
]

This can be fixed if by using a different serializer for the replies like:

class CommentSerializer < BaseSerializer
  attribute :type do "Comment" end

  attributes(
    :body,
    :parent_comment_id,
  )

  attribute :replies, if: -> { expand.include?('replies') } do
    CommentReplySerializer.render(comment.replies)
  end
end

class CommentReplySerializer < CommentSerializer
end

Or you can manually define the attributes like so:

attribute :body do
  comment.body
end

I've been browsing the source code but am not sure why it works when defining individual attributes and not when using the attributes method.

Wondering if allowing the ability to set the instance_key would fix or if there is a simpler fix I am not seeing.

Let me know if you have any ideas :)

[ElMassimo]

Hi Ken!

To be fair, I never had a use case like this, so it's the first time I've even thought about re-entrancy.

To reduce allocations and memory usage, this library reuses the same serializer instance every time a model is serialized.

The problem you are seeing occurs because while rendering the first comment, once CommentSerializer.render(comment.replies) is called, it will change the inner comment being rendered to the first reply (and subsequent ones), so by the time it moves on to rendering the next attribute for the first comment, the inner object in the serializer is the last reply (instead of the first comment).

The workaround of using CommentReplySerializer < CommentSerializer is nice. Because it's a different class, a different instance will be used, and the problem would no longer occur.

---

At this point I'm not sure whether I'd want to fully support re-entrancy, or whether I'd prefer to raise an exception when a serializer instance is "re-bound" while still rendering a previous object.

I'm leaning towards the latter, as nesting the same serializer is unusual, and we could suggest using that workaround, while preventing folks from wasting a lot of time trying to figure out what's going on.

---

why it works when defining individual attributes and not when using the attributes method.

That might appear to be the case, but maybe it's the order in which the attributes are rendered that makes it seem like so.

[kengreeff]

Yeap, I noticed the object reuse. The instance_key code triggered my solution as I noticed it was using the class name in the key:

def instance_key
  @instance_key ||= begin
    # We take advantage of the fact that this method will always be called
    # before instantiating a serializer, to apply last minute adjustments.
    prepare_serializer

    "#{name.underscore}_instance_#{object_id}".to_sym
  end
end

Do you reckon you could provide an escape hatch where you could dynamically set the instance key (maybe users could just override the method in BaseSerializer?). I probably need to dig deeper and see if ruby allows to customize the class name.

I think self-referencing is a reasonably common pattern, especially if you work with models that can be templates but understand if it is not something you want to support.

If you want to leave as is let me know and I am happy to add docs. Just need to decide how you want to refer to it e.g. self referencing models, re-entrancy etc.

[ElMassimo]

I'd prefer to raise an exception when a serializer instance is "re-bound" while still rendering a previous object.

I'd like to add this, as it will save people time when running into this situation, and the error message could suggest the simple workaround.

If you want to leave as is let me know and I am happy to add docs

That would also be nice, self-referencing models will be easier to understand (re-entrancy only makes sense from the perspective of the internal methods).