add omit-values supports #657
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |||||||
| using Microsoft.AspNetCore.Http; | ||||||||
| using Microsoft.AspNetCore.OData.Abstracts; | ||||||||
| using Microsoft.AspNetCore.OData.Common; | ||||||||
| using Microsoft.AspNetCore.OData.Formatter; | ||||||||
| using Microsoft.AspNetCore.OData.Formatter.Deserialization; | ||||||||
| using Microsoft.AspNetCore.OData.Query; | ||||||||
| using Microsoft.Extensions.DependencyInjection; | ||||||||
|
|
@@ -71,6 +72,44 @@ public static ODataOptions ODataOptions(this HttpRequest request) | |||||||
| return request.HttpContext.ODataOptions(); | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// 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> | ||||||||
|
There was a problem hiding this comment. Should we also document the exception here?
Suggested change
|
||||||||
| public static OmitValuesKind GetOmitValuesKind(this HttpRequest request) | ||||||||
|
There was a problem hiding this comment. Should this indicate that it is a header value, or even a Prefer Header. Something like GetOmitValueHeaderKind There was a problem hiding this comment. 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 There was a problem hiding this comment. What about |
||||||||
| { | ||||||||
| if (request == null) | ||||||||
|
There was a problem hiding this comment. For constant comparisons, I'd suggest using constant pattern matching on new code:
Suggested change
|
||||||||
| { | ||||||||
| throw Error.ArgumentNull(nameof(request)); | ||||||||
| } | ||||||||
|
|
||||||||
| // The 'Prefer' header from client has the top priority | ||||||||
|
There was a problem hiding this comment. 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. |
||||||||
| string preferHeader = RequestPreferenceHelpers.GetRequestPreferHeader(request.Headers); | ||||||||
| if (preferHeader != null) | ||||||||
| { | ||||||||
| // use case insensitive string comparison | ||||||||
| if (preferHeader.Contains("omit-values=nulls", StringComparison.OrdinalIgnoreCase)) | ||||||||
| { | ||||||||
| return OmitValuesKind.Nulls; | ||||||||
| } | ||||||||
|
|
||||||||
| if (preferHeader.Contains("omit-values=defaults", StringComparison.OrdinalIgnoreCase)) | ||||||||
| { | ||||||||
| return OmitValuesKind.Defaults; | ||||||||
| } | ||||||||
|
Comment on lines
+92
to
+100
There was a problem hiding this comment. 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? |
||||||||
| } | ||||||||
|
|
||||||||
| // If there's no setting from client, we move to use the configuration on the OData options. | ||||||||
| ODataOptions options = request.ODataOptions(); | ||||||||
| if (options == null) | ||||||||
| { | ||||||||
| return OmitValuesKind.Unknown; | ||||||||
| } | ||||||||
|
|
||||||||
| return options.OmitValuesKind; | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Gets the <see cref="IEdmModel"/> from the request container. | ||||||||
| /// </summary> | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| //----------------------------------------------------------------------------- | ||
| // <copyright file="OmitValuesKind.cs" company=".NET Foundation"> | ||
| // Copyright (c) .NET Foundation and Contributors. All rights reserved. | ||
| // See License.txt in the project root for license information. | ||
| // </copyright> | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| namespace Microsoft.AspNetCore.OData.Formatter | ||
| { | ||
| /// <summary> | ||
| /// Omit-Values kind | ||
| /// </summary> | ||
| public enum OmitValuesKind | ||
| { | ||
| /// <summary> | ||
| /// Not set, unknown | ||
| /// </summary> | ||
| Unknown, | ||
|
There was a problem hiding this comment. 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 |
||
|
|
||
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| Nulls, | ||
|
|
||
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| Defaults | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,55 @@ | ||||||
| //----------------------------------------------------------------------------- | ||||||
| // <copyright file="ResponsePreferenceHelpers.cs" company=".NET Foundation"> | ||||||
| // Copyright (c) .NET Foundation and Contributors. All rights reserved. | ||||||
| // See License.txt in the project root for license information. | ||||||
| // </copyright> | ||||||
| //------------------------------------------------------------------------------ | ||||||
|
|
||||||
| using System.Linq; | ||||||
| using Microsoft.AspNetCore.Http; | ||||||
| using Microsoft.AspNetCore.OData.Extensions; | ||||||
| using Microsoft.Extensions.Primitives; | ||||||
|
|
||||||
| namespace Microsoft.AspNetCore.OData.Formatter | ||||||
| { | ||||||
| internal static class ResponsePreferenceHelpers | ||||||
| { | ||||||
| public const string PreferAppliedHeaderName = "Preference-Applied"; | ||||||
|
|
||||||
| public static void SetResponsePreferAppliedHeader(HttpResponse response) | ||||||
| { | ||||||
| if (response == null) | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| { | ||||||
| throw Error.ArgumentNull(nameof(response)); | ||||||
| } | ||||||
|
|
||||||
| HttpRequest request = response.HttpContext.Request; | ||||||
|
|
||||||
| OmitValuesKind omitValuesKind = request.GetOmitValuesKind(); | ||||||
| if (omitValuesKind == OmitValuesKind.Unknown) | ||||||
| { | ||||||
| return; | ||||||
| } | ||||||
|
Comment on lines
+29
to
+32
There was a problem hiding this comment. Again, not a big fan of having this kind of logic. Can't we just make it so that |
||||||
|
|
||||||
| string prefer_applied = null; | ||||||
| if (response.Headers.TryGetValue(PreferAppliedHeaderName, out StringValues values)) | ||||||
| { | ||||||
| // If there are many "Preference-Applied" headers, pick up the first one. | ||||||
| prefer_applied = values.FirstOrDefault(); | ||||||
|
Comment on lines
+37
to
+38
There was a problem hiding this comment. 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? |
||||||
| } | ||||||
|
|
||||||
| string omitValuesHead = omitValuesKind == OmitValuesKind.Nulls ? | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| "omit-values=nulls" : | ||||||
| "omit-values=defaults"; | ||||||
|
Comment on lines
+42
to
+43
There was a problem hiding this comment. We are referencing these hardcoded strings in multiple places. I think it should be extracted to consts to avoid sharing them as hardcoded data. There was a problem hiding this comment. 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. |
||||||
|
|
||||||
| if (prefer_applied == null) | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| { | ||||||
| response.Headers[PreferAppliedHeaderName] = omitValuesHead; | ||||||
|
There was a problem hiding this comment. Instead of manually accessing each header like this, have you considered creating something like the native It could be called |
||||||
| } | ||||||
| else | ||||||
| { | ||||||
| response.Headers[PreferAppliedHeaderName] = $"{prefer_applied},{omitValuesHead}"; | ||||||
|
There was a problem hiding this comment. Is there any way to avoid this manual string concatenation by relying on the We could probably "add" the omit setting to the existing |
||||||
| } | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
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.
For consistency with the method name, I'd suggest a small rename: