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

OpenApiSpex.PathItem.from_routes breaks the contract #540

Closed
David-Klemenc opened this issue May 25, 2023 · 6 comments
Closed

OpenApiSpex.PathItem.from_routes breaks the contract #540

David-Klemenc opened this issue May 25, 2023 · 6 comments

Comments

@David-Klemenc
Copy link
Contributor

Dialyzer is saying that this breaks the contract:

OpenApiSpex.PathItem.from_routes([
  %{:opts => [], :plug => MockController.ListSomething, :verb => :get},
  %{:opts => [], :plug => MockController.ModifySomething, :verb => :put},
  %{:opts => [], :plug => MockController.NewSomething, :verb => :post}
])

breaks the contract
([route()]) :: nil | t()

Which is correct, because the path type is defined as:

@type route ::
          %{path: String.t(), verb: atom, plug: atom, opts: any}
          | %{path: String.t(), verb: atom, plug: atom, plug_opts: any}

in v3.16.1 it was defined as:

@type route ::
          %{verb: atom, plug: atom, opts: any}
          | %{verb: atom, plug: atom, plug_opts: any}

The problem was introduced here: 534

A more full example:

%OpenApi{
      info: %Info{
        title: "Mock",
        description: "mock REST backend",
        version: "1.0"
      },
      components: %Components{
        securitySchemes: %{"authorization" => %SecurityScheme{type: "http", scheme: "bearer"}}
      },
      paths: %{
        "/api/mock" =>
          OpenApiSpex.PathItem.from_routes([
            %{verb: :get, plug: MockController.List, opts: []},
            %{verb: :put, plug: MockController.Modify, opts: []},
            %{verb: :post, plug: MockController.New, opts: []}
          ])
      }
    }
    |> OpenApiSpex.resolve_schema_modules()
@mbuhot
Copy link
Collaborator

mbuhot commented May 25, 2023

Can we make path optional in the route type?

Something like:

@type route ::
          %{optional(:path) => String.t(), verb: atom, plug: atom, opts: any}
          | %{optional(:path) => String.t(), verb: atom, plug: atom, plug_opts: any}

@zorbash
Copy link
Contributor

zorbash commented May 30, 2023

Applied the suggestion #540 (comment) in the example app and it does fix the issue. @David-Klemenc Please confirm it fixes the issue for your application as well.

@David-Klemenc
Copy link
Contributor Author

Yes this solves the problem for me too.

I was looking at the example code - and was thinking of rewriting it so it uses mnesia instead of sqlite, this would get rid of a couple of dependencies and make it a bit easier to run and maintain? (the rewrite to mensia would be easy since there is no pagination in the examples). Would you like me to try this?

@zorbash
Copy link
Contributor

zorbash commented May 30, 2023

@David-Klemenc Which mix dependencies would we get rid of? With SQLite becoming more popular in the Elixir ecosystem (source) it's probably a good idea to use it in the example app.

@David-Klemenc
Copy link
Contributor Author

@zorbash these two:

{:ecto, "~> 2.2"},
{:sqlite_ecto2, "~> 2.4"},

Maybe you are correct - my thought was here we want to demonstrate the open_api part? (but in case someone uses the example as a seed - then ecto would be more appropriate) It is a slightly higher barrier to entry with ecto.

@mbuhot
Copy link
Collaborator

mbuhot commented May 31, 2023

It would be helpful to regenerate the example phoenix app using a recent version of the phoenix generator.
Something like:

mix phx.new new_phoenix_app --database=sqlite3 --no-assets --no-html --no-gettext --no-dashboard --no-mailer

The example plug app can probably be updated to ecto 3 and sqlite_ecto3 without too much change.

Eventually it would be great to have an example of marshalling Ecto.Changeset errors into an error response that conforms to an API Error schema definition.

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

3 participants