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

Pass serialization scope to lazy loaders #42

Closed
wants to merge 4 commits into from
Closed

Pass serialization scope to lazy loaders #42

wants to merge 4 commits into from

Conversation

Bajena
Copy link
Owner

@Bajena Bajena commented Dec 29, 2019

It doesn't necessarily have to get to master, but I've heard some voices that it might be useful to have access to the serialization scope.

closes #41

@Bajena Bajena requested a review from stokarenko January 8, 2020 15:24
Copy link
Collaborator

@stokarenko stokarenko left a comment

Choose a reason for hiding this comment

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

Don't have a strong opinion about this PR at the moment, please see the comments.

Also it contains a number of conflicts with #44 - could be process lazy_dig first please? ;)

serializer = serializer_for(batch_record, lrm.reflection.options)
return unless serializer

serializer.send(:load_all_lazy_relationships, batch_record, level + 1)
serializer.send(:load_all_lazy_relationships, batch_record, scope, level + 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably we need to take scope from nested serializer, something like

Suggested change
serializer.send(:load_all_lazy_relationships, batch_record, scope, level + 1)
serializer.send(:load_all_lazy_relationships, batch_record, serializer.scope, level + 1)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think that's possible, because AFAIK serializer here is the class not the instance.
Btw the scope is in most cases (I haven't seen other :P) provided by the Rails controller by calling e.g. serialization_scope :current_user, so this should be fine.


records.each do |r|
d = r.blog_posts
d = d.where(title: scope[:title]) if scope
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not very familiar with Serializer scopes at the moment, but looks like such kind of customized loader will play against idempotency (serializer with lazy_relationships will produce different output than pure serializer), nor will duplicate the business logic (how scope-basic logic has been applied to pure serializer, why that logic was not applied to lazy serializer out-from-the-box?)..

Copy link
Owner Author

@Bajena Bajena Jan 10, 2020

Choose a reason for hiding this comment

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

serializer with lazy_relationships will produce different output than pure serializer

Not necessarily - imagine you had a following use case (it's stupid I know, but couldn't imagine better ATM :D):

class BlogPostsController
  serialization_scope :current_user

  def index
     render json: BlogPost.all
  end
end

class BlogPostSerializer
  has_many :comments do
    current_user.premium? ? object.comments : object.comments.first
  end
end

if you want to rework it using lazy relationships you could do sth like the code below, but you still need to load ALL comments for each post and select the first one if current user is non-premium.

class BlogPostSerializer
  lazy_relationship :comments

  has_many :comments do
    # We still need to load ALL comments for non-premium users
    current_user.premium? ? lazy_comments : lazy_comments.first
  end
end

with scope passed to lazy loaders you can fetch just the first record already in the loader:

  class CommentsLoader < AmsLazyRelationships::Loaders::Base
      def load_data(records, loader, current_user)
        # load all comments for each post if user is premium, 
       # but only one for each post if user is non-premium
      end
  end

class BlogPostSerializer
  lazy_has_many :comments, loader: CommentsLoader
end

It should be more clear now. Did I explain it clearly enough?
I could probably add something to the documentation as well...

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Bajena got the idea, need some time to think about )

Copy link
Collaborator

Choose a reason for hiding this comment

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

The last concern that I feel is that such kind of conditional filtering is out from serializer's scope of responsibilities, as well as it is out from JSON API specification. Even more, each step deeper than straightforward rendering of plain AR association within serializer's includes will lead the team to the dreams about include-less serializers, eventually.

The need to filter association sounds so close to the need to paginate association, isn't? There is an example how that looks at the end

TLDR;
We did not implement includes.

After theory-crafting out scaling issues, it became apparent we would need to paginate includes if we returned them… BUT we would also want to have a mechanism for allowing to specify what “page” we are paginating each includes on and return them properly. This was a pain in the … well you know. It also came to our attention that if we did paginate the includes, some partners may assume the includes we would returning are all of them, not just a subset.

Because the spec does no specify how to do this completely, we decided not to implement includes. (includes are not required for spec as far as I can tell). This does mean partners must make more requests. But the up side: we can limit the number of queries per request to a manageable number and keep response times sub 100ms.

In general I agree that controller scope passed into serializer can help to deal with very special business cases, but that can be a marker of unlucky API design in the same time, leading to the similar We did not implement includes after the days..

@Bajena
Copy link
Owner Author

Bajena commented Jan 16, 2020

@stokarenko I'll move on with this PR as soon as your PRs are shipped :)

@Bajena
Copy link
Owner Author

Bajena commented Jan 28, 2020

@stokarenko I've rebased to master and added a section about custom loaders to README. Take a look if you have some time :)

Copy link
Collaborator

@stokarenko stokarenko left a comment

Choose a reason for hiding this comment

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

LGTM, but sill.

I would like to drop a hint in Readme that need to filter association appears to be bad idea most times, and much better to make one more API request to associated resources collection with possibility to specify any kind of filters and without the hacking of JSON API specification.

```

It should be more clear now. Did I explain it clearly enough?
I could probably add something to the documentation as well...
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks fine for me at least )
Perhaps we need to remove these two lines before the merge )


records.each do |r|
d = r.blog_posts
d = d.where(title: scope[:title]) if scope
Copy link
Collaborator

Choose a reason for hiding this comment

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

The last concern that I feel is that such kind of conditional filtering is out from serializer's scope of responsibilities, as well as it is out from JSON API specification. Even more, each step deeper than straightforward rendering of plain AR association within serializer's includes will lead the team to the dreams about include-less serializers, eventually.

The need to filter association sounds so close to the need to paginate association, isn't? There is an example how that looks at the end

TLDR;
We did not implement includes.

After theory-crafting out scaling issues, it became apparent we would need to paginate includes if we returned them… BUT we would also want to have a mechanism for allowing to specify what “page” we are paginating each includes on and return them properly. This was a pain in the … well you know. It also came to our attention that if we did paginate the includes, some partners may assume the includes we would returning are all of them, not just a subset.

Because the spec does no specify how to do this completely, we decided not to implement includes. (includes are not required for spec as far as I can tell). This does mean partners must make more requests. But the up side: we can limit the number of queries per request to a manageable number and keep response times sub 100ms.

In general I agree that controller scope passed into serializer can help to deal with very special business cases, but that can be a marker of unlucky API design in the same time, leading to the similar We did not implement includes after the days..

end

let(:level0_serializer_class) do
class Level1Serializer11 < BaseTestSerializer
Copy link
Collaborator

Choose a reason for hiding this comment

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

That should be Level1Serializer16 I believe )
I think will be good some day to refactor all the tests there, in order to isolate them better )

@Bajena Bajena closed this Apr 14, 2022
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.

Pass serialization scope to loaders
2 participants