Skip to content

Add nullable to DataProtection #22591

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

Merged
merged 2 commits into from
Jun 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<PropertyGroup>
<TargetFrameworks>netstandard2.0;$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<TargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<Nullable>annotations</Nullable>
</PropertyGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<Compile Include="Microsoft.AspNetCore.DataProtection.Abstractions.netstandard2.0.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public static IDataProtector CreateProtector(this IDataProtectionProvider provid
// because we don't want the code provider.CreateProtector() [parameterless] to inadvertently compile.
// The actual signature for this method forces at least one purpose to be provided at the call site.

IDataProtector protector = provider.CreateProtector(purpose);
IDataProtector? protector = provider.CreateProtector(purpose);
if (subPurposes != null && subPurposes.Length > 0)
{
protector = protector?.CreateProtector((IEnumerable<string>)subPurposes);
Expand All @@ -111,7 +111,7 @@ public static IDataProtectionProvider GetDataProtectionProvider(this IServicePro

// We have our own implementation of GetRequiredService<T> since we don't want to
// take a dependency on DependencyInjection.Interfaces.
IDataProtectionProvider provider = (IDataProtectionProvider)services.GetService(typeof(IDataProtectionProvider));
var provider = (IDataProtectionProvider?)services.GetService(typeof(IDataProtectionProvider));
if (provider == null)
{
throw new InvalidOperationException(Resources.FormatDataProtectionExtensions_NoService(typeof(IDataProtectionProvider).FullName));
Expand Down
2 changes: 1 addition & 1 deletion src/DataProtection/Abstractions/src/Error.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace Microsoft.AspNetCore.DataProtection
{
internal static class Error
{
public static CryptographicException CryptCommon_GenericError(Exception inner = null)
public static CryptographicException CryptCommon_GenericError(Exception? inner = null)
{
return new CryptographicException(Resources.CryptCommon_GenericError, inner);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>ASP.NET Core data protection abstractions.
Expand All @@ -9,6 +9,7 @@ Microsoft.AspNetCore.DataProtection.IDataProtector</Description>
<IsAspNetCoreApp>true</IsAspNetCoreApp>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;dataprotection</PackageTags>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<PropertyGroup>
<TargetFrameworks>netstandard2.0;$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<TargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<Nullable>annotations</Nullable>
</PropertyGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<Compile Include="Microsoft.AspNetCore.Cryptography.Internal.netstandard2.0.cs" />
Expand Down
3 changes: 2 additions & 1 deletion src/DataProtection/Cryptography.Internal/src/CryptoUtil.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Runtime.ConstrainedExecution;
using System.Runtime.InteropServices;
Expand All @@ -16,7 +17,7 @@ internal unsafe static class CryptoUtil
{
// This isn't a typical Debug.Assert; the check is always performed, even in retail builds.
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void Assert(bool condition, string message)
public static void Assert([DoesNotReturnIf(false)] bool condition, string message)
{
if (!condition)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>Infrastructure for ASP.NET Core cryptographic packages. Applications and libraries should not reference this package directly.</Description>
Expand All @@ -8,6 +8,7 @@
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;dataprotection</PackageTags>
<Nullable>annotations</Nullable>
</PropertyGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<PropertyGroup>
<TargetFrameworks>netstandard2.0;$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<TargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<Nullable>annotations</Nullable>
</PropertyGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<Compile Include="Microsoft.AspNetCore.Cryptography.KeyDerivation.netstandard2.0.cs" />
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>ASP.NET Core utilities for key derivation.</Description>
Expand All @@ -7,6 +7,8 @@
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;dataprotection</PackageTags>
<Nullable>enable</Nullable>
<Nullable Condition="'$(TargetFramework)' == 'netstandard2.0'">annotations</Nullable>
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a seemingly random choice of enable or annotations for each csproj, and why is this the only multi-targeted one that does this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, we can only turn on nullable = annotations for ns2.0 projects. All ns2.0 APIs are oblivious, so if you rely on BCL's api for nullability, it doesn't work. In this case, we rely on annotations in Debug.Assert. Most of the other projects here are implementation heavy, changing it is going to take a lot of work, which is they are annotation only.

</PropertyGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<PropertyGroup>
<TargetFrameworks>netstandard2.0;$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<TargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<Nullable>annotations</Nullable>
</PropertyGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<Compile Include="Microsoft.AspNetCore.DataProtection.netstandard2.0.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;dataprotection</PackageTags>
<Nullable>annotations</Nullable>
</PropertyGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace Microsoft.AspNetCore.DataProtection.EntityFrameworkCore
Expand All @@ -16,11 +16,11 @@ public class DataProtectionKey
/// <summary>
/// The friendly name of the <see cref="DataProtectionKey"/>.
/// </summary>
public string FriendlyName { get; set; }
public string? FriendlyName { get; set; }

/// <summary>
/// The XML representation of the <see cref="DataProtectionKey"/>.
/// </summary>
public string Xml { get; set; }
public string? Xml { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,25 @@ public EntityFrameworkCoreXmlRepository(IServiceProvider services, ILoggerFactor
/// <inheritdoc />
public virtual IReadOnlyCollection<XElement> GetAllElements()
{
using (var scope = _services.CreateScope())
// forces complete enumeration
return GetAllElementsCore().ToList().AsReadOnly();

IEnumerable<XElement> GetAllElementsCore()
{
var context = scope.ServiceProvider.GetRequiredService<TContext>();
using (var scope = _services.CreateScope())
{
var context = scope.ServiceProvider.GetRequiredService<TContext>();

foreach (var key in context.DataProtectionKeys.AsNoTracking())
{
_logger.ReadingXmlFromKey(key.FriendlyName!, key.Xml);

// Put logger in a local such that `this` isn't captured.
var logger = _logger;
return context.DataProtectionKeys.AsNoTracking().Select(key => TryParseKeyXml(key.Xml, logger)).ToList().AsReadOnly();
if (!string.IsNullOrEmpty(key.Xml))
{
yield return XElement.Parse(key.Xml);
}
}
}
}
}

Expand All @@ -67,18 +79,5 @@ public void StoreElement(XElement element, string friendlyName)
context.SaveChanges();
}
}

private static XElement TryParseKeyXml(string xml, ILogger logger)
{
try
{
return XElement.Parse(xml);
}
catch (Exception e)
{
logger?.LogExceptionWhileParsingKeyXml(xml, e);
return null;
}
}
}
}
18 changes: 9 additions & 9 deletions src/DataProtection/EntityFrameworkCore/src/LoggingExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand All @@ -7,23 +7,23 @@ namespace Microsoft.Extensions.Logging
{
internal static class LoggingExtensions
{
private static readonly Action<ILogger, string, Exception> _anExceptionOccurredWhileParsingKeyXml;
private static readonly Action<ILogger, string, string, Exception> _savingKeyToDbContext;
private static readonly Action<ILogger, string?, string?, Exception?> _readingXmlFromKey;
private static readonly Action<ILogger, string, string, Exception?> _savingKeyToDbContext;

static LoggingExtensions()
{
_anExceptionOccurredWhileParsingKeyXml = LoggerMessage.Define<string>(
eventId: new EventId(1, "ExceptionOccurredWhileParsingKeyXml"),
logLevel: LogLevel.Warning,
formatString: "An exception occurred while parsing the key xml '{Xml}'.");
_readingXmlFromKey = LoggerMessage.Define<string?, string?>(
eventId: new EventId(1, "ReadKeyFromElement"),
logLevel: LogLevel.Debug,
formatString: "Reading data with key '{FriendlyName}', value '{Value}'.");
_savingKeyToDbContext = LoggerMessage.Define<string, string>(
eventId: new EventId(2, "SavingKeyToDbContext"),
logLevel: LogLevel.Debug,
formatString: "Saving key '{FriendlyName}' to '{DbContext}'.");
}

public static void LogExceptionWhileParsingKeyXml(this ILogger logger, string keyXml, Exception exception)
=> _anExceptionOccurredWhileParsingKeyXml(logger, keyXml, exception);
public static void ReadingXmlFromKey(this ILogger logger, string? friendlyName, string? keyXml)
=> _readingXmlFromKey(logger, friendlyName, keyXml, null);

public static void LogSavingKeyToDbContext(this ILogger logger, string friendlyName, string contextName)
=> _savingKeyToDbContext(logger, friendlyName, contextName, null);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>Support for storing keys using Entity Framework Core.</Description>
Expand All @@ -7,6 +7,7 @@
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;dataprotection;entityframeworkcore</PackageTags>
<IsPackable>true</IsPackable>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<PropertyGroup>
<TargetFrameworks>netstandard2.0;$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<TargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<Nullable>annotations</Nullable>
</PropertyGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<Compile Include="Microsoft.AspNetCore.DataProtection.Extensions.netstandard2.0.cs" />
Expand Down
4 changes: 2 additions & 2 deletions src/DataProtection/Extensions/src/DataProtectionProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,9 @@ public static IDataProtectionProvider Create(
}

internal static IDataProtectionProvider CreateProvider(
DirectoryInfo keyDirectory,
DirectoryInfo? keyDirectory,
Action<IDataProtectionBuilder> setupAction,
X509Certificate2 certificate)
X509Certificate2? certificate)
{
// build the service collection
var serviceCollection = new ServiceCollection();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>Additional APIs for ASP.NET Core data protection.</Description>
Expand All @@ -7,6 +7,7 @@
<IsAspNetCoreApp>true</IsAspNetCoreApp>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;dataprotection</PackageTags>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ internal sealed class TimeLimitedDataProtector : ITimeLimitedDataProtector
private const string MyPurposeString = "Microsoft.AspNetCore.DataProtection.TimeLimitedDataProtector.v1";

private readonly IDataProtector _innerProtector;
private IDataProtector _innerProtectorWithTimeLimitedPurpose; // created on-demand
private IDataProtector? _innerProtectorWithTimeLimitedPurpose; // created on-demand

public TimeLimitedDataProtector(IDataProtector innerProtector)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>Support for storing data protection keys in Redis.</Description>
Expand All @@ -7,6 +7,7 @@
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;dataprotection;redis</PackageTags>
<IsPackable>true</IsPackable>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
Expand Down
1 change: 0 additions & 1 deletion src/Shared/WebEncoders/WebEncoders.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

#nullable enable

using System;
#if NETCOREAPP
using System.Buffers;
Expand Down