Skip to content

[release/6.0-rc1] Treat reference type parameters in oblivious nullability context as optional#35526

Merged
wtgodbe merged 4 commits intorelease/6.0-rc1from
safia/nullability-fix
Aug 20, 2021
Merged

[release/6.0-rc1] Treat reference type parameters in oblivious nullability context as optional#35526
wtgodbe merged 4 commits intorelease/6.0-rc1from
safia/nullability-fix

Conversation

@captainsafia
Copy link
Copy Markdown
Contributor

@captainsafia captainsafia commented Aug 19, 2021 •
edited
Loading

Description

Prior to this change, the feature that automatically resolves parameter optionality from nullability annotations would've behaved unpredictably.

#nullable disable
app.MapGet("/{name}", (string name) => ...);

Would have treated name as required, even thought it makes more sense for it to be optional in the oblivious context.

Customer Impact

This PR makes the optionality-from-nullability feature more intuitive for users who would otherwise get unexpected 400 errors from their applications.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Change was validating manual and with unit tests and affects a new codepath that is limited to minimal APIs.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

Fixes #35507

@captainsafia captainsafia added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Aug 19, 2021
@captainsafia captainsafia marked this pull request as ready for review August 19, 2021 22:10
@captainsafia captainsafia added the ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. label Aug 19, 2021
@captainsafia captainsafia changed the title [release/6.0-rc1] Treat parameters in oblivious nullability context as optional [release/6.0-rc1] Treat reference type parameters in oblivious nullability context as optional Aug 20, 2021
davidfowl
davidfowl reviewed Aug 20, 2021
View reviewed changes
Comment thread src/Http/Http.Extensions/src/RequestDelegateFactory.cs Outdated
@davidfowl
Copy link
Copy Markdown
Member

davidfowl commented Aug 20, 2021

You also need to update the EndpointApiExplorer

@captainsafia
Copy link
Copy Markdown
Contributor Author

captainsafia commented Aug 20, 2021

You also need to update the EndpointApiExplorer

Ah! I see what you mean now. Fixed.

@davidfowl davidfowl added the api-approved API was approved in API review, it can be implemented label Aug 20, 2021
@davidfowl
Copy link
Copy Markdown
Member

davidfowl commented Aug 20, 2021

Approved for rc1

davidfowl
davidfowl reviewed Aug 20, 2021
View reviewed changes
Comment thread src/Http/Http.Extensions/src/RequestDelegateFactory.cs
@davidfowl
Copy link
Copy Markdown
Member

davidfowl commented Aug 20, 2021

@captainsafia After my merge, you also need to update the new code that handles BindAsync

@davidfowl davidfowl added Servicing-approved Shiproom has approved the issue and removed api-approved API was approved in API review, it can be implemented labels Aug 20, 2021
@captainsafia captainsafia force-pushed the safia/nullability-fix branch from a9bf6f9 to 039733c Compare August 20, 2021 15:42
@lewing
Copy link
Copy Markdown
Member

lewing commented Aug 20, 2021

cc @wtgodbe

@wtgodbe wtgodbe merged commit 781f4fb into release/6.0-rc1 Aug 20, 2021
@wtgodbe wtgodbe deleted the safia/nullability-fix branch August 20, 2021 18:56
@ghost ghost added this to the 6.0-rc1 milestone Aug 20, 2021
captainsafia pushed a commit that referenced this pull request Aug 20, 2021
@captainsafia
[release/6.0-rc1] Treat reference type parameters in oblivious nullab…
4a376a0 
…ility context as optional (#35526)

* Treat parameters in oblivious nullability context as optional

* Only apply fix for reference types

* Update optionality check in API descriptor

* Update check in BindAsync and Mvc.ApiExplorer test
@captainsafia captainsafia mentioned this pull request Aug 20, 2021
Merged
dougbu added a commit that referenced this pull request Aug 24, 2021
@dougbu @dotnet-maestro
@github-actions @Tratcher @JamesNK @MackinnonBuck @BrennanConroy @rafikiassumani-msft
Merge from 'release/6.0-rc1' into 'release/6.0' (#35588)
057ed19 
* [release/6.0-rc1] Update dependencies from dotnet/runtime dotnet/efcore (#35513)

* Set HttpSys read error log levels to debug #35490 (#35542)

Co-authored-by: Chris R <Tratcher@outlook.com>

* [release/6.0-rc1] Treat reference type parameters in oblivious nullability context as optional (#35526)

* Treat parameters in oblivious nullability context as optional

* Only apply fix for reference types

* Update optionality check in API descriptor

* Update check in BindAsync and Mvc.ApiExplorer test

* HTTP/3: Use new QuicStream.ReadsCompleted property in transport (#35483)

Co-authored-by: James Newton-King <james@newtonking.com>

* HTTP/3: Fix incorrectly pooling aborted streams (#35441)

* [release/6.0-rc1] Binding support for 'bool' values with InputRadioGroup and InputSelect (#35523)

* Binding support for 'bool' values with InputRadioGroup and InputSelect (#35318)

* Update CodeCheck.ps1

Co-authored-by: Brennan <brecon@microsoft.com>

* [release/6.0-rc1] Update dependencies from dotnet/efcore dotnet/runtime (#35558)

* Update dependencies from https://github.com/dotnet/efcore build 20210820.19

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.19

* Update dependencies from https://github.com/dotnet/efcore build 20210820.22

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.22

* Update dependencies from https://github.com/dotnet/efcore build 20210820.30

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.30

* Update dependencies from https://github.com/dotnet/runtime build 20210820.15

Microsoft.NETCore.Platforms , Microsoft.NETCore.BrowserDebugHost.Transport , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.Win32.SystemEvents , Microsoft.NET.Runtime.MonoAOTCompiler.Task , Microsoft.Internal.Runtime.AspNetCore.Transport , Microsoft.Extensions.Primitives , Microsoft.Extensions.Options.DataAnnotations , Microsoft.Extensions.Options.ConfigurationExtensions , Microsoft.NET.Runtime.WebAssembly.Sdk , Microsoft.Extensions.Options , Microsoft.Extensions.Logging.TraceSource , Microsoft.Extensions.Logging.EventSource , Microsoft.Extensions.Configuration.UserSecrets , Microsoft.Extensions.Configuration.Json , Microsoft.Extensions.Configuration.Ini , Microsoft.Extensions.Configuration.FileExtensions , Microsoft.Extensions.Configuration.EnvironmentVariables , Microsoft.Extensions.Configuration.CommandLine , Microsoft.Extensions.Configuration.Binder , Microsoft.Extensions.Configuration.Xml , Microsoft.Extensions.Configuration.Abstractions , Microsoft.Extensions.Caching.Memory , Microsoft.Extensions.Caching.Abstractions , Microsoft.Extensions.Configuration , Microsoft.Extensions.DependencyInjection , Microsoft.Extensions.DependencyInjection.Abstractions , Microsoft.Extensions.Logging.EventLog , Microsoft.Extensions.Logging.Debug , Microsoft.Extensions.Logging.Console , Microsoft.Extensions.Logging.Configuration , Microsoft.Extensions.Logging.Abstractions , Microsoft.Extensions.Logging , Microsoft.Extensions.Http , Microsoft.Extensions.Hosting.Abstractions , Microsoft.Extensions.Hosting , Microsoft.Extensions.HostFactoryResolver.Sources , Microsoft.Extensions.FileSystemGlobbing , Microsoft.Extensions.FileProviders.Physical , Microsoft.Extensions.FileProviders.Composite , Microsoft.Extensions.FileProviders.Abstractions , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.browser-wasm , Microsoft.NETCore.App.Ref , System.Windows.Extensions , System.Threading.Channels , System.Text.Json , System.Text.Encodings.Web , System.ServiceProcess.ServiceController , System.Drawing.Common , System.DirectoryServices.Protocols , System.Diagnostics.EventLog , System.Diagnostics.DiagnosticSource , System.IO.Pipelines , System.Security.Permissions , System.Security.Cryptography.Xml , System.Security.Cryptography.Pkcs , System.Runtime.CompilerServices.Unsafe , System.Resources.Extensions , System.Reflection.Metadata , System.Net.Http.WinHttpHandler , System.Net.Http.Json
 From Version 6.0.0-rc.1.21420.7 -> To Version 6.0.0-rc.1.21420.15

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Improve Minimal APIs support for request media types #35082 (#35230) (#35579)

* add support for request media types

* [release/6.0-rc1] Update dependencies from dotnet/efcore dotnet/runtime (#35573)

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Chris R <Tratcher@outlook.com>
Co-authored-by: Safia Abdalla <safia@microsoft.com>
Co-authored-by: James Newton-King <james@newtonking.com>
Co-authored-by: Mackinnon Buck <mackinnon.buck@gmail.com>
Co-authored-by: Brennan <brecon@microsoft.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Rafiki Assumani <87031580+rafikiassumaniMSFT@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidfowl davidfowl davidfowl approved these changes

@halter73 halter73 Awaiting requested review from halter73

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy

@Pilchie Pilchie Awaiting requested review from Pilchie

@Tratcher Tratcher Awaiting requested review from Tratcher

@javiercn javiercn Awaiting requested review from javiercn

@pranavkm pranavkm Awaiting requested review from pranavkm

Assignees

No one assigned

Labels

ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Servicing-approved Shiproom has approved the issue

Projects

None yet

Milestone

6.0-rc1

Development

Successfully merging this pull request may close these issues.

4 participants

@captainsafia @davidfowl @lewing @wtgodbe