Merge remote-tracking branch 'origin/ign-fuel-tools7' into scpeters/m…

…erge_7_8

CMakeLists.txt

@@ -70,6 +70,7 @@ set(GZ_MSGS_VER ${gz-msgs9_VERSION_MAJOR})
 #--------------------------------------
 # Find gz-tools
 find_program(HAVE_GZ_TOOLS gz)
+set (GZ_TOOLS_VER 2)
 
 #============================================================================
 # Configure the build

Changelog.md

@@ -71,6 +71,20 @@
 
 ## Gazebo Fuel Tools 7.x
 
+### Gazebo Fuel Tools 7.3.0 (2023-06-13)
+
+1. Forward merges
+    * [Pull request #355](https://github.com/gazebosim/gz-fuel-tools/pull/355)
+
+1. Add bash completion
+    * [Pull request #329](https://github.com/gazebosim/gz-fuel-tools/pull/329)
+
+1. Reflect pagination of RESTful requests in iterator API
+    * [Pull request #350](https://github.com/gazebosim/gz-fuel-tools/pull/350)
+
+1. Support link referral download
+    * [Pull request #333](https://github.com/gazebosim/gz-fuel-tools/pull/333)
+
 ### Gazebo Fuel Tools 7.2.2 (2023-03-29)
 
 1. Support link referral download
@@ -289,6 +303,19 @@
 
 ### Gazebo Fuel Tools 4.8.3 (2023-03-29)
 
+1. Support link referral download
+    * [Pull request #333](https://github.com/gazebosim/gz-fuel-tools/pull/333)
+
+### Gazebo Fuel Tools 4.9.0 (2023-05-03)
+
+1. Add bash completion
+    * [Pull request #329](https://github.com/gazebosim/gz-fuel-tools/pull/329)
+
+1. Reflect pagination of RESTful requests in iterator API
+    * [Pull request #350](https://github.com/gazebosim/gz-fuel-tools/pull/350)
+
+### Gazebo Fuel Tools 4.8.3 (2023-03-29)
+
 1. Support link referral download
     * [Pull request #333](https://github.com/gazebosim/gz-fuel-tools/pull/333)
 

include/gz/fuel_tools/FuelClient.hh

@@ -136,25 +136,46 @@ namespace gz
       /// \brief Returns models matching a given identifying criteria
       /// \param[in] _id a partially filled out identifier used to fetch models
       /// \remarks Fulfills Get-One requirement
-      /// \remarks It's not yet clear if model names are unique, so this API
+      /// \remarks Model names are unique to the owner, so this API
       ///          allows the possibility of getting multiple models with the
-      ///          same name.
+      ///          same name if the owner is not specified.
       /// \return An iterator of models with names matching the criteria
       public: ModelIter Models(const ModelIdentifier &_id);
 
       /// \brief Returns models matching a given identifying criteria
       /// \param[in] _id a partially filled out identifier used to fetch models
       /// \remarks Fulfills Get-One requirement
-      /// \remarks It's not yet clear if model names are unique, so this API
+      /// \remarks Model names are unique to the owner, so this API
       ///          allows the possibility of getting multiple models with the
-      ///          same name.
+      ///          same name if the owner is not specified.
       /// \return An iterator of models with names matching the criteria
       public: ModelIter Models(const ModelIdentifier &_id) const;
 
-      /// \brief Returns an iterator for the models found in a collection.
-      /// \param[in] _id a partially filled out identifier used to fetch a
-      /// collection.
-      /// \return An iterator of models in the collection.
+      /// \brief Returns models matching a given identifying criteria
+      /// \param[in] _id a partially filled out identifier used to fetch models
+      /// \param[in] _checkCache Whether to check the cache.
+      /// \remarks Fulfills Get-One requirement
+      /// \remarks Model names are unique to the owner, so this API
+      ///          allows the possibility of getting multiple models with the
+      ///          same name if the owner is not specified.
+      /// \return An iterator of models with names matching the criteria
+      public: ModelIter Models(const ModelIdentifier &_id, bool _checkCache);
+
+      /// \brief Returns models matching a given identifying criteria
+      /// \param[in] _id a partially filled out identifier used to fetch models
+      /// \param[in] _checkCache Whether to check the cache.
+      /// \remarks Fulfills Get-One requirement
+      /// \remarks Model names are unique to the owner, so this API
+      ///          allows the possibility of getting multiple models with the
+      ///          same name if the owner is not specified.
+      /// \return An iterator of models with names matching the criteria
+      public: ModelIter Models(const ModelIdentifier &_id,
+                               bool _checkCache) const;
+
+       /// \brief Returns an iterator for the models found in a collection.
+       /// \param[in] _id a partially filled out identifier used to fetch a
+       /// collection.
+       /// \return An iterator of models in the collection.
       public: ModelIter Models(const CollectionIdentifier &_id) const;
 
       /// \brief Returns worlds matching a given identifying criteria

src/FuelClient.cc

@@ -399,16 +399,31 @@ WorldIter FuelClient::Worlds(const ServerConfig &_server) const
 //////////////////////////////////////////////////
 ModelIter FuelClient::Models(const ModelIdentifier &_id)
 {
-  return const_cast<const FuelClient*>(this)->Models(_id);
+  return this->Models(_id, true);
 }
 
 //////////////////////////////////////////////////
 ModelIter FuelClient::Models(const ModelIdentifier &_id) const
 {
-  // Check local cache first
-  ModelIter localIter = this->dataPtr->cache->MatchingModels(_id);
-  if (localIter)
-    return localIter;
+  return this->Models(_id, true);
+}
+
+//////////////////////////////////////////////////
+ModelIter FuelClient::Models(const ModelIdentifier &_id, bool _checkCache)
+{
+  return const_cast<const FuelClient*>(this)->Models(_id, _checkCache);
+}
+
+//////////////////////////////////////////////////
+ModelIter FuelClient::Models(const ModelIdentifier &_id, bool _checkCache) const
+{
+  if (_checkCache)
+  {
+    // Check local cache first
+    ModelIter localIter = this->dataPtr->cache->MatchingModels(_id);
+    if (localIter)
+      return localIter;
+  }
 
   // TODO(nkoenig) try to fetch model directly from a server
   // Note: gz-fuel-server doesn't like URLs ending in /
@@ -419,8 +434,7 @@ ModelIter FuelClient::Models(const ModelIdentifier &_id) const
     path = path / _id.Owner() / "models";
 
   if (path.Str().empty())
-    // cppcheck-suppress identicalConditionAfterEarlyExit
-    return localIter;
+    return ModelIterFactory::Create();
 
   gzmsg << _id.UniqueName() << " not found in cache, attempting download\n";
 

src/FuelClient_TEST.cc

@@ -1366,6 +1366,52 @@ TEST_F(FuelClientTest, Models)
   }
 }
 
+//////////////////////////////////////////////////
+TEST_F(FuelClientTest, ModelsCheckCached)
+{
+  ClientConfig config;
+  std::string cacheDir = common::joinPaths(common::cwd(), "test_cache");
+  common::removeAll(cacheDir );
+  ASSERT_TRUE(common::createDirectories(cacheDir));
+  config.SetCacheLocation(cacheDir);
+  FuelClient client(config);
+  ServerConfig serverConfig;
+  ModelIdentifier modelId;
+  modelId.SetOwner("openroboticstest");
+  std::vector<Model> modelInfos;
+  {
+    for (ModelIter iter = client.Models(modelId, true); iter; ++iter)
+    {
+      modelInfos.push_back(*iter);
+    }
+  }
+  ASSERT_FALSE(modelInfos.empty());
+  // Download one of the models to test the behavior of Models() with
+  // different values for _checkCache
+  EXPECT_TRUE(client.DownloadModel(modelInfos.front().Identification()));
+  {
+    std::size_t counter = 0;
+    for (ModelIter iter = client.Models(modelId, true); iter; ++iter, ++counter)
+    {
+    }
+    std::cout << "counter: " << counter << std::endl;
+    // Expect only one result with checkCache=true because we've downloaded only
+    // one model
+    EXPECT_EQ(counter, 1u);
+    EXPECT_GT(modelInfos.size(), counter);
+  }
+
+  {
+    std::size_t counter = 0;
+    for (ModelIter iter = client.Models(modelId, false); iter;
+         ++iter, ++counter)
+    {
+    }
+    std::cout << "counter: " << counter << std::endl;
+    EXPECT_EQ(counter, modelInfos.size());
+  }
+}
+
 /////////////////////////////////////////////////
 // Protocol "https" not supported or disabled in libcurl for Windows
 // https://github.com/gazebosim/gz-fuel-tools/issues/105

src/ModelIter.cc

@@ -148,65 +148,55 @@ IterRestIds::~IterRestIds()
 {
 }
 
+std::vector<ModelIdentifier> IterRestIds::ParseIdsFromResponse(
+    const RestResponse &resp)
+{
+  if (resp.data == "null\n" || resp.statusCode != 200)
+    return {};
+
+  // Parse the response.
+  return JSONParser::ParseModels(resp.data, this->config);
+}
+
 //////////////////////////////////////////////////
 IterRestIds::IterRestIds(const Rest &_rest, const ServerConfig &_config,
     const std::string &_api)
-  : config(_config), rest(_rest)
+  : config(_config), rest(_rest), api(_api)
+{
+  this->idIter = this->ids.begin();
+  this->Next();
+}
+
+//////////////////////////////////////////////////
+RestResponse IterRestIds::MakeRestRequest(std::size_t _page)
 {
   HttpMethod method = HttpMethod::GET;
-  this->config = _config;
-  int page = 1;
   std::vector<std::string> headers = {"Accept: application/json"};
-  RestResponse resp;
   std::vector<ModelIdentifier> modelIds;
-  this->ids.clear();
-
-  do
-  {
-    // Prepare the request with the next page.
-    std::string queryStrPage = "page=" + std::to_string(page);
-    std::string path = _api;
-    ++page;
-
-    // Fire the request.
-    resp = this->rest.Request(method, this->config.Url().Str(),
+  // Prepare the request with the requested page.
+  std::string queryStrPage = "page=" + std::to_string(_page);
+  std::string path = this->api;
+  // Fire the request.
+  return this->rest.Request(method, this->config.Url().Str(),
       this->config.Version(),
       std::regex_replace(path, std::regex(R"(\\)"), "/"),
       {queryStrPage}, headers, "");
-
-    // TODO(nkoenig): resp.statusCode should return != 200 when the page
-    // requested does
-    // not exist. When this happens we should stop without calling ParseModels()
-    if (resp.data == "null\n" || resp.statusCode != 200)
-      break;
-
-    // Parse the response.
-    modelIds = JSONParser::ParseModels(resp.data, this->config);
-
-    // Add the vector of models to the list.
-    this->ids.insert(std::end(this->ids), std::begin(modelIds),
-      std::end(modelIds));
-  } while (!modelIds.empty());
-
-  if (this->ids.empty())
-    return;
-
-  this->idIter = this->ids.begin();
-
-  // make first model
-  std::shared_ptr<ModelPrivate> ptr(new ModelPrivate);
-  ptr->id = *(this->idIter);
-  ptr->id.SetServer(this->config);
-  this->model = Model(ptr);
-
-  gzdbg << "Got response [" << resp.data << "]\n";
 }
 
 //////////////////////////////////////////////////
 void IterRestIds::Next()
 {
   // advance pointer
-  ++(this->idIter);
+  if (this->idIter != this->ids.end())
+    ++(this->idIter);
+
+  if (this->idIter == this->ids.end())
+  {
+    ++this->currentPage;
+    RestResponse resp = this->MakeRestRequest(this->currentPage);
+    this->ids = this->ParseIdsFromResponse(resp);
+    this->idIter = this->ids.begin();
+  }
 
   // Update personal model class
   if (this->idIter != this->ids.end())
@@ -216,7 +206,6 @@ void IterRestIds::Next()
     ptr->id.SetServer(this->config);
     this->model = Model(ptr);
   }
-  // TODO(nkoenig) request next page if api is paginated
 }
 
 //////////////////////////////////////////////////
@@ -257,7 +246,6 @@ ModelIter::operator bool() const
 //////////////////////////////////////////////////
 ModelIter &ModelIter::operator++()
 {
-  // TODO(nkoenig) Request more data if there are more pages
   if (!this->dataPtr->HasReachedEnd())
   {
     this->dataPtr->Next();

src/ModelIterPrivate.hh

@@ -130,7 +130,7 @@ namespace gz
     };
 
     /// \brief class for iterating through model ids from a rest API
-    class GZ_FUEL_TOOLS_VISIBLE IterRestIds: public ModelIterPrivate
+    class GZ_FUEL_TOOLS_HIDDEN IterRestIds: public ModelIterPrivate
     {
       /// \brief constructor
       public: IterRestIds(const Rest &_rest,
@@ -153,11 +153,29 @@ namespace gz
       /// \brief RESTful client
       public: Rest rest;
 
+      /// \brief The API (path) of the RESTful requests
+      public: const std::string api;
+
+      /// \brief Make a RESTful request for the given page
+      /// \param[in] _page Page number to request
+      /// \return Response from the request
+      protected: RestResponse MakeRestRequest(std::size_t _page);
+
+      /// \brief Parse model identifiers from a RESTful response
+      /// \param[in] _resp RESTful response
+      /// \return A vector of identifiers extracted from the response.
+      protected: std::vector<ModelIdentifier> ParseIdsFromResponse(
+          const RestResponse &_resp);
+
       /// \brief Model identifiers in the current page
       protected: std::vector<ModelIdentifier> ids;
 
       /// \brief Where the current iterator is in the list of ids
       protected: std::vector<ModelIdentifier>::iterator idIter;
+
+      /// \brief Keep track of page number for pagination of response data from
+      /// server.
+      protected: std::size_t currentPage{0};
     };
   }
 }

src/Zip.cc

@@ -37,7 +37,7 @@ bool CompressFile(zip *_archive, const std::string &_file,
 {
   if (gz::common::isDirectory(_file))
   {
-    if (zip_add_dir(_archive, _entry.c_str()) < 0)
+    if (zip_dir_add(_archive, _entry.c_str(), 0) < 0)
     {
       gzerr << "Error adding directory to zip: " << _file << std::endl;
       return false;
@@ -69,7 +69,7 @@ bool CompressFile(zip *_archive, const std::string &_file,
       return false;
     }
 
-    if (zip_add(_archive, _entry.c_str(), source)
+    if (zip_file_add(_archive, _entry.c_str(), source, 0)
         < 0)
     {
       gzerr << "Error adding file to zip: " << _file << std::endl;