Rails 5.2 support #67
Conversation
All the instance variable wrangling has been kept for compatibility’s sake (given they’re in the specs). Instead of hand-crafting dirty support, this commit makes use of Rails’ own approach, though it does differ slightly between versions.
|
Thanks for the feedback Justin - just made most of the changes you requested (and provided a reason for the one I didn't). If there's any further tweaks required, do let me know! |
|
@madding Thanks! I wasn't able to allocate time to get Rails 5.2 support into the 0.5.0 release, but I hope to find time to review this and get another release out soon. Thanks for your patience! |
| @@ -267,12 +252,16 @@ def __vault_persist_attribute!(attribute, options) | |||
|
|
|||
| # Only persist changed attributes to minimize requests - this helps | |||
| # minimize the number of requests to Vault. | |||
| if !changed.include?("#{attribute}") | |||
| return | |||
| if ActiveRecord.gem_version >= Gem::Version.new("5.2") | |||
There was a problem hiding this comment.
I thinks it would be a good idea to use Rails previous_changes here.
previous_changes exists from Rails 3.0 and it works on the latest Rails 6(master) also.
previous_changes_include? is dropped in Rails 5.2.3
Something like this:
previous_changes.include?(attribute)
There was a problem hiding this comment.
Method https://github.com/rails/rails/blob/6-0-stable/activemodel/lib/active_model/dirty.rb#L227 based on changes, but changes didn't work for attributes like this.
There was a problem hiding this comment.
They behave differently, changes returns hash of changes before save, previous_changes after it's saved.
person = Person.first
person.name = "New name"
person.changes #=> {"name"=>["Old name", "New name"]}
person.previous_changes #=> {}
person.save
person.changes #=> {}
person.previous_changes #=> {"name"=>["Old name", "New name"],...}
There was a problem hiding this comment.
I've just pull this PR and test it against rails master. With previous_changes_include?(dropped method) it doesn't work, with previous_changes works as expected
There was a problem hiding this comment.
This is not my pull request, I have other one:
https://github.com/hashicorp/vault-rails/pull/76/files
I try show previous_changes here https://github.com/hashicorp/vault-rails/pull/76/files#diff-ba19de511d4dbf96b22d51bf1ecbf448R268 and it was empty when changed_attributes present.
May be it specific only for my PR.
There was a problem hiding this comment.
@dixpac thanks for the suggestion, and you're right, if I make that change and get the latest Rails 6 RC into the Appraisals file, the test suite is green.
@justincampbell did you want the Rails 6 fix added to this PR? Or would you prefer to merge what's here in first, and then I can create a smaller one for just Rails 6?
There was a problem hiding this comment.
Hello @pat and @justincampbell we are using this branch since September and would like to know if there is a todolist of outstanding items for the merge, now we already need support for Rails 6 as well.
We can work on this pull request if needed
There was a problem hiding this comment.
Hello! I'm interested in using this in production as well, but also would need this merged to master. Can I help at all?
There was a problem hiding this comment.
Hello @pat Is there any work left to proceed with the merge ?
There was a problem hiding this comment.
@mod if you're finding it works for you, then I think it's ready? I'm not an owner of the repo, I can't merge things.
And also: the organisation I worked for when creating this PR no longer exists, and so I'm no longer using this gem – so your experience with it is going to be far more valid and recent than mine.
Valarissa
dismissed
justincampbell’s stale review
I reviewed it myself and the changes requested were provided by the contributor(s).
|
Thanks @Valarissa :) |
Pull up a chair, and join me on this very fun walk-through of why, as of release 0.6.0, this gem doesn't actually support Rails 4.2 anymore and probably shouldn't. In #67, Rails 5+ support was added in the form of ditching our own dirty attribute logic and instead utilizing the fancy new Attributes API, which was officially introduced in Rails 5 but present (sorta) in Rails 4.2 as well. This very obviously cut off support of vault-rails for Rails 4.1, which doesn't have this API at all. Tests passed for Rails 4.2, so it looked good. However, the Attributes API in Rails 4.2 is actually totally private API - it's no more than a refactoring in anticipation of the real API in 5, and some of the API was accidentally exposed in documentation anyway in the 4.2 release. The whole point of the Attributes API is to provide a separation between ActiveModel attributes and the real database columns behind them (if any!), allowing you to flexibly cast types when putting and retrieving data from the database. However, in Rails 4.2 the API is simply a refactoring and the separation there doesn't exist: attributes are one-to-one with database columns. When vault-rails began to use the Attributes API in 0.6.0, a subtle behavior change occurred (for all versions of Rails): a `vault_attribute` is now registered as a real model attribute. This change is fine...in Rails 5+. In modern versions, ActiveRecord is smart enough to know that this defined attribute is _not_ backed by a database column, and is omitted from things that expect a column to exist. In Rails 4.2, it's not. The newly discovered attribute is expected to be backed by a database column, and things depend heavily on this expectation. The case that led me down finding this issue are complex queries utilizing joins and where conditions, which generate aliases through intermediate tables, e.g.: ``` SELECT "dogs"."id" AS t0_r0, "dogs"."owner_id" AS t0_r1, ........ ``` These queries are generated with a model's columns in mind, so you will now get an error with 0.6.0 and Rails 4.2 (where current_thought is a vault_attribute): ``` ActiveRecord::StatementInvalid: PG::UndefinedColumn: ERROR: column dogs.current_thought does not exist LINE 1: ...AS t3_r38, "dogs"."current_thought" AS t3_r39, "dogs"."u... ``` Conclusion: As Rails 4.2 is long EOL'd anyway and we do *not* wish to maintain the old implementation alongside using the Attributes API, it makes sense to consider 0.6.x as supporting only Rails 5+. 0.5.0 will be the last known compatible version with Rails =< 4.2
Pull up a chair, and join me on this very fun walk-through of why, as of release 0.6.0, this gem doesn't actually support Rails 4.2 anymore and probably shouldn't. In #67, Rails 5+ support was added in the form of ditching our own dirty attribute logic and instead utilizing the fancy new Attributes API, which was officially introduced in Rails 5 but present (sorta) in Rails 4.2 as well. This very obviously cut off support of vault-rails for Rails 4.1, which doesn't have this API at all. Tests passed for Rails 4.2, so it looked good. However, the Attributes API in Rails 4.2 is actually totally private API - it's no more than a refactoring in anticipation of the real API in 5, and some of the API was accidentally exposed in documentation anyway in the 4.2 release. The whole point of the Attributes API is to provide a separation between ActiveModel attributes and the real database columns behind them (if any!), allowing you to flexibly cast types when putting and retrieving data from the database. However, in Rails 4.2 the API is simply a refactoring and the separation there doesn't exist: attributes are one-to-one with database columns. When vault-rails began to use the Attributes API in 0.6.0, a subtle behavior change occurred (for all versions of Rails): a `vault_attribute` is now registered as a real model attribute. This change is fine...in Rails 5+. In modern versions, ActiveRecord is smart enough to know that this defined attribute is _not_ backed by a database column, and is omitted from things that expect a column to exist. In Rails 4.2, it's not. The newly discovered attribute is expected to be backed by a database column, and things depend heavily on this expectation. The case that led me down finding this issue are complex queries utilizing joins and where conditions, which generate aliases through intermediate tables, e.g.: ``` SELECT "dogs"."id" AS t0_r0, "dogs"."owner_id" AS t0_r1, ........ ``` These queries are generated with a model's columns in mind, so you will now get an error with 0.6.0 and Rails 4.2 (where current_thought is a vault_attribute): ``` ActiveRecord::StatementInvalid: PG::UndefinedColumn: ERROR: column dogs.current_thought does not exist LINE 1: ...AS t3_r38, "dogs"."current_thought" AS t3_r39, "dogs"."u... ``` Conclusion: As Rails 4.2 is long EOL'd anyway and we do *not* wish to maintain the old implementation alongside using the Attributes API, it makes sense to consider 0.6.x as supporting only Rails 5+. 0.5.0 will be the last known compatible version with Rails =< 4.2. This commit sets up dropping Rails 4.2 by demonstrating at this point that tests will fail with Rails 4.2 and pass with Rails 5+
This is built on top of #66, but I'm just going to focus here on what's been done in 169f021 (and further commits after that, should there be changes):
@ssn), given they're covered by specs - though I personally would prefer to scrap that support, which makes the code a little more simpler. They're ideally not part of any public API, I'd hope?klass.instance_methods.klasscannot be instantiated (no table exists) hence switching to usePersoninstead.