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.

Sign up for GitHub

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

Rails 5.2 support #67

Merged
merged 1 commit into from
May 4, 2020
Merged

Rails 5.2 support #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Appraisals
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
appraise "rails-4.1" do
gem "rails", "~> 4.1.0"
end if RUBY_VERSION.to_f < 2.4

appraise "rails-4.2" do
gem "rails", "~> 4.2.0"
end
Expand All @@ -13,3 +9,7 @@ end
appraise "rails-5.1" do
gem "rails", "~> 5.1.0"
end

appraise "rails-5.2" do
gem "rails", "~> 5.2.0"
end
42 changes: 16 additions & 26 deletions lib/vault/encrypted_model.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,13 @@ def vault_attribute(attribute, options = {})
serializer.define_singleton_method(:decode, &options[:decode])
end

self.attribute attribute.to_s, ActiveRecord::Type::Value.new,
default: nil

# Getter
define_method("#{attribute}") do
self.__vault_load_attributes! unless @__vault_loaded
instance_variable_get("@#{attribute}")
super()
pat marked this conversation as resolved.
Show resolved Hide resolved
end

# Setter
Expand All @@ -79,34 +82,15 @@ def vault_attribute(attribute, options = {})

attribute_will_change!("#{attribute}")
instance_variable_set("@#{attribute}", value)
super(value)

# Return the value to be consistent with other AR methods.
value
end

# Checker
define_method("#{attribute}?") do
self.__vault_load_attributes! unless @__vault_loaded
instance_variable_get("@#{attribute}").present?
end

# Dirty method
define_method("#{attribute}_change") do
changes["#{attribute}"]
end

# Dirty method
define_method("#{attribute}_changed?") do
changed.include?("#{attribute}")
end

# Dirty method
define_method("#{attribute}_was") do
if changes["#{attribute}"]
changes["#{attribute}"][0]
else
public_send("#{attribute}")
end
public_send(attribute).present?
end

# Make a note of this attribute so we can use it in the future (maybe).
Expand Down Expand Up @@ -212,7 +196,7 @@ def __vault_load_attribute!(attribute, options)

# If the user provided a value for the attribute, do not try to load
# it from Vault
if instance_variable_get("@#{attribute}")
if attributes[attribute.to_s]
return
end

Expand All @@ -232,6 +216,7 @@ def __vault_load_attribute!(attribute, options)

# Write the virtual attribute with the plaintext value
instance_variable_set("@#{attribute}", plaintext)
@attributes.write_from_database attribute.to_s, plaintext
end

# Encrypt all the attributes using Vault and set the encrypted values back
Expand Down Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@dixpac dixpac Aug 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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"],...}

Copy link
Contributor

@dixpac dixpac Aug 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

@madding madding Aug 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

Hello! I'm interested in using this in production as well, but also would need this merged to master. Can I help at all?

Copy link

Choose a reason for hiding this comment

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

Hello @pat Is there any work left to proceed with the merge ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return unless previous_changes_include?(attribute)
elsif ActiveRecord.gem_version >= Gem::Version.new("5.1")
return unless saved_change_to_attribute?(attribute.to_s)
else
return unless attribute_changed?(attribute)
pat marked this conversation as resolved.
Show resolved Hide resolved
end

# Get the current value of the plaintext attribute
plaintext = instance_variable_get("@#{attribute}")
plaintext = attributes[attribute.to_s]

# Apply the serialize to the plaintext value, if one exists
if serializer
Expand Down Expand Up @@ -319,6 +308,7 @@ def reload(*)
# from Vault
self.class.__vault_attributes.each do |attribute, _|
self.instance_variable_set("@#{attribute}", nil)
@attributes.write_from_database attribute.to_s, nil
end

self.__vault_initialize_attributes!
Expand Down
7 changes: 3 additions & 4 deletions spec/unit/encrypted_model_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,9 @@
end

it "defines dirty attribute methods" do
klass.vault_attribute(:foo)
expect(klass.instance_methods).to include(:foo_change)
expect(klass.instance_methods).to include(:foo_changed?)
expect(klass.instance_methods).to include(:foo_was)
expect(Person.new).to respond_to(:ssn_change)
expect(Person.new).to respond_to(:ssn_changed?)
expect(Person.new).to respond_to(:ssn_was)
end
end
end