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

Nullability in SignalR #29219

Merged
merged 9 commits into from Feb 17, 2021
Merged

Nullability in SignalR #29219

merged 9 commits into from Feb 17, 2021

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Jan 11, 2021

Fixes #25612
Fixes #28954

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Jan 11, 2021
@@ -116,7 +116,7 @@ internal StreamTracker StreamTracker
/// <summary>
/// Gets the user for this connection.
/// </summary>
public virtual ClaimsPrincipal? User => Features.Get<IConnectionUserFeature>()?.User;
public virtual ClaimsPrincipal User => Features.Get<IConnectionUserFeature>()?.User ?? new ClaimsPrincipal();
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what to do here. Cache the User?

The reason I'm not keeping it null is because it's nicer to deal with an empty ClaimsPrincipal than a null one. An empty one will not be authenticated in any way or have any claims. The Auth types we use don't have null annotations for the user as the expectation is that you'll at least pass an empty one.

Copy link
Member

Choose a reason for hiding this comment

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

Assign blank ClaimsPrincipal to underlying IConnectionUserFeature when the property is accessed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The IConnectionUserFeature could come from anyone (in theory), and the interface allows a null user. Plus the interface itself could not be on the connection.

@JamesNK
Copy link
Member

JamesNK commented Jan 21, 2021

Will review when PR is out of draft 🥇

@@ -43,7 +43,7 @@ public static Task<TResult> InvokeAsync<TResult>(this HubConnection hubConnectio
/// The <see cref="Task{TResult}.Result"/> property returns a <typeparamref name="TResult"/> for the hub method return value.
/// </returns>
[SuppressMessage("ApiDesign", "RS0026:Do not add multiple overloads with optional parameters", Justification = "Required to maintain compatibility")]
public static Task<TResult> InvokeAsync<TResult>(this HubConnection hubConnection, string methodName, object arg1, CancellationToken cancellationToken = default)
public static Task<TResult> InvokeAsync<TResult>(this HubConnection hubConnection, string methodName, object? arg1, CancellationToken cancellationToken = default)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be Task<TResult?>

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so? That would force callers to always handle null even if they never expect it to be null. If they use a nullable for TResult then that means they expect a null value and should handle it, and the compiler will let them know that. So seems better to let the user decide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. It's effectively the same choice we made with JSInterop. But we also do not provide any guarantees against it which is crummy.

src/SignalR/common/Shared/MemoryBufferWriter.cs Outdated Show resolved Hide resolved
@@ -80,7 +78,7 @@ public static bool ReadAsBoolean(this ref Utf8JsonReader reader, string property
throw new InvalidDataException($"Expected '{propertyName}' to be of type {JsonTokenType.String}.");
}

return reader.GetString();
return reader.GetString()!;
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that's just a lie

Copy link
Member Author

Choose a reason for hiding this comment

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

No it isn't, we check above that the token type is a string. If the string is null it would have token type null.

Base automatically changed from master to main January 22, 2021 01:33
@BrennanConroy BrennanConroy marked this pull request as ready for review January 22, 2021 21:25
@BrennanConroy BrennanConroy requested review from halter73 and a team as code owners January 22, 2021 21:25
@@ -98,7 +98,7 @@ public static IDisposable On<T1>(this HubConnection hubConnection, string method

return hubConnection.On(methodName,
new[] { typeof(T1), typeof(T2), typeof(T3) },
args => handler((T1)args[0], (T2)args[1], (T3)args[2]));
args => handler((T1)args[0]!, (T2)args[1]!, (T3)args[2]!));
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I wonder if there is a better way to avoid all these bangs.

{
#pragma warning disable CS8764 // Nullability of return type doesn't match overridden member (possibly because of nullability attributes).
Copy link
Member

Choose a reason for hiding this comment

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

Should ConnectionId be made nullable on ConnectionContext?

Copy link
Member

Choose a reason for hiding this comment

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

I really think that people should be able to rely on ConnectionContext.ConnectionId being non-null. I'm thinking this should just throw if someone tries to access this property before StartAsync() completes.

This implementation of ConnectionContext.ConnectionId is already a bit odd in that its setter throws, so I don't think throwing from the getter when the connection hasn't started is a big deal.

Copy link
Member Author

Choose a reason for hiding this comment

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

people should be able to rely on ConnectionContext.ConnectionId being non-null

It's not just if you access it before starting, if you skip negotiation then this will be null as well.

@@ -80,12 +80,12 @@ public override Task RemoveFromGroupAsync(string connectionId, string groupName,
}

/// <inheritdoc />
public override Task SendAllAsync(string methodName, object?[]? args, CancellationToken cancellationToken = default)
public override Task SendAllAsync(string methodName, object?[] args, CancellationToken cancellationToken = default)
Copy link
Member

Choose a reason for hiding this comment

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

Is this a change from .NET 5? There is an aspnetcore annoucement issue for nullability changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an API change, however passing null would have thrown

{
if (_user is null)
{
_user = Features.Get<IConnectionUserFeature>()?.User ?? new ClaimsPrincipal();
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -100,7 +99,7 @@ public CookieContainer Cookies
/// <summary>
/// Gets or sets the URL used to send HTTP requests.
/// </summary>
public Uri Url { get; set; }
public Uri Url { get; set; } = default!;
Copy link
Member

@halter73 halter73 Jan 27, 2021

Choose a reason for hiding this comment

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

var httpConnectionOptions = new HttpConnectionOptions();
var urlString = httpConnectionOptions.Url.ToString();

The above throws, right? If so, I think this should be marked nullable.

I might feel differently if it was super common for developer to call the HttpConnectionOptions.Url getter and in practice it was always set, but I expect most developers just call WithUrl() and never touch this property.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I changed it was because of "! fatigue", everywhere we access this property internally it will be non-null

if (_httpConnectionOptions.Url != null && _httpConnectionOptions.Url != uriEndPoint.Uri)
{
throw new InvalidOperationException($"If {nameof(HttpConnectionOptions)}.{nameof(HttpConnectionOptions.Url)} was set, it must match the {nameof(UriEndPoint)}.{nameof(UriEndPoint.Uri)} passed to {nameof(ConnectAsync)}.");

Copy link
Member

Choose a reason for hiding this comment

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

I do appreciate trying to avoid "! fatigue" but I don't think that's enough of a reason to misrepresent the nullability of a public property. We could add an internal non-nullable property that mirrors Url and throws if it is null as a sanity check to avoid the fatigue.

Copy link
Member

Choose a reason for hiding this comment

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

We do this in places where something could be null, but for all typical usage it won't be.

e.g. https://github.com/dotnet/aspnetcore/blob/a08aa2dd318956d5b7295561d9423252f6071a52/src/Hosting/Abstractions/src/WebHostBuilderContext.cs

Copy link
Member

Choose a reason for hiding this comment

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

Right, that's why I said this initially:

I might feel differently if it was super common for developer to call the HttpConnectionOptions.Url getter and in practice it was always set, but I expect most developers just call WithUrl() and never touch this property.

Copy link
Member Author

Choose a reason for hiding this comment

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

I expect most developers just call WithUrl() and never touch this property

Right, that makes it less of an issue if we lie with the API.

Copy link
Member

Choose a reason for hiding this comment

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

I would argue that if it's not a huge issue either way, we should default to no lying with the API.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try out a private member so we can avoid using ! everywhere in HttpConnection then.

@BrennanConroy BrennanConroy added this to the 6.0-preview2 milestone Feb 17, 2021
@BrennanConroy BrennanConroy merged commit 46359cd into main Feb 17, 2021
@BrennanConroy BrennanConroy deleted the brecon/nullable branch February 17, 2021 23:46
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
5 participants