Nullable attribute missing for collection type reference #2034

chrisspre · 2021-03-23T17:45:09Z

Issues

*This pull request fixes issue #2028

Description

Added parameter to WriteNullableAttribute to indicate if the property should be written always or depending on the default value.

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

mikepizzo

This PR makes the nullable attribute required for collections in both XML and JSON. However, I think the nullable attribute requirement only applies to XML -- I believe it is valid in JSON to have a collection valued property that does not specify the nullable attribute.

This means that the factoring is a bit off. Rather than deciding in the common serialization classes whether the nullable attribute is required, we should pass down the information required for the xml serializer to decide whether or not the null should be written.

mikepizzo · 2021-08-10T18:03:18Z

test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlWriterTests.cs

@@ -311,7 +311,8 @@ public void WriteNavigationPropertyInComplexType()
      },
      ""Addresses"": {
        ""$Collection"": true,
-        ""$Type"": ""DefaultNs.Address""
+        ""$Type"": ""DefaultNs.Address"",


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).

mikepizzo · 2021-08-10T18:09:30Z

src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSerializationVisitor.cs

@@ -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)


nullableAttributeRequiredForCollection

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

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.

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.

habbes · 2021-08-11T12:48:05Z

src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSchemaJsonWriter.cs

@@ -384,10 +384,18 @@ 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="alwaysWrite">Specifies if the attribute is always written or not if the value is the default value.
+        internal override void WriteNullableAttribute(IEdmTypeReference reference, bool alwaysWrite)


When is the alwaysWrite parameter ever set to true and why? what determines whether it should be set true or false?

It gets called from EdmModelCsdlSerializationVisitor.ProcessFacets

And that method tests if the property is of a collection type and if the serializer wants to always serialize the nullable attributes for collection (which is true for XML, false for JSON)

…data.net into nullable-collection-csdl

pull-request-quantifier-deprecated · 2021-08-11T23:34:27Z

This PR has 134 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Medium
Size       : +88 -46
Percentile : 46.8%

Total files changed: 9

Change summary by file extension:
.cs : +88 -46

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

mikepizzo · 2021-08-12T03:50:44Z

...lTests/Microsoft.OData.Edm.Tests/Csdl/Serialization/EdmModelCsdlSerializationVisitorTests.cs

+    ""test02"": {
+      ""$Collection"": true,
+      ""$Type"": ""Edm.Decimal"",
+      ""$Nullable"": false


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

add parameter to control if Nullable gets written always

94de6d3

chrisspre changed the title ~~add parameter to control if Nullable gets written always~~ Added parameter to control if Nullable attribute is always written. Mar 23, 2021

chrisspre changed the title ~~Added parameter to control if Nullable attribute is always written.~~ Nullable attribute missing for collection type reference Mar 24, 2021

added Nullable=""true"" to existing test's data.

0fda561

chrisspre marked this pull request as ready for review March 24, 2021 15:24

ensure only colelction property require teh nullable attribute

74b4a94

chrisspre requested review from mikepizzo and xuzhg March 24, 2021 16:08

chrisspre mentioned this pull request Mar 25, 2021

Nullable attribute missing for collection type reference. #2028

Open

chrisspre added 2 commits March 29, 2021 16:18

fix expected JSON CSDL values

8be4c66

fixed EdmLibTests.FunctionalTests.CsdlSerializingTests

23de419

chrisspre added the Ready for review Use this label if a pull request is ready to be reviewed label Mar 30, 2021

KenitoInc requested review from KenitoInc, ElizabethOkerio, gathogojr, habbes, marabooy and Sreejithpin July 14, 2021 08:20

mikepizzo added this to the 7.9.1 milestone Aug 10, 2021

mikepizzo requested changes Aug 10, 2021

View reviewed changes

mikepizzo reviewed Aug 10, 2021

View reviewed changes

habbes reviewed Aug 11, 2021

View reviewed changes

move login intto writer.

3d02769

pull-request-quantifier-deprecated bot added the Medium label Aug 11, 2021

chrisspre added 3 commits August 11, 2021 16:31

move logic to writer

2ae59f6

Merge branch 'nullable-collection-csdl' of https://github.com/OData/o…

baa96d6

…data.net into nullable-collection-csdl

fix test case for JSON with nullable collection

93d30f3

mikepizzo reviewed Aug 12, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nullable attribute missing for collection type reference #2034

Nullable attribute missing for collection type reference #2034

chrisspre commented Mar 23, 2021 •

edited by corranrogue9

Loading

mikepizzo left a comment

mikepizzo Aug 10, 2021 •

edited

Loading

mikepizzo Aug 10, 2021

chrisspre Aug 11, 2021

mikepizzo Aug 12, 2021

habbes Aug 11, 2021

chrisspre Aug 11, 2021

pull-request-quantifier-deprecated bot commented Aug 11, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

mikepizzo Aug 12, 2021

Nullable attribute missing for collection type reference #2034

Are you sure you want to change the base?

Nullable attribute missing for collection type reference #2034

Conversation

chrisspre commented Mar 23, 2021 • edited by corranrogue9 Loading

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

mikepizzo left a comment

Choose a reason for hiding this comment

mikepizzo Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

mikepizzo Aug 10, 2021

Choose a reason for hiding this comment

chrisspre Aug 11, 2021

Choose a reason for hiding this comment

mikepizzo Aug 12, 2021

Choose a reason for hiding this comment

habbes Aug 11, 2021

Choose a reason for hiding this comment

chrisspre Aug 11, 2021

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Aug 11, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

mikepizzo Aug 12, 2021

Choose a reason for hiding this comment

chrisspre commented Mar 23, 2021 •

edited by corranrogue9

Loading

mikepizzo Aug 10, 2021 •

edited

Loading