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

Nested combine fails when using views #568

Open
mrship opened this issue Dec 16, 2019 · 9 comments
Open

Nested combine fails when using views #568

mrship opened this issue Dec 16, 2019 · 9 comments

Comments

@mrship
Copy link

mrship commented Dec 16, 2019

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 mrship added the bug label Dec 16, 2019
@solnic solnic added this to the 5.1.3 milestone Dec 16, 2019
@solnic
Copy link
Member

solnic commented Dec 16, 2019

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

@solnic
Copy link
Member

solnic commented Dec 22, 2019

@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?

@mrship
Copy link
Author

mrship commented Dec 23, 2019

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)

@solnic
Copy link
Member

solnic commented Dec 23, 2019

@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.

@mrship
Copy link
Author

mrship commented Jan 2, 2020

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>]>]>]

@solnic
Copy link
Member

solnic commented Jan 3, 2020

@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.

@mrship
Copy link
Author

mrship commented Apr 29, 2020

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

@solnic
Copy link
Member

solnic commented Apr 29, 2020

@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 :(

@mrship
Copy link
Author

mrship commented Apr 30, 2020

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

@solnic solnic modified the milestones: 5.1.3, 6.0.0 Jun 24, 2020
@solnic solnic moved this to Backlog in rom-rb 6.0 May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

2 participants