Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

flexible content negotiation #1054

Closed
FND opened this issue Jul 26, 2015 · 15 comments
Closed

flexible content negotiation #1054

FND opened this issue Jul 26, 2015 · 15 comments

Comments

@FND
Copy link
Contributor

FND commented Jul 26, 2015

As far as I can tell (though I might well be wrong 🙏), it's currently not possible to perform standard content negotiation without compromising on behavior:

I want to provide multiple representations (e.g. HTML and JSON) for the same resource. As I understand it, previous discussions suggest either using different URIs (i.e. separate resources) or extending the default :browser pipeline to accept additional content types. The former is unrelated to content negotiation (and not always an option), the latter seems less than ideal because much of that pipeline is irrelevant - indeed undesirable, e.g. CSRF protection unnecessarily complicates JSON requests.

So I suppose I'm arguing that pipelines should be dependent on individual requests' Accept header rather than being hard-coded in the router.

@chrismccord
Copy link
Member

As far as I can tell (though I might well be wrong ), it's currently not possible to perform standard content negotiation without compromising on behavior:

It's plugs all the way down, so it is indeed possible. To get what you want, you could create a plug that dispatches to another stack of plugs based on the accept header and then use that in your pipeline:

defmodule Router do
  pipeline :browser do
    plug AcceptPipline, %{"json" => JsonPipeline,
                          "html" => HTMLPipeline}

  end
  ...
end


defmodule AcceptPipeline do
  import Plug.Conn

  def init(opts), do: opts

  def call(conn, opts) do
    conn
    |> Phoenix.Controller.accepts(conn, Map.keys(opts))
    |> dispatch_by_accept(opts)
  end

  defp dispatch_by_accept(%Plug.Conn{halted: true} = conn, _), do: conn
  defp dispatch_by_accept(conn, opts) do
    plug = Map.fetch!(opts, conn.params["format"])
    plug.call(conn, plug.init([]))
  end
end


defmodule JsonPipeline do
  use Plug.Builder

  plug :token_auth
  ...
end

defmodule HTMLPipeline do
  use Plug.Builder

  plug :protect_from_forgery
  ...
end

@FND
Copy link
Contributor Author

FND commented Jul 26, 2015

Excellent, thank you! As discussed on IRC (thanks for your patience there), a few adjustments were necessary:
https://gist.github.com/FND/1b22ce5cabef52d95cb6
This now works as I'd imagined.¹

While I'm happy with that solution, and it's nice that you can open the trap door to work directly with Plug, it seems like something as basic (and IMO essential) as content negotiation deserves framework-level support. So it might be nice to encapsulate the above for posterity, which might look like this:

  pipeline :browser do
    plug :fetch_session
    plug :fetch_flash
    plug ContentNegotiator, %{"html" => [:protect_from_forgery],
                              "json" => []}
  end

¹ Though I'm not entirely sure yet about the implications of always including :fetch_session and :fetch_flash, as they're not needed for non-HTML representation. Presumably that's just a bit of negligible overhead.

@chrismccord
Copy link
Member

it seems like something as basic (and IMO essential) as content negotiation deserves framework-level support.

We have content negotiation, just not the particular flavor you want. But as we've seen here, you can accomplish it in 30 LOC, so I think the framework is doing a fine job providing content negotiation and the flexibility to bend it to your needs.

@FND
Copy link
Contributor Author

FND commented Jul 26, 2015

We have content negotiation, just not the particular flavor you want.

At the risk of flogging a dead horse, allow me to explain (in a constructive manner, I hope) why I'm not convinced:

  • content negotiation means that there are potentially multiple representations of a resource (i.e. one and the same URL)
  • thus relegating non-HTML requests to a separate URL with a different routing pipeline (like :api) is not germane in this context
  • :request_from_forgery is indispensable for HTML requests
  • thus simply extending the :browser pipeline to also include non-HTML requests places an unnecessary burden on those very requests (like requiring a CSRF token when POSTing)

Does this make sense? I'm not trying to split hairs here, but honestly regard "my particular flavor" as an essential feature.

@lancehalvorsen
Copy link
Member

@FND you might consider publishing your solution to hex first, and then we could determine the level of necessity from there.

@josevalim
Copy link
Member

I think the solutions discussed here are more complex than they should be. I also believe Phoenix provides content negotiation just fine. We should not mix how pipelines work, which are based on the route match, with content negotiation.

@FND you want the same request to go through the same pipeline, right? Here is what I would do:

plug :default do
  plug :accepts, ~w(html json)
  plug :stuff_only_for_html
  plug :stuff_for_both
end

defp stuff_only_for_html(conn, []) do
  if conn.params["format"] == "html" do
    conn
    |> fetch_session([])
    |> fetch_flash([])
    |> protect_from_forgery([])
  else
    conn
  end
end

And that's it. If you want things to go through the same pipeline, then you will need to make things conditional on the format inside that pipeline. Someone could think about having format-based pipelines but that's a complete separate discussion and I am not convinced it is worth it as it would add complexity and it is not as useful as the route-based pipelines we have.

@FND
Copy link
Contributor Author

FND commented Jul 28, 2015

I like @josevalim's approach, as it's significantly simpler (and requires less boilerplate). For posterity, here's what I ended up with (after some flailing about and more IRC assistance):

defmodule Sampler.Router do
  use Sampler.Web, :router
  import Phoenix.Controller

  defp content_negotiation(%{params: %{"format" => "html"}} = conn, opts) do
    conn
    |> fetch_session(opts)
    |> fetch_flash(opts)
    |> protect_from_forgery(opts)
  end
  defp content_negotiation(conn, []), do: conn

  pipeline :default do
    plug :accepts, ["html", "json"]
    plug :content_negotiation
  end

  scope "/", Sampler do
    pipe_through :default

    get "/", PageController, :index
  end
end
defmodule Sampler.PageController do
  use Sampler.Web, :controller

  def index(%{params: %{"format" => "html"}} = conn, _params) do
    render conn, "index.html"
  end
  def index(%{params: %{"format" => "json"}} = conn, _params) do
    json conn, %{hello: "world"}
  end
end

Nevertheless, my original concern remains: It shouldn't be left to individual users to come up with this on their own (even taking into account my own ignorance, it's clearly not trivial) - or, if they're lucky, find this discussion.

@josevalim
Copy link
Member

@FND it would be awesome if you could write a blog post then and document this. :) It is impossible for Phoenix to handle all possible workflows someone can have in their stack, so the more we talk about, explain and document this, the more people will be able to customize it.

@chrismccord
Copy link
Member

Nevertheless, my original concern remains: It shouldn't be left to individual users to come up with this on their own

The fact that you can solve your requirements in about 10 LOC in the end is an amazing testament that we have built things properly. So I don't share your concern in this case. Your use-case isn't unreasonable, but in my last 6 years of consulting on dozens of apps, I've never needed such a setup. Having it available in 10 LOC if a users needs your style makes me feel extremely good about our setup.

The only other thing I will add here is José's solution solves you requirements, but if you want to use a plug in either pipeline that halts, you may need to move to my approach.

@FND
Copy link
Contributor Author

FND commented Jul 29, 2015

an amazing testament that we have built things properly

Oh, absolutely - despite my (well-intentioned) persistence regarding what amounts to convenience wrappers, I never doubted that the foundation is pretty solid here.

I'll see about writing this up soonish, but might want to dig a little deeper first.

if you want to use a plug in either pipeline that halts, you may need to move to my approach

Good point, thanks for pointing that out.

@jcoyne
Copy link

jcoyne commented Feb 22, 2017

An update for those who stumble upon this solution, with phoenix 1.2.1 you'll need to update @FND's solution to match on:

defp content_negotiation(%{private: %{phoenix_format: "html"}} = conn, opts)

@chrismccord
Copy link
Member

you should use Phoenix.Controller.get_format, then match on that value, instead of reaching into private assigns

@jcoyne
Copy link

jcoyne commented Feb 22, 2017

@chrismccord thanks for the tip. I'm a elixir noob. Would I call Phoenix.Controller.get_format within content_negotiation/2, or is there a clever way to write a matching expression?

@chrismccord
Copy link
Member

def content_negotiation(conn opts) do
  negotiate(conn, opts, Phoenix.Controller.get_format(conn))
end
defp negotiate(conn, opts, "html") do ...
defp negotiate(conn, opts, "json") do ...

@jcoyne
Copy link

jcoyne commented Feb 22, 2017

@chrismccord Thanks, that's exactly what I did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants