local-ch / lhs

⚛️ REST services accelerator: Rails gem providing an easy, active-record-like interface for http (hypermedia) json services
GNU General Public License v3.0
137 stars 3 forks source link

Pagination on included resources #184

Open skaestle opened 7 years ago

skaestle commented 7 years ago

situation is unclear at the moment how LHS handles included resources in regards to page-limits.

this code (sales-butler)

IS::Place
        .options(auth: { bearer: token })
        .includes(contracts: :product)
        .references(contracts: { auth: { bearer: token }, product: { auth: { bearer: token } } })
        .find_by(id: online_entry_identifier)

definition: https://github.com/local-ch/sales-butler/blob/f246f8257c3c9f506cf38130f3068e80a669bd88/app/models/is/place.rb usage: https://github.com/local-ch/sales-butler/blob/4bb4230ed56d8f930fe36d247dcd570c8bebb785/lib/is/datastore/place.rb#L50

for example will not retrieve all contracts for this place. the hrefs included uses for expansion are sent with: offset=0&limit=10. i guess the expansion should check whether there are more pages to page through... not sure to be honest!

nethad commented 7 years ago

We saw that pagination does not work properly for nested resources, i.e. pagination is ignored and only the first page is fetched.

Pseudo code:

Place.find(42).contracts

would result only 10 contracts (default limit) and you would never see contracts from further pages.

My opinion: if LHS should behave like ActiveRecord, place.contracts should always return the full collection, meanging that it would have to fetch all pages exhaustively. This could have an effect on current LHS clients that expect only the first page fetched implicitly.

ActiveRecord has an API for paged fetching: http://api.rubyonrails.org/classes/ActiveRecord/Batches.html#method-i-find_in_batches

This would result in:

place.contracts.find_in_batches

place.contracts.find_in_batches(batch_size: 10)

The first would fetch in default limit batches, the second is more explicit.

If LHS followed this, pagination could be solved that way. It's unclear how an API in LHS looks like that should just fetch the first page (i.e. current behavior).

Perhaps something like place.contracts.limit(:page) could work, which has no equivalent in ActiveRecord as far as I know.

10xSebastian commented 7 years ago

@nethad no.

Fetching data on accessing is declined and was discussed intensively in the web team, as it's the guarantor for the N+1 problem. You have to define, what data you want to fetch, beforehand (see ActiveRecords and LHS's Querybuilder). BTW: your example is totally missing the include statements, which are required to get linked data, in the first place.

Pagination is exactly a situation where you can't reference to ActiveRecord, as there is nothing like pagination when you are directly connected to a DB, in sense of a default limit of records, due to performance improvements. There are batches, but that's mainly for keeping the amount of used memory smaller and to have some custom exit strategies when iterating big data sets.

10xSebastian commented 7 years ago

Another thing worth mentioning here and worth considering, that a lot of APIs tempt to apply pagination on endpoints, just because you do paginations right? For a lot of endpoints, this just causes more implementation work on client side but instead, just dropping the pagination for certain endpoints should also be considered for some cases, but does not change the fact, that we need and interface in LHS do deal with paginated, included resources.

10xSebastian commented 7 years ago

As using includes is a prerequisite to run into this problem at first place, I would like to use this as a starting point, when it comes to an interface that tells LHS to load all linked resources. Instead of bloating the existing includes and for backwards compatibility reasons, I suggest to provide a includes_all interface, that works exactly like includes, is compatible with references but triggers LHS internal functionality that ensures to get all business object from a certain endpoint (see: here).

IS::Place
        .options(auth: { bearer: token })
        .includes_all(contracts: :product)
        .references(contracts: { auth: { bearer: token }, product: { auth: { bearer: token } } })
        .find_by(id: online_entry_identifier)
skaestle commented 7 years ago

imgres-2

simple and easy to remember

10xSebastian commented 7 years ago

BTW: If not already applied by you guys. A quickfix would be to apply a higher limit to the reference options:

IS::Place
  .options(auth: { bearer: token })
  .includes(contracts: :product)
  .references(
    contracts: { 
      auth: { bearer: token },
      params: { limit: 1000 },
      product: { 
        auth: { bearer: token },
        params: { limit: 1000 },
      } 
    }
  )
  .find_by(id: online_entry_identifier)