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

Allow defining operations by module attributes #76

Closed
hauleth opened this issue Jan 3, 2019 · 10 comments
Closed

Allow defining operations by module attributes #76

hauleth opened this issue Jan 3, 2019 · 10 comments

Comments

@hauleth
Copy link
Contributor

hauleth commented Jan 3, 2019

Example implementation that I have used in my project (still lacks a little bit options, but overall idea can be seen):

defmodule KokonWeb.Rest do
  alias OpenApiSpex.Operation

  defmacro __using__(_opts) do
    quote do
      alias KokonWeb.Rest.Schema
      alias OpenApiSpex.Operation

      plug(OpenApiSpex.Plug.Cast)
      plug(OpenApiSpex.Plug.Validate)

      @on_definition KokonWeb.Rest
      @before_compile KokonWeb.Rest

      Module.register_attribute(__MODULE__, :parameter, accumulate: true)
      Module.register_attribute(__MODULE__, :response, accumulate: true)

      Module.register_attribute(__MODULE__, :open_api_operations, accumulate: true)
    end
  end

  def __on_definition__(_env, _type, :open_api_operation, _args, _guards, _body), do: nil
  def __on_definition__(_env, _type, :action, _args, _guards, _body), do: nil

  def __on_definition__(%Macro.Env{module: mod}, :def, name, _args, _guards, _body) do
    parameters = Module.delete_attribute(mod, :parameter)
    response = Module.delete_attribute(mod, :response)
    {summary, doc} = docs(Module.get_attribute(mod, :doc))

    operation =
      %Operation{
        summary: summary || "TODO",
        description: doc,
        operationId: module_name(mod) <> ".#{name}",
        parameters: parameters,
        responses: Map.new(response)
      }

    Module.put_attribute(mod, :open_api_operations, {name, operation})
  end

  def __on_definition__(_env, _type, _name, _args, _guards, _body), do: nil

  defmacro __before_compile__(_env) do
    quote unquote: false do
      for {name, operation} <- Module.delete_attribute(__MODULE__, :open_api_operations) do
        def open_api_operation(unquote(name)), do: unquote(Macro.escape(operation))
      end
    end
  end

  defp docs(nil), do: {nil, nil}

  defp docs({_, doc}) do
    [summary | _] = String.split(doc, ~r/\n\s*\n/, parts: 2)

    {summary, doc}
  end

  defp module_name(mod) when is_atom(mod), do: module_name(Atom.to_string(mod))
  defp module_name("Elixir." <> name), do: name
  defp module_name(name) when is_binary(name), do: name
end

This causes controller to look like this:

defmodule KokonWeb.Rest.Controllers.Schedule do
  use KokonWeb.Rest

  @doc "List schedule"
  @response {200, Operation.response(
    "Submissions",
    "application/json",
    KokonWeb.Rest.Schema.Submissions
  )}
  def index(conn, _params) do
    {:ok, submissions} = Kokon.Submissions.all()

    json(conn, submissions)
  end

  @doc "Create new submission"
  @parameter Operation.parameter(:title, :query, :string, "Submission title",
    required: true
  )
  @parameter Operation.parameter(
    :abstract,
    :query,
    :string,
    "Submission description",
    required: true
  )
  @response {200, Operation.response(
    "Submissions",
    "application/json",
    KokonWeb.Rest.Schema.Submission
  )}
  def create(conn, %{title: title, abstract: abstract}) do
    with {:ok, submission} <-
      Kokon.Submissions.create(%{title: title, abstract: abstract}) do
      json(conn, submission)
    end
  end
end

I am opening this as an issue instead of PR as I would like to know opinions about this beforehand.

@moxley
Copy link
Contributor

moxley commented Jan 3, 2019

@hauleth: What are the advantages you see from this approach?

@hauleth
Copy link
Contributor Author

hauleth commented Jan 3, 2019

@moxley for me it is "more ergonomic" and I can reuse ExDocs as OAS documentation. But both of these are very opinionated. That is why I opened this issue, to know what other people think about such approach and whether it is worth to be submitted as a PR or not.

@moxley
Copy link
Contributor

moxley commented Jan 4, 2019

@hauleth: Now that you mention it, it does seem to read more ergonomically, at least for your example. That's one possible advantage.

When you say you can reuse ExDocs as OAS documentation, how would that work? ExDoc understands the @doc attribute. But your approach introduces @parameter and @response attributes to describe details of the operations. How would that work with
ExDoc?

@hauleth
Copy link
Contributor Author

hauleth commented Jan 4, 2019

@moxley I use @doc as source for summary and description fields. I would like also to use @doc deprecated: true, but elixir-lang/elixir#8095 is blocking there as currently there is no safe way to fetch documentation metadata for the function at the compile time.

@hauleth
Copy link
Contributor Author

hauleth commented Jan 4, 2019

I could parse @doc more thoughtfully, but I do not think it would result in something readable.

@moxley
Copy link
Contributor

moxley commented Jan 4, 2019

@hauleth: I'm curious, why do you want to document a web API with ExDoc? Open API Spex already generates web API docs. That's a major benefit of it.

@hauleth
Copy link
Contributor Author

hauleth commented Jan 4, 2019

@moxley it is not that I want to document API with ExDoc, I want to reuse the function documentation as description. These are slightly different things.

@fenollp
Copy link
Contributor

fenollp commented Jan 5, 2019

I like this. It looks like what phoenix_swagger does.
How do you go about defining a 200 response + a default one? Just by using 2 attributes?

@moxley
Copy link
Contributor

moxley commented Jan 21, 2019

@hauleth Sorry for the long delay in responding to this. I appreciate the elegance of the syntax and the small benefit it brings in DRYing up endpoint/action titles in the docs. I feel it has some drawbacks:

  1. This is a different syntax than used in non-operation schemas. The operations would use this new syntax, while other schemas in the codebase would use the old syntax. Is this the intent, or do you see using this new syntax for the non-operation schemas too?
  2. It strays far from the structure of Open API schemas. I find one of the major selling points of Open API Spex is that the structure of the schemas are very similar to the structure of Open API itself.
  3. In the user documentation for showing how to define operation schemas in the controller, would you present both syntaxes, or just this new one? I think showing both would dilute the educational message.

My recommendation would be to provide this new syntax as an add-on alternative, but not as the canonical way to define operations.

Your proposal reminds me of a concern I have with the current way of defining operation schemas. I would like to avoid this meta-programming:

  @spec open_api_operation(any) :: Operation.t
  def open_api_operation(action), do: apply(__MODULE__, :"#{action}_operation", [])

Also, I don't like how messy the controller gets when the operation schemas are fully defined there. The definition is considerably bigger than the action function it references.

  def show_operation() do
  ... # 10-20 lines of code
  end
  def show(conn, %{"id" => id}) do
    {:ok, user} = MyApp.Users.find_by_id(id)
    json(conn, 200, user)
  end

In the codebase we're working on, we've pulled out the operation schemas from the controller to separate modules. The controllers now look more like this:

  def open_api_operation(action), do: MyApp.Schemas.UserOperations.operation(action)

  def show(conn, params) do
    # ...
  end

  def create(conn, params) do
    # ...
  end

This eliminates the meta-programming, and it cleans up the controller. A small downside is that there is no longer a reference to the operation next to each action function as there was before. I feel this drawback is small enough to outweigh the benefits.

@mbuhot
Copy link
Collaborator

mbuhot commented Aug 22, 2020

Closing this issue as we figure out what to do about releases in #242 / #265

@mbuhot mbuhot closed this as completed Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants