Skip to content

use one of behind a flag instead for nullable enums #3428

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 1 commit into
base: master
Choose a base branch
from

Conversation

ItsVeryWindy
Copy link
Contributor

@ItsVeryWindy ItsVeryWindy commented May 27, 2025

Pull Request

The issue or feature being addressed

Fixes #3225.

Details on the issue fix or feature implementation

This changes how the nullable enums work so that instead of creating a reference to a separate type, it uses one of with a reference and a null enum. This is behind a flag UseOneOfForNullableEnums which defaults to off.

Also for non body parameters it will always default to non nullable as there is no way of specifying a nullable parameter.

I've extended the snapshot tests to include more nullable examples. This did uncover another example where if the enum itself is the body of the request it should be marked as nullable.

At the moment I've left inline enums to include null, I'm not sure if this should also behind a flag.

Copy link

codecov bot commented May 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.32%. Comparing base (cec1dc0) to head (a1e89ed).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3428      +/-   ##
==========================================
+ Coverage   94.28%   94.32%   +0.04%     
==========================================
  Files         109      109              
  Lines        3728     3756      +28     
  Branches      707      719      +12     
==========================================
+ Hits         3515     3543      +28     
  Misses        213      213              
Flag Coverage Δ
Linux 94.32% <100.00%> (+0.04%) ⬆️
Windows 94.32% <100.00%> (+0.04%) ⬆️
macOS 94.32% <100.00%> (+0.04%) ⬆️

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.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@martincostello
Copy link
Collaborator

Can you bump this to 8.2.0 please?

<VersionPrefix>8.1.3</VersionPrefix>

@martincostello
Copy link
Collaborator

@captainsafia Any thoughts on this change, as I imagine null handling has come up in the OpenAPI work for .NET 10? As the original change caused unintended issues this PR is aiming to make opt-in to resolve them, I'd like to make sure we're doing it right this time around 😅.

@martincostello
Copy link
Collaborator

@ItsVeryWindy The original change has been manually reverted in #3436 so I can push out a new patch release - you'll need to merge with the default branch and restore the relevant changes in this PR for 8.2.0.

@martincostello martincostello added this to the v8.2.0 milestone Jun 3, 2025
@martincostello
Copy link
Collaborator

My updated thinking here is:

  1. Ship v8.1.3 with Revert #3377 #3436.
  2. Wait a week or so to check there's no more issues from early adopters.
  3. Merge Drop netstandard support #3422 as v9.0.0 (also unblocks Using GZip to compress swagger-ui-dist files in embedded resource to reduce the output dll size #3399).
  4. This PR and fix minimal api json options not being respected #3378 then become part of v9.1.0.
  5. Support .NET 10 #3283 becomes v10.0.0.

@ItsVeryWindy ItsVeryWindy force-pushed the enums-again branch 3 times, most recently from 9c9568b to df2027d Compare June 10, 2025 06:48
@martincostello
Copy link
Collaborator

martincostello commented Jun 27, 2025

Now that it's opt-in I'm kinda OK with this change, but I'd like a second opinion first: #3428 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: required nullable enum should include null
2 participants