On Wednesday, November 7, 2018 at 11:04:00 AM UTC-8, Ben Alavi wrote: > > While upgrading to Sequel 5 we hit an issue using schemas + CTI: we need > to use qualified identifiers in the table names to include our schema > names, but that then breaks associations that reference the CTI models > because of the table alias defined on the subquery used by CTI. > > This gist illustrates the whole issue and solution: > https://gist.github.com/benalavi/26ff9eff12c004245bc3d350eedc5045 > > The CTI structure w/ qualified table name + associated model: > > class Artist < Sequel::Model(Sequel[:charts][:artists]) > one_to_many :sold_albums > end > > class Album < Sequel::Model(Sequel[:charts][:albums]) > plugin :class_table_inheritance, > key: :type, > table_map: { > SoldAlbum: Sequel[:charts][:sold_albums] > } > > many_to_one :artist > end > > class SoldAlbum < Album > end > > (Note the table_map is needed because implicit_table_name won't derive the > schema name from the class name, but I don't think there is any way around > that) > > The resulting SQL for artist.sold_albums looks like: > > SELECT * FROM (SELECT "charts"."albums"."id", "charts"."albums"."type", > "charts"."albums"."title", "charts"."albums"."artist_id" FROM > "charts"."albums" INNER JOIN "charts"."sold_albums" ON > ("charts"."sold_albums"."id" = "charts"."albums"."id")) AS > "#<Sequel::SQL::QualifiedIdentifier:0x007fe24163b980>" WHERE > ("charts"."albums"."artist_id" = 1) > > After some digging based on the note here: > https://sequel.jeremyevans.net/rdoc-plugins/classes/Sequel/Plugins/ClassTableInheritance.html#module-Sequel::Plugins::ClassTableInheritance-label-Subclass+loading > > we realized that we could explicitly pass an alias to the CTI plugin so > that the same alias would be used in both the CTI dataset and the > association, i.e.: > > plugin :class_table_inheritance, > key: :type, > table_map: { > SoldAlbum: Sequel[:charts][:sold_albums] > }, > alias: :charts__sold_albums > > ...the alias can be *anything* though, which I believe is because when the > association determines how to add conditions to the dataset it actually > qualifies the QualifiedIdentifer: > https://github.com/jeremyevans/sequel/blob/master/lib/sequel/model/associations.rb#L464 > > ...whereas when the CTI dataset is built it just passes the > QualifiedIdentifier (which becomes the default :alias option to the CTI > class) as the :alias option to the dataset here: > https://github.com/jeremyevans/sequel/blob/master/lib/sequel/plugins/class_table_inheritance.rb#L292 > > So if I understand all of that correctly then rather than needing to > explicitly define the :alias any time you use CTI w/ a dataset based on a > QualifiedIdentifier, the default CTI alias can just be transformed into > something that will agree for both uses above, which can be done by > coercing it into a string (or a symbol which is what the specs expect). > > So I think changing: > https://github.com/jeremyevans/sequel/blob/master/lib/sequel/plugins/class_table_inheritance.rb#L210 > > to: > > @cti_alias = opts[:alias] || @dataset.first_source.to_s.to_sym > > ...will remove the need to explicitly define the alias. I used to_s.to_sym > because QualifiedIdentifier doesn't respond to to_sym, but if defined there > it could be @dataset.first_source.to_sym. > > The proposed change passed all the existing specs. I took a stab at > writing a spec for it but got caught up trying to figure out how to set up > the mocks properly using QualifiedIdentifers instead of symbols so wanted > to check if this even made sense before I continued. >
Thanks for posting about this. I agree that it would be nice if CTI models could automatically handle qualified tables. I don't think .to_s.to_sym makes sense. If first_source is a qualified identifier, you'll end up with something like: :"#<Sequel::SQL::QualifiedIdentifier:0x000001d053653590>" Maybe something like: opts[:alias] || case source = @dataset.first_source when SQL::QualifiedIdentifier @dataset.unqualified_column_for(source) else source end I think something like that will make sense. I don't think we want to force the use of symbols. If the specs are requiring symbols even if the dataset source is a String or SQL::Identifier, they should probably be changed. The alias value should be either a Symbol, String, or SQL::Identifier. Thanks, Jeremy -- You received this message because you are subscribed to the Google Groups "sequel-talk" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at https://groups.google.com/group/sequel-talk. For more options, visit https://groups.google.com/d/optout.
