Repository: jeregrine/jsonapi
Pull request #176 (2019-02-08 23:40:28)

Issue175
Makes `FormatRequired` plug accept a legal multi-RIO payload.

jherdman: The Credo changes are a bit surprising. On my dev machine, and CI, `mix credo` reports no concerns. Would you mind discussing your set up? Perhaps it'll help us figure out the discrepancies.
kbaird: Sure. 

```
$ iex -v
Erlang/OTP 21 [erts-10.2.3] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:1] [hipe]

IEx 1.8.1 (compiled with Erlang/OTP 20)
```

```
$ mix credo --strict
Ignoring an undefined check: Credo.Check.Warning.NameRedeclarationByAssignment
Ignoring an undefined check: Credo.Check.Warning.NameRedeclarationByCase
Ignoring an undefined check: Credo.Check.Warning.NameRedeclarationByDef
Ignoring an undefined check: Credo.Check.Warning.NameRedeclarationByFn
Checking 17 source files ...

info: the following checks were skipped because they're not compatible with
your version of Elixir (1.8.1).

   1) Credo.Check.Refactor.MapInto


  Code Readability                                                              
┃ 
┃ [R] ↘ The alias `JSONAPI.Config` is not alphabetically ordered among its 
┃       group.
┃       lib/jsonapi/serializer.ex:9:9 #(JSONAPI.Serializer)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃       lib/jsonapi.ex:33 #(JSONAPI.mime_type)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃       lib/jsonapi.ex:12 #(JSONAPI.json_library)

Please report incorrect results: https://github.com/rrrene/credo/issues

Analysis took 0.4 seconds (0.01s to load, 0.4s running checks)
168 mods/funs, found 3 code readability issues.
```
jherdman: OH! Gotcha. OK, AFAIK we're not using the `--strict` flag. Would you mind reverting the Credo changes for now? If we want to move to strict that may be a worthy PR in-and-of-itself.
kbaird: Credo changes now in https://github.com/jeregrine/jsonapi/pull/177
jherdman: @kbaird could you rebase this against master? It has your Credo changes, and I want to make sure the build is still green (I'm sure it is, but ya know... safe vs. sorry).
kbaird: Rebased via the procedure at https://gist.github.com/ravibhure/a7e0918ff4937c9ea1c456698dcd58aa
doomspork: @kbaird something isn't quite right but I think it's c1b1849 that's the culprit, it adds `()` to the functions and fails Credo.

There's 2 ways we can fix this:

```shell
$ git fetch --all --prune
$ git rebase upstream/master -i --autostash --autosquash
# I do interactive rebase so I can squash, fixup, and rename commits.
# At the screen that pops up, delete the c1b1849 line, save and exit.
# That should remove that commit for us.
$ git push origin master -f
```

You could add a new commit that undoes those changes, when we squash it won't matter 😁 

I'd be happy to help you if that doesn't resolve the issues. I'd expect the `credo --strict` call to pass on your branch.
kbaird: @doomspork I just backed it up one commit. Let me know if it needs any more attention from me. Thanks.

Patch:
diff --git a/.gitignore b/.gitignore
index 1a3bc0a..13eda0c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -5,5 +5,9 @@ erl_crash.dump
 mix.lock
 /doc
 
+# Dialyzer
+/priv/plts/*.plt
+/priv/plts/*.plt.hash
+
 .DS_Store
 .elixir_ls
diff --git a/.travis.yml b/.travis.yml
index db6e0ac..037aa99 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -30,12 +30,9 @@ env:
 notifications:
   email: false
 
-before_cache:
-  - rm -rf _build/$MIX_HOME/lib
-
 cache:
   directories:
-    - _build/$MIX_HOME
+    - priv/plts
 
 script:
   - mix test
diff --git a/lib/jsonapi/error_view.ex b/lib/jsonapi/error_view.ex
index 26f5024..d8292a3 100644
--- a/lib/jsonapi/error_view.ex
+++ b/lib/jsonapi/error_view.ex
@@ -52,6 +52,16 @@ defmodule JSONAPI.ErrorView do
     |> serialize_error
   end
 
+  def to_many_relationships_payload_for_standard_endpoint do
+    "Data parameter has multiple Resource Identifier Objects for a non-relationship endpoint"
+    |> build_error(
+      400,
+      "Check out https://jsonapi.org/format/#crud-updating-to-many-relationships for more info.",
+      "/data"
+    )
+    |> serialize_error
+  end
+
   def send_error(conn, %{errors: [%{status: status}]} = error),
     do: send_error(conn, status, error)
 
diff --git a/lib/jsonapi/plugs/format_required.ex b/lib/jsonapi/plugs/format_required.ex
index 67fcd64..38b424d 100644
--- a/lib/jsonapi/plugs/format_required.ex
+++ b/lib/jsonapi/plugs/format_required.ex
@@ -5,12 +5,24 @@ defmodule JSONAPI.FormatRequired do
 
   import JSONAPI.ErrorView
 
+  # Cf. https://jsonapi.org/format/#crud-updating-to-many-relationships
+  @update_has_many_relationships_methods ~w[DELETE PATCH POST]
+
   def init(opts), do: opts
 
-  def call(%{method: method} = conn, _opts) when method in ["DELETE", "GET", "HEAD"], do: conn
+  def call(%{method: method} = conn, _opts) when method in ~w[DELETE GET HEAD], do: conn
 
   def call(%{method: "POST", params: %{"data" => %{"type" => _}}} = conn, _), do: conn
 
+  def call(%{method: method, params: %{"data" => [%{"type" => _} | _]}} = conn, _)
+      when method in @update_has_many_relationships_methods do
+    if String.contains?(conn.request_path, "relationships") do
+      conn
+    else
+      send_error(conn, to_many_relationships_payload_for_standard_endpoint())
+    end
+  end
+
   def call(%{params: %{"data" => %{"type" => _, "id" => _}}} = conn, _), do: conn
 
   def call(%{params: %{"data" => _}} = conn, _),
diff --git a/mix.exs b/mix.exs
index 04d83f0..7157087 100644
--- a/mix.exs
+++ b/mix.exs
@@ -40,6 +40,7 @@ defmodule JSONAPI.Mixfile do
 
   defp dialyzer do
     [
+      plt_file: {:no_warn, "priv/plts/dialyzer.plt"},
       plt_add_deps: :app_tree
     ]
   end


Test patch:
diff --git a/priv/plts/.gitkeep b/priv/plts/.gitkeep
new file mode 100644
index 0000000..e69de29
diff --git a/test/jsonapi/plugs/format_required_test.exs b/test/jsonapi/plugs/format_required_test.exs
index 239980c..7623b80 100644
--- a/test/jsonapi/plugs/format_required_test.exs
+++ b/test/jsonapi/plugs/format_required_test.exs
@@ -62,6 +62,38 @@ defmodule JSONAPI.FormatRequiredTest do
     refute conn.halted
   end
 
+  test "halts with a multi-RIO payload to a non-relationship PATCH endpoint" do
+    :patch
+    |> conn("/example", Jason.encode!(%{data: [%{type: "something"}]}))
+    |> call_plug
+    |> assert_improper_use_of_multi_rio()
+  end
+
+  test "halts with a multi-RIO payload to a non-relationship POST endpoint" do
+    :post
+    |> conn("/example", Jason.encode!(%{data: [%{type: "something"}]}))
+    |> call_plug
+    |> assert_improper_use_of_multi_rio()
+  end
+
+  test "accepts a multi-RIO payload for relationship PATCH endpoints" do
+    conn =
+      :patch
+      |> conn("/example/relationships/things", Jason.encode!(%{data: [%{type: "something"}]}))
+      |> call_plug
+
+    refute conn.halted
+  end
+
+  test "accepts a multi-RIO payload for relationship POST endpoints" do
+    conn =
+      :post
+      |> conn("/example/relationships/things", Jason.encode!(%{data: [%{type: "something"}]}))
+      |> call_plug
+
+    refute conn.halted
+  end
+
   test "passes request through" do
     conn =
       :post
@@ -71,6 +103,22 @@ defmodule JSONAPI.FormatRequiredTest do
     refute conn.halted
   end
 
+  defp assert_improper_use_of_multi_rio(conn) do
+    assert conn.halted
+    assert 400 == conn.status
+
+    %{"errors" => [error]} = Jason.decode!(conn.resp_body)
+
+    assert %{
+             "detail" =>
+               "Check out https://jsonapi.org/format/#crud-updating-to-many-relationships for more info.",
+             "source" => %{"pointer" => "/data"},
+             "status" => 400,
+             "title" =>
+               "Data parameter has multiple Resource Identifier Objects for a non-relationship endpoint"
+           } = error
+  end
+
   defp call_plug(conn) do
     parser_opts = Plug.Parsers.init(parsers: [:json], pass: ["text/*"], json_decoder: Jason)