add omit-values supports #657

xuzhg · 2022-08-08T23:48:00Z

OData specs has the following at: http://docs.oasis-open.org/odata/odata/v4.01/cs01/part1-protocol/odata-v4.01-cs01-part1-protocol.html#_Toc505771137

 8.2.8.6 [Preference omit-values](http://docs.oasis-open.org/odata/odata/v4.01/cs01/part1-protocol/odata-v4.01-cs01-part1-protocol.html#sec_Preferenceomitvalues)
The omit-values preference specifies values that MAY be omitted from a response payload. Valid values are nulls or defaults.

If nulls is specified, then the service MAY omit properties containing null values from the response, in which case it MUST specify the Preference-Applied response header with omit-values=nulls.

If defaults is specified, then the service MAY omit properties containing default values from the response, including nulls for properties that have no other defined default value. Nulls MUST be included for properties that have a non-null default value defined. If the service omits default values it MUST specify the Preference-Applied response header with omit-values=defaults.

Properties with null or default values MUST be included in delta payloads, if modified.

The response to a POST operation MUST include any properties not set to their default value, and the response to a PUT/PATCH operation MUST include any properties whose values were changed as part of the operation.

The omit-values preference does not affect a request payload.

This PR is used to discuss the service side support for omit_values prefer header.

chrisspre · 2022-08-09T16:35:45Z

src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs

+        /// </summary>
+        /// <param name="request">The <see cref="HttpRequest"/> instance to access.</param>
+        /// <returns>The <see cref="OmitValuesKind"/> from the request.</returns>
+        public static OmitValuesKind GetOmitValuesKind(this HttpRequest request)


Should this indicate that it is a header value, or even a Prefer Header. Something like GetOmitValueHeaderKind

Emm, actually it's not only from request header, it's also from ODataOption setting if there's no setting.

But, I am curious which one is a higher priority. @mikepizzo

What about GetOmitValuesPreferenceKind, or even just GetOmitValuesPreference with a rename on the enum to OmitValuesPreference?

src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs

…s.cs Co-authored-by: Christof Sprenger <[email protected]>

julealgon · 2022-08-24T17:50:17Z

src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs

@@ -71,6 +72,44 @@ public static ODataOptions ODataOptions(this HttpRequest request)
            return request.HttpContext.ODataOptions();
        }

+        /// <summary>
+        /// Returns the <see cref="OmitValuesKind"/> from the request.


For consistency with the method name, I'd suggest a small rename:

Suggested change

/// Returns the <see cref="OmitValuesKind"/> from the request.

/// Gets the <see cref="OmitValuesKind"/> from the request.

julealgon · 2022-08-24T17:51:59Z

src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs

+        /// Returns the <see cref="OmitValuesKind"/> from the request.
+        /// </summary>
+        /// <param name="request">The <see cref="HttpRequest"/> instance to access.</param>
+        /// <returns>The <see cref="OmitValuesKind"/> from the request.</returns>


Should we also document the exception here?

Suggested change

/// <returns>The <see cref="OmitValuesKind"/> from the request.</returns>

/// <returns>The <see cref="OmitValuesKind"/> from the request.</returns>

/// <exception cref="ArgumentNullException"><paramref name="request"/> is <see langword="null"/>.</exception>

julealgon · 2022-08-24T17:52:25Z

src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs

+        /// <returns>The <see cref="OmitValuesKind"/> from the request.</returns>
+        public static OmitValuesKind GetOmitValuesKind(this HttpRequest request)
+        {
+            if (request == null)


For constant comparisons, I'd suggest using constant pattern matching on new code:

Suggested change

if (request == null)

if (request is null)

julealgon · 2022-08-24T17:55:25Z

src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs

+                throw Error.ArgumentNull(nameof(request));
+            }
+
+            // The 'Prefer' header from client has the top priority


Instead of having code blocks with comments such as this, I'd suggest extracting each "strategy" into at least a local function, and then calling them in the explicit fallback order, something like:

return GetOmitValuePreferenceFromHeaders() ?? GetOmitValuePreferenceFromODataConfiguration();

This not only cleanly isolates each "strategy" into a named method (most likely removing the need for the comment itself), but also models the fallback logic in a more clean and direct way that is more apparent to the reader.

julealgon · 2022-08-24T17:58:29Z

src/Microsoft.AspNetCore.OData/Formatter/OmitValuesKind.cs

+        /// <summary>
+        /// Not set, unknown
+        /// </summary>
+        Unknown,


Have you considered just not having this entry at all? Since it is completely invalid/not part of the spec, I don't think it is a good idea to even map it.

If there are any places where the value is optional, a nullable OmitValuesKind could be used.

julealgon · 2022-08-24T18:11:11Z

src/Microsoft.AspNetCore.OData/Formatter/ResponsePreferenceHelpers.cs

+                prefer_applied = values.FirstOrDefault();
+            }
+
+            string omitValuesHead = omitValuesKind == OmitValuesKind.Nulls ?


Suggested change

string omitValuesHead = omitValuesKind == OmitValuesKind.Nulls ?

string omitValuesHeaderValue = omitValuesKind == OmitValuesKind.Nulls ?

julealgon · 2022-08-24T18:17:29Z

src/Microsoft.AspNetCore.OData/Formatter/ResponsePreferenceHelpers.cs

+
+            string omitValuesHead = omitValuesKind == OmitValuesKind.Nulls ?
+                "omit-values=nulls" :
+                "omit-values=defaults";


Instead of handling these with raw strings, have you considered creating a small model and/or factory method for it?

Imagine something like

public sealed class PreferHeader { public static PreferHeader OmitValues(OmitValuesKind kind) { return new("omit-values", kind.ToText()) } }

This would remove the need to ever handle raw strings. You can parse values into this model and use factory methods to create strongly typed instances for each preference OData has.

julealgon · 2022-08-24T18:21:34Z

src/Microsoft.AspNetCore.OData/Formatter/ResponsePreferenceHelpers.cs

+            }
+            else
+            {
+                response.Headers[PreferAppliedHeaderName] = $"{prefer_applied},{omitValuesHead}";


Is there any way to avoid this manual string concatenation by relying on the StringValues type itself to do it?

We could probably "add" the omit setting to the existing StringValues instance and then write that directly.

julealgon · 2022-08-24T18:22:36Z

src/Microsoft.AspNetCore.OData/Formatter/ResponsePreferenceHelpers.cs

+                // If there are many "Preference-Applied" headers, pick up the first one.
+                prefer_applied = values.FirstOrDefault();


Can you elaborate why we are picking the first one? Shouldn't we just append ours to the end of the existing list no matter how many there are?

julealgon · 2022-08-24T18:36:56Z

src/Microsoft.AspNetCore.OData/ODataOptions.cs

+        /// <summary>
+        /// Gets or sets a omit values kind for the property value serialization.
+        /// </summary>
+        public OmitValuesKind OmitValuesKind { get; set; } = OmitValuesKind.Unknown;


As per other comments, I think it would be better if this was a nullable:

Suggested change

public OmitValuesKind OmitValuesKind { get; set; } = OmitValuesKind.Unknown;

public OmitValuesKind? OmitValuesKind { get; set; };

Nthemba · 2022-08-26T07:34:17Z

src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs

+                if (preferHeader.Contains("omit-values=nulls", StringComparison.OrdinalIgnoreCase))
+                {
+                    return OmitValuesKind.Nulls;
+                }
+
+                if (preferHeader.Contains("omit-values=defaults", StringComparison.OrdinalIgnoreCase))
+                {
+                    return OmitValuesKind.Defaults;
+                }


Is there a possibility for both 'omit-values=defaults' and 'omit-values=nulls' to be added as headers in a request? In that case won't one of them be ignored?

Nthemba

Add some tests

add omit-values supports

4a54f9a

xuzhg requested review from corranrogue9, gathogojr, habbes, KenitoInc and mikepizzo August 8, 2022 23:48

xuzhg added the feature label Aug 8, 2022

chrisspre mentioned this pull request Aug 9, 2022

Expose ability to over-control complex property serialization #658

Closed

chrisspre reviewed Aug 9, 2022

View reviewed changes

src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs Outdated Show resolved Hide resolved

Update src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtension…

57bf43b

…s.cs Co-authored-by: Christof Sprenger <[email protected]>

xuzhg mentioned this pull request Aug 24, 2022

Support Omit-values prefer header #672

Open

julealgon reviewed Aug 24, 2022

View reviewed changes

Nthemba reviewed Aug 26, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add omit-values supports #657

add omit-values supports #657

xuzhg commented Aug 8, 2022

chrisspre Aug 9, 2022

xuzhg Aug 10, 2022

julealgon Aug 24, 2022

julealgon Aug 24, 2022

julealgon Aug 24, 2022

julealgon Aug 24, 2022

julealgon Aug 24, 2022

julealgon Aug 24, 2022

julealgon Aug 24, 2022

julealgon Aug 24, 2022

julealgon Aug 24, 2022

julealgon Aug 24, 2022

julealgon Aug 24, 2022

Nthemba Aug 26, 2022

Nthemba left a comment

	/// Returns the <see cref="OmitValuesKind"/> from the request.
	/// Gets the <see cref="OmitValuesKind"/> from the request.

	/// <returns>The <see cref="OmitValuesKind"/> from the request.</returns>
	/// <returns>The <see cref="OmitValuesKind"/> from the request.</returns>
	/// <exception cref="ArgumentNullException"><paramref name="request"/> is <see langword="null"/>.</exception>

	string omitValuesHead = omitValuesKind == OmitValuesKind.Nulls ?
	string omitValuesHeaderValue = omitValuesKind == OmitValuesKind.Nulls ?

		// If there are many "Preference-Applied" headers, pick up the first one.
		prefer_applied = values.FirstOrDefault();

	public OmitValuesKind OmitValuesKind { get; set; } = OmitValuesKind.Unknown;
	public OmitValuesKind? OmitValuesKind { get; set; };

add omit-values supports #657

Are you sure you want to change the base?

add omit-values supports #657

Conversation

xuzhg commented Aug 8, 2022

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

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

Nthemba left a comment

Choose a reason for hiding this comment