Skip to content

Commit

Permalink
Reflect pagination of RESTful requests in iterator API (#350)
Browse files Browse the repository at this point in the history 
The ModelIter iterator was fetching all available pages before making the first model available. This PR makes it so that each page is fetched from Fuel when the iterator is advanced.

This also adds a new member function FuelClient::Models(onst ModelIdentifier &_id, bool _checkCache) that allows bypassing the cache when getting a list of models owned by a user from the server.

---------

Signed-off-by: Addisu Z. Taddese <[email protected]>
  • Loading branch information
@azeey
azeey authored Apr 26, 2023
1 parent d8ce77b commit fe66f33
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 60 deletions.
37 changes: 29 additions & 8 deletions include/gz/fuel_tools/FuelClient.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,25 +132,46 @@ namespace ignition
/// \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
Expand Down
20 changes: 19 additions & 1 deletion include/gz/fuel_tools/ModelIterPrivate.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ namespace ignition
};

/// \brief class for iterating through model ids from a rest API
class IGNITION_FUEL_TOOLS_VISIBLE IterRestIds: public ModelIterPrivate
class IGNITION_FUEL_TOOLS_HIDDEN IterRestIds: public ModelIterPrivate
{
/// \brief constructor
public: IterRestIds(const Rest &_rest,
Expand All @@ -153,11 +153,29 @@ namespace ignition
/// \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};
};
}
}
Expand Down
27 changes: 21 additions & 6 deletions src/FuelClient.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,16 +378,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: ign-fuel-server doesn't like URLs ending in /
Expand All @@ -398,7 +413,7 @@ ModelIter FuelClient::Models(const ModelIdentifier &_id) const
path = path / _id.Owner() / "models";

if (path.Str().empty())
return localIter;
return ModelIterFactory::Create();

ignmsg << _id.UniqueName() << " not found in cache, attempting download\n";

Expand Down
46 changes: 46 additions & 0 deletions src/FuelClient_TEST.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1384,6 +1384,52 @@ TEST_F(FuelClientTest, Models)
}
}

//////////////////////////////////////////////////
TEST_F(FuelClientTest, ModelsCheckCached)
{
ClientConfig config;
std::string cacheDir = common::joinPaths(PROJECT_BINARY_PATH, "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
Expand Down
78 changes: 33 additions & 45 deletions src/ModelIter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,65 +147,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);

igndbg << "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())
Expand All @@ -215,7 +205,6 @@ void IterRestIds::Next()
ptr->id.SetServer(this->config);
this->model = Model(ptr);
}
// TODO(nkoenig) request next page if api is paginated
}

//////////////////////////////////////////////////
Expand Down Expand Up @@ -256,7 +245,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();
Expand Down

0 comments on commit fe66f33

Please sign in to comment.