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.

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

Update Rust guidelines for module layout #8490

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update Rust guidelines for module layout #8490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 36 additions & 0 deletions docs/rust/implementation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,42 @@ Azure/azure-sdk-for-rust/
└─ Cargo.toml
```

## Module Layout {#rust-modules}

Rust modules should be defined such that:

1. All clients and models that the user can create are exported from the crate root e.g., `azure_security_keyvault_secrets`.
Copy link
Member

Choose a reason for hiding this comment

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

I worry that this makes the root a bit cluttered. Maybe it's ok for clients/models/enum types but for client method options do we think there's value in having them in the root? More often than not callers just pass None (at least this is the case in Go). What if we were to either leave them in clients only, or put them in a separate module?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to it. The idea was to have all creatable in the root, but there's likely more client method options than request models. Well, unless the request models include a lot of sub-models like Cognitive would have. Hmm, now I'm starting to rethink this. Maybe we shouldn't export models from the root.

@RickWinter @JeffreyRichter what do you think? I mean, rust-analyzer should make light work of these either way and it does mitigate possibly shuffling models in an out (edge case, but definitely happens).

So, it'd be:

  • Creatable clients, and their client options and client method options, in the root (but re-exported from clients just so all clients are always found there).
  • Subclients, and their client options and client method options, in the clients submodule only.
  • All models only from models.

@analogrelay and @vincenttran-msft how do you feel about this?

Copy link
Member

Choose a reason for hiding this comment

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

IMO it's cleaner.

I do still have reservations about client method options being in the root. What if we put them in the root IFF they contain modeled fields? I think it would help quiet down the noise, unless we expect callers to do something with the method_options field on a fairly routine basis.

Copy link
Member Author

@heaths heaths Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe, but I'm not too crazy about the subtleties here. I doubt customers would get the subtlety and I imagine I'll probably even forget after a time why some are re-exported from the root and others aren't.

What does Go do here? Or, IIRC, does it export everything from the root which, yes, is noisy. I do want to be less noisy, but also not introduce a bunch of subtlety that would likely be hard to maintain for various reasons and likely lost on customers anyway.

That said, but the reasons you brought up, maybe all client method options are only exported from clients. I think we still keep client options with their clients as those will likely get used more.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, client options stay with their client (e.g. for instantiable clients, its client options type would also go in the root).

Agreed on the subtleties. Keeping the client method options in clients is an improvement IMO since so many of them are essentially empty. And if customers hate it, we can always change it later before GA.

In Go we dump everything in the root for two reasons.

  • to avoid import explosions
  • to avoid sub-package import name collisions

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point. However, I think the same applies if we are to keep them in the clients module. With that being the case, do we like foo_crate::clients::FooClientGetOptions or foo_crate::options::FooClientGet better?

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer the latter just because it's clear. By default, rust-analyzer is going to import types so you eng up with FooClientGet which is not clear on face value what it does. "Options" being part of the name hopefully seems more obvious. It's also idiomatic: std, tokio, and many others have "***Options" types.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, let's keep the Options suffix. So, do we want to keep them all in the clients module then? Or do we want to put all client method options in some other module?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think client options should be exported from clients but client method options should / could be exported from models. In a way, that's really what they are and I don't want to add a bunch of modules unless it makes a lot of sense to.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I like this, let's start with that.

2. All subclients that can only be created from other clients should only be exported from the `clients` submodule e.g., `azure_security_keyvault_secrets::clients`.
3. All client options and client method options are exported from the same module(s) from which their associated clients are exported e.g., `azure_security_keyvault_secrets` and `azure_security_keyvault_secrets::clients`.
4. Extension methods on clients should be exported from the same module(s) from which their associated clients are exported.
5. Extension methods on models should be exported from the same module(s) from which their associated models are exported.

Effectively, export creatable types from the root and keep associated items together. These creatable types are often the only types that users will need to reference by name so we want them easily discoverable.
All clients will be exported from a `clients` submodule so they are easy to find, but creatable clients would be re-exported from the crate root e.g.,

```rust
// lib.rs
mod generated;
mod helpers;

pub use generated::clients::*;
pub use generated::clients::{
SecretClient,
SecretClientOptions,
SecretClientSetSecretOptions,
// ...
};
pub mod models {
pub use generated::enums::*;
pub use generated::models::*;
};
pub use models::{
SetSecretParameters,
// ...
};
pub use helpers::*;
```

<!-- Links -->

{% include refs.md %}
Expand Down
64 changes: 31 additions & 33 deletions docs/rust/introduction.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,18 @@ error[E0252]: the name `Client` is defined multiple times
|
```

{% include requirement/SHOULD id="rust-client-namespace" %} place service client types that the consumer is most likely to interact with in the root module of the client library e.g., `azure_security_keyvault_secrets`.
{% include requirement/SHOULD id="rust-client-namespace" %} export all service client types that the consumer can create and is most likely to interact with in the root module of the client library e.g., `azure_security_keyvault_secrets`.

{% include requirement/MUST id="rust-client-submodule" %} export all clients and subclients from a `clients` submodule of the crate root e.g., `azure_security_keyvault_secrets::clients`. Clients that can be created directly as described above should be re-exported from the crate root e.g.,

```rust
// lib.rs
pub mod clients;

pub use clients::SecretClient;
```

See [Rust modules][rust-modules] for more information.

{% include requirement/MUST id="rust-client-immutable" %} ensure that all service client methods are thread safe (usually by making them immutable and stateless).

Expand Down Expand Up @@ -160,7 +171,9 @@ Client options should be plain old data structures to allow easy, idiomatic crea

{% include requirement/MUST id="rust-client-configuration-name" %} define a client options struct with the same as the client name + "Options" e.g., a `SecretClient` takes a `SecretClientOptions`.

{% include requirement/SHOULD id="rust-client-configuration-namespace" %} export client option structs from the root module of the client library e.g., `azure_security_keyvault_secrets`.
{% include requirement/SHOULD id="rust-client-configuration-namespace" %} export client option structs from the same module(s) from which clients and subclients are exported e.g., `azure_security_keyvault_secrets` and `azure_security_keyvault_secrets::clients` for `SecretClient`.

See [Rust modules][rust-modules] for more information.

{% include requirement/MUST id="rust-client-configuration-fields" %} define all client-specific fields of client option structs as public and of type `Option<T>` except for `api_version` of type `String`, if applicable.

Expand Down Expand Up @@ -223,7 +236,9 @@ impl Default for SecretClientOptions {
{% include requirement/MUST id="rust-client-methods-configuration-name" %} define a client method options struct with the same name as the client, client method name, and "Options" e.g., a `set_secret` takes an `Option<SecretClientSetSecretOptions>` as the last parameter.
This is required even if the service method does not currently take any options because - should it ever add options - the client method signature does not have to change and will not break callers.

{% include requirement/SHOULD id="rust-client-methods-configuration-namespace" %} export client method option structs from the root module of the client library e.g., `azure_security_keyvault_secrets`.
{% include requirement/SHOULD id="rust-client-methods-configuration-namespace" %} export client method option structs from the same module(s) from which associated clients and subclients are exported e.g., `azure_security_keyvault_secrets` and `azure_security_keyvault_secrets::clients` for `SecretClient`.

See [Rust modules][rust-modules] for more information.

{% include requirement/MUST id="rust-client-methods-configuration-fields" %} define all client method-specific fields of method option structs as public and of type `Option<T>`.

Expand All @@ -244,7 +259,6 @@ impl SecretClientMethods for SecretClient {
async fn set_secret(
&self,
name: &str,
value: String,
options: Option<SecretClientSetSecretOptions>,
) -> azure_core::Result<Response<KeyVaultSecret>> {
todo!()
Expand Down Expand Up @@ -334,41 +348,19 @@ Various extensions also exist that the caller may use that may otherwise not wor

{% include requirement/MUST id="rust-parameters-self" %} take a `&self` as the first parameter. All service clients must be immutable

{% include requirement/MUST id="rust-parameters-into" %} declare parameter types as concrete types e.g., `String` (or any type `T`) when the data will be owned e.g., a field in a request model; or `&str` (or any reference to type `&T`)` when the data only needs to be borrowed e.g., a URL parameter.

This will be most common when the data passed to a function will be stored in a struct e.g.:

```rust
pub struct SecretClientOptions {
api_version: String,
}

impl SecretClientOptions {
pub fn new(api_version: String) -> Self {
Self {
api_version,
}
}
}
```

This allows callers to pass a `String` or `str` e.g., `SecretClientOptions::new("7.4")`.

{% include requirement/MUST id="rust-parameter-asref" %} declare parameter types as `impl AsRef<T>` where `T` is a common `std` reference type that implements `AsRef<T>` e.g., `str`, when the parameter data is merely borrowed.

This is useful when the parameter data is temporary, such as allowing a `str` endpoint to be passed that will be parsed into an `azure_core::Url` e.g.:
{% include requirement/MUST id="rust-parameters-into" %} declare parameter types as reference types e.g., `&str` (or any reference to type `&T`)` when the data only needs to be borrowed e.g., a URL parameter.

```rust
impl SecretClient {
pub fn new(endpoint: impl AsRef<str>) -> Result<Self> {
let endpoint = azure_core::Url::parse(endpoint.as_ref())?;
pub fn new(endpoint: &str) -> Result<Self> {
let endpoint = azure_core::Url::parse(endpoint)?;

todo!()
}
}
```

The `endpoint` parameter is never saved so a reference is fine. This also allows callers to pass a `String` or `str` e.g., `SecretClient::new("https://myvault.vault.azure.net")`.
The `endpoint` parameter is never saved so a reference is fine. Except for possible body parameters, any parameter should typically be borrowed since required parameters comprise URL path segments or query parameters.

{% include requirement/MUST id="rust-parameters-request-content" %} declare a parameter named `content` of type `RequestContent<T>`, where `T` is the service-defined request model.

Expand Down Expand Up @@ -438,7 +430,9 @@ pub struct SetSecretOptions {

{% include requirement/MUST id="rust-subclients-suffix" %} name all client methods returning a client with the `_client` suffix e.g., `CosmosClient::database_client()`.

{% include requirement/MUSTNOT id="rust-subclients-export" %} export subclients from the crate root.
{% include requirement/MUSTNOT id="rust-subclients-export" %} export subclients from the crate root. They should be exported from a `clients` submodule of the crate root example `azure_security_keyvault_secrets::clients`.

See [Rust modules][rust-modules] for more information.

{% include requirement/MUSTNOT id="rust-subclients-noasync" %} define client methods returning a client as asynchronous.

Expand All @@ -452,6 +446,10 @@ In addition to service client types, Azure SDK APIs provide and use other suppor

This section describes guidelines for the design _model types_ and all their transitive closure of public dependencies (i.e. the _model graph_). A model type is a representation of a REST service's resource.

{% include requirement/MUST id="rust-model-types-export" %} export all models used in requests from the crate root.

See [Rust modules][rust-modules] for more information.

{% include requirement/MUST id="rust-model-types-derive" %} derive or implement `Clone` and `Default` for all model structs.

{% include requirement/MUST id="rust-model-types-serde" %} derive or implement `serde::Serialize` and/or `serde::Deserialize` as appropriate i.e., if the model is input (serializable), output (deserializable), or both.
Expand Down Expand Up @@ -751,6 +749,8 @@ Cargo.lock
Cargo.toml
```

You can find a complete example of our directory structure in our [implementation documentation][rust-directories].

#### Common Libraries

{% include requirement/MUST id="rust-common-macros-review" %} review new macros with your language architect(s).
Expand Down Expand Up @@ -902,12 +902,10 @@ let client = SecretClient::new(...);
/// # Arguments
///
/// * `name` - The name of the secret.
/// * `value` - The value of the secret.
/// * `options` - Optional properties of the secret.
async fn set_secret(
&self,
name: &str,
value: String,
options: Option<SetSecretMethodOptions>,
) -> Result<Response>;
```
Expand Down
2 changes: 2 additions & 0 deletions docs/rust/refs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,6 @@
[general-recorded-tests]: {{ site.baseurl }}{% link docs/general/implementation.md %}#recorded-tests
[registered package list]: {{ site.baseurl }}{% link docs/tables/registered_namespaces.md %}
[rust-client-convenience]: {{ site.baseurl }}{% link docs/rust/implementation.md %}#rust-client-convenience
[rust-directories]: {{ site.baseurl }}{% link docs/rust/implementation.md %}#rust-directories
[rust-safety-debug]: {{ site.baseurl }}{% link docs/rust/implementation.md %}#rust-safety-debug
[rust-modules]: {{ site.baseurl }}{% link docs/rust/implementation.md %}#rust-modules