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

Draft
wants to merge 3 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 three commits:

  • Use request-trait template contains the same updates that we do to the templates, plus some extras:
    • Use uuid instead of &str
    • Change the use of async_trait to be ?Send. This is needed because reqwest in WASM is not Send.
    • Updated the generated docs to point to the correct function. (This is getting upstreamed soon)
    • Simplified lifetimes, as we don't really use any references (they can't be entirely removed as mockall requires them)
    • Simplified the new ApiClient. Previously this would eagerly initialize all the sub clients, but we have over 60 of them, So I've updated the template to initialize them on use. Also replaced the manual implementation of the mock ApiClient by automock
  • 🤖 Generate bindings: Generate the bindings using the template from the previous step. this is based on the same server bindings as Update OpenAPI version and templates #296
  • Update code to new api. Updated our code to build with the new template, most changes are very simple, the biggest changes are in the client/internal.rs file: 04fa21c#diff-2cf6e40146fc7fd4790f2501e1d115cd208d1c025ae6da7095e7e9aa7a590168

⏰ 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 2 times, most recently from f562246 to 285bb80 Compare July 7, 2025 11:09
Also updated the request-trait templates to match the changes we'de done to reqwest:
- Replace &str by uuids when appropiate
- Removed lifetimes, which were unused

Also updated the build script and added missing async-trait and mockall dependencies
@dani-garcia dani-garcia force-pushed the ps/migrate-to-openapi-rust-trait branch from 285bb80 to 94dbf76 Compare July 7, 2025 11:40
Copy link

sonarqubecloud bot commented Jul 7, 2025

@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

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