Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wfxr opened this issue Aug 13, 2018 · 6 comments
Open

Comments

@wfxr
Copy link

wfxr commented Aug 13, 2018

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>>

pub type SearchRequestBuilder<TSender, TDocument, TBody> = RequestBuilder<TSender, SearchRequestInner<TDocument, TBody>>;
#[doc(hidden)]
pub struct SearchRequestInner<TDocument, TBody> {
index: Option<Index<'static>>,
ty: Option<Type<'static>>,
body: TBody,
_marker: PhantomData<TDocument>,
}

@KodrAus
Copy link
Member

KodrAus commented Aug 14, 2018

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.

@wfxr
Copy link
Author

wfxr commented Aug 14, 2018

@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()?;

@mtorromeo
Copy link
Contributor

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.

@KodrAus
Copy link
Member

KodrAus commented Jun 20, 2019

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
Copy link
Member

KodrAus commented Jul 7, 2019

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.

@DevQps
Copy link

DevQps commented Jul 18, 2019

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants