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.

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

[PM-6137] Fix invalid Swagger generation in knowndevice #3760

Merged
merged 3 commits into from
Feb 12, 2024

Conversation

dani-garcia
Copy link
Member

Type of change

- [x] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Using [FromHeader] without a header name on a request parameter generates an invalid Swagger schema, so we need to lift the parameters out of the class to make sure the generated schema is correct.

@dani-garcia dani-garcia changed the title Fix invalid swagger generation in knowndevice Fix invalid Swagger generation in knowndevice Feb 7, 2024
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (1d9fe79) 39.33% compared to head (c979c84) 39.34%.

Files Patch % Lines
src/Api/Controllers/DevicesController.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3760   +/-   ##
=======================================
  Coverage   39.33%   39.34%           
=======================================
  Files        1040     1039    -1     
  Lines       51632    51630    -2     
  Branches     4613     4613           
=======================================
  Hits        20312    20312           
+ Misses      30376    30374    -2     
  Partials      944      944           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dani-garcia dani-garcia changed the title Fix invalid Swagger generation in knowndevice [PM-6137] Fix invalid Swagger generation in knowndevice Feb 7, 2024
@dani-garcia dani-garcia requested review from a team and JaredSnider-Bitwarden February 7, 2024 16:18
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

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

LGTM. Seems weird that Swagger can't figure out [FromHeader] on the request model and on each property within it though.

@dani-garcia
Copy link
Member Author

Yeah, been an open issue in Swashbuckle for a while domaindrivendev/Swashbuckle.AspNetCore#1938

Thankfully this is the only place where FromHeader is used this way.

@JaredSnider-Bitwarden
Copy link
Contributor

Thank you for linking the issue. Interesting stuff.

@bitwarden-bot
Copy link

bitwarden-bot commented Feb 7, 2024

Logo
Checkmarx One – Scan Summary & Details88ed17e6-2b9c-40bd-b140-b024da3e2e46

No New Or Fixed Issues Found

dani-garcia added a commit to bitwarden/sdk-sm that referenced this pull request Feb 8, 2024
## Type of change
```
- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other
```

## Objective
Update swagger generator to version 7.2.0, pull in the new templates and
generate the new API crates.

I've also updated the build instructions to use .NET 8, as is required
in the latest server code.

The version of the server code used to generate this is
bitwarden/server@fc1d7c7
I've had to make some small modifications to the server code to get it
working, ~~and I'll link the PR here shortly.~~
bitwarden/server#3760

To make this easier to review, I've split this into three commits:
- b42f189: Update the swagger and
dotnet versions, and pull in the new templates as-is. Note that this
won't generate working code.
- 9727d46: Patched the provided
templates to make it work with our code. This is in a separate commit to
make it easier to see what we've changed and hopefully make it easier in
the future to update these templates. The changes are:
    - Ignore warnings by default 
    - Use Uuids instead of strings
    - Use serde_repr to support int enums
    - Disabled default features for reqwest (to remove default tls)
- Removing double optionals (This could be reverted once we're better at
marking everything as required in the server)
- ~~f9b63606477d0f0f987b8534ef6e6755c76003a2: New autogenerated swagger
code, no manually edited files here.~~ Moved to a separate PR (#593)
@dani-garcia dani-garcia merged commit fd3f05d into main Feb 12, 2024
45 of 47 checks passed
@dani-garcia dani-garcia deleted the ps/fix-swagger-knowndevice branch February 12, 2024 10:04
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.

3 participants