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

Expose Swagger-UI in addition to API spec #174

Closed
wants to merge 1 commit into from

Conversation

artem-korolev
Copy link

@artem-korolev artem-korolev commented Mar 30, 2020

Hi Ravi,

I have a proposal to expose Swagger-UI in addition to API spec. It makes it really handful in development environment.
In production I can use some docker instance with Swagger-UI aggregating several api specs from several api urls.
But in development environment it is overkill to configure Swagger UI separatelly for each service.

So I extended your very cool plugin to automatically host swagger-ui on "/<json_spec_path>/swagger-ui.html". I strongly believe, that if you already provide to client api spec in json format, then you want to give them swagger-ui as well. And some of them (corporate users), for example, don't want to use public services to browse their private API. Cause of security reasons

Regardless implementation (i am currently 3 days Rust programmer, so it tooked 2 hours for me to write this 10 lines of code, but it works! so don't beat me please :D):

  • I'm not sure if concatenate of path with "swagger-ui.html" is just enough. Maybe its needed to predicate it with root service url - idk. But want to know

  • I love askama template engine. Cause it embeding templates into executable. And checks all parameters used in templates for their existance in compile time. No chance for runtime errors.

  • SwaggerUIHandler(path.to_string()) - have no idea how to avoid this to_string call :D

  • I copy pasted swagger-ui.html directly from its original source. From here - https://github.com/swagger-api/swagger-ui/tree/master/dist
    And I embedded all javascripts directly inside html to just speed up the process for me right now. Probably I need (or someone else) to fine tune there favicons and so.

  • And one more thing about security. swagger-ui should not use any external resources and make call to external web resources, because of security reasons. I think that should be taken as a requirement for this plugin

Thank you for great job!

@wafflespeanut wafflespeanut self-requested a review March 30, 2020 16:51
Copy link
Collaborator

@wafflespeanut wafflespeanut left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! I have a few comments.

@@ -18,6 +18,7 @@ paperclip-core = { path = "../../core", version = "0.1.0", features = ["actix"]
paperclip-macros = { path = "../../macros", version = "0.2.0", features = ["actix"] }
parking_lot = "0.10"
serde_json = "1.0"
askama = "0.9"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make this an optional dependency and add a feature "swagger-ui" below, which then depends on askama

actix_web::web::resource(path)
.route(actix_web::web::get().to(SpecHandler(self.spec.clone()))),
);
self.inner = self
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename the method to with_swagger_spec_at, as it feels more appropriate than having both HTML and JSON in a method that's named "json" - once that's done, we should also update the book.

actix_web::web::resource(path)
.route(actix_web::web::get().to(SpecHandler(self.spec.clone()))),
)
.service(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This call can now be feature-gated with #[cfg(feature = "swagger-ui")]

Copy link
Author

Choose a reason for hiding this comment

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

Hi
unfortunetelly im stuck in here. I can configure Cargo.toml with that feature.
But this works only when "swagger-ui" feature is enabled. When its disabled I'm getting compile error, cause service expects 1 parameter:

self.inner = self
            .inner
            .service(
                actix_web::web::resource(path)
                    .route(actix_web::web::get().to(SpecHandler(self.spec.clone()))),
            )
            .service(
                #[cfg(feature = "swagger-ui")]
                actix_web::web::resource(path.to_owned() + "/swagger-ui.html")
                    .route(actix_web::web::get().to(SwaggerUIHandler(path.to_string()))),
            );
        self

if I move feature-gate up (before "service" ), then I'm getting syntax error - its not supported by the language.

Of course tried to make separate call to self.inner, but, as you can guess, compiler does not allow it. ownership problem. I'm a bit tiered for today. Need to repeat this lection about ownership in rust.
But if you can help here, I appreciate that

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can try this?

self.inner = self
            .inner
            .service(
                actix_web::web::resource(path)
                    .route(actix_web::web::get().to(SpecHandler(self.spec.clone()))),
            );
#[cfg(feature = "swagger-ui")]
self.inner = self
            .inner
            .service(
                actix_web::web::resource(path.to_owned() + "/swagger-ui.html")
                    .route(actix_web::web::get().to(SwaggerUIHandler(path.to_string()))),
            );
self

Copy link
Author

@artem-korolev artem-korolev May 14, 2020

Choose a reason for hiding this comment

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

Hi, sorry for such a long delay. Was too far from all of this.
But yes. I tried this before. And tried this one more time right now. It raises compile error:

error[E0658]: attributes on expressions are experimental
241 |         #[cfg(feature = "swagger-ui")]
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries. In that case, it could be moved to two private helper functions below. One is no-op, for when the feature is disabled, another which contains the actual logic for the feature.

Copy link
Author

Choose a reason for hiding this comment

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

yes. right. I will do this

@wafflespeanut
Copy link
Collaborator

I'm not sure if concatenate of path with "swagger-ui.html" is just enough.

I'm planning to support other web frameworks in the future, so this won't reside in where it's at right now. In the current state, the UI doesn't seem like it's configurable. While it's certainly appreciated to have a simple method to get the UI up and running, there should be an option for users to decide how the UI files should be served (CORS headers, custom icon, gated by authentication, etc.).

I love askama template engine. Cause it embeding templates into executable. And checks all parameters used in templates for their existance in compile time. No chance for runtime errors.

I think askama feels a little bit heavy for us, given that we're only using one variable and it's not that complex. I'm using tinytemplate in the build script to help with rendering, so I'll probably move to it later.

I embedded all javascripts directly inside html to just speed up the process for me right now.

Maybe we should just add a few more options to the swagger handler, expose it from the plugin and let the users decide how they wish to configure/mount it in their app.

@oli-cosmian
Copy link
Contributor

I love this! Just what I need.

General question Isn't this completely independent of paperclip? As in this could live in its own crate and simply be added as another endpoint just like users add their own endpoints?

I mean, paperclip's docs could still describe how to set it up to work together, but it doesn't need to be part of paperclip.

@wafflespeanut
Copy link
Collaborator

General question. Isn't this completely independent of paperclip? As in this could live in its own crate and simply be added as another endpoint just like users add their own endpoints?

As for me, a number of web framework plugins support serving the UI, so I think paperclip should have this feature. While I'm okay with having the static files in this crate (because it's feature-gated and future plugins will find this useful), it doesn't necessarily have to be in this crate. It can live in its own crate.

@artem-korolev
Copy link
Author

I think askama feels a little bit heavy for us, given that we're only using one variable and it's not that complex. I'm using tinytemplate in the build script to help with rendering, so I'll probably move to it later.

Yes, sir! I'll check tinytemplate and try to use it

@wafflespeanut wafflespeanut force-pushed the master branch 2 times, most recently from c995a1e to a64059c Compare June 13, 2020 16:55
strohel added a commit to strohel/locations-rs that referenced this pull request Jul 21, 2020
This is far from perfect, but it works!
- This is version Swagger 2.0 (current is OpenAPI 3.0)
  - Paperclip dev says they would work on v3.0 for next release actix/actix-web#310 (comment)
- It doesn't seem to show descriptions of API operations (sigh), but it does
  show descriptions of *some* output schema fields.
- It even lists possible error codes and descriptions, but doesn't mention
  schema of the error response (undestandably).
- No Swagger UI is exposed, but there is an open MR for that in Paperclip:
  paperclip-rs/paperclip#174
- `language` enum varians are nicely listed and correctly lowercased.

Given that Paperclip says it is WIP, this seems rather fine.

They also claim (about the actix support) that *While it's not feature complete,
you can rely on it to not break your actix-web flow.* And diff of this commit
confirms that, is is basically just addition of some derives, attributes and
wrappers.
@cfvescovo
Copy link

I definitely need this! It's such a great idea!
Let me know if you need any help. I would be glad to contribute.

@artem-korolev
Copy link
Author

I definitely need this! It's such a great idea!
Let me know if you need any help. I would be glad to contribute.

you are welcome to finalize this. i switched to another more important for me things. so i have no idea, when I can back to this. you can check the content of pull request - its very simple, and read discussion here about problems that it have. I thinks there was only one or two thing left to improve before merge

@cfvescovo
Copy link

Okay, I will open a new pull request soon

@cfvescovo
Copy link

I have been really busy in the last few days.
I hope I will have enough time to open the new PR tomorrow.

@cfvescovo cfvescovo mentioned this pull request Sep 26, 2020
@cfvescovo
Copy link

I opened the new PR.
See #228

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.

4 participants