enable custom Blob#key configuration #1

frantisekrokusekpa · 2019-12-31T17:19:28Z

Summary

Hello Rails Team!

First of all I want to thank you for all job that you make here!!! Rails is a wonderfull framework to work with.

Here is my issue: ActiveStorage custom storage path configuration

Here is ou business issue: We use the apartment gem to split our datas on a client basis and we want to make the same with uploaded files and store them in separate folders.

Rails versions: >5.2

thoughtbot has made a wonderfull paperclip gem, but since it is depracated we are in the process of switching to ActiveStorage all our attachments management.

In Paperclip there is a fine feature where you can choose a specific storage path for each of the model attachable attribute and interpolate values in it as follows:

# app/models/user.rb

# in Paperclip
has_attached_file :avatar,
                  path: ':tenant/users/:user_id/:hash/:filename'

The idea is to make this possible in ActiveStorage too and have something like that:

# app/models/user.rb

# in futur ActiveStorage
has_one_attached :avatar,
                 path: ':tenant/users/:id/:unique_secure_token'

For the moment we have made a monkey_patching to implement those storage path by changing the ActiveStorage::Blob#key.

# app/models/user.rb

def document=(attachable)
  document.attach(create_blob(attachable))
end

def attachable_storage_path
  [
    Apartment::Tenant.current.parameterize,
    'users',
    id,
    ActiveStorage::Blob.generate_unique_secure_token
  ].compact.join('/')
end

# app/models/application_record.rb

def attachable_storage_path
  raise NotImplementedError
end

def create_blob(attachable)
  return if attachable.nil?

  ActiveStorage::Blob.new.tap do |blob|
    blob.filename = attachable.original_filename
    blob.key = attachable_storage_path
    blob.upload(attachable)
    blob.save!
  end
end

I have began to make a PR on ActiveStorage but want to have your feedback on this feature before digging deeper in this problematic and validate the right direction! Some questions to guide:

Is the ActiveStorage::Blob#keythe right attribute to integrate this feature?
Do you think it is worth to make it interpolable as in Paperclip, to avoid being requested to create an #attachable_storage_path method?

Here it is: ActiveStorage custom storage path configuration

Thank you for your feedback and advices!

Other Information

jgigault

I know it's too early to submit a review, but it may help you going further!

activestorage/lib/active_storage/attached/model.rb

activestorage/app/models/active_storage/blob.rb

jgigault · 2020-01-06T18:45:56Z

activestorage/lib/active_storage/attached/changes/create_one.rb

@@ -77,5 +79,9 @@ def find_or_build_blob
      def attachment_service_name
        record.attachment_reflections[name].options[:service_name]
      end
+
+      def attachment_key(key: key)


What is the purpose of the argument? Are you willing to use it somewhere?

sedubois · 2020-03-26T08:29:16Z

@frantisekrokusekpa do you still have any plans to submit this PR to Rails? It would be quite helpful to better organize files.

Ispirett · 2020-11-30T06:41:44Z

activestorage/app/models/active_storage/blob.rb

@@ -116,6 +116,14 @@ def create_before_direct_upload!(key: nil, filename:, byte_size:, checksum:, con
    def generate_unique_secure_token(length: MINIMUM_TOKEN_LENGTH)


THis is great work did you finish building this feature?

Yes I will try, now I have more time.

@Ispirett the PR have been pushed to rails. Do not hesitate to upvote!

activestorage/lib/active_storage/attached/model.rb

frantisekrokusekpa · 2020-12-18T22:23:59Z

@jgigault and @guillaumebriday would you mind to have a quick review please?

activestorage/app/models/active_storage/blob.rb

activestorage/lib/active_storage/attached/model.rb

sedubois · 2020-12-25T08:59:44Z

The Rails 6.1 changelog contains this info :

You can optionally provide a custom blob key when attaching a new file:

user.avatar.attach key: "avatars/#{user.id}.jpg",
  io: io, content_type: "image/jpeg", filename: "avatar.jpg"
Active Storage will store the blob's data on the configured service at the provided key.

George Claghorn

So is this PR still needed?

frantisekrokusekpa · 2021-01-03T15:40:19Z

The Rails 6.1 changelog contains this info :

You can optionally provide a custom blob key when attaching a new file:

user.avatar.attach key: "avatars/#{user.id}.jpg",
  io: io, content_type: "image/jpeg", filename: "avatar.jpg"
Active Storage will store the blob's data on the configured service at the provided key.

George Claghorn

So is this PR still needed?

Thanks Sébastien, I am aware of that change. This PR is tackling an other part of that feature.

frantisekrokusekpa · 2021-01-03T17:19:49Z

Please upvote/comment the final PR on Rails 🎉 rails#41004 Thanks

Fix empty request inside helpers test

…on-assert-nothing-raised Increment assertions count on assert_nothing_raised

In cache stores prepending `LocalCache`, serialized `Entry` instances are stored in `LocalCache::LocalStore`. This speeds up hot key lookups without a network roundtrip in a context of a single given request. However, with these entries being stored in their serialized form, `#read_multi_entries` returns them directly to cache consumers. Instead, we will now deserialize these entries first.

This commit adds documentation to the constants and methods that are part of Active Model's Attributes API. So far this API has been hidden with the :nodoc: flag since its inception in Active Record and subsequent move to Active Model (rails#30920 and rails#30985); as the API matures and gets ready for public usage, visible documentation for its endpoints becomes necessary. The classes and modules being documented and publicized by this commit are the main `Attributes` module, the `Type` namespace, and all the standard attribute type classes included in the current API, which users will be able to extend and replicate to suit their customization needs. Some private modules are also receiving documetation, although they will continue using the :nodoc: flag. Those are `Attribute` and `AttributeSet`. Although they remain private I found useful to add some comments to describe their responsibilities.

Documentation for Active Model Attributes [ci-skip]

…g-warning-in-test Fix ruby 2.7 keyword args warning for postgresql reconnection_error test

add hint to ActionView's fields_for

The `filter_parameters` configuration includes a list of filters in the latest `filter_parameter_logging` initializer template. This updates the guides to reflect those changes.

@counter

Running the Action Pack tests outputs a warning: ./actionpack/test/controller/test_case_test.rb:1007: warning: instance variable @counter not initialized Surrounding the line with silence_warnings cleans up the output.

Update parameter filter logging guides [ci-skip]

Follow-up to rails#44388.

Add `ActiveModel::Access`

Also fix AbstractMysqlAdapterTest

…_state Use finish_with_state inside Rack::Logger

The database server discards prepared statements when the client disconnects

@connection

This is unfortunately going to force a change into all external adapter subclasses, but I'm going to be making other changes too, so it's not the worst of them... and it's just so confusing at the moment to have "@connection" sometimes mean the AR adapter and sometimes the inner connection, depending on exactly which class we're in.

Rename the raw connection ivar to `@raw_connection`

This gets an annoyingly verbose comment, because otherwise it could be removed and continue passing almost all the time.

This adds a very awkward test-private method that attempts to detect whether a raw connection physically has a transaction open -- which is quite Not Great, but this seems important to get right.

Modern browsers don't render this HTML so it goes unused in practice. The delivered bytes are therefore a small waste (although very small) and unnecessary and could be optimized away. Additionally, the HTML fails validation. Using the W3C v.Nu, we see the following errors: Warning: Consider adding a lang attribute to the html start tag to declare the language of this document. Error: Start tag seen without seeing a doctype first. Expected <!DOCTYPE html>. Error: Element head is missing a required instance of child element title. These errors may surface in site-wide compliance tests (either internal tests or external contractual tests). Avoid the false positives by removing the HTML. While these warnings and errors could be resolved, it would be simpler on future maintenance to remove the body altogether (especially as it isn't rendered by the browser). As the same string is copied around a few places, this removes multiple touch points to resolve the current validation errors as well as new ones. Many other frameworks and web servers don't include an HTML body on redirect, so there isn't a reason for Rails to do so. By removing the custom Rails HTML, there are fewing "fingerprints" that a malicious bot could use to identify the backend technologies. Application controllers that wish to add a response body after calling redirect_to can continue to do so.

Remove body content from redirect responses

Ruby 2.6 added block argument processing to `Enumerable#to_h`. https://bugs.ruby-lang.org/issues/15143 Rails 7 requires Ruby 2.7.0 or higher, so the new feature can use it. `Style/MapToHash` cop will detect it. And this cop in the `Style` department, but this seems to improve performance as follows: ```ruby # map_to_hash.rb require 'benchmark/ips' ARRAY = (1..100).to_a HASH = {foo: 1, bar: 2} Benchmark.ips do |x| x.report('array.map.to_h') { ARRAY.map { |v| [v, v * 2] }.to_h } x.report('array.to_h') { ARRAY.to_h { |v| [v, v * 2] } } x.compare! end Benchmark.ips do |x| x.report('hash.map.to_h') { HASH.map { |k, v| [k.to_s, v * 2] }.to_h } x.report('hash.to_h') { HASH.to_h { |k, v| [k.to_s, v * 2] } } x.compare! end ``` ```console % ruby map_to_hash.rb Warming up -------------------------------------- array.map.to_h 9.063k i/100ms array.to_h 9.609k i/100ms Calculating ------------------------------------- array.map.to_h 89.063k (± 3.9%) i/s - 453.150k in 5.096572s array.to_h 96.449k (± 1.7%) i/s - 490.059k in 5.082529s Comparison: array.to_h: 96448.7 i/s array.map.to_h: 89063.4 i/s - 1.08x (± 0.00) slower Warming up -------------------------------------- hash.map.to_h 106.284k i/100ms hash.to_h 149.354k i/100ms Calculating ------------------------------------- hash.map.to_h 1.102M (± 2.2%) i/s - 5.527M in 5.019657s hash.to_h 1.490M (± 0.9%) i/s - 7.468M in 5.013264s Comparison: hash.to_h: 1489707.0 i/s hash.map.to_h: 1101561.5 i/s - 1.35x (± 0.00) slower ``` `Style/MapToHash` cop ... https://docs.rubocop.org/rubocop/1.25/cops_style.html#stylemaptohash

Bumps rubocop version to 1.25.1

Fix flaky Action View tests

…roller_doc Edit ActionController Base API docs [ci-skip]

…n-activemodel-testcases Fixed inconsistencies in ActiveModel test cases

Enable `Style/MapToHash` cop

Correct CSP initializer copy

…uide Add Permissions-Policy header to the security guide [ci-skip]

…if it is not able to

Return the blob/blobs when #attach is able to save the record

Switch from fork to queue_classic beta release

It's not a common operation, but still, no point running two queries when one will do.

This allows us to simulate an immediately-nested transaction without ever needing to begin or commit the child: those operations are no-ops, and we need only run a command if the child rolls back. Further, we can use a single query to implement that rollback: where the parent is a savepoint, the standard behaviour is to roll back to immediately _after_ the savepoint was set. Where the parent is a top-level transaction, most databases support ROLLBACK AND CHAIN.

Calling `skip_forgery_protection` without first calling `protect_from_forgery`--either manually or through default settings--raises an `ArgumentError` because `verify_authenticity_token` has not been defined as a callback. Since Rails 7.0 adds `skip_forgery_protection` to the `Rails::WelcomeController` (PR rails#42864), this behavior means that setting `default_protect_from_forgery` to false and visiting the Rails Welcome page (`/`) raises an error. This behavior also created an issue for `ActionMailbox` that was previously fixed in the Mailbox controller by running `skip_forgery_protection` only if `default_protect_from_forgery` was true (PR rails#35935). This PR addresses the underlying issue by setting the `raise` option for `skip_before_action` to default to false inside `skip_forgery_protection`. The fix is implemented in `request_forgery_protection.rb`. The change to `ActionMailbox`'s `base_controller.rb` removes the now-unnecessary check of `default_protect_from_forgery`. The tests added in `request_forgery_protection_test.rb` and `routing_test.rb` both raise an error when run against the current codebase and pass with the changes noted above.

Rollback nested transactions by restarting the parent where possible

Allow skip_forgery_protection if no protection set

jgigault reviewed Jan 6, 2020

View reviewed changes

frantisekrokusekpa self-assigned this Jan 7, 2020

frantisekrokusekpa added the ActiveStorage label Jan 7, 2020

Ispirett reviewed Nov 30, 2020

View reviewed changes

frantisekrokusekpa force-pushed the enable_custom_blob_key_configuration branch 2 times, most recently from 393ebf6 to 10a5a08 Compare December 18, 2020 22:21

frantisekrokusekpa commented Dec 18, 2020

View reviewed changes

activestorage/lib/active_storage/attached/model.rb Outdated Show resolved Hide resolved

jgigault suggested changes Dec 19, 2020

View reviewed changes

frantisekrokusekpa force-pushed the enable_custom_blob_key_configuration branch from 60e2f59 to c9c1bac Compare January 3, 2021 17:01

frantisekrokusekpa force-pushed the enable_custom_blob_key_configuration branch from 372a803 to d2a92a8 Compare January 4, 2021 13:44

frantisekrokusekpa force-pushed the enable_custom_blob_key_configuration branch from e912bbf to 80dfe91 Compare September 26, 2021 21:44

frantisekrokusekpa force-pushed the enable_custom_blob_key_configuration branch from 1892071 to 1ac0891 Compare November 26, 2021 16:41

kamipo and others added 13 commits February 9, 2022 17:33

Merge pull request rails#44337 from paulhenri-l/patch-1

135f41d

Fix empty request inside helpers test

Merge pull request rails#44365 from nvasilevski/increment-assertions-…

85c32e8

…on-assert-nothing-raised Increment assertions count on assert_nothing_raised

Merge pull request rails#44306 from volmer/attributes-docs

6146807

Documentation for Active Model Attributes [ci-skip]

Fix ruby 2.7 keyword args warning for postgresql reconnection_error test

2418596

Merge pull request rails#44381 from nvasilevski/fix-ruby27-keyword-ar…

0b6d625

…g-warning-in-test Fix ruby 2.7 keyword args warning for postgresql reconnection_error test

Merge pull request rails#44093 from motine/doc-hint-for-fields_for

b961af3

add hint to ActionView's fields_for

Update parameter filter logging guides [ci-skip]

e3beba1

The `filter_parameters` configuration includes a list of filters in the latest `filter_parameter_logging` initializer template. This updates the guides to reflect those changes.

Silence uninitialized instance var warning in tests

c9cb1aa

Running the Action Pack tests outputs a warning: ./actionpack/test/controller/test_case_test.rb:1007: warning: instance variable @counter not initialized Surrounding the line with silence_warnings cleans up the output.

Merge pull request rails#44388 from p8/guides/filter-parameters-default

e42e60e

Update parameter filter logging guides [ci-skip]

Fixes flaky test case in cache_store_behaviour

c8fde9a

Insert space before opening parenthesis [ci-skip]

0f36425

Follow-up to rails#44388.

rafaelfranca and others added 30 commits February 24, 2022 15:12

Merge pull request rails#44544 from jonathanhefner/active_model-access

e28972a

Add `ActiveModel::Access`

Use PG Connection#escape over Connection.escape

a2dc26e

Also fix AbstractMysqlAdapterTest

Merge pull request rails#44454 from jhawthorn/rack_logger_finish_with…

0a44c3f

…_state Use finish_with_state inside Rack::Logger

Merge pull request rails#44527 from matthewd/reset-prepared-statements

a96c5d4

The database server discards prepared statements when the client disconnects

Merge pull request rails#44530 from matthewd/rename-raw-connection-ivar

26caacb

Rename the raw connection ivar to `@raw_connection`

Ensure Contract#metadata is reliably orderable

8d04150

This gets an annoyingly verbose comment, because otherwise it could be removed and continue passing almost all the time.

Be clearer about when transactions are reset by reconnection

49299c4

This adds a very awkward test-private method that attempts to detect whether a raw connection physically has a transaction open -- which is quite Not Great, but this seems important to get right.

Merge pull request rails#44554 from jdufresne/redirect-to-body

9d65858

Remove body content from redirect responses

Merge pull request rails#44532 from ghousemohamed/bumb-rubocop-to-1.25.1

cefaf15

Bumps rubocop version to 1.25.1

Merge pull request rails#44529 from Shopify/actionview-fix-flaky-tests

b64cbc0

Fix flaky Action View tests

Merge pull request rails#44523 from SValkanov/apidocs_edit_actioncont…

eaccffa

…roller_doc Edit ActionController Base API docs [ci-skip]

Merge pull request rails#44501 from ghousemohamed/fix-inconsistency-i…

269037a

…n-activemodel-testcases Fixed inconsistencies in ActiveModel test cases

Merge pull request rails#44555 from koic/enable_style_map_to_hash_cop

201aecb

Enable `Style/MapToHash` cop

Merge pull request rails#44488 from p8/railties/csp-initializer-copy

fec53a1

Correct CSP initializer copy

Merge pull request rails#44512 from p8/guides/add-permission-policy-g…

d9dcaf7

…uide Add Permissions-Policy header to the security guide [ci-skip]

Return blob/blobs when #attach is able to save the record and return …

fbfb6e6

…if it is not able to

Merge pull request rails#44439 from ghousemohamed/patch-6

fb88db5

Return the blob/blobs when #attach is able to save the record

Merge PR rails#44361

8137932

Switch from fork to queue_classic beta release

e0edc1a

Merge pull request rails#44562 from matthewd/queue-classic-beta

cece807

Switch from fork to queue_classic beta release

Reformat Postgres version checks to reflect their structure

0e8f429

Save a query when starting an isolated txn in Postgres

73d7b5b

It's not a common operation, but still, no point running two queries when one will do.

Merge pull request rails#44526 from matthewd/restart-transaction

0dd8c08

Rollback nested transactions by restarting the parent where possible

Merge pull request rails#44537 from brtrick/fix-skip-forgery-protection

1cf3fde

Allow skip_forgery_protection if no protection set

Merge branch 'main' into enable_custom_blob_key_configuration

20d3481

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

enable custom Blob#key configuration #1

enable custom Blob#key configuration #1

Uh oh!

frantisekrokusekpa commented Dec 31, 2019 •

edited

Loading

Uh oh!

jgigault left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

jgigault Jan 6, 2020

Uh oh!

sedubois commented Mar 26, 2020

Uh oh!

Ispirett Nov 30, 2020

Uh oh!

frantisekrokusekpa Nov 30, 2020

Uh oh!

frantisekrokusekpa Jan 6, 2021

Uh oh!

Uh oh!

frantisekrokusekpa commented Dec 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sedubois commented Dec 25, 2020

Uh oh!

frantisekrokusekpa commented Jan 3, 2021

Uh oh!

frantisekrokusekpa commented Jan 3, 2021

Uh oh!

Uh oh!

		@@ -116,6 +116,14 @@ def create_before_direct_upload!(key: nil, filename:, byte_size:, checksum:, con
		def generate_unique_secure_token(length: MINIMUM_TOKEN_LENGTH)

enable custom Blob#key configuration #1

Are you sure you want to change the base?

enable custom Blob#key configuration #1

Uh oh!

Conversation

frantisekrokusekpa commented Dec 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Other Information

Uh oh!

jgigault left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jgigault Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

sedubois commented Mar 26, 2020

Uh oh!

Ispirett Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

frantisekrokusekpa Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

frantisekrokusekpa Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

frantisekrokusekpa commented Dec 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sedubois commented Dec 25, 2020

Uh oh!

frantisekrokusekpa commented Jan 3, 2021

Uh oh!

frantisekrokusekpa commented Jan 3, 2021

Uh oh!

Uh oh!

frantisekrokusekpa commented Dec 31, 2019 •

edited

Loading

jgigault left a comment •

edited

Loading