Convert get database operation to pipeline architecture #286
Conversation
There was a problem hiding this comment.
Nothing to hold up the PR - the code changes look good - but I do see a number of design changes we should make before moving more to the new pipeline. For example, the pipeline should define common methods for sending requests and handling responses, including extensible status code validation, request and response sanitization (later - at least for tests), etc. We consolidate all that in other SDKs for ease, consistency, and productivity.
| @@ -20,7 +21,9 @@ async fn main() -> Result<(), Box<dyn Error + Send + Sync>> { | |||
|
|
|||
| let database_client = client.into_database_client(database_name.clone()); | |||
|
|
|||
| let response = database_client.get_database().execute().await?; | |||
| let response = database_client | |||
| .get_database(Context::new(), GetDatabaseOptions::new()) | |||
There was a problem hiding this comment.
For client and method options, do we want to standardize on new or default? See my comment above for context. In other SDKs, this argument is typically optional e.g. default parameter value in .NET, overload in Java, kwargs in Python, and options bag in JS, hence I was thinking default.
There was a problem hiding this comment.
Some options won't be able to have default implementations because they have required arguments. If we opt for Default, we won't be able to do that for all option types.
Unfortunately since Rust doesn't have default params, this is fairly verbose and Default impls don't help necessarily. We could have one method get_database that only takes a context and default the options, and a get_database_with_options which takes an options param.
There was a problem hiding this comment.
Generally, if a parameter is required (hence making it required to create options) it's its own parameter. Eventually we'll want to be consistent with the other SDKs when not idiomatic e.g. which parameters are parameters and which are in some sort of options bag.
| @@ -196,6 +183,10 @@ impl CosmosClient { | |||
| DatabaseClient::new(self, database_name) | |||
| } | |||
|
|
|||
| /// Prepares an `http::RequestBuilder`. | |||
| /// | |||
| /// Note: This is used by legacy operations that have not moved to the use of the | |||
There was a problem hiding this comment.
Nit: replace "Note:" with "TODO:" so we have a consistent approach to finding them all. We should do so by GA, so having GA'd docs with "TODO" shouldn't be likely.
| let mut request = self | ||
| .prepare_request_with_database_name(http::Method::GET) | ||
| .body(bytes::Bytes::new()) | ||
| .unwrap() | ||
| .into(); | ||
| options.decorate_request(&mut request)?; | ||
| let response = self | ||
| .pipeline() | ||
| .send(&mut ctx, &mut request) | ||
| .await | ||
| .map_err(crate::Error::PolicyError)? | ||
| .validate(http::StatusCode::OK) | ||
| .await?; |
There was a problem hiding this comment.
This is something the pipeline should actually do, and the types of errors, which can be retried, etc. is typically handled by the pipeline. Again, we can do this in a separate PR, but I bring it up as a design point. Having every single method repeat this is burdensome and exactly what the pipeline was meant to handle. For an example reference, see .NETs: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/src/Pipeline/HttpPipeline.cs. Our other language SDKs use something similar.
There was a problem hiding this comment.
Are you talking about the validation? This something that depends on the specific operation being performed, so I don't think we can move this into the pipeline. I agree it's currently fairly verbose, but I'm not sure the right way to solve that is moving it into pipelines rather than moving some of this into a function somewhere.
There was a problem hiding this comment.
Some of our SDKs use a response classifier they can pass in when building the pipeline. We don't necessarily have to be the same, but - even if validate is kept separate here - serializing and sending the request, and deserializing and handling the response should be done in the pipeline as much as possible. We do have some clients (Key Vault does it in a couple places) that will occasionally "override" that behavior, but it's pretty rare we need to.
No description provided.