Sam,
As I have been hacking away at the impact of NonNullable Reference types in the OData Convention builder (I am close to making it work), I think I have found a bug in ReconfigureEntityTypesAsComplexType().
The situation I am seeing is that as GetEdmModel is doing its work, it builds up all of the types and code of this form:
public class Employee
{
// Other properties elided
public Address HomeAddress { get; set; } = new Address();
}
public class Address
{
// Other properties elided
public string State { get; set; } = string.Empty;
public string IgnoreThis { get; set; } = "IgnoreThis";
}
It will initially create through MapTypes the HomeAddress as a NavigationPropertyConfiguration and set its Multiplicity. After the initial type mapping it then calls RediscoverComplexTypes to detect the Navigation types that are misconfigured (e.g. Address has no Key) and need to really be Complex type.
The code then flows into ReconfigureEntityTypesAsComplexType until it gets to this comment:
// go through all structural types to remove all properties defined by this mis-configed type.
Where it iterates over the structural types that need to be “patched”, in this case Employee which has a Navigation type property HomeAddress of type Address.
It then checks on the Multiplicity to determine what it should do.
It first checks if EdmMultiplicity.Many and handles that as a Collection by calling AddCollectionProperty.
Else, it treats all other values of Multiplicity as a Complex property.
This is where the bug occurs. When calling AddComplexProperty it will set the Nullability property based on the underlying ClrType, which is correct for EdmMultiplicity.ZeroOrOne but is not correct for EdmMultiplicity.One as that means it cannot be zero, thus Nullability must == false and the code is setting it to be true. A bug!
Looking at the code that I changed to address this (restyling into a switch instead of a long if-then-else chain):
switch (propertyToBeRemoved.Multiplicity)
{
case EdmMultiplicity.Many:
propertyConfiguration =
structuralToBePatched.AddCollectionProperty(propertyToBeRemoved.PropertyInfo);
break;
case EdmMultiplicity.One:
propertyConfiguration =
structuralToBePatched.AddComplexProperty(propertyToBeRemoved.PropertyInfo);
(propertyConfiguration as ComplexPropertyConfiguration).NullableProperty = false;
break;
case EdmMultiplicity.ZeroOrOne:
case EdmMultiplicity.Unknown:
default:
propertyConfiguration =
structuralToBePatched.AddComplexProperty(propertyToBeRemoved.PropertyInfo);
break;
}
I found this bug in my code where I have a test case where HomeAddress is set to be a NonNullable type, but I it can also be reproduced in the shipping version by simply adding the Required Attribute to the HomeAddress property. I will see if I can create a reproducable test case in the existing test repository.
xuzhg
commented
3 years ago
From: David Robinson david.robinson@live.com
Sam, As I have been hacking away at the impact of NonNullable Reference types in the OData Convention builder (I am close to making it work), I think I have found a bug in ReconfigureEntityTypesAsComplexType().
The situation I am seeing is that as GetEdmModel is doing its work, it builds up all of the types and code of this form:
It will initially create through MapTypes the HomeAddress as a NavigationPropertyConfiguration and set its Multiplicity. After the initial type mapping it then calls RediscoverComplexTypes to detect the Navigation types that are misconfigured (e.g. Address has no Key) and need to really be Complex type.
The code then flows into ReconfigureEntityTypesAsComplexType until it gets to this comment: // go through all structural types to remove all properties defined by this mis-configed type. Where it iterates over the structural types that need to be “patched”, in this case Employee which has a Navigation type property HomeAddress of type Address.
It then checks on the Multiplicity to determine what it should do. It first checks if EdmMultiplicity.Many and handles that as a Collection by calling AddCollectionProperty. Else, it treats all other values of Multiplicity as a Complex property.
This is where the bug occurs. When calling AddComplexProperty it will set the Nullability property based on the underlying ClrType, which is correct for EdmMultiplicity.ZeroOrOne but is not correct for EdmMultiplicity.One as that means it cannot be zero, thus Nullability must == false and the code is setting it to be true. A bug!
Looking at the code that I changed to address this (restyling into a switch instead of a long if-then-else chain):
I found this bug in my code where I have a test case where HomeAddress is set to be a NonNullable type, but I it can also be reproduced in the shipping version by simply adding the Required Attribute to the HomeAddress property. I will see if I can create a reproducable test case in the existing test repository.