vt-elixir / ja_serializer

JSONAPI.org Serialization in Elixir.
Other
640 stars 148 forks source link

Type ignored on relationship definition #342

Open elvanja opened 2 years ago

elvanja commented 2 years ago

Hi!

So I am trying to reuse an existing serializer for a relation that has a different (include) name/type. For example:

defmodule AuthorView do
  has_many :articles,
    serializer: ArticleView,
    link: :articles_url

  has_one :pinned_article,
    serializer: ArticleView,
    link: :pinned_article_url,
    type: :pinned_article
end

defmodule ArticleView do
end

But, the type: :pinned_article is ignored. Relation always gets rendered as:

"pinned-article" => %{
 "data" => %{
   "id" => payment.id,
   "type" => "article"
 }
}

And the same for included relation/object as well. The type remains article, instead of pinned-article. The main reason for reusing is to not have to repeat the additional logic ArticleView, e.g. how it sets the self URL, some attributes etc.

Any ideas on how to solve this one? Thank you 🙇🏼

elvanja commented 2 years ago

Looking around the code, found that type is indeed something that can be defined on HasOne relation definition. But, find_type in ResourceIdentifier ignores it. Thought of maybe changing the resource identifier to do:

defp find_type(_data, _context, %{type: type}) when type != nil, do: type
# and then the rest of current implementation

This would work, but not sure how (yet) it would affect the rest of the system. And I have no solution for rendering type in included object. As far as I can tell, ResourceObject also ignores the type in relationship definition.

elvanja commented 2 years ago

I have a couple solution that work:

Duplicate logic in another view

defmodule AuthorView do
  has_many :articles,
    serializer: ArticleView,
    link: :articles_url

  has_one :pinned_article,
    serializer: PinnedArticleView,
    link: :pinned_article_url
end

defmodule ArticleView do
end

defmodule PinnedArticleView do
end

This works, but requires duplication of logic between those two views. I mean, I can get around duplication via use CommonStuff or something similar, but it is a bit ugly.

Handle type in ArticleView

defmodule AuthorView do
  has_many :articles,
    serializer: ArticleView,
    link: :articles_url

  has_one :pinned_article,
    serializer: PinnedArticleView,
    link: :pinned_article_url
end

defmodule ArticleView do
  def type(_, conn) do
    if conn.request_path =~ "pinned" do
      "pinned-article"
    else
      "article"
    end
  end
end

This one introduces coupling to paths used, so not a good idea either.

beerlington commented 2 years ago

I agree that what you're trying to do make sense and it does seem like specifying the type in the parent serializer should take precedence. Having said that, this is not a use case I've run into before.

I'm curious why the pinned article needs to be a different type though. Based on your example, it seems like a pinned article is the same thing as a regular article, but has a specific relationship to the author. What is the rationale for needing to use a different type for the pinned article? If it truly is a different type, maybe it makes sense to have two different views as you proposed in your first solution.

elvanja commented 2 years ago

Hmmm, good point! I assumed that relationship name determines type in a way. Mostly because if felt odd to have to search for included article even though relation was pinned-article. But you are correct. Lookup in included should rely on type and ID from relation section, so it is OK to have it be different from relation name. OK, that also solves the problem, thank you 😄 Feel free to close this or leave open, given how type can still be defined on parent serializer (maybe that does not make sense?).

beerlington commented 2 years ago

After thinking about it a bit more, I think the code is correct as-is, but the docs need to be updated. It needs to be clear that you can either specify the type when not using a serializer OR specify the type in the serializer. I can't think of a use case where you'd need to do both. I will close after I get a chance to update the docs.