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

Type ignored on relationship definition #342

Open
elvanja opened this issue Jul 1, 2022 · 5 comments
Open

Type ignored on relationship definition #342

elvanja opened this issue Jul 1, 2022 · 5 comments

Comments

@elvanja
Copy link

elvanja commented Jul 1, 2022

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
Copy link
Author

elvanja commented Jul 1, 2022

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
Copy link
Author

elvanja commented Jul 1, 2022

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
Copy link
Contributor

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
Copy link
Author

elvanja commented Jul 1, 2022

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
Copy link
Contributor

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.

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

2 participants