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

Remove reflection-based JSON serialization from SwaggerUIMiddleware #2724

Closed
wants to merge 1 commit into from
Closed

Remove reflection-based JSON serialization from SwaggerUIMiddleware #2724

wants to merge 1 commit into from

Conversation

VMelnalksnis
Copy link

Fixes #2550

@VMelnalksnis VMelnalksnis marked this pull request as ready for review November 21, 2023 08:58
@HakamFostok
Copy link

Can I ask a question, I saw the bug and I reviewed the PR, I know this is not my business, but I just want to wonder, if possible, why this PR is not merged yet.
I faced this problem in my Application, that led me to here.
Thank you very much for your efforts.

Havunen added a commit to Havunen/DotSwashbuckle2 that referenced this pull request Feb 11, 2024
@TurcanStanislav
Copy link

TurcanStanislav commented Feb 15, 2024

Hi everyone. @VMelnalksnis, @bdukes, @domaindrivendev, @captainsafia, @eslamisepehr can I ask whether you will have time soon to fix the issues with the workflow and merge this PR? It would be great if you would manage to do this because this will help a lot of people. Thanks!

@VMelnalksnis
Copy link
Author

I see that CI is failing because .NET 6 SDK is missing:
image
Looks like a fix for this was already provided in #2672, though I'm not sure what is the correct fix - changing the test target framework or installing the correct SDK. Either way, I can't merge PRs so someone else will have to review/merge/publish new version.

Havunen added a commit to Havunen/DotSwashbuckle that referenced this pull request Feb 18, 2024
@martincostello
Copy link
Collaborator

Thanks for contributing - if you'd like to continue with this pull request, please rebase against the default branch to pick up our new CI. We'd also appreciate tests for this scenario to prevent regressions.

@VMelnalksnis
Copy link
Author

Ok, I'll check out the current tests and see where I can add them.

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.71%. Comparing base (0108842) to head (0bdd165).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2724      +/-   ##
==========================================
+ Coverage   91.69%   91.71%   +0.01%     
==========================================
  Files          91       91              
  Lines        3022     3029       +7     
  Branches      519      519              
==========================================
+ Hits         2771     2778       +7     
  Misses        251      251              
Flag Coverage Δ
Linux 91.71% <100.00%> (+0.01%) ⬆️
Windows 91.71% <100.00%> (+0.01%) ⬆️
macOS 91.71% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@martincostello
Copy link
Collaborator

Ok, I'll check out the current tests and see where I can add them.

I was looking at this myself yesterday and came up with this that builds on top of #2799: 880fc5f

If you'd like to cherry-pick those changes I'm happy to close my PR in favour of this one if it's making basically the same updates.

@VMelnalksnis
Copy link
Author

I see that the PR already adds net8.0 targets to tests, and I guess fixes all AOT/reflection based issues, not just in this one place. It would make sense to go with that PR, and I'm just happy to see the issue resolved.

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.

SwaggerUIMiddleware is broken when used with .NET Native AOT
6 participants