Fix deltas #1365
Fix deltas #1365
Conversation
-only write properties in deltaset items that have been set -support writing additional properties for deleted entities -nested resource sets that aren't deltasets should be written w/o @delta -factored out ODataDeletedResourceSerializer -support writing delta payloads w/out knowing navigation source (i.e., when serializing results from a function)
Leverage more common functionality in ODataResourceSerializer.
src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataDeletedResourceSerializer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataDeletedResourceSerializer.cs
Outdated
Show resolved
Hide resolved
| @@ -75,7 +75,7 @@ public async Task DeltaSet_WithDeletedAndODataId_IsSerializedSuccessfully() | |||
| {'ID':2,'Name':'Employee2','[email protected]':[{'@id':'Friends(1)'}]} | |||
| ]}"; | |||
|
|
|||
| string expectedResponse = "{\"@context\":\"http://localhost/convention/$metadata#Employees/$delta\",\"value\":[{\"ID\":1,\"Name\":\"Employee1\",\"Friends@delta\":[{\"@removed\":{\"reason\":\"changed\"},\"@id\":\"http://host/service/Friends(1)\"}]},{\"ID\":2,\"Name\":\"Employee2\",\"Friends@delta\":[{\"Id\":1}]}]}"; | |||
| string expectedResponse = "{\"@context\":\"http://localhost/convention/$metadata#Employees/$delta\",\"value\":[{\"ID\":1,\"Name\":\"Employee1\",\"Friends@delta\":[{\"@removed\":{\"reason\":\"changed\"},\"@id\":\"http://host/service/Friends(1)\",\"id\":1}]},{\"ID\":2,\"Name\":\"Employee2\",\"Friends@delta\":[{\"Id\":1}]}]}"; | |||
There was a problem hiding this comment.
why dose the expected response look different? and since it contains @id, why does it contain "id" again? #ByDesign
There was a problem hiding this comment.
Previously, we weren't returning properties of a deleted resource even if they were set by the service, which is why id was not being returned (which was a bug).
In this case, both the @id and the id were explicitly set in the controller (they were both set in the request payload which is echoed in the response payload), so they should both be returned in the response.
src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataResourceSerializer.cs
Outdated
Show resolved
Hide resolved
Change ODataDeletedResourceSerializer to derive from ODataResourceSerializer
| // Preserve setting of OmitODataPrefix | ||
| if (writerSettings.Version.GetValueOrDefault() == ODataVersion.V4) | ||
| { | ||
| writerSettings.SetOmitODataPrefix(writerSettings.GetOmitODataPrefix(ODataVersion.V4), ODataVersion.V401); |
There was a problem hiding this comment.
why don't we enable it using configuration? not hard-coded?
By default, it's ok to enable the configuration, but at least , maybe we need a way to let customer change the default? #Resolved
There was a problem hiding this comment.
This code just fixes a bug where we weren't honoring the writing of the odata prefix in cases where we did change the version. I'd like to separately track if the user can override serializing deltas as 4.01, as this is existing behavior, and changing that behavior is outside of the scope of this PR.
| Error.Format(SRResources.TypeCannotBeSerialized, expectedType.ToTraceString())); | ||
| } | ||
| await serializer.WriteResourceContent(writer, selectExpandNode, resourceContext, /*isDelta*/ true); | ||
| await writer.WriteEndAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
shall we call 'ConfigureAwait' for all await?
There was a problem hiding this comment.
We currently always use ConfigureAwait(false) to improve perf and avoid possible deadlocks because we are a library and not an app.
| private bool WriteDeltaDeletedResourceAsyncIsOverridden() | ||
| { | ||
| MethodInfo method = GetType().GetMethod("WriteDeltaDeletedResourceAsync", new Type[] { typeof(object), typeof(ODataWriter), typeof(ODataSerializerContext) }); | ||
| Contract.Assert(method != null, "WriteDeltaDeletedResourceAsync is not defined."); |
There was a problem hiding this comment.
- can we leave a details comment for this method?
- Moreover, what about if customer creates/customize his own ODataDeletedResourceSerilizer? #Resolved
There was a problem hiding this comment.
- Added comments
- This is only here for backward compatibility. If the user customizes their own ODataDeletedResourceSerializer, they can provide whatever custom logic they need.
| /// ODataSerializer for serializing instances of <see cref="IEdmDeltaDeletedResourceObject"/>/> | ||
| /// </summary> | ||
| [SuppressMessage("Microsoft.Maintainability", "CA1506:AvoidExcessiveClassCoupling", Justification = "Relies on many ODataLib classes.")] | ||
| public class ODataDeletedResourceSerializer : ODataResourceSerializer |
There was a problem hiding this comment.
I didn't see to register this serializer into the DI? Will you do it later or miss that?
There was a problem hiding this comment.
The regular method of using the EdmTypeSerializer doesn't work because it doesn't differentiate between writing a resource and writing a deleted resource. So, I added code to the only place the DeletedResourceSerializer is created, which is from the DeltaResourceSetSerializer, to use DI, and registered the ODataDeletedResourceSerailizer in AddDefaultApiServices. Note that DeltaResourceSetSerilizer falls back to directly creating the ODataDeletedResoruceSerializer if GetService returns null, for example, if the service isn't using our AddDefaultWebApiServices.
Fixes #1360