elastic icon indicating copy to clipboard operation
elastic copied to clipboard

Is there some resean the `index` and `ty` must be static in `SearchRequestInner`?

Open wfxr opened this issue 7 years ago • 6 comments
trafficstars

Is there some reasons the index and ty must be static in SearchRequestInner?

I find it is hard to give SearchRequestBuilder a static index name since it only can be computed runtime in some scenario.

pub fn index<I>(mut self, index: I) -> Self
       where I: Into<Index<'static>>

https://github.com/elastic-rs/elastic/blob/216ba8a1f3903bd23df5624fa11c14ac251788b0/src/elastic/src/client/requests/search.rs#L30-L38

wfxr avatar Aug 13 '18 11:08 wfxr

Hi @wfxr! That 'static lifetime is a bit misleading, because internally those parameters use a Cow, which means you can give it an owned String and it will satisfy that lifetime constraint.

The reason we don't currently accept borrowed strings with some shorter lifetime is that we potentially offload request processing to a different thread, so it's simpler to just require values that are guaranteed to live as long as we need, which includes both &'static str and String.

KodrAus avatar Aug 14 '18 00:08 KodrAus

@KodrAus Thanks for your very clear explanation!

I follow your instruction and use a owned String as parameter, then the compiler does not complain anymore.

But can we clone the &str inner index function? Otherwise the caller have to do it like this:

client.search::<Value>()
    // Here my_index is a String and likely to be used frequently.
    // I have to clone it again and again manually.
    .index(my_index.clone())
    .body(...)
    .send()?;

wfxr avatar Aug 14 '18 02:08 wfxr

All those static lifetimes are really hard to work with, with dynamic indexes.

Passing a String to the client means moving the index into the client and consequently, if I am going to do thousand of queries there are a lot of unnecessary clones required.

I'm also having similar issues when using a DocumentClient and trying to use a dynamic index. Since DocumentClient uses doc.index() I'm not even sure if it's possible.

I'm probably just going to use raw HTTP requests for this.

mtorromeo avatar Jun 07 '19 13:06 mtorromeo

Yeh we could probably do better here and rethink the need for 'static lifetimes so urls could be built up without requiring owned parameters.

KodrAus avatar Jun 20 '19 06:06 KodrAus

Ok, now that we have a single crate to work in we can look at tackling this. There are two routes we could take:

  • Add lifetime constraints to request builders, one for each parameter. So we'd have something like SearchRequestBuilder<'index, 'ty, TSender, TDocument, TBody>. We couldn't use just one parameter because it'll be eagerly bound to the first parameter, so setting the index for instance would impose constraints on setting the type.
  • Make the builders generic over those parameters so we'd have something like SearchRequestBuilder<TSender, TIndex: impl Into<Index<'_>>, TType: impl Into<Type<'_>>, TDocument, TBody> (I think I prefer this one).

For both of these cases it would be nice to also find a solution to #258 because these builders are becoming pretty complex.

KodrAus avatar Jul 07 '19 02:07 KodrAus

Both solutions look fine with me! Though I think I prefer the second one as well. Good work! You honestly saved my day with this crate many times by now :)

DevQps avatar Jul 18 '19 10:07 DevQps