Move length-validation for encrypted columns after schema is loaded #43009
Conversation
Doing it before meant it required a database connection at class loading time, which was a source a trouble. See rails#42991 (comment)
jorgemanrubia
force-pushed
the
encryption-lenght-validation
branch
from
7d0340a to
f0fe547
Compare
|
Thank you! |
| validate_column_size attribute_name | ||
| end | ||
| end | ||
|
|
||
| def validate_column_size(attribute_name) | ||
| if table_exists? && limit = columns_hash[attribute_name.to_s]&.limit |
There was a problem hiding this comment.
This is slightly outside the diff and apologies for the nitpick comment.
I'm wondering why we call table_exists? for every attribute_name is the table for every attribute the same? Can we move this call a little higher and return early in the method add_length_validation_for_encrypted_columns?
There was a problem hiding this comment.
(found this whilst reading the code, it's totally unrelated to the changes so I'm not sure if it's worth doing) I think we can move this https://github.com/rails/rails/pull/43009/files#R51 slightly higher too, because the scheme looks to be the same for every attribute.
There was a problem hiding this comment.
I'm wondering why we call table_exists? for every attribute_name is the table for every attribute the same?
Great catch @hahmed. With the new approach where we only add validations when the schema loads, so this check is not needed. I had added it back in the day to deal with the situation where Rails tried to load a class with encrypted attributes without a table (such as when you don't use action text but Rails loads it). I removed the check 👍.
There was a problem hiding this comment.
I think we can move this https://github.com/rails/rails/pull/43009/files#R51 slightly higher too, because the scheme looks to be the same for every attribute.
@hahmed sorry which line is that? I can't see which piece of code you are referring to in that link 😅.
Thanks for the review!
There was a problem hiding this comment.
Whoops, I meant this line here https://github.com/basecamp/rails/blob/f0fe547e20476e00637d15de57c14532fec6c01c/activerecord/lib/active_record/encryption/encryptable_record.rb#L51
So I think that we could do something like this?
def encrypts(*names, key_provider: nil, key: nil, deterministic: false, downcase: false, ignore_case: false, previous: [], **context_properties)
self.encrypted_attributes ||= Set.new # not using :default because the instance would be shared across classes
scheme = scheme_for key_provider: key_provider, key: key, deterministic: deterministic, downcase: downcase, \
ignore_case: ignore_case, previous: previous, **context_properties
names.each do |name|
encrypt_attribute name, scheme
end
end
As in, we don't need to compute the scheme for every attribute?
There was a problem hiding this comment.
Oh totally @hahmed, good eye on that one 🙏! I just pushed the suggested change.
There was a problem hiding this comment.
Created a new PR with the two suggestions #43021
There was a problem hiding this comment.
(added you as coauthor there)
|
Thanks for taking care of this @jorgemanrubia 👍 |
Co-authored-by: Haroon Ahmed <[email protected]>
See rails#43009 (comment) Remove `table_exists?` check that became unnecessary after rails#43009 Co-authored-by: Haroon Ahmed <[email protected]>
Doing it before makes it require a database connection at class loading time, which can be a source of trouble.
For context, we validate length for encrypted columns by default because it uses compression under the hood, and this can be leveraged to store huge payloads specially crafted to be compressible.
See #42991 (comment)
cc @byroot @hahmed