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.

Sign up for GitHub

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
Open

Nullable attribute missing for collection type reference #2034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
94de6d3
add parameter to control if Nullable gets written always
chrisspre Mar 23, 2021
0fda561
added Nullable=""true"" to existing test's data.
chrisspre Mar 24, 2021
74b4a94
ensure only colelction property require teh nullable attribute
chrisspre Mar 24, 2021
8be4c66
fix expected JSON CSDL values
chrisspre Mar 29, 2021
23de419
fixed EdmLibTests.FunctionalTests.CsdlSerializingTests
chrisspre Mar 30, 2021
3d02769
move login intto writer.
chrisspre Aug 11, 2021
2ae59f6
move logic to writer
chrisspre Aug 11, 2021
baa96d6
Merge branch 'nullable-collection-csdl' of https://github.com/OData/o…
chrisspre Aug 11, 2021
93d30f3
fix test case for JSON with nullable collection
chrisspre Aug 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSchemaJsonWriter.cs
View file
Open in desktop
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
8 changes: 7 additions & 1 deletion src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSchemaWriter.cs
View file
Open in desktop
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
14 changes: 12 additions & 2 deletions src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSchemaXmlWriter.cs
View file
Open in desktop
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
34 changes: 19 additions & 15 deletions src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSerializationVisitor.cs
View file
Open in desktop
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
16 changes: 9 additions & 7 deletions test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlWriterTests.cs
View file
Open in desktop
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 •
edited
Loading

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
52 changes: 50 additions & 2 deletions ...sts/Microsoft.OData.Edm.Tests/Csdl/Serialization/EdmModelCsdlSerializationVisitorTests.cs
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion test/FunctionalTests/Microsoft.OData.Edm.Tests/Vocabularies/AlternateKeysVocabularyTests.cs
View file
Open in desktop
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