Fix review comments.

* Simplify SQL logic for delegations. * Reduce if/for nesting for target searching. Signed-off-by: Patrick Vacek <[email protected]>
advancedtelematic · Jan 31, 2019 · e51c532 · e51c532
1 parent e655ae5
commit e51c532
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 36 deletions.
diff --git a/src/libaktualizr/storage/sqlstorage.cc b/src/libaktualizr/storage/sqlstorage.cc
@@ -522,17 +522,10 @@ void SQLStorage::storeDelegation(const std::string& data, const Uptane::Role rol
     return;
   }
 
-  auto del_statement = db.prepareStatement<std::string>("DELETE FROM delegations WHERE role_name=?;", role.ToString());
+  auto statement = db.prepareStatement<SQLBlob, std::string>("INSERT OR REPLACE INTO delegations VALUES (?, ?);",
+                                                             SQLBlob(data), role.ToString());
 
-  if (del_statement.step() != SQLITE_DONE) {
-    LOG_ERROR << "Can't clear delegation metadata: " << db.errmsg();
-    return;
-  }
-
-  auto ins_statement = db.prepareStatement<SQLBlob, std::string>("INSERT INTO delegations VALUES (?, ?);",
-                                                                 SQLBlob(data), role.ToString());
-
-  if (ins_statement.step() != SQLITE_DONE) {
+  if (statement.step() != SQLITE_DONE) {
     LOG_ERROR << "Can't add delegation metadata: " << db.errmsg();
     return;
   }
@@ -543,8 +536,8 @@ void SQLStorage::storeDelegation(const std::string& data, const Uptane::Role rol
 bool SQLStorage::loadDelegation(std::string* data, const Uptane::Role role) {
   SQLite3Guard db = dbConnection();
 
-  auto statement = db.prepareStatement<std::string>(
-      "SELECT meta FROM delegations WHERE role_name=? ORDER BY role_name DESC LIMIT 1;", role.ToString());
+  auto statement =
+      db.prepareStatement<std::string>("SELECT meta FROM delegations WHERE role_name=? LIMIT 1;", role.ToString());
   int result = statement.step();
 
   if (result == SQLITE_DONE) {

diff --git a/src/libaktualizr/uptane/imagesrepository.cc b/src/libaktualizr/uptane/imagesrepository.cc
@@ -127,33 +127,33 @@ std::unique_ptr<Uptane::Target> ImagesRepository::getTarget(const Uptane::Target
   // Search for the target in the top-level targets metadata.
   Uptane::Targets toplevel = targets["targets"];
   auto it = std::find(toplevel.targets.cbegin(), toplevel.targets.cend(), director_target);
-  if (it == toplevel.targets.cend()) {
-    // Check if the target matches any of the delegation paths.
-    for (const auto& delegate_name : toplevel.delegated_role_names_) {
-      Role delegate_role = Role::Delegation(delegate_name);
-      std::vector<std::string> patterns = toplevel.paths_for_role_[delegate_role];
-      for (const auto& pattern : patterns) {
-        if (fnmatch(pattern.c_str(), director_target.filename().c_str(), 0) == 0) {
-          // Match! Check delegation.
-          Uptane::Targets delegate = targets[delegate_name];
-          auto d_it = std::find(delegate.targets.cbegin(), delegate.targets.cend(), director_target);
-          if (d_it == delegate.targets.cend()) {
-            // TODO: recurse here if the delegation is non-terminating. For now,
-            // assume that all delegations are terminating.
-            if (!toplevel.terminating_role_[delegate_role]) {
-              LOG_ERROR << "Nested delegations are not currently supported.";
-            }
-            return std::unique_ptr<Uptane::Target>(nullptr);
-          } else {
-            return std_::make_unique<Uptane::Target>(*d_it);
-          }
-        }
+  if (it != toplevel.targets.cend()) {
+    return std_::make_unique<Uptane::Target>(*it);
+  }
+
+  // Check if the target matches any of the delegation paths.
+  for (const auto& delegate_name : toplevel.delegated_role_names_) {
+    Role delegate_role = Role::Delegation(delegate_name);
+    std::vector<std::string> patterns = toplevel.paths_for_role_[delegate_role];
+    for (const auto& pattern : patterns) {
+      if (fnmatch(pattern.c_str(), director_target.filename().c_str(), 0) != 0) {
+        continue;
+      }
+      // Match! Check delegation.
+      Uptane::Targets delegate = targets[delegate_name];
+      auto d_it = std::find(delegate.targets.cbegin(), delegate.targets.cend(), director_target);
+      if (d_it != delegate.targets.cend()) {
+        return std_::make_unique<Uptane::Target>(*d_it);
       }
+      // TODO: recurse here if the delegation is non-terminating. For now,
+      // assume that all delegations are terminating.
+      if (!toplevel.terminating_role_[delegate_role]) {
+        LOG_ERROR << "Nested delegations are not currently supported.";
+      }
+      return std::unique_ptr<Uptane::Target>(nullptr);
     }
-    return std::unique_ptr<Uptane::Target>(nullptr);
-  } else {
-    return std_::make_unique<Uptane::Target>(*it);
   }
+  return std::unique_ptr<Uptane::Target>(nullptr);
 }
 
 }  // namespace Uptane