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.

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

Adds support for defined variants and generic support for attachment configuration #35290

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@abhchand
Copy link

commented Feb 15, 2019

This feature was originally proposed on the rails-core mailing list in November 2018.

The original post contains the justification for why this change would add value and was reviewed by @kaspth and @georgeclaghorn

Overview

This change allows variants to be named and defined as a fixed list of transformations to apply:

class User < ApplicationRecord
  has_one_attached :avatar do |attachment|
    attachment.has_variant :thumbnail, resize: "50x50"
  end

  has_many_attached :photos do |attachment|
    attachment.has_variant :rotated, rotated: "-90"
  end
end

Variants can then be generated by name:

user.avatar.variant(:thumbnail)

user.photos.first.variant(:rotated)

NOTE: As a byproduct of this implementation, this change also allows the has_*_attached methods to accept a &block. The block yields a configurable attachment object which can be used to configure this and any other future features as well.

Implementation

How it works today

When @user.avatar.variant(foo: "bar") is called, activestorage handles it as follows

@user model
  |
  |-> Calls `.avatar` attachment (`ActiveStorage::Attached::One`)
    |
    |-> Delegates `.variant()` to `.attachment` (`ActiveStorage::Attachment`)
      |
      |-> Delegates `.variant()` to `.blob` (`ActiveStorage::Blob`)
        |
        |-> Delegates `.variant()` to `Representable` concern (`ActiveStorage::Blob::Representable`)

Updated Implementation

Firstly, this commit allows has_one_attached and has_many_attached to receive a &block argument. It yields to the block a new configuration object (ActiveStorage::Attached::Configuration) which just a simple object that extends Hash with setter methods (like has_variant())

This new configuration is stored on the attachment (::One and ::Many objects)

Secondly, this change "intercepts" the delegated flow above by adding a new variant() method to the Attachment model. This new variant method translates a variant name (e.g. :thumbnail) into the hash of transformations it maps to (e.g resize: "100x100") and then forwards the call on to .blob

NOTE: This "intercepting method" has to be on attachment because in the case of many attachments, it needs to support

user.photos[0].variant(...)

where user.photos[0] returns an Attachment model object.

Notes

  • Add tests for configuring and generating defined variants, for both one and many attachments
  • rubocop detected no errors
  • Updated activestorage README
  • Updated activestorage rails guide
  • Updated activestorage CHANGELOG
Adds support for defined variants and generic support for attachment …
…configuration.

This change allows variants to be named and defined as a fixed list of transformations to apply:

    class User < ApplicationRecord
      has_one_attached :avatar do |attachment|
        attachment.has_variant :thumbnail, resize: "50x50"
      end

      has_many_attached :photos do |attachment|
        attachment.has_variant :rotated, rotated: "-90"
      end
    end

Variants can then be generated by name:

    user.avatar.variant(:thumbnail)

    user.photos.first.variant(:rotated)

As a byproduct of this implementation, this change also allows the `has_*_attached` methods to accept a `&block`. The block yields a configurable object (`ActiveStorage::Attached::Configuration`) which can be used to configure this and any future features as well.
@abhchand
Copy link
Author

left a comment

Hi @kaspth , @georgeclaghorn 👋

Just added this change based on our previous discussion here.

This is my first change to the Rails codebase, so I double checked that it conforms to the rails contribution guidelines.

I'm happy to make any changes based on your feedback. Really appreciate your time in reviewing in this.

Thanks!

user.avatar.variant(:thumbnail)
```

*Abhishek Chandrasekhar*

This comment has been minimized.

Copy link
@abhchand

abhchand Feb 15, 2019

Author

Kept the changelog entry short by not adding an example for configuring has_many_attached.

<%# Will generate a service url based on the `:thumbnail` transformations defined above %>
<%= image_tag user.avatar.variant(:thumbnail) %>
```

This comment has been minimized.

Copy link
@abhchand

abhchand Feb 15, 2019

Author

Again, keeping a short example for the README.

end

blob.variant(transformations)
end

This comment has been minimized.

Copy link
@abhchand

abhchand Feb 15, 2019

Author

Here is the "intercepting method" outlined in the Pull Request description.

  1. If arg is a Hash, it is a list of transformations and it gets delegated to blob.variant
  2. If arg is not a Hash, it is a named variant. Ask the record attachment if it knows about a variant with this name
    • If it does, the attachment returns the transformations and it gets delegated to blob.variant
    • If it doesn't, it raises an error

I wanted to clarify two decisions here:

Why is an error raised?

I was debating how to handle the case where someone invokes variant with an undefined name:

user.avatar.variant(:some_bad_name)
# => ???
  • We can't call blob.variant with nil, we get NoMethodError Exception: undefined method each_with_object' for nil:NilClass`
  • We can't call blob.variant() with a {} placeholder because this does generate a new variant with a new service path. It seems wrong to me that an invalid variant name should result in actual changes on the disk/service when processed.

In light of that, I chose to raise a more descriptive error and message: ActiveStorage::UndefinedVariant. Please let me know if another option is preferred, I'd be happy to change it!

Finding a variant by name

The pull request description describes why this variant() method was added to the Attachment model.

As a result of that, it needs to access the .#{attachment} on the original record, so it calls record.avatar (more generically record.send(name)). While it is a bit circular, this correctly does not perform a new DB query/lookup since ActiveStorage defines the associations as reflective and the :record association would be loaded already.

Small Note: It could technically result in a lookup if someone queries the Attachment record directly and calls variant() on it. This is an edge case and even the ActiveStorage documentation emphasizes that end users should not really be working with these models directly anyway.

@name, @record = name, record
def initialize(name, record, configuration)
@name, @record, @configuration = name, record, configuration
end

This comment has been minimized.

Copy link
@abhchand

abhchand Feb 15, 2019

Author

Store the user defined configuration on the attachment

end

def find_variant_by_name(variant_name)
@configuration[:defined_variants][variant_name.to_sym]
end

This comment has been minimized.

Copy link
@abhchand

abhchand Feb 15, 2019

Author

Convenience/Readability method to find a defined variant. Emphasizes consistently storing and looking up names as Symobls

@@ -0,0 +1,23 @@
# frozen_string_literal: true

This comment has been minimized.

Copy link
@abhchand

abhchand Feb 15, 2019

Author

This is the new configuration object that's yielded to the block. It's really just a ruby Hash that has setter/configuration methods on it.

As mentioned in the PR description, this can be added to over time as ActiveStorage builds out other features that allow for configuration (if any)

has_many_attached :vlogs, dependent: false
has_many_attached :vlogs, dependent: false do |attachable|
attachable.has_variant :thumbnail, resize: "100x100"
end

This comment has been minimized.

Copy link
@abhchand

abhchand Feb 15, 2019

Author

Modified the default fixtures to ensure some are configured with defined variants

@abhchand

This comment has been minimized.

Copy link
Author

commented Feb 28, 2019

@kaspth , @georgeclaghorn - Just following up on this PR to see if it can be reviewed. Thanks!

@kaspth
Copy link
Member

left a comment

Think this is a fine first code draft, but we definitely need to reconsider how the transformations get carried from the has_one_x definition down to the eventual attachment that gets called variant on.

```ruby
class User < ApplicationRecord
has_one_attached :avatar do |attachment|
attachment.has_variant :thumbnail, resize: "50x50"

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 4, 2019

Member

I don't like the has_ prefix here. Don't see the point in it.

This comment has been minimized.

Copy link
@abhchand

abhchand Mar 6, 2019

Author

Noted. Original thought was to match the readable style of has_one_*.

Will remove ✂️

@@ -41,7 +61,6 @@ def purge_dependent_blob_later
blob&.purge_later if dependent == :purge_later
end


This comment has been minimized.

Copy link
@kaspth

kaspth Mar 4, 2019

Member

Keep this.

This comment has been minimized.

Copy link
@abhchand

abhchand Mar 6, 2019

Author

Restored


module ActiveStorage
# Provides a configuration object use to configure the attachment
class Attached::Configuration < Hash

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 4, 2019

Member

This is overkill at this point, we don't want to expose a complete hash that people can mangle at will.

How about?

def has_one_attached(name, &block)
  variants = VariantsCollector.collect(&block)
end

# Replace Configuration
class VariantsCollector # :nodoc:
  def self.collect(&block)
    {}.tap do |variants|
      yield new(variants) if block_given?
    end
  end

  def initialize(variants)
    @variants = variants
  end
  
  def variant(name, **transformations)
    @variants[name] = transformations
  end
end

This comment has been minimized.

Copy link
@abhchand

abhchand Mar 6, 2019

Author

Noted 😄

I like the approach 👍 . I'll update this PR to use that.

Depending on the feedback from the other comment about naming I can also update this to TransformationsCollector or similar.

#
# class User < ActiveRecord::Base
# has_one_attached :avatar do |attachment|
# attachment.has_variant :thumbnail, resize: "50x50"

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 4, 2019

Member

Missing an end.

@@ -12,6 +12,15 @@ module Attached::Model
# has_one_attached :avatar
# end
#
# Optionaly accepts a block specifying the configuration. See

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 4, 2019

Member

Optionally.

# the variant has not been defined.
def variant(arg)
transformations =
arg.is_a?(Hash) ? arg : record.send(name).find_variant_by_name(arg)

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 4, 2019

Member

So let's say user.avatar.variant(:thumbnail).

user.avatar returns Attached::One, variant is delegated to attachment and now we're calling back to Attached::One here with send + find_variant_by_name? Something seems odd about this delegation of responsibilities.

I can see why it's tough because of Attached::Many delegating missing methods to attachments…

This comment has been minimized.

Copy link
@abhchand

abhchand Mar 6, 2019

Author

Agreed, it's definitely circular. I really just couldn't think of a better way to handle it for Attached::Many.

I had some ideas around implementing a ::OneOfMany object that would be 1:1 with Attachment but it was either too complex or broke other functionality.

Open to any ideas you or anyone else has on this.

@name, @record, @configuration = name, record, configuration
end

def find_variant_by_name(variant_name)

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 4, 2019

Member

I keep going back and forth on whether or not we should just call this (and the internal language) transformations_for(key) since they aren't full variants. Either case this should be # :nodoc: so it's private API.

This comment has been minimized.

Copy link
@abhchand

abhchand Mar 6, 2019

Author

Added # :nodoc:

To be honest I struggled with the naming of the internal language before finally settling on "defined variant" to indicate a pre-defined mapping of a name to a list of transformations. So this could just be renamed find_defined_variant_by_name() which would make it consistent with other internal naming and hopefully more clearer.

Alternatively, I can re-brand the internal language to "transformations" if you prefer that?

@abhchand
Copy link
Author

left a comment

@kaspth thanks for reviewing this!

I noted down for the next commit all the minor spelling and formatting fixes as well as your suggestion to use aCollector class.

Two larger issues seem to be

  1. Internal naming/language - sticking with "Defined Variants" or using something like "Transformations"

  2. The circular delegation pattern introduced by Attachment#variant()

More on those in comments below.

Would love your thoughts on that and I'd be happy to incorporate them and add a new commit for review.

Thanks!

```ruby
class User < ApplicationRecord
has_one_attached :avatar do |attachment|
attachment.has_variant :thumbnail, resize: "50x50"

This comment has been minimized.

Copy link
@abhchand

abhchand Mar 6, 2019

Author

Noted. Original thought was to match the readable style of has_one_*.

Will remove ✂️

@@ -41,7 +61,6 @@ def purge_dependent_blob_later
blob&.purge_later if dependent == :purge_later
end


This comment has been minimized.

Copy link
@abhchand

abhchand Mar 6, 2019

Author

Restored

@name, @record, @configuration = name, record, configuration
end

def find_variant_by_name(variant_name)

This comment has been minimized.

Copy link
@abhchand

abhchand Mar 6, 2019

Author

Added # :nodoc:

To be honest I struggled with the naming of the internal language before finally settling on "defined variant" to indicate a pre-defined mapping of a name to a list of transformations. So this could just be renamed find_defined_variant_by_name() which would make it consistent with other internal naming and hopefully more clearer.

Alternatively, I can re-brand the internal language to "transformations" if you prefer that?


module ActiveStorage
# Provides a configuration object use to configure the attachment
class Attached::Configuration < Hash

This comment has been minimized.

Copy link
@abhchand

abhchand Mar 6, 2019

Author

Noted 😄

I like the approach 👍 . I'll update this PR to use that.

Depending on the feedback from the other comment about naming I can also update this to TransformationsCollector or similar.

# the variant has not been defined.
def variant(arg)
transformations =
arg.is_a?(Hash) ? arg : record.send(name).find_variant_by_name(arg)

This comment has been minimized.

Copy link
@abhchand

abhchand Mar 6, 2019

Author

Agreed, it's definitely circular. I really just couldn't think of a better way to handle it for Attached::Many.

I had some ideas around implementing a ::OneOfMany object that would be 1:1 with Attachment but it was either too complex or broke other functionality.

Open to any ideas you or anyone else has on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.