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

Support passing of arbitrary options to the serializer #234

Closed
marcelotto opened this issue Mar 23, 2017 · 2 comments
Closed

Support passing of arbitrary options to the serializer #234

marcelotto opened this issue Mar 23, 2017 · 2 comments

Comments

@marcelotto
Copy link

First, thank you very much for this great library.

I've already commented on issue #214 about this, but wanted to make this a separate issue, since it is a serious hurdle for us now with both the mentioned problems of pagination of nested resources and the double calling of custom methods for included relationships.

I also think this indicates a more general problem: A serializer is a MVC-view and as we all know should not contain any logic except presentation-related logic. Arbitrary options would better support such logic-less views, by allowing it to put this logic into the controller where it belongs.

Custom attribute or relationship methods are not suitable for performing non-view logic esp. db queries, as they are performed twice in the case of included relationships (which might deserve a separate issue, but is IMO just a corollary issue to this one, and besides that more of a documentation problem).

In fact, JaSerializer actively promotes putting non-view logic into a serializer, with messages like that:

** (JaSerializer.AssociationNotLoadedError) The sub_events relationship returned %Ecto.Association.NotLoaded{}.

Please pre-fetch the relationship before serialization or override the
sub_events/2 function in your serializer.

Example:

   def sub_events(struct, conn) do
     case struct.sub_events do
       %Ecto.Association.NotLoaded{} ->
         struct
         |> Ecto.assoc(:sub_events)
         |> Repo.all
       other -> other
     end
   end

@Cohedrin's suggestion seems a good starting point:

# controller 
def show do
  json(conn, JaSerializer.format(Serializer, data, conn, instance_opts: %{comments: [] }))
end


# Serializer 
defmodule MyApp.Serializer do
  attributes [:id]

  has_many :comments, ...
   
  def comments(post, conn, instance_opts) do
    # pagination logic here
    # comments = instance_opts.comments
  end
end

Do you see any serious hurdles to support this?

As I've just noticed, issue #228 seems to request something similar.

@alanpeabody
Copy link
Contributor

Thanks for the feedback @marcelotto. At this time this is not a use case I have (or expect to have).

You are welcome to open a PR, however if it is not backwards compatible it will not be accepted at this time.

@beerlington
Copy link
Contributor

Closing this issue since I don't think it's something we're going to support any time soon. I totally agree with the comments about not encouraging the serializer to be responsible for data loading, and think this should be done at a higher level (i.e. context or controller) to avoid performance issues. I'm not convinced that adding instance opts is the way to solve it but would like to update the docs with info on how to handle this case.

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

No branches or pull requests

3 participants