Skip to content

Operation Builder Future #510

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Dec 10, 2021
Merged

Operation Builder Future #510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a9176fc
Convert create_database to a builder pattern
rylev Nov 11, 2021
22f61a2
Inline the future
rylev Nov 11, 2021
8a0b746
Fix changes with pipeline
rylev Dec 9, 2021
0c37373
Fix clippy error
rylev Dec 9, 2021
8cb2fd0
Fix test
rylev Dec 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sdk/core/src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/// Creates setter methods
///
/// The methods created are of the form `with_$name` that takes an argument of type `$typ`
/// The methods created are of the form `$name` that takes an argument of type `$typ`
/// and sets the field $name to result of calling `$transform` with the value of the argument.
///
/// In other words. The following macro call:
Expand Down
5 changes: 0 additions & 5 deletions sdk/core/src/options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,14 @@ use std::time::Duration;
/// ```
#[derive(Clone, Debug, Default)]
pub struct ClientOptions {
// TODO: Expose transport override.
/// Policies called per call.
pub(crate) per_call_policies: Vec<Arc<dyn Policy>>,

/// Policies called per retry.
pub(crate) per_retry_policies: Vec<Arc<dyn Policy>>,

/// Retry options.
pub(crate) retry: RetryOptions,

/// Telemetry options.
pub(crate) telemetry: TelemetryOptions,

/// Transport options.
pub(crate) transport: TransportOptions,
}
Expand Down
7 changes: 2 additions & 5 deletions sdk/cosmos/examples/cancellation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use azure_core::prelude::*;
use azure_cosmos::prelude::*;
use stop_token::prelude::*;
use stop_token::StopSource;
Expand All @@ -19,8 +18,7 @@ async fn main() -> azure_cosmos::Result<()> {
let client = CosmosClient::new(account.clone(), authorization_token.clone(), options);

// Create a new database, and time out if it takes more than 1 second.
let options = CreateDatabaseOptions::new();
let future = client.create_database(Context::new(), "my_database", options);
let future = client.create_database("my_database").into_future();
let deadline = Instant::now() + Duration::from_secs(1);
match future.until(deadline).await {
Ok(Ok(r)) => println!("successful response: {:?}", r),
Expand All @@ -36,8 +34,7 @@ async fn main() -> azure_cosmos::Result<()> {
// Clone the stop token for each request.
let stop = source.token();
tokio::spawn(async move {
let options = CreateDatabaseOptions::new();
let future = client.create_database(Context::new(), "my_database", options);
let future = client.create_database("my_database").into_future();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better. To confirm, all required parameters will be in the function signature,. All optional parameters are setter methods. Correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] all required parameters will be in the function signature,. All optional parameters are setter methods. Correct?

That's indeed the idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is close to what we do across other languages, but idiomatically there are some differences. For example, in .NET, once we hit ~6 (it's not a hard limit) required parameters, we do a {MethodName}Options class. In fact, that naming convention is consistent across languages when used. For Python, they use a combination of named params and kwargs. Java and JavaScript are similar in nature to .NET, though JS uses options bags (typed in TS, but of course doesn't really matter in pure JS).

We should follow suit here. So any optional parameters should maybe be in a {MethodName}Options class for methods. For clients, it's typically {ClientName}Options, but Java uses a builder pattern that basically follows what @cataggar mentioned above.

match future.until(stop).await {
Ok(Ok(r)) => println!("successful response: {:?}", r),
Ok(Err(e)) => println!("request was made but failed: {:?}", e),
Expand Down
17 changes: 2 additions & 15 deletions sdk/cosmos/src/clients/cosmos_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,21 +169,8 @@ impl CosmosClient {
}

/// Create a database
pub async fn create_database<S: AsRef<str>>(
&self,
ctx: Context,
database_name: S,
options: CreateDatabaseOptions,
) -> crate::Result<CreateDatabaseResponse> {
let mut request = self.prepare_request_pipeline("dbs", http::Method::POST);

options.decorate_request(&mut request, database_name.as_ref())?;
let response = self
.pipeline()
.send(ctx.clone().insert(ResourceType::Databases), &mut request)
.await?;

Ok(CreateDatabaseResponse::try_from(response).await?)
pub fn create_database<S: AsRef<str>>(&self, database_name: S) -> CreateDatabaseBuilder {
CreateDatabaseBuilder::new(self.clone(), database_name.as_ref().to_owned())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to figure out the best way to pass parameters in #520 #527 and as_ref().to_owned() may be an anti-pattern. Should impl Into<String> be used instead. Could the Builders avoid some cloning with Cow?

Copy link
Contributor

@yoshuawuyts yoshuawuyts Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should impl Into<String> be used instead.

I think your suggestion to use impl Into<String> is a good one; it evades an unnecessary allocation when an owned string is passed directly. This should make for an easy improvement!

Could the Builders avoid some cloning with Cow?

Potentially we could; for example using impl Into<Cow<'static, str>> as the API bound could potentially save us from having to perform an allocation when passing string slices as well. But I think if we go down this path we should have guidelines in place. As we know from experience it's really easy to accidentally push lifetimes into customer-facing APIs, which ends up having real consequences for user experience. And Cow can make it easy to fall into that trap.

In general I'd prefer it if we took a gradual approach to this. The change to using Into<String> over AsRef<str> should be an easy one we can do straight away. And once we're comfortable with the general shape of the APIs, we can look at optimizing things further by seeing if we can make the switch over to the slightly more complicated Into<Cow<'static, str>>. How does that sound?

}

/// List all databases
Expand Down
62 changes: 43 additions & 19 deletions sdk/cosmos/src/operations/create_database.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,44 +3,68 @@ use crate::prelude::*;
use crate::resources::Database;
use crate::ResourceQuota;
use azure_core::headers::{etag_from_headers, session_token_from_headers};
use azure_core::{collect_pinned_stream, Request as HttpRequest, Response as HttpResponse};
use azure_core::{collect_pinned_stream, Context, Response as HttpResponse};
use chrono::{DateTime, Utc};

#[derive(Debug, Clone)]
pub struct CreateDatabaseOptions {
pub struct CreateDatabaseBuilder {
client: CosmosClient,
database_name: String,
consistency_level: Option<ConsistencyLevel>,
context: Context,
}

impl CreateDatabaseOptions {
pub fn new() -> Self {
impl CreateDatabaseBuilder {
pub(crate) fn new(client: CosmosClient, database_name: String) -> Self {
Self {
client,
database_name,
consistency_level: None,
context: Context::new(),
}
}

setters! {
consistency_level: ConsistencyLevel => Some(consistency_level),
context: Context => context,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note on Context: I like it the way it's implemented here 👍. Having a mandatory, possibile empty struct in every function call is ugly: this way the SDK user will only pass the Context if it's meaningful in some way.

}
}

impl CreateDatabaseOptions {
pub(crate) fn decorate_request(
&self,
request: &mut HttpRequest,
database_name: &str,
) -> crate::Result<()> {
#[derive(Serialize)]
struct CreateDatabaseRequest<'a> {
pub id: &'a str,
}
let req = CreateDatabaseRequest { id: database_name };
pub fn insert<E: Send + Sync + 'static>(&mut self, entity: E) -> &mut Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should come up with a more explicit function name. insert does not make clear that we are changing the Context. Maybe something like override_context (supposing to merge #508 as well)?

Side note: What about having a trait for this function? It won't change much but it would signal unequivocally the meaning of this.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about having a trait for this function? It won't change much but it would signal unequivocally the meaning of this.

We discussed this, and adding a trait means that users of the method would need to import it into scope before being able to use it. That's not necessarily a bad idea, but before we decide to do that we should probably understand better how often the context interface is used.

We settled on keeping it as an inherent function for this proposal, but leaving the option open to re-evaluate once we have a better grip on usage.

self.context.insert(entity);
self
}

pub fn into_future(mut self) -> CreateDatabase {
Box::pin(async move {
let mut request = self
.client
.prepare_request_pipeline("dbs", http::Method::POST);

let body = CreateDatabaseBody {
id: self.database_name.as_str(),
};

azure_core::headers::add_optional_header2(&self.consistency_level, request)?;
request.set_body(bytes::Bytes::from(serde_json::to_string(&req)?).into());
Ok(())
azure_core::headers::add_optional_header2(&self.consistency_level, &mut request)?;
request.set_body(bytes::Bytes::from(serde_json::to_string(&body)?).into());
Copy link
Member

@cataggar cataggar Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serde_json::to_string is always a red flag. Recommend using to_json. Currently in core::http_client.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was existing code, so we can put this in a new issue. No need to hold up this awesomeness.

let response = self
.client
.pipeline()
.send(self.context.insert(ResourceType::Databases), &mut request)
.await?;

CreateDatabaseResponse::try_from(response).await
})
}
}

/// A future of a create database response
type CreateDatabase = futures::future::BoxFuture<'static, crate::Result<CreateDatabaseResponse>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type probably should be pub:

Suggested change
type CreateDatabase = futures::future::BoxFuture<'static, crate::Result<CreateDatabaseResponse>>;
pub type CreateDatabase = futures::future::BoxFuture<'static, crate::Result<CreateDatabaseResponse>>;


#[derive(Serialize)]
struct CreateDatabaseBody<'a> {
pub id: &'a str,
}

#[derive(Debug, Clone, PartialEq, PartialOrd)]
pub struct CreateDatabaseResponse {
pub database: Database,
Expand Down
7 changes: 2 additions & 5 deletions sdk/cosmos/tests/attachment_00.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,8 @@ async fn attachment() -> Result<(), azure_cosmos::Error> {

// create a temp database
let _create_database_response = client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 💘!

.into_future()
.await
.unwrap();

Expand Down
4 changes: 1 addition & 3 deletions sdk/cosmos/tests/collection_operations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ async fn collection_operations() -> Result<(), BoxedError> {
let database_name = "test-collection-operations";
let context = Context::new();

client
.create_database(context.clone(), database_name, CreateDatabaseOptions::new())
.await?;
client.create_database(database_name).into_future().await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context above is no longer needed, correct?


// create collection!
let db_client = client.clone().into_database_client(database_name.clone());
Expand Down
14 changes: 4 additions & 10 deletions sdk/cosmos/tests/cosmos_collection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,8 @@ async fn create_and_delete_collection() {
let client = setup::initialize().unwrap();

client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
.into_future()
.await
.unwrap();

Expand Down Expand Up @@ -85,11 +82,8 @@ async fn replace_collection() {
const COLLECTION_NAME: &str = "test-collection";

client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
.into_future()
.await
.unwrap();

Expand Down
7 changes: 2 additions & 5 deletions sdk/cosmos/tests/cosmos_database.rs.disabled
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,8 @@ async fn create_and_delete_database() {

// create a new database and check if the number of DBs increased
let database = client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
.into_future()
.await
.unwrap();

Expand Down
21 changes: 6 additions & 15 deletions sdk/cosmos/tests/cosmos_document.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,8 @@ async fn create_and_delete_document() {
let client = setup::initialize().unwrap();

client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
.into_future()
.await
.unwrap();

Expand Down Expand Up @@ -126,11 +123,8 @@ async fn query_documents() {
let client = setup::initialize().unwrap();

client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
.into_future()
.await
.unwrap();
let database_client = client.into_database_client(DATABASE_NAME);
Expand Down Expand Up @@ -203,11 +197,8 @@ async fn replace_document() {
let client = setup::initialize().unwrap();

client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
.into_future()
.await
.unwrap();
let database_client = client.into_database_client(DATABASE_NAME);
Expand Down
8 changes: 1 addition & 7 deletions sdk/cosmos/tests/create_database_and_collection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,7 @@ async fn create_database_and_collection() -> Result<(), BoxedError> {

// create database!
log::info!("Creating a database with name '{}'...", database_name);
let db = client
.create_database(
context.clone(),
&database_name,
CreateDatabaseOptions::new(),
)
.await?;
let db = client.create_database(&database_name).into_future().await?;
log::info!("Successfully created a database");
log::debug!("The create_database response: {:#?}", db);

Expand Down
7 changes: 2 additions & 5 deletions sdk/cosmos/tests/permission.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,8 @@ async fn permissions() {

// create a temp database
let _create_database_response = client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
.into_future()
.await
.unwrap();

Expand Down
7 changes: 2 additions & 5 deletions sdk/cosmos/tests/permission_token_usage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,8 @@ async fn permission_token_usage() {

// create a temp database
let _create_database_response = client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
.into_future()
.await
.unwrap();

Expand Down
7 changes: 2 additions & 5 deletions sdk/cosmos/tests/trigger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,8 @@ async fn trigger() -> Result<(), azure_cosmos::Error> {

// create a temp database
let _create_database_response = client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
.into_future()
.await
.unwrap();

Expand Down
7 changes: 2 additions & 5 deletions sdk/cosmos/tests/user.rs.disabled
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,8 @@ async fn users() {

// create a temp database
let _create_database_response = client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
.into_future()
.await
.unwrap();

Expand Down
7 changes: 2 additions & 5 deletions sdk/cosmos/tests/user_defined_function00.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,8 @@ async fn user_defined_function00() -> Result<(), azure_cosmos::Error> {

// create a temp database
let _create_database_response = client
.create_database(
azure_core::Context::new(),
DATABASE_NAME,
CreateDatabaseOptions::new(),
)
.create_database(DATABASE_NAME)
.into_future()
.await
.unwrap();

Expand Down