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

Add nullable annotations #300

Open
maxkoshevoi opened this issue Jan 25, 2022 · 8 comments · May be fixed by #590
Open

Add nullable annotations #300

maxkoshevoi opened this issue Jan 25, 2022 · 8 comments · May be fixed by #590
Assignees

Comments

@maxkoshevoi
Copy link

Please, add support for nullable reference types to these packages.

Microsoft.Extensions.* dependencies should be annotated in .Net 7 (not all of them are annotated so far)

@zhiyuanliang-ms
Copy link
Contributor

@maxkoshevoi Where do you want to have nullable reference types?

Is there any use case/scenario where nullable reference would be very helpful for you? Could you provide a code snippet?

@maxkoshevoi
Copy link
Author

@zhiyuanliang-ms Here's an example where they would be helpful. This is the example code from this repository. If I launch it and don't have a connection_string in appsettings.json file, I get a NRE. With nullable reference types there would be a warning that options.Connect() doesn't allow nulls, and settings["connection_string"] might be null

image

var builder = WebApplication.CreateBuilder(args);
builder.Host.ConfigureAppConfiguration((hostingContext, config) =>
{
    var settings = config.AddJsonFile("appsettings.json").Build();
    config.AddAzureAppConfiguration(options =>
    {
        options.Connect(settings["connection_string"]);
    });
});
builder.Services.AddAzureAppConfiguration();

var app = builder.Build();
app.UseAzureAppConfiguration();

app.Run();

@zhiyuanliang-ms
Copy link
Contributor

zhiyuanliang-ms commented Jul 17, 2024

Hi, @maxkoshevoi Thank you for the response.

If I launch it and don't have a connection_string in appsettings.json file, I get a NRE.

This is expected, isn't it? Are you suggesting that we should not throw exception when connection string is null/empty, instead, we should just log warning and do nothing?

BTW, could you make your code like this by having null check for connection string before call AddAzureAppConfiguration.

@maxkoshevoi
Copy link
Author

maxkoshevoi commented Jul 17, 2024

Are you suggesting that we should not throw exception

Not at all, exception is what I would expect in this situation. No behavioral changes are needed.
I'm suggesting that there should be a warning telling the user that there's a possible NRE since IConfigurations indexer is already telling the compiler that it might return null
image

The only thing left is to tell compiler that options.Connect() doesn't expect null as a first argument.

All nullable reference types do is give the compiler additional information, so that it can warn users of potential NREs at compile time. Like so:
image

BTW, could you make your code like this by having null check for connection string before call AddAzureAppConfiguration.

In this case there won't be an NRE (and if nullable reference types were to be enabled in this package the compiler also wouldn't show the warning, where in the previous example it would which would encourage the user to add the if you mentioned to check connection string for null beforehand)

@maxkoshevoi
Copy link
Author

I can create the PR for this issue if it's okay

@zhiyuanliang-ms
Copy link
Contributor

Hi, @maxkoshevoi

I can create the PR for this issue if it's okay

That will be great! Thank you so much.

@maxkoshevoi maxkoshevoi linked a pull request Aug 21, 2024 that will close this issue
@maxkoshevoi
Copy link
Author

@zhiyuanliang-ms could you take a look at the PR please?

@zhiyuanliang-ms
Copy link
Contributor

@maxkoshevoi Sure. I was distracted by some other tasks. I didn't put my main focus on .NET provider. I will take a look this RP once I finish my current tasks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

3 participants