RMST-236: iterates the list of signatures in CollectionMetadata

[leplatrem]

Pull Request checklist

• Breaking changes: This PR follows our breaking change policy
  • This PR follows the breaking change policy:
    • This PR has no breaking API changes, or
    • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
• Quality: This PR builds and tests run cleanly
  • Note:
    • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
    • If this pull request includes a breaking change, consider cutting a new release after merging.
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
  • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
• Dependencies: This PR follows our dependency management guidelines
  • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

[alexcottner]

looks reasonable to me

[leplatrem]

Is it more idiomatic to do this as it was?

--- a/components/remote_settings/src/storage.rs
+++ b/components/remote_settings/src/storage.rs
@@ -140,7 +140,7 @@ impl Storage {
         let mut stmt_metadata = tx.prepare(
             "SELECT bucket, signatures FROM collection_metadata WHERE collection_url = ?",
         )?;
-        let result = stmt_metadata
+        if let Some(metadata) = stmt_metadata
             .query_row(params![collection_url], |row| {
                 let bucket: String = row.get(0)?;
                 let signatures_json: String = row.get(1)?;
@@ -154,8 +154,12 @@ impl Storage {
                 })?;
                 Ok(CollectionMetadata { bucket, signatures })
             })
-            .optional()?;
-        Ok(result)
+            .optional()?
+        {
+            Ok(Some(metadata))
+        } else {
+            Ok(None)
+        }
     }

[alexcottner]

Is it more idiomatic to do this as it was?

I think I prefer the way you have it now slightly. But I'm still somewhat new to rust.

[bendk]

Is it more idiomatic to do this as it was?

I think I prefer the way you have it now slightly. But I'm still somewhat new to rust.

I think the way you have it is fine. But maybe you could just do something like this:

stmt_metadata
  .query_row(params![collection_url], |row| {
    // ...
   })
   .optional()

[bendk]

Looks right to me and the tests are great. I left a few suggestions, but none of them are required. Whatever you think is best is good with me.

[bendk]

BTW, I don't think it helps here but there's also the .transpose() method which swaps between Result<Option<T>> and Option<Result<T>>. I find that useful in a lot of situations where you're dealing with both. When the types get too complicated to track in my mind, I usually write the types out explicitly and see what works:

let foo: Option<Foo> = some_computation()
   .map(|result| 
     // some more complicated stuff
   )?;
   // and if that doesn't work, I change the last line to ).transpose()?