Re-enabling Windows tests (#376)

Fix tests around Windows' handling (or not handling) of : and @ in filesystem paths.

Signed-off-by: Michael Carroll <[email protected]>

include/gz/fuel_tools/FuelClient.hh

@@ -315,6 +315,13 @@ namespace gz
                   const std::vector<WorldIdentifier> &_ids,
                   size_t _jobs = 2);
 
+      /// \brief Check if a model is already present in the local cache.
+      /// \param[in] _id The model identifier
+      /// \param[out] _path Local path where the model can be found.
+      /// \return FETCH_ERROR if not cached, FETCH_ALREADY_EXISTS if cached.
+      public: Result CachedModel(const ModelIdentifier &_id,
+                                 std::string &_path);
+
       /// \brief Check if a model is already present in the local cache.
       /// \param[in] _modelUrl The unique URL of the model on a Fuel server.
       /// E.g.: https://fuel.gazebosim.org/1.0/caguero/models/Beer

include/gz/fuel_tools/Helpers.hh

@@ -39,6 +39,21 @@ namespace gz
 {
 namespace fuel_tools
 {
+/// \brief Convert the authority portion of a URI to a string
+/// suitable to be used as a path on disk for all platforms.
+///
+/// It encodes illegal characters on Windows and Linux filesystems with
+/// their corresponding URL-encoded values.
+///
+/// This assumes an authority of the form: username@host:port
+/// "@" is encoded as %40
+/// ":" is encoded as %3A
+///
+/// \param[in] _uriAuthority the authority section of the URI to convert.
+/// \return String suitable to use in file paths
+GZ_FUEL_TOOLS_VISIBLE
+std::string sanitizeAuthority(const std::string &_uriAuthority);
+
 /// \brief Convert a URI to a string suitable to use as a path on disk.
 /// It strips the scheme and authority's `//` prefix.
 /// \param[in] _uri URI to convert.

src/ClientConfig.cc

@@ -83,7 +83,7 @@ class gz::fuel_tools::ServerConfigPrivate
           }
 
   /// \brief URL to reach server
-  public: common::URI url{"https://fuel.gazebosim.org"};
+  public: common::URI url{"https://fuel.gazebosim.org", true};
 
   /// \brief A key to auth with the server
   public: std::string key = "";

src/CollectionIdentifier_TEST.cc

@@ -45,7 +45,7 @@ TEST(CollectionIdentifier, SetFields)
 /////////////////////////////////////////////////
 /// \brief Unique Name
 // See https://github.com/gazebosim/gz-fuel-tools/issues/231
-TEST(CollectionIdentifier, GZ_UTILS_TEST_DISABLED_ON_WIN32(UniqueName))
+TEST(CollectionIdentifier, UniqueName)
 {
   gz::fuel_tools::ServerConfig srv1;
   srv1.SetUrl(common::URI("https://localhost:8001"));

src/FuelClient.cc

@@ -730,7 +730,8 @@ Result FuelClient::ModelDependencies(const ModelIdentifier &_id,
   // Locate any dependencies from the input model and download them.
   std::string path;
   gz::msgs::FuelMetadata meta;
-  if (this->CachedModel(gz::common::URI(_id.UniqueName()), path))
+
+  if (this->CachedModel(_id, path))
   {
     std::string metadataPath =
       gz::common::joinPaths(path, "metadata.pbtxt");
@@ -1097,7 +1098,11 @@ bool FuelClient::ParseModelUrl(const common::URI &_modelUrl,
   }
 
   // Get remaining server information from config
-  _id.Server().SetUrl(common::URI(scheme + "://" + server));
+  common::URI serverUri;
+  serverUri.SetScheme(scheme);
+  serverUri.SetAuthority(gz::common::URIAuthority("//" + server));
+
+  _id.Server().SetUrl(serverUri);
   _id.Server().SetVersion(apiVersion);
   for (const auto &s : this->dataPtr->config.Servers())
   {
@@ -1163,7 +1168,11 @@ bool FuelClient::ParseWorldUrl(const common::URI &_worldUrl,
   }
 
   // Get remaining server information from config
-  _id.Server().SetUrl(common::URI(scheme + "://" + server));
+  common::URI serverUri;
+  serverUri.SetScheme(scheme);
+  serverUri.SetAuthority(gz::common::URIAuthority("//" + server));
+
+  _id.Server().SetUrl(serverUri);
   _id.Server().SetVersion(apiVersion);
   for (const auto &s : this->dataPtr->config.Servers())
   {
@@ -1231,7 +1240,11 @@ bool FuelClient::ParseModelFileUrl(const common::URI &_modelFileUrl,
   }
 
   // Get remaining server information from config
-  _id.Server().SetUrl(common::URI(scheme + "://" + server));
+  common::URI serverUri;
+  serverUri.SetScheme(scheme);
+  serverUri.SetAuthority(gz::common::URIAuthority("//" + server));
+
+  _id.Server().SetUrl(serverUri);
   _id.Server().SetVersion(apiVersion);
   for (const auto &s : this->dataPtr->config.Servers())
   {
@@ -1300,7 +1313,11 @@ bool FuelClient::ParseWorldFileUrl(const common::URI &_worldFileUrl,
   }
 
   // Get remaining server information from config
-  _id.Server().SetUrl(common::URI(scheme + "://" + server));
+  common::URI serverUri;
+  serverUri.SetScheme(scheme);
+  serverUri.SetAuthority(gz::common::URIAuthority("//" + server));
+
+  _id.Server().SetUrl(serverUri);
   _id.Server().SetVersion(apiVersion);
   for (const auto &s : this->dataPtr->config.Servers())
   {
@@ -1367,7 +1384,11 @@ bool FuelClient::ParseCollectionUrl(const common::URI &_url,
   }
 
   // Get remaining server information from config
-  _id.Server().SetUrl(common::URI(scheme + "://" + server));
+  common::URI serverUri;
+  serverUri.SetScheme(scheme);
+  serverUri.SetAuthority(gz::common::URIAuthority("//" + server));
+
+  _id.Server().SetUrl(serverUri);
   _id.Server().SetVersion(apiVersion);
   for (const auto &s : this->dataPtr->config.Servers())
   {
@@ -1414,9 +1435,6 @@ Result FuelClient::DownloadModel(const common::URI &_modelUrl,
   if (!result)
     return result;
 
-  // TODO(anyone) We shouldn't need to reconstruct the path, SaveModel should
-  // be changed to return it
-
   // We need to figure out the version for the tip
   if (id.Version() == 0 || id.VersionStr() == "tip")
   {
@@ -1425,8 +1443,7 @@ Result FuelClient::DownloadModel(const common::URI &_modelUrl,
   }
 
   _path = gz::common::joinPaths(this->Config().CacheLocation(),
-      id.Server().Url().Path().Str(), id.Owner(), "models", id.Name(),
-      id.VersionStr());
+      id.UniqueName(), id.VersionStr());
 
   return result;
 }
@@ -1453,15 +1470,23 @@ Result FuelClient::DownloadWorld(const common::URI &_worldUrl,
 }
 
 //////////////////////////////////////////////////
-bool FuelClient::CachedModel(const common::URI &_modelUrl)
+Result FuelClient::CachedModel(const ModelIdentifier &_id,
+                               std::string &_path)
 {
-  // Get data from URL
-  ModelIdentifier id;
-  if (!this->ParseModelUrl(_modelUrl, id))
-    return Result(ResultType::FETCH_ERROR);
+  auto modelIter = this->dataPtr->cache->MatchingModel(_id);
+  if (modelIter)
+  {
+    _path = modelIter.PathToModel();
+    return Result(ResultType::FETCH_ALREADY_EXISTS);
+  }
+  return Result(ResultType::FETCH_ERROR);
+}
 
-  // Check local cache
-  return this->dataPtr->cache->MatchingModel(id) ? true : false;
+//////////////////////////////////////////////////
+bool FuelClient::CachedModel(const common::URI &_modelUrl)
+{
+  std::string path;
+  return this->CachedModel(_modelUrl, path).Type() != ResultType::FETCH_ERROR;
 }
 
 //////////////////////////////////////////////////
@@ -1475,15 +1500,7 @@ Result FuelClient::CachedModel(const common::URI &_modelUrl,
     return Result(ResultType::FETCH_ERROR);
   }
 
-  // Check local cache
-  auto modelIter = this->dataPtr->cache->MatchingModel(id);
-  if (modelIter)
-  {
-    _path = modelIter.PathToModel();
-    return Result(ResultType::FETCH_ALREADY_EXISTS);
-  }
-
-  return Result(ResultType::FETCH_ERROR);
+  return this->CachedModel(id, _path);
 }
 
 //////////////////////////////////////////////////