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

Implement ActiveRecord::Base.ignored_columns #21720

Merged
merged 1 commit into from
Sep 24, 2015
Merged

Conversation

byroot
Copy link
Member

@byroot byroot commented Sep 22, 2015

Use cases

This is useful when you use online schema change tools like https://github.com/soundcloud/lhm or pt-online-schema-change.

Since some columns will appear or disappear at any moment, you want to make those columns invisible to AR for a while. Otherwise you could have some processes knowing about the columns and others who don't.

Another use case could be to prevent the use of a column in one class that share a table via STI.

Unanswered questions

There is a couple edge cases I'd like thoughts on:

  • The current implementation doesn't allow to change the list of ignored_columns after the columns have been loaded. Should we allow it? If so it mean we need to undefine methods which not great. Maybe just a warning?
  • Quid of inheritance? Should those be inherited or not? FWIW I think they should.

@matthewd @sgrif @rafaelfranca for review please.

@rafaelfranca
Copy link
Member

cc @jeremy

@sgrif
Copy link
Contributor

sgrif commented Sep 22, 2015

The current implementation doesn't allow to change the list of ignored_columns after the columns have been loaded. Should we allow it? If so it mean we need to undefine methods which not great. Maybe just a warning?

It actually wouldn't be hard to handle this. I think you'll just need to call clear_caches_calculated_from_columns, and undefine_attribute_methods.

@sgrif
Copy link
Contributor

sgrif commented Sep 22, 2015

Also just as a note, even though this is tangential to the feature, I'd like to have an explicit test that clarifies that ignored_columns is a very specific and correct name. e.g. this only skips automatic invocations of the attributes API based on schema inference, but if there is an explicitly defined attribute on the model (either through the attributes API, or manual method definitions), then this will have no affect.

@byroot
Copy link
Member Author

byroot commented Sep 22, 2015

if there is an explicitly defined attribute on the model (either through the attributes API, or manual method definitions), then this will have no affect.

Sure I can test this.

@egilburg
Copy link
Contributor

Is this feature intended only for schema changing purposes, or would it make sense to use it for app logic in order to not expose to AR uneeded db columns for security, performance, or encapsulation reasons?

In latter case, it's blacklist approach so I wonder whether it'll eventually grow into the same issue as attr_protected vs attr_accessible years ago. Should a whitelist alternative exist as well?

@sgrif
Copy link
Contributor

sgrif commented Sep 22, 2015

A low priority feature I'm planning is the ability to turn off automatic
schema inference entirely, so that + attributes API would work quite well
as a whitelist.

On Tue, Sep 22, 2015 at 3:01 PM Eugene Gilburg notifications@github.com
wrote:

Is this feature intended only for schema changing purposes, or would it
make sense to use it for app logic in order to not expose to AR uneeded db
columns for security, performance, or encapsulation reasons?

In latter case, it's blacklist approach so I wonder whether it'll
eventually grow into the same issue as attr_protected vs attr_accessible
years ago. Should a whitelist alternative exist as well?


Reply to this email directly or view it on GitHub
#21720 (comment).

@byroot
Copy link
Member Author

byroot commented Sep 23, 2015

@egilburg as mentioned in the PR description usages others than online schema change can be:

  • Hiding some columns from STI subclasses
  • Ignoring some columns from a legacy database (I once worked on an app where a column was named associations... fun times).

But yes online schema change is the biggest use case.

@jeremy
Copy link
Member

jeremy commented Sep 23, 2015

We use pt-osc also. Adding and changing columns are generally safe, but dropping columns needs something like this. We rarely drop, partly because it's more problematic to pull off with no downtime.

Declaring ignored attributes on the model is a pragmatic approach and a good proxy for reflecting the effective new schema change, but feels at a mismatched level of abstraction. Is there an alternate way to tell AR about the effective new schema?

@byroot
Copy link
Member Author

byroot commented Sep 23, 2015

feels at a mismatched level of abstraction

Not sure I understand what you mean here. You don't think the list of columns is an AR::Base concern?

@haileys
Copy link
Contributor

haileys commented Sep 24, 2015

👍

Basically every large Rails app is already doing something like this. It'd be great to have an officially blessed way of doing it.

sgrif added a commit that referenced this pull request Sep 24, 2015
Implement ActiveRecord::Base.ignored_columns
@sgrif sgrif merged commit 8587cf0 into rails:master Sep 24, 2015
@jeremy
Copy link
Member

jeremy commented Sep 24, 2015

@byroot The behavior of the model and characteristics of its attributes are AR::Base concerns for sure 😁 I think this is a great immediate change that exposes a long-running feeling that we have a missing link between the model and the db conn: a table definition.

AR::Base handles automatic schema introspection and mapping, but doesn't expose an abstraction for the table definition itself or allow it to be altered. Perhaps columns/schema introspection should be at arms' length from AR::Base: delegate the job to a TableDefinition that we can provide ourselves, introspect from the db conn, and alter.

Another approach would be to push the responsibility to Arel. Make the table's base relation a projection that doesn't include the defunct column.

In either case, we're pushing up against a missing capability to control schema introspection ourselves.

jonatack added a commit to jonatack/rails that referenced this pull request Sep 24, 2015
refute_includes Developer.columns_hash.keys, 'first_name'
end

test "ignored columns have no attirbute methods" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix. Thanks for the heads up.

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jun 7, 2016
jturkel added a commit to jturkel/strong_migrations that referenced this pull request May 17, 2017
Update the "Removing a column" section to include details on the `ActiveRecord::Base.ignored_columns` method that landed in rails/rails#21720.
@jrochkind
Copy link
Contributor

Would there be any way to use this with single-table inheritance, to hide certain columns not relevant to a specific sub-class even though they are in the shared table? Or is the cached attribute list going to be shared, so you end up removing them from all or nothing anyway?

ngen-brett pushed a commit to ngen-brett/gitlab-ce that referenced this pull request Aug 10, 2020
`IgnorableColumn` module in `app/models/concerns/ignorable_column.rb`
which is used in models to ignore dealing with specified columns.

In Rails 5 with this [pull
request](rails/rails#21720),
`self.ignored_columns` was introduced to do this job.

For example, in app/models/user.rb:

```ruby
ignore_column :external_email
ignore_column :email_provider
```

can be changed to:

```ruby
self.ignored_columns = %i[external_email email_provider]
```
fatkodima added a commit to fatkodima/online_migrations that referenced this pull request Jan 23, 2022
…ord 4.2

`ActiveRecord::Base.ignored_columns` was added in Rails 5 - rails/rails#21720
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants