Repository: jeregrine/jsonapi (elixir)

Issue:
Bad include can return 500
According to the includes spec on jsonapi ([1](http://jsonapi.org/format/#fetching-includes)), invalid includes should only return 400.


https://github.com/jeregrine/jsonapi/blob/242499eaf42362fa0f1e6602ecc08f9baa7feaad/lib/jsonapi/query_parser.ex#L183

Looks to be due to this line. Based on whether or not the atom exists in the app, the line raises an error only on certain invalid includes


Pull request description:
Fix Bad Includes Returns HTTP 500 Response
It looks like the smoking gun was in our tests this whole time.

Resolves #134

Patch:
diff --git a/lib/jsonapi/config.ex b/lib/jsonapi/config.ex
index bfd6c1c..0ff5030 100644
--- a/lib/jsonapi/config.ex
+++ b/lib/jsonapi/config.ex
@@ -3,6 +3,8 @@ defmodule JSONAPI.Config do
   Configuration struct containing JSON API information for a request
   """
 
+  alias JSONAPI.Page
+
   defstruct data: nil,
             fields: %{},
             filter: [],
@@ -11,5 +13,17 @@ defmodule JSONAPI.Config do
             required_fields: nil,
             sort: nil,
             view: nil,
-            page: %JSONAPI.Page{}
+            page: %Page{}
+
+  @type t :: %__MODULE__{
+          data: struct,
+          fields: map,
+          filter: keyword,
+          include: keyword,
+          opts: any,
+          required_fields: any,
+          sort: any,
+          view: any,
+          page: Page.t()
+        }
 end
diff --git a/lib/jsonapi/exceptions.ex b/lib/jsonapi/exceptions.ex
index e1a7275..a4701fa 100644
--- a/lib/jsonapi/exceptions.ex
+++ b/lib/jsonapi/exceptions.ex
@@ -1,7 +1,7 @@
 defmodule JSONAPI.Exceptions do
   defmodule InvalidQuery do
     @moduledoc """
-    Defines a generic exception for when an invalid query is recieved and is unable to be parsed nor handlded.
+    Defines a generic exception for when an invalid query is recieved and is unable to be parsed nor handled.
 
     All JSONAPI exceptions on index routes return a 400.
     """
diff --git a/lib/jsonapi/page.ex b/lib/jsonapi/page.ex
index 767e113..bd040ba 100644
--- a/lib/jsonapi/page.ex
+++ b/lib/jsonapi/page.ex
@@ -8,4 +8,12 @@ defmodule JSONAPI.Page do
             size: nil,
             page: nil,
             cursor: nil
+
+  @type t :: %__MODULE__{
+          limit: non_neg_integer,
+          offset: non_neg_integer,
+          size: non_neg_integer,
+          page: non_neg_integer,
+          cursor: non_neg_integer
+        }
 end
diff --git a/lib/jsonapi/plugs/query_parser.ex b/lib/jsonapi/plugs/query_parser.ex
index 1b3af2e..426aa43 100644
--- a/lib/jsonapi/plugs/query_parser.ex
+++ b/lib/jsonapi/plugs/query_parser.ex
@@ -182,22 +182,33 @@ defmodule JSONAPI.QueryParser do
       if inc =~ ~r/\w+\.\w+/ do
         acc ++ handle_nested_include(inc, valid_includes, config)
       else
-        inc = inc |> String.to_existing_atom()
+        inc =
+          try do
+            String.to_existing_atom(inc)
+          rescue
+            ArgumentError -> raise_invalid_include_query(inc, config.view.type())
+          end
 
         if Enum.any?(valid_includes, fn {key, _val} -> key == inc end) do
           acc ++ [inc]
         else
-          raise InvalidQuery, resource: config.view.type(), param: inc, param_type: :include
+          raise_invalid_include_query(inc, config.view.type())
         end
       end
     end)
   end
 
+  @spec handle_nested_include(key :: String.t(), valid_include :: list(), config :: Config.t()) ::
+          list() | no_return()
   def handle_nested_include(key, valid_include, config) do
     keys =
-      key
-      |> String.split(".")
-      |> Enum.map(&String.to_existing_atom/1)
+      try do
+        key
+        |> String.split(".")
+        |> Enum.map(&String.to_existing_atom/1)
+      rescue
+        ArgumentError -> raise_invalid_include_query(key, config.view.type())
+      end
 
     last = List.last(keys)
     path = Enum.slice(keys, 0, Enum.count(keys) - 1)
@@ -205,7 +216,7 @@ defmodule JSONAPI.QueryParser do
     if member_of_tree?(keys, valid_include) do
       put_as_tree([], path, last)
     else
-      raise InvalidQuery, resource: config.view.type(), param: key, param_type: :include
+      raise_invalid_include_query(key, config.view.type())
     end
   end
 
@@ -224,6 +235,12 @@ defmodule JSONAPI.QueryParser do
     end
   end
 
+  @spec raise_invalid_include_query(param :: String.t(), resource_type :: String.t()) ::
+          no_return()
+  defp raise_invalid_include_query(param, resource_type) do
+    raise InvalidQuery, resource: resource_type, param: param, param_type: :include
+  end
+
   defp build_config(opts) do
     view = Keyword.fetch!(opts, :view)
     struct(Config, opts: opts, view: view)


Test patch:
diff --git a/test/jsonapi/plugs/query_parser_test.exs b/test/jsonapi/plugs/query_parser_test.exs
index a56a9b7..166ff77 100644
--- a/test/jsonapi/plugs/query_parser_test.exs
+++ b/test/jsonapi/plugs/query_parser_test.exs
@@ -82,10 +82,6 @@ defmodule JSONAPI.QueryParserTest do
     assert parse_include(config, "comments,author").include == [:comments, :author]
     assert parse_include(config, "comments.user").include == [comments: :user]
     assert parse_include(config, "best_friends").include == [:best_friends]
-
-    assert_raise ArgumentError, "argument error", fn ->
-      assert parse_include(config, "author.top-posts")
-    end
   end
 
   test "parse_include/2 errors with invalid includes" do
@@ -102,6 +98,14 @@ defmodule JSONAPI.QueryParserTest do
     assert_raise InvalidQuery, "invalid include, comments.author.user for type mytype", fn ->
       parse_include(config, "comments.author.user")
     end
+
+    assert_raise InvalidQuery, "invalid include, author.top-posts for type mytype", fn ->
+      assert parse_include(config, "author.top-posts")
+    end
+
+    assert_raise InvalidQuery, "invalid include, fake-rel for type mytype", fn ->
+      assert parse_include(config, "fake-rel")
+    end
   end
 
   test "parse_fields/2 turns a fields map into a map of validated fields" do