Fix parsing issue for Flags enum values with combined flags by WanjohiSammy · Pull Request #3324 · OData/odata.net

WanjohiSammy · 2025-08-05T18:11:00Z

Issues

This pull request fixes #3244, fixes #3226, fixes #3178

Description

OData queries fail when filtering against Flags enum properties using combined enum values, whether specified as string representations ('Premium,Loyal') or numeric equivalents (66).

The current implementation in MetadataBindingUtils.cs does not properly handle:

Combined flag values in string format (e.g., 'Premium,Loyal')
Numeric representations of combined flags (e.g., 66 for Premium | Loyal)

Changes Made

Enhanced enum value parsing logic in MetadataBindingUtils.cs
Added support for comma-separated flag combinations in string format
Implemented numeric value parsing for combined flags
Updated validation logic to recognize valid flag combinations

Query Patterns Now Supported

/customers?filter=Type eq 'Premium,Loyal'
/customers?filter=Type eq 66
/customers?filter=Type in ('Premium', 'VIP,Regular,Returning')
/customers?filter=Type in (12, 44)
/customers?filter=Type in ('12', '44')

OData ABNF rules for enums

https://docs.oasis-open.org/odata/odata/v4.01/os/abnf/odata-abnf-construction-rules.txt

Rule 1: Enum literals follow the format qualifiedEnumTypeName'SQUOTE enumValue SQUOTE.
- qualifiedEnumTypeName (e.g., Namespace.Color) is optional.
- Example: Namespace.Color'Blue' or 'Blue'.
Rule 2: enumValue can be a single value or multiple comma-separated values (for flags).
- Examples: 'Blue', 'Red,Green'.
Rule 3: Each singleEnumValue can be a named member (Red) or a numeric value (4).
- Supports unknown or bitwise values.
Rule 4: Numeric values must be valid int64.
- Example: '8' or 8.

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.

Repository notes

Team members can start a CI build by adding a comment with the text /AzurePipelines run to a PR. A bot may respond indicating that there is no pipeline associated with the pull request. This can be ignored if the build is triggered.

Team members should not trigger a build this way for pull requests coming from forked repositories. They should instead trigger the build manually by setting the "branch" to refs/pull/{prId}/merge where {prId} is the ID of the PR.

WanjohiSammy · 2025-08-05T18:34:19Z

/AzurePipelines run

azure-pipelines · 2025-08-05T18:34:25Z

No pipelines are associated with this pull request.

WanjohiSammy · 2025-08-07T09:28:13Z

/AzurePipelines run

azure-pipelines · 2025-08-07T09:28:19Z

No pipelines are associated with this pull request.

WanjohiSammy · 2025-08-25T10:43:35Z

/AzurePipelines run

azure-pipelines · 2025-08-25T10:43:42Z

No pipelines are associated with this pull request.

src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs

WanjohiSammy · 2025-10-08T08:11:10Z

/AzurePipelines run

src/Microsoft.OData.Core/EdmExtensionMethods.cs

xuzhg · 2025-10-22T20:57:47Z

src/Microsoft.OData.Core/EdmExtensionMethods.cs

+        {
+            // Sort members by descending flag value to prioritize composite flags
+            var members = enumType.Members
+                .OrderByDescending(m => m.Value.Value);


OrderBy is necessary? I cannot see any advantage to use the orderred members?

@xuzhg Thanks for pointing this out.

The use of OrderByDescending is necessary for correct flag parsing when dealing with composite enum values. This ensures the output string uses the most specific (composite) flag names when possible. Removing the ordering would break this logic for flags enums with composite members.

Reasoning

Flags enums can have members that represent combinations of other members (composite flags). For example:

Read = 1

Write = 2

ReadWrite = 3 (which is Read | Write)

If you process members in ascending order, you may match and consume the individual flags (Read, Write) before considering the composite flag (ReadWrite). This would prevent the composite flag from ever being matched.

By sorting in descending order, composite flags (with higher values) are considered first. This ensures that the largest possible flag combinations are matched before their individual components.

Example

Suppose value = 3 and members are:

Read = 1

Write = 2

ReadWrite = 3

If you process in ascending order:

Match Read (1), remaining = 2

Match Write (2), remaining = 0

ReadWrite (3) is never matched

If you process in descending order:
• Match ReadWrite (3), remaining = 0
• Done

Can we make the 'Members' orderred always then we don't need to do order again and again?

@xuzhg This is possible. I have made the change here https://github.com/OData/odata.net/pull/3324/files#diff-bf7c2bf531a2d14a23a047276ee7fd40e4b2d6e5b0a1a2a550ba9c531163081d

src/Microsoft.OData.Core/EdmExtensionMethods.cs

WanjohiSammy · 2025-10-28T07:33:44Z

/AzurePipelines run

WanjohiSammy · 2025-10-28T07:47:47Z

/AzurePipelines run

WanjohiSammy · 2025-10-28T08:03:47Z

/AzurePipelines run

gathogojr · 2025-11-11T08:26:12Z

...ts/Client/Microsoft.OData.Client.E2E.Tests/EnumerationTypeTests/EnumerationTypeQueryTests.cs

+        Assert.Equal((AccessLevel.Write | AccessLevel.Execute | AccessLevel.Read), enumResult[0]);
+        Assert.Equal((AccessLevel.ReadWrite | AccessLevel.Execute), enumResult[0]);


I'm not sure I understand why 2 assert statements

Same here as https://github.com/OData/odata.net/pull/3324/files#r2513325252

src/Microsoft.OData.Core/UriParser/Binders/MetadataBindingUtils.cs

src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs

src/Microsoft.OData.Core/EdmExtensionMethods.cs

gathogojr · 2025-11-11T08:58:56Z

src/Microsoft.OData.Core/EdmExtensionMethods.cs

+            var result = new List<string>();
+            long remaining = value;
+
+            for (int index = enumType.Members.Count() - 1; index >= 0; index--)


Calling Members.Count() and Members.ElementAt(index) in a loop can turn this into an O(n^2) operation.
I'd suggest you first call .ToList():

List<IEdmEnumMember> members = enumType.Members.ToList();

After doing the above, you can also avoid the string.Reverse call you're doing at the end by iterating the elements in the order you intend to have them in.

Suppose value = 3 and members are:

Read = 1

Write = 2

ReadWrite = 3

If you process in ascending order:

Match Read (1), remaining = 2

Match Write (2), remaining = 0

ReadWrite (3) is never matched

If you process in descending order:

Match ReadWrite (3), remaining = 0

Done

In general, iterating in declaration order would allow us to avoid the Reverse call. However, for flag enums that include composite members (e.g., ReadWrite = 3), processing in ascending order would cause the individual flags (Read = 1, Write = 2) to consume the bits before the composite member is checked, so the composite would never match.

By iterating in descending order (from highest to lowest value), we ensure that composite members are matched first, which is necessary for correct flag decomposition. The Reverse at the end is then required to present the result in declaration order. This approach ensures correct handling for all flag enum scenarios, including those with composite members.

src/Microsoft.OData.Core/EdmExtensionMethods.cs

WanjohiSammy · 2025-11-11T10:23:18Z

/AzurePipelines run

…ith comma

WanjohiSammy · 2025-11-11T14:00:15Z

/AzurePipelines run

src/Microsoft.OData.Core/EdmExtensionMethods.cs

Co-authored-by: John Gathogo <john.gathogo@microsoft.com>

WanjohiSammy · 2025-11-11T15:47:58Z

/AzurePipelines run

WanjohiSammy · 2025-11-11T16:21:09Z

/AzurePipelines run

gathogojr · 2025-11-11T18:19:35Z

src/Microsoft.OData.Core/EdmExtensionMethods.cs

+            // Iterate members in reverse order to match higher-value flags first.
+            for (int index = members.Count - 1; index >= 0; index--)
+            {
+                ulong flagValue = Convert.ToUInt64(members[index].Value.Value);


I don't think the logic here is right. I think it should be:

Suggested change

ulong flagValue = Convert.ToUInt64(members[index].Value.Value);

ulong flagValue = unchecked((ulong)Convert.ToInt64(members[index].Value.Value));

Illustration:

string negativeIntAsString = "-1"; Convert.ToUInt64(negativeIntAsString); // throws overflow exception unchecked((ulong)Convert.ToInt64(negativeIntAsString)) // 18446744073709551615

I'd actually suggest putting the entire block in an unchecked block because of the other operations involving the ulong values

unchecked { long remaining = (ulong)value; // Iterate members in reverse order to match higher-value flags first. for (int index = members.Count - 1; index >= 0; index--) { ulong flagValue = (ulong)Convert.ToInt64(members[index].Value.Value); if (flagValue != 0 && (remaining & flagValue) == flagValue) { result.Add(members[index].Name); remaining &= ~flagValue; // Remove matched bits } } }

The method ParseFlagsFromIntegralValue uses long instead of ulong for its value parameter and internal calculations. This is intentional because the IEdmEnumMemberValue.Value property is defined as long, ensuring type consistency throughout the method.

The core logic works as follows:

The input value (of type long) is assigned to a variable called remaining.

The method iterates through all enum members, checking if each member’s value is present in the remaining bits of the input.

For each matching flag, the corresponding member name is added to the result, and its value is subtracted from remaining using bitwise operations.

This process continues until all set flags are accounted for, or until no more matches are possible.

By using long, the method avoids unnecessary type conversions and aligns with the EDM enum member value type, ensuring correctness when reducing the flag values.

WanjohiSammy · 2025-11-11T19:02:30Z

/AzurePipelines run

* Add support for Flags enum * Resolve for composite flags and enableCaseInsensitive * Normalize Enum Collection Items --------- Co-authored-by: John Gathogo <john.gathogo@microsoft.com>

WanjohiSammy marked this pull request as ready for review August 7, 2025 09:55

WanjohiSammy requested review from ElizabethOkerio, gathogojr, habbes and xuzhg and removed request for xuzhg August 7, 2025 09:55

WanjohiSammy force-pushed the fix/3244-support-flags-enum branch from 8e03a95 to 2bfab4c Compare August 23, 2025 17:23

gathogojr reviewed Sep 10, 2025

View reviewed changes

src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs Outdated Show resolved Hide resolved

gathogojr reviewed Sep 10, 2025

View reviewed changes

src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs Outdated Show resolved Hide resolved

gathogojr reviewed Sep 10, 2025

View reviewed changes

src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs Outdated Show resolved Hide resolved

gathogojr reviewed Sep 10, 2025

View reviewed changes

src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs Outdated Show resolved Hide resolved

xuzhg reviewed Oct 22, 2025

View reviewed changes

src/Microsoft.OData.Core/EdmExtensionMethods.cs Outdated Show resolved Hide resolved

xuzhg reviewed Oct 22, 2025

View reviewed changes

src/Microsoft.OData.Core/EdmExtensionMethods.cs Outdated Show resolved Hide resolved

xuzhg reviewed Oct 22, 2025

View reviewed changes

src/Microsoft.OData.Core/EdmExtensionMethods.cs Show resolved Hide resolved

xuzhg previously approved these changes Oct 22, 2025

View reviewed changes

WanjohiSammy dismissed xuzhg’s stale review via 852ac65 October 28, 2025 07:29

WanjohiSammy requested review from gathogojr and xuzhg October 28, 2025 07:33

gathogojr reviewed Nov 11, 2025

View reviewed changes

src/Microsoft.OData.Core/UriParser/Binders/MetadataBindingUtils.cs Show resolved Hide resolved

gathogojr reviewed Nov 11, 2025

View reviewed changes

src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs Outdated Show resolved Hide resolved

gathogojr reviewed Nov 11, 2025

View reviewed changes

src/Microsoft.OData.Core/EdmExtensionMethods.cs Outdated Show resolved Hide resolved

gathogojr reviewed Nov 11, 2025

View reviewed changes

src/Microsoft.OData.Core/EdmExtensionMethods.cs Show resolved Hide resolved

gathogojr reviewed Nov 11, 2025

View reviewed changes

src/Microsoft.OData.Core/EdmExtensionMethods.cs Show resolved Hide resolved

WanjohiSammy added 2 commits November 11, 2025 12:16

add tests that throws

171e219

handle 0 if present

01d083f

WanjohiSammy added 2 commits November 11, 2025 16:21

Add more tests and ensure valid memberName should not start or ends w…

4e69342

…ith comma

Add more E2E tests

84e269b

gathogojr reviewed Nov 11, 2025

View reviewed changes

src/Microsoft.OData.Core/EdmExtensionMethods.cs Outdated Show resolved Hide resolved

gathogojr reviewed Nov 11, 2025

View reviewed changes

src/Microsoft.OData.Core/EdmExtensionMethods.cs Outdated Show resolved Hide resolved

WanjohiSammy and others added 2 commits November 11, 2025 18:47

Update src/Microsoft.OData.Core/EdmExtensionMethods.cs

7e44588

Co-authored-by: John Gathogo <john.gathogo@microsoft.com>

Update src/Microsoft.OData.Core/EdmExtensionMethods.cs

bc0eb18

Co-authored-by: John Gathogo <john.gathogo@microsoft.com>

removing space after ,

4fdbd79

WanjohiSammy requested a review from gathogojr November 11, 2025 16:40

gathogojr reviewed Nov 11, 2025

View reviewed changes

use long instead of ulong

43293b9

gathogojr approved these changes Nov 12, 2025

View reviewed changes

WanjohiSammy merged commit 74b1e45 into main Nov 12, 2025
2 checks passed

WanjohiSammy deleted the fix/3244-support-flags-enum branch November 12, 2025 07:12

lakshithaw99x mentioned this pull request Feb 24, 2026

Microsoft.OData.Core 8.4.3: in() with nullable enum and null throws The string 'null' is not a valid enumeration type constant. #3491

Open

		Assert.Equal((AccessLevel.Write \| AccessLevel.Execute \| AccessLevel.Read), enumResult[0]);
		Assert.Equal((AccessLevel.ReadWrite \| AccessLevel.Execute), enumResult[0]);

	ulong flagValue = Convert.ToUInt64(members[index].Value.Value);
	ulong flagValue = unchecked((ulong)Convert.ToInt64(members[index].Value.Value));

Conversation

WanjohiSammy commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues

Description

Changes Made

Query Patterns Now Supported

OData ABNF rules for enums

Checklist (Uncheck if it is not completed)

Additional work necessary

Repository notes

Uh oh!

WanjohiSammy commented Aug 5, 2025

Uh oh!

azure-pipelines bot commented Aug 5, 2025

Uh oh!

WanjohiSammy commented Aug 7, 2025

Uh oh!

azure-pipelines bot commented Aug 7, 2025

Uh oh!

WanjohiSammy commented Aug 25, 2025

Uh oh!

azure-pipelines bot commented Aug 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WanjohiSammy commented Oct 8, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Reasoning

Example

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WanjohiSammy Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

WanjohiSammy commented Oct 28, 2025

Uh oh!

WanjohiSammy commented Oct 28, 2025

Uh oh!

WanjohiSammy commented Oct 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

WanjohiSammy commented Nov 11, 2025

Uh oh!

WanjohiSammy commented Nov 11, 2025

Uh oh!

Uh oh!

Uh oh!

WanjohiSammy commented Nov 11, 2025

Uh oh!

WanjohiSammy commented Nov 11, 2025

Uh oh!

gathogojr Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WanjohiSammy commented Aug 5, 2025 •

edited

Loading

WanjohiSammy Nov 7, 2025 •

edited

Loading

gathogojr Nov 11, 2025 •

edited

Loading