Should we stop passing activerecord objects between components and use poro's or ids instead?

System Design
21

Message originally sent by slack user U783MOJYF8Z

We have a query layer implementation that analyzes the query upfront and generates a pseudo-optimal set of database queries (one per component) but it’s pretty unwieldy right now

22

Message originally sent by slack user U783MOJYF8Z

TBD, currently nobody at Shopify is working on this.

23

Message originally sent by slack user U783UION177

So we’ve been using this gem: https://github.com/exAspArk/batch-loader

24

Message originally sent by slack user U783UION177

Here’s an example from the readme:

def load_user(post)
  BatchLoader.for(post.user_id).batch do |user_ids, loader|
    User.where(id: user_ids).each { |user| loader.call(user.id, user) }
  end
end
25

Message originally sent by slack user U783MOJYF8Z

yeah, I think that’s approximately the same

26

Message originally sent by slack user U783UION177

So in your example, we might do something like:

class TireQuery
  def by_car(car_id)
    BatchLoader.for(car_id).batch do |car_ids, loader|
      Tire.where(car_id: car_ids).each { |tire| loader.call(tire.id, tire) }
    end
  end
end
27

Message originally sent by slack user U783MOJYF8Z

:+1:

28

Message originally sent by slack user U783UION177

so the caller of TireQuery doesn’t need to know about batch loading

29

Message originally sent by slack user U783MOJYF8Z

thank you for sharing

30

Message originally sent by slack user U783MOJYF8Z

It’s interesting, I think our batchloader implementation is too caught up in graphQL

31

Message originally sent by slack user U783UION177

So I lifted this idea from Dan Schafer’s talk here:
https://www.youtube.com/watch?v=etax3aEe2dA

32

Message originally sent by slack user U783UION177

it’s an interesting idea to think of batch loading as a solution to n+1s independently of GQL

33

Message originally sent by slack user U783MOJYF8Z

How do you handle pagination in that case? Is there an abstraction for that or do you need to manually implement on every query that can be paginated?

34

Message originally sent by slack user U783UION177

Currently it’s implemented on every query individually. But it’s on the list.

35

Message originally sent by slack user U783UION177

Not every query, but those that need it I should say.

36

Message originally sent by slack user U783UION177

So in your example, CarsQuery would need it, TiresQuery would not.

37

Message originally sent by slack user U783MOJYF8Z

assuming the number of tires per car is bounded :wink:

38

Message originally sent by slack user U783UION177

true

39

Message originally sent by slack user U783MOJYF8Z

I’ll think about generalizing our batch loaders so they can be separated from the graphql layer. It’s an interesting approach.

40

Message originally sent by slack user U783UION177

Let me know how it goes! Still early days for us with this architecture, so I’ll report back any trouble we run into.