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

Custom Method with nullable properties #1084

Open
bjelbo opened this issue Nov 3, 2023 · 12 comments · May be fixed by #1090 or #1374
Open

Custom Method with nullable properties #1084

bjelbo opened this issue Nov 3, 2023 · 12 comments · May be fixed by #1090 or #1374
Assignees
Labels
bug Something isn't working

Comments

@bjelbo
Copy link

bjelbo commented Nov 3, 2023

I am experiencing problems when creating Custom Methods that should handle nullable properties.

My database entity contains nullable properties. e.g.

public DateTime? SomeDateTime { get; set; }

My custom method also takes a nullable DateTime as argument. e.g.

public static Date? DateOnly(DateTime? date)
{
    if (date == null)
    {
        return null;
    }
    return date.Value.ToUniversalTime().Date;
}

An OData query with the following compute statement throws an exception.

$apply=compute(DateOnly(SomeDateTime) as date)

The Exception is:

"'Expression of type 'System.DateTime' cannot be used for parameter of type 'System.Nullable`1[System.DateTime]' of method 'System.Nullable`1[Microsoft.OData.Edm.Date] DateOnly(System.Nullable`1[System.DateTime])' (Parameter 'arg0')"

The problem lies in the method ExpressionBinderHelper.MakeFunctionCall

In this method line 228 the following method is called

// if the argument is of type Nullable<T>, then translate the argument to Nullable<T>.Value as none
// of the canonical functions have overloads for Nullable<> arguments.
functionCallArguments = ExtractValueFromNullableArguments(functionCallArguments);

Then when calling Expression.Call it will fail because 'System.DateTime' is not System.Nullable`1[System.DateTime]

@bjelbo bjelbo added the bug Something isn't working label Nov 3, 2023
@xuzhg
Copy link
Member

xuzhg commented Nov 6, 2023

@SirTipzy Since you have a method call "DateOnly(...)", Can you share me your configuration about this method into the model?
Otherwise, ODL doesn't know this method?

And, if you can share a repro using a github repository, it can help us to dig more for you. Thanks.

@bjelbo
Copy link
Author

bjelbo commented Nov 7, 2023

@xuzhg I unfortunately do not have a github repo I can share.

I do not have problems for your OData Library to recognize and use my methods. I would not even get the exception I am getting if it did not know of the method.

Here is my class I use to add my methods. I call the UseCustomODataFunctions at startup

public static class CustomODataFunctionService
{
    public static void UseCustomODataFunctions(this IApplicationBuilder app)
    {
        var methodInfos = typeof(CustomODataFunctions).GetMethods(BindingFlags.Static | BindingFlags.Public);
        foreach (var methodInfo in methodInfos)
        {
            RegisterCustomFunction(methodInfo);
        }          
    }

    private static void RegisterCustomFunction(MethodInfo methodInfo)
    {
        var functionName = methodInfo.Name;
        var returnType = TypeToReference(methodInfo.ReturnType);
        var args = methodInfo.GetParameters()
            .Select(x => TypeToReference(x.ParameterType))
            .ToArray();
        var signature = new FunctionSignatureWithReturnType(returnType, args);
        ODataUriFunctions.AddCustomUriFunction(functionName, signature, methodInfo);
    }

    private static IEdmTypeReference TypeToReference(Type type)
    {
        var underlyingType = Nullable.GetUnderlyingType(type) ?? type;
        var primitiveTypeKind = EdmCoreModel.Instance.GetPrimitiveTypeKind(underlyingType.Name);
        if(underlyingType == typeof(DateTime))
        {
            primitiveTypeKind = EdmPrimitiveTypeKind.DateTimeOffset;
        }
        var isNullable = type.IsClass || Nullable.GetUnderlyingType(type) != null;
        return new EdmPrimitiveTypeReference(
            EdmCoreModel.Instance.GetPrimitiveType(primitiveTypeKind),
            isNullable);
    }
}

I have similar methods that have string parameters. These functions work as they are not explicitly set as nullables. I have also tested with none nullable DateTimes. These work if I change my DateOnly parameter to a normal DateTime. But as all my DateTime entity attributes are nullable in my database, I need these custom methods to contain nullable parameters.

A solution for you is to make a new method called ExpressionBinderHelper.MakeCustomFunctionCall where this method looks somewhat like this

    public static Expression MakeCustomFunctionCall(MemberInfo member, ODataQuerySettings querySettings, params Expression[] arguments)
    {
        Contract.Assert(member.MemberType == MemberTypes.Property || member.MemberType == MemberTypes.Method);
        
        Expression functionCall;
        if (member.MemberType == MemberTypes.Method)
        {
            MethodInfo method = member as MethodInfo;
            if (method.IsStatic)
            {
                functionCall = Expression.Call(null, method, arguments);
            }
            else
            {
                functionCall = Expression.Call(arguments.First(), method, arguments.Skip(1));
            }
        }
        else
        {
            var functionCallArguments = ExtractValueFromNullableArguments(arguments);
            
            functionCall = Expression.Property(functionCallArguments.First(), member as PropertyInfo);
        }

        return functionCall;
    }

I have tested creating this method in a copy of your repository and it will work.

@xuzhg
Copy link
Member

xuzhg commented Nov 7, 2023

@SirTipzy I see. Do you mind sharing a contribution based on your investigation?

By the way, OData spec has this built-in function at: https://docs.oasis-open.org/odata/odata/v4.01/odata-v4.01-part2-url-conventions.html#sec_date, did you try to use 'date()' method directly? not use your customized method?

@bjelbo
Copy link
Author

bjelbo commented Nov 8, 2023

@xuzhg Yes I know of this function, but it does not work in your library. Your library just returns the same datetime value as was passed to the function.

image

We also have other custom methods, DateOnly was just our most simple one I used as an example.

Yes, I can share a contribution.

@xuzhg
Copy link
Member

xuzhg commented Nov 13, 2023

@bjelbo You can NOT override the corresponding virtual methods to customize your scenarios?

@bjelbo
Copy link
Author

bjelbo commented Nov 13, 2023

@bjelbo You can NOT override the corresponding virtual methods to customize your scenarios?

I have done so for FilterBinder, but the same is not possible for ComputeBinder if I am not mistaking. Please correct me if I am wrong.

@bjelbo
Copy link
Author

bjelbo commented Dec 19, 2023

@xuzhg I am still very much interested in getting some clarification on this bug. Also I have left a comment for you on the pull request I am still waiting for a reply on.

@bjelbo
Copy link
Author

bjelbo commented Jan 9, 2024

@xuzhg @KenitoInc This is still an issue which I really hope will be resolved. Locally I have tested my fix in PR #1090 thoroughly and it works as intended. @xuzhg I have a comment on your comment in the PR. Your comment in the PR "MakeFunctionCall does MORE than MakeCustomFunctionCall. So, it's enough and safe to change using MakeCustomFunctionCall?" From my tests is that custom function calls are applied after querying the database, and therefore, the MakeCustomFunctionCall does less than MakeFunctionCall.

@rbalagangadharan
Copy link

@bjelbo Can you share the working code for CustomFunctions. I also have same use case #1229

@bjelbo
Copy link
Author

bjelbo commented Jun 26, 2024

@rbalagangadharan I have this PR for it that fixes the issue. Though I have not had the time to create the unit tests they are asking.
#1090

@bjelbo
Copy link
Author

bjelbo commented Dec 12, 2024

@xuzhg Is there a status regarding this bug?

@bjelbo
Copy link
Author

bjelbo commented Dec 16, 2024

@xuzhg see PR #1374

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