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

fix: more flexible polymorphic types lookup #1434

Merged
merged 8 commits into from
Jan 25, 2024

Conversation

bf4
Copy link
Collaborator

@bf4 bf4 commented Jan 19, 2024

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

All Submissions:

  • I've checked to ensure there aren't other open Pull Requests for the same update/change.
  • I've submitted a ticket for my issue if one did not already exist.
  • My submission passes all tests. (Please run the full test suite locally to cut down on noise from travis failures.)
  • I've used Github auto-closing keywords in the commit message or the description.
  • I've added/updated tests for this change.

New Feature Submissions:

  • I've submitted an issue that describes this feature, and received the go ahead from the maintainers.
  • My submission includes new tests.
  • My submission maintains compliance with JSON:API.

Bug fixes and Changes to Core Features:

  • I've included an explanation of what the changes do and why I'd like you to include them.
  • I've provided test(s) that fails without the change.

Test Plan:

Reviewer Checklist:

  • Maintains compliance with JSON:API
  • Adequate test coverage exists to prevent regressions

@@ -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)
Copy link
Collaborator Author

@bf4 bf4 Jan 22, 2024

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

Copy link
Collaborator Author

@bf4 bf4 Jan 22, 2024

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

@bf4 bf4 force-pushed the v-11-refactor_polymorphic_inference branch from af00515 to 30445f1 Compare January 24, 2024 15:16
# # klass.base_class may be nil
# warn "[POLYMORPHIC TYPE] #{__callee__} #{klass} #{ex.inspect}"
# nil
# end
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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!
Copy link
Collaborator Author

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}"
Copy link
Collaborator Author

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}"
Copy link
Collaborator Author

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.
@bf4 bf4 force-pushed the v-11-refactor_polymorphic_inference branch from 30445f1 to 93ac314 Compare January 24, 2024 18:17
@lgebhardt lgebhardt merged commit ae9017b into v0-11-dev Jan 25, 2024
39 checks passed
@lgebhardt lgebhardt deleted the v-11-refactor_polymorphic_inference branch January 25, 2024 20:29
lgebhardt pushed a commit that referenced this pull request Apr 18, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants