Error when SkipToken contains a nullable string

[Grauenwolf]

If sorting by a nullable string, $skiptoken may contain a null value. This causes an exception show below.

Example URL:

https://localhost:44322/odata/TransactionExceptionDetails?$select=TxnId%2CContactCity%2CContactState&$orderby=ContactCity%2CTxnId&$skiptoken=ContactCity-null,TxnId-12005

Note the use of $orderby=ContactCity%2CTxnId and $skiptoken=ContactCity-null,TxnId-12005.

If I remove ContactCity from the order by, then the generated skiptoken doesn't contain it either and I don't get an error.

Assemblies affected

    <PackageReference Include="Microsoft.AspNetCore.OData" Version="7.3.0" />
    <PackageReference Include="Microsoft.EntityFrameworkCore" Version="3.1.1" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="3.1.1" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Tools" Version="3.1.1">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
    <PackageReference Include="Microsoft.Extensions.Logging.Debug" Version="3.1.0" />
    <PackageReference Include="Microsoft.VisualStudio.Web.CodeGeneration.Design" Version="3.1.0" />

Reproduce steps

The simplest set of steps to reproduce the issue. If possible, reference a commit that demonstrates the issue.

Expected result

What would happen if there wasn't a bug.

Actual result

ArgumentException: Expression of type 'Microsoft.OData.ODataNullValue' cannot be used for parameter of type 'System.String' of method 'Int32 Compare(System.String, System.String, System.StringComparison)' (Parameter 'arg1')


System.Dynamic.Utils.ExpressionUtils.ValidateOneArgument(MethodBase method, ExpressionType nodeKind, Expression arguments, ParameterInfo pi, string methodParamName, string argumentParamName, int index)
System.Linq.Expressions.Expression.Call(MethodInfo method, Expression arg0, Expression arg1, Expression arg2)
Microsoft.AspNet.OData.Query.Expressions.ExpressionBinderBase.CreateBinaryExpression(BinaryOperatorKind binaryOperator, Expression left, Expression right, bool liftToNull)
Microsoft.AspNet.OData.Query.DefaultSkipTokenHandler.ApplyToCore(IQueryable query, ODataQuerySettings querySettings, IList<OrderByNode> orderByNodes, ODataQueryContext context, string skipTokenRawValue)
Microsoft.AspNet.OData.Query.DefaultSkipTokenHandler.ApplyTo(IQueryable query, SkipTokenQueryOption skipTokenQueryOption)
Microsoft.AspNet.OData.Query.SkipTokenQueryOption.ApplyTo(IQueryable query, ODataQuerySettings querySettings, ODataQueryOptions queryOptions)
Microsoft.AspNet.OData.Query.ODataQueryOptions.ApplyTo(IQueryable query, ODataQuerySettings querySettings)
Microsoft.AspNet.OData.EnableQueryAttribute.ApplyQuery(IQueryable queryable, ODataQueryOptions queryOptions)
Microsoft.AspNet.OData.EnableQueryAttribute.ExecuteQuery(object responseValue, IQueryable singleResultCollection, IWebApiActionDescriptor actionDescriptor, Func<Type, IEdmModel> modelFunction, IWebApiRequestMessage request, Func<ODataQueryContext, ODataQueryOptions> createQueryOptionFunction)
Microsoft.AspNet.OData.EnableQueryAttribute.OnActionExecuted(object responseValue, IQueryable singleResultCollection, IWebApiActionDescriptor actionDescriptor, IWebApiRequestMessage request, Func<Type, IEdmModel> modelFunction, Func<ODataQueryContext, ODataQueryOptions> createQueryOptionFunction, Action<HttpStatusCode> createResponseAction, Action<HttpStatusCode, string, Exception> createErrorAction)
Microsoft.AspNet.OData.EnableQueryAttribute.OnActionExecuted(ActionExecutedContext actionExecutedContext)
Microsoft.AspNetCore.Mvc.Filters.ActionFilterAttribute.OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, object state, bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|19_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, object state, bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext)
Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

Additional detail

[marwalsch]

@KanishManuja-MS I can observe the same behaviour in OData 7.5.8 on .NET 5.

[nicovos]

Hello @KanishManuja-MS,

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.