Allow decorating objects after pagination

[mamhoff]

This simplification allows decorating objects after they are paginated, without losing the correct total object count.

I'm using an instance variable on the including controller here, because the decorating the paginated collection will have us lose the instance variable we set on it.

Here's the case where this happens: We have a complex ActiveRecord collection that we run through Ransack and Kaminari, but before rendering we want to convert each object in it using a SimpleDelegator.

Here's a simplified version of the controller action we're looking at:

  class UserDecorator < SimpleDelegator
    def fantastic_for_rendering
      "Whoah"
    end
  end

  def index
    allowed_fields = [
      :first_name, :last_name, :created_at,
      :notes_created_at, :notes_quantity
    ]
    options = { sort_with_expressions: true }

    jsonapi_filter(User.all, allowed_fields, options) do |filtered|
      result = filtered.result
      jsonapi_paginate(result) do |paginated|
        paginated = paginated.map { |user| UserDecorator.new() }
        render jsonapi: paginated
      end
    end
  end

What is the current behavior?

When modifying objects after they are paginated, we lose the total count and get wrong pagination information.

What is the new behavior?

In this and other cases, we get correct pagination behaviour.

Checklist

Please make sure the following requirements are complete:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes /
  features)
• All automated checks pass (CI/CD)

[stas]

@mamhoff jsonapi_paginate() works on plain ruby arrays:
https://github.com/stas/jsonapi.rb/blob/master/spec/dummy.rb#L97

Have you considered doing the decoration before the array is passed to the method?

Eg.:

  class UserDecorator < SimpleDelegator
    def fantastic_for_rendering
      "Whoah"
    end
  end

  def index
    allowed_fields = [
      :first_name, :last_name, :created_at,
      :notes_created_at, :notes_quantity
    ]
    options = { sort_with_expressions: true }

    jsonapi_filter(User.all, allowed_fields, options) do |filtered|
-      result = filtered.result
+      result = filtered.result.to_a.map { |user| UserDecorator.new() }
      jsonapi_paginate(result) do |paginated|
-       paginated = paginated.map { |user| UserDecorator.new() }
        render jsonapi: paginated
      end
    end
  end

[mamhoff]

Have you considered doing the decoration before the array is passed to the method?

Yes - the problem is that the decoration materializes the ActiveRecord scope before pagination, and we instantiate and decorate way more objects than we need, leading to a pretty large performance problem, unfortunately.

[stas]

Yes - the problem is that the decoration materializes the ActiveRecord scope before pagination, and we instantiate and decorate way more objects than we need, leading to a pretty large performance problem, unfortunately.

Ok @mamhoff but how is this an issue this project should solve? Could you just provide the right object interface to mimic an enumerable/array and do your jazz in the wrapper/decorator implementation on your side?

I'm just confused why we need to bend this generic (and pretty flexible already) library to some specific use-case that just you guys have internally? 🙈

[mamhoff]

Sure, we can work around this issue in our local project; I found the fix I'm proposing here more elegant than writing that wrapper. I was confused about why the pagination was off, and only by digging through the source of this library did I find that we're making an assumption that the object being paginated is - and stays - and ActiveRecord relation between pagination and rendering OR is - and stays - an array between the two steps. I understand where you're coming from, but I don't see where this PR introduces more complexity to this library.

[stas]

The proposed changes are removing fixes for known and performance issues, like unscoping an ActiveRecord collection.

It also moves the caching of the total from the passed object, into the instance of the controller, which could lead to other issues with mutability/caching.

If you can work around the challenges you are facing in your project, I suggest taking that route, since I don't see a lot of upside in moving forward with this changeset 🙏

[mamhoff]

The proposed changes are removing fixes for known and performance issues, like unscoping an ActiveRecord collection.

I've looked through the commit history to find those issues, but I couldn't find any that this changeset breaks. I do calculate the total size of the collection before paginating ("scoping") it, making the need for unscoping unnecessary. This will issue a count query to the database, just like calling size on an unscoped ActiveRecord collection will. I don't think this leads to a noticeable change in performance. Am I missing something here?

It also moves the caching of the total from the passed object, into the instance of the controller, which could lead to other issues with mutability/caching.

To the contrary: This fixes an issue with mutability. The current code in the library mutates the collection being passed in, with the consequence that I can't safely use map on an Array. While adding an instance variable to the controller looks hamfisted and unelegant, I think it's cleaner to be explicitly unelegant than implicitly adding instance variables to things that don't expect to have instance variables and thus cannot deal with it when e.g. mapping over them.

If you can work around the challenges you are facing in your project, I suggest taking that route, since I don't see a lot of upside in moving forward with this changeset pray

It turns out that because jsonapi.rb performs the pagination, I would have to make an object that conforms very, very closely to the current implementation of the library (e.g. I'd have to name an instance variable of my class @original_size, and then I'd have to find a way to protect against that implementation detail changing in the library) The current fix is overwriting jsonapi_paginate and jsonapi_pagination_meta in our project: AlchemyCMS/alchemy-json_api@4a273fb.

We're great fans of this library and really, really do value and appreciate your work on it. As a maintainer myself, I understand the value of being conservative with accepting change. However, in this particular case I have a hard time understanding the downsides of this change.