Deduplicate resource objects in includeds

[ivan-sibilla]

A resource reachable through multiple include paths gets serialized once per path. When those paths load different relationships (e.g. author.company loads the user's company but comments.user does not) the variants differ, so accumulating them in a MapSet keeps all of them.

The result is more than one resource object for the same type and id in included, which violates the JSON:API compound documents rule:

A compound document MUST NOT include more than one resource object for each type and id pair.

This PR makes sure that after building included, we collapse entries by type + id, merging their attributes and relationships into a single object.

[lucacorti]

There's something unclear about this. MapSet should take care of duplication automatically. So:

1. Why is this not happening? (see https://elixir.hexdocs.pm/MapSet.html#put/2)

iex(1)> a = MapSet.new([%{type: "a", id: 1}])
MapSet.new([%{id: 1, type: "a"}])
iex(3)> MapSet.put(a, %{id: 1, type: "a"})
MapSet.new([%{id: 1, type: "a"}])
iex(5)> MapSet.union(a, a)
MapSet.new([%{id: 1, type: "a"}])

2. If MapSet isn't working for a valid reason, can we implement a solution that avoids enumerating the included list again? (e.g. using a map with a type {type, id} tuple as key to avoid duplicating entries)

@treere ideas?

[ivan-sibilla]

Why is this not happening?

IIUC, the root cause would be in the normalize_resource_included logic, which only includes the relationship if it's loaded. This means that for the same {type, id} pair we could have two different objects (because the relationships are different), even though they effectively represent the same type.

I think that's MapSet's expected behaviour, but correct me if I'm missing something:

iex(1)> a = MapSet.new([%{type: "a", id: 1}])
MapSet.new([%{id: 1, type: "a"}])
iex(2)> b = MapSet.new([%{type: "a", id: 1, relationships: %{"company" => :loaded}}])
MapSet.new([%{id: 1, type: "a", relationships: %{"company" => :loaded}}])
iex(3)> MapSet.union(a, b)
MapSet.new([
  %{id: 1, type: "a"},
  %{id: 1, type: "a", relationships: %{"company" => :loaded}}
])

If MapSet isn't working for a valid reason, can we implement a solution that avoids enumerating the included list again? (e.g. using a map with a type {type, id} tuple as key to avoid duplicating entries)

I've tried implementing a solution with this approach, let me know if you had something different in mind. Thank you!

[lucacorti]

Ok, so this only happens on many relationships when loaded relationships differ from relationships for an already selected include from a different include path. It should not be necessary to request the same include twice with two different paths. However, I see this can be a problem if you do.

[lucacorti]

@ivan-sibilla there's an unused function. If you can fix it and test the changes work as intended for you we can merge and release an updated version

[ivan-sibilla]

@lucacorti Removed the unused function and double checked that this should fix the problem we encountered. Thank you 🙏

[ivan-sibilla]

@lucacorti Would you mind merging yourself? I don't have merge permissions.

[lucacorti]

@ivan-sibilla done, if you can test this against main in production I can then release a patch version.

[ivan-sibilla]

@lucacorti Tested in production against the current revision on main, and the fix is working as intended; I think we can proceed with the patch version. Thank you so much!

[lucacorti]

@ivan-sibilla 2.0.4 is out including your changes. Thanks ❤️