json_api_client icon indicating copy to clipboard operation
json_api_client copied to clipboard

[Feature] Use the association options to lookup relationship class

Open sebasjimenez10 opened this issue 3 years ago • 3 comments

Hey there,

I'm opening this PR because I noticed an issue with the way the association class types are being looked up when including related resources.

I discovered this when having cross-referenced types when using namespaces, like the example suggests:

module CrossNamespaceTwo
  class Nail < TestResource
    property :size
  end
end

module CrossNamespaceOne
  class Hammer < TestResource
    property :brand

    has_many :nails, class: CrossNamespaceTwo::Nail
  end
end

In this case whenever the IncludedData class was initializing an instance, the utils class would not be able to produce a correct type to use for the related included type producing an error similar to the following:

Finished in 0.447569s, 538.4645 runs/s, 1619.8620 assertions/s.

  1) Error:
AssociationTest#test_cross_namespace_resource_references:
NameError: uninitialized constant CrossNamespaceOne::Hammer::Nail

Since one should be able to add options to the association definition the class option makes sense here. This PR makes use of that to try and figure out the appropriate type before defaulting to the existing behavior.

Thanks in advance for taking a look!

sebasjimenez10 avatar Aug 11 '22 05:08 sebasjimenez10

Hey @gaorlov 👋 hope you can take a look at this one as well 🙂 thanks in advance!

sebasjimenez10 avatar Aug 11 '22 05:08 sebasjimenez10

@sebasjimenez10 thanks for the contribution! This looks very similar to the built in class_name functionality?


module CrossNamespaceOne
  class Hammer < TestResource
    property :brand

    has_many :nails, class_name: 'CrossNamespaceTwo::Nail'
  end
end

would that work for your usecase?

gaorlov avatar Aug 12 '22 21:08 gaorlov

@gaorlov thanks for the response! I went ahead and commented the changes I made to utils.rb and used class_name but I still got the:

NameError: uninitialized constant CrossNamespaceOne::Hammer::Nail

error.

Any ideas on why? I'm happy to try something else! 🙏

sebasjimenez10 avatar Aug 13 '22 00:08 sebasjimenez10

@gaorlov small bump on this one as well! 🙏

sebasjimenez10 avatar Oct 21 '22 21:10 sebasjimenez10

@gaorlov thanks for the reply! Merged in main into the branch and added the changelog updates!

sebasjimenez10 avatar Jun 06 '23 04:06 sebasjimenez10

@gaorlov small bump! 🙏

sebasjimenez10 avatar Sep 07 '23 21:09 sebasjimenez10

@sebasjimenez10 I'm on a trip right now, but i will merge and release a new version when I'm back next week. Thank you for your contributions and patience!

gaorlov avatar Sep 09 '23 18:09 gaorlov

@gaorlov small bump! we're almost there! 🙏

sebasjimenez10 avatar Nov 08 '23 06:11 sebasjimenez10

hello! Sorry about the delay! this is now available in 1.22.0 Thank you so much for your contribution!

gaorlov avatar Nov 22 '23 23:11 gaorlov