JsonApiClient / json_api_client

Build client libraries compliant with specification defined by jsonapi.org
MIT License
362 stars 186 forks source link

feat: Use the association options to lookup relationship class #403

Closed sebasjimenez10 closed 11 months ago

sebasjimenez10 commented 2 years ago

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 commented 2 years ago

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

gaorlov commented 2 years ago

@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?

sebasjimenez10 commented 2 years ago

@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 commented 2 years ago

@gaorlov small bump on this one as well! 🙏

sebasjimenez10 commented 1 year ago

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

sebasjimenez10 commented 1 year ago

@gaorlov small bump! 🙏

gaorlov commented 1 year ago

@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!

sebasjimenez10 commented 12 months ago

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

gaorlov commented 11 months ago

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