JsonApiDotNetCore icon indicating copy to clipboard operation
JsonApiDotNetCore copied to clipboard

Add support for handling of non-nullable reference types in `required`.

Open maurei opened this issue 3 years ago • 4 comments

For non-nullable reference types, a resource attribute should be included in the OpenAPI required property if model state validation is enabled, and should be excluded otherwise.

The following table displays a comprehensive overview of the behaviour of required, nullable in an OpenAPI schema depending on the kind of type of a property, usage of [Required], configuration of NRT and configuration of model state validation.

Screenshot 2022-07-05 at 20 41 17

maurei avatar Nov 16 '21 17:11 maurei

An interesting case worth elaborating on is the OAS nullability on B6 and H6. One might expect TRUE here, after all within the scope an OAS description it is possible for the field here to be required (it cannot be omitted) while simultaneously allowing for null.

The caveat here, however, is that our implementation will not allow null because the model state validation will not allow it. If MSV is disabled, it will would still fail because of a null constraint violation in the database.

maurei avatar Jul 05 '22 18:07 maurei

I'm not entirely sure about H6. The API may be running against a pre-existing database without column constraints in place, or not use EF Core at all. Is there a use case where someone intentionally declares a property as [Required] string?, or is this just a theoretical case?

I'm thinking of a database column that used to be optional, but has become required in vNext. The database column remains nullable (to preserve existing data), yet the API would not accept null as input anymore.

Also, note that [Required] means more than just not-null. Excerpt from https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.dataannotations.requiredattribute?view=net-6.0:

The RequiredAttribute attribute specifies that when a field on a form is validated, the field must contain a value. A validation exception is raised if the property is null, contains an empty string (""), or contains only white-space characters.

bkoelman avatar Jul 11 '22 07:07 bkoelman

Another (theoretical) use case for having something like[Required] string? property could be to read null as a "pristine" state, but disallowing a client to write null. Eg. [Required] int? Grade with value null could mean "no grade (yet)".

I don't know if this is actually configurable with EF Core, because RequiredAttribute means not null, as far as it is concerns mapping to a database column using EF Core..

Given that JANDC is designed to work out-of-the-box with EF Core and these examples require to deviate from that, I would propose to leave these cases out of the picture.

maurei avatar Sep 06 '22 19:09 maurei

C# allows to have different nullabilities for the getter and setter of a property, as described at https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/attributes/nullable-analysis#preconditions-allownull-and-disallownull. We could support that in OAS by emitting different models for read/write verbs. But I'd consider that a nice-to-have, probably best postponed until asked for.

However I disagree with leaving [Required] string? out of the picture. The use of EF Core is just a (pluggable) implementation detail that shouldn't drive the meaning of the exposed model. Even so, it's easy enough to deviate in EF Core by having [Required] with .Required(false) in mappings. My last comment describes the use case for doing that.

Explicitly adding [Required] wins over any C# nullability state when MSV is on, and I believe that should be the case when MSV is off as well. Because turning on MSV afterwards should not change the OAS model. Therefore I think H6 is correct, after all. In a broader sense, .NET attributes have always overridden/tweaked what's expressed at the C# language level, so it makes sense to me to adhere to that here as well. Examples: ComVisible, PrincipalPermission, MaybeNull, Out, DllExport, DebuggerVisible.

bkoelman avatar Sep 06 '22 21:09 bkoelman