Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include records on API Order / Product queries #3002

Merged
merged 1 commit into from
Jan 11, 2019

Conversation

fastjames
Copy link
Contributor

When querying for Orders or Products via the API, the significant number
of related records needed to render the JSON for each Order / Product
can lead to a lot of extra database traffic. By using includes we can
reduce the query traffic (and thus response time) significantly.

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I just left some code formatting opinions.

On the performance side I've tried the /api/orders endpoint locally (here's a report) and I can't see huge differences. I've done this test with a sandbox and ~250 orders so I imagine the difference is more evident with a lot of records, right?

:line_items
]
orders_base = query.result.includes(order_includes)
@orders = paginate(orders_base)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about reformatting code as:

orders_includes = [
  :user,
  :payments,
  :adjustments,
  :line_items
]

@orders = paginate(
  Spree::Order
    .includes(orders_includes)
    .ransack(params[:q])
    .result
)
respond_with(@orders)

?

I think this reduces the complexity since:

  • we have less variables around;
  • I think it's better to rename order_includes into orders_includes since it refers to a query to retrieve a collection of orders;
  • having the includes called before ransack and result looks more natural to me, even if I don't have strong opinion here.

With yield_self of Ruby 2.5 we could have simply done:

@orders = Spree::Order
  .includes(orders_includes)
  .ransack(params[:q])
  .result
  .yield_self { |orders| paginate(orders) }

respond_with(@orders)

which is much cleaner IMO but we still need to support older Ruby versions. 😢

Also, what do you think about moving order_includes as a private method in the controller?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on maintaining some level of backcompat. I have made this change.

:product_properties,
{ classifications: :taxon }
]
@products = query.result.includes(product_includes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here:

# Let's evaluate moving this into a controller's private method
products_includes = [
  :variants,
  :option_types,
  :product_properties,
  { classifications: :taxon }
]

@products = product_scope
  .includes(products_includes)
  .ransack(params[:q])
  .result

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable, done.

When querying for Orders or Products via the API, the significant number
of related records needed to render the JSON for each Order / Product
can lead to a lot of extra database traffic. By using `includes` we can
reduce the query traffic (and thus response time) significantly.
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kennyadsl kennyadsl merged commit fbc5266 into solidusio:master Jan 11, 2019
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.

4 participants