Skip to content

Conversation

@gaorlov
Copy link

@gaorlov gaorlov commented Jun 4, 2018

First and foremost: thanks so much for putting this gem out here!

Problem Statement

I have a service that uses Elasticsearch instead of ActiveRecord and I would like to return the result set counts. Because ActiveRecord::Relation is referenced directly in the counting method, the app explodes. Under the current implementation, my options are to:

  • monkeypatch Pagination.count_records in every Elasticsearch-backed service I write
  • do the counting in the controller and pass it into jsonapi_render via options[:count]

Option one is really a strawman, and it felt odd to sprinkle options: { count: MyEsModel.count( params ) } in every single controller action across multiple controllers in several projects.

Proposed Contribution

I extracted the underlying counting interface in to a RecordCounter module that responds to count( records, params = {}, options ={} ) and loops through dedicated Counter classes that know how to count a specific class type. So like, ActiveRecord::Relation objects go to the ActiveRecordCounter and Arrays go to the ArrayCounter class.

The #{Type}Counter classes register themselves with RecordCounter on declaration with counts #{Type.to_s.underscore} making adding new counters as easy as just declaring them in your project.

What this does

I will admit that this is a somewhat involved change, but there are several benefits to this added complexity. It:

  • eliminates the hard dependency on ActiveRecord::Relation in the counting mechanism
  • allows for easy extensions to counting, both in adding counting for other backend technologies, as well as modifying existing counting functionality without having to modify JSONAPI::Utils

Example

To use my use case, an Elasticsearch-backed system would implement a counter approximately like so:

class ElasticsearchCounter < JSONAPI::Utils::Support::Pagination::RecordCounter::BaseCounter
  counts "elasticsearch/persistence/repository/response/results"

  # the one thing we have to define
  def count
    # not the actual implementation 
    model_class = @records.first.class

    query = model_class.query_from_params @params

    model_class.count query
  end
end

gaorlov added 10 commits May 31, 2018 09:38
updating testing instructions to run all the tests
Moving counting logic into (.*)Counter classes to allow for extensibility
Moving countind delegation into RecordCounter class to allow for a single entrypoint
Passing params into RecordCounters to allow them to do complex counting based on the query
moving filter application out of *Counter classes due to scoping issues
updating instructions to run tests
allowing apply_sort to fall through with no modifications for unexpected result sets
fixing RecordCounter.count to pass request params into the counter classes
@gaorlov
Copy link
Author

gaorlov commented Jun 27, 2018

@tiagopog, I would love some feedback on this.

@gaorlov
Copy link
Author

gaorlov commented Aug 1, 2018

@tiagopog bump?

@gaorlov
Copy link
Author

gaorlov commented May 1, 2019

@tiagopog have you had a chance to look at this at all?

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.

1 participant