-
Notifications
You must be signed in to change notification settings - Fork 534
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
fix: more flexible polymorphic types lookup #1434
Conversation
@@ -17,14 +26,28 @@ def build_polymorphic_types_lookup | |||
{}.tap do |hash| | |||
ObjectSpace.each_object do |klass| | |||
next unless Module === klass | |||
if ActiveRecord::Base > klass | |||
is_active_record_inspectable = ActiveRecord::Base > klass | |||
is_active_record_inspectable &&= klass.respond_to?(:reflect_on_all_associations, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this we get errors in our test suite upgrading from 0.9 like
undefined method `reflect_on_all_associations' for #<refinement:ActiveRecord::Base@TestProf::Ext::ActiveRecordRefind>\n" +
klass.reflect_on_all_associations(:has_many).select { |r| r.options[:as] }.each do |reflection|\n" +
^^^^^^^^^^^^^^^^^^^^^^^^^^^^",
"/app/.bundle/vendor/ruby/3.1.0/bundler/gems/jsonapi-resources-76ef777fd012/lib/jsonapi/utils/polymorphic_types_lookup.rb:21:in `block (2 levels) in build_polymorphic_types_lookup'",
"/app/.bundle/vendor/ruby/3.1.0/bundler/gems/jsonapi-resources-76ef777fd012/lib/jsonapi/utils/polymorphic_types_lookup.rb:18:in `each_object'",
"/app/.bundle/vendor/ruby/3.1.0/bundler/gems/jsonapi-resources-76ef777fd012/lib/jsonapi/utils/polymorphic_types_lookup.rb:18:in `block in build_polymorphic_types_lookup'",
and
undefined method `underscore' for nil:NilClass\n" +
(hash[reflection.options[:as]] ||= []) << klass.name.underscore\n" +
^^^^^^^^^^^",
"/app/.bundle/vendor/ruby/3.1.0/bundler/gems/jsonapi-resources-76ef777fd012/lib/jsonapi/utils/polymorphic_types_lookup.rb:22:in `block (3 levels) in build_polymorphic_types_lookup'",
"/app/.bundle/vendor/ruby/3.1.0/bundler/gems/jsonapi-resources-76ef777fd012/lib/jsonapi/utils/polymorphic_types_lookup.rb:21:in `each'",
"/app/.bundle/vendor/ruby/3.1.0/bundler/gems/jsonapi-resources-76ef777fd012/lib/jsonapi/utils/polymorphic_types_lookup.rb:21:in `block (2 levels) in build_polymorphic_types_lookup'",
"/app/.bundle/vendor/ruby/3.1.0/bundler/gems/jsonapi-resources-76ef777fd012/lib/jsonapi/utils/polymorphic_types_lookup.rb:18:in `each_object'",
"/app/.bundle/vendor/ruby/3.1.0/bundler/gems/jsonapi-resources-76ef777fd012/lib/jsonapi/utils/polymorphic_types_lookup.rb:18:in `block in build_polymorphic_types_lookup'",
where klass.inspect
is #<#<Class:TenderJobScheduleShift(
. Interestingly, klass.superclass
seems to work here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notably #1045 there was a change to inferring of class_name
which I noted in 0.10
I haven't confirmed yet if it helps in 0.11
I wrote this override in our 0.10 test branch
# since https://github.com/cerebris/jsonapi-resources/pull/1045/files
def define_relationship_methods(relationship_name, relationship_klass, options)
if !options.key?(:class_name)
# Initialize from an ActiveRecord model's properties
if _model_class && _model_class.ancestors.collect { |ancestor| ancestor.name }.include?("ActiveRecord::Base")
model_association = _model_class.reflect_on_association(relationship_name)
if model_association
reflected_class_name = model_association.class_name
if reflected_class_name.underscore.downcase.singularize != relationship_name.to_s.singularize
options[:class_name] = reflected_class_name
# warn "[JR-TEST] Added resource #{name} relationship #{relationship_name} option class_name=#{options[:class_name]}"
end
end
end
end
super(relationship_name, relationship_klass, options)
end
af00515
to
30445f1
Compare
# # klass.base_class may be nil | ||
# warn "[POLYMORPHIC TYPE] #{__callee__} #{klass} #{ex.inspect}" | ||
# nil | ||
# end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
# is_active_record_inspectable = true | ||
# is_active_record_inspectable &&= klass.respond_to?(:reflect_on_all_associations, true) | ||
# is_active_record_inspectable &&= format_polymorphic_klass_type(klass).present? | ||
# return unless is_active_record_inspectable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
polymorphic_types_lookup[name.to_sym] | ||
singleton_class.attr_accessor :build_polymorphic_types_lookup_strategy | ||
self.build_polymorphic_types_lookup_strategy = | ||
:build_polymorphic_types_lookup_from_object_space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
@@ -19,6 +19,10 @@ class Railtie < ::Rails::Railtie | |||
JSONAPI::MimeTypes.parser.call(body) | |||
} | |||
end | |||
|
|||
initializer "jsonapi_resources.initialize", after: :initialize do | |||
JSONAPI::Utils::PolymorphicTypesLookup.polymorphic_types_lookup_clear! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
@@ -193,7 +193,7 @@ def polymorphic_type | |||
def setup_implicit_relationships_for_polymorphic_types(exclude_linkage_data: true) | |||
types = self.class.polymorphic_types(_relation_name) | |||
unless types.present? | |||
warn "No polymorphic types found for #{parent_resource.name} #{_relation_name}" | |||
warn "[POLYMORPHIC TYPE] No polymorphic types found for #{parent_resource.name} #{_relation_name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
|
||
def handle_polymorphic_type_name_found | ||
@handle_polymorphic_type_name_found ||= lambda do |name| | ||
warn "[POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for #{name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
they pass on v-11-dev I'm going to look into the existing lookup warnings now ``` [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for fileable [POLYMORPHIC TYPE] No polymorphic types found for FilePropertiesResource fileable [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for respondent [POLYMORPHIC TYPE] No polymorphic types found for QuestionResource respondent [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for respondent [POLYMORPHIC TYPE] No polymorphic types found for AnswerResource respondent [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for keepable [POLYMORPHIC TYPE] No polymorphic types found for KeeperResource keepable ```
This reverts commit 0979a7243b6bc816dd2327d3ff23f70209c52dce.
30445f1
to
93ac314
Compare
* fix: more flexible polymorphic types lookup * test: add polymorphic lookup tests they pass on v-11-dev I'm going to look into the existing lookup warnings now ``` [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for fileable [POLYMORPHIC TYPE] No polymorphic types found for FilePropertiesResource fileable [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for respondent [POLYMORPHIC TYPE] No polymorphic types found for QuestionResource respondent [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for respondent [POLYMORPHIC TYPE] No polymorphic types found for AnswerResource respondent [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for keepable [POLYMORPHIC TYPE] No polymorphic types found for KeeperResource keepable ``` * Revert "test: add polymorphic lookup tests" This reverts commit 0979a7243b6bc816dd2327d3ff23f70209c52dce. * feat: easily clear the lookup * feat: add a descendents strategy * test: polymorphic type lookup * feat: make polymorphic type lookup configurable * feat: clear polymorphic lookup after initialize
they pass on v-11-dev
I'm going to look into the existing lookup warnings
now
All Submissions:
New Feature Submissions:
Bug fixes and Changes to Core Features:
Test Plan:
Reviewer Checklist: