Refactor `ColumnDefinition` to contain `options` hash #27890

kamipo · 2017-02-03T00:42:19Z

Column options are passed as an hash args then used as options hash in
add_column_options!. Converting args to attributes is inconvinient for
using options as an hash.

Column options are passed as an hash args then used as `options` hash in `add_column_options!`. Converting args to attributes is inconvinient for using options as an hash.

jeremy

I like the improvement to creating column definitions. Much simpler.

But the tradeoff is that code is accessing column.options[…] directly and breaking compat with other adapters.

Perhaps there's a path to using options hash and keyword args to construct column definitions, but expose an opaque attr-style API for the option values.

jeremy · 2017-02-07T03:43:45Z

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb

-        primary_key || type.to_sym == :primary_key
+    ColumnDefinition = Struct.new(:name, :type, :options, :sql_type) do # :nodoc:
+      def [](key)
+        options[key]


This poses a compatibility challenge for third party database adapters. Could we offer an gentler upgrade path? Possibly by deprecating the old methods:

class ColumnDefinition attr_reader :name, :type, :sql_type, :options def initialize(*args, options: nil) if options initialize_with_options(*args, options) else initialize_with_struct_args(*args) end end def initialize_with_options(name, type, sql_type, options) @name, @type, @options, @sql_type = name, type, options, sql_type end def initialize_with_struct_args(name, type, limit, etc…) ActiveSupport::Deprecation.warn … @name, @type, @sql_type = name, type, sql_type @options = { limit: limit, precision: precision, … } end %i[ limit precision … ].each do |option_name| define_method(option_name) do ActiveSupport::Deprecation.warn … options[option_name] end define_method("#{option_name}=") do |value| ActiveSupport::Deprecation.warn … options[option_name] = value end end end

After reading the rest of the PR, perhaps we could provide these reader/writer methods without deprecation.

jeremy · 2017-02-07T03:45:32Z

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb

@@ -305,7 +305,7 @@ def column(name, type, options = {})
        type = type.to_sym if type
        options = options.dup

-        if @columns_hash[name] && @columns_hash[name].primary_key?
+        if @columns_hash[name] && @columns_hash[name][:primary_key]


This usage feels like a regression. It's nice to call a clear predicate method to ask about the column's attributes rather than digging in to the column options to fetch the data.

jeremy · 2017-02-07T03:46:04Z

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb

@@ -360,28 +360,16 @@ def references(*args, **options)
      end
      alias :belongs_to :references

-      def new_column_definition(name, type, options) # :nodoc:
+      def new_column_definition(name, type, **options) # :nodoc:


+keyword args for options.

jeremy · 2017-02-07T03:49:51Z

activerecord/lib/active_record/connection_adapters/postgresql/schema_definitions.rb

-
-          def create_column_definition(name, type)
-            PostgreSQL::ColumnDefinition.new name, type
-          end


Nice simplification. Pays off.

jeremy · 2017-02-07T03:50:42Z

activerecord/lib/active_record/connection_adapters/postgresql/schema_creation.rb

+      class SchemaCreation < AbstractAdapter::SchemaCreation # :nodoc:
+        private
+          def visit_ColumnDefinition(o)
+            o.sql_type = type_to_sql(o.type, o[:limit], o[:precision], o[:scale], o[:array])


Seeing this repeated. Good candidate to switch to keyword args also.

jeremy · 2017-02-07T03:51:15Z

activerecord/lib/active_record/connection_adapters/sqlite3/schema_creation.rb

-            options[:null] = false if o.primary_key
-            options
-          end
-


jeremy · 2017-02-07T03:53:34Z

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

-            self.precision = options[:precision] if options.include?(:precision)
-            self.scale = options[:scale] if options.include?(:scale)
-            self.collation = options[:collation] if options.include?(:collation)
+            self.options = options


This appears to overwrite all the existing column options instead of just the ones provided by change_column.

ah... good catch. I'll fix it.

jeremy · 2017-02-07T03:54:25Z

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -418,21 +418,15 @@ def change_column_null(table_name, column_name, null, default = nil) #:nodoc:
          exec_query("UPDATE #{quote_table_name(table_name)} SET #{quote_column_name(column_name)}=#{quote(default)} WHERE #{quote_column_name(column_name)} IS NULL")
        end
        alter_table(table_name) do |definition|
-          definition[column_name].null = null
+          definition[column_name].options[:null] = null


These cases of modifying a data collection directly feel like a step back from calling specific writer methods.

It's nice to call a clear predicate method to ask about the column's attributes rather than digging in to the column options to fetch the data.

kamipo · 2017-02-08T05:43:34Z

I addressed to switch to keyword args for type_to_sql and define reader/writer methods for options.

Originally ColumnDefiniton is a short-lived object and is only used in create_table, change_table, change_column, change_column_null, and change_column_default.

So it is enough to address only the changes of type_to_sql for third party adapters.

I created a patch for oracle-enhanced and sqlserver-adapter.

oracle-enhanced:
https://gist.github.com/kamipo/d718a823f61d13db736975fd2d7b3a5b#file-oracle-enhanced-patch

sqlserver-adapter:
https://gist.github.com/kamipo/d718a823f61d13db736975fd2d7b3a5b#file-sqlserver-adapter-patch

How about it?

jeremy · 2017-02-09T07:33:17Z

Merged @ ae39b1a

yahonda · 2017-02-09T12:01:08Z

@kamipo Thanks for making a patch for Oracle enhanced adapter. Would you mind pushing the branch to github. of course pull request is welcomed.

kamipo · 2017-02-09T15:47:50Z

Yeah, I'll create the branch and pull request.

Refer rails/rails#27890

davidalejandroaguilar · 2021-11-17T17:02:26Z

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb

-        column.as          = options[:as]
-        column
+        options[:primary_key] ||= type == :primary_key
+        options[:null] = false if options[:primary_key]


Hey @kamipo @jeremy , I'm wondering why this got added here? Just noticed this got moved from the sqlite3 adapter into abstract. Not sure why a column could not be null if you specify the primary_key option.

Sorry if you get a duplicate notification, for some reason my previous comment got deleted.

Refactor ColumnDefinition to contain options hash

de3cede

Column options are passed as an hash args then used as `options` hash in `add_column_options!`. Converting args to attributes is inconvinient for using options as an hash.

maclover7 added the activerecord label Feb 3, 2017

jeremy reviewed Feb 7, 2017

View reviewed changes

rafaelfranca assigned jeremy Feb 7, 2017

maclover7 added the needs work label Feb 7, 2017

kamipo added 3 commits February 8, 2017 10:29

Restore primary_key? predicate method

77967eb

It's nice to call a clear predicate method to ask about the column's attributes rather than digging in to the column options to fetch the data.

Switch to keyword args for type_to_sql

aaf2789

Define reader/writer methods for options

f469a89

jeremy closed this Feb 9, 2017

kamipo deleted the refactor_column_definition branch February 9, 2017 07:36

kamipo added a commit to kamipo/oracle-enhanced that referenced this pull request Feb 9, 2017

Switch to keyword args for type_to_sql

1538e7d

Refer rails/rails#27890

kamipo mentioned this pull request Feb 9, 2017

Switch to keyword args for type_to_sql rsim/oracle-enhanced#1167

Merged

davidalejandroaguilar reviewed Nov 17, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `ColumnDefinition` to contain `options` hash #27890

Refactor `ColumnDefinition` to contain `options` hash #27890

kamipo commented Feb 3, 2017

jeremy left a comment

jeremy Feb 7, 2017

jeremy Feb 7, 2017

jeremy Feb 7, 2017

jeremy Feb 7, 2017

jeremy Feb 7, 2017

jeremy Feb 7, 2017

jeremy Feb 7, 2017

jeremy Feb 7, 2017

kamipo Feb 7, 2017

jeremy Feb 7, 2017

kamipo commented Feb 8, 2017

jeremy commented Feb 9, 2017

yahonda commented Feb 9, 2017

kamipo commented Feb 9, 2017

davidalejandroaguilar Nov 17, 2021

Refactor ColumnDefinition to contain options hash #27890

Refactor ColumnDefinition to contain options hash #27890

Conversation

kamipo commented Feb 3, 2017

jeremy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamipo commented Feb 8, 2017

jeremy commented Feb 9, 2017

yahonda commented Feb 9, 2017

kamipo commented Feb 9, 2017

Choose a reason for hiding this comment

Refactor `ColumnDefinition` to contain `options` hash #27890

Refactor `ColumnDefinition` to contain `options` hash #27890