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

Proof of concept Json-API with OpenAPI support (take 2) #59

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mbuhot
Copy link
Contributor

@mbuhot mbuhot commented Nov 24, 2022

This draft PR is a little different to #58, exploring how OpenAPI would work if the application used OpenApiSpex to define the outline of the API spec, and use AshJsonApi helpers to fill in the schemas and operations.

The corresponding branch for AshJsonApi is: https://github.com/team-alembic/ash_json_api/tree/feat/open-api-spex-helpers

Contributor checklist

  • Bug fixes include regression tests
  • Features include unit/acceptance tests

Using AshJsonApi.OpenApi helpers to generate OpenAPI
json_api do
prefix "/json_api"
serve_schema? true
open_api {AshHq.Docs.OpenApi, :spec, []}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used an MFA here as the most flexible option, but I'm not sure if it feels like idiomatic Ash?
Just the module name would work, as long as it exposes a spec/0 callback.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "ash way" is typically {:spark_behaviour, BehaviourName}, which will allow them to pass Module or {Module, opts}, and you'll always get {Module, opts} for the value, at which point you can say Module.spec(opts).

@@ -0,0 +1,35 @@
defmodule AshHq.Docs.OpenApi do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indicative of the kind of module Ash users would need to write for the outline of their OpenAPI document.

I like that this gives the users a chance to add extra schemas, tags, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Its a bit boilerplate-y, but its also very likely that people will eventually want to add their own custom endpoints and things like that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll want these functions to take lists of APIs if they don't already. Then they can set @apis in the top of the file. I'd also consider something like this:

    %OpenApi{
      info: %Info{
        title: "AshHQ Docs API",
        version: "1.1"
     },
     servers: [Server.from_endpoint(AshHqWeb.Endpoint]
    }
    |> AshJsonApi.OpenApi.extend_open_api_spex(@apis)

Either way, this is likely nitpicking as I think you've found a good balance between "boilerplate" and extensibility.

@@ -0,0 +1,15 @@
defmodule AshHqWeb.DocsJsonApiRouter do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made some modifications to the AshJsonApi.Api.Router to allow additional routes to be defined, for things like SwaggerUI / ReDoc. These can also go in the Phoenix Router, I just thought it was tidy to put all the JsonAPI/OpenAPI related things in one file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smart 👍🏻

@zachdaniel zachdaniel closed this Nov 24, 2022
@zachdaniel zachdaniel reopened this Nov 24, 2022
@zachdaniel
Copy link
Collaborator

Sorry, accidentally clicked the close button 😆 Usage wise, looks like a great pattern 👍🏻.

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

Successfully merging this pull request may close these issues.

2 participants