Fix for Error occurs when you check whether a string or integer literal is in an enum collection

[WanjohiSammy]

Issues

This pull request fixes #2371.

Description

This change is to fix the issue raised where the in operator does not allow using a string or integer literal as left operand when comparing against a collection of enums.

The change involves:

• Checking if the left operand is either an integral or a string type and the right operand is a collection of enums.
• If this is true, it calls MetadataBindingUtils.ConvertToTypeIfNeeded() method to try convert the left operand to the same enum type if possible.
• If this is not possible, exception is thrown.

Assume you have the following entity:

public class Employee 
{
    public int Id { get; set; }
    public string Name { get; set; }
    public List<WeekDay> WorkingDays { get; set; }
}

public enum WeekDay
{
    Monday = 1,
    Tuesday = 2,
    Wednesday = 3,
    Thursday = 4,
    Friday = 5,
    Saturday = 6,
    Sunday = 7
}

This change enables support of the following scenarios for $filter with In Operator:

// left operand without fully qualified namespace
/Employees?$filter='Monday' in WorkingDays

// left operand with integral value in single quotes
/Employees?$filter='1' in WorkingDays

// left operand with integral value without single quotes
/Employees?$filter=1 in WorkingDays

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.

[ElizabethOkerio]

// left operand with fully qualified namespace
/Employees?$filter=Fully.Qualified.Namespace'Monday' in WorkingDays

// left operand without fully qualified namespace
/Employees?$filter='Monday' in WorkingDays

// left operand with integral value in single quotes
/Employees?$filter='1' in WorkingDays

// left operand with integral value without single quotes
/Employees?$filter=1 in WorkingDays

From the issue description, the first and last scenarios are currently supported..is that true? What this PR is supporting is the second and third scenario?

[WanjohiSammy]

// left operand with fully qualified namespace
/Employees?$filter=Fully.Qualified.Namespace'Monday' in WorkingDays

// left operand without fully qualified namespace
/Employees?$filter='Monday' in WorkingDays

// left operand with integral value in single quotes
/Employees?$filter='1' in WorkingDays

// left operand with integral value without single quotes
/Employees?$filter=1 in WorkingDays

From the issue description, the first and last scenarios are currently supported..is that true? What this PR is supporting is the second and third scenario?

@ElizabethOkerio Currently, we are supporting only the 1st scenario. This PR is to support 2nd, 3rd and 4th scenarios. I have edited the description.

[habbes]

Is the cause of the exception the fact that Red in the filterQuery is not in single quotes? If so, why does it throw a null reference exception? Is that by design or a bug? Should it throw a friendlier error message that states what the error is? (e.g. Red is not a known property or is not of the correct type or whatever the appropriate error should be)?

[WanjohiSammy]

@habbes Thanks for bringing this up. This was happening because the EdmEntityType for testing was set to be open. In case where isOpen is false, the exception thrown is: "Could not find a property named '{1}' on type '{0}'."

internal SingleValueOpenPropertyAccessNode GeneratePropertyAccessQueryForOpenType(
EndPathToken endPathToken, SingleValueNode parentNode)
{
    if (parentNode.TypeReference == null ||
        parentNode.TypeReference.Definition.IsOpen() ||
        IsAggregatedProperty(endPathToken))
    {
        return new SingleValueOpenPropertyAccessNode(parentNode, endPathToken.Identifier);
    }
    else
    {
        throw new ODataException(ODataErrorStrings.MetadataBinder_PropertyNotDeclared(
            parentNode.TypeReference.FullName(),
            endPathToken.Identifier));
    }
}

I have modified the isOpen to false

this.entityType = new EdmEntityType("NS", "MyEntityType", isAbstract: false, isOpen: false, baseType: null);

[habbes]

Okay that makes sense. But it seems like there are two issues. This has not addressed the issue when we have an open type, which I assume would still throw a null-reference exception. I'd still be curious about that, because it's possible that customers are using open types. I guess the challenge is that we may not know the type. But regardless, if it's an error, it should throw a meaningful error, not a null-reference exception.

[WanjohiSammy]

@habbes That seems like an existing issue. Even if we remove this fix, we will still get the same error. If you pass Red without single quotes, it will be treated like a property and hence the error when trying to check if left node can be assignableFrom right node. This is happening because the left node does not exist as a property and therefore has a null EdmTypeReference:

This image shows where the error is thrown:

I found that this is where the issue is in the case of In Operator:

if (!this.left.GetEdmTypeReference().IsAssignableFrom(this.right.ItemType) &&
!this.right.ItemType.IsAssignableFrom(this.left.GetEdmTypeReference()))

In the above code, this.left.GetEdmTypeReference() is always null for this example: ?$filter=Red in Colors.

public InNode(SingleValueNode left, CollectionNode right)
{
    ExceptionUtils.CheckArgumentNotNull(left, "left");
    ExceptionUtils.CheckArgumentNotNull(right, "right");
    this.left = left;
    this.right = right;

    if (!right.GetEdmTypeReference().IsUntyped())
    {
        if (!this.left.GetEdmTypeReference().IsAssignableFrom(this.right.ItemType) &&
            !this.right.ItemType.IsAssignableFrom(this.left.GetEdmTypeReference()))
        {
            throw new ArgumentException(ODataErrorStrings.Nodes_InNode_CollectionItemTypeMustBeSameAsSingleItemType(
                this.right.ItemType.FullName(), this.left.GetEdmTypeReference().FullName()));
        }
    }
}

[WanjohiSammy]

@habbes Unless the spec permit, we can try to convert the left operand Red to the same enum type if possible. Hence, to support the following. But this will only work if the EdmEntityType is open.

/Employees?$filter=Red in Colors

The left node will be converted to ConvertNode() since the left.TypeReference == null

internal static SingleValueNode ConvertToTypeIfNeeded(SingleValueNode source, IEdmTypeReference targetTypeReference)
{
    Debug.Assert(source != null, "source != null");

    if (targetTypeReference == null)
    {
        return source;
    }

    if (source.TypeReference != null)
    {
        ...
    }
    else
    {
        // If the source doesn't have a type (possibly an open property), then it's possible to convert it
        // cause we don't know for sure.
        return new ConvertNode(source, targetTypeReference);
    }
}

[habbes]

Since this PR was specific about the literals, may no need to add support for type coercions of non-literals. I was just concerned about the error message.