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

Conversation

Projects
None yet
8 participants
@byroot
Member

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

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca
Member

rafaelfranca commented Sep 22, 2015

cc @jeremy

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Sep 22, 2015

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Sep 22, 2015

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@byroot

byroot Sep 22, 2015

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Sep 22, 2015

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?

Contributor

egilburg commented Sep 22, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Sep 22, 2015

Member

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).

Member

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

This comment has been minimized.

Show comment
Hide comment
@byroot

byroot Sep 23, 2015

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Sep 23, 2015

Member

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?

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

This comment has been minimized.

Show comment
Hide comment
@byroot

byroot Sep 23, 2015

Member

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?

Member

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?

@charliesome

This comment has been minimized.

Show comment
Hide comment
@charliesome

charliesome Sep 24, 2015

Contributor

👍

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

Contributor

charliesome 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

Merge pull request #21720 from byroot/ignore-columns
Implement ActiveRecord::Base.ignored_columns

@sgrif sgrif merged commit 8587cf0 into rails:master Sep 24, 2015

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Sep 24, 2015

Member

@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.

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

This comment has been minimized.

@henrik

henrik Sep 26, 2015

Contributor

Typo

@henrik

henrik Sep 26, 2015

Contributor

Typo

This comment has been minimized.

@byroot

byroot Sep 26, 2015

Member

Will fix. Thanks for the heads up.

@byroot

byroot Sep 26, 2015

Member

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

Leverage Rails 5 ignored_columns
Update the "Removing a column" section to include details on the `ActiveRecord::Base.ignored_columns` method that landed in rails/rails#21720.
@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Nov 6, 2017

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?

Contributor

jrochkind commented Nov 6, 2017

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment