np (NullPropagation) throws NullReferenceException with property on .NET Core 3.1

[ahocquet]

After upgrading to .NET Core 3.1 from .NET Core 2.2, it seems that the np() operator now throws an NullReferenceException, even on properties. Here's a little piece of code that I run on LinqPad 6 that produces the bug :

void Main()
{
	var users = new[] { new User() { FirstName = "John" } }.AsQueryable();

	// Act
	var resultDynamic = users.Any("c => np(c.LastName, string.Empty).ToUpper() == \"Doe\"");
	resultDynamic.Dump();
}

public class User
{
	public string FirstName { get; set; }
	public string LastName { get; set; }
	public string EmailAddress { get; set; }
}

[StefH]

@ahocquet Can you provide a full EF 3.1 example project please?

[ahocquet]

I don't use Entity Framework, or maybe you mean .NET Core 3.1 ?

[StefH]

I see.

The problem is that the code np(c.LastName, string.Empty) does not work correctly, the string.empty is not applied, so you get null-pointer exception.

Note that nested properties work fine:

public static void Main()
        {
            var users = new[] { new User() { FirstName = "John" } }.AsQueryable();

            var resultDynamic = users.Any("c => np(c.Permission.Name, string.Empty).ToUpper() == \"Doe\"");
            Console.WriteLine(resultDynamic);
        }

        public class User
        {
            public string FirstName { get; set; }
            public string LastName { get; set; }
            public string EmailAddress { get; set; }

            public Permission Permission { get; set; }
        }

        public class Permission
        {
            public string Name { get; set; }
        }

I will investigate and try to fix.

[ahocquet]

Sounds great, thank you!

[StefH]

@ahocquet can you try MyGet version System.Linq.Dynamic.Core.1.0.20-ci-12438.nupkg ?

[ahocquet]

Works great, thanks for fixing it so fast!

[StefH]

PR will be merged to master #338

And a new official NuGet will be released.

[clazarr]

@StefH, thank you for all of your hard work maintaining this library! It's a really useful library and it wouldn't be possible without your helpful development support.

I wanted to report that I did encounter a problem with this fix in that it does not seem to add a safeguard check for null for the last property in a nested member reference and the type of the last property is a reference type such as a string.

Using the above code you provided as an example, the v1.0.20 of the System.Linq.Dynamic.Core NuGet yields an expression that looks like:
user => (((user != null) && (user.Permission != null)) ? user.Permission.Name : string.Empty).ToUpper() == "DOE"

If the nested Permission property is non-null but the Name property on Permission is null, the above expression will result in a NullReferenceException.

Here's a full ConsoleApp example (I ran it targeting .NET Core 3.1) that expands on the one you provided above and demonstrates the issue:

using System;
using System.Linq;
using System.Linq.Dynamic.Core;
using System.Linq.Expressions;

namespace ConsoleApp1
{
    internal class Program
    {
        #region Public Methods

        public static void Main()
        {
            var users = new[] { new User() { FirstName = "John", Permission = new Permission() } }.AsQueryable();

            // var resultDynamic = users.Any("c => np(c.Permission.Name, string.Empty).ToUpper() == \"DOE\"");
            string expression = "np(Permission.Name, string.Empty).ToUpper() == \"DOE\"";
            LambdaExpression predicate = DynamicExpressionParser.ParseLambda(ParsingConfig.Default, true, typeof(User), typeof(bool), expression);

            // Above predicate will look like this: user => (((user != null) && (user.Permission != null)) ? user.Permission.Name : string.Empty).ToUpper() == "DOE"
            // When executing predicate, because user.Permission.Name is not also propagated to null and .ToUpper() is called on null, the expression will result in:
            // System.NullReferenceException: 'Object reference not set to an instance of an object.'
            // The null propagation logic *should* also do the null check on user.Permission.Name e.g.,
            // user => (((user != null) && (user.Permission != null) && (user.Permission.Name != null)) ? user.Permission.Name : string.Empty).ToUpper() == "DOE"
            var resultDynamic = users.Where(predicate).Count();
            Console.WriteLine(resultDynamic);
        }

        #endregion Public Methods

        #region Public Classes

        public class User
        {
            #region Public Properties

            public string FirstName { get; set; }
            public string LastName { get; set; }
            public string EmailAddress { get; set; }

            public Permission Permission { get; set; }

            #endregion Public Properties
        }

        public class Permission
        {
            #region Public Properties

            public string Name { get; set; }

            #endregion Public Properties
        }

        #endregion Public Classes
    }
}

[StefH]

Can you try MyGet version System.Linq.Dynamic.Core.1.0.20-ci-12472.nupkg ?

[clazarr]

@StefH thank you for the super fast response!

The pre-release version addressed the issue in both the example code and in my main project where I'm currently using System.Linq.Dynamic.Core. For the example code, the lambda expression that is produced correctly checks the last property, which is nullable, for null as well as all of the preceding members too:

user => ((((user != null) && (user.Permission != null)) && (user.Permission.Name != null))
    ? user.Permission.Name
    : string.Empty).ToUpper() == "DOE"

So the new version works nicely!

I expanded the example code to include test cases for the following additional properties and the expressions produced correctly handled the value types and nullable value types and executed without exceptions:

public class Permission
{
    public string Name { get; set; }

    public int Value { get; set; }

    public int? References { get; set; }

    public Guid? AlternateIdentifier { get; set; }
}

I was just curious about the level of sophistication of the null propagation (np) handling logic.

Nice work Stef and thank you again for your effort and commitment!

[StefH]

@clazarr You're welcome.

The code supports (hopefully) all value-types and also objects and nullable types. See this test for more details: https://github.com/StefH/System.Linq.Dynamic.Core/blob/c9e98c9f050a21a24dbbae45e4031faca1416aa0/test/System.Linq.Dynamic.Core.Tests/ExpressionTests.cs#L1309

It should event support methods (like your ToUpper) and #302

I'm closing this issue again and will merge the fixes to master. Just keep an eye on this repo + nuget to see when a new official nuget is released.

And if you detect another problem, or want to add some code or tests, just make a PR.