-
Notifications
You must be signed in to change notification settings - Fork 351
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
Omit null property value in JSON response #1106
base: release-7.x
Are you sure you want to change the base?
Changes from 13 commits
6f45c7c
2190c88
fdcbb0a
defcef9
35ec7d0
a75d181
3f9908a
f8e687a
df084b6
7f83fe5
6af64ea
c1012d1
2a5b9b1
fd97a44
455f5bd
db86b90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,6 +84,7 @@ internal void WriteTopLevelProperty(ODataProperty property) | |
null /*owningType*/, | ||
true /* isTopLevel */, | ||
false /* allowStreamProperty */, | ||
false /*omitNullValues */, | ||
this.CreateDuplicatePropertyNameChecker()); | ||
this.JsonLightValueSerializer.AssertRecursionDepthIsZero(); | ||
|
||
|
@@ -100,11 +101,15 @@ internal void WriteTopLevelProperty(ODataProperty property) | |
/// Whether the properties are being written for complex value. Also used for detecting whether stream properties | ||
/// are allowed as named stream properties should only be defined on ODataResource instances | ||
/// </param> | ||
/// <param name="omitNullValues">Whether to omit null property values. | ||
/// Value should be 'false' if writing delta payload, otherwise derived from request preference header. | ||
/// </param> | ||
/// <param name="duplicatePropertyNameChecker">The DuplicatePropertyNameChecker to use.</param> | ||
internal void WriteProperties( | ||
IEdmStructuredType owningType, | ||
IEnumerable<ODataProperty> properties, | ||
bool isComplexValue, | ||
bool omitNullValues, | ||
IDuplicatePropertyNameChecker duplicatePropertyNameChecker) | ||
{ | ||
if (properties == null) | ||
|
@@ -119,6 +124,7 @@ internal void WriteProperties( | |
owningType, | ||
false /* isTopLevel */, | ||
!isComplexValue, | ||
omitNullValues, | ||
duplicatePropertyNameChecker); | ||
} | ||
} | ||
|
@@ -141,7 +147,7 @@ private bool IsOpenProperty(ODataProperty property) | |
else | ||
{ | ||
// TODO: (issue #888) this logic results in type annotations not being written for dynamic properties on types that are not | ||
// marked as open. Type annotations should always be written for dynamic properties whose type cannot be hueristically | ||
// marked as open. Type annotations should always be written for dynamic properties whose type cannot be heuristically | ||
// determined. Need to change this.currentPropertyInfo.MetadataType.IsOpenProperty to this.currentPropertyInfo.MetadataType.IsDynamic, | ||
// and fix related tests and other logic (this change alone results in writing type even if it's already implied by context). | ||
isOpenProperty = (!this.WritingResponse && this.currentPropertyInfo.MetadataType.OwningType == null) // Treat property as dynamic property when writing request and owning type is null | ||
|
@@ -164,17 +170,23 @@ private bool IsOpenProperty(ODataProperty property) | |
/// <param name="isTopLevel">true when writing a top-level property; false for nested properties.</param> | ||
/// <param name="allowStreamProperty">Should pass in true if we are writing a property of an ODataResource instance, false otherwise. | ||
/// Named stream properties should only be defined on ODataResource instances.</param> | ||
/// <param name="omitNullValues"> Whether to omit null property values for writing. | ||
/// Value should be 'false' if writing top level properties or delta payload, otherwise derived from request preference header. | ||
/// </param> | ||
/// <param name="duplicatePropertyNameChecker">The DuplicatePropertyNameChecker to use.</param> | ||
[SuppressMessage("Microsoft.Maintainability", "CA1506:AvoidExcessiveClassCoupling", Justification = "Splitting the code would make the logic harder to understand; class coupling is only slightly above threshold.")] | ||
private void WriteProperty( | ||
ODataProperty property, | ||
IEdmStructuredType owningType, | ||
bool isTopLevel, | ||
bool allowStreamProperty, | ||
bool omitNullValues, | ||
IDuplicatePropertyNameChecker duplicatePropertyNameChecker) | ||
{ | ||
WriterValidationUtils.ValidatePropertyNotNull(property); | ||
|
||
ODataValue value = property.ODataValue; | ||
|
||
string propertyName = property.Name; | ||
|
||
if (this.JsonLightOutputContext.MessageWriterSettings.Validations != ValidationKinds.None) | ||
|
@@ -191,9 +203,6 @@ private void WriteProperty( | |
this.currentPropertyInfo = this.JsonLightOutputContext.PropertyCacheHandler.GetProperty(propertyName, owningType); | ||
} | ||
|
||
WriterValidationUtils.ValidatePropertyDefined(this.currentPropertyInfo, this.MessageWriterSettings.ThrowOnUndeclaredPropertyForNonOpenType); | ||
|
||
duplicatePropertyNameChecker.ValidatePropertyUniqueness(property); | ||
|
||
if (currentPropertyInfo.MetadataType.IsUndeclaredProperty) | ||
{ | ||
|
@@ -202,7 +211,18 @@ private void WriteProperty( | |
|
||
WriteInstanceAnnotation(property, isTopLevel, currentPropertyInfo.MetadataType.IsUndeclaredProperty); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
ODataValue value = property.ODataValue; | ||
WriterValidationUtils.ValidatePropertyDefined(this.currentPropertyInfo, this.MessageWriterSettings.ThrowOnUndeclaredPropertyForNonOpenType); | ||
|
||
duplicatePropertyNameChecker.ValidatePropertyUniqueness(property); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
bool isNullValue = (value == null || value is ODataNullValue); | ||
if (isNullValue && omitNullValues) | ||
{ | ||
if (!this.currentPropertyInfo.IsTopLevel) | ||
{ | ||
return; | ||
} | ||
} | ||
|
||
// handle ODataUntypedValue | ||
ODataUntypedValue untypedValue = value as ODataUntypedValue; | ||
|
@@ -227,7 +247,7 @@ private void WriteProperty( | |
return; | ||
} | ||
|
||
if (value is ODataNullValue || value == null) | ||
if (isNullValue) | ||
{ | ||
this.WriteNullProperty(property); | ||
return; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reduce unnecessary changes, can we move this back to after writing the instance annotation on line 213? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. done.
In reply to: 233183812 [](ancestors = 233183812)