Flop.Phoenix.table breaks with date filter and flop 0.21.0

[andreasknoepfle]

Hey @woylie,

I am trying to update our code to flop >= 0.21.0 and we have a simple date filter on a schema with a date column.
As soon as I have a filter value for the date filter our Flop.Phoenix.table/1 call breaks on the path helper when rendering the
sorting column paths with errors like this:

structs expect an :id key when converting to_param or a custom implementation of the Phoenix.Param protocol (read Phoenix.Param docs for more information), got: ~D[2023-01-01]

The underlying Flop struct looks like described in the upgrade guide:

%Flop{
  after: nil,
  before: nil,
  first: nil,
  last: nil,
  limit: nil,
  offset: nil,
  order_by: [:entry_date],
  order_directions: [:asc],
  page: 1,
  page_size: 14,
  decoded_cursor: nil,
  filters: [
    %Flop.Filter{field: :entry_date, op: :>=, value: nil},
    %Flop.Filter{field: :entry_date, op: :<=, value: ~D[2023-01-01]}
  ]
}

To enable Flop.Phoenix to build a query string for filter parameters, the filter value must be convertible into a string via to_string/1. If ecto_type is set to a custom Ecto type that casts values into a struct, the String.Chars protocol must be implemented for that struct.

But our Phoenix route helper breaks with the input it gets when building the path somewhere in build_path/3, since Phoenix.Param is not implemented by default on Dates.
From the description in the upgrade guide the requirement is just implementing to_string, which is implemented for Date.

What am I missing? Is it expected that we implement Phoenix.Param for things like Date and DateTime?

For reference the schema looks like this:

  @derive {
    Flop.Schema,
    sortable: [
      :entry_date
    ],
    filterable: [:entry_date],
    default_order: %{
      order_by: [:entry_date],
      order_directions: [:desc]
    }
  }
  
  schema "entries" do
    field(:entry_date, :date)
    timestamps()
  end

TY in advance 💚

Andi

[andreasknoepfle]

I changed the phoenix route helper to a path using Phoenix VerifiedRoutes and that seems to work 🤔.

[woylie]

I changes the phoenix route helper to a path using Phoenix VerifiedRoutes and that seems to work 🤔.

That's interesting. We've moved everything to verified routes a while ago, so I guess I didn't notice because of that. Does this fix the issue for you? If so, I'll keep the issue open, but regard it as low priority.

[andreasknoepfle]

In the two instances where it caused issues I now used the new verified routes. But since we still haven't migrated at all to the verified routes, it is currently a bit weird.

The issue appeared also in other places where we would call Flop.Phoenix.to_query() to get a map of query params that we then put into the route helpers again. Since before the change all values in the filter struct were atoms or strings it worked before always. Since Flop.Phoenix.to_query returns the filter values with the Date struct now, the router helpers seem to try to go for to_param instead for those values and everything breaks.

I wonder if an easy fix would be to always call to_string within Flop.Phoenix.to_query() for filter values.

If you think that solution is viable then I can also prepare a PR :)

[woylie]

I'm actually the same error with verified routes just with:

~p"/page?#{[d: ~D[2000-01-01]]}"

So I don't know why the error disappeared for you after switching to verified routes.

I'm not sure about converting everything into a string in to_query. The Phoenix.Param protocol is responsible for URL parameters, and I wouldn't want to override that. I don't know why Phoenix doesn't implement the protocol for Date, DateTime and Time structs (presumably because different applications may have different requirements for the format to use), but I think the recommendation would be to do that in your application then:

defimpl Phoenix.Param, for: Date do
  def to_param(%Date{} = d), do: to_string(d)
end

defimpl Phoenix.Param, for: DateTime do
  def to_param(%DateTime{} = dt), do: to_string(dt)
end

defimpl Phoenix.Param, for: Time do
  def to_param(%Time{} = t), do: to_string(t)
end

I'll update the changelog accordingly and add a note somewhere in the docs.

[woylie]

Documentation updated in #288, changelog updated here.