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 Default ODataEnumSerializer and ODataEnumDeserializer Fails to Convert Multi-Value Flag Enum Values to Lower Camel Case #1367

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

WanjohiSammy
Copy link
Contributor

@WanjohiSammy WanjohiSammy commented Dec 4, 2024

Issues

This pull request fixes #1359.

Description

Currently, the ODataEnumSerializer and ODataEnumDeserializer fails to convert multi-value Flag enums to LowerCamelCase as specified in EnumMember attribute. However, this works with single-value Flag enums.

Example:

Consider the following Flag enum:

[Flags]
[DataContract(Name = "exampleEnum")]
public enum ExampleEnum
{
    [EnumMember(Value = "firstValue")]
    FirstValue = 1,

    [EnumMember(Value = "secondValue")]
    SecondValue = 2,

    [EnumMember(Value = "thirdValue")]
    ThirdValue = 4
}

With the default ODataEnumSerializer, a single-value Flag enum like ExampleEnum.FirstValue is correctly converted to ("firstValue"). However, a multi-value Flag enum like ExampleEnum.FirstValue | ExampleEnum.SecondValue is not converted to ("firstValue, secondValue") as expected. Instead, it remains in its original form as "FirstValue, SecondValue"

This change modifies both ODataEnumSerializer and ODataEnumDeserializer to correctly convert multi-value Flag enums to LowerCamelCase. This is to ensure that the serialization and deserialization of multi-value Flag enums are consistent with the single-value Flag enums.

With this change, the following are supported by default without having to write custom converters:

  • GET odata/examples:

    {
     "@odata.context": "https://localhost:7085/odata/$metadata#Examples",
     "value": [
       {
         "id": 1,
         "name": "Example1",
         "ExampleEnum": "firstValue"
       },
       {
         "id": 2,
         "name": "Example2",
         "ExampleEnum": "firstValue, secondValue" // support write-to multi-value Flags enums to Lower Camel Case
       },
       ...
     ]
    }
  • POST odata/examples:

    {
      "Id": 0,
      "Name": "John Doe Example",
      "ExampleEnum": "firstValue, thirdValue" // support read-in multi-value Flags enums in Lower Camel Case
    }
  • PATCH odata/examples(1):

    {
        "ExampleEnum": "firstValue, thirdValue" // support read-in multi-value Flags enums in Lower Camel Case
    }

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 8 changed files in this pull request and generated no suggestions.

@WanjohiSammy WanjohiSammy requested a review from Copilot December 4, 2024 18:38

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 8 changed files in this pull request and generated no suggestions.

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 8 changed files in this pull request and generated no suggestions.

Files not reviewed (3)
  • src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml: Language not supported
  • test/Microsoft.AspNetCore.OData.E2E.Tests/Enums/EnumsController.cs: Evaluated as low risk
  • test/Microsoft.AspNetCore.OData.E2E.Tests/Enums/EnumsEdmModel.cs: Evaluated as low risk
Comments skipped due to low confidence (1)

test/Microsoft.AspNetCore.OData.Tests/Formatter/Deserialization/ODataEnumDeserializerTests.cs:321

  • There is a duplicate assertion for expectedAccessLevelValue. The second assertion should be for employeeTypeValue instead.
Assert.Equal(expectedAccessLevelValue, accessLevel);
@WanjohiSammy WanjohiSammy requested a review from Copilot December 5, 2024 09:59

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 8 changed files in this pull request and generated no suggestions.


// For flags enum, we need to split the value and convert it to the enum value.
int count = source.Split(ranges, ',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries);
for(int i = 0; i < count; i++)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could consider having a single loop that also handle the split of the strings, this also avoids the need to allocate the ranges array:

ReadOnlySpan<char> source = enumValue.AsSpan();
int currentRangeStart = 0;
for (int i = 0; i , source.Length; i++)
{
    if (source[i] != ',')
    {
       continue;
    }
    ReadOnlySpan<char> rawValue = source[currentRangeStart..i];
    ReadOnlySpan<char> value = rawValue.Trim();
    currentRangeStart = i;
   // handle value
}

// handle the last value
ReadOnlySpan<char> rawValue = source[currentRangeStart..];
ReadOnlySpan<char> value = rawValue.Trim();

PS: I didn't test it, verify for correctness. But the idea is we can get the ranges and the value in single scan. The Trim() is probably unnecessary if we check for spaces as well, but didn't want to complicate it too match especially since the Trim() is non-allocating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refer to: #1367 (comment)

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml: Language not supported
  • test/Microsoft.AspNetCore.OData.Tests/Formatter/Deserialization/ODataEnumDeserializerTests.cs: Evaluated as low risk
  • test/Microsoft.AspNetCore.OData.E2E.Tests/Enums/EnumsController.cs: Evaluated as low risk
@WanjohiSammy WanjohiSammy requested a review from Copilot December 13, 2024 10:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 8 changed files in this pull request and generated 1 comment.

Files not reviewed (3)
  • src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml: Language not supported
  • src/Microsoft.AspNetCore.OData/Formatter/Deserialization/ODataEnumDeserializer.cs: Evaluated as low risk
  • src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataEnumSerializer.cs: Evaluated as low risk

@WanjohiSammy WanjohiSammy requested a review from Copilot December 14, 2024 18:14
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 8 changed files in this pull request and generated 1 comment.

Files not reviewed (3)
  • src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml: Language not supported
  • test/Microsoft.AspNetCore.OData.E2E.Tests/Enums/EnumsController.cs: Evaluated as low risk
  • test/Microsoft.AspNetCore.OData.E2E.Tests/Enums/EnumsEdmModel.cs: Evaluated as low risk
Comments suppressed due to low confidence (4)

src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataEnumSerializer.cs:104

  • Add a null check for memberMapAnnotation in the CreateODataEnumValue method to prevent a potential null reference exception.
if (memberMapAnnotation != null)

src/Microsoft.AspNetCore.OData/Formatter/Deserialization/ODataEnumDeserializer.cs:162

  • Use StringComparison.OrdinalIgnoreCase instead of StringComparison.InvariantCultureIgnoreCase for better performance and consistency.
parsed = currentValue.Equals(enumMember.Name.AsSpan(), StringComparison.InvariantCultureIgnoreCase);

test/Microsoft.AspNetCore.OData.E2E.Tests/Enums/EnumsDataModel.cs:45

  • [nitpick] The enum member value 'full time' is inconsistently cased compared to other values like 'Part Time'. Consider using a consistent casing convention.
[EnumMember(Value = "full time")]

test/Microsoft.AspNetCore.OData.E2E.Tests/Enums/EnumsDataModel.cs:48

  • [nitpick] The enum member value 'Part Time' is inconsistently cased compared to other values like 'full time'. Consider using a consistent casing convention.
[EnumMember(Value = "Part Time")]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants