Incorrect InvalidQuery Errors Caused by String.to_existing_atom/1

[kyleboe]

This is kind of a wild edge case, but when a nested, valid include is defined, the QueryParser throws an error when that relationship is being included because the atom representation of the nested relationship has not yet been loaded in BEAM.

Example

defmodule PlaylistsController do
  plug JSONAPI.QueryParser,
    include: ~w(songs songs.artists),
    view: PlaylistView
  # . . .
end

defmodule PlaylistView do
  def relationships do
    [songs: SongView]
  end
end

defmodule SongView do
  def relationships do
    [artists: ArtistView]
  end
end

When calling to the controller with the include=songs.artists query param, a 500 error will be returned because of runtime loading of modules/atoms. Specifically this line in handle_nested_include/3:

jsonapi/lib/jsonapi/plugs/query_parser.ex

Line 258 in 00d01cf

|> Enum.map(&String.to_existing_atom/1)

Proposed Solution

While I understand the reasoning for the current implementation (we don't want to cast external params to atoms and allow for DOS memory attacks), I think we can approach the problem differently.

Instead of converting the external params to match the internal setting. Could we convert the internal setting to match the incoming params. So instead of using a word list to define valid includes, could we use atoms to begin with and then cast them to strings and compare those strings with the ones from the internet? That way the atoms present in BEAM as soon as the module is loaded.

This may look something like this:

defmodule PlaylistsController do
  plug JSONAPI.QueryParser,
    include: [:songs, songs: :artists],
    view: PlaylistView
  # . . .
end

[mattpolzin]

So instead of using a word list to define valid includes, could we use atoms to begin with and then cast them to strings and compare those strings with the ones from the internet?

Something along those lines is probably preferable to the existing implementation in more than one way. We need to decide if it is good or bad (or unimportant) that the existing implementation takes nested include options literally (songs.artists means "including artists on songs is allowed" not "including songs is allowed and including artists on songs is allowed"). The current behavior was a side effect of the quickest implementation possible, rather than a carefully considered decision.

I'd like to start planning for a v2 release of the library so we can introduce some breaking changes and this could be one of them.