Azure / Azure/azure-sdk-for-cpp

Unify Url class to never have leading `/` in the path

Open
#6,532 1 comment 0 reactions 0 assignees View on GitHub
Azure.Core Client needs-team-attention
Dominant language
C++
Stars
205
Forks
172
Avg merge
1d 3h
Merged PRs (30d)
37

Description

There's this
https://github.com/Azure/azure-sdk-for-cpp/blob/868a14f4d89ad8d7e1d79fa80714e4aa34736369/sdk/core/azure-core/inc/azure/core/url.hpp#L152-L159

and this
https://github.com/Azure/azure-sdk-for-cpp/blob/9271d13542a9e227d246b4ca3939823038199b30/sdk/core/azure-core/src/http/url.cpp#L218-L226

and also this
https://github.com/Azure/azure-sdk-for-cpp/blob/ac75bd58e998184b4e37c34e094e5dbd546b64ba/sdk/core/azure-core/src/http/url.cpp#L80-L84

and then this
https://github.com/Azure/azure-sdk-for-cpp/blob/7fe547bd3c8e1083e1cd39155bddc36e43453810/sdk/core/azure-core/src/http/curl/curl.cpp#L725-L732

Which, if you do
```cpp
Url url("http://www.microsoft.com");
url.AppendPath("/path");
```

will make no difference when you call `url.GetAbsoluteUrl()`, but should you call `GetRelativeUrl()`, will have the leading slash. It will result in `GET //path HTTP 1.1` instead of `GET /path HTTP 1.1` when using curl transport.
WinHTTP apparently corrects for this, so the bug becomes platform-specific. Servers don't like the `//`, and may return 404 (FWIW, typespec's Spector test server does, and I think it won't be alone).

What I think we should do, is fix inside the `Url::AppendPath()` - add `if (existingPath.empty() && pathThatWeReAboutToAdd.startsWith('/')) { pathThatWeReAboutToAdd.dropLeading('/'); }`
This will be in line with #5187, making the approach consistent in a case when the existing path was empty. ALso, this will be consistent with the `Url::Url(std::string)` constructor which does parsing and drops the `/` before parsing path.

Because otherwise, why would it be ok to
```cpp
Url url("http://www.microsoft.com/existingPath/");
// always safe to have leading '/' when appending
url.AppendPath("/newPath"); // GetRelativeUrl() => "existingPath/newPath"
```
but not
```cpp
Url url("http://www.microsoft.com/");
// suddenly NOT safe to have leading '/' when appending ?!
url.AppendPath("/newPath"); // GetRelativeUrl() => "/newPath", should be "newPath"
```

cc @LarryOsterman, if maybe I am missing something, and you have another thoughts?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.