You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/09/14 14:30:08 UTC
[GitHub] [arrow] pitrou opened a new pull request #8185: ARROW-9859: [C++] Decode username and password in URIs
pitrou opened a new pull request #8185:
URL: https://github.com/apache/arrow/pull/8185
Allow passing a %-encoded secret key in a S3 URI.
Also, detect and report unrecognized options in S3 URIs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8185: ARROW-9859: [C++] Decode username and password in URIs
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8185:
URL: https://github.com/apache/arrow/pull/8185#issuecomment-692096470
@nealrichardson I've checked this fixes the issue with a S3 URI from Python.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8185: ARROW-9859: [C++] Decode username and password in URIs
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8185:
URL: https://github.com/apache/arrow/pull/8185#issuecomment-692100329
https://issues.apache.org/jira/browse/ARROW-9859
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #8185: ARROW-9859: [C++] Decode username and password in URIs
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #8185:
URL: https://github.com/apache/arrow/pull/8185#discussion_r487995903
##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -271,17 +271,16 @@ Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
options.ConfigureDefaultCredentials();
}
- auto it = options_map.find("region");
- if (it != options_map.end()) {
- options.region = it->second;
- }
- it = options_map.find("scheme");
- if (it != options_map.end()) {
- options.scheme = it->second;
- }
- it = options_map.find("endpoint_override");
- if (it != options_map.end()) {
- options.endpoint_override = it->second;
+ for (const auto& kv : options_map) {
+ if (kv.first == "region") {
+ options.region = kv.second;
+ } else if (kv.first == "scheme") {
+ options.scheme = kv.second;
+ } else if (kv.first == "endpoint_override") {
+ options.endpoint_override = kv.second;
+ } else {
+ return Status::Invalid("Unexpected option in S3 URI: '", kv.first, "'");
Review comment:
IIUC this would be a clearer, more helpful error message:
```suggestion
return Status::Invalid("Unexpected query parameter in S3 URI: '", kv.first, "'");
```
##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -271,17 +271,16 @@ Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
options.ConfigureDefaultCredentials();
}
- auto it = options_map.find("region");
- if (it != options_map.end()) {
- options.region = it->second;
- }
- it = options_map.find("scheme");
- if (it != options_map.end()) {
- options.scheme = it->second;
- }
- it = options_map.find("endpoint_override");
- if (it != options_map.end()) {
- options.endpoint_override = it->second;
+ for (const auto& kv : options_map) {
Review comment:
Is it guaranteed that all of these query param values can only have safe characters and don't need to be URI decoded? (Related: I don't see docs for all of these so I don't know how to use them or what valid values for them are.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8185: ARROW-9859: [C++] Decode username and password in URIs
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8185:
URL: https://github.com/apache/arrow/pull/8185#issuecomment-692198570
CI failures look unrelated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8185: ARROW-9859: [C++] Decode username and password in URIs
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8185:
URL: https://github.com/apache/arrow/pull/8185#discussion_r488000166
##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -271,17 +271,16 @@ Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
options.ConfigureDefaultCredentials();
}
- auto it = options_map.find("region");
- if (it != options_map.end()) {
- options.region = it->second;
- }
- it = options_map.find("scheme");
- if (it != options_map.end()) {
- options.scheme = it->second;
- }
- it = options_map.find("endpoint_override");
- if (it != options_map.end()) {
- options.endpoint_override = it->second;
+ for (const auto& kv : options_map) {
+ if (kv.first == "region") {
+ options.region = kv.second;
+ } else if (kv.first == "scheme") {
+ options.scheme = kv.second;
+ } else if (kv.first == "endpoint_override") {
+ options.endpoint_override = kv.second;
+ } else {
+ return Status::Invalid("Unexpected option in S3 URI: '", kv.first, "'");
Review comment:
Thanks, will do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8185: ARROW-9859: [C++] Decode username and password in URIs
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8185:
URL: https://github.com/apache/arrow/pull/8185#discussion_r488000405
##########
File path: cpp/src/arrow/filesystem/s3fs.cc
##########
@@ -271,17 +271,16 @@ Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
options.ConfigureDefaultCredentials();
}
- auto it = options_map.find("region");
- if (it != options_map.end()) {
- options.region = it->second;
- }
- it = options_map.find("scheme");
- if (it != options_map.end()) {
- options.scheme = it->second;
- }
- it = options_map.find("endpoint_override");
- if (it != options_map.end()) {
- options.endpoint_override = it->second;
+ for (const auto& kv : options_map) {
Review comment:
They are automatically decoded already by the `Uri` class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #8185: ARROW-9859: [C++] Decode username and password in URIs
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #8185:
URL: https://github.com/apache/arrow/pull/8185
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org