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

feat: OpenAPI integration #984

Open
wants to merge 51 commits into
base: master
Choose a base branch
from

Conversation

NexVeridian
Copy link
Contributor

@NexVeridian NexVeridian commented Nov 13, 2024

related #855

cargo check -F all_openapi
cargo nextest run --test-threads 1 -F all_openapi

TODO:

  • config.yaml
    • maybe no enable: true since that's handled by the feature
      openapi:
        redoc:
          !Redoc
            url: /redoc
            spec_json_url: /redoc/openapi.json
            spec_yaml_url: /redoc/openapi.yaml
        scalar:
          !Scalar
            url: /scalar
            spec_json_url: /scalar/openapi.json
            spec_yaml_url: /scalar/openapi.yaml
        swagger:
          !Swagger
            url: /swagger-ui
            spec_json_url: /api-docs/openapi.json
            spec_yaml_url: /api-docs/openapi.yaml
    • finish .merge(Redoc::with_url("/redoc", api.clone()))
    • openapi.josn and openapi.yaml endpoints for all types
  • split feature openapi into feature all_openapi swagger-ui redoc scalar
  • rstest feature flagged cases
  • SecurityAddon
    • put impl Modify for SecurityAddon somewhere, maybe with config
    • set the jwt token location
  • tests
    • update src/tests_cfg/db.rs:86:1
    • src/tests_cfg/config.rs
    • config from file test_from_folder_openapi()
    • snapshots
  • docs
  • maybe auto fill / wrap utoipa::path if possible
  • check that get in get(get_action_openapi) is still grabbed with routes!(get_action_openapi)
  • fix AppContext - check that api_router.routes(method.with_state::<AppContext>(())) doesn't break the ctx with .layer
  • cargo test is broken with JWT_LOCATION.get_or_init, nextest works correctly
  • codegen
    • cargo loco generate controller --openapi
    • adding #[derive[utoipa::ToSchema)] if used in utoipa::path
    • routes! macro

cc @DenuxPlays

@DenuxPlays
Copy link
Contributor

Looks like a good start.

I think the yaml also should contain a few other things:

  • openapi and ui endpoints (I am not sure if this is want you mean in your description)
  • whether it serves json, yaml or both (Perhaps this can be controlled by using features to exclude unnecessary dependencies)

@NexVeridian NexVeridian force-pushed the OpenAPI-integration branch 4 times, most recently from 9055d99 to 3e106d0 Compare November 22, 2024 09:21
@DenuxPlays
Copy link
Contributor

Hey @NexVeridian

Can I help you finish this pr?

@NexVeridian
Copy link
Contributor Author

@DenuxPlays yeah of course

@DenuxPlays
Copy link
Contributor

How can I help?

Also just one Note to your pr description.
I wouldn't wrap the utoipa path macro.
Its working very well just how it is and wrapping it would make upgrading and maintaining very difficult

@NexVeridian
Copy link
Contributor Author

NexVeridian commented Nov 27, 2024

Also just one Note to your pr description. I wouldn't wrap the utoipa path macro. Its working very well just how it is and wrapping it would make upgrading and maintaining very difficult

thanks!

this would be nice if possible:

impl Routes {
/// .add_openapi(routes!(get_action, post_action))
pub fn add_openapi(mut self, method: UtoipaMethodRouter<AppContext>) -> Self {}
}

any of the unchecked ones in the pr description would be great:
maybe cargo loco generate controller --openapi and docs first

@kaplanelad
Copy link
Contributor

@NexVeridian, let us know when it is ready for review.
in the meantime, I am moving it to draft

@kaplanelad kaplanelad marked this pull request as draft January 8, 2025 08:07
@NexVeridian
Copy link
Contributor Author

@kaplanelad this is ready for review

I think everything looks good, except for the cargo loco generate controller --openapi I think the easiest would be to drop 00e72b9 and leave` for another pr, I haven't used the codegen before

@kaplanelad
Copy link
Contributor

kaplanelad commented Jan 15, 2025

First, thank you so much for all the hard work! This feature is fantastic for us.

Before starting the code review, I want to focus on the user journey when adding it to their project.

I’ve gone through your documentation, and here’s my feedback:

Creating a New Template

I ran the command:

LOCO_DEV_MODE_PATH="[PATH TO LOCAL LOCO]" loco new

Updating Cargo.toml

  1. After creating the template, I went to Cargo.toml and added all_openapi as a feature.
    In your docs, you mentioned:
loco-rs = { version = "0.13", features = ["scalar"] }

Can you please adjust this to include the feature under loco-rs = { workspace = true }?

  1. I need to add the utopia crate into my project (this is not documented). can you try avoid adding this crate and expose this create directly from loco?

Implementing initial_openapi_spec as hook

When I removed set_jwt_location_ctx, I encountered this panic:

thread 'main' panicked at /tmp/open-api/loco/src/auth/openapi.rs:31:24: called `Option::unwrap()` on a `None` value

Route Definitions

When defining routes, I had to add use utoipa_axum::routes; to my code. It wasn’t clear from the docs that I needed this crate.
Also, can we expose utoipa_axum directly from loco?

Generator Improvements

I think it would be better to add an --openapi flag directly to the controller, instead of creating a separate generator command for it. The same applies to scaffolding.

Removing the Demo App

We plan to remove the demo app from our repo. Please refer to the related issue.
I think the example project would be better suited as a dedicated repository under the loco organization.

Could you please open a new repo and commit the loco new files to master (without open API implementation)?
Then, create a PR detailing the steps users need to follow to implement this feature in their projects. This will help us understand what improvements we can make.

@kaplanelad
Copy link
Contributor

Hi @NexVeridian, instead of working on demo, please open a PR here with the implementation.
Make sure to update the Cargo.toml file to point to your branch.

@kaplanelad kaplanelad marked this pull request as ready for review January 18, 2025 15:03
@DenuxPlays
Copy link
Contributor

Just one small thing I noticed.
Structs like these: https://github.com/NexVeridian/loco/blob/d72c46032a93a676004ade03266f76e6268b26bc/src/controller/views/pagination.rs would need to implement/derive the ToSchema trait right?

@DenuxPlays
Copy link
Contributor

Also for the PaginationQuery there needs to be an derive(IntoParameters)

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.

3 participants