Use native NullabilityInfoContext #1167
Conversation
Codecov Report
@@ Coverage Diff @@
## openapi #1167 +/- ##
===========================================
+ Coverage 92.24% 92.27% +0.03%
===========================================
Files 302 301 -1
Lines 8768 8756 -12
===========================================
- Hits 8088 8080 -8
+ Misses 680 676 -4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
I'm in favor of relying on NullabilityInfoContext eventually, but I think tests should be part of this PR to ensure correctness today.
Resolving the nullability state turns out to be quite complicated when generics are involved. And I found that .NET 6 contains quite some bugs, which were recently fixed for .NET 7.
For example, simple cases like the following fail to produce the correct result on .NET 6:
#nullable enable
class Customer : Identifiable<string>
{
}
PropertyInfo property = typeof(Customer).GetProperty("Id")!;
NullabilityInfoContext nullContext = new NullabilityInfoContext();
NullabilityInfo nullability = nullContext.Create(property);
Console.WriteLine(nullability.ReadState); // prints Nullable| }; | ||
| NullabilityInfo nullabilityInfo = NullabilityInfoContext.Create(source.Property); | ||
|
|
||
| return nullabilityInfo.ReadState == NullabilityState.Nullable && !hasRequiredAttribute; |
There was a problem hiding this comment.
I think the first part should be nullabilityInfo.ReadState != NullabilityState.NotNull, to account for Unknown (NRT off)
There was a problem hiding this comment.
Unlike JsonApiSchemaGenerator.IsDataPropertyNullable, here properties can indeed live in types that have NRT disabled, so this is a bug. This slipped through because at this point there aren't any tests that check the behaviour for NRT disabled input. I have a PR lined up for #1111 where these scenarios are extensively tested
I agree that it would be better to have these test cases worked out extensively before changing this mechanism. I will put this PR on hold for now.
|
|
||
| return typeCategory == TypeCategory.NullableReferenceType; | ||
| NullabilityInfo nullabilityInfo = NullabilityInfoContext.Create(dataProperty); | ||
| return nullabilityInfo.ReadState == NullabilityState.Nullable; |
There was a problem hiding this comment.
Should probably be:
return nullabilityInfo.ReadState != NullabilityState.NotNull;to handle the case where the state is Unknown (NRT off). Although I wonder if that situation can occur, assuming the Data property is defined inside JADNC.
There was a problem hiding this comment.
Although I wonder if that situation can occur, assuming the Data property is defined inside JADNC.
Correct! The nullability info of property it checks are within types that are internal to JADNC, where NRT is guaranteed to be enabled.
Upon looking at it again, however, I realised there is an even easier check we can perform. This resulted in removing this block code entirely.
| @@ -224,6 +225,7 @@ private static bool IsDataPropertyNullable(Type type) | |||
| throw new UnreachableCodeException(); | |||
| } | |||
|
|
|||
| return dataProperty.GetTypeCategory() == TypeCategory.NullableReferenceType; | |||
| NullabilityInfo nullabilityInfo = NullabilityInfoContext.Create(dataProperty); | |||
| return nullabilityInfo.ReadState == NullabilityState.Nullable; | |||
There was a problem hiding this comment.
Same remark as for JsonApiSchemaGenerator.IsDataPropertyNullable applies here.
There was a problem hiding this comment.
Same answer applies as for JsonApiSchemaGenerator.IsDataPropertyNullable. The argument type is always a relationship schema type managed by JADNC.
| where TData : ResourceIdentifierObject | ||
| // ReSharper disable once RedundantNotNullConstraint | ||
| // See ReSharper bug: https://youtrack.jetbrains.com/issue/RSRP-489137/False-positive-on-RedundantNotNullConstraint | ||
| where TData : notnull, ResourceIdentifierObject |
There was a problem hiding this comment.
I'm not seeing any tests fail when removing the notnull constraint. Why is it needed here?
There was a problem hiding this comment.
Initially I had an implementation for JsonApiSchemaGenerator.IsDataPropertyNullable that did depend on this constraint being there. I did something similar to the bug report I did for ReSharper. However now these helper methods behave in an entirely different way, so no longer tests will fail.
Nevertheless, for completeness I figured it would be good to keep the constraints.
However, looking closely at this bug report, I wonder if this is really a bug report in ReSharper, or if ReSharper is actually right and it is a bug in net60.
maurei
force-pushed
the
openapi-nullability-context
branch
from
0cc59db
to
7143a7d
Compare
maurei
force-pushed
the
openapi-nullability-context
branch
from
7143a7d
to
49f9ff2
Compare
Use the native
NullabilityInfoContextfor checking on nullability information rather than relying on Swashbuckles work-around.