Fix nesting for includes #87
Conversation
|
cc @doomspork if I could get a second set of eyes on this |
|
@jeregrine I'll take a peek today 👍 |
lib/jsonapi/serializer.ex
Outdated
| @@ -83,7 +83,14 @@ defmodule JSONAPI.Serializer do | |||
| if {rel_view, :include} == valid_include_view && is_data_loaded?(rel_data) do | |||
| rel_query_includes = | |||
| if is_list(query_includes) do | |||
| Keyword.get(query_includes, key, []) | |||
| Enum.map(query_includes, fn include -> | |||
There was a problem hiding this comment.
Could we refactor this entire pipechain into a Enum.reduce/3?
Enum.reduce(query_includes, [], fn
{_key, value}, acc -> acc ++ [value]
_, acc -> acc
end)
lib/jsonapi/serializer.ex
Outdated
| base = view.relationships() | ||
| Keyword.get(base, include) | ||
| defp get_includes(view, query_includes) do | ||
| get_default_includes(view) ++ get_query_includes(view, query_includes) |
There was a problem hiding this comment.
Let's avoid pipes for single function calls.
includes = get_default_includes(view) ++ get_query_includes(view, query_includes)
Enum.uniq(includes)
lib/jsonapi/serializer.ex
Outdated
| defp get_default_includes(view) do | ||
| rels = view.relationships() | ||
| default_includes = rels |> Enum.filter(fn {k, v} -> | ||
| case v do |
There was a problem hiding this comment.
You don't need to use a case here, match on the function head 👍
lib/jsonapi/serializer.ex
Outdated
|
|
||
| defp get_query_includes(view, query_includes) do | ||
| rels = view.relationships() | ||
| Enum.map(query_includes, fn include -> |
There was a problem hiding this comment.
I think there is an opportunity to refactor this. We could probably move to Enum.reduce/3 and we can eliminate the case by matching on the function signature instead.
| @@ -9,7 +9,7 @@ defmodule JSONAPITest do | |||
| def type, do: "mytype" | |||
| def relationships do | |||
| [author: {JSONAPITest.UserView, :include}, | |||
| other_user: {JSONAPITest.UserView, :include}] | |||
| other_user: JSONAPITest.UserView] | |||
There was a problem hiding this comment.
Will these changes result in a breaking change? If not, let's leave the existing tests alone and add new ones to cover these changes.
There was a problem hiding this comment.
Yeah this changes the functionality. Although, I think it fixes the original intended functionality of :include meaning that the relationship will be included by default. I'm basing this on this bit from the readme:
def relationships do
# The post's author will be included by default
[author: {MyApp.UserView, :include},
comments: MyApp.CommentView]
end
Thoughts?
There was a problem hiding this comment.
@doomspork @jeregrine ping
There was a problem hiding this comment.
I agree with @doomspork's suggestions. Especially re-testing, when adding new functionality its best to leave existing tests the same and creating new ones. This will help cover backwards compatibility.
|
@doomspork @jeregrine @jlvallelonga bump, any other thoughts on this one? What's needed to get this merged in? |
|
Sorry @jlvallelonga, I've been away on a business trip and didn't have too much time to go through GH. Since these are a breaking change, I'm going to have to spend some time looking through them closer. That said, if @jeregrine wants to go forward with it 👍 |
|
Thank you @jlvallelonga! @StareIntoTheBeard I will be cutting a new release shortly 👍 |
fixes nested includes. query strings like this should now include data as expected:
?include=user.company.industry.tags