Closed mwilliammyers closed 4 years ago
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?
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?
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
)...
@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.
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
.
In order to maintain backwards compatibility by not introducing a new generic parameter on
UpdateResponse
, I had to useserde_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:
I am hoping to tackle
bulk
updates ASAP too...