Skip to content

Update bindings to use the reqwest-trait template #343

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.

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dani-garcia
Copy link
Member

@dani-garcia dani-garcia commented Jul 4, 2025

🎟️ Tracking

📔 Objective

This template is trait based which should allow easier mocking. It also follows a similar pattern to our clients which I think would make it easier to use.

The PR is in four commits:

  • Update OpenAPI and templates to latest version: Updates the openapi generator and the templates to the latest version. These files should match with the files at https://github.com/OpenAPITools/openapi-generator/tree/98b315c13717adf13d00011787eab915b64d3a14/modules/openapi-generator/src/main/resources/rust I've also gone ahead and deleted some unused files, and added others to .openapi-generator-ignore, to make it clearer which files are being used.
  • Add our changes to the templates
    • Updated build script to run format again, due to what looks like a rustfmt bug
    • Updates to templates
      • Use uuid instead of &str
      • Remove double option pattern
      • Commented out traits usage, temporarily until we fix the size bloat
      • Removed the user agent set from the template, and instead set it on the client during initialization
      • Removed lifetimes, as we don't really use any references
      • Removed generic errors, as we don't use them
  • 🤖 Generate bindings: Generate the bindings using the template from the previous step. this is based on the same server bindings as [PM-22256] - Updating OpenAPI bindings #366
  • Update code to new api. Updated our code to build with the new template, most changes are very simple API adjustments, the biggest changes are in the client/internal.rs file and consist in modifications to how we initialize and update the ApiConfigurations struct to match the newer API.

PR review recommendation

As this PR edits the generated bindings, the total changes are huge and difficult to review. As such, I've written a suggested review guide for the teams.

For vault

Filter by team owned, the vault related changes are fairly small:

  • small update in the /sync code to use the new API
  • updated the folder API code to use the new API

The changes are contained entirely in the last commit: 90007b7

For platform

Due to the big autogenerated changes, I recommend reviewing commit by commit. The first, third and fourth commits should be reviewed as normal. The second commit, marked as 🤖, should instead be reviewed by regenerating the bindings and making sure that nothing is changed:

cd server
git checkout a84e5554fb3c8f5e3618c32a7fc4b1df02a6b939

dotnet tool restore
cd src/Api
dotnet build
dotnet swagger tofile --output "../../api.json" --host "https://api.bitwarden.com" "./bin/Debug/net8.0/Api.dll" "internal"

cd ../../src/Identity
dotnet build
ASPNETCORE_ENVIRONMENT=development  dotnet swagger tofile --output "../../identity.json" --host "https://identity.bitwarden.com" "./bin/Debug/net8.0/Identity.dll" "v1"

cd ../../../sdk-internal
git pull
git checkout ps/migrate-to-openapi-rust-trait
./support/build-api.sh

# Ignore the cargo.toml files, which get updates from renovate
git checkout crates/bitwarden-api-api/Cargo.toml
git checkout crates/bitwarden-api-identity/Cargo.toml

git status
# This should report a clean work tree

I have a server PR including a script to make this easier next time: bitwarden/server#6066

⏰ 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

  • 👍 (:+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

This comment was marked as resolved.

This comment was marked as resolved.

@Hinton
Copy link
Member

Hinton commented Jul 7, 2025

Should we open upstream issues and see if any of the changes can upstreamed?

It would be interesting to see how the mocked usage would be as a comparison to using wiremock.

@dani-garcia dani-garcia force-pushed the ps/migrate-to-openapi-rust-trait branch 3 times, most recently from 285bb80 to 94dbf76 Compare July 7, 2025 11:40
@dani-garcia
Copy link
Member Author

I'm planning to upstream the doc link issue at least, which clearly seems like a mistake. Probably also the removing Send bound, which seems safe. The rest could be harder:

  • Using str for uuids seems intentional for the templates, as it's happening in all variants, even though they use the uuid type in other places.
  • The lifetimes change is mostly stylistic, as it simplifies the generated code a bit, but there may be cases where it's useful to have separately named lifetimes, not sure.
  • The APIclient initialization is fairly subjective on whether it should eagerly initialize all the subclients or to initialize them on each call. To me it makes more sense to do it on each call than to initialize the 60 of them at the same time, but for smaller APIs maybe that doesn't matter

The mocks look something like this: 77e2333

@dani-garcia dani-garcia force-pushed the ps/migrate-to-openapi-rust-trait branch 3 times, most recently from 689da35 to d98d249 Compare July 11, 2025 17:34
@dani-garcia dani-garcia force-pushed the ps/migrate-to-openapi-rust-trait branch 2 times, most recently from 60d0b85 to f867177 Compare July 21, 2025 15:52
@dani-garcia dani-garcia marked this pull request as ready for review July 21, 2025 16:37
@dani-garcia dani-garcia requested review from a team as code owners July 21, 2025 16:37
Hinton
Hinton previously approved these changes Jul 21, 2025
@@ -36,7 +36,7 @@ impl Configuration {
impl Default for Configuration {
fn default() -> Self {
Configuration {
base_path: "http://localhost".to_owned(),
base_path: "https://identity.bitwarden.com".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.

Could this be potentially dangerous? We should never use the default config as we don't know if it's EU or US.

Copy link
Member Author

Choose a reason for hiding this comment

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

We never use this default, we use the value from ClientSettings:

impl Default for ClientSettings {
fn default() -> Self {
Self {
identity_url: "https://identity.bitwarden.com".into(),
api_url: "https://api.bitwarden.com".into(),
user_agent: "Bitwarden Rust-SDK".into(),
device_type: DeviceType::SDK,
}

@dani-garcia dani-garcia marked this pull request as draft July 21, 2025 19:31
@dani-garcia
Copy link
Member Author

dani-garcia commented Jul 21, 2025

Moving to draft for now as the compiler can't seem to optimize the unused trait impls away, leading to 2x file sizes for the final wasm bundle. I'll look into modifying the generated code to not be as resource heavy.

Edit: I have some improvements to the generated code that reduce the size of the binary considerably, but they are deeper changes into the templates that will benefit from upstreaming, so for now I've commented out the use of traits in this PR, and I will revisit that in a separate PR when I'm back from vacation.

@dani-garcia dani-garcia force-pushed the ps/migrate-to-openapi-rust-trait branch from f867177 to c3d62e3 Compare July 31, 2025 16:05
@dani-garcia dani-garcia force-pushed the ps/migrate-to-openapi-rust-trait branch from c3d62e3 to d4c8e00 Compare July 31, 2025 16:24
@dani-garcia dani-garcia force-pushed the ps/migrate-to-openapi-rust-trait branch from d4c8e00 to 90007b7 Compare July 31, 2025 16:43
Copy link

@dani-garcia dani-garcia marked this pull request as ready for review July 31, 2025 16:53
@dani-garcia dani-garcia requested a review from Hinton July 31, 2025 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants