dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.95k stars 4.65k forks source link

Null configuration elements deserialized as empty strings #36510

Open kenlyon opened 5 years ago

kenlyon commented 5 years ago

Describe the bug

When a property is explicitly set a value of null in appsettings.json, I expected that to be null after the configuration was loaded, but it actually has a value of "". This led to a bug in our code that was hard to track down. Ideally, I'd like a way to change this behaviour, although it is possible to work around it, now that I know this is how it behaves.

To Reproduce

Steps to reproduce the behavior:

  1. In Visual Studio 2017, create a project of type "ASP.NET Core Web Application".
  2. When prompted for a template, choose the following options:
    • .NET Core
    • ASP.NET Core 2.1
    • API
  3. Add a new class called "MyConfig" and add the following properties:

    public string Prop1 { get; set; }
    public string Prop2 { get; set; }  
    public string Prop3 { get; set; }  
    public string Prop4 { get; set; }
  4. Add the following elements to the "appsettings.json" file:

    "Prop1": "Prop 1 Value",
    "Prop2": "",
    "Prop3": null

    (IMPORTANT: Do not add an entry for Prop4.)

  5. Edit the Startup class:

    • Append this line to the ConfigureServices method:

      services.Configure(Configuration);

    • Append the following parameter to the Configure method:

      IOptions config

  6. Add a breakpoint in the Configure method and debug the project in IIS Express.
  7. When the breakpoint is hit, inspect the value of the config parameter.

Results

Property Expected Value Actual Value
config.Value.Prop1 "Prop 1 Value" "Prop 1 Value"
config.Value.Prop2 "" ""
config.Value.Prop3 null ""
config.Value.Prop4 null null

Additional context

Output of dotnet --info:

C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional>dotnet --info
.NET Core SDK (reflecting any global.json):
 Version:   2.1.403
 Commit:    04e15494b6

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.17134
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\2.1.403\

Host (useful for support):
  Version: 2.1.5
  Commit:  290303f510

.NET Core SDKs installed:
  1.0.0-preview2-003131 [C:\Program Files\dotnet\sdk]
  1.0.4 [C:\Program Files\dotnet\sdk]
  1.1.0 [C:\Program Files\dotnet\sdk]
  2.0.2 [C:\Program Files\dotnet\sdk]
  2.1.4 [C:\Program Files\dotnet\sdk]
  2.1.100 [C:\Program Files\dotnet\sdk]
  2.1.104 [C:\Program Files\dotnet\sdk]
  2.1.200 [C:\Program Files\dotnet\sdk]
  2.1.201 [C:\Program Files\dotnet\sdk]
  2.1.202 [C:\Program Files\dotnet\sdk]
  2.1.302 [C:\Program Files\dotnet\sdk]
  2.1.400 [C:\Program Files\dotnet\sdk]
  2.1.402 [C:\Program Files\dotnet\sdk]
  2.1.403 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.1.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 1.0.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 1.0.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 1.1.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.0.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.0.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.0.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.0.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]

To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download

C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional>
oliverjanik commented 4 years ago

Any update here? This is truly a violation of principle of least surprise.

vdailly commented 3 years ago

Hello,

Some days ago I had the same disappointed experience regarding configuration. And as many other I spend a lof of time to understand the issue, and digging into source code. With the number of opened issues on this subject, the IConfiguration and related, while very convenient and powerful miss something probably.

Let's consider this minimal Json example:

{
   "obj1": [ ],
   "obj2": [
      "value"
    ]
}

The IConfiguration result is as follow: obj2:0 "value"

obj1 has completely disappeared between internal JsonParser and IConfiguration. Nevertheless when binding on a Dictionnary<string, T> the key obj1 is meaningful, even if T get its default values. Providing explicitly a property in the configuration of an application means this information is meaningful for the application.

In my own case I add the following configuration:

{
  "obj1": {
     "prop1" : [ ]
   },
  "obj2": {
    "prop1": [
       { "subprop" : "value" }
     ]
  }
}
class Conf {
  public List<Element> Prop1 {get; set; } = new List<Element>();
}

class Element {
  public string SubProp {get; set;} = string.Empty;
}
services.Configure<Dictionnary<string, Conf>>(Configuration);

For me "objx" keys were clearly meaningful, and I supposed to get a Dictionnary<string, Conf> with 2 keys where obj1 should be a default Conf object. But obj1 key-value pair was meaningful enough to break the application when I've updated prop1 to an empty Json array without a clear knowledge that obj1 would disappear.

This topic can be splitted into 2 issues:

Issue 1: parent properties lost on unique null Json Array

Whatever the depth of the Json, if there is only one empty Json array as leaf property, then all parent Json objet are lost. As intermediate solution I would propose to store all the path, whatever the Json kind of property encountered on leaf node. In the example provided, this would store obj1:prop1 in IConfiguration.

So the Binder may create the default Conf`` element of the example. Nevertheless this is only a part of the solution, because we don't exactly know what to store as a value for theobj1:prop1` configuration path. By default this would be null or empty string (I've don't checked). Here the value is not null, it is an empty Json array.

But this lead to the second issue.

An alternate solution is to update the configuration as follow (by example):

[
   { 
      "name" : "obj1",
      "prop1" : [ ] 
   },
   {
     "name" : "obj2",
     "prop1" : [
       { "subprop" : "value" }
     ]
   }
]

But this is to the caller to transform the List<Conf> to a Dictionnary<string, Conf>. And from the configuration file point of view, Json provide unique property name, while the intent is not clear from a Json array that application require to have a unique name property.

Issue 2: Distinguish explicit null from explicit empty Json array

The main question here is to distinguish an explicit null value from an explicit empty Json Array. As mentioned in several issues, an explicit Json null must be a null. But what is an empty Json Array when stored in the IConfiguration?

Probably the best may be handle empty collection as String.Empty. For the solution, simply use IConfiguration[path].GetChildren().Any() to distinguish a null path element with children from an empty Json array. Meaning that if the Binder encounter by reflection a generic IEnumerable or Collection, Array or whatever collection, the behavior may be:

Propositions

I don't have a PR to submit, because I don't have the tooling for the .NET5 and so on. But Issue1 this is related to file JsonConfigurationFileParser. Something like that may solve the issue1 described here.

var arrayElements = value.EnumerateArray();
if (arrayElements.Count == 0) {
    string key = _currentPath;
    if (_data.ContainsKey(key))
    {
        throw new FormatException(SR.Format(SR.Error_KeyIsDuplicated, key));
    }
    _data[key] = value.ToString();   // or String.Empty internal JsonElement.ToString() does not already set to string.Empty.
} else {
    foreach (JsonElement arrayElement in arrayElements) {
        EnterContext(index.ToString());
        VisitValue(arrayElement);
        ExitContext();
        index++;
    }
}
break;

If I have some time I'll try to search in the binder what to eventually update.

and for issue 2 this may be ConfigurationBinder.cs


private static object BindInstance(Type type, object instance, IConfiguration config, BinderOptions options)
{
           //best update here because value is available before attempting bind on collections.
          if (convertedValue == String.Empty || config.GetChildren().Any()) {
            ... 
          }
         else {
            return convertedValue;
         }
}
maryamariyan commented 3 years ago

In the PR https://github.com/dotnet/runtime/pull/43297 that just got closed we discussed we need to fix this by adding an API, introducing a flag

Link to guideline for formal API proposal is available in api-review-process.md

safern commented 3 years ago

@vdailly would you be interested in submitting an API proposal for a flag that people can use to enable this behavior?

vdailly commented 3 years ago

Hello,

Thanks for your proposal.

Issues described above are linked to 2 librairies and doesn't happen at the same time. The easiest may be to add a flag into the binder

Flag proposition for issue2 (Binder)

For the issue2 described above this may be enabled by adding a property to BinderOptions

    public class BinderOptions
    {
        /// <summary>
        /// When false (the default), the binder will consider null string as null.
        /// If true, the binder will attempt to bind strictly null objets, or create default object. This is useful for Json array handling.
        /// </summary>
        public bool StrictBinding { get; set; }
   }

This may be called like this

services.Configure<MyObject>(context.Configuration.GetSection("section"), binder => binder.StrictBinding = true );

And finally the ConfigurationBinder should respect this new property to enable behavior of PR #43297 .

Flag proposition for issue1 (Json)

For the issue1 this may be more complicated because this happens very early. JsonConfigurationFileParser relie on System.Text.Json and especially JsonDocumentOptions.

Add a flag to System.Text.Json.JsonDocumentOptions (not recommended)

There may be an additionnal flag StrictNull there.

    public struct JsonDocumentOptions
    {
        public bool AllowTrailingCommas { readonly get; set; }
        public JsonCommentHandling CommentHandling { readonly get; set; }
        public int MaxDepth { readonly get; set; }
        public bool StrictNull { readonly get; set }
    }

But I don't like the idea to update a third library, especially because it is multi-purpose.

Create a new Microsoft.Extensions.Configuration.Json.JsonOptions class (recommended)

Create a new class JsonOptions like the BinderOptions. By this way it may be possible to add a new extension method like this to control the JsonConfigurationFileParser StrictNull flag either with a default value, or a customized one.

public static IConfigurationBuilder AddJsonFile(this IConfigurationBuilder builder, IFileProvider provider, string path, bool optional, bool reloadOnChange, Action<JsonOptions> configureOptions)
        {

        }

This may be called like this:

configbuilder.AddJsonFile("appsettings.json", optional: false, reloadOnChange: true, options => {
    options.StrictNull = true;
});
public class JsonOptions {

     public bool StrictNull {get; set;}
}

Finally the JsonConfigurationFileParser should be updated to respect the StrictNull flag of JsonOptions, by updating the behavior like described in the PR #43297. How to provide this new JsonOptions class to JsonConfigurationFileParser is another story. I don't want to dig too much into details nor provide a PR. It is just a proposal.

Default for all (not recommended)

Some may want this as a default behavior. But I don't think this may be a good idea because behavior doesn't happen at the same time and it may be too difficult to update default flags on both libraries simultaneously, even with a high level extension method. Or this may require another level of abstraction.

The best is that end-user enable the behavior in each library separately, depending of their need, and the combination of these flags enable the behavior. There may be a documentation paragraph about this with a small example. A simplified version of new test from PR #43297 by example.

Waiting your feedback about this proposal.

tarockx commented 3 years ago

Wasted an hour today trying to debug my .NET5 application when this was the problem all along... is there a fix yet, or should we just always check for empty string for now?

pinkfloydx33 commented 3 years ago

I spent a few hours debugging this today. For IConfiguration-based setup, the Azure SDK looks for a property called serviceUri, which we have set in our base AppSettings.json file.

For local development though, we need to use the connectionString property with UseDevelopmentStorage=true. We tried to override serviceUri with an explicit null. Unfortunately, the Azure SDK checks for serviceUri first and foremost and considers any non-null as a "valid" value. It then throws when it tries to use the empty string as a Uri.

There are workarounds--setting serviceUri to the azurite endpoint (which could vary for a developer) and hard-coding the credential/accesskey (which doesn't change, so is sort of OK). Another option is a dedicated config section and switching on environment, or something user-secrets based. A final option is to set the root of the section to the connection string, making it both a section and a value upon config source layering as Azure special-cases when the section has a value (doesn't work for us though as we have other settings in the section).

Either way, it's a lot of cruft to have to manage when we should have been able to just set serviceUri to null to begin with.

Spending several hours tracking this down wasn't fun either.

I'm going to open an issue over at Azure to try and get them to check for the empty string in addition to null. But it would be nice to have this built in. I realize the ship has sailed on the default behavior, but something opt-in would be nice. In the meantime we may just add a custom version of the json file provider to our common libraries. As we come to understand the actual behavior, we now realize we have code/configuration that is only working properly by chance as we've made the assumption generally across our stack.

quetzalcoatl commented 2 years ago

It's really disappointing that #43297 was just closed.. This may easily lead to simply throwing it away instead of escalating this ridiculous bug. While I can somewhat understand trimming 'empty' subtrees, changing NULLs to ""? Pray tell, why?

montanag commented 2 years ago

Does anyone have a workaround for this? Very annoying bug. Considering just parsing the json and passing that into my options classes.

pinkfloydx33 commented 2 years ago

I copy/pasted the code and made my own configuration provider that handled the nulls differently. It works, just something extra to maintain.

p3t3rix commented 2 years ago

@maryamariyan Is one of the proposals of @vdailly appropriate/considered ? The added JsonOptions class looks pretty non-disruptive to the api and would allow further extensions.

lsadehaan commented 2 years ago

Just wasted a lot of time on this, still no fix?

ErroneousFatality commented 2 years ago

This is ridiculous. It's been 4 years since this was reported. Can we get this resolved or should be just accept it as the standard now?

qbalipka commented 1 year ago

Still nothing?

Swimburger commented 1 year ago

This causes issues when binding multiple sections to a single object, as the null values should not override previously bound properties, but it does because it isn't null, but an empty string.

I ended up sanitizing this for config sections I know this is safe to do and necessary, before binding:

private static void ChangeEmptyStringToNull(IConfigurationSection configSection)
{
    if (configSection == null) return;
    if (configSection.Value == "") configSection.Value = null;
    foreach (var childConfigSection in configSection.GetChildren())
    {
        ChangeEmptyStringToNull(childConfigSection);
    }
}
crozone commented 1 year ago

Currently working around this issue by having the property convert to null:

private string? someProperty ;
public string? SomeProperty {
    get => !string.IsNullOrEmpty(someProperty ) ? someProperty : null;
    set => someProperty = value;
}

I can get away with this because I never expect an empty string in this value, but it's still not a great solution.

I can't believe this is still an issue after 4 years. It's such a basic, easy to hit issue.

Varorbc commented 1 year ago

@tarekgh Do you have any plans to fix it?

tarekgh commented 1 year ago

We'll look at this at some point in the future. This is why we are keeping the issue open.

MagnetDS commented 1 year ago

2023, this is still an issue on .Net 7.

Rudazzle commented 1 year ago

In case anyone missed it in the original post, one workaround is to remove any elements from your json file that are strings and should be null. If the element is not found, then it gets set to NULL instead of empty string. Also, I noticed with non-string nullable types, it handles NULL as expected.

Sample JSON File { "Options": [ { "name": "None", //"nullableString": null, "nullableInt": null }, { "name": "Option 1", //"nullableString": null, "nullableInt": null }, { "name": "Option 2", "nullableString": "OPTION-TWO", "nullableInt": 6 } ] }

SoftStoneDevelop commented 11 months ago

Any news on solving the problem? Unexpected behavior when on null in json config return empty string is still here.

tarekgh commented 11 months ago

This issue is marked to be addressed in the future release. I am seeing some workaround is mentioned above. can't you use that for now?

SoftStoneDevelop commented 11 months ago

@tarekgh I didn't realize that this issue was marked to be addressed in the next release. Sure, I can use workarounds, I just wish I had a fix rather than a workaround considering the problem has been lingering for almost five years.

vriesdea commented 7 months ago

My two cents as workaround (but no empty string support): `static public class ConfigurationExtensions {

static public string GetRequiredString(this IConfigurationSection section, string key) {
    string? value = section[key];
    return (string.IsNullOrWhiteSpace(value) ? throw new ArgumentNullException(key) : value);
}
ninjamojo commented 4 months ago

Just ran into a production issue with this, and spent far too much time figuring it out. The problem here is that everything else in the configuration API suggests a null value of a setting should be honored in the deserialization. This is a surprising result, and I can't imagine anyone would expect or rely on this behavior. Now I'm wondering what other ticking time bombs are out there in my other configs.

AFract commented 2 months ago

Hello, I am replying here just to ask if .Net 8 brought an update regarding this ? Like all others, I don't like missing settings treated as "empty" or "default" of the type, instead I would like to be able to throw an exception or choose behavior to have a "fail fast" option instead of unexpected behavior in production.

tarekgh commented 2 months ago

Due to other high-priority work, we haven't had a chance to address this issue yet.

seanamos commented 1 month ago

This is a surprisingly big trap in configuration. I've known about it for a while, but every couple months, it pops up again as someone adds a new config or a new project is started and hours are lost to debugging.