4582084 REVIEW: With `PayloadUpsert` in `radicle`, we can avoid introducing another ad-hoc struct, and using `itertools`. — heartwood

heartwood

Radicle Heartwood Protocol & Stack

REVIEW: With `PayloadUpsert` in `radicle`, we can avoid introducing another ad-hoc struct, and using `itertools`.

✗ CI failure Lorenz Leutgeb committed 6 months ago

commit 45820843c6d147802b49a780dda800dcafb52a69
parent 53c39f7a783054515e2bc4646273a6391df4c0e6

1 failed (1 total) View logs

3 files changed +73 -71

modified crates/radicle-cli/src/commands/id.rs

@@ -4,8 +4,6 @@ use std::collections::BTreeSet;

 use anyhow::{anyhow, Context};
 use itertools::Itertools as _;
 use radicle::cob::identity::{self, IdentityMut, Revision, RevisionId};
 use radicle::cob::Title;
 use radicle::identity::doc::update;

@@ -163,9 +161,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {

                 // TODO(erikli): whenever `clap` starts supporting custom value parsers
                 // for a series of values, we can parse into `Payload` implicitly.
                 let payloads = args::PayloadUpsert::parse_many(&payload)
                     .map_ok(|p| (p.id, p.key, p.value))
                     .collect::<Result<Vec<_>, _>>()?;
                 let payloads = args::parse_many_upserts(&payload).collect::<Result<Vec<_>, _>>()?;
                 update::payload(proposal, payloads)?
             };

modified crates/radicle-cli/src/commands/id/args.rs

@@ -9,6 +9,7 @@ use thiserror::Error;

 use radicle::cob::{Title, TypeNameParse};
 use radicle::identity::doc::update::EditVisibility;
 use radicle::identity::doc::update::PayloadUpsert;
 use radicle::identity::doc::PayloadId;
 use radicle::prelude::{Did, RepoId};

@@ -32,43 +33,34 @@ pub enum PayloadUpsertParseError {

     Value(#[from] json::Error),
 }
 #[derive(Clone, Debug)]
 pub struct PayloadUpsert {
     pub(super) id: PayloadId,
     pub(super) key: String,
     pub(super) value: json::Value,
 }
 impl PayloadUpsert {
     /// Parses a slice of all payload upserts as aggregated by `clap`
     /// (see [`Command::Update::payload`]).
     /// E.g. `["com.example.one", "name", "1", "com.example.two", "name2", "2"]`
     /// will result in iterator over two [`PayloadUpsert`]s.
     ///
     /// # Panics
     ///
     /// If the length of `values` is not divisible by 3.
     /// (To catch errors in the definition of the parser derived from
     /// [`Command::Update`] or `clap` itself, and unexpected changes to
     /// `clap`s behaviour in the future.)
     pub(super) fn parse_many(
         values: &[String],
     ) -> impl Iterator<Item = Result<Self, PayloadUpsertParseError>> + use<'_> {
         // `clap` ensures we have 3 values per option occurrence,
         // so we can chunk the aggregated slice exactly.
         let chunks = values.chunks_exact(3);
         assert!(chunks.remainder().is_empty());
         chunks.map(|chunk| {
             // Slice accesses will not panic, guaranteed by `chunks_exact(3)`.
             Ok(PayloadUpsert {
                 id: PayloadId::from_str(&chunk[0])?,
                 key: chunk[1].to_owned(),
                 value: json::from_str(&chunk[2].to_owned())?,
             })
 /// Parses a slice of all payload upserts as aggregated by `clap`
 /// (see [`Command::Update::payload`]).
 /// E.g. `["com.example.one", "name", "1", "com.example.two", "name2", "2"]`
 /// will result in iterator over two [`PayloadUpsert`]s.
 ///
 /// # Panics
 ///
 /// If the length of `values` is not divisible by 3.
 /// (To catch errors in the definition of the parser derived from
 /// [`Command::Update`] or `clap` itself, and unexpected changes to
 /// `clap`s behaviour in the future.)
 pub(super) fn parse_many_upserts(
     values: &[String],
 ) -> impl Iterator<Item = Result<PayloadUpsert, PayloadUpsertParseError>> + use<'_> {
     // `clap` ensures we have 3 values per option occurrence,
     // so we can chunk the aggregated slice exactly.
     let chunks = values.chunks_exact(3);
     assert!(chunks.remainder().is_empty());
     chunks.map(|chunk| {
         // Slice accesses will not panic, guaranteed by `chunks_exact(3)`.
         Ok(PayloadUpsert {
             id: PayloadId::from_str(&chunk[0])?,
             key: chunk[1].to_owned(),
             value: json::from_str(&chunk[2].to_owned())?,
         })
     }
     })
 }
 #[derive(Clone, Debug)]

@@ -247,7 +239,7 @@ pub(super) enum Command {

 #[cfg(test)]
 mod test {
     use super::{Args, PayloadUpsert};
     use super::{parse_many_upserts, Args};
     use clap::error::ErrorKind;
     use clap::Parser;

@@ -316,7 +308,7 @@ mod test {

     #[test]
     fn should_parse_into_payload() {
         let payload: Result<Vec<_>, _> = PayloadUpsert::parse_many(&[
         let payload: Result<Vec<_>, _> = parse_many_upserts(&[
             "xyz.radicle.project".to_string(),
             "name".to_string(),
             "{}".to_string(),

@@ -329,7 +321,7 @@ mod test {

     #[should_panic(expected = "assertion failed: chunks.remainder().is_empty()")]
     fn should_not_parse_into_payload() {
         let _: Result<Vec<_>, _> =
             PayloadUpsert::parse_many(&["xyz.radicle.project".to_string(), "name".to_string()])
             parse_many_upserts(&["xyz.radicle.project".to_string(), "name".to_string()])
                 .collect();
     }
 }

modified crates/radicle/src/identity/doc/update.rs

@@ -139,40 +139,52 @@ where

     }
 }
 /// [`Payload`]: super::Payload
 /// A change (update or insertion) to particular `key` within a [`Payload`]
 /// in a document.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct PayloadUpsert {
     /// [`Payload`]: super::Payload
     /// The identifier for the document [`Payload`].
     pub id: PayloadId,
     /// [`Payload`]: super::Payload
     /// The key within the [`Payload`] that is being updated.
     pub key: String,
     /// [`Payload`]: super::Payload
     /// The value to update within the [`Payload`].
     pub value: json::Value,
 }
 // TODO(finto): I think this API would likely be much nicer if we use [JSON Patch][patch] and [JSON Merge Patch][merge]
 //
 // [patch]: https://datatracker.ietf.org/doc/html/rfc6902
 // [merge]: https://datatracker.ietf.org/doc/html/rfc7396
 /// Change the payload of the document, using the set of triples:
 ///
 ///   - [`PayloadId`]: the identifier for the document [`Payload`]
 ///   - [`String`]: the key within the [`Payload`] that is being updated
 ///   - [`json::Value`]: the value to update the [`Payload`]
 /// [`Payload`]: super::Payload
 /// Change (update or insert) a key in a [`Payload`] of the document,
 /// using the provided `updates`.
 ///
 /// # Errors
 ///
 /// This fails if one of the [`PayloadId`]s does not point to a JSON object as
 /// its value.
 ///
 /// [`Payload`]: super::Payload
 pub fn payload(
     mut raw: RawDoc,
     payload: Vec<(PayloadId, String, json::Value)>,
     upserts: impl IntoIterator<Item = PayloadUpsert>,
 ) -> Result<RawDoc, error::PayloadError> {
     for (id, key, val) in payload {
     for PayloadUpsert { id, key, value } in upserts {
         if let Some(ref mut payload) = raw.payload.get_mut(&id) {
             if let Some(obj) = payload.as_object_mut() {
                 if val.is_null() {
                 if value.is_null() {
                     obj.remove(&key);
                 } else {
                     obj.insert(key, val);
                     obj.insert(key, value);
                 }
             } else {
                 return Err(error::PayloadError::ExpectedObject { id });
             }
         } else {
             raw.payload
                 .insert(id, serde_json::json!({ key: val }).into());
                 .insert(id, serde_json::json!({ key: value }).into());
         }
     }
     Ok(raw)

@@ -277,21 +289,23 @@ mod test {

         test::arbitrary,
     };
     use super::PayloadUpsert;
     #[test]
     fn test_can_update_crefs() {
         let raw = arbitrary::gen::<RawDoc>(1);
         let raw = super::payload(
             raw,
             vec![(
                 PayloadId::canonical_refs(),
                 "rules".to_string(),
                 json!({
             [PayloadUpsert {
                 id: PayloadId::canonical_refs(),
                 key: "rules".to_string(),
                 value: json!({
                     "refs/tags/*": {
                         "threshold": 1,
                         "allow": "delegates"
                     }
                 }),
             )],
             }],
         )
         .unwrap();
         let verified = super::verify(raw);

@@ -306,10 +320,10 @@ mod test {

         ));
         let raw = super::payload(
             raw,
             vec![(
                 PayloadId::canonical_refs(),
                 "rules".to_string(),
                 json!({
             [PayloadUpsert {
                 id: PayloadId::canonical_refs(),
                 key: "rules".to_string(),
                 value: json!({
                     "refs/tags/*": {
                         "threshold": 1,
                         "allow": "delegates"

@@ -319,7 +333,7 @@ mod test {

                         "allow": "delegates",
                     }
                 }),
             )],
             }],
         )
         .unwrap();
         assert!(

@@ -339,16 +353,16 @@ mod test {

         ));
         let raw = super::payload(
             raw,
             vec![(
                 PayloadId::canonical_refs(),
                 "rules".to_string(),
                 json!({
             [PayloadUpsert {
                 id: PayloadId::canonical_refs(),
                 key: "rules".to_string(),
                 value: json!({
                     "refs/tags/*": {
                         "threshold": 1,
                         "allow": "delegates"
                     }
                 }),
             )],
             }],
         )
         .unwrap();
         let verified = super::verify(raw).unwrap();