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.

Sign up for GitHub

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

Allows the view to add links #107

Merged
merged 4 commits into from
Jun 12, 2018
Merged

Allows the view to add links #107

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af9df0c
Allows the view to add links
zamith May 2, 2018
cdd1037
Adds specific tests for pagination links
zamith May 3, 2018
50d2779
Sets pagination via the Application env
zamith Jun 4, 2018
26f879d
Uses logger instead of warn
zamith Jun 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions lib/jsonapi/serializer.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ defmodule JSONAPI.Serializer do
meta: meta
}

merge_links(encoded_data, data, view, conn, remove_links?())
merge_links(encoded_data, data, view, conn, remove_links?(), true)
Copy link

Choose a reason for hiding this comment

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

We need to make this configurable, pagination is optional http://jsonapi.org/format/#fetching-pagination and not everyone will want it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, but if links is not overridden on the view, this will have no effect, because it'll merge an empty map, right? Does it need to be more configurable than that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the suggestion here? Add an environment config like remove_links?

Copy link

Choose a reason for hiding this comment

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

@zamith sorry for slow response, yes similar to remove_links

end

def encode_data(view, data, conn, query_includes) when is_list(data) do
Expand All @@ -48,7 +48,7 @@ defmodule JSONAPI.Serializer do
relationships: %{}
}

doc = merge_links(encoded_data, data, view, conn, remove_links?())
doc = merge_links(encoded_data, data, view, conn, remove_links?(), false)

doc =
case view.meta(data, conn) do
Expand Down Expand Up @@ -135,11 +135,19 @@ defmodule JSONAPI.Serializer do
merge_related_links(data, info, remove_links?())
end

defp merge_links(doc, data, view, conn, false) do
defp merge_links(doc, data, view, conn, false, true) do
links =
%{self: view.url_for(data, conn)}
|> Map.merge(view.links(data, conn))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of passing doc.meta to the links function as well, as it's probably the best way to send pagination data in. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I would err on the side of passing along less information than more until there is a specific need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you propose passing in pagination data such as a cursor, limit or offset then? I'm assuming that the controller (or whoever is serializing the data) will be in charge of that and it will be pretty opaque to the view. Am I seeing this wrong?

Copy link
Member

@doomspork doomspork May 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In my case the pagination is part of the data so I have what I need. Could you share your code where you need doc?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's also important to remain consistent in the code and we are passing data, conn for the other overridable functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I need it. I'm still trying to figure out if I really do. Let's say I have a limit and offset I want to use, do I pass that as data in a map? Something like:

render(conn, "index.json", %{
  data: %{
    collection: data,
    pagination: %{
      limit: limit,
      offset: offset
    }
  }
})

Copy link
Member

Choose a reason for hiding this comment

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

@zamith that's how the Elixir pagination libraries I'm familiar with work. You're also passing in the conn so you could do something like put_private(conn, :pagination, ...)

On a side note, are you a time traveler?! GitHub keeps sending me notifications about your notifications occurring in the future 🤔

screen shot 2018-05-02 at 8 58 06 am

Copy link
Contributor Author

@zamith zamith May 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Well, that was my first approach, the problem is that the view assumes that data is an array of models and the serializer will then map_reduce over that. From this lines:

def index(models, conn, _params, meta \\ nil), do: serialize(__MODULE__, models, conn, meta)

def render("index.json", %{data: data, conn: conn}), do: index(data, conn, conn.params)

So, if anything other than an array is passed, the iteration is messed up. That's why I reached for the metadata as a way to pass in extra information. Does this make sense?

Well, I'm guessing timezones are to blame :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@doomspork There is still this unresolved issue, or do you think it's not an issue at all?


Map.merge(doc, %{links: links})
end

defp merge_links(doc, data, view, conn, false, false) do
Map.merge(doc, %{links: %{self: view.url_for(data, conn)}})
end

defp merge_links(doc, _data, _view, _conn, _remove_links), do: doc
defp merge_links(doc, _data, _view, _conn, _remove_links, _with_pagination), do: doc

defp merge_related_links(
encoded_data,
Expand Down
18 changes: 18 additions & 0 deletions lib/jsonapi/view.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ defmodule JSONAPI.View do
end)
end

def links(_data, _conn), do: %{}

def meta(_data, _conn), do: nil

def relationships, do: []
Expand Down Expand Up @@ -155,6 +157,21 @@ defmodule JSONAPI.View do
"#{url_for(data, conn)}/relationships/#{rel_type}"
end

def url_for_pagination(data, conn, pagination_attrs) do
pagination_attrs
|> Enum.reduce(%{}, fn {key, value}, acc ->
Map.put(acc, "page[#{key}]", value)
end)
|> URI.encode_query()
|> prepare_url(data, conn)
end

defp prepare_url("", data, conn), do: url_for(data, conn)

defp prepare_url(query, data, conn) do
"#{url_for(data, conn)}?#{query}"
end

if Code.ensure_loaded?(Phoenix) do
def render("show.json", %{data: data, conn: conn, params: params, meta: meta}),
do: show(data, conn, params, meta: meta)
Expand All @@ -178,6 +195,7 @@ defmodule JSONAPI.View do
defp scheme(conn), do: Application.get_env(:jsonapi, :scheme, to_string(conn.scheme))

defoverridable attributes: 2,
links: 2,
fields: 0,
hidden: 0,
id: 1,
Expand Down
7 changes: 7 additions & 0 deletions test/jsonapi/view_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@ defmodule JSONAPI.ViewTest do
"ftp://www.otherhost.com/api/posts/1/relationships/comments"
end

test "url_for_pagination/3" do
assert PostView.url_for_pagination(nil, nil, %{}) == "/api/posts"

assert PostView.url_for_pagination(nil, nil, %{number: 1, size: 10}) ==
"/api/posts?page%5Bnumber%5D=1&page%5Bsize%5D=10"
end

@tag :compile_phoenix
test "render/2 is defined when 'Phoenix' is loaded" do
assert {:render, 2} in CommentView.__info__(:functions)
Expand Down
12 changes: 12 additions & 0 deletions test/serializer_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ defmodule JSONAPISerializerTest do
best_comments: {JSONAPISerializerTest.CommentView, :include}
]
end

def links(data, conn) do
%{
next: url_for_pagination(data, conn, %{cursor: "some-string"})
}
end
end

defmodule UserView do
Expand Down Expand Up @@ -111,6 +117,10 @@ defmodule JSONAPISerializerTest do

encoded = Serializer.serialize(PostView, data, nil)
assert encoded[:links][:self] == PostView.url_for(data, nil)

assert encoded[:links][:next] ==
PostView.url_for_pagination(data, nil, %{cursor: "some-string"})

encoded_data = encoded[:data]
assert encoded_data[:id] == PostView.id(data)
assert encoded_data[:type] == PostView.type()
Expand Down Expand Up @@ -155,10 +165,12 @@ defmodule JSONAPISerializerTest do
assert attributes[:body] == data[:body]

assert enc[:links][:self] == PostView.url_for(data, nil)
refute Map.has_key?(enc[:links], :next)
assert map_size(enc[:relationships]) == 2
end)

assert Enum.count(encoded[:included]) == 4
assert Map.has_key?(encoded[:links], :next)
end

test "serialize handles an empty relationship" do
Expand Down