Update docs for bitwarden-core #618
Open
+102
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Great job! No new security vulnerabilities introduced in this pull request |
Contributor
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #618 +/- ##
=======================================
Coverage 78.88% 78.88%
=======================================
Files 291 291
Lines 31689 31689
=======================================
Hits 24998 24998
Misses 6691 6691 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @trmartin4's task in 1m 23s —— View job Code Review Complete
Review has been completed and posted. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
crates/bitwarden-core/README.md
Outdated
| } | ||
|
|
||
| /// Extension which exposes `generator` method on the `Client` struct. | ||
| pub trait GeneratorClientExt { |
There was a problem hiding this comment.
❌ Critical: Incorrect trait name
The trait name should be GeneratorClientsExt (with an 's'), not GeneratorClientExt.
Suggested change
| pub trait GeneratorClientExt { | |
| pub trait GeneratorClientsExt { |
This matches the actual trait name in the codebase and will prevent compilation errors for developers following this example.
| /// Extension trait which exposes `generator()` method on the `Client` struct | ||
| pub trait GeneratorClientsExt { | ||
| fn generators(&self) -> GeneratorClient; |
There was a problem hiding this comment.
❌ Critical: Incorrect method name
The method should be generator() (singular), not generators() (plural).
Suggested change
| fn generators(&self) -> GeneratorClient; | |
| fn generator(&self) -> GeneratorClient<'_> { |
This matches the actual implementation in bitwarden-generators/src/generator_client.rs:106-107.
| } | ||
| // Usage: | ||
| // let password = client.generators().password(request)?; |
There was a problem hiding this comment.
❌ Critical: Incorrect method name in usage example
The method should be generator() (singular), not generators() (plural).
Suggested change
| // let password = client.generators().password(request)?; | |
| // Usage: client.generator().password(request)?; |
This must match the corrected trait method name.
Member
Author
There was a problem hiding this comment.
I kept this because plural seems to make more sense in this example?
|
Overall Assessment: APPROVE Reviewed consolidation of bitwarden-core documentation from contributing docs into the crate README. |
Hinton
reviewed
Dec 19, 2025
crates/bitwarden-core/README.md
Outdated
Comment on lines
86
to
201
| ## API requests | ||
|
|
||
| One of the responsibilities of the `Client` is managing and exposing the `ApiClient` instances for | ||
| our API and Identity back-end services, which should be used to make HTTP requests. | ||
|
|
||
| These `ApiClient`s should be accessed through the `ApiConfigurations` struct that is returned from | ||
| the `get_api_configurations()` function. `get_api_configurations()` also refreshes the | ||
| authentication token if required. | ||
|
|
||
| ```rust | ||
| # use bitwarden_core::Client; | ||
| # async fn example(client: &Client) -> Result<(), Box<dyn std::error::Error>> { | ||
| // Example API call | ||
| let api_config = client.internal.get_api_configurations().await; | ||
| let response = api_config.api_client.ciphers_api().get_all().await?; | ||
| # Ok(()) | ||
| # } | ||
| ``` | ||
|
|
||
| ### Server API bindings | ||
|
|
||
| To make the requests, we use auto-generated bindings whenever possible. We use `openapi-generator` | ||
| to generate the Rust bindings from the server OpenAPI specifications. These bindings are regularly | ||
| updated to ensure they stay in sync with the server. | ||
|
|
||
| The bindings are exposed as multiple crates, one for each backend service: | ||
|
|
||
| - [`bitwarden-api-api`](../bitwarden-api-api/README.md): For the `Api` service that contains most of | ||
| the server side functionality. | ||
| - [`bitwarden-api-identity`](../bitwarden-api-identity/README.md): For the `Identity` service that | ||
| is used for authentication. | ||
|
|
||
| When performing any API calls the goal is to use the generated bindings as much as possible. This | ||
| ensures any changes to the server are accurately reflected in the SDK. The generated bindings are | ||
| stateless, and always expects to be provided a Configuration instance. The SDK exposes these under | ||
| the get_api_configurations function on the Client struct. | ||
|
|
||
| You should not expose the request and response models of the auto-generated bindings and should | ||
| instead define and use your own models. This ensures the server request / response models are | ||
| decoupled from the SDK models and allows for easier changes in the future without breaking backwards | ||
| compatibility. | ||
|
|
||
| We recommend using either the `From` or `TryFrom` conversion traits depending on if the conversion | ||
| requires error handling or not. Below are two examples of how this can be done: | ||
|
|
||
| ```rust | ||
| # use bitwarden_crypto::EncString; | ||
| # use serde::{Serialize, Deserialize}; | ||
| # use serde_repr::{Serialize_repr, Deserialize_repr}; | ||
| # | ||
| # #[derive(Serialize, Deserialize, Debug, Clone)] | ||
| # struct LoginUri { | ||
| # pub uri: Option<EncString>, | ||
| # pub r#match: Option<UriMatchType>, | ||
| # pub uri_checksum: Option<EncString>, | ||
| # } | ||
| # | ||
| # #[derive(Clone, Copy, Serialize_repr, Deserialize_repr, Debug, PartialEq)] | ||
| # #[repr(u8)] | ||
| # pub enum UriMatchType { | ||
| # Domain = 0, | ||
| # Host = 1, | ||
| # StartsWith = 2, | ||
| # Exact = 3, | ||
| # RegularExpression = 4, | ||
| # Never = 5, | ||
| # } | ||
| # | ||
| # #[derive(Debug)] | ||
| # struct VaultParseError; | ||
| # | ||
| impl TryFrom<bitwarden_api_api::models::CipherLoginUriModel> for LoginUri { | ||
| type Error = VaultParseError; | ||
|
|
||
| fn try_from(uri: bitwarden_api_api::models::CipherLoginUriModel) -> Result<Self, Self::Error> { | ||
| Ok(Self { | ||
| uri: EncString::try_from_optional(uri.uri) | ||
| .map_err(|_| VaultParseError)?, | ||
| r#match: uri.r#match.map(|m| m.into()), | ||
| uri_checksum: EncString::try_from_optional(uri.uri_checksum) | ||
| .map_err(|_| VaultParseError)?, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| impl From<bitwarden_api_api::models::UriMatchType> for UriMatchType { | ||
| fn from(value: bitwarden_api_api::models::UriMatchType) -> Self { | ||
| match value { | ||
| bitwarden_api_api::models::UriMatchType::Domain => Self::Domain, | ||
| bitwarden_api_api::models::UriMatchType::Host => Self::Host, | ||
| bitwarden_api_api::models::UriMatchType::StartsWith => Self::StartsWith, | ||
| bitwarden_api_api::models::UriMatchType::Exact => Self::Exact, | ||
| bitwarden_api_api::models::UriMatchType::RegularExpression => Self::RegularExpression, | ||
| bitwarden_api_api::models::UriMatchType::Never => Self::Never, | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Updating bindings after a server API change | ||
|
|
||
| When the API exposed by the server changes, new bindings will need to be generated to reflect this | ||
| change for consumption in the SDK. This includes adding new fields to server request / response | ||
| models, removing fields from models, or changing types of models. | ||
|
|
||
| This can be done the following ways: | ||
|
|
||
| 1. Run the `Update API Bindings` workflow in the `sdk-internal` repo. | ||
| 2. Wait for an automatic binding update to run, which is scheduled every 2 weeks. | ||
|
|
||
| Both of these will generate a PR that will require approval from any teams whose owned code is | ||
| affected by the binding updates. | ||
|
|
||
| > [!IMPORTANT] Bindings should **not** be updated manually as part of the changes to consume the new | ||
| > server API in the SDK. Doing so manually risks causing conflicts with the auto-generated bindings | ||
| > and causing more work in the future to address it. |
Member
There was a problem hiding this comment.
I don't think this has anything to do with bitwarden-core.
Member
Author
There was a problem hiding this comment.
I moved the Server API Bindings section to the root README and started discussion of it here: #622 (comment).
Hinton
reviewed
Dec 19, 2025
crates/bitwarden-core/README.md
Outdated
Comment on lines
7
to
9
| <div class="warning"> | ||
| Generally you should <b>not</b> find yourself needing to edit this crate! When possible, please use the feature crates instead. | ||
| </div> | ||
| > [!WARNING] Do not add business logic or feature-specific functionality to this crate. Use feature | ||
| > crates instead. |
Member
There was a problem hiding this comment.
The div syntax shows up properly in rustdoc.
Hinton
reviewed
Dec 19, 2025
Comment on lines
11
to
17
| ## Features | ||
| ## `Client` structure | ||
|
|
||
| - `internal` - Internal unstable APIs that should only be consumed by internal Bitwarden clients. | ||
| - `no-memory-hardening` - Disables `bitwarden-crypto` memory hardening. | ||
| - `secrets` - Secrets Manager specific functionality. | ||
| - `uniffi` - Mobile bindings. | ||
| - `wasm` - WebAssembly bindings. |
Member
There was a problem hiding this comment.
Features should be documented as again they show up in rustdoc.
Member
Author
There was a problem hiding this comment.
Sorry, I completely missed that this was referring to Rust "features", not just a description of the features that are included in this crate. I've added it back.
Hinton
reviewed
Dec 19, 2025
crates/bitwarden-core/README.md
Outdated
|
|
||
| ### Server API bindings | ||
|
|
||
| To make the requests, we use auto-generated bindings whenever possible. We use `openapi-generator` |
Member
There was a problem hiding this comment.
Suggested change
| To make the requests, we use auto-generated bindings whenever possible. We use `openapi-generator` | |
| To make api requests, we use auto-generated bindings whenever possible. We use `openapi-generator` |
Member
Author
There was a problem hiding this comment.
This is now obsolete, as I've re-worded this in #638.
crates/bitwarden-core/README.md
Outdated
| - [`bitwarden-api-identity`](../bitwarden-api-identity/README.md): For the `Identity` service that | ||
| is used for authentication. | ||
|
|
||
| When performing any API calls the goal is to use the generated bindings as much as possible. This |
Member
There was a problem hiding this comment.
This is duplicated with the above statement. Maybe we can merge the sections.
Member
Author
There was a problem hiding this comment.
This is now obsolete, as I've re-worded this in #638.
trmartin4
added a commit
that referenced
this pull request
Dec 30, 2025
## 📔 Objective Adds documentation to the root README to describe how to consume and update API bindings. Includes changes from #622 in which @harr1424 identified issues with our local binding generation instructions as well. Despite this not being the preferred way to update bindings, it's important for those instructions to be accurate as well. Corresponding PR for removing the docs from Contributing Docs: bitwarden/contributing-docs#735. Related PR for adding more documentation to `bitwarden-core`: #618. ## 🚨 Breaking Changes <!-- Does this PR introduce any breaking changes? If so, please describe the impact and migration path for clients. If you're unsure, the automated TypeScript compatibility check will run when you open/update this PR and provide feedback. For breaking changes: 1. Describe what changed in the client interface 2. Explain why the change was necessary 3. Provide migration steps for client developers 4. Link to any paired client PRs if needed Otherwise, you can remove this section. --> ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## 🦮 Reviewer guidelines <!-- Suggested interactions but feel free to use (or not) as you desire! --> - 👍 (`:+1:`) or similar for great changes - 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info - ❓ (`:question:`) for questions - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (`:art:`) for suggestions / improvements - ❌ (`:x:`) or⚠️ (`:warning:`) for more significant problems or concerns needing attention - 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt - ⛏ (`:pick:`) for minor or nitpick changes --------- Co-authored-by: John Harrington <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29893
📔 Objective
Consolidates and updates documentation for
bitwarden-corecrate.This includes moving documentation from https://contributing.bitwarden.com/architecture/sdk/internal/, which will be removed in bitwarden/contributing-docs#733.
🚨 Breaking Changes
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes