Support RequiredAttribute

[wjdavis5]

I'm going to start working on this for my apps. But wanted to see what you thought about adding support for the RequiredAttribute. When the config class gets initialized verify all properties decorated as required evaluate to a valid value, and if not bubble up the exception.

[bungeemonkee]

Interesting idea. The way I'd been handling this is just that any value without a source is an error - ie they are all required. If you want to allow a null value simply include a DefaultAttribute with the value null. Sources can explicitly return null so this satisfies the constraint of the value having some source return something.

Then the validation I see as a separate concern. That is why currently it is simply an extension method that just uses a bit of reflection to access every public property - causing any backed by the config system to be evaluated. Any errors are collected and throw inside an AggregateException. Then it is up to the developer to catch/handle that exception as they wish.

I like this approach a bit more because it A) does not increase complexity of the config system by requiring validation to be a core feature (but still allows robust validation) and B) still allows for values that by default have no value.

But I'll consider the use case a bit ore before deciding to outright close this issue.

[wjdavis5]

A quick note on this - making the default value null doesnt work for non-nullable types.

[bungeemonkee]

True, but there's also no other way you can ever make a non-nullable value null. That's a language restriction - not a feature or bug of the lib. You can use DefaultValue to put the string "0" for any number types. You can also use "00:00:00" as a default for TimeSpan instances. And you can choose any suitable date you want as a default for DateTime/DateTimeOffset instances. Which only leaves complex structs that you can't put some default value for.

I've been trying to figure out how I can make V4 support more complex types and more dynamic behavior from the sources which probably leads to a more elegant solution to this problem anyway. Once I figure that out this whole issue might be sidestepped.

[RobertChrist]

@bungeemonkee I agree that any property that lacks a source should throw an error.

However, I worry about the case where my applicationsettings.json file is (mistakenly) populated with a propertyname defined, but an invalid null or whitespace value. In these cases, I would normally mark the property with a required attribute to ensure that the invalid null value is not used in my application code. I normally check for this edge case in all my applications, to ensure my build script didn't forget to set a value. But since the adoption of nullable reference types in c#, this is much more important.

Configgy currently works as expected for non-nullable properties, such as an int. If I supply an int with a null or missing value in my applicationsettings.json, then configgy will throw me an invalid input exception instead of supplying Default(int) into my int property. Perfect.

However, as of c#8, with nullable reference types, this is not true for other types any longer. If I mark a string as non-nullable (string, not string?), and have a property name with a null value in my appsettings.json, Configgy will still inject the null value into my configuration string.

According to MSDN, this is the correct default behavior from Configgy, and is the same behavior Microsoft implements in their frameworks. According to their official documentation, MS states that their libraries will also inject null values into non-nullable reference types at application boundaries, and it is up to the application using nullable reference types to check these values at their application input. However, validating these input values at my application boundary is exactly what Configgy is used for.

To achieve this validation, MS ensures that at these application boundaries, applications are given a method of reacting to this behavior. Using Asp.Net as our example framework, MS ensures that a ModelState is supplied with the injected parameters, that reports validity across all properties in the injected object, so the application does not need to iterate every property on its own. If MS injected a null value into a non-nullable string, the ModelState is considered invalid. And ModelState also respects the Required attribute (among others).

This leads to the following scenarios:

inputValue --> propertyType --> ModelState null --> string? --> Valid null --> string --> Invalid null --> [Required]string? --> Invalid null --> [Required]string --> Invalid string.empty --> string? --> valid string.empty --> string --> valid string.empty --> [Required(true)]string? --> valid string.empty --> [Required(true)]string --> valid string.empty --> [Required]string --> Invalid

Currently, Configgy does not support the required attribute, but Configgy also does not give the calling application a method of differentiating between the first two examples shown above (both of which Configgy would consider valid), unless the application code wants to iterate over the properties itself.

So my suggestion for Configgy would be to adopt one of the following solutions.

1 - Configgy could adopt a strict mode (whereby injecting a null into a non-nullable reference type causes Configgy to throw an invalid input error). I would note, this is how early versions of the Configgy library used to work. A separate validator attribute could potentially be used for whitespace.

2 - Configgy could implement or respect the Required attribute, which would notify Configgy whether or not injecting null values into a non-nullable reference type was acceptable behavior on a property-by-property basis, and then Configgy could throw an exception accordingly.

3 - Configgy could return a ModelState with the configuration object, that optionally respects the required attribute, to notify the application whether any properties were injected with nullable (or whitespace) values into non-nullable reference types (among other situations)

[bungeemonkee]

@RobertChrist I understand the concern with the new nullable reference types. I think, rather than complicate Configgy itself with model state validation or multiple operational modes, my preference would be to simply treat nullable and non nullable reference types appropriately per their annotations. It seems that this should always be possible for any public members, and sometimes for private members as well: https://github.com/dotnet/roslyn/blob/main/docs/features/nullable-metadata.md If this attribute is inspected we could make nullable reference types "just work" with Configgy. And the default behavior when those annotations are not present would not need to change. I'm going to work on the change to support nullable reference types - in theory it should not require updating to Net Standard 2.1 since the NullableAttribute is emitted by the compiler as an internal type to the generated assembly, in which case I could release the update as a minor version bump. However this may mean I'll have trouble accessing it properly at runtime as necessary so that could necessitate a major version bump or another solution.

That being said, until this support is added (or when using older versions of Configgy) is it possible to force errors for null or empty string values with a custom validation attribute: https://github.com/bungeemonkee/Configgy/blob/master/Documentation/Pipeline/4-Validate.md#custom-validators

A custom attribute like this could go beyond even validating that objects are non-null, it could check that arrays have a minimum length, that dictionaries contain some specific key(s), etc. There is already a regex validator that could be used to check for null simply my trying to match against ".*" - this would (I think) throw a null argument exception as passing the null string to the regex match function would probably not be seen as a valid parameter. Since this validation is applied to the raw string value before coercion this could be used as a quick-and-dirty null check for any type - not just strings.

[RobertChrist]

I was unaware that the nullable context of the calling code was discernable from within the configgy library. If so, as you state, then yes, that sounds like the ideal solution. I look forward to trying the new version!

As for the other attributes, I am aware I can write a validation attribute. If you are successful with the proposed path, that's the route I would use going forward. gl!