Hi Jeremy,
Sorry, I didn't get a chance to test this today. I will put it through
a run this weekend.
Sunny
On Nov 6, 9:29 pm, Jeremy Evans <[EMAIL PROTECTED]> wrote:
> On Nov 6, 3:31 pm, Sunny Hirai <[EMAIL PROTECTED]> wrote:
>
> > On the bright side, you've got a dedicated #identifier tester. :) On
> > the less bright side, found a few more bugs related to it.
>
> > Calling Database#schema with or without a table name results in an
> > error.
>
> This is going to be an ugly issue to deal with. First, the patch:
>
> diff --git a/lib/sequel_core/schema/sql.rb b/lib/sequel_core/schema/
> sql.rb
> index ed5d8ab..dbc9ad1 100644
> --- a/lib/sequel_core/schema/sql.rb
> +++ b/lib/sequel_core/schema/sql.rb
> @@ -202,7 +202,7 @@ module Sequel
> "ALTER TABLE #{quote_schema_table(name)} RENAME TO
> #{quote_schema_table(new_name)}"
> end
>
> - # Parse the schema from the database using the SQL standard
> INFORMATION_SCHEMA.
> + # Parse the schema from the database.
> # If the table_name is not given, returns the schema for all
> tables as a hash.
> # If the table_name is given, returns the schema for a single
> table as an
> # array with all members being arrays of length 2. Available
> options are:
> @@ -212,8 +212,11 @@ module Sequel
> # unless you are sure that schema has not been called before
> with a
> # table_name, otherwise you may only getting the schemas for
> tables
> # that have been requested explicitly.
> + # * :schema - An explicit schema to use. It may also be
> implicitly provided
> + # via the table name.
> def schema(table_name = nil, opts={})
> - table_name = table_name.to_sym if table_name
> + sch, table_name = schema_and_table(table_name) if table_name
> + opts = opts.merge(:schema=>sch) if sch && !opts.include?
> (:schema)
> if opts[:reload] && @schemas
> if table_name
> [EMAIL PROTECTED](table_name)
> @@ -241,7 +244,7 @@ module Sequel
> if respond_to?(:schema_parse_tables, true)
> [EMAIL PROTECTED] = schema_parse_tables(opts)
> elsif respond_to?(:schema_parse_table, true) and respond_to?
> (:tables, true)
> - tables.each{|t| schema(t, opts)}
> + tables.each{|t| schema(Sequel::SQL::Identifier.new(t),
> opts)}
> [EMAIL PROTECTED]
> else
> raise Error, 'schema parsing is not implemented on this
> database'
>
> That should fix both using a table name and not using a table name
> cases. Unfortunately, it brings a design deficiency to light.
> @schemas was designed to be keyed on table name symbol, which
> obviously leads to issues if you have tables with the same name in
> multiples schema. This code doesn't ensure that a symbol is always
> used as the key, which means Database#drop_table won't remove the
> entry from the @schemas when the table is dropped.
>
> I'm not sure how best to handle this situation. The correct way to
> handle it is probably keying on [table, schema], but that makes usage
> more difficult. I'll probably do that, and if someone call schema
> with the table name, look in @schemas for [table, default_schema].
> This will break backwards compatibility, but that's life. I probably
> should have had schema return nil if called without a table name (and
> automatically reload), so you could just use it for caching, and you
> had to call it with a table name to get schema information returned.
> I may make that change in 2.8. Another alternative is making @schemas
> a Hash with a default proc that does the conversion for you. I'll
> have to think about it for a while before I make my decision.
>
> Please test the above patch and let me know if it works.
>
> Thanks,
> Jeremy
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"sequel-talk" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/sequel-talk?hl=en
-~----------~----~----~----~------~----~------~--~---
