Remove header from document

[inzanez]

Hi

does it really make sense to have the Header included in the Document<T> structure? Wouldn't it make more sense to let the user of the driver decide whether or not that part of the response is going to be deserialized? Simply by including the header properties (merged or not) in the struct T?

I currently use AQL queries all over the place, and this works quite well as it just serializes/deserializes T. So I get my headers automatically, and I can send them along to the client.

If I were to use the document level driver functions instead, I would have to copy the deserialized header parts over to my T if I needed them. And as far as I can tell, the Arango driver never actually uses the Header structure. So it should be safe to remove the Header fields and just deserialize to T.

[ManevilleF]

You can put the _id , _key and _rev in your T structure and it would work, but it means that you can forget the automatic creation of these values and they become unreliable. The state of your application can be unsafe.

for aragog I use the header to allow the user to wrap its document in a DatabaseRecord<T> base element of the ODM/OGM and this way the user doesn't have to declare and use the vales in a weird way.

[inzanez]

@EpiFouloux I think it's a difference talking from the perspective of aragog as an ODM and the driver itself. Removing the _id, _key and _rev from the default Document in arangors would only mean that users of the driver would need to specify them. So for instance, aragog would specify them (as it already does; currently it just seems to move them over to the own structure by implementation of From). So that wouldn't really make a difference to aragog as far as I'm concerned.

As I personally don't use any ODM, I'm handling with the driver directly. Which means that my structures require the definition of _id, _key and _rev as I use different AQL queries and need to deserialize these values. And now there's an inconsistency between using the AQL query directly (which doesn't deserialize any of the three attributes into a driver provided structure) and the direct document access on collections (which does deserialize the three attributes into the Header defined by the driver).
So I could also declare another From implementation for all my structures, which would work; or one could argue whether or not the driver itself should show this different behavior, as I think from a driver perspective, the values of _id, _key and _rev are not really relevant. Which means that they could be omitted and the left to the user.

Does that make sense?

[ManevilleF]

Yeah, I guess it does and it doesn't change anything for me.
But since these values are mandatory that means that you need to set them as Option<String> in your structure and set them as None when you create a document for the first time ? Or do you need two structs? I think arangors would become weird to use

[inzanez]

@EpiFouloux I would leave that as an exercise to the user of arangors. I think it is not weirder in any way than what you would do using Diesel for instance. I found many examples that actually do use two separate structures for new instances of a type (omitting the ID) and existing instances. Using an Option would be an option too ;-) And not serializing on None.

If you were to use AQL queries (instead of direct document access / insert), you were required to handle this case anyway, as arangors doesn't deserialize to Document<T> but to T.

Just assuming that I have:

struct User {
  first_name: String,
  last_name: String,
}
...
...
let u = db.aql_str("FOR u in users RETURN u");
...

that would not deserialize _id, _key or _rev. So I would need to specify them myself, or be aware of the difference between direct access (collection.document(...) which does deserialize the fields into Header) and aql_str (which doesn't deserialize them) and maybe wrap my own T (User in the example above) in the Document struct provided by arangors. Which is an option too, but that will lead to many type conversions in my application code, as I do not want to work on the Document<T> but on T usually.

So what it comes down to:

• AQL queries as well as aql_str do not wrap the requested type in Document, so the header values are not deserialized automatically
• Direct document access does wrap the requested type in Document

That's inconsistent in my opinion. I've been working with AQL queries mostly, and it does the job, feels nice, no conversion required (except the handling of _id, _key and _rev). So my assumption was that direct document access should work the same. But it doesn't. And that feels kind of weird really.

[ManevilleF]

I did not notice that there was this inconstency while I did implement aql_str for my custom query system.
I think you are right, arangors is a ArangoDB driver, not a ODM and maybe 'Document' should not exist.
What does @fMeow think?

[inzanez]

@EpiFouloux Just as a quick update. Assuming I created a struct like this:

#[derive(Debug, Default, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
struct User {
    #[serde(rename="_id")]
    id: String,
    #[serde(rename="_key")]
    key: String,
    #[serde(rename="_rev")]
    rev: String,
    first_name: String,
    last_name: String,
    email_address: String,
}

I could use that structure (and I currently do use it in a similar form) with aql_str and AqlQuery in general. But using it with collection.document(_key) results in a deserialization error, as arangors is doing the header part, which leaves my own header fields empty. And making them optional is not an option, as it's a pain to work with and I know that existing instances will have these fields (they actually must). So no need for option there.

I think I can perfectly well live without the restructuring of the direct document access and removal of header. But as I realized the above 'issue', I thought I'd bring it up here, as it might result in a cleaner concept in the driver itself.

[ManevilleF]

When arangors builds the Document::header it takes the values and doesn't leave them in the serde_json::Value ? Are you sure about that?

Edit: Also why not use a wrapper for that? why don't you use a ODM?

[inzanez]

@EpiFouloux I will paste a sample, just to make sure we talk about the same thing:

#[macro_use]
extern crate serde_derive;

use arangors::Document;

#[derive(Debug, Default, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
struct User {
    #[serde(rename="_id")]
    id: String,
    #[serde(rename="_key")]
    key: String,
    #[serde(rename="_rev")]
    rev: String,
    first_name: String,
    last_name: String,
    email_address: String,
}

fn main() {
    let conn: arangors::Connection = arangors::Connection::establish_basic_auth("http://localhost:8529", "root", "password").unwrap();

    let db = conn.db("Demo").unwrap();
    let col = db.collection("users").unwrap();
    // I know that user with _key 100000 exists!
    let x: Document<User> = col.document("100000").unwrap();

    println!("{:?}", x);
}

This will lead to:
thread 'main' panicked at 'called Result::unwrap()on anErrvalue: Serde(Error("missing field_id", line: 0, column: 0))', src/main.rs:30:52

So no. They are deserialized into the header.

If I change the struct User to the following:

#[derive(Debug, Default, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
struct User {
    first_name: String,
    last_name: String,
    email_address: String,
}

everything works fine.

[ManevilleF]

Yeah that's what I was talking about and indeed it's a bit weird, the Document struct should be removed

[ManevilleF]

Hey @inzanez I can make a pull request for this if you are not already doing one

[arn-the-long-beard]

Hey guys !

Thank you for your messages 😃 . I think it is me who wrote the Header and the Document. I saw your message yesterday but I was so busy.

I wrote the API by using the official documentation and source code in the Python & Typescript version of the driver.

Can you explain me shortly what is wrong with what we have ?

[inzanez]

@arn-the-long-beard Well, I don't really want to talk about 'right' and 'wrong' here, as it's not that simple. I see that also the Golang driver does the same thing we have here. Still, I ask whether or not that's good choice.

Basically we have the following situation:

1. Using an AqlQuery will not deserialize the document header fields separately, which allows one to specify the header fields in one's own struct and retrieving them that way. This is inconvenient on one hand as one has to define the header parts, but convenient as one receives a result of T which is not wrapped in Document.
2. Using an AqlQuery deserialzing to Document<T> will allow one not to specify the headers, as they are specified by the document. But one doesn't get T as a return value, instead one get's Document<T>. And one cannot specify the header values as part of one's own struct, because this will lead to deserialization errors.
3. If one defines the header structure to be part of one's own struct as written 1., the direct access functions on a collection do not work, as the header information is parsed into the wrapping Document structure.

So either one is supposed to always deserialize to Document<T> when using the AQL query functions, or the API delivers inconsistent behavior.

@EpiFouloux No, I haven't started on anything yet.

[inzanez]

Quick addition: Another option would be to implement Deref and DerefMut for Document. This would be convenient as it would allow for things like this:

use std::ops::Deref;

struct Document<T> {
    data: T
}

impl<T> Deref for Document<T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        &self.data
    }
}

struct User {
    name: String,
}

impl User {
    fn greet(&self) -> String {
        format!("Hello, I'm {}", self.name)
    }
}


fn main() {
    let x = Document { data: User { name: "Peter Pan".to_string() } };
    println!("{}", x.greet());
}

Without the Deref, one could not work directly on T that is encapsulated in Document. And DerefMut would be required to work with mutable reference. But then again, I'm no expert on the whole Deref topic,...but that way, we would possibly be more compatible with other implementations.

And if this is the way to go, it might make sense to update the documentation to always use the Document structure provided by arangors? This ensures that users of the driver don't create their own Header fields, which will lead to problems.

[arn-the-long-beard]

Thank you for your message 👍

Okay, well my brain is not fresh on arangors since I am working on other stuff now. That is why I do not see the issue right now.

But I remember that @fMeow talked about the possibility to have something similar like this :

Wouldn't it make more sense to let the user of the driver decide whether or not that part of the response is going to be deserialized?

Simply by including the header properties (merged or not) in the struct T?

I did not know how to make this happen that is why it is missing also. Can you explain me this part ?

Anyway, I think you are very welcomed make it happen 👍

PS : you posted a new message . I do not have the knowledge for DerefMut bit I feel your concern regarding consitensy

[inzanez]

@arn-the-long-beard Well, I guess I would wait for @EpiFouloux as he's working on a proposal. But in general, it would be similar to what the current AQL query interface does. It would not deserialize the response to Document<T>, but deserialize to T directly. So if T contains the Header values, it will automatically be deserialized. If it doesn't contain them, they will just be dropped. But either way, you will receive T.

As an example:

struct User {
  name: String,
}
...
let u: User = collection.document("10000");

This would return a User, but as the struct doesn't contain any of the header fields, they will not be deserialized. So if one were to use the header fields, one would need to specify them in the struct.

Regarding Deref/DerefMut: This would allow for automatic dereferencing so that working on a Document<T> instance would feel the same as working on T directly, without dereferencing everytime. As it's done automatically. As far as I understand.