Reputation: 1392
I've been struggling to figure out how to avoid using so many SQL queries from this code. I'm taking in Collect objects via an API with Shopify, and then checking if there's a match already in my local database, and if not then creating a new local Collect object with parameters from my local matching Product and Collection. I wasn't sure how I would use includes or join because I'm doing a lookup and not just getting a value inside of the loop. Any pointers are welcomed.
def seed_collects!
shop = self.with_shopify!
c = (1..(ShopifyAPI::Collect.count.to_f/250.0).ceil).flat_map do |page|
ShopifyAPI::Collect.find(:all, :params => {:page => page.to_i, :limit => 150})
end
c.each do |collect|
next if Collect.find_by(shopify_collect_id: collect.id)
product = Product.find_by(shopify_product_id: collect.product_id.to_s)
collection = Collection.find_by(shopify_collection_id: collect.collection_id.to_s)
col = Collect.new(shopify_collect_id: collect.id,
position: collect.position,
product_id: product.id,
collection_id: collection.id)
col.save
puts col.errors.inspect if col.errors.any?
end
end
Schema
create_table "collections", force: :cascade do |t|
t.string "title"
t.string "shopify_collection_id"
t.string "collection_type"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.integer "shop_id"
end
create_table "collects", force: :cascade do |t|
t.string "position"
t.string "shopify_collect_id"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.integer "collection_id"
t.integer "product_id"
end
create_table "products", force: :cascade do |t|
t.string "title"
t.string "shopify_product_id"
t.string "product_type"
t.string "tags"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.integer "shop_id"
t.string "main_image_src"
end
Models
class Collect < ActiveRecord::Base
belongs_to :product
belongs_to :collection
delegate :shop, to: :product
delegate :shopify_product_id, to: :product
delegate :shopify_collection_id, to: :collection
end
class Collection < ActiveRecord::Base
# verify all items belong to shop
belongs_to :shop #verify
has_many :collects, dependent: :destroy
has_many :products, through: :collects
end
class Product < ActiveRecord::Base
belongs_to :shop
has_many :variants, ->{ order(:created_at) }, dependent: :destroy
has_many :price_tests, dependent: :destroy
has_many :metrics, ->{ order(:created_at) }, dependent: :destroy
has_many :collects, dependent: :destroy
has_many :collections, through: :collects
Upvotes: 1
Views: 97
Reputation: 1254
The better way to avoid N+1 in your case is to bulk-fetch collects, products, collections. Please try below code:
def seed_collects!
shop = self.with_shopify!
c = (1..(ShopifyAPI::Collect.count.to_f/250.0).ceil).flat_map do |page|
ShopifyAPI::Collect.find(:all, :params => {:page => page.to_i, :limit => 150})
end
shopify_collect_ids = c.map(&:id)
shopify_product_ids = c.map(&:product_id)
shopify_collection_ids = c.map(&:collection_id)
collects = Collect.where(shopify_collect_id: shopify_collect_ids).pluck(:shopify_collect_id, :id)
collect_ids = Hash[collects]
products = Product.where(shopify_product_id: shopify_product_ids).pluck(:shopify_product_id, :id)
product_ids = Hash[products]
collections = Collection.where(shopify_collection_id: shopify_collection_ids).pluck(:shopify_collection_ids, :id)
collection_ids = Hash[collections]
c.each do |collect|
next if collect_ids[collect.id]
col = Collect.new(shopify_collect_id: collect.id,
position: collect.position,
product_id: product_ids[collect.product_id.to_s],
collection_id: collection_ids[collect.collection_id.to_s])
col.save
puts col.errors.inspect if col.errors.any?
end
end
Upvotes: 2