Restore support for model:// URIs #172
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #172 +/- ##
==========================================
+ Coverage 76.30% 76.40% +0.10%
==========================================
Files 71 71
Lines 10049 10080 +31
==========================================
+ Hits 7668 7702 +34
+ Misses 2381 2378 -3
Continue to review full report at Codecov.
|
nkoenig
reviewed
Feb 12, 2021
include/ignition/common/URI.hh
Outdated
| public: URIPath &Path(); | ||
| /// \deprecated Use `PathSegments()`, the path doesn't contain the | ||
| /// authority anymore. | ||
| public: URIPath IGN_DEPRECATED(4.0) &Path(); |
There was a problem hiding this comment.
The Path and PathSegments functions have the same behavior, so this deprecation is strange. An end user could read the deprecation warning as "Just switch to PathSegments(), and my code should work".
Remove PathSegments and change Path() to Path(bool _includeAuthority=true). The body of Path could output a warning message when _includeAuthority==true. Line 316 in src/SystemPaths.cc could use the value of _includeAuthority to know it should use _uri.Authority().Host() or not.
Another option would be to add functions to set/get a class variable called bool pathIncludeAuthority = true. The behavior is the same as above, but easier to track and document. We can then tick-tock the value of pathIncludeAuthority at some point.
There was a problem hiding this comment.
I started working on the 2nd option with the goal of making the whole class support "included authority mode":
Diff
diff --git a/include/ignition/common/SystemPaths.hh b/include/ignition/common/SystemPaths.hh
index f6c58a2..4899e29 100644
--- a/include/ignition/common/SystemPaths.hh
+++ b/include/ignition/common/SystemPaths.hh
@@ -75,8 +75,9 @@ namespace ignition
public: std::string FindFileURI(const std::string &_uri) const;
/// \brief Find a file or path using a URI.
- /// If URI is not an absolute path, the URI's "Host / PathSegments"
- /// will be matched against all added paths and environment variables.
+ /// If URI is not an absolute path, the URI's path will be matched against
+ /// all added paths and environment variables (including URI authority as
+ /// needed).
/// \param[in] _uri the uniform resource identifier
/// \return Returns full path name to file with platform-specific
/// directory separators, or an empty string if URI couldn't be found.
diff --git a/include/ignition/common/URI.hh b/include/ignition/common/URI.hh
index c2aa1b9..ca44626 100644
--- a/include/ignition/common/URI.hh
+++ b/include/ignition/common/URI.hh
@@ -211,6 +211,16 @@ namespace ignition
/// and a Windows drive specifier (e.g. 'C:') is pushed to it.
public: void PushBack(const std::string &_part);
+ /// \brief Remove the part that's in the front of this path and return it.
+ /// \return Popped part.
+ /// Returns empty string if path doesn't have parts to be popped.
+ public: std::string PopFront();
+
+ /// \brief Remove the part that's in the back of this path and return it.
+ /// \return Popped part.
+ /// Returns empty string if path doesn't have parts to be popped.
+ public: std::string PopBack();
+
/// \brief Compound assignment operator.
/// \param[in] _part A new path to append.
/// \return A new Path that consists of "this / _part"
@@ -393,7 +403,10 @@ namespace ignition
/// \brief Construct a URI object from a string.
/// \param[in] _str A string.
- public: explicit URI(const std::string &_str);
+ /// \param[in] _pathIncludeAuthority True if the path should include the
+ /// authority.
+ public: explicit URI(const std::string &_str,
+ bool _pathIncludeAuthority = true);
/// \brief Copy constructor
/// \param[in] _uri Another URI.
@@ -420,9 +433,9 @@ namespace ignition
/// \param[in] _scheme New scheme.
public: void SetScheme(const std::string &_scheme);
- /// \brief Get a mutable version of the URI's authority.
- /// \return The authority
- public: URIAuthority &Authority();
+ /// \brief Set the URI's authority.
+ /// \param[in] _authority The new authority.
+ public: void SetAuthority(const URIAuthority &_authority);
/// \brief Get a const reference of the URI's authority.
/// \return The authority
@@ -430,25 +443,27 @@ namespace ignition
/// \brief Get a mutable version of the path component
/// \return A reference to the path
- /// \deprecated Use `PathSegments()`, the path doesn't contain the
- /// authority anymore.
+ /// \deprecated Use `SetPath` to modify path. Using this method to modify
+ /// it may cause authority inconsistencies.
public: URIPath IGN_DEPRECATED(4.0) &Path();
+ /// \brief Set the URI's path.
+ /// \param[in] _path The new path.
+ public: void SetPath(const URIPath &_path);
+
/// \brief Get a const reference of the path component.
/// \return A const reference of the path.
- /// \deprecated Use `PathSegments()`, the path doesn't contain the
- /// authority anymore.
- public: const URIPath IGN_DEPRECATED(4.0) &Path() const;
+ public: const URIPath &Path() const;
- /// \brief Get a mutable version of the path component.
- /// The path doesn't contain the authority.
- /// \return A reference to the path
- public: URIPath &PathSegments();
+ /// \brief Set whether the path should include the authority.
+ /// Defaults to true, which is the legacy behaviour.
+ /// \param[in] _include True to include the authority in the path.
+ public: void SetPathIncludeAuthority(bool _include);
- /// \brief Get a const reference of the path component.
- /// The path doesn't contain the authority.
- /// \return A const reference of the path.
- public: const URIPath &PathSegments() const;
+ /// \brief Get whether the path includes the authority.
+ /// Defaults to true, which is the legacy behaviour.
+ /// \return True to include the authority in the path.
+ public: bool PathIncludeAuthority() const;
/// \brief Get a mutable version of the query component
/// \return A reference to the query
diff --git a/src/SystemPaths.cc b/src/SystemPaths.cc
index 5cfb099..df3762a 100644
--- a/src/SystemPaths.cc
+++ b/src/SystemPaths.cc
@@ -313,10 +313,23 @@ std::string SystemPaths::FindFileURI(const std::string &_uri) const
std::string SystemPaths::FindFileURI(const ignition::common::URI &_uri) const
{
std::string prefix = _uri.Scheme();
- std::string suffix = _uri.Authority().Host() + _uri.PathSegments().Str() +
- _uri.Query().Str();
+ std::string suffix = _uri.Authority().Str().substr(2);
+ if (_uri.PathIncludeAuthority() && suffix.size() > 2u)
+ {
+ auto pathCopy = _uri.Path();
+ pathCopy.PopFront();
+ suffix += pathCopy.Str();
+ }
+ else
+ {
+ suffix += _uri.Path().Str();
+ }
+
+ suffix += _uri.Query().Str();
std::string filename;
+ignerr << suffix << std::endl;
+
if (prefix == "file")
{
// First try to find the file on the current system
@@ -421,6 +434,7 @@ std::string SystemPaths::FindFile(const std::string &_filename,
for (const std::string &filePath : this->dataPtr->filePaths)
{
auto withSuffix = NormalizeDirectoryPath(filePath) + filename;
+ignerr << withSuffix << std::endl;
if (exists(withSuffix))
{
path = ignition::common::copyFromUnixPath(withSuffix);
diff --git a/src/SystemPaths_TEST.cc b/src/SystemPaths_TEST.cc
index ce04b6f..dbc80a7 100644
--- a/src/SystemPaths_TEST.cc
+++ b/src/SystemPaths_TEST.cc
@@ -239,7 +239,7 @@ TEST_F(SystemPathsFixture, FindFileURI)
auto robot2Cb = [dir2](const ignition::common::URI &_uri)
{
return _uri.Scheme() == "robot" ?
- ignition::common::joinPaths(dir2, _uri.PathSegments().Str()) : "";
+ ignition::common::joinPaths(dir2, _uri.Path().Str()) : "";
};
EXPECT_EQ("", sp.FindFileURI("robot://test_f1"));
diff --git a/src/URI.cc b/src/URI.cc
index ca8eadf..6358e62 100644
--- a/src/URI.cc
+++ b/src/URI.cc
@@ -102,6 +102,9 @@ class ignition::common::URIPrivate
/// \brief Fragment component.
public: URIFragment fragment;
+
+ /// \brief Whether the path includes the authority
+ public: bool pathIncludeAuthority{true};
};
//////////////////////////////////////////////////
@@ -513,6 +516,28 @@ void URIPath::PushBack(const std::string &_part)
this->dataPtr->path.push_back(part);
}
+/////////////////////////////////////////////////
+std::string URIPath::PopFront()
+{
+ if (this->dataPtr->path.size() == 0)
+ return std::string();
+
+ auto result = this->dataPtr->path.front();
+ this->dataPtr->path.pop_front();
+ return result;
+}
+
+/////////////////////////////////////////////////
+std::string URIPath::PopBack()
+{
+ if (this->dataPtr->path.size() == 0)
+ return std::string();
+
+ auto result = this->dataPtr->path.back();
+ this->dataPtr->path.pop_back();
+ return result;
+}
+
/////////////////////////////////////////////////
const URIPath URIPath::operator/(const std::string &_part) const
{
@@ -906,9 +931,10 @@ URI::URI()
}
/////////////////////////////////////////////////
-URI::URI(const std::string &_str)
+URI::URI(const std::string &_str, bool _pathIncludeAuthority)
: URI()
{
+ this->dataPtr->pathIncludeAuthority = _pathIncludeAuthority;
this->Parse(_str);
}
@@ -928,10 +954,22 @@ URI::~URI()
std::string URI::Str() const
{
std::string result =
- this->dataPtr->scheme.empty() ? "" : this->dataPtr->scheme + ":";
- result += this->dataPtr->authority.Str() +
- this->dataPtr->path.Str() +
- this->dataPtr->query.Str() +
+ this->dataPtr->scheme.empty() ? "" : this->dataPtr->scheme + kSchemeDelim;
+ result += this->dataPtr->authority.Str();
+
+ if (this->dataPtr->pathIncludeAuthority &&
+ this->dataPtr->authority.Str().size() > 2u)
+ {
+ auto pathCopy = this->dataPtr->path;
+ pathCopy.PopFront();
+ result += pathCopy.Str();
+ }
+ else
+ {
+ result += this->dataPtr->path.Str();
+ }
+
+ result += this->dataPtr->query.Str() +
this->dataPtr->fragment.Str();
return result;
}
@@ -949,9 +987,14 @@ void URI::SetScheme(const std::string &_scheme)
}
/////////////////////////////////////////////////
-URIAuthority &URI::Authority()
+void URI::SetAuthority(const URIAuthority &_authority)
{
- return this->dataPtr->authority;
+ this->dataPtr->authority = _authority;
+ if (this->dataPtr->pathIncludeAuthority)
+ {
+ this->dataPtr->path.PopFront();
+ this->dataPtr->path.PushFront(_authority.Str().substr(2));
+ }
}
/////////////////////////////////////////////////
@@ -966,6 +1009,17 @@ URIPath &URI::Path()
return this->dataPtr->path;
}
+/////////////////////////////////////////////////
+void URI::SetPath(const URIPath &_path)
+{
+ this->dataPtr->path = _path;
+ if (this->dataPtr->pathIncludeAuthority)
+ {
+ auto pathCopy = _path;
+ this->dataPtr->authority = URIAuthority(pathCopy.PopFront());
+ }
+}
+
/////////////////////////////////////////////////
const URIPath &URI::Path() const
{
@@ -973,15 +1027,15 @@ const URIPath &URI::Path() const
}
/////////////////////////////////////////////////
-URIPath &URI::PathSegments()
+bool URI::PathIncludeAuthority() const
{
- return this->dataPtr->path;
+ return this->dataPtr->pathIncludeAuthority;
}
/////////////////////////////////////////////////
-const URIPath &URI::PathSegments() const
+void URI::SetPathIncludeAuthority(bool _include)
{
- return this->dataPtr->path;
+ this->dataPtr->pathIncludeAuthority = _include;
}
/////////////////////////////////////////////////
@@ -1185,6 +1239,9 @@ bool URI::Parse(const std::string &_str)
localPath = str.substr(0, pathEndPos);
str.erase(0, pathEndPos);
+ if (this->dataPtr->pathIncludeAuthority && !localAuthority.empty())
+ localPath = localAuthority + localPath;
+
size_t queryStartPos = str.find("?");
if (queryStartPos != std::string::npos)
{
diff --git a/src/URI_TEST.cc b/src/URI_TEST.cc
index ead768a..15f2856 100644
--- a/src/URI_TEST.cc
+++ b/src/URI_TEST.cc
@@ -73,6 +73,9 @@ TEST(URITEST, URIPath)
EXPECT_EQ(path6.Str(), "/");
EXPECT_TRUE(path6.IsAbsolute());
+ EXPECT_TRUE(path6.PopFront().empty());
+ EXPECT_TRUE(path6.PopBack().empty());
+
URIPath path7;
path7.PushFront("/abs");
EXPECT_EQ(path7.Str(), "/abs");
@@ -116,6 +119,11 @@ TEST(URITEST, URIPath)
EXPECT_EQ(path7.Str(), "/abs6%2Fabs5/abs4/abs3/abs2/abs");
EXPECT_TRUE(path7.IsAbsolute());
+ EXPECT_EQ("abs6%2Fabs5", path7.PopFront());
+ EXPECT_EQ("/abs4/abs3/abs2/abs", path7.Str());
+ EXPECT_EQ("abs", path7.PopBack());
+ EXPECT_EQ("/abs4/abs3/abs2", path7.Str());
+
URIPath path8;
path8.PushBack("/abs");
EXPECT_EQ(path8.Str(), "/abs");
@@ -454,53 +462,53 @@ TEST(URITEST, Path)
URI uri;
uri.SetScheme("data");
- uri.PathSegments() = uri.PathSegments() / "world";
+ uri.SetPath(URIPath("world"));
EXPECT_EQ(uri.Str(), "data:world");
- uri.PathSegments() /= "default";
+ uri.SetPath(URIPath("world/default"));
EXPECT_EQ(uri.Str(), "data:world/default");
EXPECT_TRUE(uri.Parse("file:///var/run/test"));
EXPECT_TRUE(uri.Parse("file:/var/run/test"));
EXPECT_EQ(uri.Str(), "file:/var/run/test");
- EXPECT_TRUE(uri.PathSegments().IsAbsolute());
+ EXPECT_TRUE(uri.Path().IsAbsolute());
EXPECT_TRUE(uri.Parse("file://var/run/test"));
EXPECT_EQ(uri.Str(), "file://var/run/test");
EXPECT_EQ(uri.Authority().Str(), "//var");
- EXPECT_EQ(uri.PathSegments().Str(), "/run/test");
- EXPECT_TRUE(uri.PathSegments().IsAbsolute());
+ EXPECT_EQ(uri.Path().Str(), "/var/run/test");
+ EXPECT_TRUE(uri.Path().IsAbsolute());
EXPECT_TRUE(uri.Parse("file://test%20space"));
EXPECT_EQ(uri.Authority().Str(), "//test%20space");
- EXPECT_TRUE(uri.PathSegments().Str().empty());
+ EXPECT_EQ("/test%20space", uri.Path().Str());
EXPECT_TRUE(uri.Parse("file:///abs/path/test"));
EXPECT_EQ(uri.Str(), "file:///abs/path/test");
- EXPECT_EQ(uri.PathSegments().Str(), "/abs/path/test");
- EXPECT_TRUE(uri.PathSegments().IsAbsolute());
+ EXPECT_EQ(uri.Path().Str(), "/abs/path/test");
+ EXPECT_TRUE(uri.Path().IsAbsolute());
uri.Parse("file:/var/run/test");
EXPECT_EQ(uri.Str(), "file:/var/run/test");
- EXPECT_EQ(uri.PathSegments().Str(), "/var/run/test");
- EXPECT_TRUE(uri.PathSegments().IsAbsolute());
+ EXPECT_EQ(uri.Path().Str(), "/var/run/test");
+ EXPECT_TRUE(uri.Path().IsAbsolute());
EXPECT_FALSE(uri.Parse("file://test+space"));
EXPECT_TRUE(uri.Parse("file:/test+space"));
EXPECT_EQ(uri.Str(), "file:/test+space");
- EXPECT_EQ(uri.PathSegments().Str(), "/test+space");
- EXPECT_TRUE(uri.PathSegments().IsAbsolute());
+ EXPECT_EQ(uri.Path().Str(), "/test+space");
+ EXPECT_TRUE(uri.Path().IsAbsolute());
EXPECT_TRUE(uri.Parse("file:/test%20space"));
EXPECT_EQ(uri.Str(), "file:/test%20space");
- EXPECT_EQ(uri.PathSegments().Str(), "/test%20space");
- EXPECT_TRUE(uri.PathSegments().IsAbsolute());
+ EXPECT_EQ(uri.Path().Str(), "/test%20space");
+ EXPECT_TRUE(uri.Path().IsAbsolute());
uri.Parse("file://C:/Users");
EXPECT_EQ(uri.Str(), "file:C:/Users");
- EXPECT_TRUE(uri.PathSegments().IsAbsolute());
+ EXPECT_TRUE(uri.Path().IsAbsolute());
}
/////////////////////////////////////////////////
@@ -508,13 +516,16 @@ TEST(URITEST, PathCopy)
{
URI uri;
uri.SetScheme("data");
- uri.PathSegments().PushFront("world");
+
+ auto path = uri.Path();
+ path.PushFront("world");
+ uri.SetPath(path);
const auto uriCopy(uri);
- const auto pathCopy(uriCopy.PathSegments() / "default");
+ const auto pathCopy(URIPath(uriCopy.Path().Str() + "/default"));
- EXPECT_NE(uri.PathSegments().Str(), pathCopy.Str());
- EXPECT_EQ(uri.PathSegments().Str(), "world");
+ EXPECT_NE(uri.Path().Str(), pathCopy.Str());
+ EXPECT_EQ(uri.Path().Str(), "world");
EXPECT_EQ(pathCopy.Str(), "world/default");
}
@@ -527,15 +538,15 @@ TEST(URITEST, Query)
uri.Query().Insert("p", "v");
EXPECT_EQ(uri.Str(), "data:?p=v");
- uri.PathSegments().PushFront("default");
+ uri.Path().PushFront("default");
EXPECT_EQ(uri.Str(), "data:default?p=v");
- uri.PathSegments().PushFront("world");
+ uri.Path().PushFront("world");
EXPECT_EQ(uri.Str(), "data:world/default?p=v");
URI uri2 = uri;
- uri.PathSegments().Clear();
+ uri.Path().Clear();
EXPECT_EQ(uri.Str(), "data:?p=v");
uri.Query().Clear();
@@ -572,15 +583,15 @@ TEST(URITEST, Fragment)
uri.Fragment() = "#f";
EXPECT_EQ(uri.Str(), "data:#f");
- uri.PathSegments().PushFront("default");
+ uri.Path().PushFront("default");
EXPECT_EQ(uri.Str(), "data:default#f");
- uri.PathSegments().PushFront("world");
+ uri.Path().PushFront("world");
EXPECT_EQ(uri.Str(), "data:world/default#f");
URI uri2 = uri;
- uri.PathSegments().Clear();
+ uri.Path().Clear();
EXPECT_EQ(uri.Str(), "data:#f");
uri.Fragment().Clear();
@@ -679,7 +690,7 @@ TEST(URITEST, WikipediaTests)
EXPECT_EQ("www.example.com", uri.Authority().Host());
EXPECT_EQ(123, *uri.Authority().Port());
EXPECT_EQ("//[email protected]:123", uri.Authority().Str());
- EXPECT_EQ("/forum/questions/", uri.PathSegments().Str());
+ EXPECT_EQ("/[email protected]:123/forum/questions/", uri.Path().Str());
EXPECT_EQ("?tag=networking&order=newest", uri.Query().Str());
EXPECT_EQ("#top", uri.Fragment().Str());
@@ -687,27 +698,27 @@ TEST(URITEST, WikipediaTests)
EXPECT_EQ("ldap", uri.Scheme());
EXPECT_EQ("[2001:db8::7]", uri.Authority().Host());
EXPECT_EQ("//[2001:db8::7]", uri.Authority().Str());
- EXPECT_EQ("/c=GB", uri.PathSegments().Str());
+ EXPECT_EQ("/[2001:db8::7]/c=GB", uri.Path().Str());
EXPECT_EQ("?objectClass?one", uri.Query().Str());
EXPECT_TRUE(uri.Parse("mailto:[email protected]"));
EXPECT_EQ("mailto", uri.Scheme());
EXPECT_TRUE(uri.Authority().Str().empty());
- EXPECT_EQ("[email protected]", uri.PathSegments().Str());
+ EXPECT_EQ("[email protected]", uri.Path().Str());
EXPECT_TRUE(uri.Query().Str().empty());
EXPECT_TRUE(uri.Fragment().Str().empty());
EXPECT_TRUE(uri.Parse("news:comp.infosystems.www.servers.unix"));
EXPECT_EQ("news", uri.Scheme());
EXPECT_TRUE(uri.Authority().Str().empty());
- EXPECT_EQ("comp.infosystems.www.servers.unix", uri.PathSegments().Str());
+ EXPECT_EQ("comp.infosystems.www.servers.unix", uri.Path().Str());
EXPECT_TRUE(uri.Query().Str().empty());
EXPECT_TRUE(uri.Fragment().Str().empty());
EXPECT_TRUE(uri.Parse("tel:+1-816-555-1212"));
EXPECT_EQ("tel", uri.Scheme());
EXPECT_TRUE(uri.Authority().Str().empty());
- EXPECT_EQ("+1-816-555-1212", uri.PathSegments().Str());
+ EXPECT_EQ("+1-816-555-1212", uri.Path().Str());
EXPECT_TRUE(uri.Query().Str().empty());
EXPECT_TRUE(uri.Fragment().Str().empty());
@@ -716,13 +727,13 @@ TEST(URITEST, WikipediaTests)
EXPECT_TRUE(uri.Authority().UserInfo().empty());
EXPECT_EQ("192.0.2.16", uri.Authority().Host());
EXPECT_EQ(80, uri.Authority().Port());
- EXPECT_EQ("/", uri.PathSegments().Str());
+ EXPECT_EQ("/192.0.2.16:80/", uri.Path().Str());
EXPECT_TRUE(uri.Parse("urn:oasis:names:specification:docbook:dtd:xml:4.1.2"));
EXPECT_EQ("urn", uri.Scheme());
EXPECT_TRUE(uri.Authority().Str().empty());
EXPECT_EQ("oasis:names:specification:docbook:dtd:xml:4.1.2",
- uri.PathSegments().Str());
+ uri.Path().Str());
EXPECT_TRUE(uri.Query().Str().empty());
EXPECT_TRUE(uri.Fragment().Str().empty());
}
@@ -765,16 +776,16 @@ TEST(URITEST, File)
{
URI uri;
EXPECT_TRUE(uri.Parse("file:relative/path"));
- EXPECT_EQ("relative/path", uri.PathSegments().Str());
+ EXPECT_EQ("relative/path", uri.Path().Str());
EXPECT_FALSE(uri.Authority().EmptyHostValid());
EXPECT_TRUE(uri.Parse("file:/abs/path"));
- EXPECT_EQ("/abs/path", uri.PathSegments().Str());
+ EXPECT_EQ("/abs/path", uri.Path().Str());
EXPECT_FALSE(uri.Authority().EmptyHostValid());
// Empty host is valid for file: scheme
EXPECT_TRUE(uri.Parse("file:///abs/path"));
- EXPECT_EQ("/abs/path", uri.PathSegments().Str());
+ EXPECT_EQ("/abs/path", uri.Path().Str());
EXPECT_TRUE(uri.Authority().EmptyHostValid());
}
@@ -787,6 +798,54 @@ TEST(URITEST, WinPath)
EXPECT_EQ("file:D:/my/test/dir/world.sdf", uri.Str());
}
+//////////////////////////////////////////////////
+TEST(URITEST, PathIncludeAuthority)
+{
+ {
+ // Includes by default
+ URI uri("https://[email protected]:123/forum/questions/");
+
+ EXPECT_EQ("john.doe", uri.Authority().UserInfo());
+ EXPECT_EQ("www.example.com", uri.Authority().Host());
+ EXPECT_EQ(123, *uri.Authority().Port());
+ EXPECT_EQ("//[email protected]:123", uri.Authority().Str());
+
+ EXPECT_EQ("/[email protected]:123/forum/questions/", uri.Path().Str());
+
+ // Modifyng authority also updates path
+ uri.SetAuthority(URIAuthority("//new_authority.com"));
+
+ EXPECT_TRUE(uri.Authority().UserInfo().empty());
+ EXPECT_EQ("new_authority.com", uri.Authority().Host());
+ EXPECT_FALSE(uri.Authority().Port());
+ EXPECT_EQ("//new_authority.com", uri.Authority().Str());
+
+ EXPECT_EQ("/new_authority.com/forum/questions/", uri.Path().Str());
+
+ // Modifyng path also updates authority
+ uri.SetPath(URIPath("/newest_authority.com/another/path"));
+
+ EXPECT_TRUE(uri.Authority().UserInfo().empty());
+ EXPECT_EQ("newest_authority.com", uri.Authority().Host());
+ EXPECT_FALSE(uri.Authority().Port());
+ EXPECT_EQ("//newest_authority.com", uri.Authority().Str());
+
+ EXPECT_EQ("/newest_authority.com/forum/questions/", uri.Path().Str());
+ }
+
+ {
+ // Not including
+ URI uri("https://[email protected]:123/forum/questions/", false);
+
+ EXPECT_EQ("john.doe", uri.Authority().UserInfo());
+ EXPECT_EQ("www.example.com", uri.Authority().Host());
+ EXPECT_EQ(123, *uri.Authority().Port());
+ EXPECT_EQ("//[email protected]:123", uri.Authority().Str());
+
+ EXPECT_EQ("/forum/questions/", uri.Path().Str());
+ }
+}
+
//////////////////////////////////////////////////
TEST(URITEST, Resource)
{
@@ -798,31 +857,70 @@ TEST(URITEST, Resource)
EXPECT_EQ("//model_name", uri.Authority().Str());
EXPECT_EQ("model_name", uri.Authority().Host());
EXPECT_TRUE(uri.Authority().UserInfo().empty());
- EXPECT_EQ("/meshes/mesh.dae", uri.PathSegments().Str());
+ EXPECT_EQ("/model_name/meshes/mesh.dae", uri.Path().Str());
EXPECT_TRUE(uri.Query().Str().empty());
EXPECT_TRUE(uri.Fragment().Str().empty());
}
{
URI uri;
+ uri.SetPathIncludeAuthority(false);
+ EXPECT_TRUE(uri.Parse("model://model_name/meshes/mesh.dae"));
+ EXPECT_EQ("model", uri.Scheme());
+ EXPECT_EQ("//model_name", uri.Authority().Str());
+ EXPECT_EQ("model_name", uri.Authority().Host());
+ EXPECT_TRUE(uri.Authority().UserInfo().empty());
+ EXPECT_EQ("/meshes/mesh.dae", uri.Path().Str());
+ EXPECT_TRUE(uri.Query().Str().empty());
+ EXPECT_TRUE(uri.Fragment().Str().empty());
+ }
+
+ {
+ URI uri;
+ EXPECT_TRUE(uri.Parse("model://model_name"));
+ EXPECT_EQ("model", uri.Scheme());
+ EXPECT_EQ("//model_name", uri.Authority().Str());
+ EXPECT_EQ("model_name", uri.Authority().Host());
+ EXPECT_TRUE(uri.Authority().UserInfo().empty());
+ EXPECT_EQ("/model_name", uri.Path().Str());
+ EXPECT_TRUE(uri.Query().Str().empty());
+ EXPECT_TRUE(uri.Fragment().Str().empty());
+ }
+
+ {
+ URI uri;
+ uri.SetPathIncludeAuthority(false);
EXPECT_TRUE(uri.Parse("model://model_name"));
EXPECT_EQ("model", uri.Scheme());
EXPECT_EQ("//model_name", uri.Authority().Str());
EXPECT_EQ("model_name", uri.Authority().Host());
EXPECT_TRUE(uri.Authority().UserInfo().empty());
- EXPECT_TRUE(uri.PathSegments().Str().empty());
+ EXPECT_TRUE(uri.Path().Str().empty());
+ EXPECT_TRUE(uri.Query().Str().empty());
+ EXPECT_TRUE(uri.Fragment().Str().empty());
+ }
+
+ {
+ URI uri;
+ EXPECT_TRUE(uri.Parse("package://package_name/models/model"));
+ EXPECT_EQ("package", uri.Scheme());
+ EXPECT_EQ("//package_name", uri.Authority().Str());
+ EXPECT_EQ("package_name", uri.Authority().Host());
+ EXPECT_TRUE(uri.Authority().UserInfo().empty());
+ EXPECT_EQ("/package_name/models/model", uri.Path().Str());
EXPECT_TRUE(uri.Query().Str().empty());
EXPECT_TRUE(uri.Fragment().Str().empty());
}
{
URI uri;
+ uri.SetPathIncludeAuthority(false);
EXPECT_TRUE(uri.Parse("package://package_name/models/model"));
EXPECT_EQ("package", uri.Scheme());
EXPECT_EQ("//package_name", uri.Authority().Str());
EXPECT_EQ("package_name", uri.Authority().Host());
EXPECT_TRUE(uri.Authority().UserInfo().empty());
- EXPECT_EQ("/models/model", uri.PathSegments().Str());
+ EXPECT_EQ("/models/model", uri.Path().Str());
EXPECT_TRUE(uri.Query().Str().empty());
EXPECT_TRUE(uri.Fragment().Str().empty());
}It starts to become tricky to keep the authority and the path in sync. I added a SetPath function and deprecated the mutable Path() so that the URI class has control over when the path is mutated, and can update the authority accordingly. That turned out to be a bad idea because the compiler chooses the mutable Path() instead of the const one whenever the URI object isn't itself const, which means downstream users would need to workaround this even for use cases where the const Path() is ok.
So I'm backtracking and will do the first option instead.
There was a problem hiding this comment.
Ok, I implemented URI::HasAuthority APIs that let users turn on the new behaviour, but default to the old one. The current state of this branch works with gazebosim/gz-fuel-tools#163. Mind taking a look, @nkoenig ?
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
mjcarroll
reviewed
Feb 19, 2021
Signed-off-by: Louise Poubel <[email protected]>
mjcarroll
reviewed
Feb 19, 2021
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
|
Ci is all 🟢 ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
While working on gazebo-tooling/release-tools#361, I noticed that the changes in the URI introduced in
ign-common4were very disruptive, see gazebosim/gz-fuel-tools#163 (comment).This is a proposal to reduce the impact of these changes and maintain support for the "URI" format that is widespread in the ecosystem (
model:///package://). Another goal of this PR is to make the changes more evident to downstream users through deprecation warnings.Because there was no concept of "authority" up to
ign-common3, it's safe to assume that users are currently embedding the authority into the path. This PR deprecatesURI::Path()to hint to the user that something has changed and adds an alternative function,PathSegments, that does the same. When the user checks the migration guide, there are instructions to useAuthority+PathSegments.One thing I considered was maintaining the behaviour of
Pathas it was before, that is, it would set / get both authority and path segments. I could look into it if it sounds interesting.Test it
Checklist
codecheckpassed (See contributing)another open pull request
to support the maintainers
Note to maintainers: Remember to use Squash-Merge