Introduce `compressor` option to `ActiveRecord::Encryption::Encryptor` #51735

heka1024 · 2024-05-04T10:59:27Z

Motivation / Background

There are more performant algorithms than zlib, like zstd or snappy. So I made it possible to configure the compressor to take advantage of these more performant algorithms.

Detail

Make compressor as argument which default value is zlib.

Additional information

https://facebook.github.io/zstd/

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Discussion

What do you thinks about make compressor as property of ActiveRecord::Encryption ?

flavorjones

The naming of the custom compressor's methods seems inconsistent to me. Would you consider using Encryptor's convention and name them compress/uncompress instead of ZLib's deflate/inflate?

I also think this PR should update the Rails Guides page on encryption, guides/source/active_record_encryption.md to explain how to implement a custom compressor.

heka1024 · 2024-05-07T09:26:34Z

Hi, @flavorjones ! I changed signature of compressor (compress/uncompress) and make compressor configurable. And add some document about compression. Could you review it? I'd really appreciate it if you could provide feedback!

activerecord/CHANGELOG.md

zzak · 2024-05-19T01:03:17Z

CI failures seem unrelated, you might want to rebase just to be sure as I think most of those have been fixed

activerecord/lib/active_record/encryption/compressor.rb

heka1024 · 2024-05-19T07:01:38Z

CI failures seem unrelated, you might want to rebase just to be sure as I think most of those have been fixed

@zzak I just rebased it, and it looks like all CI failures are fixed now. 😄

flavorjones

OK, so I'm only realizing now that Rails already has prior art for this sort of compression API, see the docs for ActiveSupport::Cache::Store.new and the :compressor option.

In summary, "Must respond to deflate and inflate"

So I hate to ask, especially since I asked you to change it in the first place, but can you update this PR to use that naming convention? I sincerely apologize for not realizing this sooner.

activerecord/CHANGELOG.md

guides/source/active_record_encryption.md

flavorjones · 2024-05-19T19:59:11Z

guides/source/active_record_encryption.md

@@ -497,6 +533,10 @@ The digest algorithm used to derive keys. `OpenSSL::Digest::SHA256` by default.
 Supports decrypting data encrypted non-deterministically with a digest class SHA1. Default is false, which
 means it will only support the digest algorithm configured in `config.active_record.encryption.hash_digest_class`.

+#### `config.active_record.encryption.compressor`
+
+The compressor used to compress encrypted payloads. It should respond to `compress` and `uncompress`. Default is `Zlib`. You can find more information about compressors in the [Compression](#compression) section.


Same comment here -- please use the full namespaced name to disambiguate.

So I hate to ask, especially since I asked you to change it in the first place, but can you update this PR to use that naming convention? I sincerely apologize for not realizing this sooner.

@flavorjones Don't mind. It's not a big deal.

By using inflate/deflate, we don't need wrapper class for zlib, ActiveRecord::Encryption::Compressor::Zlib. Because Zlib can respond to inflate/deflate. So I think using Zlib in document is right. What do you think?

ref: ActiveSupport::Cache

rails/activesupport/lib/active_support/cache.rb

Line 305 in b09ae67

compressor = @options.delete(:compressor) { Zlib }

Yup, makes sense.

flavorjones · 2024-05-20T18:35:15Z

guides/source/active_record_encryption.md

@@ -497,6 +533,10 @@ The digest algorithm used to derive keys. `OpenSSL::Digest::SHA256` by default.
 Supports decrypting data encrypted non-deterministically with a digest class SHA1. Default is false, which
 means it will only support the digest algorithm configured in `config.active_record.encryption.hash_digest_class`.

+#### `config.active_record.encryption.compressor`
+
+The compressor used to compress encrypted payloads. It should respond to `compress` and `uncompress`. Default is `Zlib`. You can find more information about compressors in the [Compression](#compression) section.


Yup, makes sense.

heka1024 · 2024-05-21T00:37:09Z

There is a ci failure but it looks like unrelated.

Failure:
--
| ActiveRecord::ConnectionAdapters::ReaperTest#test_connection_pool_starts_reaper_in_fork [test/cases/reaper_test.rb:161]:
| Expected #<Process::Status: pid 79 SIGABRT (signal 6) (core dumped)> to be success?.

eileencodes

The feature seems like a good idea to me, thanks for working on it @heka1024

I left a comment about the API design and I also think this should provide a config option so apps that want the same custom compressor for every model can just set it once.

eileencodes · 2024-05-22T17:29:31Z

activerecord/CHANGELOG.md

+    end
+
+    class User
+      encrypts :name, encryptor: ActiveRecord::Encryption::Encryptor.new(compressor: ZstdCompressor)


This API doesn't feel right. Having to initialize a new Encryptor in your app to set the compressor feels cumbersome and un-Rails like. I have two suggestions:

I think if we want a compressor option for every model then Rails should do the initialization for the app. The AR code would check if the compressor option is set and then Rails would do the initialization for the application. Then it's a much cleaner API because it becomes:

encrypts :name, compressor: ZstdCompressor

Rails should provide a config option so that applications can set all encryptors to use the same custom compressor.

config.active_record_encryption.compressor = ZstdCompressor

rails-bot bot added the activerecord label May 4, 2024

heka1024 force-pushed the encryption-compressor branch from aeb3171 to 17ff722 Compare May 4, 2024 11:46

flavorjones suggested changes May 5, 2024

View reviewed changes

heka1024 force-pushed the encryption-compressor branch from 17ff722 to 633cdc2 Compare May 7, 2024 08:55

rails-bot bot added the docs label May 7, 2024

heka1024 force-pushed the encryption-compressor branch 2 times, most recently from 883a321 to 3231918 Compare May 7, 2024 08:58

pokonski reviewed May 8, 2024

View reviewed changes

activerecord/CHANGELOG.md Outdated Show resolved Hide resolved

pokonski reviewed May 8, 2024

View reviewed changes

activerecord/CHANGELOG.md Show resolved Hide resolved

heka1024 force-pushed the encryption-compressor branch 5 times, most recently from a34e1dd to d2de562 Compare May 14, 2024 05:50

zzak reviewed May 19, 2024

View reviewed changes

activerecord/lib/active_record/encryption/compressor.rb Outdated Show resolved Hide resolved

heka1024 force-pushed the encryption-compressor branch 4 times, most recently from 5664b8c to d0a8297 Compare May 19, 2024 06:46

flavorjones suggested changes May 19, 2024

View reviewed changes

heka1024 force-pushed the encryption-compressor branch 3 times, most recently from a7ff27d to a0be4ad Compare May 20, 2024 14:30

flavorjones approved these changes May 20, 2024

View reviewed changes

flavorjones added the ready PRs ready to merge label May 20, 2024

heka1024 force-pushed the encryption-compressor branch 2 times, most recently from 666952d to 18b4cc2 Compare May 21, 2024 00:15

heka1024 force-pushed the encryption-compressor branch from 18b4cc2 to 038b9a9 Compare May 21, 2024 12:17

Introduce compressor option to ActiveRecord::Encryption::Encryptor

287ca48

heka1024 force-pushed the encryption-compressor branch from 038b9a9 to 287ca48 Compare May 22, 2024 06:47

eileencodes requested changes May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `compressor` option to `ActiveRecord::Encryption::Encryptor` #51735

Introduce `compressor` option to `ActiveRecord::Encryption::Encryptor` #51735

heka1024 commented May 4, 2024 •

edited

flavorjones left a comment

heka1024 commented May 7, 2024 •

edited

zzak commented May 19, 2024

heka1024 commented May 19, 2024

flavorjones left a comment

flavorjones May 19, 2024

heka1024 May 20, 2024 •

edited

flavorjones May 20, 2024

flavorjones May 20, 2024

heka1024 commented May 21, 2024

eileencodes left a comment

eileencodes May 22, 2024 •

edited

Introduce compressor option to ActiveRecord::Encryption::Encryptor #51735

Are you sure you want to change the base?

Introduce compressor option to ActiveRecord::Encryption::Encryptor #51735

Conversation

heka1024 commented May 4, 2024 • edited

Motivation / Background

Detail

Additional information

Checklist

Discussion

flavorjones left a comment

Choose a reason for hiding this comment

heka1024 commented May 7, 2024 • edited

zzak commented May 19, 2024

heka1024 commented May 19, 2024

flavorjones left a comment

Choose a reason for hiding this comment

flavorjones May 19, 2024

Choose a reason for hiding this comment

heka1024 May 20, 2024 • edited

Choose a reason for hiding this comment

flavorjones May 20, 2024

Choose a reason for hiding this comment

flavorjones May 20, 2024

Choose a reason for hiding this comment

heka1024 commented May 21, 2024

eileencodes left a comment

Choose a reason for hiding this comment

eileencodes May 22, 2024 • edited

Choose a reason for hiding this comment

Introduce `compressor` option to `ActiveRecord::Encryption::Encryptor` #51735

Introduce `compressor` option to `ActiveRecord::Encryption::Encryptor` #51735

heka1024 commented May 4, 2024 •

edited

heka1024 commented May 7, 2024 •

edited

heka1024 May 20, 2024 •

edited

eileencodes May 22, 2024 •

edited