ProbablyClem / utoipauto

Rust Macros to automate the addition of Paths/Schemas to Utoipa crate, simulating Reflection during the compilation phase
Apache License 2.0
108 stars 10 forks source link

Macro errors with `expected ','` #7

Closed domenicquirl closed 7 months ago

domenicquirl commented 8 months ago

Hi, and thanks for making this crate!

When trying to replace my (working) manual #[openapi] annotation with utoipauto, I'm getting the error

error: expected `,`
  --> service-libs/web-backend/src/api/mod.rs:23:1
   |
23 | #[utoipauto::utoipauto(paths = "./service-libs/web-backend/src/api/internal")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in the attribute macro `utoipauto::utoipauto` (in Nightly builds, run with -Z macro-backtrace for more info)

With -Z macro-backtrace, the compiler helpfully traces the error back to the macro:

::: /<PATH>/index.crates.io-6f17d22bba15001f/utoipauto-macro-0.1.4/src/lib.rs:15:1
   |
15 | / pub fn utoipauto(
16 | |     attributes: proc_macro::TokenStream, // #[utoipauto(paths = "(MODULE_TREE_PATH => MODULE_SRC_PATH) ;")]
17 | |     item: proc_macro::TokenStream,       // #[openapi(paths = "")]
18 | | ) -> proc_macro::TokenStream {
   | |____________________________- in this expansion of `#[utoipauto::utoipauto]`

The project is part of a workspace, so I'm using #[utoipauto::utoipauto(paths = "./service-libs/web-backend/src/api")] to make utoipauto look at the correct source path (there's nothing at ./src).

I've tried to figure out where the error occurs or which of my types or handlers causes it, but when I point it at one of the files with 1 ToResponse derive, 2 ToSchemas and 1 handler, the error only goes away if I remove the utoipa macros / derives from all of them. I also couldn't manage to reproduce the error by modifying your tests, which is weird. Could it be that the different file structure in a workspace messes things up?

ProbablyClem commented 8 months ago

Hello, could you please show me the full declaration of your OpenApi struct please ? This whole block

#[utoipauto]
#[derive(OpenApi)]
#[openapi(
    paths(
        test_controller::service::func_get_1,
    ),
)]
pub struct ApiDoc;

This may be an issue happening with matching auto import and manual imports (which should be supported)

domenicquirl commented 8 months ago

@ProbablyClem the full code was something like

#[derive(Debug, OpenApi)]
#[openapi(
    modifiers(&SecurityAddon),
    info(
        version = "1",
    ),
    tags(
        (name = "auth", description = "Endpoints for authentication and authorization."),
    ),
    paths(
        internal::permissions::get
    ),
    components(
        responses(
            internal::permissions::PermissionsResponse,
        ),
        schemas(
            crate::permissions::Permission,
            crate::permissions::Role,
            internal::permissions::PermissionInfo,
            internal::permissions::RoleInfo,
        )
    )
)]
pub struct ApiDoc;

But I removed all paths and components when replacing them with the #[utoipauto] invocation, so I did not declare paths manually while the struct was annotated with your macro.

ProbablyClem commented 8 months ago

Is it part of a public repo that I can check ? Could you put utoipauto back and run cargo-expand to see the generated code please ?

domenicquirl commented 8 months ago

Is it part of a public repo that I can check ?

It is not, unfortunately. But I've run cargo expand (I had previously tried to expand via rust-analyzer, but that just gives up) and this is what it outputs:

#[openapi(
    paths(
        crate::web-backend::src::api::v1::users::login,
        crate::web-backend::src::api::internal::permissions::get,
    ),
    components(
        schemas(
            crate::web-backend::src::api::v1::users::Username,
            crate::web-backend::src::api::v1::users::AuthenticationData,
            crate::web-backend::src::api::v1::users::RefreshTokenOptions,
            crate::web-backend::src::api::v1::users::RefreshTokenMethod,
            crate::web-backend::src::api::internal::permissions::PermissionInfo,
            crate::web-backend::src::api::internal::permissions::RoleInfo,
            crate::web-backend::src::permissions::Permission,
            crate::web-backend::src::permissions::Role,
        ),
        responses(
            crate::web-backend::src::api::v1::users::AuthenticationResponse,
            crate::web-backend::src::api::internal::permissions::PermissionsResponse,
        )
    ),
    modifiers(&SecurityAddon),
    info(version = "1"),
    tags(
        (name = "auth", description = "Endpoints for authentication and authorization."),
    ),
)]
pub struct ApiDoc;

I had added some more types in the meantime, but that seems reasonable and like what I would expect on first glance, except that the correct paths would be just crate::XY and not crate::web-backend::src::XY. The compiler probably can't deal with the hyphen in web-backend and thinks the path is over, so it wants a , to transition to the next path!

Are you generating these based on the paths that are given to the macro? Maybe there's a different way for me to specify where to search relative to the source file containing the macro 🤔

domenicquirl commented 7 months ago

Yeah I think this assumption:

https://github.com/ProbablyClem/utoipauto/blob/e2ca0d84228e32b2e032e38812b7e1e59d3b03a9/utoipauto-core/src/file_utils.rs#L84-L86

is not general enough. It's only true if the structure of your workspace is such that it directly contains all the crates. It doesn't work for me because the crates in the workspace are "grouped" into an intermediate level of folders, and it also wouldn't work for the thing people do sometimes where all the crates are in the same folder, but it's not the workspace folder but workspace/crates.

The fully correct thing would probably be to actually resolve the Cargo.toml responsible for the file / module to determine the crate root. But I think wrangling the paths from the macro invocation is fine if instead of hardcoding src to have to appear as path segment 1, the logic was more like "skip until you hit src or tests, then convert the remaining file path to crate::XYZ".

Alternatively, maybe the library could use the "old" syntax to support this? If I read this correctly, the left-hand side of a crate::module::foo => ./category/crate/src/module/foo is currently ignored. But you could also use that information to take the discovered paths relative to the file path and append them to the module path on the left that the user has given (modulo mod.rs being on the same level).

ProbablyClem commented 7 months ago

I would rather not bring back the old syntax (that comes from the forked repo), since it's really annoying to have to specify the file paths and the source paths. Obviously, we might found a case someday where we absolutely need it, but I would like to keep it last resort.

I have a lot of work to do currently, so feel free to submit a PR if you can. Thanks for your help

domenicquirl commented 7 months ago

Ah, if anything I thought using the old syntax over the new one would be optional if you need to specify more difficult file-to-module-path relationships. But I think I've been able to find a fix with the first approach without requiring the old syntax in #8.

Thanks a lot for your help and responsiveness on this!