Add Working with a document

[arn-the-long-beard]

Hey ! Let's implement from the Arangodb documentation -> Working with a Document

• read a single document

• read document header

• create document

• replace document

• update document

• removes a document

From our work on documents we saw the need for updating some of the existing code regarding request and response. Therefore the todolist for this PR gets also updated

• update how we do request to the server to have more flexibility for adding headers and other options on a request

• update how we handle response from the server

Same as previously, each item is implemented with code and unit test :)

I see that I got some previous commits. No worries. I ll hard push on my branch later when stuff will be ready.

Let's go !

[arn-the-long-beard]

I remade my changes. My build for document_url was missing collection name in it

[fMeow]

Actually I don't quite understand why arangoDB offers API on document. I think almost all the features in docuemnt API can be achieved by AQL query.

But it is good to fulfill these API.

[arn-the-long-beard]

I agree with you. I also use AQL myself directly because it gives more freedom/flexibility to do stuff. But when there is no need for it, I used the document instead of AQL to earn a little time and less code. It depends I think about the use we do of it.

By the way, do you thimk it is fine with document_base_url ?

[fMeow]

Sure. This is an internal API, and names for them are not that important as long as it is clear enough.

[arn-the-long-beard]

Ok :)

Since we did update develop, should I merge my branch from it or should I continue my work on it ?

[fMeow]

I perfer rebase current branch from develop for two reasons.

1. CI build on PR is enabled in the lastest commit in develop branch.
2. if there is confilct, it's advised to resolve it early. Though I think the chance of confilct is quite small.

[arn-the-long-beard]

Well, I made the Rebase and used the awesome cherry-pick . Then i made push force, since it is my own branch, it is ok right ?

[arn-the-long-beard]

I saw that your doc comments are nice and justified. Did you do it manually or did you use some tool for this ?

I am still failing with mmfiles it seems-

[fMeow]

Nightly rustfmt can format doc comment. I use stable by default, but sometimes switch to nightly only to utilize the doc format features in nightly rustfmt.

I think this line is to be blamed for. It's wierd, sometimes the server return status 2 and pass the test, while this time it is 4. You might find the test passed if you push to github next time with the exact same code. In fact, that is what happened now. The CI test on PR fail the test but in your own repo it does pass the test, while the two test run the exact same code. I don't know what is under the hood, neither do I care. I prefer to simply skip this line of test under mmfiles feature, by adding a #[cfg(not(feature="mmfiles")].

Updated: Status 2 means unloaded and 4 denotes unloading, see arango DB doc. I think both 2 and 4 are valid status, and I fixed it in this commit, together with a replacement of the collection.status type from u16 to CollectionStatus.

[arn-the-long-beard]

Okay, Maybe there is some behavior of ArangoDB that we do not know of.

Thank you for your explanations !!!!

[arn-the-long-beard]

Hello ! the last commit is very much a draft. I have an issue that I do not succeed to solve.

When running the test I got this

 
---- test_post_create_document stdout ----
Err(Serde(Error("missing field `error`", line: 0, column: 0)))
thread 'test_post_create_document' panicked at 'assertion failed: `(left == right)`: succeed create a document

Diff < left / right > :
<false
>true

', tests/collection.rs:1:1

It seems there is an issue with deserialization/mapping of the error coming from Arangodb. Strangely, I got the document created on my DB:
I also did not find the field "error" in the ClientError & ArangoErrors struct, so I am not sure what is happening.

[arn-the-long-beard]

Well, I am happy about these changes.

Thank you a lot for your Inputs, you make me learning so much. I really appreciate it 👍

Anyway, do you think the DocumentResponse is ok now ?

Also I choose HeaderName and HeaderValue as values returned because they go well with the .header(name,value) method on request

 fn make_header_from_options(
    document_read_options: DocumentReadOptions,
) -> Option<(http::header::HeaderName, http::header::HeaderValue)>

   pub fn header<K, V>(self, key: K, value: V) -> Builder
    where
        HeaderName: TryFrom<K>,
        <HeaderName as TryFrom<K>>::Error: Into<crate::Error>,
        HeaderValue: TryFrom<V>,
        <HeaderValue as TryFrom<V>>::Error: Into<crate::Error>,
    {
        self.and_then(move |mut head| {
            let name = <HeaderName as TryFrom<K>>::try_from(key).map_err(Into::into)?;
            let value = <HeaderValue as TryFrom<V>>::try_from(value).map_err(Into::into)?;
            head.headers.append(name, value);
            Ok(head)
        })
    }

[fMeow]

I think OperationResponse is useless now. What do you think?

[arn-the-long-beard]

Thank you for your feedback :)

I wish OperationResponse was useless. I am using only it there :

/// Gives extra method on the DocumentResponse to quickly check what the server
/// returns
impl<T> DocumentResponse<T> {
    /// Should be true when the server send back an empty object {}
    pub fn is_silent(&self) -> bool {
        match self {
            DocumentResponse::Silent => true,
            _ => false,
        }
    }
    /// Should be true if there is a response from the server
    pub fn has_response(&self) -> bool {
        match self {
            DocumentResponse::Response { .. } => true,
            _ => false,
        }
    }

    /// Should give None or Some(Response)
    pub fn get_response(self) -> Option<OperationResponse<T>> {
        if let DocumentResponse::Response {
            header,
            old,
            new,
            _old_rev,
        } = self
        {
            Some(OperationResponse {
                header,
                old,
                new,
                _old_rev,
            })
        } else {
            None
        }
    }
}

Is there a way I can return these values without a struct ?

[fMeow]

I see, the only usage of OperationResponse is in fn get_response(&self).

What about removing get_response and further dividing getter into fn header(&self)->Option<&Header>, fn old(&self)->Option<&T> and etc? Since header, old, new, old_rev can be of arbitrary combination, I think this is a reasonable design.

PS: Rustacean tend to use field name for getter and set_xxx for setter. For example, header for a getter and set_header for a setter.

[arn-the-long-beard]

Okay, I try to deserialize into Value and then use as_object_mut()
I got some error because I need to useunwrap() . So I needed to handle the Error. And then I had to add en Err variant in DocumentResponse, otherwise it could not compile.

[arn-the-long-beard]

I see, the only usage of OperationResponse is in fn get_response(&self).

What about removing get_response and further dividing getter into fn header(&self)->Option<&Header>, fn old(&self)->Option<&T> and etc? Since header, old, new, old_rev can be of arbitrary combination, I think this is a reasonable design.

PS: Rustacean tend to use field name for getter and set_xxx for setter. For example, header for a getter and set_header for a setter.

Haa nice catch !

I am gonna try it :)
Thank you for the tip !

PS : my last commit is not this clean, I got some style/format stuff in it. Sorry about that.

[arn-the-long-beard]

I learned how to use the &, the code looks much better now, Thank you for pushing me 👍

[arn-the-long-beard]

Yes ! You are right. I do not know why, I thought I had to keep it because I was having error, but in fact it was because of something else. I just removed and the tests still pass correctly.

[fMeow]

unwrap might be fine, but I think a better way to handle this error is to map error to serde json's error type.

Here is a minimal example for proof of concept:

let json = obj.as_object_mut().map_err(|_| de::Error::invalid_type(Unexpected::Other("not object"), &self))?;

json.remove("_id").map_err(|_| de::Error::missing_filed("_id"))?;

[arn-the-long-beard]

Yeah, right about the variant. I just removed it.

For the error handling, there is no .map_err() on obj.as_object_mut() . I tried to find a way before by using other methods like like unwrap_or_else or map , but I did not find a proper solution.

[fMeow]

For the error handling, there is no .map_err() on obj.as_object_mut() . I tried to find a way before by using other methods like like unwrap_or_else or map , but I did not find a proper solution.

My mistake. obj.as_object_mut() returns a Option and I missed a step to convert it to Result with Option::ok_or.

let json = obj.as_object_mut().ok_or(de::Error::invalid_type(Unexpected::Other("not object"), &self))?;

[arn-the-long-beard]

Well, we do not have self in this context but am trying to understand. It seems we need to use the Visitor trait or something like here https://serde.rs/deserialize-map.html

Do you think we should go this far for this case ?

[fMeow]

Do you think we should go this far for this case ?

I think no. My point is to raise a serde::de::Error, and there is some way to create a de::Error without the presense of Visitor. You can choose whatever reasonable. For example, de::Error::custom() and etc.

[arn-the-long-beard]

Well, I am sorry but I do not succeed to raise Error with any kind of good code.

For example this break

     let json_result = obj.as_object_mut().ok_or(de::Error::custom);
        if json_result.is_err() {
            json_result   <--- break there 
        };

        let json = json_result.unwrap();  <--- break there 

I do not understand how .unwrap() does not exist on this Result . Also I am not able to return the error .

I made this code compile but I do not succeed to test it

     let mut obj = serde_json::Value::deserialize(deserializer)?;
        let json_result = obj.as_object_mut();

        if json_result.is_none() {
            eprintln!(" DESERIALIZATION FAILED");

            return Err(de::Error::custom(
                "DocumentResponse deserialization has failed",
            ));
        }

I think I am missing the point because I am a bit lost on how to manage error properly there.

[arn-the-long-beard]

Thank you for the merge by the way :)

[fMeow]

Thank you for your great great work! I can't believe we have come so far. You can refer to the latest commit in develop where I have made the change myself.

I copy it here:

    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        let mut obj = serde_json::Value::deserialize(deserializer)?;

        let json = obj
            .as_object_mut()
            .ok_or(DeError::custom("should be a json object"))?;

        if json.is_empty() {
            Ok(DocumentResponse::Silent)
        } else {
            let _id = json.remove("_id").ok_or(DeError::missing_field("_id"))?;
            let _key = json.remove("_key").ok_or(DeError::missing_field("_key"))?;
            let _rev = json.remove("_rev").ok_or(DeError::missing_field("_rev"))?;
            let header: DocumentHeader = DocumentHeader {
                _id: serde_json::from_value(_id).map_err(DeError::custom)?,
                _key: serde_json::from_value(_key).map_err(DeError::custom)?,
                _rev: serde_json::from_value(_rev).map_err(DeError::custom)?,
            };

            let old = json
                .remove("old")
                .map(T::deserialize)
                .transpose()
                .map_err(DeError::custom)?;
            let new = json
                .remove("new")
                .map(T::deserialize)
                .transpose()
                .map_err(DeError::custom)?;
            let _old_rev = json.remove("_old_rev").map(|v| v.to_string());

            Ok(DocumentResponse::Response {
                header,
                old,
                new,
                _old_rev,
            })
        }
    }

[arn-the-long-beard]

Thank you !!!

The code is beautiful 👍

        let json = obj
            .as_object_mut()
            .ok_or(DeError::custom("should be a json object"))?;

Arf, why I did not think about that lol. So simple :P

Thank you a lot for your guidance.

[fMeow]

You are welcome~ :)

Thank you so much!

Would you like to add some examples on document operation?

[arn-the-long-beard]

:)

Yeah sure !