Error when SkipToken contains a nullable string

[nicovos]

trafficstars

Hello,

This issue is copied from repository OData/WebAPI after I found that the fix should be made in AspNetCoreOData.

Original issue is #2041 from OData/WebApi.

I run into the same issue. So I'm to trying to implement paging functionality into my application and thus I using the odata generated next link in the result of a OData WEB API call.

"@odata.nextLink": "http://127.0.0.1:50057/odata/views/ReportTranslations?$count=true&$orderby=Code&$skiptoken=Code-%27ADDRESSLINES%27,LanguageID-2,ReportName-null,Translation-%27Addresslines%27"

As mentioned before the error occurs when null value is used in the uri. In my sample ReportName-null.

I have investigated this in de OData source code and the problem occurs in DefaultSkipTokenHandler.ApplyToCore.

Specifically this code.

foreach (KeyValuePair<string, object> item in propertyValuePairs)
            {
                string key = item.Key;
                MemberExpression property = Expression.Property(param, key);
                object value = item.Value;

                Expression compare = null;
                ODataEnumValue enumValue = value as ODataEnumValue;
                if (enumValue != null)
                {
                    value = enumValue.Value;
                }

                Expression constant = parameterizeConstant ? LinqParameterContainer.Parameterize(value.GetType(), value) : Expression.Constant(value);
                if (directionMap.ContainsKey(key) && directionMap[key] == OrderByDirection.Descending)
                {
                    compare = ExpressionBinderHelper.CreateBinaryExpression(BinaryOperatorKind.LessThan, property, constant, true, querySettings);
                }
                else
                {
                    compare = ExpressionBinderHelper.CreateBinaryExpression(BinaryOperatorKind.GreaterThan, property, constant, true, querySettings);
                }

The CreateBinaryExpression fails because the constant variable refers to ODataNull which it does not support. It does a string compare internally that expects a value or null.

The ODataNull originals from the following code.

IDictionary<string, object> propertyValuePairs = PopulatePropertyValuePairs(skipTokenRawValue, context);

Which is called prior to iterating through all skiptoken values.

I could simple solve this by calling the following code prior to calling CreateBinaryExpression.

Expression constant = null;
                if (value.GetType() == typeof(ODataNullValue))
                {
                    constant = parameterizeConstant ? LinqParameterContainer.Parameterize(typeof(string), null) : Expression.Constant(value);
                }
                else
                    constant = parameterizeConstant ? LinqParameterContainer.Parameterize(value.GetType(), value) : Expression.Constant(value);

So basically I'm converting ODataNull to a null string which the CreateBinaryExpression support.

I know this is not a clean solution and more like a quick fix, because I'm assuming the data type to be a string. I would prefer a change to PopulatePropertyValuePairs to also return the data type of each skiptoken value. It checks the EdmModel so it has the information. Having the type the proper Expression constant variable could be created. The result is the same.

Also I would prefer to have this fixed in the OData code and added to the next release. It does not seem to be a big change. So would be great if this could be picked up.

As a temporary solution I'm busy implementing my own version of a SkipTokenHandler. Using dependency injection I can add my own implementation but in order to fix the code I have to basically copy a lot of code for a fairly simple code change. I cannot simple copy DefaultSkipTokenHandler and change the code, A lot of the dependent code is internal. So I have to copy a lot of classes let only access to the resource file containing the error messages.

Originally posted by @nicovos in https://github.com/OData/WebApi/issues/2041#issuecomment-1067764917

[nicovos]

Hello,

@corranrogue9, @Nthemba here is my changed code in DefaultSkipTokenHandler that fixed it for me.

public class DefaultSkipTokenHandler : SkipTokenHandler
{
        ...

        internal class PropertyValue
        {
            public Type Type { get; set; }
            public object Value { get; set; }
            public bool IsNull { get; set; }
        }

        ...

        internal static IDictionary<string, PropertyValue> PopulatePropertyValuePairs(string value, ODataQueryContext context)
        {
            Contract.Assert(context != null);

            IDictionary<string, PropertyValue> propertyValuePairs = new Dictionary<string, PropertyValue>();
            IList<string> keyValuesPairs = ParseValue(value, CommaDelimiter);

            IEdmStructuredType type = context.ElementType as IEdmStructuredType;
            Debug.Assert(type != null);

            foreach (string pair in keyValuesPairs)
            {
                string[] pieces = pair.Split(new char[] { propertyDelimiter }, 2);
                if (pieces.Length > 1 && !String.IsNullOrWhiteSpace(pieces[0]))
                {
                    object propValue = null;

                    IEdmTypeReference propertyType = null;
                    IEdmProperty property = type.FindProperty(pieces[0]);
                    Type propertyClrType = null;
                    if (property != null)
                    {
                        propertyType = property.Type;
                        propertyClrType = context.Model.GetClrType(propertyType);
                    }

                    propValue = ODataUriUtils.ConvertFromUriLiteral(pieces[1], ODataVersion.V401, context.Model, propertyType);
                    if (propValue.GetType() == typeof(ODataNullValue))
                        propertyValuePairs.Add(pieces[0], new PropertyValue() { Type = propertyClrType, Value = null, IsNull = true });
                    else
                        propertyValuePairs.Add(pieces[0], new PropertyValue() { Type = propertyClrType, Value = propValue, IsNull = false });
                }
                else
                {
                    throw new ODataException(Error.Format(SRResources.SkipTokenParseError, value));
                }
            }

            return propertyValuePairs;
        }

        private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings querySettings, IList<OrderByNode> orderByNodes, ODataQueryContext context, string skipTokenRawValue)
        {
             ...

            IDictionary<string, PropertyValue> propertyValuePairs = PopulatePropertyValuePairs(skipTokenRawValue, context);

             ...

            foreach (KeyValuePair<string, PropertyValue> item in propertyValuePairs)
            {
                string key = item.Key;
                MemberExpression property = Expression.Property(param, key);
                object value = item.Value.Value;

                Expression compare = null;
                ODataEnumValue enumValue = value as ODataEnumValue;
                if (enumValue != null)
                {
                    value = enumValue.Value;
                }

                Expression constant = constant = parameterizeConstant ? LinqParameterContainer.Parameterize(item.Value.Type, item.Value.Value) : Expression.Constant(value);
                if (directionMap.ContainsKey(key) && directionMap[key] == OrderByDirection.Descending)
                {
                    compare = ExpressionBinderHelper.CreateBinaryExpression(BinaryOperatorKind.LessThan, property, constant, true, querySettings);
                }
                else
                {
                    compare = ExpressionBinderHelper.CreateBinaryExpression(BinaryOperatorKind.GreaterThan, property, constant, true, querySettings);
                }

             ...

So basically I changed PopulatePropertyValuePairs to return an instance of PropertyValue.

When creating the Expression, I can get the type and value that is required for the string compare.

[EmanH]

@nicovos We've had the same issue, in our case a nullable int? @corranrogue9 , @Nthemba any update on when this might be looked at?

[EmanH]

@nicovos could you open a pull request with your fix?

[nicovos]

Hello @EmanH I'm planning to open a pull request in the near future. Just have to find some time to do it and find out how to do it.

Just for your information. Basically the nullable fields are not implemented correctly. I had to apply some additional fixes to get it working. I found out that SQL queries where being created that had "field < NULL" in them. This caused less records being returned. Also fields of type timestamp (byte[] ) and boolean failed.

I had to change three files. ExpressionBinderHelper.cs, Linq2ObjectsComparisonMethods.cs and DefaultSkipTokenHandler.cs I have added a zip files containing these files. Maybe this will help you.

Files.zip .