-
Notifications
You must be signed in to change notification settings - Fork 534
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
Conversation
Clean up the tests so as to not encourage overriding `find_records` instead of `records`.
There was a problem hiding this 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?
|
||
def include_optional_linkage_data? | ||
# :nocov: | ||
@always_include_optional_linkage_data || JSONAPI::configuration.always_include_to_many_linkage_data |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Let's deprecate |
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
5eab0c2
to
14b1a1e
Compare
@joegaudet, @dgeb Thanks for the reviews. I think I've got all the suggestions implemented or addressed, with the exception of deprecating the |
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 examplepictures.imageable#document.name
All Submissions:
New Feature Submissions:
Bug fixes and Changes to Core Features:
Test Plan:
Reviewer Checklist: