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.

Sign up for GitHub

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

Jump to bottom

fixes #931: Undeclared enum type value for untype value serialized as empty resource #961

Merged
merged 2 commits into from
Aug 10, 2023
Merged

fixes #931: Undeclared enum type value for untype value serialized as empty resource #961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc052d9
fixes #931: Undeclared enum type value for untype value serialized as…
xuzhg Jun 19, 2023
b1d107a
Update the comments.
xuzhg Aug 10, 2023
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
37 changes: 34 additions & 3 deletions src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataResourceSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,7 @@ public virtual void AppendDynamicProperties(ODataResource resource, SelectExpand

IEdmStructuredType structuredType = resourceContext.StructuredType;
IEdmStructuredObject structuredObject = resourceContext.EdmObject;
ODataSerializerContext serializierContext = resourceContext.SerializerContext;
object value;
IDelta delta = structuredObject as IDelta;
if (structuredObject is EdmUntypedObject untypedObject) // NO CLR, NO EDM
Expand Down Expand Up @@ -700,10 +701,25 @@ public virtual void AppendDynamicProperties(ODataResource resource, SelectExpand
continue;
}

IEdmTypeReference edmTypeReference = resourceContext.SerializerContext.GetEdmType(dynamicPropertyValue,
dynamicPropertyValue.GetType(), true);
Type propertyType = dynamicPropertyValue.GetType();
IEdmTypeReference edmTypeReference = serializierContext.GetEdmType(dynamicPropertyValue, propertyType, true);
if (edmTypeReference == null || edmTypeReference.IsStructuredOrUntyped())
{
if (TypeHelper.IsEnum(propertyType))
{
// we don't have the Edm enum type in the model, let's write it as string.
dynamicProperties.Add(new ODataProperty
{
Name = dynamicProperty.Key,

// TBD: Shall we write the un-declared enum value as full-name string?
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this determined?

Copy link
Member Author

Choose a reason for hiding this comment

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

So far, no. @mikepizzo

Copy link
Member

Choose a reason for hiding this comment

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

Sorry; I thought we had discussed.

I am more concerned with whether or not we are able to round-trip the value than whether we use the full name or just the member name.

If we can round-trip using just the member name, then I suspect that most closely matches what the service implementor was trying to get. If we need to use the full name in order to set the value on write to be the enum value (as opposed to string), then I'm fine with 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.

In the reading, we can't say whether it's Enum member, because its literal is a 'string'. It's weird to let the customer to specify a full name in the reading scenario. So, Let me keep writing the name string to see the feedback from customers. Is it ok?

Copy link
Member

Choose a reason for hiding this comment

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

Yes; so to recap, when writing we will write just the name ("Apple", not "Namespace.EnumTypeName.Apple"). When reading we will read "Apple" and set it as a string value. If the service wants different behavior on deserialization, they can customize the deserialization logic to detect that "Apple" is actually Namespace.EnumType.Apple.

Once we start writing as "Apple", it would be a breaking change to start writing as "Namespace.EnumTypeName.Apple" (or vice-versa), so whatever we decide will be the default behavior at least for 8.x (we can add a flag to 8.x, or change behavior in 9.x or beyond, but we shouldn't unless it's a common ask that can't easily be overridden.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I will update the comments and let me know if you have any other concerns for this PR?

// So, "Data":"Apple" => should be ""Data":"Namespace.EnumTypeName.Apple" ?
Value = dynamicPropertyValue.ToString()
});

continue;
}

resourceContext.AppendDynamicOrUntypedProperty(dynamicProperty.Key, dynamicPropertyValue);
}
else
Expand All @@ -716,7 +732,7 @@ public virtual void AppendDynamicProperties(ODataResource resource, SelectExpand
}

dynamicProperties.Add(propertySerializer.CreateProperty(
dynamicPropertyValue, edmTypeReference, dynamicProperty.Key, resourceContext.SerializerContext));
dynamicPropertyValue, edmTypeReference, dynamicProperty.Key, serializierContext));
}
}

Expand Down Expand Up @@ -1388,8 +1404,23 @@ public virtual object CreateUntypedPropertyValue(IEdmStructuralProperty structur
// 1) If we can get EdmType from model, Let's use it.
// 2) If no (aka, we don't have an Edm type associated). So, let's treat it a Untyped.
actualType = writeContext.GetEdmType(propertyValue, propertyType, true);

if (actualType.IsStructuredOrUntyped())
{
if (TypeHelper.IsEnum(propertyType))
{
// we don't have the Edm enum type in the model, let's write it as string.
return new ODataProperty
{
Name = structuralProperty.Name,

// Shall we write the un-declared enum value as full-name string?
// So, "Data":"Apple" => should be ""Data":"Namespace.EnumTypeName.Apple" ?
// We keep it simple to write it as string (enum member name), not the full-name string.
Value = propertyValue.ToString()
};
}

return propertyValue;
}

Expand Down
14 changes: 14 additions & 0 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/Untyped/UntypedDataSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,20 @@ public static IList<InModelPerson> GetAllPeople()
}
},

// not in model enum
new InModelPerson
{
Id = 22, // special Id number
Name = "Yin",
Data = NotInModelEnum.Apple,
Infos = new object[] { InModelColor.Blue, InModelColor.Green, NotInModelEnum.Apple },
Containers = new Dictionary<string, object>
{
{ "EnumDynamic1", InModelColor.Blue },
{ "EnumDynamic2", NotInModelEnum.Apple },
}
},

// In model complex
new InModelPerson
{
Expand Down
28 changes: 28 additions & 0 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/Untyped/UntypedTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,34 @@ public async Task QuerySinglePeople_OnDeclaredUntypedProperty(string request, st
Assert.Equal(expected, payloadBody);
}

[Fact]
public async Task QuerySinglePeople_WithDeclaredOrUndeclaredEnum_OnUntypedAndDynamicProperty()
{
// Arrange
HttpClient client = CreateClient();

// Act
HttpResponseMessage response = await client.GetAsync("odata/people/22");

// Assert
Assert.NotNull(response);
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
Assert.NotNull(response.Content);

string payloadBody = await response.Content.ReadAsStringAsync();

Assert.Equal("{\"@odata.context\":\"http://localhost/odata/$metadata#People/$entity\"," +
"\"Id\":22," +
"\"Name\":\"Yin\"," +
"\"EnumDynamic1@odata.type\":\"#Microsoft.AspNetCore.OData.E2E.Tests.Untyped.InModelColor\"," +
"\"EnumDynamic1\":\"Blue\"," +
"\"EnumDynamic2\":\"Apple\"," +
"\"Data@odata.type\":\"#String\"," +
"\"Data\":\"Apple\"," +
"\"Infos\":[\"Blue\",\"Green\",\"Apple\"]" +
"}", payloadBody);
}

[Fact]
public async Task QuerySinglePeople_WithCollectionInCollection_OnDeclaredUntypedProperty()
{
Expand Down