Repository: jeregrine/jsonapi
Pull request #122 (2018-07-12 14:50:44)

Corrects the format validation plug
Why:

* Currently we're validating a POST or PATCH request if it has either an
  attributes or relationships member. However, there is nothing in the
  spec (as far as I'm able to understand) that enforces this. Moreover,
  it does say that type is required for POSTs and type and id are
  required for PATCHs, which we are not validating.

This change addresses the need by:

* Taking this into account and refactoring the plug to correctly
  validate the format of requests.


Patch:
diff --git a/lib/jsonapi/deprecation.ex b/lib/jsonapi/deprecation.ex
index 09733e0..dcbd3c3 100644
--- a/lib/jsonapi/deprecation.ex
+++ b/lib/jsonapi/deprecation.ex
@@ -13,4 +13,11 @@ defmodule Deprecation do
       Macro.Env.stacktrace(__ENV__)
     )
   end
+
+  def warn(:hidden) do
+    IO.warn(
+      "`JSONAPI.View.hidden/0` is deprecated; use `JSONAPI.View.hidden/1` instead.",
+      Macro.Env.stacktrace(__ENV__)
+    )
+  end
 end
diff --git a/lib/jsonapi/plugs/format_required.ex b/lib/jsonapi/plugs/format_required.ex
index 9ecb9a3..67fcd64 100644
--- a/lib/jsonapi/plugs/format_required.ex
+++ b/lib/jsonapi/plugs/format_required.ex
@@ -8,8 +8,10 @@ defmodule JSONAPI.FormatRequired do
   def init(opts), do: opts
 
   def call(%{method: method} = conn, _opts) when method in ["DELETE", "GET", "HEAD"], do: conn
-  def call(%{params: %{"data" => %{"relationships" => _}}} = conn, _), do: conn
-  def call(%{params: %{"data" => %{"attributes" => _}}} = conn, _), do: conn
+
+  def call(%{method: "POST", params: %{"data" => %{"type" => _}}} = conn, _), do: conn
+
+  def call(%{params: %{"data" => %{"type" => _, "id" => _}}} = conn, _), do: conn
 
   def call(%{params: %{"data" => _}} = conn, _),
     do: send_error(conn, missing_data_attributes_param())
diff --git a/lib/jsonapi/query_parser.ex b/lib/jsonapi/query_parser.ex
index 4b04b45..caef14b 100644
--- a/lib/jsonapi/query_parser.ex
+++ b/lib/jsonapi/query_parser.ex
@@ -117,8 +117,7 @@ defmodule JSONAPI.QueryParser do
       requested_fields =
         value
         |> String.split(",")
-        |> Enum.map(&String.to_atom/1)
-        |> Enum.into(MapSet.new())
+        |> Enum.into(MapSet.new(), &String.to_atom/1)
 
       unless MapSet.subset?(requested_fields, valid_fields) do
         bad_fields =
diff --git a/lib/jsonapi/serializer.ex b/lib/jsonapi/serializer.ex
index fc80b44..bced474 100644
--- a/lib/jsonapi/serializer.ex
+++ b/lib/jsonapi/serializer.ex
@@ -25,10 +25,16 @@ defmodule JSONAPI.Serializer do
 
     encoded_data = %{
       data: encoded_data,
-      included: flatten_included(to_include),
-      meta: meta
+      included: flatten_included(to_include)
     }
 
+    encoded_data =
+      if is_map(meta) do
+        Map.put(encoded_data, :meta, meta)
+      else
+        encoded_data
+      end
+
     merge_links(encoded_data, data, view, conn, remove_links?(), with_pagination?())
   end
 
diff --git a/lib/jsonapi/utils/underscore.ex b/lib/jsonapi/utils/underscore.ex
index be109cc..3d2c2ff 100644
--- a/lib/jsonapi/utils/underscore.ex
+++ b/lib/jsonapi/utils/underscore.ex
@@ -24,9 +24,7 @@ defmodule JSONAPI.Utils.Underscore do
   end
 
   def underscore(value) when is_map(value) do
-    value
-    |> Enum.map(&underscore/1)
-    |> Enum.into(%{})
+    Enum.into(value, %{}, &underscore/1)
   end
 
   def underscore({key, value}) do
diff --git a/lib/jsonapi/view.ex b/lib/jsonapi/view.ex
index 7a41dbd..b8c720a 100644
--- a/lib/jsonapi/view.ex
+++ b/lib/jsonapi/view.ex
@@ -107,7 +107,15 @@ defmodule JSONAPI.View do
       end
 
       def attributes(data, conn) do
-        visible_fields = fields() -- hidden()
+        hidden =
+          if Enum.member?(__MODULE__.__info__(:functions), {:hidden, 0}) do
+            Deprecation.warn(:hidden)
+            __MODULE__.hidden()
+          else
+            hidden(data)
+          end
+
+        visible_fields = fields() -- hidden
 
         Enum.reduce(visible_fields, %{}, fn field, intermediate_map ->
           value =
@@ -128,7 +136,7 @@ defmodule JSONAPI.View do
 
       def fields, do: raise("Need to implement fields/0")
 
-      def hidden, do: []
+      def hidden(data), do: []
 
       def show(model, conn, _params, meta \\ nil), do: serialize(__MODULE__, model, conn, meta)
       def index(models, conn, _params, meta \\ nil), do: serialize(__MODULE__, models, conn, meta)
@@ -197,7 +205,7 @@ defmodule JSONAPI.View do
       defoverridable attributes: 2,
                      links: 2,
                      fields: 0,
-                     hidden: 0,
+                     hidden: 1,
                      id: 1,
                      meta: 2,
                      relationships: 0,


Test patch:
diff --git a/test/jsonapi/plugs/format_required_test.exs b/test/jsonapi/plugs/format_required_test.exs
index e1eeffe..239980c 100644
--- a/test/jsonapi/plugs/format_required_test.exs
+++ b/test/jsonapi/plugs/format_required_test.exs
@@ -35,10 +35,28 @@ defmodule JSONAPI.FormatRequiredTest do
            } = error
   end
 
-  test "does not halt if only relationships member is present" do
+  test "does not halt if only type member is present on a post" do
     conn =
       :post
-      |> conn("/example", Jason.encode!(%{data: %{relationships: %{}}}))
+      |> conn("/example", Jason.encode!(%{data: %{type: "something"}}))
+      |> call_plug
+
+    refute conn.halted
+  end
+
+  test "halts if only type member is present on a patch" do
+    conn =
+      :patch
+      |> conn("/example", Jason.encode!(%{data: %{type: "something"}}))
+      |> call_plug
+
+    assert conn.halted
+  end
+
+  test "does not halt if type and id members are present on a patch" do
+    conn =
+      :patch
+      |> conn("/example", Jason.encode!(%{data: %{type: "something", id: "some-identifier"}}))
       |> call_plug
 
     refute conn.halted
@@ -47,7 +65,7 @@ defmodule JSONAPI.FormatRequiredTest do
   test "passes request through" do
     conn =
       :post
-      |> conn("/example", Jason.encode!(%{data: %{attributes: %{}}}))
+      |> conn("/example", Jason.encode!(%{data: %{type: "something"}}))
       |> call_plug
 
     refute conn.halted
diff --git a/test/jsonapi/view_test.exs b/test/jsonapi/view_test.exs
index 7e920a6..de07094 100644
--- a/test/jsonapi/view_test.exs
+++ b/test/jsonapi/view_test.exs
@@ -15,6 +15,16 @@ defmodule JSONAPI.ViewTest do
 
   defmodule PostView do
     use JSONAPI.View, type: "posts", namespace: "/api"
+
+    def fields do
+      [:title, :body]
+    end
+
+    def hidden(%{title: "Hidden body"}) do
+      [:body]
+    end
+
+    def hidden(_), do: []
   end
 
   defmodule UserView do
@@ -91,7 +101,7 @@ defmodule JSONAPI.ViewTest do
     refute {:render, 2} in PostView.__info__(:functions)
   end
 
-  test "attributes/2 does not display hidden fields" do
+  test "attributes/2 does not display hidden fields with deprecated hidden/0" do
     expected_map = %{age: 100, first_name: "Jason", last_name: "S", full_name: "Jason S"}
 
     assert expected_map ==
@@ -100,4 +110,21 @@ defmodule JSONAPI.ViewTest do
                nil
              )
   end
+
+  test "attributes/2 does not display hidden fields based on a condition" do
+    hidden_expected_map = %{title: "Hidden body"}
+    normal_expected_map = %{title: "Other title", body: "Something"}
+
+    assert hidden_expected_map ==
+             PostView.attributes(
+               %{title: "Hidden body", body: "Something"},
+               nil
+             )
+
+    assert normal_expected_map ==
+             PostView.attributes(
+               %{title: "Other title", body: "Something"},
+               nil
+             )
+  end
 end
diff --git a/test/jsonapi_test.exs b/test/jsonapi_test.exs
index 75394fd..a713295 100644
--- a/test/jsonapi_test.exs
+++ b/test/jsonapi_test.exs
@@ -2,6 +2,14 @@ defmodule JSONAPITest do
   use ExUnit.Case
   use Plug.Test
 
+  @default_data %{
+    id: 1,
+    text: "Hello",
+    body: "Hi",
+    author: %{username: "jason", id: 2},
+    other_user: %{username: "josh", id: 3}
+  }
+
   defmodule PostView do
     use JSONAPI.View
 
@@ -83,15 +91,7 @@ defmodule JSONAPITest do
     conn =
       :get
       |> conn("/posts")
-      |> Plug.Conn.assign(:data, [
-        %{
-          id: 1,
-          text: "Hello",
-          body: "Hi",
-          author: %{username: "jason", id: 2},
-          other_user: %{username: "josh", id: 3}
-        }
-      ])
+      |> Plug.Conn.assign(:data, [@default_data])
       |> Plug.Conn.assign(:meta, %{total_pages: 1})
       |> MyPostPlug.call([])
 
@@ -133,15 +133,7 @@ defmodule JSONAPITest do
   test "handles includes properly" do
     conn =
       conn(:get, "/posts?include=other_user")
-      |> Plug.Conn.assign(:data, [
-        %{
-          id: 1,
-          text: "Hello",
-          body: "Hi",
-          author: %{username: "jason", id: 2},
-          other_user: %{username: "josh", id: 3}
-        }
-      ])
+      |> Plug.Conn.assign(:data, [@default_data])
       |> Plug.Conn.fetch_query_params()
       |> MyPostPlug.call([])
 
@@ -272,4 +264,29 @@ defmodule JSONAPITest do
 
     assert Map.has_key?(json, "links")
   end
+
+  test "omits explicit nil meta values as per http://jsonapi.org/format/#document-meta" do
+    conn =
+      :get
+      |> conn("/posts")
+      |> Plug.Conn.assign(:data, [@default_data])
+      |> Plug.Conn.assign(:meta, nil)
+      |> MyPostPlug.call([])
+
+    json = conn.resp_body |> Jason.decode!()
+
+    refute Map.has_key?(json, "meta")
+  end
+
+  test "omits implicit nil meta values as per http://jsonapi.org/format/#document-meta" do
+    conn =
+      :get
+      |> conn("/posts")
+      |> Plug.Conn.assign(:data, [@default_data])
+      |> MyPostPlug.call([])
+
+    json = conn.resp_body |> Jason.decode!()
+
+    refute Map.has_key?(json, "meta")
+  end
 end