[PM-24051] UserDecryption response model tagged with sdk-wasm in OpenApi generated schema #6171
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6171 +/- ##
==========================================
+ Coverage 48.51% 49.02% +0.50%
==========================================
Files 1755 1755
Lines 77881 77940 +59
Branches 6944 6951 +7
==========================================
+ Hits 37787 38213 +426
+ Misses 38584 38214 -370
- Partials 1510 1513 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
| public required MasterPasswordUnlockKdfResponseModel Kdf { get; init; } | ||
| [EncryptedString] public required string MasterKeyEncryptedUserKey { get; init; } | ||
| [StringLength(256)] public required string Salt { get; init; } | ||
| [Required] public required MasterPasswordUnlockKdfResponseModel Kdf { get; init; } |
There was a problem hiding this comment.
💭 It looks like in newer versions of the swagger NuGet package, the required keyword is honored. Good to know we need both for now.
There was a problem hiding this comment.
Should we just update the package in that case?
There was a problem hiding this comment.
I would ask the same question. I notice latest is v9.0.3 (releases), and there is a current renovate PR open to upgrade to latest across projects. A review of breaking changes in major 8 and 9 didn't show any immediate smells or suggestions that we'd have a big lift to upgrade the dependency unless there are details I don't know to account for with, e.g. the underlying upgrade to Microsoft.OpenApi in v8.0.0. v8.0.0 would be the first step up from our current, v7.3.2.
| public required MasterPasswordUnlockKdfResponseModel Kdf { get; init; } | ||
| [EncryptedString] public required string MasterKeyEncryptedUserKey { get; init; } | ||
| [StringLength(256)] public required string Salt { get; init; } | ||
| [Required] public required MasterPasswordUnlockKdfResponseModel Kdf { get; init; } |
There was a problem hiding this comment.
Should we just update the package in that case?
| var sdkWasmTypes = new[] | ||
| { | ||
| typeof(UserDecryptionResponseModel), typeof(MasterPasswordUnlockResponseModel), | ||
| typeof(MasterPasswordUnlockKdfResponseModel) | ||
| }; |
There was a problem hiding this comment.
issue: This is not really scalable if we want to expose more requests/responses this way.
suggestion: You could achieve the same using a custom annotation which gets tagged to the response models and checked in this filter.
|
Since this change is driven by bitwarden/sdk-internal#376 and i have not been given any confidence in this approach, i am going to close it and revert to TS clients and SDK separation for the responses's model and parsing. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-24051
📔 Objective
UserDecryption response model tagged with sdk-wasm in OpenApi generated schema.
This is easier than manually post-processing the generated json schema before generating Rust models.
See bitwarden/sdk-internal#376 for example usage.
📸 Screenshots
⏰ Reminders before review
🦮 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