Need error handling for inserting duplicate element into azks

[kevinlewi]

Right now, if you insert an element that is already a member of azks, we get a HistoryTreeNodeErr(DirectionIsNone) error. Perhaps it would be best to clarify that it is a duplicate insertion error?

We can also add a test for this error.

[slawlor]

@eozturk1 was this solved with #102 ?

[eozturk1]

Unfortunately, no.

Debugging this, it seems this error originates from direction to the leaf as well as to self being None. I am not sure whether there are any other cases though. Any other ideas on how we could identify duplicate nodes/elements?

--
To be clear, this is not an issue of multiple insertion of e.g., label value (as in 2 x publish label value) but inserting the same node multiple times:

    #[tokio::test]
    async fn test_insert_duplicate() -> Result<(), AkdError> {
        let mut rng = OsRng;
        let db = AsyncInMemoryDatabase::new();
        let mut azks = Azks::new::<_, Blake3>(&db).await?;

        // Create a random node.
        let node = NodeLabel::random(&mut rng);
        let mut input = [0u8; 32];
        rng.fill_bytes(&mut input);
        let input = Blake3Digest::new(input);

        // Duplicate inserts of the same node.
        azks.insert_leaf::<_, Blake3>(&db, node, input).await?;
        println!("Insert one leaf done.");
        azks.insert_leaf::<_, Blake3>(&db, node, input).await?;

        Ok(())
    }

[slawlor]

I mean since the public contract of the API it to only interact with the Directory struct, maybe it's worthwhile to just scan for duplicate leaves in the publish(...) call, and throw some error there. I understand we'd like a less cryptic error on the Azks insertion operation, but if we've already trimmed out duplicate leaves I don't see how we'd get duplicate nodes.

Is there anywhere in the lib where this error case is specifically handled? Like in a match or an if let for example? That would be worthwhile to identify, at least with a comment, but it seems like it might be difficult to assert that a direction is none error is a duplicate node error...

[eozturk1]

I am not aware of any such locations where we specifically say the error is due to duplicate nodes. It is just that NoDirectionError is thrown at a certain location on the code if we insert the same node multiple times -- but I am not sure whether that code is reached there are duplicate nodes.

One thing we could do is to not insert duplicate nodes in the tests?

@kevinlewi, should we investigate this further? Any pointers on what to look for to identify duplicate nodes?

[kevinlewi]

Sorry for the delayed reply on this @eozturk1 .

So I guess what I mean is, let's suppose a consumer of this library accidentally attempts to insert duplicate nodes (not just in the tests). Right now, a "NoDirectionError" is thrown, which is ambiguous. Can we make it so that a more instructive error is thrown, that explicitly indicates that a duplicate node was attempted to be inserted?

This might just be a matter of converting the "NoDirectionError" to a new error type like "DuplicateNodeInsertionError". But being careful to only convert this error in the actual event of a duplicate node (since we don't want the DuplicateNodeInsertionError to be thrown when something else unrelated goes wrong).

Hope that makes sense!

[kevinlewi]

No longer relevant since we handle directory publishing with versioning