Make SWAGGER_UI_DOWNLOAD_URL support file:// urls #923
Conversation
| } else if url.starts_with("file://") { | ||
| let file_path = url.replace("file://", ""); | ||
| println!("start copy to : {:?}", zip_path); | ||
| fs::copy(file_path, zip_path.clone()).unwrap(); |
There was a problem hiding this comment.
No need to copy the file here, right? Wouldn't it be enough to set zip_path = file_path;?
There was a problem hiding this comment.
In principle yes, but I prefer having minimal changes between the 2 cases. Here once the download/copy operation is performed, the next steps are exactly the same: Everthing happens in target_dir.
There was a problem hiding this comment.
Fwiw, this also means that if the zip file changes then that won't take effect before you cargo clean (or remove the cached file manually).
There was a problem hiding this comment.
Hm. I would then guess it would be useful to make the file type of SWAGGER_UI_DOWNLOAD_URL to just rely on external Swagger UI. Then it is not tied to the cargo lifecycle and can be externally updated.
There was a problem hiding this comment.
Note that if the file name changes (via env var update), then no need to cargo clean. And these .zip files are meant to be immutable, straight from official source.
There was a problem hiding this comment.
Oh, yeah indeed because of this: println!("cargo:rerun-if-changed={:?}", zip_path.clone());
There was a problem hiding this comment.
and just if !zip_path.exists() {...} tests if download/copy is necessary.
There was a problem hiding this comment.
The rerun-if-changed detects if the cached zip changes, not the source zip fwiw.
There was a problem hiding this comment.
So to summarize:
- one can change a
swagger-xxx.zipfile via env varSWAGGER_UI_DOWNLOAD_URLwithfile:// - these files, typically from official releases, are uniquely identified by their name.
- if for some reason somebody creates their own, they just have to change filename, change
SWAGGER_UI_DOWNLOAD_URLcorrespondingly andcargo build- if they use the same name it will not be downloaded/copied again. - to modifiy the unzipped contents one can use env var
SWAGGER_UI_OVERWRITE_FOLDERfor selective overwrites - this will be applied in all cases.
This fine grained config allows most if not all custom needs, I think.
| println!("start download to : {:?}", zip_path); | ||
| download_file(&url, zip_path.clone()).unwrap(); | ||
| } else if url.starts_with("file://") { | ||
| let file_path = url.replace("file://", ""); |
There was a problem hiding this comment.
Arguably, file URLs should still be parsed as URLs. If it looks like a URL I'd usually assume urlencoding and such to apply as usual (on the other hand, building the path becomes easier when you just have to prepend file://... sigh).
There was a problem hiding this comment.
Except for space in a path, any other case where prepending file:// is different from parsing as url ?
There was a problem hiding this comment.
Lots of special characters end up percentage-encoded. The most obvious case would be that % is used as the escape character, so it itself needs to be escaped as %25.
There was a problem hiding this comment.
Good point, though, not sure whether this actually comes to a problem, but if it does, I guess this could then be addressed. While I did not find a way to simply url encode string for the purpose with a quick look I assume as the ZipArchive works with rust File which itself uses Path to describe the location of a file system agnostic manner it should just simply fail if for one reason or another the rust cannot interpret the input as a valid OsStr that points to a real location.
That said e.g. Unix BASH does require strings to be double quoted to prevent the word globbing thus the variable could just be defined as SWAGGER_UI_DOWNLOAD_URL="/path/to/my spaced swagger ui.zip" if I am not mistaken.
There was a problem hiding this comment.
Even if it will seldom be a problem this could be fixed, probably with something like:
// let file_path = url.replace("file://", "");
let file_path = Url.parse(url).to_file_path().unwrap();What u think ?
There was a problem hiding this comment.
Seems okay, I checked the amount of dependencies and it only has few of them so think we can have this there as well. I believe it does not increase the compile time enormously to out weight the benefit of "validating" URL.
There was a problem hiding this comment.
reqwest::Url is already just a reexport of url::Url. There's also no reparsing required for reqwest::get, since that also takes a preparsed Url (via IntoUrl).
There was a problem hiding this comment.
Indeed reqwest can be used.
So I committed to my fork:
use reqwest::Url;
// in copy section
let file_path = Url::parse(&url).unwrap().to_file_path().unwrap();Hopefully that should solve this edge case.
There was a problem hiding this comment.
Since the current PR is merged already, I created a new one with just this improvement: #925
Cf. merged PR #845 for context.
Resolves #922