sugar-framework / sugar

Modular web framework for Elixir
https://sugar-framework.github.io/
MIT License
430 stars 29 forks source link

Figure out incompatibility with Plug.ErrorHandler #69

Closed slogsdon closed 9 years ago

slogsdon commented 9 years ago

Also affects Plug.Debugger

slogsdon commented 9 years ago

Seems there is an issue with using defoverridable twice for a function without defining the function between them.

In HttpRouter, there is something similar to:

defmodule HttpRouter do
  ...
  defmacro __before_compile__(_env) do
    quote do
      def init(opts) do
        opts
      end

      def call(conn, opts) do
        do_call(conn, opts)
      end

      defoverridable [init: 1, call: 2]
    end
  end
  ...
end

And in Plug.ErrorHandler and Plug.Debugger, there is:

defmodule Plug.ErrorHandler do
  ...
  defmacro __before_compile__(_env) do
    quote location: :keep do
      defoverridable [call: 2]

      def call(conn, opts) do
        try do
          super(conn, opts)
        catch
          kind, reason ->
            Plug.ErrorHandler.__catch__(conn, kind, reason, &handle_errors/2)
        end
      end
    end
  end
  ...
end

Since we use both of them, a router's compiled code essentially becomes:

defmodule Router do
  ...
  def call(conn, opts) do
    ...
  end
  defoverridable [call: 2]
  defoverridable [call: 2]
  def call(conn, opts) do
    ...
  end
  ...
end

The double defoverridable [call: 2] is the issue. The first makes the first call/2 lazily defined, and the second attempts to do the same thing, but since it technically hasn't been defined yet since call/2 is now lazy, it fails.

The simple fix is to remove the defoverridable from HttpRouter as we were just providing a convenience with that call and educate end-users that they will need to include it in their code if they ever need to customize those Plug behaviour functions.

slogsdon commented 9 years ago

Fixed as of 449bed1b01d967020cf33529f52f414bbd7ce2d7