Skip to content

Fix resource including for STI objects #349

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

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

stokarenko
Copy link
Contributor

The problem was introduced by #305

When we including the kind of STI resource, we are running in issue with unexpected type attribute:

Given: resource is kind of 'file' type, with 'type' attribute equals to STIDocumentFile
When: we including such resource
Expected: parent_resource.file to be available as usually
Actual: the exception raised

NoMethodError:
       undefined method `[]' for nil:NilClass
     # /gems/json_api_client-1.12.0/lib/json_api_client/included_data.rb:46:in `record_for'

This happens because of

module JsonApiClient
  class IncludedData
    def initialize(result_set, data)
       # ...
       # `parameters_from_resource` will override the actual resource `type` value
       # with value from attribute `type`
       params = klass.parser.parameters_from_resource(datum)
       # ...
       # such overridden `type` is used for grouping.
       # i.e. the resources will be grouped by `STIDocumentFile` instead of `file`
       @data = included_set.group_by(&:type)
       # ...
    end
  end
end

@stokarenko
Copy link
Contributor Author

@gaorlov FYI )

@stokarenko
Copy link
Contributor Author

Will fix the missing transform_values method on Monday )

@stokarenko stokarenko force-pushed the fix-included-data-for-sti branch from c4d8d1c to d28e104 Compare June 24, 2019 11:06
Copy link
Collaborator

@gaorlov gaorlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your contribution! I only have one comment:
This section of code had grown to be quite complex. I spent about an hour thinking of a good way to simplify it, but couldn't come up with anything meaningful, aside from a couple variable names suggestions which i left in comments.
Again, I super appreciate you taking the time to dive into all the complexities of this piece and contribute.

@stokarenko stokarenko force-pushed the fix-included-data-for-sti branch from d28e104 to c8995de Compare June 25, 2019 10:43
@stokarenko
Copy link
Contributor Author

@gaorlov Applied all the things, also added the record to changelog )

Thank you a lot for assistance! )

@gaorlov
Copy link
Collaborator

gaorlov commented Jun 25, 2019

Marvelous. Thanks once again!

@gaorlov gaorlov merged commit de379a5 into JsonApiClient:master Jun 25, 2019
@gaorlov
Copy link
Collaborator

gaorlov commented Jun 25, 2019

@stokarenko 1.12.1 is now live.

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