Skip to content

[#478] Fix handling of plus (+) and use Boost.URL (main) - #511

Open
korydraughn wants to merge 3 commits into
irods:mainfrom
korydraughn:478.m
Open

[#478] Fix handling of plus (+) and use Boost.URL (main)#511
korydraughn wants to merge 3 commits into
irods:mainfrom
korydraughn:478.m

Conversation

@korydraughn

Copy link
Copy Markdown
Contributor

All non-OIDC tests pass.


// TODO Create a better name.
auto to_argument_list(const std::string_view _urlencoded_string) -> std::unordered_map<std::string, std::string>;
auto parse_urlencoded_data(const std::string_view _urlencoded_string)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Document that this implements the following.

Comment thread core/src/common.cpp
Comment on lines +202 to +211
auto parse_result = boost::urls::parse_origin_form(_req.target());
if (!parse_result) {
const auto& ec = parse_result.error();
logging::error(
"{}: Could not parse URL path [{}]; error code=[{}], message=[{}].",
__func__,
_req.target(),
ec.value(),
ec.message());
THROW(SYS_LIBRARY_ERROR, "URL parse error");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Trigger this code path to see what the log message looks like.

Or go look at the docs for error_code::value/::message.

Comment on lines +738 to +741
collection = f'/{self.zone_name}/home/{self.rodsuser_username}/=issue+478'

try:
# Create a collection that has a "+" in its logical path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The comment on L741 is for the path on L738. Consider moving the comment and adding more context.

Comment thread core/src/common.cpp
#include <boost/beast.hpp>
#include <boost/url.hpp>

#include <curl/curl.h>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

still need curl?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, for now. There are still a few places where we use libcurl.

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.

2 participants