Allows the view to add links

[zamith]

Why:

• The JSON:API spec supports pagination by adding links to the top level
  links object. The URL itself has only one restriction, which is that
  the query params names need to all be within the page array.

This change addresses the need by:

• Allowing the view to override a links function which should return a
  map with the urls to be appended. Note that there is nothing specific
  about pagination here.
• Adding url_for_pagination a helper function to more easily create
  JSON:API compliant pagination urls.

[jeregrine]

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

[zamith]

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?

[zamith]

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

[jeregrine]

@zamith sorry for slow response, yes similar to remove_links

[zamith]

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?

[doomspork]

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

[zamith]

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?

[doomspork]

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?

[doomspork]

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

[zamith]

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
    }
  }
})

[doomspork]

@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 🤔

[zamith]

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 :)

[zamith]

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

[doomspork]

@zamith could you write new test cases in addition to modifying the existing ones? It would be great to see 1 entire block of tests that explicitly tests this functionality so we can more readily maintain it.

[zamith]

@doomspork Sure.

[zamith]

@doomspork @jeregrine I'm sure you both have been really busy and I'm sorry to bother you, but is there any update on this?

[doomspork]

@zamith I'll have to defer to @jeregrine, I'm limited to only my phone for the next 6 days.

[zamith]

@jeregrine Made the changes and also added a warning for the potential confusing situation when the config is set to true, and no links are defined.

I'm not sure a warning is the way the go, but I do believe that's an important message to send across at that stage. What do you think?

[doomspork]

Shouldn't we use Logger?

[zamith]

I'm fine with that. I was not sure what would be the best way to pass this along to the user. What log level though? info?

[doomspork]

@zamith looks good to me. @jeregrine any final thoughts?

[zamith]

@doomspork Cool. Nice to get this merged. I still have the outstanding issue with how to pass pagination data in. I found metadata to be the nicest way, but it does not conform to the convention of data and conn being the only two arguments passed to these overrides.

On the other hand, I haven't been able to pass in the pagination info without messing up the enumeration of the data when it's a list.

What do you suggest?

[doomspork]

@zamith #101 right? I will get to that next. Finally have the bandwidth 😌

[zamith]

@doomspork Hum... I don't think that solves it. The issue here is that I need access to the pagination data on the links overridable function, so I can do something like this:

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

Here it's being passed in the meta, if I try to pass it in the data like so:

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

Then the collection will be passed in on its entirety to the view, instead of being iterated over. That is because of this:

  def encode_data(view, data, conn, query_includes) when is_list(data) do
    Enum.map_reduce(data, [], fn d, acc ->
      {to_include, encoded_data} = encode_data(view, d, conn, query_includes)
      {to_include, acc ++ [encoded_data]}
    end)
  end

which will not be called, since data is not a list.

Does that make sense, or am I missing something?

[lucacorti]

@zamith @doomspork I'm experiencing this issue too. It is basically impossibile to generate pagination links and total page and item count information without the ability to pass this data back to the view from the source generating the paged data.

As a workaround this could be passed in in the conn but is not the cleanest way.