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(swagger-ui): cache swagger zip #1214

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

Conversation

thewh1teagle
Copy link

@thewh1teagle thewh1teagle commented Nov 21, 2024

Enabled with

cargo build -p utoipa-swagger-ui --features "cache,reqwest"

Resolve #1209 (comment)

Copy link
Owner

@juhaku juhaku left a comment

Choose a reason for hiding this comment

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

Still needs:

  • Update to CHANGELOG.md
  • Update docs and README.md

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe I am not educated enough, do we need all that C crap and pointer juggling? Aren't there a better and perhaps more simply way to the the same result or at least similar result?

As I understand this tries to use the users local cache dir for caching the data. I'd rather place it just the OUTPUT dir of the crate in question. Or target dir since the OUTPUT dir can change?

Copy link
Owner

Choose a reason for hiding this comment

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

I'd expect at least some level of documentation what all that black magic does. Whats more is that I can guarantee that there are few that even dare to touch this code and I don't want to maintain this code.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe I am not educated enough, do we need all that C crap and pointer juggling? Aren't there a better and perhaps more simply way to the the same result or at least similar result?

I also doesn't like that. though it's that expressive because the original dev wanted to support as many platforms as he can. but it's just for caching and I guess windows/linux/macos support is enough so we can simplify it into few lines

Copy link
Author

Choose a reason for hiding this comment

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

As I understand this tries to use the users local cache dir for caching the data. I'd rather place it just the OUTPUT dir of the crate in question. Or target dir since the OUTPUT dir can change?

We shouldn't use OUTPUT dir the whole idea is to cache the downlloaded file in known location that will be still cached even if we rebuild the whole project / delete target folder and rebuild

Copy link
Author

Choose a reason for hiding this comment

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

I'd expect at least some level of documentation what all that black magic does. Whats more is that I can guarantee that there are few that even dare to touch this code and I don't want to maintain this code.

It just returns the cache directory on different platforms

Copy link
Owner

Choose a reason for hiding this comment

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

Rather than using path/to/internal/mod.rs use path/to/intenral.rs This is a better syntax and is the recommended syntax over the mod.rs files. Eventually the code base ends up having a billion mod.rs files and that is not really compelling.

See more here:
https://doc.rust-lang.org/book/ch07-02-defining-modules-to-control-scope-and-privacy.html#modules-cheat-sheet
https://doc.rust-lang.org/stable/book/ch07-05-separating-modules-into-different-files.html#alternate-file-paths
https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html#no-more-modrs

The mod.rs syntax is old relic pre 2018 Edition time (before Rust 1.31) just to make the Rust compiler support the modules.

Copy link
Author

Choose a reason for hiding this comment

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

Nice! didn't knew we can do the same thing without using mod.rs files. thanks

@@ -24,6 +31,11 @@ const SWAGGER_UI_DOWNLOAD_URL_DEFAULT: &str =
const SWAGGER_UI_DOWNLOAD_URL: &str = "SWAGGER_UI_DOWNLOAD_URL";
const SWAGGER_UI_OVERWRITE_FOLDER: &str = "SWAGGER_UI_OVERWRITE_FOLDER";

// wget <url> && sha256sum <file> | tr '[a-z]' '[A-Z]'
#[cfg(feature = "cache")]
const SWAGGER_UI_FILE_HASH: &str =
Copy link
Owner

Choose a reason for hiding this comment

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

But what happens if the user changes the url? I guess the validation will then just fail?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, it is only used as a suffix, well this is not very good to be hard coded because users can change the SWAGGER_UI_DOWNLOAD_URL which effectively is changing the version that changes the hash as well.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, it is only used as a suffix, well this is not very good to be hard coded because users can change the SWAGGER_UI_DOWNLOAD_URL which effectively is changing the version that changes the hash as well.

I think we should avoid use cache if user supplied the URL

@thewh1teagle
Copy link
Author

Added documentation and simplified the implementation. If this looks good, I'll squash the commits.

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.

Cache assets downloaded in build.rs
2 participants