Skip to content

Conversation

lgebhardt
Copy link
Member

Deprecates and replaces the allow_include config option

Replaces allow_includes with default_allow_include_to_one and default_allow_include_to_many configuration options.
Adds support for allow_include option on Relationships to override the config settings.

Purpose:

  • Includes without pagination allows potentially resource intensive requests.
  • Pagination of includes is a non-trivial problem to solve efficiently with SQL databases
  • By allowing includes defaults to be set for to_one and to_many relationship types separately includes for the riskier to_many relationships can be disabled by default, while still allowing them on specific relationships if desired.

@lgebhardt lgebhardt requested review from dgeb and removed request for barelyknown July 3, 2018 14:42
"#{name}_type" if polymorphic?
end

def allow_include?
Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome, would be cool to have the context passed in, similarly to fetchable_fields useful to remove some depending on the current user.

Copy link
Member Author

Choose a reason for hiding this comment

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

@joegaudet Good idea

Replaces `allow_includes` with `default_allow_include_to_one` and `default_allow_include_to_many` configuration options.
Adds support for `allow_include` option on Relationships to override the config settings.
@lgebhardt
Copy link
Member Author

Updated for @joegaudet so you can now provide a boolean, a callable name, or a lambda for allow_include. Context is passed to the callable and lambda.

Copy link
Member

@dgeb dgeb left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Needs one simple fix. I'd also feel more comfortable if the recursion logic in check_include were tested.

@dgeb dgeb merged commit fb35962 into master Oct 31, 2018
@dgeb dgeb deleted the includes_config branch October 31, 2018 14:50
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