Allow to disable attribute discovery in ActiveRecord #27903

kirs · 2017-02-04T13:20:26Z

This works as an opposite to ignore_columns. Sometimes we want to explicitly declare used columns with Attributes API in the model and use only them, ignoring the rest of columns:

class Cat < ActiveRecord::Base
  self.attribute_auto_discovery = false

  attribute :name, :string
  attribute :engines_count, :integer
  attribute :created_at, :datetime
  attribute :updated_at, :datetime
end

@wvanbergen @rafaelfranca @matthewd

rails-bot · 2017-02-04T13:20:28Z

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

simi · 2017-02-04T13:37:34Z

activerecord/lib/active_record/model_schema.rb

+ #
+ # Despite on the fact that `projects` table has more columns that `name` and `members_count`,
+ # ActiveRecord will only use explicitly declared attributes
+ def attribute_auto_discovery


Can't be this moved to included block and defined via mattr_accessor or class_attribute with default value as another options?

matthewd · 2017-02-04T13:45:04Z

activerecord/lib/active_record/model_schema.rb

@@ -448,6 +471,11 @@ def load_schema

 def load_schema!
 @columns_hash = connection.schema_cache.columns_hash(table_name).except(*ignored_columns)
+
+ unless attribute_auto_discovery
+ @columns_hash.reject! { |k| !attributes_to_define_after_schema_loads.key?(k) }


Why is this filtering necessary? Why can't we just skip the schema load entirely? 😕

Agreed. This should definitely just be return unless attribute_auto_discovery

This filtering in necessary because otherwise @columns_hash will include all table columns. It's loaded in the line above.

We already do this filter for ignored columns (.except(*ignored_columns))

sgrif · 2017-02-04T14:27:17Z

I'd prefer something like define_attributes_from_schema for the option name.

kaspth · 2017-02-05T16:10:28Z

Generally if the default is true for a new configuration is true, we invert the name: self.skip_defining_attributes_from_schema = true.

See #26976 (comment) for more.

wvanbergen · 2017-02-05T18:21:56Z

id is automatically defined?
Should you define foo_id, or should belongs_to :foo take care of that?

sgrif · 2017-02-07T00:42:51Z

id is automatically defined?

id should continue to refer to the primary key. If self.primary_key is set to something other than id, it will need to be defined.

Should you define foo_id, or should belongs_to :foo take care of that?

foo_id should need to be manually defined. The only reason id is treated specially is because the method id exists without the column

sgrif · 2017-02-07T00:44:03Z

Note: I want to be conservative to start. We can always change it to have foo_id defined automatically later without breaking backwards compat.

kirs · 2017-02-14T19:03:36Z

Thanks everyone for the feedback!
I've updated the PR and it's ready for another round of 👀

sgrif · 2017-02-14T19:34:00Z

activerecord/lib/active_record/model_schema.rb

@@ -149,6 +176,9 @@ module ModelSchema
 class_attribute :ignored_columns, instance_accessor: false
 self.ignored_columns = [].freeze

+ class_attribute :skip_defining_attributes_from_schema, instance_accessor: false
+ self.skip_defining_attributes_from_schema = false


Can we drop the skip_ and have it default to true?

sgrif · 2017-02-14T19:35:53Z

activerecord/lib/active_record/model_schema.rb

+
+ if skip_defining_attributes_from_schema
+ if ignored_columns.any?
+ raise ArgumentError, "can't use ignored_columns and skip_defining_attributes_from_schema at the same time"


Let's handle this in the writer method for define_attributes_from_schema and ignored_columns. We want to error early, and we shouldn't assume this is the only place the two can interact.

sgrif · 2017-02-14T19:36:37Z

activerecord/lib/active_record/model_schema.rb

+ raise ArgumentError, "can't use ignored_columns and skip_defining_attributes_from_schema at the same time"
+ end
+
+ if primary_key && !attributes_to_define_after_schema_loads.key?(primary_key)


This feels like it should be unnecessary.

Sometimes you want to explicitly declare attributes in the model with Attributes API and ignore the rest of table columns.

kirs · 2017-02-15T15:20:15Z

I've addressed all concerns 👌

Let's handle this in the writer method for define_attributes_from_schema and ignored_columns. We want to error early, and we shouldn't assume this is the only place the two can interact.

For that, I had to move away from class_attribute and define custom accessors.

kirs · 2017-02-20T20:17:50Z

🙏 👀 when you have a minute.

sgrif · 2017-07-17T14:40:49Z

activerecord/lib/active_record/model_schema.rb

- @columns_hash = connection.schema_cache.columns_hash(table_name).except(*ignored_columns)
+ @columns_hash = connection.schema_cache.columns_hash(table_name)
+
+ unless define_attributes_from_schema


This seems like the wrong place to handle things. We should not be modifying columns_hash based on this setting (other than perhaps not setting it at all). It seems like we should be skipping the loop down below instead.

kirs · 2017-07-27T10:36:44Z

@sgrif I'm ready to invest time into finishing this feature and addressing the feedback, but is it something that you'd like to see in ActiveRecord?

robotdana · 2019-02-15T00:26:11Z

is this something that is being worked on? I would like to use a feature like this. I'm happy to take finishing this PR if @kirs would like me to

rafaelfranca · 2019-02-15T01:29:20Z

@robotdana yes! I was planning to have someone in my team to work on it but we are happy to also review a PR if it is open.

robotdana · 2019-03-04T05:31:22Z

#35309 I have a PR ready, rebasing and finishing this. Unfortunately @sgrif it looked like not modifying the columns_hash was more complex than not, and if we changed it we should also change the existing implementation of ignored_columns (which does modify the columns_hash)

kirs · 2019-12-11T16:29:01Z

Closing in favour of #35309

rails-bot assigned sgrif Feb 4, 2017

simi reviewed Feb 4, 2017

View reviewed changes

matthewd reviewed Feb 4, 2017

View reviewed changes

maclover7 added the activerecord label Feb 4, 2017

sgrif suggested changes Feb 14, 2017

View reviewed changes

maclover7 added the needs work label Feb 14, 2017

Allow to disable attribute discovery in ActiveRecord

a959fb8

Sometimes you want to explicitly declare attributes in the model with Attributes API and ignore the rest of table columns.

sgrif suggested changes Jul 17, 2017

View reviewed changes

robotdana mentioned this pull request Feb 18, 2019

Optionally disable attribute discovery #35309

Closed

kirs closed this Dec 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to disable attribute discovery in ActiveRecord #27903

Allow to disable attribute discovery in ActiveRecord #27903

kirs commented Feb 4, 2017

rails-bot commented Feb 4, 2017

simi Feb 4, 2017

matthewd Feb 4, 2017

sgrif Feb 4, 2017

kirs Feb 5, 2017

sgrif commented Feb 4, 2017

kaspth commented Feb 5, 2017

wvanbergen commented Feb 5, 2017

sgrif commented Feb 7, 2017

sgrif commented Feb 7, 2017

kirs commented Feb 14, 2017

sgrif Feb 14, 2017

sgrif Feb 14, 2017

sgrif Feb 14, 2017

kirs commented Feb 15, 2017

kirs commented Feb 20, 2017

sgrif Jul 17, 2017

kirs commented Jul 27, 2017

robotdana commented Feb 15, 2019 •

edited

Loading

rafaelfranca commented Feb 15, 2019

robotdana commented Mar 4, 2019

kirs commented Dec 11, 2019

Allow to disable attribute discovery in ActiveRecord #27903

Allow to disable attribute discovery in ActiveRecord #27903

Conversation

kirs commented Feb 4, 2017

rails-bot commented Feb 4, 2017

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

sgrif commented Feb 4, 2017

kaspth commented Feb 5, 2017

wvanbergen commented Feb 5, 2017

sgrif commented Feb 7, 2017

sgrif commented Feb 7, 2017

kirs commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirs commented Feb 15, 2017

kirs commented Feb 20, 2017

Choose a reason for hiding this comment

kirs commented Jul 27, 2017

robotdana commented Feb 15, 2019 • edited Loading

rafaelfranca commented Feb 15, 2019

robotdana commented Mar 4, 2019

kirs commented Dec 11, 2019

robotdana commented Feb 15, 2019 •

edited

Loading