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

Jump to bottom

Idea: Don't return virtual attributes in attribute methods unless the ciphertext attribute is present #186

Closed
4ndypanda opened this issue Jan 16, 2024 · 3 comments
Closed

Idea: Don't return virtual attributes in attribute methods unless the ciphertext attribute is present #186

4ndypanda opened this issue Jan 16, 2024 · 3 comments

Comments

@4ndypanda
Copy link
Contributor

I was also annoyed by this issue since I constantly run into ActiveModel::MissingAttributeError when doing stuff like

# User class defined in test/support/active_record.rb
User.create!(email: "[email protected]")
user = User.select('id').last

# Normally this is done with some metaprogramming, just showing an example
# that raises ActiveModel::MissingAttributeError
user.email if user.has_attribute?("email")

What are your thoughts on overriding #init_with_attributes in ActiveRecord::Core? Before ever setting the @attributes instance variable for the model, we could filter and check if the ciphertext attribute is present

# From ActiveRecord::Core
def init_with_attribute(attributes, new_record = false)
  # filter out lockbox attributes that don't have ciphertext attribute loaded
  attributes = attributes.reject do |attribute, _value|
    self.lockbox_attributes[attribute.to_sym].present? &&
    !attributes.key?(
      self.lockbox_attributes[attribute.to_sym][:encrypted_attribute],
    )
  end
  super(attributes, new_record)
end

I was trying to see where else to do it. The caller here takes the attributes from the database result set and merges it with the self.class._default_attributes that includes the virtual attributes.

# Given a class, an attributes hash,  instantiate_instance_of  returns a
# new instance of the class. Accepts only keys as strings.
def instantiate_instance_of(klass, attributes, column_types = {}, &block)
  attributes = klass.attributes_builder.build_from_database(attributes, column_types)
  klass.allocate.init_with_attributes(attributes, &block)
end

Overriding attributes_builder.build_from_database (source) is another option.

What are your thoughts?
@4ndypanda
Copy link
Contributor Author

A tricky thing is if people do want virtual attributes with a ciphertext virtual attribute (not from an ActiveRecord column)

class User < ActiveRecord::Base
  # This is never saved to the database, idk why one
  # would want this though.
  attribute :foo_ciphertext
  has_encrypted :foo
end

This is one case where it probably should still show up in the attribute methods attributes, attribute_names, has_attribute?.

@ankane
Copy link
Owner

ankane commented Jan 18, 2024

Hey @4ndypanda, will take a look at this as part of 2.0 since it's a breaking change.

@ankane ankane closed this as completed in 65691d2 Oct 23, 2024
@ankane
Copy link
Owner

ankane commented Oct 24, 2024

Hi @4ndypanda, spent some time on this, and I think it'll (probably) be safer to override the external attributes methods rather than the internal ones, so pushed that change in the commit above.

Thanks for suggesting this improvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ankane @4ndypanda