elastic / elasticsearch-rs

Official Elasticsearch Rust Client
https://www.elastic.co/guide/en/elasticsearch/client/rust-api/current/index.html
Apache License 2.0
705 stars 72 forks source link

[DISCUSS] Fluent builder with Sender trait or fluent (closure) function #1

Closed russcam closed 4 years ago

russcam commented 4 years ago

The current implementation implements APIs as structs that follow the consuming builder pattern, mutating self when functions are called to set values. For example,

#[derive(Default)]
pub struct CatAliases {
    client: Elasticsearch,
    error_trace: Option<bool>,
    // ... etc.
}
impl CatAliases {
    pub fn new(client: Elasticsearch) -> Self {
        CatAliases {
            client,
            ..Default::default()
        }
    }
    #[doc = "Include the stack trace of returned errors."]
    pub fn error_trace(mut self, error_trace: Option<bool>) -> Self {
        self.error_trace = error_trace;
        self
    }

    // ...etc. 
}

Each builder struct also implements the Sender trait, a terminal function for sending the actual request, which consumes (transfers ownership of) the builder

impl Sender for CatAliases {
    fn send<T>(self) -> Result<ElasticsearchResponse<T>>
    where
        T: DeserializeOwned,
    {
        // send actual request
    }
}

This allows for the following usage pattern

let cat_response = client.cat()
                         .aliases() // <-- returns new instance of CatAliases 
                         .error_trace(Some(true))
                         .send()?;

An alternative implementation considered would be for API functions to accept a function parameter where the argument is the builder struct. For example

let cat_response = client.cat()
                         .aliases(|p| { p.error_trace(Some(true)) });

This would remove the need for each builder to implement the Sender trait, and the explicit send() function to execute the request. It would however make it trickier to compose default values for a request and use it as the basis for multiple requests. For example,

let cat_aliases = client.cat()
                         .aliases() // <-- returns new instance of CatAliases 
                         .error_trace(Some(true));

let mut cat_aliases_clone = cat_aliases.clone();
// add v parameter to the clone
cat_aliases_clone = cat_aliases_clone.v(Some(true));

let cat_response = cat_aliases.send()?;
let cat_clone_response = cat_aliases_clone.send()?;

Interested to hear technical arguments for each approach.

russcam commented 4 years ago

The Sender trait is gone in #21, now that send() is async, because async traits are currently not supported.

One potential improvement that may be made to the API is for builder structs to implement the Future trait, such that .await can be called on the builder and removing the send() fn. Something like

let search_response = client
    // Value is the request body type
    .search(SearchUrlParts::None)
    .body(Some(json!({
        "query": {
            "match_all": {}
        }
    })))
    .allow_no_indices(Some(true))
    .await?; // no explicit .send() before hand

This is similar to Async Builders and Async Finalizers. One wrinkle to iron out would be determining where the request data is constructed; this may be OK to move to the poll() function in the Future impl.

russcam commented 4 years ago

I’m going to close this issue for now; we’ve settled on the fluent builder approach.