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

ActiveStorage reflection #33018

Merged
merged 3 commits into from Jun 1, 2018
Merged

ActiveStorage reflection #33018

merged 3 commits into from Jun 1, 2018

Conversation

@kddnewton
Copy link
Contributor

@kddnewton kddnewton commented May 29, 2018

Add the ability to query for the attachments that have been defined on a model through the ActiveRecord::Base::defined_attachments method.

Useful for writing things on top of ActiveStorage that can loop through models and their attachments to perform things in bulk.

@rails-bot
Copy link

@rails-bot rails-bot commented May 29, 2018

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented May 29, 2018

I'm fine with a reflection API but we should follow the same interface and similar implementation of the association and aggregation reflection API. https://github.com/rails/rails/blob/375a4143cf5caeb6159b338be824903edfd62836/activerecord/lib/active_record/reflection.rb

@kddnewton kddnewton changed the title ActiveRecord::Base::defined_attachments ActiveStorage reflection May 30, 2018
@kddnewton
Copy link
Contributor Author

@kddnewton kddnewton commented May 30, 2018

@rafaelfranca I updated it to hook into ActiveRecord::Reflection, let me know what you think

@@ -1038,5 +1039,7 @@ def aliased_table

def all_includes; yield; end
end

ActiveSupport.run_load_hooks(:active_record_reflection, self)
Copy link
Member

@rafaelfranca rafaelfranca May 30, 2018

I think we can do this without adding a new load hook to the public API. The class_attribute should be added to the ActiveRecord::Base class. The ReflectionExtensions can be added directly to ActiveRecord::Reflection without need a on_load hook.

Copy link
Contributor Author

@kddnewton kddnewton May 30, 2018

Sure that makes sense. I was just trying to keep the ActiveStorage stuff to itself, but happy to move it over.

# Holds all the metadata about a has_one_attached attachment as it was
# specified in the Active Record class.
class HasOneAttachedReflection <
::ActiveRecord::Reflection::MacroReflection #:nodoc:
Copy link
Member

@rafaelfranca rafaelfranca May 30, 2018

Why you need the :: here? Also keep everything in the same line.

Copy link
Contributor Author

@kddnewton kddnewton May 30, 2018

Top level constant just for the milliseconds it would shave off the constant lookup - I'll remove

Add the ability to reflect on the attachments that have been defined using ActiveRecord::Reflection.
@kddnewton
Copy link
Contributor Author

@kddnewton kddnewton commented May 30, 2018

@rafaelfranca updated once more

Copy link
Member

@rafaelfranca rafaelfranca left a comment

I ok with this current implementation. Even that those reflections can only be added once you have active storage loaded I think it is fine to have the reflections always on Active Record.

I'd prefer to have it totally separated though, and it is possible to do without introducing new on_load hooks.

I'll check if other member of the team before implementing it.

@georgeclaghorn @matthewd options?

@georgeclaghorn
Copy link
Contributor

@georgeclaghorn georgeclaghorn commented May 30, 2018

I’m okay with this implementation, too, but I’d prefer for it to live in Active Storage.

@kddnewton
Copy link
Contributor Author

@kddnewton kddnewton commented May 31, 2018

I'd be happy to refactor, just wasn't sure how to hook it in up the engine to ensure ActiveRecord::Reflection had been loaded without adding an on_load hook to ActiveSupport. Any ideas? Honestly I can probably just put it into the on_load for :active_record, I'll bet that would work.

@kddnewton
Copy link
Contributor Author

@kddnewton kddnewton commented May 31, 2018

Okay so refactored and this works. I can move these modules into ActiveRecord or leave them here, let me know which is preferable.

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Yeah, now it is better

end
end

module ReflectionExtension
Copy link
Member

@rafaelfranca rafaelfranca May 31, 2018

I think this one should be :nodoc: like all methods in Reflection are.

end

module ReflectionExtension
def reflection_class_for(macro)
Copy link
Member

@rafaelfranca rafaelfranca May 31, 2018

This method should be private

@kddnewton
Copy link
Contributor Author

@kddnewton kddnewton commented Jun 1, 2018

@rafaelfranca updated!

@rafaelfranca rafaelfranca merged commit c184447 into rails:master Jun 1, 2018
2 checks passed
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jun 1, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants