[Client bug]: Nullable Reference Types are misleading

[CollinAlpert]

Describe the bug Properties are marked as nullable which should never be null.

To Reproduce Consider the following method and assume I have <Nullable>enable</Nullable> set in my .csproj file:

private async Task<List<MailFolder>> GetMailFolders(UserItemRequestBuilder user)
{
    var folders = await user.MailFolders.GetAsync();
    return folders.Value;
}

When building, I get two nullability warnings, CS8602 (folders might be null) and CS8603 (folders.Value might be null). The first one I could understand. Something went wrong with the request so the response is null. The second, however, I have a hard time wrapping my head around. Collection data structures should rarely be null. In the case something went wrong, they are often simply empty. I looked into the SDK code and saw that pretty much every property is marked as nullable, even properties which can never be null, like an Id. This doesn't help the developer at all. I just upgraded from v4 to v5 and my build is now littered with incorrect nullability warnings.

Expected behavior Only properties which actually can be null, are marked as nullable.

Client version 5.2.0

Desktop (please complete the following information):

• OS: macOS
• Version: Ventura (13.2.1)

[AtOMiCNebula]

I think the v5 SDK's support for NRTs is actually pretty broken, which is unfortunate. For your first warning ("folders might be null"), an exception is thrown if something goes wrong with the request (documented here: https://github.com/microsoftgraph/msgraph-sdk-dotnet/blob/dev/docs/errors.md), it doesn't actually return null.

From looking at the posted Graph docs, it seems to me like they weren't written with NRT in mind.

• The "Create Requests" page uses var for all response types (https://learn.microsoft.com/en-us/graph/sdks/create-requests?tabs=CS), but if you inspect the type they're all unexpectedly nullable.
• The "Paging" example is the most suspicious, since if you paste the first example (https://learn.microsoft.com/en-us/graph/sdks/paging?tabs=csharp#iterate-over-all-the-messages) into a NRT-enabled context, it throws warnings because the messages response is nullable but CreatePageIterator (rightly!) doesn't expect that.
• At least one of the examples in the Upgrade to v5 docs doesn't work without warnings either (https://github.com/microsoftgraph/msgraph-sdk-dotnet/blob/dev/docs/upgrade-to-v5.md#collections).

Graph SDK team, what's going on here? I'm sticking with the V4 SDK until this is cleaned up, since I'd rather not have to pepper nullable types and null-forgiving operators all over my code just to pick up V5. ☹️

Example V4 code adapted from a project I was looking to upgrade to V5:

IGraphServiceApplicationsCollectionPage response = await graphClient.Applications
    .Request()
    .Filter($"appId eq '00000000-0000-0000-0000-000000000000'")
    .GetAsync();
Application app = response.Single();
Console.WriteLine($"App's ObjectId is: {app.Id}");

Example V5 code changes I'd have to make, with maybe-null result on line 1 and two null-forgiveness operators on line 5:

ApplicationCollectionResponse? response = await graphClient.Applications.GetAsync(request =>
{
    request.QueryParameters.Filter = $"appId eq '00000000-0000-0000-0000-000000000000'";
});
Application app = response!.Value!.Single();
Console.WriteLine($"App's ObjectId is: {app.Id}");

[andrekiba]

@CollinAlpert @AtOMiCNebula I'm completely agree with you...as many (really many!) other things in Graph this is a mess... If there is a meaning I don't understand can someone please try to explain it? ☹️ It's unbelievable...

[MrAdam]

It would be great if we could get an indication about whether this will actually change or not. Right now, this is the main thing keeping us from upgrading to v5, as we don't want to litter null-forgiving operators around the code-base.

[CollinAlpert]

Absolutely, it would be nice to know if this will be changed sometime soon.

@MrAdam What I did was create a wrapper class around the client which just passes through the arguments and then added #nullable disable to the top of that wrapper.

[MrAdam]

Any news on whether this will actually be fixed, or not?

[panmona]

Also stumbled over this inconsistency. It would be nice if the value in .Value can just be empty and not null.