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

Problems with the validation of the include option #41

Open
christophersansone opened this issue Jan 6, 2020 · 3 comments · May be fixed by #60
Open

Problems with the validation of the include option #41

christophersansone opened this issue Jan 6, 2020 · 3 comments · May be fixed by #60
Milestone

Comments

@christophersansone
Copy link
Contributor

(Currently there is a bug that is fixed in both #13 and #32, but there is more to the story. Creating this issue to discuss further.)

The validate_includes! method is designed to check the include option prior to serialization to see if there are any items that do not actually have a relationship defined. For example:

class FooSerializer
  include FastJsonapi::ObjectSerializer
  attribute :name
end

FooSerializer.new(model, { include: :bar })

This example would not pass the validation, because it is attempting to include bar, which has not been defined as a relationship.

However, there are two problems with this:

  1. It cannot possibly validate relationships that have a dynamic serializer, such as polymorphic or a serializer Proc (Improved relationship serializer options #32). The best it can do is ignore these cases. Prior to the fixes in includes : disable static checks when using polymorphic relations #13 and Improved relationship serializer options #32, it would incorrectly fail the validation.

  2. This validation occurs for every single serialization. You can pass in the same set of includes 100K times, and it will still iterate through each item and validate it. This is extra work with performance implications for every serialization that occurs, for little benefit.

First, I think we should decide whether we want the serializer to fail in this situation. The alternative is to simply ignore any items in the list that do not apply.

If we want to ignore, then I believe all we need to do is remove this validation.

If we want it to fail, then I believe a better approach would simply be to fail when it actually attempts to serialize that included relationship, rather than to do a pre-flight validation. I think we can change get_included_records to fail when it encounters an invalid item. If we did it that way, then it would resolve the problems with the existing implementation: it could validate the polymorphic relationships with zero performance impact.

@stas
Copy link
Collaborator

stas commented Jan 7, 2020

Related Netflix/fast_jsonapi#401

It would be great to actually have a proper exception type to be able to handle the 400 requests when this exact issue occurs.

@christophersansone
Copy link
Contributor Author

I built a nice implementation of this yesterday. I want to write a suite of tests specifically for options[:include] and update the documentation before submitting the PR. Spoiler alert, the updated implementation also allows you to specify nested relationships as a hash, e.g. include: [ :aaa, { bbb: :ccc } ]. 😉

@JeffLuckett
Copy link

include: [ :aaa, { bbb: :ccc } ]

A familiar syntax, just like ActiveRecord. I like it.

@christophersansone christophersansone linked a pull request Feb 13, 2020 that will close this issue
@stas stas mentioned this issue Oct 23, 2020
3 tasks
@stas stas added this to the V3 milestone Oct 24, 2020
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 a pull request may close this issue.

3 participants