Omit null property value in JSON response #1106

biaol-odata · 2018-03-06T17:48:08Z

Issues

This pull request introduces omitNullValues controlled by omit-values=nulls Preference header per OData v4.01.

Description

Current changes include the following areas: *

serialize JSON object to response with omit null values option according to Preference-Applied header's parameter value.
de-serialize JSON object from response with omitted null values restored per the option used according to the Preference-Applied header's parameter value.
has no effects on request payload and delta payload.

Based on PR #944 from kosinsky.

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.

kosinsky · 2018-03-06T21:08:32Z

src/Microsoft.OData.Core/ODataMessageReaderSettings.cs

+        /// <summary>
+        /// True if null values are omitted, per the omit-values parameter in Preference header of the message.
+        /// </summary>
+        internal bool NullValuesOmitted { get; set; }


internal [](start = 8, length = 8)

Should we make that public to allow tests in projects using ODL simple injection? Injecting via PreferenceAppliedHeader() can be a bit complicated (to much stuff to mock) #Closed

I think the NullValuesOmitted is caused by Preference-Applied header parameter value in the specific request. ODataPreferenceHeader(responseMessage).OmitValus = null should do the trick.
If it were the case for DependencyInjection, we would need to do it with bool flag for convenience, but it is not the case here, right? #Closed

kosinsky · 2018-03-06T21:17:25Z

...ctionalTests/Tests/DataOData/Tests/OData.Writer.Tests/JsonLight/JsonLightEntryWriterTests.cs

+                this.WriterTestConfigurationProvider.JsonLightFormatConfigurationsWithIndent,
+                (testDescriptor, testConfiguration) =>
+                {
+                    testConfiguration.MessageWriterSettings.OmitNullValues = true;


Could you add EndToEnd test that uses responseMessageWithModel.PreferenceAppliedHeader().OmitValues= "nulls"; to check e2e? Something like tests for AnnotationFilter in WritePayloadTests or InstanceAnnotationWriterIntegrationTests #Closed

Added e2e test WriteJsonWittoutModelTests::ExpandedCustomerEntryOmitNullValuesTest with proper header and null values for various cases. #Closed

kosinsky · 2018-03-06T21:33:54Z

        if (this.JsonLightOutputContext.MessageWriterSettings.Validations != ValidationKinds.None)

For performance reasons consider adding following optimization:

        ODataValue value = property.ODataValue;

        // If no validation is required, we don't need property serialization info and could try to write null property right away
        // For very wide and sparse outputs it allows to avoid a lot of dictionary lookups
        if (!this.MessageWriterSettings.ThrowIfTypeConflictsWithMetadata && omitNullValues)
        {
            if (value is ODataNullValue || value == null)
            {
                return;
            }
        } #Closed

Refers to: src/Microsoft.OData.Core/JsonLight/ODataJsonLightPropertySerializer.cs:192 in df084b6. [](commit_id = df084b6, deletion_comment = False)

biaol-odata · 2018-03-07T22:17:36Z

...e.Tests/IntegrationTests/Reader/JsonLight/PropertyAndValueJsonLightReaderIntegrationTests.cs

+                            entries.Add(reader.Item as ODataResource);
+                            break;
+                        case ODataReaderState.NestedResourceInfoStart:
+                            navigationLink = (ODataNestedResourceInfo)reader.Item;


rename navigationLink to "nestedResourceInfo". #Closed

Note that "navigationLink" is still used for nestedResourceInfo in a number of places

In reply to: 173003599 [](ancestors = 173003599)

I think you are looking at some original test cases that are not changed by this PR. Since you have spotted them, and it makes sense to renaming it, I have updated the names to 'nestedResourceInfo'.

In reply to: 233550345 [](ancestors = 233550345,173003599)

biaol-odata · 2018-03-07T22:20:10Z

...e.Tests/IntegrationTests/Reader/JsonLight/PropertyAndValueJsonLightReaderIntegrationTests.cs

@@ -353,6 +367,64 @@ public void ReadingTypeDefinitionPayloadJsonLight()
            address.Properties.FirstOrDefault(s => string.Equals(s.Name, "CountryRegion", StringComparison.OrdinalIgnoreCase)).Value.Should().Be("China");
        }

+        [Fact]
+        public void ReadingTypeDefinitionPayloadJsonLightWithOmittedNullValues()


add a comparable test case of same payload with omitedNullValue = false, expecting different outcome. #Closed

Added false case for comparison for all new tests as well (omittedNullValue=false should be the default case for all old tests).

In reply to: 173004227 [](ancestors = 173004227)

biaol-odata · 2018-03-07T22:37:33Z

...e.Tests/IntegrationTests/Reader/JsonLight/PropertyAndValueJsonLightReaderIntegrationTests.cs

+        }
+
+        [Fact]
+        public void ReadingPropertyInTopLevelOrInComplexTypeShouldRestoreOmittedNullValuesWithSelectedPropertiesEntireSubTree()


Add a test case for dynamic property selected, and it is not in the payload, should be materialized as null.

Add unknown properties to context URI in test cases to verify unknown properties are ignored during deserialization.
Dynamic properties don't show up in context URI in response, therefore omitted one are not materialized as nulls.

In reply to: 173008510 [](ancestors = 173008510)

I need to look at the code, but this comment seems wrong. Dynamic properties do show up in ContextUrl if explicitly selected.

In reply to: 174266340 [](ancestors = 174266340,173008510)

biaol-odata · 2018-03-07T22:54:00Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightReader.cs

+                            // resource properties read and navigation properties read.
+                            if (!resource.Properties.Any(
+                                p => p.Name.Equals(currentProperty.Name, StringComparison.Ordinal))
+                                && !resourceState.NavigationPropertiesRead.Contains(currentProperty.Name))


Make sure we have test case to cover complex type and expanded navigation property. #Resolved

Already have coverage for complex type.
Added coverage for navigation link in e2e test ExpandedCustomerEntryOmitNullValuesTest.
Added various test case for expanded navigation property in PropertyAndValueJsonLightReaderIntegrationTests: expanded navigation link should have no content when the link is null (see sample link: http://services.odata.org/V4/TripPinService/People('ronaldmundy')?$expand=Photo)

In reply to: 173012135 [](ancestors = 173012135)

I don't think TripPinService is handling this case correctly; if an expanded single-value navigation property is null, I would expect a null value be returned in the payload, and I would expect to get the null value as nested content when I ready the payload. We should make sure that ODL does support reading and writing a single-value null navigation property, and fix this case to handle a missing explicitly selected null navigation property the same as an explicit null value for a nullable single-value navigation property in the payload. Note that navigation properties (null or otherwise) wouldn't be returned int he properties collection of the resource, but rather as nestedInfo. So the logic to handle any missing null-valued nav props should come before adding the final properties in the endentry case.

In reply to: 174645486 [](ancestors = 174645486,173012135)

expanded single-value navigation property would be a nested resource if it is not null; When it is null, the json reader won't be able to enter the resourcestart state since the name token is not in the reponse payload, therefore won't be able to produce a resource item in a natural way. The omitted nested resource item will be null as expected.

In reply to: 186565783 [](ancestors = 186565783,174645486,173012135)

kosinsky

mikepizzo · 2018-05-07T19:24:48Z

src/Microsoft.OData.Core/Microsoft.OData.Core.txt

@@ -242,6 +242,8 @@ HttpHeaderValueLexer_FailedToReadTokenOrQuotedString=An error occurred when pars
 HttpHeaderValueLexer_InvalidSeparatorAfterQuotedString=An error occurred when parsing the HTTP header '{0}'. The header value '{1}' is incorrect at position '{2}' because '{3}' is not a valid separator after a quoted-string.
 HttpHeaderValueLexer_EndOfFileAfterSeparator=An error occurred when parsing the HTTP header '{0}'. The header value '{1}' is incorrect at position '{2}' because the header value should not end with the separator '{3}'.

+HttpPreferenceHeader_InvalidValueForToken=The value '{0}' is not a valid value for OData token '{1}' in HTTP Prefer or Preference-Applied headers.


Referencing "tokens" isn't very end-user friendly. How about "The value '{0}' is not a valid value for the preference or preference-applied header '{1}'. #Closed

removed because unknown preference header value should just be ignored.

In reply to: 186522770 [](ancestors = 186522770)

mikepizzo · 2018-05-07T19:39:56Z

src/Microsoft.OData.Core/ODataMessageReader.cs

+                // be applied to the reader's setting.
+                this.settings.NullValuesOmitted =
+                    omitValuePreferenceApplied.Equals(ODataConstants.OmitValuesNulls, StringComparison.OrdinalIgnoreCase);
+            }


The check for the omit-nulls preference in the ODataMessageWriter is more concise. Any reason not to use the same code here? #Closed

Updated to be the same as the writer; should be equivalent except that on reader side this flag should be false by default.

In reply to: 186526760 [](ancestors = 186526760)

mikepizzo · 2018-05-07T19:47:01Z

src/Microsoft.OData.Core/ODataPreferenceHeader.cs

+                }
+
+                // No-ops for other preference values, including "defaults" which is in the OData protocol but not implemented yet.
+            }


Should we allow setting of other preference values here, and just ignore later in the stack? Seems odd that this would silently fail to set the value... #Closed

We could, that would require figuring out the value and associated parameters of the omit-values header (see HttpHeaderValueElement c'tor) that we don't have details about or doesn't belong to this task yet. It is typical to silently ignore unrecognizable preference header, and we have inline-comment. Sounds reasonable?

In reply to: 186528628 [](ancestors = 186528628)

mikepizzo · 2018-05-07T19:59:27Z

src/Microsoft.OData.Core/SelectedPropertiesNode.cs

+            Debug.Assert(this.selectedProperties != null, "selectedProperties != null");
+
+            // Get declared properties selected, and filter out unrecognized properties.
+            return this.selectedProperties.Select(structuredType.FindProperty).OfType<IEdmProperty>();


Why are we omitting undefined properties? If a dynamic property is explicitly requested in the select list, I would expect to return a null value for it if it was a null value or omitted in the payload. #Closed

Added GetSelectedDynamicProperties, invoked by ODataJsonLightReader.RestoreOmittedNullValues seperatly after this for open type nodes. Thanks.

In reply to: 186531887 [](ancestors = 186531887)

mikepizzo · 2018-05-07T20:02:38Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightReader.cs

+            // Restore omitted null value only when processing response and when
+            // Preference header omit-values=nulls is specified.
+            if (this.jsonLightInputContext.ReadingResponse
+                && this.jsonLightInputContext.MessageReaderSettings.NullValuesOmitted)


should this be encapsulated in a method on ODataReaderCore, similar to the ShouldOmitNullValues on ODataWriterCore? #Closed

Moved to ODataReaderCore.ShouldRestoreNullValues().

In reply to: 186532720 [](ancestors = 186532720)

mikepizzo · 2018-05-07T21:35:02Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightPropertySerializer.cs

@@ -202,8 +223,6 @@ private bool IsOpenProperty(ODataProperty property)

            WriteInstanceAnnotation(property, isTopLevel, currentPropertyInfo.MetadataType.IsUndeclaredProperty);


this logic should come before the check for null values. Even if the property is null, it can have instance annotations (in fact, a common use case for instance annotations is annotating a null value with why it is null). Please move this before the check for null values, and add a test to verify that instance annotations are correctly written/read for annotations applied to null values, with and without omit-nulls. #Closed

Moved. Thanks.

In reply to: 186558582 [](ancestors = 186558582)

mikepizzo · 2018-05-07T21:49:17Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightPropertySerializer.cs

            {
-                this.WriteNullProperty(property);
+                this.WriteNullProperty(property, omitNullValues);


do you need the new omitNullValues parameter in WriteNullProperty? Why would you get this far if the property was a null value and omitNullValues was true? Is this only for validating the metadata? Can you handle that when you check for isNullValue, and get rid of the new omitNullValues flag to WriteNullProperty? #Closed

The omitNullValues is needed in WriteNullProperty to differentiate delta and others; When we get this far, we knows property is a null but omitNullValues can still be true or false; I think validating metadata is done before this (L215,L217); I believe WriteNullProperty properly encapsulates the logic to write null property, without the omitNullValues flag it wouldn't be able to tell whether null values needs to be serialized (e.g. delata or non-delta).
I have added additional information about the omitNullValues param in pending changes.

In reply to: 186561856 [](ancestors = 186561856)

On L219, if the value is the null value and omitNullValues is true, you would have returned, so you would never reach this line for a null value if omitNullValues is true. Right?

In reply to: 187209557 [](ancestors = 187209557,186561856)

There is a nested if-clause inside L219, code won't return when the if-clause @L221 is not satisfied.

In reply to: 198078564 [](ancestors = 198078564,187209557,186561856)

The nested if clause checks two things:

If this is top level, in which case omitNullValues doesn't apply (it's already ignored in WriteNullProperty)

whether we should check metadata

For #2, I'm not convinced we should skip checking metadata for null values as this would introduce a change in what the writer was able to write whether omit values was true or false:
a) this would allow the writer to write a non-null value and one or more null values for the same property when omitNullValues was true, which should really not be valid.
b) this would allow the writer to write a null value for an undeclared property, even though throwOnUndeclaredProperty was true, when omitNullValues is true

So, we should simplify move the validations in L227 and L229 before the check for null values in L219, and in L219 return for null non-top level elements if omitNullValues is true. Then we can remove the omitNullValues argument from WriteNullProperty, since it will only be called when we really want to write the null value.

In reply to: 209787942 [](ancestors = 209787942,198078564,187209557,186561856)

For performance reason, kosinsky has requested (see his comment at L192) that skipping the validations at L227-229, using check for null values in L219 before hand. Switching the order as you suggest above would contradict that, right?

In reply to: 213515156 [](ancestors = 213515156,209787942,198078564,187209557,186561856)

Quite honestly, throwOnUndeclaredProperty is a carry-over from OData V3. In OData V4 it should always be false which means this check should never have to occur. Unfortunately, the default value is currently false, but we should consider changing it.

I still dislike having the behavior change based on the omit-nulls flag; having the same code throw an exception if omit-nulls is false and succeed if omit-nulls is true is unintuitive; for example, avoiding the duplicateNamesChecker means that the app could succeed in writing the same property twice, once as null and once not as null, with omit-nulls equal to true, and fail with omit-nulls equal to false. Omit-nulls should we a wire optimization, not a change in behavior.

All that said, it's a nice optimization, even if throwOnUndeclaredProperty is false, to avoid the duplicateNameChecker....

In reply to: 213868365 [](ancestors = 213868365,213515156,209787942,198078564,187209557,186561856)

As we discussed with kosinsky, we can ignore the optimization for validation. I have updated the code accordingly including removal of the omitNullValues argument from WriteNullProperty. Thanks.

In reply to: 229533703 [](ancestors = 229533703,213868365,213515156,209787942,198078564,187209557,186561856)

mikepizzo · 2018-05-07T21:51:22Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightPropertySerializer.cs

        private void WriteNullProperty(
-            ODataProperty property)
+            ODataProperty property,
+            bool omitNullValues)


See above. I would think that you can handle all cases of null and omit=null in WriteProperty, and never get to WriteNullProperty for null properties in the omit nulls case. #Closed

For all cases of null and omit=null, code block around L392 is needed for <V7. Am I missing anything?

In reply to: 186562291 [](ancestors = 186562291)

You only call WriteNullProperty in one place, and you have an optimization before calling writeNullProperty that, if the property is null and omitNullValues is true, you return. So this will only ever be called if it is a null value that you really want to write.

In reply to: 187208933 [](ancestors = 187208933,186562291)

omitNullVallues could be either true or false and needs to be passed in. See related comment in caller.

In reply to: 198079167 [](ancestors = 198079167,187208933,186562291)

see comment in caller. I don't see any reason why we can't check this case before calling the method and would have to pass it in.

In reply to: 209788338 [](ancestors = 209788338,198079167,187208933,186562291)

see response in the caller.

In reply to: 213515321 [](ancestors = 213515321,209788338,198079167,187208933,186562291)

mikepizzo · 2018-05-07T21:56:13Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightReader.cs

+                        foreach (IEdmProperty currentProperty in selectedProperties)
+                        {
+                            Debug.Assert(currentProperty.Type != null, "currentProperty.Type != null");
+                            if (!currentProperty.Type.IsNullable || currentProperty.PropertyKind == EdmPropertyKind.Navigation)


|| currentProperty.PropertyKind == EdmPropertyKind.Navigation [](start = 65, length = 61)

this seems suspect. Doesn't that mean that we wouldn't return a null value for a nav prop that was explicitly selected? why did we add the nav properties to the selectedProperties list if we just ignore them here? #Resolved

Yes, see comment inside the if block. I believe nav links (@odata.navigationlink annotation) are added by SelectedPropertiesNodes.GetSelectedNavigationProperties method, it looks like a logical method, additionally it has 10 references of src+tests. Filtering out here during the processing of null value restoration should be a reasonable approach?

Nested nav property which is PropertyKind of EdmPropertyKind.Structual could be restored to nested resource, but first thing to do that is to require json reader to create a resource start srate for a resouce that is not present on the wire. Another natural and simpler approach, which is taken here, to handle such case is don't restore the resource, and client will get the same null ODataResource as result.

In reply to: 186563420 [](ancestors = 186563420)

biaol-odata · 2018-05-14T17:56:56Z

@mikepizzo I just pushed latest commit for CR updates above. As we discussed offline, please check whether it is OK to not emitting the null for nested resource omitted on the wire; otherwise we need to make further change related to the overall json reader that could be hacky. Thanks. #Closed

biaol-odata · 2018-06-01T00:54:07Z

@mikepizzo I have updated with the latest commit to not omit null values for expanded navigation properties. Can you please take a look? Thanks. #Closed

mikepizzo · 2018-06-26T08:27:37Z

src/Microsoft.OData.Core/ODataMessageReader.cs

@@ -169,6 +169,16 @@ public ODataMessageReader(IODataResponseMessage responseMessage, ODataMessageRea
            this.model = model ?? GetModel(this.container);
            this.edmTypeResolver = new EdmTypeReaderResolver(this.model, this.settings.ClientCustomTypeResolver);

+            // Whether null values were omitted in the response, false by default.
+            this.settings.NullValuesOmitted = false;


why not make this an else for the following if? #Closed

Setting the false as the default value, see comment in the line above.

In reply to: 198055075 [](ancestors = 198055075)

in this case, you are always setting the value to false, and then possibly setting it back to true. Why not just set it to true if the preference apply matches, otherwise set it to false, as in:

if (string.Equals(responseMessage.PreferenceAppliedHeader().OmitValues, ODataConstants.OmitValuesNulls, StringComparison.OrdinalIgnoreCase)) { // If the Preference-Applied header's omit-values parameter is present in the response, its value should // be applied to the reader's setting. this.settings.NullValuesOmitted = true; } else { this.settings.NullValuesOmitted = false; }

In reply to: 209783953 [](ancestors = 209783953,198055075)

These two ways are mathematically equivalent. I still slightly incline to initializing it to false first in order to emphasize the fact that preference header omit-values=nulls is required to be set explicitly.

In reply to: 213509847 [](ancestors = 213509847,209783953,198055075)

I still prefer setting it once to twice as it feels cleaner, but I'm not going to force it.

In reply to: 213861831 [](ancestors = 213861831,213509847,209783953,198055075)

mikepizzo · 2018-06-26T08:40:46Z

src/Microsoft.OData.Core/SelectedPropertiesNode.cs

+
+            if (this.selectionType == SelectionType.EntireSubtree || this.hasWildcard)
+            {
+                return structuredType.DeclaredProperties;


DeclaredProperties [](start = 38, length = 18)

DeclaredProperties [](start = 38, length = 18)

This only returns the properties declared directly on the type; you should use structuredType.Properties to include inherited properties. #Closed

by design. For current level only.

In reply to: 198059137 [](ancestors = 198059137)

Why is this by design? If this is a derived type, why are we only restoring properties declared directly on the derived type and not inherited properties?

In reply to: 209788492 [](ancestors = 209788492,198059137)

Updated to use method structuredType.Properties().

In reply to: 213509115 [](ancestors = 213509115,209788492,198059137)

mikepizzo · 2018-06-26T08:49:33Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightReader.cs

+        /// </summary>
+        /// <param name="edmStructuredType">The EDM structural type.</param>
+        /// <returns>All the properties of this type.</returns>
+        private static IEnumerable<IEdmProperty> GetSelfAndBaseTypeProperties(IEdmStructuredType edmStructuredType)


GetSelfAndBaseTypeProperties [](start = 49, length = 28)

Note that you can use the Properties() extension method to get all declared and inherited properties for a structured type. #Closed

OK. Thanks.

In reply to: 198061988 [](ancestors = 198061988)

biaol-odata · 2018-11-07T18:32:40Z

@mikepizzo I have made one more update for the optimization we discussed in office yesterday. Thanks for the suggestion. #Closed

mikepizzo · 2018-11-13T19:10:24Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightPropertySerializer.cs

+            WriterValidationUtils.ValidatePropertyDefined(this.currentPropertyInfo, this.MessageWriterSettings.ThrowOnUndeclaredPropertyForNonOpenType);
+
+            duplicatePropertyNameChecker.ValidatePropertyUniqueness(property);
+


Not sure why these were moved? Can we put the two checks back before line 206 (before we write the type annotation for undeclared properties)? #Closed

Moved back to before L206. Thanks.

In reply to: 233183156 [](ancestors = 233183156)

mikepizzo · 2018-11-13T19:12:00Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightPropertySerializer.cs

            IDuplicatePropertyNameChecker duplicatePropertyNameChecker)
        {
            WriterValidationUtils.ValidatePropertyNotNull(property);

+            ODataValue value = property.ODataValue;
+
            string propertyName = property.Name;


To reduce unnecessary changes, can we move this back to after writing the instance annotation on line 213? #Closed

sure. done.

In reply to: 233183812 [](ancestors = 233183812)

mikepizzo · 2018-11-13T19:21:21Z

src/Microsoft.OData.Core/SelectedPropertiesNode.cs

+            }
+
+            return dynamicProperties;
+        }


These two methods should be deleted if they are no longer used.

cleaned up GetSelectedDynamicProperties.
GetSelectedProperties is still used by the test w/o using resource state.

In reply to: 233187531 [](ancestors = 233187531)

I don't like having methods (even internal) that are only used by tests. Is there another way the test can get the selected properties it is validating?

In reply to: 233297316 [](ancestors = 233297316,233187531)

mikepizzo · 2018-11-13T19:24:58Z

src/Microsoft.OData.Core/SelectedPropertiesNode.cs

+        }
+
+        /// <summary>
+        /// Gets the list of names for selected declared or dynamic properties having values of null.


Gets the list of names for selected declared or dynamic properties having values of null. [](start = 11, length = 90)

Does this return the names of the properties having null values, or the names of (nullable) properties missing from the payload? #Closed

Updated. Should be the names of properties missing from payload, which will be restored to null.

In reply to: 233188777 [](ancestors = 233188777)

mikepizzo · 2018-11-13T19:30:38Z

src/Microsoft.OData.Core/SelectedPropertiesNode.cs

+        /// <param name="structuredType">The current structured type.</param>
+        /// <param name="resourceState">The current resource state.</param>
+        /// <returns>The list of property names with null values.</returns>
+        internal IList<string> GetSelectedNullValueProperties(IEdmStructuredType structuredType,


GetSelectedNullValueProperties [](start = 31, length = 30)

We are doing a lot of the same work (i.e., determining the set of nullable properties) for each resource in the result. I wonder if it wouldn't be more efficient to determine the set of nullable property names up front, and then, at the end of writing each resource, restore nulls for the properties from that static list that have not been read.

The current logic follows the overall design of ODataJsonLightReader's EndEntry() method and has been the design from the beginning. It restores all missing nulls for the current entry all at once when the entry is deserialized. If we restored at the end of writing each resource, it would still need to perform the same restoration of null values, and with additional cost of caching information for null properties. Therefore doing it in flight is more efficient.

In reply to: 233190829 [](ancestors = 233190829)

I understand that we are restoring null values at the end -- that's fine. What I am questioning is whether we have to identify the set of nullable properties each time, or if we can do that once for the result (i.e., could the SelectPropertiesNode cache the set of names of nullable properties, and then when we restore we can just compare the set of properties written to the set of nullable properties and restore any missing, rather than recalculating the set of nullable properties each time?

In reply to: 233299115 [](ancestors = 233299115,233190829)

Since SelectedPropertiesNode is designed as part of context url parsing and is used to represent relatively static context url information, I am not sure it is proper for SelectedPropertiesNode to cache null properties, which is related to runtime payload and is not intrinsic part of SelectedPropertiesNode.
Additionally, there are no 'recalculating' per se; the calculations of set of omitted null properties are required for each structured property or entry on different level regardless whether restoration is done in-flight or via caching, and restoring it in-flight can save the cost of caching and fits with the design of the reader.

In reply to: 233553959 [](ancestors = 233553959,233299115,233190829)

Again, I'm interested in whether we can avoid determining the set of nullable properties in the SelectedPropertiesNode each time, not evaluating what the set of actual null property values is for each resource.

In reply to: 233666008 [](ancestors = 233666008,233553959,233299115,233190829)

mikepizzo · 2018-11-13T19:33:59Z

I like the new code much better. See my comments inline:

it looks like the old methods, that separately read declared and dynamic, are still there and should be removed?
rather than determine the list of nullable properties each time we read a resource, can we do it once for the result, and then compare what we've read with that static list of nullable properties?

In reply to: 436729775 [](ancestors = 436729775)

mikepizzo · 2018-11-13T19:35:36Z

As per separate thread, based on clarification from OASIS, we should not omit annotated values that are null, and should not restore missing values for annotated properties.

In reply to: 436424268 [](ancestors = 436424268,435924987)

biaol-odata · 2018-11-14T02:40:18Z

The changes related to excluding annotated values is in a forked branch I have shared with you. Will be integrated into this PR, pending on double-checking with detailed requirements as we discussed.
Edit:
Actually the change related to excluding annotated values has been integrated into this PR as commit fd97a44. Pending on finalized requirement.

In reply to: 438406870 [](ancestors = 438406870,436424268,435924987)

biaol-odata · 2018-11-14T02:51:08Z

@mikepizzo Please see the latest updates per your comments today. Currently there is one issue pending related to the annotated properties skipping omit-nulls, which will be merged into this PR when the detailed requirement is finalized. Thanks. #Closed

… tests in PropertyAndValueJsonLightReaderIntegrationTests.cs

biaol-odata · 2018-11-15T00:07:55Z

@mikepizzo I have updated the names to 'nestedResourceInfo'. They are from some original test cases that are not changed by this PR but it makes sense to update them. I have also provided my thoughts on your question related to caching null properties during restoration. Thanks. #Closed

mikepizzo · 2018-11-27T17:01:16Z

I don't see fd97a44 as part of this PR.

It looks like biaol-odata@fd97a44 handles not omitting writing the null value for annotated properties, but we need to validate handling of reading an annotated property that has no value.

Can ODL read/write a property with annotations and no value? Is Value=null handled the same as Value=ODataNull? Seems like they should be handled differently (the later former should write the annotations, if any, but no value). This could be a breaking change, however, if this is not how it's handled currently.

In reply to: 438516788 [](ancestors = 438516788,438406870,436424268,435924987)

agrabhi · 2019-09-16T07:17:01Z

@mikepizzo @biaol-odata @xuzhg
we need above feature for one of above api, because many properties tend to be null there. It seems like this PR has not been touched for a while. What can be done to get this PR checked in

agrabhi · 2019-10-27T07:29:09Z

@mikepizzo @biaol-odata @xuzhg
Is this PR on radar for checkin soon?

biaol-odata changed the title ~~Pr omit values null~~ Omit null property value in JSON response Mar 6, 2018

biaol-odata requested a review from mikepizzo March 6, 2018 20:36

kosinsky reviewed Mar 6, 2018

View reviewed changes

biaol-odata commented Mar 7, 2018

View reviewed changes

kosinsky previously approved these changes Mar 13, 2018

View reviewed changes

biaol-odata force-pushed the pr-omitValuesNull branch from ba5d506 to 7c1dba3 Compare March 14, 2018 23:58

biaol-odata force-pushed the pr-omitValuesNull branch from 7c1dba3 to bc4a274 Compare March 30, 2018 06:12

biaol-odata added the under-review label Apr 16, 2018

mikepizzo mentioned this pull request Apr 23, 2018

Ignore Null Values in output #944

Closed

mikepizzo reviewed May 7, 2018

View reviewed changes

biaol-odata force-pushed the pr-omitValuesNull branch 2 times, most recently from 954c798 to 88fa5ef Compare May 14, 2018 17:24

mikepizzo reviewed Jun 26, 2018

View reviewed changes

non-functional cleanup.

c1012d1

biaol-odata removed the Ready for review Use this label if a pull request is ready to be reviewed label Nov 8, 2018

Revert the optimization related to throwOnUndeclaredProperty.

2a5b9b1

biaol-odata added the Ready for review Use this label if a pull request is ready to be reviewed label Nov 8, 2018

Don't omit-nulls for annotated properties.

fd97a44

mikepizzo reviewed Nov 13, 2018

View reviewed changes

CR comments.

455f5bd

CR comment: renamed navigationLink to nestedResourceInfo for original…

db86b90

… tests in PropertyAndValueJsonLightReaderIntegrationTests.cs

raheph assigned mikepizzo and unassigned biaol-odata Dec 19, 2018

mikepizzo removed the Ready for review Use this label if a pull request is ready to be reviewed label Apr 29, 2019

xuzhg force-pushed the master branch 2 times, most recently from f9519fd to 14a4b12 Compare September 22, 2019 21:12

KenitoInc dismissed kosinsky’s stale review via db86b90 March 4, 2022 06:38

mikepizzo marked this pull request as draft March 28, 2023 04:43

		@@ -202,8 +223,6 @@ private bool IsOpenProperty(ODataProperty property)

		WriteInstanceAnnotation(property, isTopLevel, currentPropertyInfo.MetadataType.IsUndeclaredProperty);

		WriterValidationUtils.ValidatePropertyDefined(this.currentPropertyInfo, this.MessageWriterSettings.ThrowOnUndeclaredPropertyForNonOpenType);

		duplicatePropertyNameChecker.ValidatePropertyUniqueness(property);

Omit null property value in JSON response #1106

Are you sure you want to change the base?

Omit null property value in JSON response #1106

Conversation

biaol-odata commented Mar 6, 2018

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

kosinsky Mar 6, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

biaol-odata Mar 6, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

kosinsky Mar 6, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

biaol-odata Mar 12, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

kosinsky commented Mar 6, 2018 • edited by mikepizzo Loading

biaol-odata Mar 7, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biaol-odata Mar 7, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biaol-odata Mar 7, 2018 • edited by mikepizzo Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

biaol-odata Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo May 7, 2018 • edited Loading

Choose a reason for hiding this comment

biaol-odata May 10, 2018 • edited Loading

Choose a reason for hiding this comment

kosinsky left a comment

Choose a reason for hiding this comment

mikepizzo May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo May 7, 2018 • edited Loading

Choose a reason for hiding this comment

biaol-odata May 9, 2018 • edited Loading

Choose a reason for hiding this comment

mikepizzo May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo May 7, 2018 • edited by biaol-odata Loading

Choose a reason for hiding this comment

biaol-odata May 10, 2018 • edited Loading

Choose a reason for hiding this comment

biaol-odata commented May 14, 2018 • edited by mikepizzo Loading

biaol-odata commented Jun 1, 2018 • edited by mikepizzo Loading

kosinsky Mar 6, 2018 •

edited by mikepizzo

Loading

biaol-odata Mar 6, 2018 •

edited by mikepizzo

Loading

kosinsky Mar 6, 2018 •

edited by mikepizzo

Loading

biaol-odata Mar 12, 2018 •

edited by mikepizzo

Loading

kosinsky commented Mar 6, 2018 •

edited by mikepizzo

Loading

biaol-odata Mar 7, 2018 •

edited by mikepizzo

Loading

biaol-odata Mar 7, 2018 •

edited by mikepizzo

Loading

biaol-odata Mar 7, 2018 •

edited by mikepizzo

Loading

mikepizzo Nov 27, 2018 •

edited

Loading

biaol-odata Mar 7, 2018 •

edited

Loading

mikepizzo May 7, 2018 •

edited

Loading

biaol-odata May 10, 2018 •

edited

Loading

mikepizzo May 7, 2018 •

edited

Loading

mikepizzo May 7, 2018 •

edited

Loading

mikepizzo May 7, 2018 •

edited

Loading

mikepizzo May 7, 2018 •

edited

Loading

biaol-odata May 9, 2018 •

edited

Loading

mikepizzo May 7, 2018 •

edited

Loading

mikepizzo May 7, 2018 •

edited

Loading

mikepizzo May 7, 2018 •

edited

Loading

mikepizzo May 7, 2018 •

edited

Loading

mikepizzo May 7, 2018 •

edited by biaol-odata

Loading

biaol-odata May 10, 2018 •

edited

Loading

biaol-odata commented May 14, 2018 •

edited by mikepizzo

Loading

biaol-odata commented Jun 1, 2018 •

edited by mikepizzo

Loading

mikepizzo Jun 26, 2018 •

edited

Loading

mikepizzo Jun 26, 2018 •

edited

Loading

mikepizzo Jun 26, 2018 •

edited

Loading

biaol-odata commented Nov 7, 2018 •

edited by mikepizzo

Loading

mikepizzo Nov 13, 2018 •

edited

Loading

mikepizzo Nov 13, 2018 •

edited

Loading

mikepizzo Nov 13, 2018 •

edited

Loading

mikepizzo Nov 13, 2018 •

edited

Loading

mikepizzo Nov 13, 2018 •

edited

Loading