vt-elixir / ja_resource

A behaviour to reduce boilerplate code in your JSON-API compliant Phoenix controllers without sacrificing flexibility.
Other
113 stars 33 forks source link

README documenting `record/2` is incorrect. #35

Closed johnmosesman closed 7 years ago

johnmosesman commented 7 years ago

Hi there,

I am trying to use the record/2 hook described in the README. When I inspect the query I see it's a %Plug.Conn{}, which errors when piping it to the Repo with the error:

(Protocol.UndefinedError) protocol Ecto.Queryable not implemented for %Plug.Conn

Here's my controller:

# web/controllers/api/outfitter_controller.ex
defmodule Hunting.Api.OutfitterController do
  use Hunting.Web, :controller
  use JaResource

  plug JaResource

  def record(query, slug_as_id) do
    query
    |> Hunting.Repo.get_by(slug: slug_as_id)
  end

  def records(%Plug.Conn{}) do
    raise "hi"
  end
end

Am I making a mistake somewhere? I also noticed that the records/1 function is not called either.

Full error:

[info] GET /api/outfitters/3
[debug] Processing by Hunting.Api.OutfitterController.show/2
  Parameters: %{"id" => "3"}
  Pipelines: [:api]
[info] Sent 500 in 4ms
[error] #PID<0.406.0> running Hunting.Endpoint terminated
Server: localhost:4000 (http)
Request: GET /api/outfitters/3
** (exit) an exception was raised:
    ** (Protocol.UndefinedError) protocol Ecto.Queryable not implemented for %Plug.Conn{adapter: {Plug.Adapters.Cowboy.Conn, :...}, assigns: %{}, before_send: [#Function<0.23638007/1 in JaSerializer.ContentTypeNegotiation.set_content_type/2>, #Function<1.69507563/1 in Plug.Logger.call/2>, #Function<0.131639940/1 in Phoenix.LiveReloader.before_send_inject_reloader/1>], body_params: %{}, cookies: %Plug.Conn.Unfetched{aspect: :cookies}, halted: false, host: "localhost", method: "GET", owner: #PID<0.392.0>, params: %{"id" => "3"}, path_info: ["api", "outfitters", "3"], peer: {{127, 0, 0, 1}, 63280}, port: 4000, private: %{Hunting.Router => {[], %{}}, :phoenix_action => :show, :phoenix_controller => Hunting.Api.OutfitterController, :phoenix_endpoint => Hunting.Endpoint, :phoenix_format => "json-api", :phoenix_layout => {Hunting.LayoutView, :app}, :phoenix_pipelines => [:api], :phoenix_route => #Function<21.100032585/1 in Hunting.Router.match_route/4>, :phoenix_router => Hunting.Router, :phoenix_view => Hunting.Api.OutfitterView, :plug_session_fetch => #Function<1.82590416/1 in Plug.Session.fetch_session/1>}, query_params: %{}, query_string: "", remote_ip: {127, 0, 0, 1}, req_cookies: %Plug.Conn.Unfetched{aspect: :cookies}, req_headers: [{"host", "localhost:4000"}, {"user-agent", "curl/7.43.0"}, {"accept", "*/*"}], request_path: "/api/outfitters/3", resp_body: nil, resp_cookies: %{}, resp_headers: [{"cache-control", "max-age=0, private, must-revalidate"}, {"x-request-id", "up7ehkp3ejqdagkpatcn4n4f2us2q3et"}], scheme: :http, script_name: [], secret_key_base: "/HVcM/Qrq5RnZE1bJLSaPEl8+GwO2vFQACTJEQWGKv3CIj2d4XXuocQ3O7J/tveZ", state: :unset, status: nil}
        (ecto) lib/ecto/queryable.ex:1: Ecto.Queryable.impl_for!/1
        (ecto) lib/ecto/queryable.ex:9: Ecto.Queryable.to_query/1
        (ecto) lib/ecto/query/builder/filter.ex:81: Ecto.Query.Builder.Filter.apply/3
        (ecto) lib/ecto/repo/queryable.ex:52: Ecto.Repo.Queryable.get_by/5
        (ja_resource) lib/ja_resource/show.ex:61: JaResource.Show.call/2
        (ja_resource) lib/ja_resource/plug.ex:61: JaResource.Plug.call/2
        (hunting) web/controllers/api/outfitter_controller.ex:1: Hunting.Api.OutfitterController.phoenix_controller_pipeline/2
        (hunting) lib/hunting/endpoint.ex:1: Hunting.Endpoint.instrument/4
        (hunting) lib/phoenix/router.ex:261: Hunting.Router.dispatch/2
        (hunting) web/router.ex:1: Hunting.Router.do_call/2
        (hunting) lib/hunting/endpoint.ex:1: Hunting.Endpoint.phoenix_pipeline/1
        (hunting) lib/plug/debugger.ex:123: Hunting.Endpoint."call (overridable 3)"/2
        (hunting) lib/hunting/endpoint.ex:1: Hunting.Endpoint.call/2
        (plug) lib/plug/adapters/cowboy/handler.ex:15: Plug.Adapters.Cowboy.Handler.upgrade/4
        (cowboy) src/cowboy_protocol.erl:442: :cowboy_protocol.execute/4
JAResource 0.2.0
Elixir 1.3.4
Phoenix 1.2.1
johnmosesman commented 7 years ago

I noticed that the Record docs show it using the model() function. Does the README have a typo?

This works for me:

defmodule Hunting.Api.OutfitterController do
  use Hunting.Web, :controller
  use JaResource

  plug JaResource

  def model, do: Hunting.Outfitter

  def record(query, slug_as_id) do
    model()
    |> Hunting.Repo.get_by(slug: slug_as_id)
  end

  def records(%Plug.Conn{}) do
    raise "hi"  # this never gets called
  end

end

I am still confused why records/1 isn't called. Based on the docs description ("record/2 receives the results of records/1") I would think that hook gets fired before passing its result to records/2. Is that incorrect?

alanpeabody commented 7 years ago

Hi @johnmosesman you are totally right, the readme is completely wrong. Sorry about that!

All callbacks except model/0 and repo/0 get the %Plug.Conn{} as the first argument. The default implementation is really:

def model, do: MyModel
def records(_conn), do: model()
def record(conn, id), do: conn |> records |> repo.get(id)

So in your case you should be able to only override record with:

def record(conn, slug), do: conn |> records |> repo.get_by(slug: slug)

I will leave this issue open to remind myself to update the README, thanks for reporting it and sorry for any inconvenience!

johnmosesman commented 7 years ago

No worries! A little doc diving never hurt anyone :)

Would you want me to submit a PR for the change?

alanpeabody commented 7 years ago

A PR would be very welcome!