rom icon indicating copy to clipboard operation
rom copied to clipboard

Nested combine fails when using views

Open mrship opened this issue 4 years ago • 9 comments

Describe the bug

When using a nested combine and views we get an error thrown in the transproc gem.

/Users/shipmana/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/transproc-1.1.0/lib/transproc/array/combine.rb:51:in `block in group_candidates_by_keys': no implicit conversion of Symbol into Integer (TypeError)

Without using the nested combine, the data structure is retrieved correctly (although obviously without the child data)

As far as I can tell, the data is pulled from the data source correctly. We end up with a hash of data that fails the group_by call in transproc.

To Reproduce

#!/usr/bin/env ruby
require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'rom'
  gem 'rom-sql'
  gem 'rom-repository'
  gem 'dry-types'
  gem 'sqlite3'
end

module Types
  include Dry::Types(default: :nominal)
end

rom = ROM.container(:sql, 'sqlite::memory') do |c|
  c.gateways[:default].create_table :product_groups do
    primary_key :id
    String :name
  end

  c.gateways[:default].create_table :products do
    primary_key :id
    String :reference
    String :name
  end

  c.gateways[:default].create_table :grouped_products do
    primary_key :id
    Integer :product_group_id
    Integer :product_id
  end

  c.gateways[:default].create_table :allocations do
    primary_key :id
    Integer :product_config_id
    String :name
  end

  c.gateways[:default].create_table :product_configs do
    primary_key :id
    Integer :product_group_id
  end

  c.gateways[:default].use_logger(Logger.new($stdout))

  c.relation(:product_groups) do
    schema(:product_groups, infer: false) do
      attribute :id, Types::Integer, primary_key: true
      attribute :name, Types::String

      associations do
        has_many :products, view: :for_product_groups, override: true, combine_keys: { id: :product_group_id }
      end
    end

    def for_allocations(_assoc, loaded)
      product_groups
        .join(:product_configs, { product_group_id: :id })
        .where(product_configs[:id] => loaded.pluck(:product_config_id).uniq)
        .select_append(product_configs[:id].as(:product_config_id))
    end
  end

  c.relation(:grouped_products) do
    schema(:grouped_products, infer: false) do
      attribute :id, Types::Integer.meta(primary_key: true)
      attribute :product_group_id, Types::Integer
      attribute :product_id, Types::String
    end
  end

  c.relation(:products) do
    schema(:products, infer: false) do
      attribute :id, Types::Integer.meta(primary_key: true)
      attribute :reference, Types::String
      attribute :name, Types::String
    end

    def for_product_groups(_assoc, loaded)
      products
        .join(:grouped_products, { grouped_products[:product_id] => products[:reference] })
        .where(grouped_products[:product_group_id] => loaded.pluck(:id).uniq)
        .select_append(grouped_products[:product_group_id])
    end
  end

  c.relation(:allocations) do
    schema(:allocations, infer: false) do
      attribute :id, Types::Integer.meta(primary_key: true)
      attribute :product_config_id, Types::Integer
      attribute :name, Types::String

      associations do
        has_one :product_group, view: :for_allocations, override: true,
          combine_keys: { product_config_id: :product_config_id }
      end
    end
  end

  c.relation(:product_configs) do
    schema(:product_configs, infer: false) do
      attribute :id, Types::Integer.meta(primary_key: true)
      attribute :product_group_id, Types::Integer
    end
  end
end

rom.relations[:products].insert id: 1001, reference: "ABC", name: "TV"
rom.relations[:product_groups].insert id: 2002, name: "Broadcasters"
rom.relations[:grouped_products].insert id: 3003, product_group_id: 2002, product_id: "ABC"
rom.relations[:product_configs].insert id: 4004, product_group_id: 2002
rom.relations[:allocations].insert id: 5005, product_config_id: 4004

class AllocationRepo < ROM::Repository[:allocations]
  def boom
    allocations.combine(product_group: :products).to_a
  end

  def ok
    allocations.combine(:product_group).to_a
  end
end

repo = AllocationRepo.new(rom)

# puts repo.ok.inspect
puts repo.boom.inspect # => transproc-1.1.0/lib/transproc/array/combine.rb:51:in `block in group_candidates_by_keys': no implicit conversion of Symbol into Integer (TypeError)

Expected behavior

No error thrown and a correctly combined dataset.

Your environment

  • Affects my production application: NO
  • Ruby version: ruby 2.6.2p47 (2019-03-13 revision 67232) [x86_64-darwin18]
  • OS: MacOS

mrship avatar Dec 16 '19 16:12 mrship

Thanks for the report. I'll look into this.

solnic avatar Dec 16 '19 16:12 solnic

@mrship that's an interesting bug 😅 It looks like the same mapper is being applied twice, I need to dig deeper. Does the same problem happen when you have identical setup except that standard PK/FK are used so that there's no need for overidden views?

solnic avatar Dec 22 '19 17:12 solnic

If I use PK/FK for those relationships where I can - leaving one view for a many-to-many join then it does still throw the same error:

The "simpler" setup:

#!/usr/bin/env ruby
require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'rom'
  gem 'rom-sql'
  gem 'rom-repository'
  gem 'dry-types'
  gem 'sqlite3'
end

module Types
  include Dry::Types(default: :nominal)
end

rom = ROM.container(:sql, 'sqlite::memory') do |c|
  c.gateways[:default].create_table :product_groups do
    primary_key :id
    String :name
  end

  c.gateways[:default].create_table :products do
    String :id
    String :name
  end

  c.gateways[:default].create_table :grouped_products do
    primary_key :id
    Integer :product_group_id
    Integer :product_id
  end

  c.gateways[:default].create_table :allocations do
    primary_key :id
    Integer :product_config_id
    String :name
  end

  c.gateways[:default].create_table :product_configs do
    primary_key :id
    Integer :product_group_id
  end

  c.gateways[:default].use_logger(Logger.new($stdout))

  c.relation(:product_groups) do
    schema(:product_groups, infer: false) do
      attribute :id, Types::Integer, primary_key: true
      attribute :name, Types::String

      associations do
        has_many :grouped_products
        has_many :products, through: :grouped_products
      end
    end

    def for_allocations(_assoc, loaded)
      product_groups
        .join(:product_configs, { product_group_id: :id })
        .where(product_configs[:id] => loaded.pluck(:product_config_id).uniq)
        .select_append(product_configs[:id].as(:product_config_id))
    end
  end

  c.relation(:grouped_products) do
    schema(:grouped_products, infer: false) do
      attribute :id, Types::Integer.meta(primary_key: true)
      attribute :product_group_id, Types::Integer
      attribute :product_id, Types::String

      associations do
        belongs_to :products
      end
    end
  end

  c.relation(:products) do
    schema(:products, infer: false) do
      attribute :id, Types::String.meta(primary_key: true)
      attribute :name, Types::String
    end
  end

  c.relation(:allocations) do
    schema(:allocations, infer: false) do
      attribute :id, Types::Integer.meta(primary_key: true)
      attribute :product_config_id, Types::Integer
      attribute :name, Types::String

      associations do
        belongs_to :product_config
        has_one :product_group, view: :for_allocations, override: true,
          combine_keys: { product_config_id: :product_config_id }
      end
    end
  end

  c.relation(:product_configs) do
    schema(:product_configs, infer: false) do
      attribute :id, Types::Integer.meta(primary_key: true)
      attribute :product_group_id, Types::Integer

      associations do
        belongs_to :product_groups
      end
    end
  end
end

rom.relations[:products].insert id: "ABC", name: "TV"
rom.relations[:product_groups].insert id: 2002, name: "Broadcasters"
rom.relations[:grouped_products].insert id: 3003, product_group_id: 2002, product_id: "ABC"
rom.relations[:product_configs].insert id: 4004, product_group_id: 2002
rom.relations[:allocations].insert id: 5005, product_config_id: 4004

class AllocationRepo < ROM::Repository[:allocations]
  def boom
    allocations.combine(product_group: :products).to_a
  end

  def ok
    allocations.combine(:product_group).to_a
  end
end

repo = AllocationRepo.new(rom)

# puts repo.ok.inspect
puts repo.boom.inspect # => transproc-1.1.0/lib/transproc/array/combine.rb:51:in `block in group_candidates_by_keys': no implicit conversion of Symbol into Integer (TypeError)

mrship avatar Dec 23 '19 12:12 mrship

@mrship If I use PK/FK for those relationships where I can - leaving one view for a many-to-many join then it does still throw the same error

Is it possible to set it up in a way that this view is not needed? I just want to make sure that this only happens with a custom overridden view.

solnic avatar Dec 23 '19 14:12 solnic

Yes, it works (in a test, per below) using another join table and not using a view. However, that's not how my real world application works unfortunately.

#!/usr/bin/env ruby
require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'rom'
  gem 'rom-sql'
  gem 'rom-repository'
  gem 'dry-types'
  gem 'sqlite3'
end

module Types
  include Dry::Types(default: :nominal)
end

rom = ROM.container(:sql, 'sqlite::memory') do |c|
  c.gateways[:default].create_table :product_groups do
    primary_key :id
    String :name
  end

  c.gateways[:default].create_table :products do
    String :id
    String :name
  end

  c.gateways[:default].create_table :grouped_products do
    primary_key :id
    Integer :product_group_id
    Integer :product_id
  end

  c.gateways[:default].create_table :allocation_product_groups do
    primary_key :id
    Integer :product_group_id
    Integer :allocation_id
  end

  c.gateways[:default].create_table :allocations do
    primary_key :id
    Integer :product_config_id
    String :name
  end

  c.gateways[:default].create_table :product_configs do
    primary_key :id
    Integer :product_group_id
  end

  c.gateways[:default].use_logger(Logger.new($stdout))

  c.relation(:product_groups) do
    schema(:product_groups, infer: false) do
      attribute :id, Types::Integer, primary_key: true
      attribute :name, Types::String

      associations do
        has_many :grouped_products
        has_many :products, through: :grouped_products
      end
    end

    def for_allocations(_assoc, loaded)
      product_groups
        .join(:product_configs, { product_group_id: :id })
        .where(product_configs[:id] => loaded.pluck(:product_config_id).uniq)
        .select_append(product_configs[:id].as(:product_config_id))
    end
  end

  c.relation(:grouped_products) do
    schema(:grouped_products, infer: false) do
      attribute :id, Types::Integer.meta(primary_key: true)
      attribute :product_group_id, Types::Integer
      attribute :product_id, Types::String

      associations do
        belongs_to :products
      end
    end
  end

  c.relation(:products) do
    schema(:products, infer: false) do
      attribute :id, Types::String.meta(primary_key: true)
      attribute :name, Types::String
    end
  end

  c.relation(:allocations) do
    schema(:allocations, infer: false) do
      attribute :id, Types::Integer.meta(primary_key: true)
      attribute :product_config_id, Types::Integer
      attribute :name, Types::String

      associations do
        belongs_to :product_config
        has_many :allocation_product_groups
        has_many :product_groups, through: :allocation_product_groups
      end
    end
  end

  c.relation(:product_configs) do
    schema(:product_configs, infer: false) do
      attribute :id, Types::Integer.meta(primary_key: true)
      attribute :product_group_id, Types::Integer

      associations do
        belongs_to :product_groups
      end
    end
  end

  c.relation(:allocation_product_groups) do
    schema(:allocation_product_groups, infer: false) do
      attribute :allocation_id, Types::Integer
      attribute :product_group_id, Types::Integer

      associations do
        belongs_to :product_groups
      end
    end
  end
end

rom.relations[:products].insert id: "ABC", name: "TV"
rom.relations[:product_groups].insert id: 2002, name: "Broadcasters"
rom.relations[:grouped_products].insert id: 3003, product_group_id: 2002, product_id: "ABC"
rom.relations[:product_configs].insert id: 4004, product_group_id: 2002
rom.relations[:allocations].insert id: 5005, product_config_id: 4004
rom.relations[:allocation_product_groups].insert id: 3003, product_group_id: 2002, allocation_id: 5005

class AllocationRepo < ROM::Repository[:allocations]
  def boom
    allocations.combine(product_groups: :products).to_a
  end
end

repo = AllocationRepo.new(rom)

puts repo.boom.inspect 
# => [#<ROM::Struct::Allocation id=5005 product_config_id=4004 name=nil product_groups=[#<ROM::Struct::ProductGroup id=2002 name="Broadcasters" allocation_id=5005 products=[#<ROM::Struct::Product id="ABC" name="TV" product_group_id=2002>]>]>]

mrship avatar Jan 02 '20 15:01 mrship

@mrship thank you for checking it, this is good news because I was worried the association type is broken regardless if you're custom views or not. OK I will keep digging into your scenario then and fix it in the next release.

solnic avatar Jan 03 '20 10:01 solnic

@solnic just checking in on this one as I've been bitten by it again today in another project.

mrship avatar Apr 29 '20 09:04 mrship

@mrship no progress yet, sorry. It's hard to find time to work on OSS these days. I'll try to look into it later this week but can't promise anything :(

solnic avatar Apr 29 '20 11:04 solnic

@solnic FYI we worked around this by using a database view instead of combining multiple tables in Ruby, so no rush to fix! Thanks.

mrship avatar Apr 30 '20 09:04 mrship