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

Support for many_through_many associations #236

Open
dikond opened this issue Aug 18, 2017 · 10 comments
Open

Support for many_through_many associations #236

dikond opened this issue Aug 18, 2017 · 10 comments

Comments

@dikond
Copy link
Contributor

dikond commented Aug 18, 2017

Hi! This issue was extracted from the gitter discussion, here is the link.

The following example will tell better than I will.

class Eans < ROM::Relation[:sql]
  schema(:eans, infer: true) do
    associations do
      has_many :ean_stats
      has_many :contract_ean_stats, through: :ean_stats
      has_many :contracts,          through: :contract_ean_stats
      has_many :companies,          through: :contracts
    end
  end
end

eans.combine(:contract_ean_stats).limit(1).one
# => ROM::Struct::Ean
eans.combine(:contracts).limit(1).one
# => KeyError: :ean_id attribute doesn't exist in contract_ean_stats schema
@solnic solnic added this to the v2.0.0 milestone Aug 18, 2017
@solnic solnic added the bug label Aug 22, 2017
@dikond
Copy link
Contributor Author

dikond commented Aug 22, 2017

@solnic here is a self-contained snippet, just in case.

require 'rom'
require 'sqlite3'

rom = ROM.container(:sql, 'sqlite::memory') do |conf|
  conf.default.create_table(:eans) do
    primary_key :id
  end

  conf.default.create_table(:ean_stats) do
    primary_key :id
    column :ean_id, Integer
  end

  conf.default.create_table(:contract_ean_stats) do
    primary_key :id
    column :contract_id, Integer
    column :ean_stat_id, Integer
  end

  conf.default.create_table(:contracts) do
    primary_key :id
  end

  conf.relation(:eans) do
    schema(infer: true) do
      associations do
        has_many :ean_stats
        has_many :contract_ean_stats, through: :ean_stats
        has_many :contracts,          through: :contract_ean_stats
      end
    end
  end

  conf.relation(:ean_stats) do
    schema(infer: true) do
      associations do
        belongs_to :ean
        has_many :contract_ean_stats
      end
    end
  end

  conf.relation(:contract_ean_stats) do
    schema(infer: true) do
      associations do
        belongs_to :contract
        belongs_to :ean_stat
      end
    end
  end

  conf.relation(:contracts) do
    schema(infer: true) do
      associations do
        has_many :contract_ean_stats
        has_many :ean_stats, through: :contract_ean_stats
      end
    end
  end
end

rom.relations[:eans].combine(:contract_ean_stats).one
# => nil
rom.relations[:eans].combine(:contracts).one
# => KeyError: :ean_id attribute doesn't exist in contract_ean_stats schema

@solnic
Copy link
Member

solnic commented Aug 22, 2017

@dikond thanks, this is very helpful. I'll address this tomorrow.

@solnic
Copy link
Member

solnic commented Aug 23, 2017

@dikond is this a legacy schema? any reason why there are no real foreign keys in the database?

@solnic
Copy link
Member

solnic commented Aug 23, 2017

Please take a look at what I did in #238

@dikond
Copy link
Contributor Author

dikond commented Aug 23, 2017

Hey @solnic, the schema isn't legacy. When we designed it we haven't thought about this association. But later on we had to associate eans with companies and it turned out we can do it with many_through_many (in Sequel). Maybe it isn't the best way but it just works and we use this association not that often.

If your spec is passing then I believe my problem is solved (although I cannot try it myself until the 1-2 of September). However, I am not sure if it really closes this issue. BTW, I saw your tweet and now I think there is something horrible going on with these type of assocs, but I just do not imagine :D

@solnic
Copy link
Member

solnic commented Aug 24, 2017

@dikond the problem is that there are no FKs defined (at least in the example you attached), and rom-sql doesn't infer FKs from a schema when...there are no FKs :) The fact that a column is called "foo_id" isn't enough for rom-sql, it has to be a real FK.

@dikond
Copy link
Contributor Author

dikond commented Aug 24, 2017

Oh, that actually may be mine mistake, because all columns with '_id' suffix are FKs

@dikond
Copy link
Contributor Author

dikond commented Aug 24, 2017

have FKs*

@solnic
Copy link
Member

solnic commented Aug 24, 2017

@dikond OK, so I'll tweak this setup to use real FKs and look into this again :)

@solnic
Copy link
Member

solnic commented Sep 11, 2017

ok, I spent another 1.5h on this association scenario. Eventually I realized this requires a completely separate association type, which is a new feature. What we can quickly do now, is raise a meaningful error when foreign key is not found via join relation. We can add support for this type of associations in rom-sql 2.1.

@solnic solnic modified the milestones: v2.1.0, v2.0.0 Sep 11, 2017
@solnic solnic added feature and removed bug labels Sep 11, 2017
@solnic solnic removed this from the v2.1.0 milestone Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants