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

Always include to_one linkage #1218

Merged
merged 15 commits into from
Feb 11, 2019
Merged

Always include to_one linkage #1218

merged 15 commits into from
Feb 11, 2019

Conversation

lgebhardt
Copy link
Member

This PR reenables the always_include_to_one_linkage_data configuration option. This joins in the to_one relationship in order to retrieve the linkage data without additional database requests.

The path parsing logic for includes, filters and sorts was moved to a Path class that parses into Relationship and Field path parts.

Filtering is extended across polymorphic relationships.

Enables the a new join_left ponyfill to extend left_joins to rails 4 and works around an issue with nested joins not aliasing correctly on rails 5 and 5.1.

Streamlines the JoinTree and Joins handling of polymorphic relationships in paths. Filters can now specify the specific resource type of a polymorphic relation using the format relationshipname#resource_type. For example pictures.imageable#document.name

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

Copy link
Contributor

@joegaudet joegaudet left a comment

Choose a reason for hiding this comment

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

A few comments, largely looks good.

The polymorphic resource specification stuff is interesting, I take it not part of the JSONAPI spec though?

test/test_helper.rb Outdated Show resolved Hide resolved
lib/jsonapi/path_part.rb Outdated Show resolved Hide resolved
lib/jsonapi/path_part.rb Outdated Show resolved Hide resolved
lib/jsonapi/path.rb Outdated Show resolved Hide resolved
lib/jsonapi/path.rb Outdated Show resolved Hide resolved
lib/jsonapi/active_relation_resource_finder/join_tree.rb Outdated Show resolved Hide resolved

def include_optional_linkage_data?
# :nocov:
@always_include_optional_linkage_data || JSONAPI::configuration.always_include_to_many_linkage_data
Copy link
Contributor

Choose a reason for hiding this comment

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

So there's no way for me to say that the broad configuration is always included, but for one relationship do not? Not that I think this is a likely case, but a case nonetheless.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not supported for ActiveRecord whether set globally or per relationship. The code just doesn't exist yet, and it's going to potentially be expensive so I'm not planning to tackle it soon.
What do you think the best way to handle this in the relationship would be? Conceivably we could write a Mongo adapter that easily supports this. I want the Relationship to work with either Adapter. We could interrogate the Adapter for capabilities.

lib/jsonapi/resource.rb Outdated Show resolved Hide resolved
@dgeb
Copy link
Member

dgeb commented Feb 7, 2019

Let's deprecate Resource#find_records properly to discourage its use.

lib/jsonapi/resource.rb Outdated Show resolved Hide resolved
lib/jsonapi/resource.rb Outdated Show resolved Hide resolved
lib/jsonapi/path_part.rb Outdated Show resolved Hide resolved
Rails 4 is missing the left join functionality added to rails 5. In addition rails 5.0.x and 5.1.x produced erroneous SQL when joining the same table as both inner and left (see rails/rails#30504). JR is now generating joins like this for including linkages on related record queries and includes.
This patch add the logic from the left_join gem as a new ActiveRecord Adapter for rails 4.2 through 5.1.
raises an error if segment specifies a type that the relationship
does not support the resource type
@lgebhardt
Copy link
Member Author

@joegaudet, @dgeb Thanks for the reviews. I think I've got all the suggestions implemented or addressed, with the exception of deprecating the find_records method. I'm still looking for a good way to handle that so people who have it overridden can be notified they may need to change it. As it is now it will at least fail so I haven't renamed the method. Any suggestions?

@lgebhardt lgebhardt mentioned this pull request Feb 11, 2019
1 task
@lgebhardt lgebhardt merged commit c4242e3 into master Feb 11, 2019
@lgebhardt lgebhardt deleted the always_includes branch February 11, 2019 22:55
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.

3 participants