Addressed comments

Signed-off-by: Nate Koenig <nate@openrobotics.org>
gazebosim · Jun 9, 2020 · aba1a14 · aba1a14
1 parent 2567e69
commit aba1a14
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 54 deletions.
diff --git a/include/ignition/fuel_tools/JSONParser.hh b/include/ignition/fuel_tools/JSONParser.hh
@@ -116,15 +116,6 @@ namespace ignition
       /// \return The list of tags.
       private: static std::vector<std::string> ParseTags(
                   const Json::Value &_json);
-
-      /// \brief Parse a JSON object as a license.
-      /// \param[in] _json JSON object containing a single license.
-      /// \param[out] _license License information where the first value in
-      /// the pair is the name of the license and the second value is
-      /// the license id on the Fuel server.
-      /// \return True if parsing succeeded or false otherwise
-      private: static bool ParseLicenseImpl(const Json::Value &_json,
-                   std::pair<std::string, unsigned int> &_license);
     };
   }
 }

diff --git a/src/FuelClient.cc b/src/FuelClient.cc
@@ -424,16 +424,22 @@ Result FuelClient::UploadModel(const std::string &_pathToModelDir,
   // a string. Otherwise, we have to bake into each client a mapping of
   // license name to integer.
   //
-  // If there is no license information, then default to
-  // "Creative Commons - Public Domain"
-  if (this->dataPtr->licenses.empty() || !meta.has_legal())
-  {
-    form.emplace("license", "1");
-  }
-  // Otherwise, we have license information (see PopulateLicenses) and
-  // legal information
-  else
+  // If we have legal, then attempt to fill in the correct license information.
+  if (meta.has_legal())
   {
+    // Attempt to retreive the available licenses, if we have no available
+    // licenses.
+    if (this->dataPtr->licenses.empty())
+    {
+      this->PopulateLicenses(_id.Server());
+      // Fail if a license has been requested, but we couldn't get the
+      // available licenses.
+      if (this->dataPtr->licenses.empty())
+      {
+        return Result(ResultType::UPLOAD_ERROR);
+      }
+    }
+
     // Find the license by name.
     std::map<std::string, unsigned int>::const_iterator licenseIt =
       this->dataPtr->licenses.find(meta.legal().license());
@@ -447,7 +453,6 @@ Result FuelClient::UploadModel(const std::string &_pathToModelDir,
       std::string validLicenseNames;
       auto end = this->dataPtr->licenses.end();
       std::advance(end, -1);
-      //for (size_t i = 0; i < this->dataPtr->licenses.size(); ++i)
       for (licenseIt = this->dataPtr->licenses.begin();
            licenseIt != end; ++licenseIt)
       {
@@ -462,6 +467,12 @@ Result FuelClient::UploadModel(const std::string &_pathToModelDir,
       return Result(ResultType::UPLOAD_ERROR);
     }
   }
+  // If there is no license information, then default to
+  // "Creative Commons - Public Domain"
+  else
+  {
+    form.emplace("license", "1");
+  }
 
   // Add tags
   std::string tags;

diff --git a/src/JSONParser.cc b/src/JSONParser.cc
@@ -55,6 +55,45 @@ std::time_t ParseDateTime(const std::string &_datetime)
   return timegm(&tm);
 }
 
+/////////////////////////////////////////////////
+/// \brief Parse a JSON object as a license.
+/// \param[in] _json JSON object containing a single license.
+/// \param[out] _license License information where the first value in
+/// the pair is the name of the license and the second value is
+/// the license id on the Fuel server.
+/// \return True if parsing succeeded or false otherwise
+bool ParseLicenseImpl(const Json::Value &_json,
+    std::pair<std::string, unsigned int> &_license)
+{
+  try
+  {
+    if (!_json.isObject())
+    {
+      ignerr << "License isn't a json object!\n";
+      return false;
+    }
+
+    if (_json.isMember("name"))
+      _license.first = _json["name"].asString();
+    if (_json.isMember("ID"))
+      _license.second = _json["ID"].asUInt();
+  }
+#if JSONCPP_VERSION_MAJOR < 1 && JSONCPP_VERSION_MINOR < 10
+  catch (...)
+  {
+    std::string what;
+#else
+  catch (const Json::LogicError &error)
+  {
+    std::string what = ": [" + std::string(error.what()) + "]";
+#endif
+    ignerr << "Bad response from server" << what << "\n";
+    return false;
+  }
+
+  return true;
+}
+
 /////////////////////////////////////////////////
 std::vector<std::string> JSONParser::ParseTags(const Json::Value &_json)
 {
@@ -346,36 +385,3 @@ bool JSONParser::ParseLicenses(const std::string &_json,
 
   return true;
 }
-
-/////////////////////////////////////////////////
-bool JSONParser::ParseLicenseImpl(const Json::Value &_json,
-    std::pair<std::string, unsigned int> &_license)
-{
-  try
-  {
-    if (!_json.isObject())
-    {
-      ignerr << "License isn't a json object!\n";
-      return false;
-    }
-
-    if (_json.isMember("name"))
-      _license.first = _json["name"].asString();
-    if (_json.isMember("ID"))
-      _license.second = _json["ID"].asUInt();
-  }
-#if JSONCPP_VERSION_MAJOR < 1 && JSONCPP_VERSION_MINOR < 10
-  catch (...)
-  {
-    std::string what;
-#else
-  catch (const Json::LogicError &error)
-  {
-    std::string what = ": [" + std::string(error.what()) + "]";
-#endif
-    ignerr << "Bad response from server" << what << "\n";
-    return false;
-  }
-
-  return true;
-}
diff --git a/src/JSONParser_TEST.cc b/src/JSONParser_TEST.cc
@@ -274,8 +274,7 @@ TEST(JSONParser, ParseLicenses)
         break;
       case 7:
         EXPECT_EQ(i, licenses[
-            "Creative Commons - Attribution - Non Commercial - No Derivatives"]
-            );
+           "Creative Commons - Attribution - Non Commercial - No Derivatives"]);
         break;
       default:
         FAIL() << "Invalid license id[" << i << "]\n";