Query parameters in self url

[CostantiniMatteo]

The self url includes query parameters.

For example, the self url for http://localhost/users?page[page]=2 is the url itself, instead of http://localhost/users

[snewcomer]

👋 Why is this needed? Is there an assertion we could add for a similar example you had in the PR description?

[CostantiniMatteo]

Why is this needed?

Because, otherwise, the query_params field of the connection is a Plug.Conn.Unfetched struct, and JSONAPI.View.url_for_pagination/3 fails with the following error.

1) test with a dasherized API handles sparse fields properly (JSONAPITest)
     test/jsonapi_test.exs:328
     ** (Protocol.UndefinedError) protocol Enumerable not implemented for %{:__struct__ => Plug.Conn.Unfetched, :aspect => :query_params, "page" => %{}} of type Plug.Conn.Unfetched (a struct). This protocol is implemented for the following type(s): HashSet, Range, Map, Function, List, Stream, Date.Range, HashDict, GenEvent.Stream, MapSet, File.Stream, IO.Stream
     code: |> MyPostPlug.call([])
     stacktrace:
       (elixir 1.10.2) lib/enum.ex:1: Enumerable.impl_for!/1
       (elixir 1.10.2) lib/enum.ex:141: Enumerable.reduce/3
       (elixir 1.10.2) lib/enum.ex:3383: Enum.flat_map/2
       test/jsonapi_test.exs:14: JSONAPITest.PostView.url_for_pagination/3
       (jsonapi 1.2.3) lib/jsonapi/serializer.ex:178: JSONAPI.Serializer.merge_links/7
       test/jsonapi_test.exs:86: JSONAPITest.MyPostPlug.passthrough/2
       test/jsonapi_test.exs:73: JSONAPITest.MyPostPlug.plug_builder_call/2
       test/jsonapi_test.exs:334: (test)

Is there an assertion we could add for a similar example you had in the PR description?

I added a test to serializer_test.exs. Does it look good?

[snewcomer]

Great test addition!

[snewcomer]

Thank you for this PR!

[CostantiniMatteo]

I've noticed that the detail of a resource also include pagination links, but since it's only a single resource and not a list, I wouldn't expect such links there.
For example http://localhost:4000/users/123 returns the following json:

{
  "data": {
    "attributes": {
      "username": "john"
    },
    "id": "123",
    "links": {
      "self": "http://localhost:4000/users/123"
    },
    "relationships": {},
    "type": "users"
  },
  "included": [],
  "links": {
    "first": "http://localhost:4000/users/123?page%5Bpage%5D=1",
    "last": "http://localhost:4000/users/123?page%5Bpage%5D=0",
    "next": null,
    "prev": null,
    "self": "http://localhost:4000/users/123"
  }
}

I added a commit trying to fix this, but as you can see, tests fail when the connection is nil.
It's not clear to me, when a connection will be nil, and how such cases should be handled.
Am I doing something wrong? If not, how should I update those tests?

Here is an example of failing test:

  1) test serialize does not include pagination links if they are not defined (JSONAPI.SerializerTest)
     test/jsonapi/serializer_test.exs:620
     ** (FunctionClauseError) no function clause matching in JSONAPI.SerializerTest.UserView.url_for_pagination/3

     The following arguments were given to JSONAPI.SerializerTest.UserView.url_for_pagination/3:
     
         # 1
         [%{id: 1}]
     
         # 2
         nil
     
         # 3
         nil
     
     code: encoded = Serializer.serialize(UserView, data, nil)
     stacktrace:
       test/jsonapi/serializer_test.exs:76: JSONAPI.SerializerTest.UserView.url_for_pagination/3
       (jsonapi 1.2.3) lib/jsonapi/serializer.ex:172: JSONAPI.Serializer.merge_links/7
       test/jsonapi/serializer_test.exs:623: (test)

Thank you!

[CostantiniMatteo]

I've fixed the failing tests by replacing nil with a connection and also fixed a small bug where index with an empy list returned the wrong self url.
Does it look good?

The unsuccessful check is due to the length of the quote block in lib/jsonapi/view.ex.

[lucacorti]

I think the real issue here is the __using__ macro block is huge. This is a bit of an antipattern, generated code should be kept to a minimum, however fixing this is not straightforward.

Maybe we can just ignore this specific credo warning by adding

# credo:disable-for-next-line Credo.Check.Refactor.LongQuoteBlocks

before the quote do block and maybe address issues with view macro size in another iteration.

[CostantiniMatteo]

I've temporary suppressed the warning as suggested by @lucacorti.

[jherdman]

LGTM. I wouldn't mind a few more eyes on this before merge.