Skip to content
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

Fix deltas #1365

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,14 @@ internal static async Task WriteToStreamAsync(
writerSettings.BaseUri = baseAddress;

//use v401 to write delta payloads.
if (serializer.ODataPayloadKind == ODataPayloadKind.Delta)
if (serializer.ODataPayloadKind == ODataPayloadKind.Delta && version < ODataVersion.V401)
{
// Preserve setting of OmitODataPrefix
if (writerSettings.Version.GetValueOrDefault() == ODataVersion.V4)
{
writerSettings.SetOmitODataPrefix(writerSettings.GetOmitODataPrefix(ODataVersion.V4), ODataVersion.V401);
Copy link
Member

@xuzhg xuzhg Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

writerSettings.Version = ODataVersion.V401;
}
else
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
//-----------------------------------------------------------------------------
// <copyright file="ODataDeletedResourceSerializer.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;
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.Contracts;
using System.Runtime.Serialization;
using Microsoft.AspNetCore.OData.Formatter.Value;
using Microsoft.AspNetCore.OData.Routing;
using Microsoft.OData;
using Microsoft.OData.Edm;
using System.Threading.Tasks;
using Microsoft.AspNetCore.OData.Deltas;

namespace Microsoft.AspNetCore.OData.Formatter.Serialization;

/// <summary>
/// ODataSerializer for serializing instances of <see cref="IEdmDeltaDeletedResourceObject"/>/>
/// </summary>
[SuppressMessage("Microsoft.Maintainability", "CA1506:AvoidExcessiveClassCoupling", Justification = "Relies on many ODataLib classes.")]
public class ODataDeletedResourceSerializer : ODataResourceSerializer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see to register this serializer into the DI? Will you do it later or miss that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
private const string Resource = "DeletedResource";

/// <inheritdoc />
public ODataDeletedResourceSerializer(IODataSerializerProvider serializerProvider)
: base(serializerProvider)
{
}

/// <inheritdoc />
public override async Task WriteObjectAsync(object graph, Type type, ODataMessageWriter messageWriter,
ODataSerializerContext writeContext)
{
if (messageWriter == null)
{
throw Error.ArgumentNull(nameof(messageWriter));
}

if (writeContext == null)
{
throw Error.ArgumentNull(nameof(writeContext));
}

bool isUntypedPath = writeContext.Path.IsUntypedPropertyPath();
IEdmTypeReference edmType = writeContext.GetEdmType(graph, type, isUntypedPath);
Contract.Assert(edmType != null);

IEdmNavigationSource navigationSource = writeContext.NavigationSource;
ODataWriter writer = await messageWriter.CreateODataResourceWriterAsync(navigationSource, edmType.ToStructuredType())
.ConfigureAwait(false);
await WriteObjectInlineAsync(graph, edmType, writer, writeContext).ConfigureAwait(false);
}

/// <inheritdoc />
public override async Task WriteObjectInlineAsync(object graph, IEdmTypeReference expectedType, ODataWriter writer,
ODataSerializerContext writeContext)
{
if (writer == null)
{
throw Error.ArgumentNull(nameof(writer));
}

if (writeContext == null)
{
throw Error.ArgumentNull(nameof(writeContext));
}

if (graph == null || graph is NullEdmComplexObject)
{
throw new SerializationException(Error.Format(SRResources.CannotSerializerNull, Resource));
}
else
{
await WriteDeletedResourceAsync(graph, writer, writeContext, expectedType).ConfigureAwait(false);
}
}

private async Task WriteDeletedResourceAsync(object graph, ODataWriter writer, ODataSerializerContext writeContext,
IEdmTypeReference expectedType)
{
Contract.Assert(writeContext != null);

IEdmStructuredTypeReference structuredType = ODataResourceSerializer.GetResourceType(graph, writeContext);
ResourceContext resourceContext = new ResourceContext(writeContext, structuredType, graph);

SelectExpandNode selectExpandNode = CreateSelectExpandNode(resourceContext);
if (selectExpandNode != null)
{
ODataDeletedResource odataDeletedResource;

if (graph is EdmDeltaDeletedResourceObject edmDeltaDeletedEntity)
{
odataDeletedResource = CreateDeletedResource(edmDeltaDeletedEntity.Id, edmDeltaDeletedEntity.Reason ?? DeltaDeletedEntryReason.Deleted, selectExpandNode, resourceContext);
if (edmDeltaDeletedEntity.NavigationSource != null)
{
resourceContext.NavigationSource = edmDeltaDeletedEntity.NavigationSource;
ODataResourceSerializationInfo serializationInfo = new ODataResourceSerializationInfo
{
NavigationSourceName = edmDeltaDeletedEntity.NavigationSource.Name
};
odataDeletedResource.SetSerializationInfo(serializationInfo);
}
}
else if (graph is IDeltaDeletedResource deltaDeletedResource)
{
odataDeletedResource = CreateDeletedResource(deltaDeletedResource.Id, deltaDeletedResource.Reason ?? DeltaDeletedEntryReason.Deleted, selectExpandNode, resourceContext);
}
else
{
throw new SerializationException(Error.Format(SRResources.CannotWriteType, GetType().Name, graph?.GetType().FullName));
}

await writer.WriteStartAsync(odataDeletedResource).ConfigureAwait(false);
ODataResourceSerializer serializer = SerializerProvider.GetEdmTypeSerializer(expectedType) as ODataResourceSerializer;
if (serializer == null)
{
throw new SerializationException(
Error.Format(SRResources.TypeCannotBeSerialized, expectedType.ToTraceString()));
}
await serializer.WriteResourceContent(writer, selectExpandNode, resourceContext, /*isDelta*/ true);
await writer.WriteEndAsync().ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we call 'ConfigureAwait' for all await?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently always use ConfigureAwait(false) to improve perf and avoid possible deadlocks because we are a library and not an app.

}
}

/// <summary>
/// Creates the <see cref="ODataResource"/> to be written while writing this resource.
/// </summary>
/// <param name="id">The id of the Deleted Resource to be written (may be null if properties contains all key properties)</param>
/// <param name="reason">The <see cref="DeltaDeletedEntryReason"/> for the removal of the resource.</param>
/// <param name="selectExpandNode">The <see cref="SelectExpandNode"/> describing the response graph.</param>
/// <param name="resourceContext">The context for the resource instance being written.</param>
/// <returns>The created <see cref="ODataResource"/>.</returns>
public virtual ODataDeletedResource CreateDeletedResource(Uri id, DeltaDeletedEntryReason reason, SelectExpandNode selectExpandNode, ResourceContext resourceContext)
{
if (selectExpandNode == null)
{
throw Error.ArgumentNull(nameof(selectExpandNode));
}

if (resourceContext == null)
{
throw Error.ArgumentNull(nameof(resourceContext));
}

string typeName = resourceContext.StructuredType.FullTypeName();

ODataDeletedResource resource = new ODataDeletedResource
{
Id = id ?? (resourceContext.NavigationSource == null ? null : resourceContext.GenerateSelfLink(false)),
TypeName = typeName ?? "Edm.Untyped",
Properties = CreateStructuralPropertyBag(selectExpandNode, resourceContext),
Reason = reason
};

InitializeODataResource(selectExpandNode, resource, resourceContext);

string etag = CreateETag(resourceContext);
if (etag != null)
{
resource.ETag = etag;
}

// Try to add the dynamic properties if the structural type is open.
AppendDynamicPropertiesInternal(resource, selectExpandNode, resourceContext);

return resource;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System;
using System.Collections;
using System.Diagnostics.Contracts;
using System.Reflection;
using System.Runtime.Serialization;
using System.Threading.Tasks;
using Microsoft.AspNetCore.OData.Abstracts;
Expand Down Expand Up @@ -56,10 +57,6 @@ public override async Task WriteObjectAsync(object graph, Type type, ODataMessag
}

IEdmEntitySetBase entitySet = writeContext.NavigationSource as IEdmEntitySetBase;
if (entitySet == null)
{
throw new SerializationException(SRResources.EntitySetMissingDuringSerialization);
}

IEdmTypeReference feedType = writeContext.GetEdmType(graph, type);
Contract.Assert(feedType != null);
Expand Down Expand Up @@ -162,11 +159,19 @@ private async Task WriteDeltaResourceSetAsync(IEnumerable enumerable, IEdmTypeRe
}

lastResource = item;
DeltaItemKind kind = GetDelteItemKind(item);
DeltaItemKind kind = GetDeltaItemKind(item);
switch (kind)
{
case DeltaItemKind.DeletedResource:
await WriteDeltaDeletedResourceAsync(item, writer, writeContext).ConfigureAwait(false);
// hack. if the WriteDeltaDeletedResourceAsync isn't overridden, call the new version
if (WriteDeltaDeletedResourceAsyncIsOverridden())
{
await WriteDeltaDeletedResourceAsync(item, writer, writeContext).ConfigureAwait(false);
}
else
{
await WriteDeletedResourceAsync(item, elementType, writer, writeContext).ConfigureAwait(false);
}
break;
case DeltaItemKind.DeltaDeletedLink:
await WriteDeltaDeletedLinkAsync(item, writer, writeContext).ConfigureAwait(false);
Expand Down Expand Up @@ -212,7 +217,7 @@ await entrySerializer.WriteDeltaObjectInlineAsync(item, elementType, writer, wri
/// <param name="writeContext">The serializer context.</param>
/// <returns>The function that generates the NextLink from an object.</returns>
/// <returns></returns>
internal static Func<object, Uri> GetNextLinkGenerator(ODataDeltaResourceSet deltaResourceSet, IEnumerable enumerable, ODataSerializerContext writeContext)
internal static Func<object, Uri> GetNextLinkGenerator(ODataResourceSetBase deltaResourceSet, IEnumerable enumerable, ODataSerializerContext writeContext)
{
return ODataResourceSetSerializer.GetNextLinkGenerator(deltaResourceSet, enumerable, writeContext);
}
Expand Down Expand Up @@ -266,6 +271,8 @@ public virtual ODataDeltaResourceSet CreateODataDeltaResourceSet(IEnumerable fee
/// <param name="value">The object to be written.</param>
/// <param name="writer">The <see cref="ODataDeltaWriter" /> to be used for writing.</param>
/// <param name="writeContext">The <see cref="ODataSerializerContext"/>.</param>
[Obsolete("WriteDeltaDeletedResourceAsync(object, ODataWriter, ODataSerializerContext) is Deprecated and will be removed in the next version." +
"Please use WriteDeletedResourceAsync(object, IEdmEntityTypeReference, ODataWriter, ODataSerializerContext)")]
public virtual async Task WriteDeltaDeletedResourceAsync(object value, ODataWriter writer, ODataSerializerContext writeContext)
{
if (writer == null)
Expand Down Expand Up @@ -304,6 +311,27 @@ public virtual async Task WriteDeltaDeletedResourceAsync(object value, ODataWrit
}
}

/// <summary>
/// Writes the given deltaDeletedEntry specified by the parameter graph as a part of an existing OData message using the given
/// messageWriter and the writeContext.
/// </summary>
/// <param name="value">The object to be written.</param>
/// <param name="expectedType">The expected type of the deleted resource.</param>
/// <param name="writer">The <see cref="ODataDeltaWriter" /> to be used for writing.</param>
/// <param name="writeContext">The <see cref="ODataSerializerContext"/>.</param>
public virtual async Task WriteDeletedResourceAsync(object value, IEdmStructuredTypeReference expectedType, ODataWriter writer, ODataSerializerContext writeContext)
{
if (writer == null)
{
throw Error.ArgumentNull(nameof(writer));
}

//TODO:in future, use SerializerProvider -- requires differentiating between resource serializer and deleted resource serializer for same EdmType
ODataDeletedResourceSerializer deletedResourceSerializer = new ODataDeletedResourceSerializer(SerializerProvider);

await deletedResourceSerializer.WriteObjectInlineAsync(value, expectedType, writer, writeContext);
}

/// <summary>
/// Writes the given deltaDeletedLink specified by the parameter graph as a part of an existing OData message using the given
/// messageWriter and the writeContext.
Expand Down Expand Up @@ -382,7 +410,7 @@ public virtual async Task WriteDeltaLinkAsync(object value, ODataWriter writer,
}
}

internal DeltaItemKind GetDelteItemKind(object item)
internal DeltaItemKind GetDeltaItemKind(object item)
{
IEdmChangedObject edmChangedObject = item as IEdmChangedObject;
if (edmChangedObject != null)
Expand Down Expand Up @@ -414,4 +442,11 @@ private static IEdmStructuredTypeReference GetResourceType(IEdmTypeReference fee
string message = Error.Format(SRResources.CannotWriteType, typeof(ODataDeltaResourceSetSerializer).Name, feedType.FullName());
throw new SerializationException(message);
}

private bool WriteDeltaDeletedResourceAsyncIsOverridden()
{
MethodInfo method = GetType().GetMethod("WriteDeltaDeletedResourceAsync", new Type[] { typeof(object), typeof(ODataWriter), typeof(ODataSerializerContext) });
Contract.Assert(method != null, "WriteDeltaDeletedResourceAsync is not defined.");
Copy link
Member

@xuzhg xuzhg Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. can we leave a details comment for this method?
  2. Moreover, what about if customer creates/customize his own ODataDeletedResourceSerilizer? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Added comments
  2. This is only here for backward compatibility. If the user customizes their own ODataDeletedResourceSerializer, they can provide whatever custom logic they need.

return method.DeclaringType != typeof(ODataDeltaResourceSetSerializer);
}
}
Loading