Add support for _source on update requests

[mwilliammyers]

In order to maintain backwards compatibility by not introducing a new generic parameter on UpdateResponse, I had to use serde_json::Value as an intermediate value...

If we are ok with not maintaining backwards compatibility I can update this PR and introduce a new generic parameter, which will result in something like this: UpdateRequestBuilder<TSender, TBody, TDocument>.

As a result, as of right now, you use this like:

#[derive(Deserialize, ElasticType, Debug)]
struct User {
    #[elastic(id)]
    id: String,
    name: String,
}

#[derive(Serialize, ElasticType, Debug)]
struct UpdateUser {
    #[elastic(id)]
    id: String,
    #[serde(skip_serializing_if = "Option::is_none")]
    name: Option<String>,
}

let response = client
    .document::<UpdateUser>()
    .update(1)
    .script(r#"ctx._source.name = "Updated Name""#)
    .params_fluent(|params| params.url_param("_source", true))
    .send()?;

dbg!(&response.into_document::<User>());

I am hoping to tackle bulk updates ASAP too...

[mwilliammyers]

We can also add found: bool, sequence_number: Option<i32>, and primary_term: Option<i32> to this if we want. I felt like they were a little superfluous...

[mwilliammyers]

Oh also, should I add a mechanism to specify _source=true in the actual request body, as opposed to in the URL params? I am not sure if that is possible right now?

[KodrAus]

Thanks for the PR @mwilliammyers!

Hmm, it keeping with the rest of the crate style I think we would want to make UpdateResponse generic over the returned document type, but since we use the type name to determine default index and type names I think there could be a bit of a footgun in this example:

#[derive(Deserialize, ElasticType, Debug)]
struct User {
    #[elastic(id)]
    id: String,
    name: String,
}

#[derive(Serialize, ElasticType, Debug)]
struct UpdateUser {
    #[elastic(id)]
    id: String,
    #[serde(skip_serializing_if = "Option::is_none")]
    name: Option<String>,
}

let response = client
    .document::<UpdateUser>()
    .update(1)
    .script(r#"ctx._source.name = "Updated Name""#)
    .params_fluent(|params| params.url_param("_source", true))
    .send()?;

dbg!(&response.into_document::<User>());

where UpdateUser points to a different index than User.

What if we did something like:

let response = client
    .document::<User>()
    .update(1)
    .script(r#"ctx._source.name = "Updated Name""#)
    .source::<UpdateUser>()
    .send()?;

So that we effectively have UpdateRequestBuilder<TSender, TBody, TSource>, which defaults to Value, but is set to T in the source method?

What do you think?

[mwilliammyers]

That makes sense. I did something similar for _bulk requests in #382...

I don't think it really resolves the footgun problem because I expose UpdateUser via GraphQL to let the API user decide which fields to update (because they are all Option) but it is definitely more ergonomic. So it has to look like:

let response = client
    .document::<UpdateUser>()
    .update(1)
    .script(r#"ctx._source.name = "Updated Name""#)
    .source::<User>()
    .send()?;

I didn't notice this issue because I have to manually resolve the index for each document via the .index() method on the builders because it looks something like: users-COMPANY_ID where COMPANY_ID is a UUID that I get from an authorization header attached to each request coming into the GraphQL server.

This is definitely a separate issue, and I will open one when I have more concrete ideas, but I wonder if we could/should expand the "index name from a method" mechanism to be able to pass in parameters (e.g. the COMPANY_ID)...

[mwilliammyers]

@KodrAus I implemented it like you suggested so I am going to merge this if that is ok with you.

I am going to wait to merge this until we decide what to do with #382. My vote is we do into_document<T>() on both. Either way, I am going to keep the source() method here so you don't have to do .params_fluent(|params| params.url_param("_source", true)), but I will probably make it so you can pass in more options like partial documents etc.

[mwilliammyers]

Until I can figure out a better way to do #382, I propose we merge this.

I will have to revisit this anyway to add support for passing in a Value to source.