mab122
/
radicle-heartwood-lfs
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

cob: Return `RevisionId` instead of `Revision` 

Previously we panicked if a revision wasn't found after an update. But
it can happen that the update goes through yet a concurrent update
prevents the revision from being valid.

To partially deal with this, we don't try to lookup the revision after
an update, we simply return the ID. This means a subsequent lookup can
fail, and the error can be handled there.
This commit is contained in:
cloudhead 2023-12-11 12:59:52 +01:00
parent 57e3b2a154
commit ecb59d687b
No known key found for this signature in database
3 changed files with 55 additions and 66 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
radicle-cli/src/commands/id.rs
View File

@ -278,7 +278,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
.interactive .interactive
.confirm(format!("Accept revision {}?", term::format::tertiary(id))) .confirm(format!("Accept revision {}?", term::format::tertiary(id)))
{ {
identity.accept(&revision, &signer)?; identity.accept(&revision.id, &signer)?;
if let Some(revision) = identity.revision(&id) { if let Some(revision) = identity.revision(&id) {
// Update the canonical head to point to the latest accepted revision. // Update the canonical head to point to the latest accepted revision.
@ -605,8 +605,12 @@ fn update<R: WriteRepository + cob::Store, G: Signer>(
signer: &G, signer: &G,
) -> anyhow::Result<Revision> { ) -> anyhow::Result<Revision> {
if let Some((title, description)) = edit_title_description(title, description)? { if let Some((title, description)) = edit_title_description(title, description)? {
let revision = current.update(title, description, &doc, signer)?; let id = current.update(title, description, &doc, signer)?;
Ok(revision) let revision = current
.revision(&id)
.ok_or(anyhow!("update failed: revision {id} is missing"))?;
Ok(revision.clone())
} else { } else {
Err(anyhow!("you must provide a revision title and description")) Err(anyhow!("you must provide a revision title and description"))
} }

2
radicle/src/cob.rs
View File

@ -17,4 +17,4 @@ pub use radicle_cob::{
Create, Embed, Entry, Evaluate, History, Manifest, ObjectId, Store, TypeName, Update, Updated, Create, Embed, Entry, Evaluate, History, Manifest, ObjectId, Store, TypeName, Update, Updated,
Version, Version,
}; };
pub use radicle_cob::{create, get, list, remove, update}; pub use radicle_cob::{create, get, git, list, remove, update};

109
radicle/src/cob/identity.rs
View File

@ -128,6 +128,8 @@ pub enum Error {
Op(#[from] op::OpEncodingError), Op(#[from] op::OpEncodingError),
#[error(transparent)] #[error(transparent)]
Doc(#[from] DocError), Doc(#[from] DocError),
#[error("revision {0} was not found")]
NotFound(RevisionId),
} }
/// An evolving identity document. /// An evolving identity document.
@ -890,26 +892,26 @@ where
description: impl ToString, description: impl ToString,
doc: &Doc<Verified>, doc: &Doc<Verified>,
signer: &G, signer: &G,
) -> Result<Revision, Error> { ) -> Result<RevisionId, Error> {
let parent = self.current; let parent = self.current;
let id = self.transaction("Propose revision", signer, |tx| { let id = self.transaction("Propose revision", signer, |tx| {
tx.revision(title, description, doc, Some(parent), signer) tx.revision(title, description, doc, Some(parent), signer)
})?; })?;
// SAFETY: Since the revision was just added, it's guaranteed to be there. Ok(id)
Ok(self
.revision(&id)
.expect("IdentityMut::update: revision exists")
.clone())
} }
/// Accept an active revision. /// Accept an active revision.
pub fn accept<G: Signer>(&mut self, revision: &Revision, signer: &G) -> Result<EntryId, Error> { pub fn accept<G: Signer>(
&mut self,
revision: &RevisionId,
signer: &G,
) -> Result<EntryId, Error> {
let id = *revision;
let revision = self.revision(revision).ok_or(Error::NotFound(id))?;
let signature = revision.sign(signer)?; let signature = revision.sign(signer)?;
self.transaction("Accept revision", signer, |tx| { self.transaction("Accept revision", signer, |tx| tx.accept(id, signature))
tx.accept(revision.id, signature)
})
} }
/// Reject an active revision. /// Reject an active revision.
@ -1042,10 +1044,7 @@ mod test {
// Let's add another delegate. // Let's add another delegate.
doc.delegate(bob.public_key()); doc.delegate(bob.public_key());
// The update should go through now. // The update should go through now.
let r1 = identity let r1 = identity.update(title, description, &doc, signer).unwrap();
.update(title, description, &doc, signer)
.unwrap()
.id;
assert!(identity.revision(&r1).unwrap().is_accepted()); assert!(identity.revision(&r1).unwrap().is_accepted());
assert_eq!(identity.current, r1); assert_eq!(identity.current, r1);
// With two delegates now, we need two signatures for any update to go through. // With two delegates now, we need two signatures for any update to go through.
@ -1055,16 +1054,16 @@ mod test {
let r2 = identity.update(title, description, &doc, signer).unwrap(); let r2 = identity.update(title, description, &doc, signer).unwrap();
// R1 is still the head. // R1 is still the head.
assert_eq!(identity.current, r1); assert_eq!(identity.current, r1);
assert_eq!(r2.state, State::Active); assert_eq!(identity.revision(&r2).unwrap().state, State::Active);
assert_eq!(repo.canonical_identity_head().unwrap(), r1); assert_eq!(repo.canonical_identity_head().unwrap(), r1);
assert_eq!(repo.identity_doc().unwrap().visibility, Visibility::Public); assert_eq!(repo.identity_doc().unwrap().visibility, Visibility::Public);
// Now let's add a signature on R2 from Bob. // Now let's add a signature on R2 from Bob.
identity.accept(&r2, &bob).unwrap(); identity.accept(&r2, &bob).unwrap();
// R2 is now the head. // R2 is now the head.
assert_eq!(identity.current, r2.id); assert_eq!(identity.current, r2);
assert_eq!(identity.revision(&r2.id).unwrap().state, State::Accepted); assert_eq!(identity.revision(&r2).unwrap().state, State::Accepted);
assert_eq!(repo.canonical_identity_head().unwrap(), r2.id); assert_eq!(repo.canonical_identity_head().unwrap(), r2);
assert_eq!( assert_eq!(
repo.canonical_identity_doc().unwrap().visibility, repo.canonical_identity_doc().unwrap().visibility,
Visibility::private([]) Visibility::private([])
@ -1084,10 +1083,7 @@ mod test {
// Let's add another delegate. // Let's add another delegate.
doc.delegate(bob.public_key()); doc.delegate(bob.public_key());
let r1 = identity let r1 = identity.update(title, description, &doc, signer).unwrap();
.update(title, description, &doc, signer)
.unwrap()
.id;
assert_eq!(identity.current, r1); assert_eq!(identity.current, r1);
doc.visibility = Visibility::private([]); doc.visibility = Visibility::private([]);
@ -1096,8 +1092,8 @@ mod test {
.unwrap(); .unwrap();
// 1/2 rejected means that we can never reach the required 2/2 votes. // 1/2 rejected means that we can never reach the required 2/2 votes.
identity.reject(r2.id, &bob).unwrap(); identity.reject(r2, &bob).unwrap();
let r2 = identity.revision(&r2.id).unwrap(); let r2 = identity.revision(&r2).unwrap();
assert_eq!(r2.state, State::Rejected); assert_eq!(r2.state, State::Rejected);
// Now let's add another delegate. // Now let's add another delegate.
@ -1106,7 +1102,7 @@ mod test {
.update("Add Eve", description, &doc, &node.signer) .update("Add Eve", description, &doc, &node.signer)
.unwrap(); .unwrap();
let _ = identity.accept(&r3, &bob).unwrap(); let _ = identity.accept(&r3, &bob).unwrap();
assert_eq!(identity.current, r3.id); assert_eq!(identity.current, r3);
doc.visibility = Visibility::Public; doc.visibility = Visibility::Public;
let r3 = identity let r3 = identity
@ -1114,8 +1110,8 @@ mod test {
.unwrap(); .unwrap();
// 1/3 rejected means that we can still reach the 2/3 required votes. // 1/3 rejected means that we can still reach the 2/3 required votes.
identity.reject(r3.id, &bob).unwrap(); identity.reject(r3, &bob).unwrap();
let r3 = identity.revision(&r3.id).unwrap().clone(); let r3 = identity.revision(&r3).unwrap().clone();
assert_eq!(r3.state, State::Active); // Still active. assert_eq!(r3.state, State::Active); // Still active.
// 2/3 rejected means that we can no longer reach the 2/3 required votes. // 2/3 rejected means that we can no longer reach the 2/3 required votes.
@ -1138,8 +1134,7 @@ mod test {
alice_doc.delegate(bob.signer.public_key()); alice_doc.delegate(bob.signer.public_key());
let a1 = alice_identity let a1 = alice_identity
.update("Add Bob", "", &alice_doc, &alice.signer) .update("Add Bob", "", &alice_doc, &alice.signer)
.unwrap() .unwrap();
.id;
bob.repo.fetch(alice); bob.repo.fetch(alice);
@ -1155,8 +1150,7 @@ mod test {
// Bob makes the same change without knowing Alice already did. // Bob makes the same change without knowing Alice already did.
let b1 = bob_identity let b1 = bob_identity
.update("Make private", "", &alice_doc, &bob.signer) .update("Make private", "", &alice_doc, &bob.signer)
.unwrap() .unwrap();
.id;
// Bob gets Alice's data. // Bob gets Alice's data.
bob.repo.fetch(alice); bob.repo.fetch(alice);
@ -1168,17 +1162,14 @@ mod test {
alice.repo.fetch(bob); alice.repo.fetch(bob);
alice_identity.reload().unwrap(); alice_identity.reload().unwrap();
assert_eq!(alice_identity.current, a1); assert_eq!(alice_identity.current, a1);
assert_eq!(bob_identity.revision(&a2.id).unwrap().state, State::Active); assert_eq!(bob_identity.revision(&a2).unwrap().state, State::Active);
assert_eq!(bob_identity.revision(&b1).unwrap().state, State::Active); assert_eq!(bob_identity.revision(&b1).unwrap().state, State::Active);
// Now Bob accepts Alice's proposal. This voids his own. // Now Bob accepts Alice's proposal. This voids his own.
bob_identity.accept(&a2, &bob.signer).unwrap(); bob_identity.accept(&a2, &bob.signer).unwrap();
assert_eq!(bob_identity.current, a2.id); assert_eq!(bob_identity.current, a2);
assert_eq!(bob_identity.revision(&a1).unwrap().state, State::Accepted); assert_eq!(bob_identity.revision(&a1).unwrap().state, State::Accepted);
assert_eq!( assert_eq!(bob_identity.revision(&a2).unwrap().state, State::Accepted);
bob_identity.revision(&a2.id).unwrap().state,
State::Accepted
);
assert_eq!(bob_identity.revision(&b1).unwrap().state, State::Stale); assert_eq!(bob_identity.revision(&b1).unwrap().state, State::Stale);
} }
@ -1198,8 +1189,7 @@ mod test {
let a0 = alice_identity.root; let a0 = alice_identity.root;
let a1 = alice_identity let a1 = alice_identity
.update("Add Bob", "Eh.", &alice_doc, &alice.signer) .update("Add Bob", "Eh.", &alice_doc, &alice.signer)
.unwrap() .unwrap();
.id;
alice_doc.visibility = Visibility::private([eve.signer.public_key().into()]); alice_doc.visibility = Visibility::private([eve.signer.public_key().into()]);
let a2 = alice_identity let a2 = alice_identity
@ -1207,23 +1197,20 @@ mod test {
.unwrap(); .unwrap();
bob.repo.fetch(alice); bob.repo.fetch(alice);
let a3 = alice_identity.redact(a2.id, &alice.signer).unwrap(); let a3 = alice_identity.redact(a2, &alice.signer).unwrap();
assert!(alice_identity.revision(&a1).is_some()); assert!(alice_identity.revision(&a1).is_some());
assert_eq!(alice_identity.timeline, vec![a0, a1, a2.id, a3]); assert_eq!(alice_identity.timeline, vec![a0, a1, a2, a3]);
let mut bob_identity = Identity::load_mut(&*bob.repo).unwrap(); let mut bob_identity = Identity::load_mut(&*bob.repo).unwrap();
let b1 = bob_identity.accept(&a2, &bob.signer).unwrap(); let b1 = bob_identity.accept(&a2, &bob.signer).unwrap();
assert_eq!(bob_identity.timeline, vec![a0, a1, a2.id, b1]); assert_eq!(bob_identity.timeline, vec![a0, a1, a2, b1]);
assert_eq!( assert_eq!(bob_identity.revision(&a2).unwrap().state, State::Accepted);
bob_identity.revision(&a2.id).unwrap().state,
State::Accepted
);
bob.repo.fetch(alice); bob.repo.fetch(alice);
bob_identity.reload().unwrap(); bob_identity.reload().unwrap();
assert_eq!(bob_identity.timeline, vec![a0, a1, a2.id, a3, b1]); assert_eq!(bob_identity.timeline, vec![a0, a1, a2, a3, b1]);
assert_eq!(bob_identity.revision(&a2.id), None); assert_eq!(bob_identity.revision(&a2), None);
assert_eq!(bob_identity.current, a1); assert_eq!(bob_identity.current, a1);
} }
@ -1244,8 +1231,7 @@ mod test {
let a0 = alice_identity.root; let a0 = alice_identity.root;
let a1 = alice_identity let a1 = alice_identity
.update("Add Bob and Eve", "Eh.", &alice_doc, &alice.signer) .update("Add Bob and Eve", "Eh.", &alice_doc, &alice.signer)
.unwrap() .unwrap();
.id;
alice_doc.rescind(eve.signer.public_key()).unwrap(); alice_doc.rescind(eve.signer.public_key()).unwrap();
let a2 = alice_identity let a2 = alice_identity
@ -1258,7 +1244,7 @@ mod test {
let mut bob_identity = Identity::load_mut(&*bob.repo).unwrap(); let mut bob_identity = Identity::load_mut(&*bob.repo).unwrap();
let b1 = bob_identity.accept(&a2, &bob.signer).unwrap(); let b1 = bob_identity.accept(&a2, &bob.signer).unwrap();
assert_eq!(bob_identity.current, a2.id); assert_eq!(bob_identity.current, a2);
let mut eve_identity = Identity::load_mut(&*eve.repo).unwrap(); let mut eve_identity = Identity::load_mut(&*eve.repo).unwrap();
let mut eve_doc = eve_identity.doc().clone(); let mut eve_doc = eve_identity.doc().clone();
@ -1269,7 +1255,7 @@ mod test {
eve.repo.fetch(bob); eve.repo.fetch(bob);
eve_identity.reload().unwrap(); eve_identity.reload().unwrap();
assert_eq!(eve_identity.timeline, vec![a0, a1, a2.id, e1.id, b1]); assert_eq!(eve_identity.timeline, vec![a0, a1, a2, e1, b1]);
assert!(!eve_identity.is_delegate(eve.signer.public_key())); assert!(!eve_identity.is_delegate(eve.signer.public_key()));
} }
@ -1290,8 +1276,7 @@ mod test {
let a0 = alice_identity.root; let a0 = alice_identity.root;
let a1 = alice_identity let a1 = alice_identity
.update("Add Bob and Eve", "Eh.", &alice_doc, &alice.signer) .update("Add Bob and Eve", "Eh.", &alice_doc, &alice.signer)
.unwrap() .unwrap();
.id;
alice_doc.visibility = Visibility::private([]); alice_doc.visibility = Visibility::private([]);
let a2 = alice_identity let a2 = alice_identity
@ -1308,7 +1293,7 @@ mod test {
// Eve rejects the revision, not knowing. // Eve rejects the revision, not knowing.
let mut eve_identity = Identity::load_mut(&*eve.repo).unwrap(); let mut eve_identity = Identity::load_mut(&*eve.repo).unwrap();
let e1 = eve_identity.reject(a2.id, &eve.signer).unwrap(); let e1 = eve_identity.reject(a2, &eve.signer).unwrap();
// Then she submits a new revision. // Then she submits a new revision.
let mut eve_doc = eve_identity.doc().clone(); let mut eve_doc = eve_identity.doc().clone();
@ -1323,11 +1308,11 @@ mod test {
eve.repo.fetch(bob); eve.repo.fetch(bob);
eve_identity.reload().unwrap(); eve_identity.reload().unwrap();
assert_eq!(eve_identity.timeline, vec![a0, a1, a2.id, e1, b1, e2.id]); assert_eq!(eve_identity.timeline, vec![a0, a1, a2, e1, b1, e2]);
// Her revision is there, although stale, since another revision was accepted since. // Her revision is there, although stale, since another revision was accepted since.
// However, it wasn't pruned, even though rejecting an accepted revision is an error. // However, it wasn't pruned, even though rejecting an accepted revision is an error.
let e2 = eve_identity.revision(&e2.id).unwrap(); let e2 = eve_identity.revision(&e2).unwrap();
assert_eq!(e2.state, State::Stale); assert_eq!(e2.state, State::Stale);
} }
@ -1383,15 +1368,15 @@ mod test {
.update("Change visibility #2", "Woops", &eve_doc, &eve.signer) .update("Change visibility #2", "Woops", &eve_doc, &eve.signer)
.unwrap(); .unwrap();
assert_eq!(eve_identity.revisions().count(), 4); assert_eq!(eve_identity.revisions().count(), 4);
assert_eq!(e1.state, State::Active); assert_eq!(eve_identity.revision(&e1).unwrap().state, State::Active);
let a2 = alice_identity.accept(&b1, &alice.signer).unwrap(); let a2 = alice_identity.accept(&b1, &alice.signer).unwrap();
eve.repo.fetch(alice); eve.repo.fetch(alice);
eve_identity.reload().unwrap(); eve_identity.reload().unwrap();
assert_eq!(eve_identity.timeline, vec![a0, a1.id, b1.id, e1.id, a2]); assert_eq!(eve_identity.timeline, vec![a0, a1, b1, e1, a2]);
assert_eq!(eve_identity.revision(&e1.id).unwrap().state, State::Stale); assert_eq!(eve_identity.revision(&e1).unwrap().state, State::Stale);
} }
#[test] #[test]
@ -1448,7 +1433,7 @@ mod test {
let identity: Identity = Identity::load(&repo).unwrap(); let identity: Identity = Identity::load(&repo).unwrap();
let root = repo.identity_root().unwrap(); let root = repo.identity_root().unwrap();
let doc = repo.identity_doc_at(revision.id).unwrap(); let doc = repo.identity_doc_at(revision).unwrap();
assert_eq!(identity.signatures().count(), 2); assert_eq!(identity.signatures().count(), 2);
assert_eq!(identity.revisions().count(), 5); assert_eq!(identity.revisions().count(), 5);
@ -1456,7 +1441,7 @@ mod test {
assert_eq!(identity.root().id, root); assert_eq!(identity.root().id, root);
assert_eq!(identity.current().blob, doc.blob); assert_eq!(identity.current().blob, doc.blob);
assert_eq!(identity.current().description.as_str(), "Bob's repository"); assert_eq!(identity.current().description.as_str(), "Bob's repository");
assert_eq!(identity.head(), revision.id); assert_eq!(identity.head(), revision);
assert_eq!(identity.doc(), &*doc); assert_eq!(identity.doc(), &*doc);
assert_eq!( assert_eq!(
identity.doc().project().unwrap().description(), identity.doc().project().unwrap().description(),