-
Notifications
You must be signed in to change notification settings - Fork 53
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
Replace the filename type of DownloadObjectArgs and UploadObjectArgs #149
base: main
Are you sure you want to change the base?
Conversation
…gs from std::string to std::filesystem::path
@@ -213,12 213,12 @@ error::Error DownloadObjectArgs::Validate() const { | |||
if (error::Error err = ObjectReadArgs::Validate()) { | |||
return err; | |||
} | |||
if (!utils::CheckNonEmptyString(filename)) { | |||
if (!utils::CheckNonEmptyString(filename.u8string())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't filename.string()
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filename.string()
may make errors with the pure utf-8 encoded path. Let's consider following:
#include <iostream>
#include <string>
#include <filesystem>
int main(int argc, char* argv[])
{
try {
std::filesystem::path fileStr = L"G:\\Tuấn Anh\\tết.txt";
std::string test1 = fileStr.u8string();
std::string test2 = fileStr.string(); // >>>>>>>>>>> throw exception
}
catch (const std::exception& ex) {
std::cerr << ex.what() << std::endl;
return 1;
}
return 0;
}
I tried again with filename.string()
, the exception was thrown.
With minio::utils::Trim(...)
, I don't think there is a different in result with filename.string()
or filename.u8string()
I think this is not fixing the root cause of the problem though. The C Changing Not sure what would be the best thing to do in this case, I would like to just tell C 's filesystem that we have indeed an UTF-8 encoded string to avoid going through mbtowc. |
@kobalicek Currently, I only use
I'm also not sure which is better. However, it seems we don't have much choice. |
If you use When it comes to Windows the library code would most likely use WideChar API under the hood, so using I'm still not sure about the best solution here, whether exposing std::filesystem in public API makes sense or not. |
what does AWS CPP library do in this scenario? @kobalicek |
I think so, too. |
I have no idea. Usually when you want UTF-8 file naming on Windows you would convert your UTF-8 string to Windows WideChar (UTF-16) and use WinAPI unicode-aware functions to access the file. And here the problem is file names on Windows platform can even have invalid surrogate pairs, etc... as the API doesn't validate the names, it just wants a sequence of 16-bit chars (similarly Linux API just wants a sequence of bytes). This means that if you use a regular validating UTF-8 -> UTF-16 conversion it could refuse your "invalid" file names. And quite frankly, I have no idea how C BTW check this issue regarding path names on Windows: This means that under windows we would want to use WTF-8 encoding instead of UTF-8: That's why I don't really know the best way of solving this in minio-cpp. Ideally we should do the conversion ourselves to be sure, but honestly that just sounds like a lot of work (basically to write the code and have it tested). Depending on |
Looks like there is no simple solution available. Why can't we skip supporting such special use case in |
yeah since this is a language specific problem, we can make this assumption and also add a doc line on what to do when a user wishes to deal with UTF-8. |
Using |
std::ifstream file(filename);
// construct PutObjectArgs
PutObjectResponse resp = PutObject(args);
file.close();
// Use resp |
Hi.
I have a problem with the
filename
attribute type ofDownloadObjectArgs
andUploadObjectArgs
.The current type is
std::string
, this leads to a file with utf-8 characters in the path that cannot read.Example path:
G:\\Tuấn Anh\tết.txt
.So, I suggest replacing
std::string
withstd::filesystem::path
Thanks.