The Wayback Machine - https://web.archive.org/web/20220210101140/https://github.com/dotnet/aspnetcore/pull/40063
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

Jump to bottom

[main] Update dependencies from dotnet/runtime #40063

Merged
merged 4 commits into from Feb 9, 2022
Merged

[main] Update dependencies from dotnet/runtime #40063

merged 4 commits into from Feb 9, 2022
+188 −187

Conversation

@dotnet-maestro
Copy link
Contributor

@dotnet-maestro dotnet-maestro bot commented Feb 8, 2022 •
edited

This pull request updates the following dependencies

From https://github.com/dotnet/runtime

  • Subscription: 32db3699-5666-45da-a1b7-08d8b804cd75
  • Build: 20220208.17
  • Date Produced: February 9, 2022 8:05:28 AM UTC
  • Commit: c4c1c3aac7c42494791aaa5b791ae3641dc2561a
  • Branch: refs/heads/main
@dotnet-maestro
Update dependencies from https://github.com/dotnet/runtime build 2022…
0a3acf0 
…0207.26

Microsoft.NETCore.Platforms , Microsoft.NETCore.BrowserDebugHost.Transport , Microsoft.NETCore.App.Runtime.win-x64 , System.Diagnostics.EventLog , System.Diagnostics.DiagnosticSource , System.DirectoryServices.Protocols , System.Security.Cryptography.Pkcs , System.Runtime.CompilerServices.Unsafe , System.Resources.Extensions , System.Reflection.Metadata , System.Net.Http.Json , System.IO.Pipelines , System.Net.Http.WinHttpHandler , Microsoft.NET.Runtime.WebAssembly.Sdk , Microsoft.NET.Runtime.MonoAOTCompiler.Task , Microsoft.Internal.Runtime.AspNetCore.Transport , Microsoft.Extensions.Primitives , Microsoft.Extensions.DependencyInjection , Microsoft.Extensions.Configuration.Xml , 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.Abstractions , Microsoft.Extensions.Configuration , Microsoft.Extensions.Caching.Memory , Microsoft.Extensions.Caching.Abstractions , Microsoft.Extensions.DependencyInjection.Abstractions , Microsoft.Extensions.DependencyModel , Microsoft.Extensions.FileProviders.Abstractions , Microsoft.Extensions.Options.DataAnnotations , Microsoft.Extensions.Options.ConfigurationExtensions , Microsoft.Extensions.Options , Microsoft.Extensions.Logging.TraceSource , Microsoft.Extensions.Logging.EventSource , Microsoft.Extensions.Logging.EventLog , Microsoft.Extensions.Logging.Debug , Microsoft.Extensions.Logging.Console , Microsoft.Extensions.Logging.Configuration , Microsoft.Extensions.Logging.Abstractions , 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.Logging , Microsoft.NETCore.App.Ref , Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.browser-wasm , System.Security.Cryptography.Xml , System.Threading.RateLimiting , System.Threading.Channels , System.Text.Json , System.Text.Encodings.Web , System.ServiceProcess.ServiceController
 From Version 7.0.0-preview.2.22103.2 -> To Version 7.0.0-preview.2.22107.26
@dotnet-maestro dotnet-maestro bot requested a review from dougbu as a code owner Feb 8, 2022
msftbot[bot]
msftbot bot approved these changes Feb 8, 2022
View changes
Copy link
Contributor

@msftbot msftbot bot left a comment

Auto-approving dependency update.

@SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Feb 8, 2022

@dougbu It looks something like a breaking change in the compiler or build toolchain means that lots (like, 50+) of our code files that were considered valid before are now considered invalid for nullability rules.

Do you recommend any specific contact on dotnet/runtime we should approach for info about this?

@dougbu
Copy link
Member

@dougbu dougbu commented Feb 8, 2022

@jeffhandley why would IMemoryCache.TryGetValue(...) ever leave the out value as null when the function returns null❔ More generally, the new errors all relate to IMemoryCache and ClaimsCache. If you're not the right contact, please refer us.

lots (like, 50+) of our code files that were considered valid before are now considered invalid for nullability rules

@SteveSandersonMS I only see complaints about 3 files in the following. Where are you seeing 50+ problematic files❔

D:\a\_work\1\s\src\Security\Authentication\Negotiate\src\Internal\LdapAdapter.cs(29,35): error CS8602: Dereference of a possibly null reference. [D:\a\_work\1\s\src\Security\Authentication\Negotiate\src\Microsoft.AspNetCore.Authentication.Negotiate.csproj]
D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\Infrastructure\DefaultFileVersionProvider.cs(64,41): error CS8600: Converting null literal or possible null value to non-nullable type. [D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\Microsoft.AspNetCore.Mvc.Razor.csproj]
D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\Infrastructure\DefaultFileVersionProvider.cs(66,20): error CS8603: Possible null reference return. [D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\Microsoft.AspNetCore.Mvc.Razor.csproj]
D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\Infrastructure\DefaultFileVersionProvider.cs(93,17): error CS8600: Converting null literal or possible null value to non-nullable type. [D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\Microsoft.AspNetCore.Mvc.Razor.csproj]
D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\Infrastructure\DefaultFileVersionProvider.cs(94,16): error CS8603: Possible null reference return. [D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\Microsoft.AspNetCore.Mvc.Razor.csproj]
D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\RazorViewEngine.cs(281,56): error CS8600: Converting null literal or possible null value to non-nullable type. [D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\Microsoft.AspNetCore.Mvc.Razor.csproj]
D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\RazorViewEngine.cs(291,16): error CS8603: Possible null reference return. [D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\Microsoft.AspNetCore.Mvc.Razor.csproj]
D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\RazorViewEngine.cs(406,16): error CS8603: Possible null reference return. [D:\a\_work\1\s\src\Mvc\Mvc.Razor\src\Microsoft.AspNetCore.Mvc.Razor.csproj]

@SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Feb 8, 2022 •
edited

Where are you seeing 50+ problematic files

Right, yes, sorry. I was looking at GitHub's "Files Changed" tab and saw a huge list. On closer inspection this includes duplicates for every target platform, so you're right that the underlying set of affected files is much smaller:

image

So yes it does look like some possibly incorrect change in MemoryCache.

@jeffhandley
Copy link
Member

@jeffhandley jeffhandley commented Feb 9, 2022

We just merged in [Group 2] Enable nullable annotations for Microsoft.Extensions.Caching.Abstractions by maxkoshevoi · Pull Request #64018 · dotnet/runtime yesterday, which would be our culprit.

I'll defer to @dotnet/area-extensions-caching for the accuracy of this specific annotation and if the annotation needs to be corrected or if these calls sites need to be.

@eerhardt
Copy link
Member

@eerhardt eerhardt commented Feb 9, 2022

why would IMemoryCache.TryGetValue(...) ever leave the out value as null when the function returns null❔

This is because MemoryCache isn't generic, like for example Dictionary<TKey, TValue>. So there is no way to annotate whether the MemoryCache can cache null values or not. Since MemoryCache supports setting null values, it has to return true and then the out object value will be null in those cases. Since it is a valid scenario to cache null values, the API had to be annotated as public static bool TryGetValue<TItem>(this IMemoryCache cache, object key, out TItem? value). See dotnet/extensions#1260 which originally brought this functionality in.

So if in your usages of the MemoryCache you know that you will never cache null, you can add a ! suppression here.

dotnet-maestro bot and others added 2 commits Feb 9, 2022
@dotnet-maestro
Update dependencies from https://github.com/dotnet/runtime build 2022…
6191086 
…0208.17

Microsoft.NETCore.Platforms , Microsoft.NETCore.BrowserDebugHost.Transport , Microsoft.NETCore.App.Runtime.win-x64 , System.Diagnostics.EventLog , System.Diagnostics.DiagnosticSource , System.DirectoryServices.Protocols , System.Security.Cryptography.Pkcs , System.Runtime.CompilerServices.Unsafe , System.Resources.Extensions , System.Reflection.Metadata , System.Net.Http.Json , System.IO.Pipelines , System.Net.Http.WinHttpHandler , Microsoft.NET.Runtime.WebAssembly.Sdk , Microsoft.NET.Runtime.MonoAOTCompiler.Task , Microsoft.Internal.Runtime.AspNetCore.Transport , Microsoft.Extensions.Primitives , Microsoft.Extensions.DependencyInjection , Microsoft.Extensions.Configuration.Xml , 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.Abstractions , Microsoft.Extensions.Configuration , Microsoft.Extensions.Caching.Memory , Microsoft.Extensions.Caching.Abstractions , Microsoft.Extensions.DependencyInjection.Abstractions , Microsoft.Extensions.DependencyModel , Microsoft.Extensions.FileProviders.Abstractions , Microsoft.Extensions.Options.DataAnnotations , Microsoft.Extensions.Options.ConfigurationExtensions , Microsoft.Extensions.Options , Microsoft.Extensions.Logging.TraceSource , Microsoft.Extensions.Logging.EventSource , Microsoft.Extensions.Logging.EventLog , Microsoft.Extensions.Logging.Debug , Microsoft.Extensions.Logging.Console , Microsoft.Extensions.Logging.Configuration , Microsoft.Extensions.Logging.Abstractions , 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.Logging , Microsoft.NETCore.App.Ref , Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.browser-wasm , System.Security.Cryptography.Xml , System.Threading.RateLimiting , System.Threading.Channels , System.Text.Json , System.Text.Encodings.Web , System.ServiceProcess.ServiceController
 From Version 7.0.0-preview.2.22103.2 -> To Version 7.0.0-preview.2.22108.17
@pranavkm
React to nullable changes
25309aa
@pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Feb 9, 2022 •
edited

I updated this PR, but the setters feel super weird:

public static TItem? Set<TItem>(this IMemoryCache cache, object key, TItem? value)
public static TItem? Set<TItem>(this IMemoryCache cache, object key, TItem? value, MemoryCacheEntryOptions? options)
public static TItem? Set<TItem>(this IMemoryCache cache, object key, TItem? value, IChangeToken expirationToken)
public static TItem? Set<TItem>(this IMemoryCache cache, object key, TItem? value, DateTimeOffset absoluteExpiration)
public static TItem? Set<TItem>(this IMemoryCache cache, object key, TItem? value, TimeSpan absoluteExpirationRelativeToNow)

Why not just let it accept a value of type TItem and let the compiler infer the nullability of the type? In the current form, it results in quirky behavior:

string? cached = memoryCache.Set<string>("some-key", "some-value"); // Why did the memory cache set a null value?

pranavkm
pranavkm reviewed Feb 9, 2022
View changes
src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs
@@ -24,7 +24,7 @@ public static async Task RetrieveClaimsAsync(LdapSettings settings, ClaimsIdenti
settings.ClaimsCache = new MemoryCache(new MemoryCacheOptions { SizeLimit = settings.ClaimsCacheSize });
}

if (settings.ClaimsCache.TryGetValue<IEnumerable<string>>(user, out var cachedClaims))
if (settings.ClaimsCache.TryGetValue<IEnumerable<string>>(user, out var cachedClaims) && cachedClaims is not null)
Copy link
Contributor

@pranavkm pranavkm Feb 9, 2022

Choose a reason for hiding this comment

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

FYI @Tratcher

Copy link
Member

@dougbu dougbu Feb 9, 2022

Choose a reason for hiding this comment

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

Ick. Do we need TryGetNonNullableValue(...) methods to make this look more reasonable when <T> isn't nullable❔ Could stick extension methods like that somewhere…

Copy link
Contributor

@pranavkm pranavkm Feb 9, 2022

Choose a reason for hiding this comment

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

The suggestion @eerhardt had was to use the forgiveness operator, but that just as iffy.

@eerhardt
Copy link
Member

@eerhardt eerhardt commented Feb 9, 2022

@pranavkm, since that extension method is generic, I think that one might have been a mistake. Do you think you could open an issue in dotnet/runtime for that? Looking deeper, there is also a generic extension method public static bool TryGetValue<TItem>(this IMemoryCache cache, object key, out TItem? value) that maybe could be adjusted? We will have to look.

FYI - @maxkoshevoi

@pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Feb 9, 2022

Here you go: dotnet/runtime#65107

@dotnet-maestro dotnet-maestro bot merged commit a1b85ab into main Feb 9, 2022
27 checks passed
@dotnet-maestro dotnet-maestro bot deleted the darc-main-ee53989b-a935-4bc3-895f-b27abde23cf2 branch Feb 9, 2022
@msftbot msftbot bot added this to the 7.0-preview2 milestone Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pranavkm pranavkm

@msftbot msftbot

@dougbu dougbu

@javiercn javiercn

@Tratcher Tratcher

Assignees
No one assigned
Labels
area-infrastructure Type: Dependency Update 🔼
Projects
None yet
Milestone
7.0-preview2
Linked issues

Successfully merging this pull request may close these issues.

None yet
5 participants
@SteveSandersonMS @dougbu @jeffhandley @eerhardt @pranavkm