Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullable attribute missing for collection type reference #2034

Open
wants to merge 9 commits into
base: release-7.x
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -384,10 +384,12 @@ internal override void WriteEnumMemberElementHeader(IEdmEnumMember member)
/// 7.2.1 Nullable, A Boolean value specifying whether a value is required for the property.
/// </summary>
/// <param name="reference">The Edm type reference.</param>
internal override void WriteNullableAttribute(IEdmTypeReference reference)
/// <param name="parentTypeKind">The parent object's (the property) type kind.
internal override void WriteNullableAttribute(IEdmTypeReference reference, EdmTypeKind parentTypeKind)
{
// The value of $Nullable is one of the Boolean literals true or false. Absence of the member means false.
this.jsonWriter.WriteOptionalProperty("$Nullable", reference.IsNullable, defaultValue: false);
// The value of $Nullable is one of the Boolean literals true or false. Absence of the JSON property means false.
// parentTypeKind gets intentionally ignored since JSON CSDL never writes $Nullable=false
this.jsonWriter.WriteOptionalProperty("$Nullable", reference.IsNullable, defaultValue: false);
}

internal override void WriteTypeDefinitionAttributes(IEdmTypeDefinitionReference reference)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,13 @@ internal virtual void WriteNavigationPropertyBindingsEnd(IEnumerable<IEdmNavigat
// Nothing here
}

internal abstract void WriteNullableAttribute(IEdmTypeReference reference);
/// <summary>
/// 7.2.1 Nullable, A Boolean value specifying whether a value is required for the property.
/// </summary>
/// <param name="reference">The Edm type reference.</param>
/// <param name="parentTypeKind">The parent object's (the property's) type kind.
internal abstract void WriteNullableAttribute(IEdmTypeReference reference, EdmTypeKind parentTypeKind);


internal abstract void WriteTypeDefinitionAttributes(IEdmTypeDefinitionReference reference);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,19 @@ internal override void WriteEnumMemberElementEnd(IEdmEnumMember member)
this.xmlWriter.WriteEndElement();
}

internal override void WriteNullableAttribute(IEdmTypeReference reference)
/// <inheritdoc/>
internal override void WriteNullableAttribute(IEdmTypeReference reference, EdmTypeKind parentTypeKind)
{
this.WriteOptionalAttribute(CsdlConstants.Attribute_Nullable, reference.IsNullable, CsdlConstants.Default_Nullable, EdmValueWriter.BooleanAsXml);
// if the parent type is a collection ensure the XML attribute is always written, even if false.
// see section 7.2.1 Nullable of OData 4.0.1
if (parentTypeKind == EdmTypeKind.Collection)
{
this.WriteRequiredAttribute(CsdlConstants.Attribute_Nullable, reference.IsNullable, EdmValueWriter.BooleanAsXml);
}
else
{
this.WriteOptionalAttribute(CsdlConstants.Attribute_Nullable, reference.IsNullable, CsdlConstants.Default_Nullable, EdmValueWriter.BooleanAsXml);
}
}

internal override void WriteTypeDefinitionAttributes(IEdmTypeDefinitionReference reference)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,10 @@ protected override void ProcessEntityType(IEdmEntityType element)
protected override void ProcessStructuralProperty(IEdmStructuralProperty element)
{
bool inlineType = IsInlineType(element.Type);
this.BeginElement(element, (IEdmStructuralProperty t) => { this.schemaWriter.WriteStructuralPropertyElementHeader(t, inlineType); }, e => { this.ProcessFacets(e.Type, inlineType); });
this.BeginElement(element,
(IEdmStructuralProperty t) => { this.schemaWriter.WriteStructuralPropertyElementHeader(t, inlineType); },
e => { this.ProcessFacets(e.Type, inlineType, true); }
);
if (!inlineType)
{
VisitTypeReference(element.Type);
Expand Down Expand Up @@ -243,7 +246,7 @@ protected override void ProcessTypeDefinition(IEdmTypeDefinition element)
protected override void ProcessTerm(IEdmTerm term)
{
bool inlineType = term.Type != null && IsInlineType(term.Type);
this.BeginElement(term, (IEdmTerm t) => { this.schemaWriter.WriteTermElementHeader(t, inlineType); }, e => { this.ProcessFacets(e.Type, inlineType); });
this.BeginElement(term, (IEdmTerm t) => { this.schemaWriter.WriteTermElementHeader(t, inlineType); }, e => { this.ProcessFacets(e.Type, inlineType, false); });
if (!inlineType)
{
if (term.Type != null)
Expand Down Expand Up @@ -271,7 +274,7 @@ protected override void ProcessOperationParameter(IEdmOperationParameter element
this.BeginElement(
element,
(IEdmOperationParameter t) => { this.schemaWriter.WriteOperationParameterElementHeader(t, inlineType); },
e => { this.ProcessFacets(e.Type, inlineType); });
e => { this.ProcessFacets(e.Type, inlineType, false); });
if (!inlineType)
{
VisitTypeReference(element.Type);
Expand Down Expand Up @@ -303,7 +306,7 @@ protected override void ProcessOperationReturn(IEdmOperationReturn operationRetu
if (inlineType)
{
this.schemaWriter.WriteTypeAttribute(type);
this.ProcessFacets(type, true /*inlineType*/);
this.ProcessFacets(type, true /*inlineType*/, false);
}
else
{
Expand All @@ -319,7 +322,7 @@ protected override void ProcessCollectionType(IEdmCollectionType element)
this.BeginElement(
element,
(IEdmCollectionType t) => this.schemaWriter.WriteCollectionTypeElementHeader(t, inlineType),
e => this.ProcessFacets(e.ElementType, inlineType));
e => this.ProcessFacets(e.ElementType, inlineType, false));
if (!inlineType)
{
VisitTypeReference(element.ElementType);
Expand Down Expand Up @@ -439,7 +442,7 @@ protected override void ProcessIsTypeExpression(IEdmIsTypeExpression expression)

if (this.isXml)
{
this.BeginElement(expression, (IEdmIsTypeExpression t) => { this.schemaWriter.WriteIsTypeExpressionElementHeader(t, inlineType); }, e => { this.ProcessFacets(e.Type, inlineType); });
this.BeginElement(expression, (IEdmIsTypeExpression t) => { this.schemaWriter.WriteIsTypeExpressionElementHeader(t, inlineType); }, e => { this.ProcessFacets(e.Type, inlineType, false); });
if (!inlineType)
{
VisitTypeReference(expression.Type);
Expand All @@ -453,7 +456,7 @@ protected override void ProcessIsTypeExpression(IEdmIsTypeExpression expression)
this.BeginElement(expression, (IEdmIsTypeExpression t) => { this.schemaWriter.WriteIsTypeExpressionElementHeader(t, inlineType); });
this.VisitExpression(expression.Operand);
this.schemaWriter.WriteIsOfExpressionType(expression, inlineType);
this.ProcessFacets(expression.Type, inlineType);
this.ProcessFacets(expression.Type, inlineType, false);
this.EndElement(expression);
}
}
Expand Down Expand Up @@ -533,7 +536,7 @@ protected override void ProcessCastExpression(IEdmCastExpression expression)

if (this.isXml)
{
this.BeginElement(expression, (IEdmCastExpression t) => { this.schemaWriter.WriteCastExpressionElementHeader(t, inlineType); }, e => { this.ProcessFacets(e.Type, inlineType); });
this.BeginElement(expression, (IEdmCastExpression t) => { this.schemaWriter.WriteCastExpressionElementHeader(t, inlineType); }, e => { this.ProcessFacets(e.Type, inlineType, false); });
if (!inlineType)
{
VisitTypeReference(expression.Type);
Expand All @@ -549,7 +552,7 @@ protected override void ProcessCastExpression(IEdmCastExpression expression)
this.VisitExpression(expression.Operand);

this.schemaWriter.WriteCastExpressionType(expression, inlineType);
this.ProcessFacets(expression.Type, inlineType);
this.ProcessFacets(expression.Type, inlineType, false);

this.EndElement(expression, t => this.schemaWriter.WriteCastExpressionElementEnd(t, inlineType));
}
Expand Down Expand Up @@ -653,7 +656,7 @@ private void ProcessReferentialConstraint(IEdmReferentialConstraint element)
}
}

private void ProcessFacets(IEdmTypeReference element, bool inlineType)
private void ProcessFacets(IEdmTypeReference element, bool inlineType, bool nullableAttributeRequiredForCollection)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullableAttributeRequiredForCollection

So this gets set true for structural property facets, and false otherwise? Can we just call this something like "isStructuralPropertyDeclaration"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the call "chain" is
EdmModelCsdlSerializationVisitor.{ProcessStructuralProperty|ProcessTerm|ProcessCollectionType|...)
EdmModelCsdlSerializationVisitor.ProcessFacets ->
EdmModelCsdlSchemaWriter.WriteNullableAttribute

and EdmModelCsdlSchemaWriter is abstract (one concrete subclass for XML, one for JSON)

And yes, it is true that only ProcessStructuralProperty is passing in true. But that doesn't describe the behavior of ProcessFacets which is : instruct the writer to write the property for a collection type (-reference) even if it is false (which otherwise dosn't get written). The name is based on what the method does, not on how and when it is called.

But I don't have a strong opinion. Happy to change it if you prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss -- my concern is that, as I understand it, a component that should be format-agnostic (EdmModelCsdlSerializationVistor) would be making the decision of whether to write the nullable attribute or not based on the format (write for xml, not for json). ideally, we wouldn't be making format-specific decisions in these common classes, they would all be made in the format-specific implementations.

{
if (element != null)
{
Expand All @@ -665,15 +668,16 @@ private void ProcessFacets(IEdmTypeReference element, bool inlineType)

if (inlineType)
{
if (element.TypeKind() == EdmTypeKind.Collection)
var typeKind = element.TypeKind();
if (typeKind == EdmTypeKind.Collection)
{
IEdmCollectionTypeReference collectionElement = element.AsCollection();
this.schemaWriter.WriteNullableAttribute(collectionElement.CollectionDefinition().ElementType);
VisitTypeReference(collectionElement.CollectionDefinition().ElementType);
var elementType = element.AsCollection().CollectionDefinition().ElementType;
this.schemaWriter.WriteNullableAttribute(elementType, typeKind);
VisitTypeReference(elementType);
}
else
{
this.schemaWriter.WriteNullableAttribute(element);
this.schemaWriter.WriteNullableAttribute(element, typeKind);
VisitTypeReference(element);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,8 @@ public void WriteNavigationPropertyInComplexType()
},
""Addresses"": {
""$Collection"": true,
""$Type"": ""DefaultNs.Address""
""$Type"": ""DefaultNs.Address"",
Copy link
Member

@mikepizzo mikepizzo Aug 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nullable facet is required for collections in XML, but I don't think it's required in JSON.

Which means our factoring is a bit off -- we shouldn't have code in the serializer to determine whether the null needs to be written. Instead, we should pass down the information that lets the XML serializer decide whether or not to write the null value (i.e., whether or not we are writing a structural property).

""$Nullable"": false
}
},
""City"": {
Expand Down Expand Up @@ -745,7 +746,7 @@ public void SetNavigationPropertyPartnerTest()
},
""ComplexProp"": {
""$Collection"": true,
""$Type"": ""NS.ComplexType1""
""$Type"": ""NS.ComplexType1"",
},
""OuterNavA"": {
""$Kind"": ""NavigationProperty"",
Expand Down Expand Up @@ -1421,7 +1422,7 @@ public void ShouldWriteEdmPathTypeProperty()
"<edmx:DataServices>" +
"<Schema Namespace=\"NS\" xmlns=\"http://docs.oasis-open.org/odata/ns/edm\">" +
"<ComplexType Name=\"SelectType\">" +
"<Property Name=\"DefaultSelect\" Type=\"Collection(Edm.PropertyPath)\" />" +
"<Property Name=\"DefaultSelect\" Type=\"Collection(Edm.PropertyPath)\" Nullable=\"true\" />" +
"<Property Name=\"DefaultHidden\" Type=\"Collection(Edm.NavigationPropertyPath)\" Nullable=\"false\" />" +
"</ComplexType>" +
"<Term Name=\"MyTerm\" Type=\"NS.SelectType\" />" +
Expand All @@ -1442,7 +1443,8 @@ public void ShouldWriteEdmPathTypeProperty()
},
""DefaultHidden"": {
""$Collection"": true,
""$Type"": ""Edm.NavigationPropertyPath""
""$Type"": ""Edm.NavigationPropertyPath"",
""$Nullable"": false
}
},
""MyTerm"": {
Expand Down Expand Up @@ -1604,8 +1606,8 @@ public void CanWriteEdmEntityTypeWithCollectionAbstractTypeButValidationFailed()
"<PropertyRef Name=\"Id\" />" +
"</Key>" +
"<Property Name=\"Id\" Type=\"Edm.Int32\" Nullable=\"false\" />" +
"<Property Name=\"Primitive\" Type=\"Collection(Edm.PrimitiveType)\" />" +
"<Property Name=\"Complex\" Type=\"Collection(Edm.ComplexType)\" />" +
"<Property Name=\"Primitive\" Type=\"Collection(Edm.PrimitiveType)\" Nullable=\"true\" />" +
"<Property Name=\"Complex\" Type=\"Collection(Edm.ComplexType)\" Nullable=\"true\" />" +
"</EntityType>" +
"</Schema>" +
"</edmx:DataServices>" +
Expand Down Expand Up @@ -2377,7 +2379,7 @@ public void CanWriteEdmModelWithUntypedProperty()
"</Key>" +
"<Property Name=\"Id\" Type=\"Edm.String\" />" +
"<Property Name=\"Value\" Type=\"Edm.Untyped\" />" +
"<Property Name=\"Data\" Type=\"Collection(Edm.Untyped)\" />" +
"<Property Name=\"Data\" Type=\"Collection(Edm.Untyped)\" Nullable=\"true\" />" +
"</EntityType>" +
"</Schema>" +
"</edmx:DataServices>" +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public void VerifyComplexTypeWithNavigationPropertiesWrittenCorrectly()
// Act & Assert for XML
VisitAndVerifyXml(v => v.VisitSchemaType(complexType),
@"<ComplexType Name=""Address"">
<Property Name=""Street"" Type=""Collection(Edm.String)"" MaxLength=""42"" Unicode=""false"" />
<Property Name=""Street"" Type=""Collection(Edm.String)"" Nullable=""true"" MaxLength=""42"" Unicode=""false"" />
<NavigationProperty Name=""City"" Type=""NS.City"" Nullable=""false"" ContainsTarget=""true"">
<OnDelete Action=""Cascade"" />
</NavigationProperty>
Expand Down Expand Up @@ -216,7 +216,8 @@ public void VerifyEntityTypeWithNavigationPropertiesWrittenCorrectly()
""$Kind"": ""EntityType"",
""Locations"": {
""$Collection"": true,
""$Type"": ""NS.Address""
""$Type"": ""NS.Address"",
""$Nullable"": false
},
""Orders"": {
""$Kind"": ""NavigationProperty"",
Expand Down Expand Up @@ -2015,6 +2016,53 @@ public void VerifyAnnotationWithRecordExpressionWrittenCorrectly()
}
#endregion

#region Collection Type References
[Fact]
public void VerifyCollectionTypeReferencesWrittenCorrectly()
{
// see https://github.com/OData/odata.net/issues/2028
// Arrange
EdmComplexType structuredType = new EdmComplexType("NS", "Customer");
structuredType.AddStructuralProperty("test01", EdmCoreModel.GetCollection(EdmCoreModel.Instance.GetDecimal(true)));
structuredType.AddStructuralProperty("test02", EdmCoreModel.GetCollection(EdmCoreModel.Instance.GetDecimal(false)));
structuredType.AddStructuralProperty("test03", EdmCoreModel.Instance.GetDecimal(true));
structuredType.AddStructuralProperty("test04", EdmCoreModel.Instance.GetDecimal(false));

// Act & Assert for XML
VisitAndVerifyXml(v => v.VisitSchemaType(structuredType), @"<ComplexType Name=""Customer"">
<Property Name=""test01"" Type=""Collection(Edm.Decimal)"" Nullable=""true"" />
<Property Name=""test02"" Type=""Collection(Edm.Decimal)"" Nullable=""false"" />
<Property Name=""test03"" Type=""Edm.Decimal"" />
<Property Name=""test04"" Type=""Edm.Decimal"" Nullable=""false"" />
</ComplexType>"
);

// Act & Assert for JSON
VisitAndVerifyJson(v => v.VisitSchemaType(structuredType), @"{
""Customer"": {
""$Kind"": ""ComplexType"",
""test01"": {
""$Collection"": true,
""$Type"": ""Edm.Decimal"",
""$Nullable"": true
},
""test02"": {
""$Collection"": true,
""$Type"": ""Edm.Decimal"",
""$Nullable"": false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be written in this case for json csdl, right?

},
""test03"": {
""$Type"": ""Edm.Decimal"",
""$Nullable"": true
},
""test04"": {
""$Type"": ""Edm.Decimal""
}
}
}");
}
#endregion

internal void VisitAndVerifyXml(Action<EdmModelCsdlSerializationVisitor> testAction, string expected, bool indent = true)
{
XmlWriter xmlWriter;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public void TestAlternateKeysVocabularyModel()
const string expectedText = @"<?xml version=""1.0"" encoding=""utf-16""?>
<Schema Namespace=""OData.Community.Keys.V1"" Alias=""Keys"" xmlns=""http://docs.oasis-open.org/odata/ns/edm"">
<ComplexType Name=""AlternateKey"">
<Property Name=""Key"" Type=""Collection(Keys.PropertyRef)"">
<Property Name=""Key"" Type=""Collection(Keys.PropertyRef)"" Nullable=""true"">
<Annotation Term=""Core.Description"" String=""The set of properties that make up this key"" />
</Property>
</ComplexType>
Expand Down
Loading