True False methods not intuitive

[rhyous]

Scott. Nice library. Saved me a ton of time. Even if you didn't start it, thanks for bringing it to GitHub.

I just started using this library. I solved my problem.

I am going to enter a 'usability' issue if you don't mind. There is no bug.

The issue is with the PredicateBuilder.True<T> and PredicateBuilder.False<T>. Those aren't very intuitive. And they are identical except for the return value. But when to use which is confusing and has to be documented.

public static Expression<Func<T, bool>> True<T> () { return f => true; }
public static Expression<Func<T, bool>> False<T> () { return f => false; }

I still am not sure if I understand correctly why there are two. Is seems like this has a few usability problems:

1. Training issue. You have two ways to create a predicate instead of one. That adds learning curve of which to use for no reason.
2. True or False don't mean True or False in a query. What do they mean?
3. The code is not self-documenting or even intuitive at all.

I think True and False mean this:

• True - Append predicate starting with AND.
• False - Append predicate starting with OR.

Am I right?

If so, can we improve this?

Obviously, we would have to leave both the True, False calls for backwards compatibility, but wouldn't the usability be easier and more self-documenting to have one method. But they could be marked as deprecated.

Just spitballing here, but we could do something like this:

public enum AppendPredicateWith
{
    Or,
    And
}

Then use that enum to create the predicate.

public static Expression<Func<T, bool>> Create<T> (AppendPredicateWith andOr = AppendPredicateWith.Or) { return f => andOr == AppendQueryWith.And; }

Or AppendPredicateWith.And could be the default.

Now every time PredicateBuilder is called, it is easy to understand and the code reads like what is happening?

PredicateBuilder.Create<Product>(AppendPredicateWith.Or)

The benefits.

1. The True False objects that have no apparent meaning are replaced with something meaningful.
2. There aren't two objects to start a predicate anymore.
3. The code tells you what is happening and is self-documenting.

Have a nice day.

[StefH]

Would the name "StartPredicateWith" be a better name instead of "AppendPredicateWith" ? Because this statement is the first statement you do, and you define what you want: an AND or an OR.

/// <summary>
/// Creates a Predicate with OR (false) or AND (true).
/// </summary>
/// <typeparam name="T">The generic Type</typeparam>
/// <param name="start">The StartPredicateWith (can be "And" or "Or").</param>
/// <returns>Expression{Func{T, bool}}</returns>
public static Expression<Func<T, bool>> Create<T>(StartPredicateWith start = StartPredicateWith.Or)
{
    return f => start == StartPredicateWith.And;
}

[Thorium]

I vote for keeping existing functionality and not adding anything if you really don't need it. :-)

[rhyous]

I am not too picky with the enum name. Try a few out in code, see what you like best. I think it is more important to have one method, not two, and to have the code read and/or instead of true/false.

AppendPredicateWith.And AppendPredicateWith.Or

StartsPredicateWith.And StartsPredicateWith.Or

PredicateOperator.And PredicateOperator.Or

Oper.Or Oper.And

[rhyous]

@Thorium I think the functionality is there, but the usability and code readability does "really need it."

[StefH]

New versions uploaded to NuGet:

• https://www.nuget.org/packages/LinqKit/1.1.5
• https://www.nuget.org/packages/LinqKit.Microsoft.EntityFrameworkCore/1.0.3
• https://www.nuget.org/packages/LinqKit.EntityFramework/1.0.2

The extension methods have been added:

public static Expression<Func<T, bool>> Create<T>(PredicateOperator startOperator = PredicateOperator.Or)

public static Expression<Func<T, bool>> Extend<T>([NotNull] this Expression<Func<T, bool>> first, [NotNull] Expression<Func<T, bool>> second, PredicateOperator @operator = PredicateOperator.Or)

Please test and close issue if it's fine.

[StefH]

@rhyous ; can you please test ?

[rhyous]

I'll update my code tomorrow or Monday.

[rhyous]

I have become much more familiar with LinqKit. I no longer think this code change is a good idea. Thorium was right, this code change was wrong.

However, a code change was definitely needed, it was just a different one. The flaw is completely different that what I understood.

When I wrote my first post, I was very clear that I didn't understand what this code was doing any my understanding was limited. Now I understand what is going one and the fix that makes sense is completely different.

The bug is a confusion bug that leaves LinqKit confusing to use. Every Predicate starts out as true or false. When used in Entity Framework, there is an extra WHERE expression. Let me explain:

var pred = new PredicateBuilder.False<Person>(); <-- Which method to use True or False?
//var pred = new PredicateBuilder.True<Person>(); 

var names = List<string>() {"Name1","Name2"}
foreach (var name in names)
{
    var innerPred = new PredicateBuilder.True<Person>();
     innerPred .Or(p => p.Name == name)
}

The average user of LinqKit is going to have no clue whether to use True or False methods. I know I sure didn't.

With False(), it it doesn't work. Using with Entity Framework returns all People. With True(), it it doesn't work. Using with Entity Framework returns all People.

What is going on?

Why? Because of the query. It has an extra 1=1 or 1=0 stubbed in expression before every new predicate.

SELECT [Extent1].[Id] AS [Id], [Extent1].[Name] AS [Name] FROM [dbo].[Person] AS [Extent1]
WHERE (1=p__linq__0) <-- what is this doing here?
OR (1 = p__linq__1 OR ([Extent1].[Name] = @p__linq__2))
OR (1 = p__linq__3 OR ([Extent1].[Name] = @p__linq__4))

Once I realized this, I could fix my code and move on. But this is what I mean by a confusion bug. It is too confusing. It needs to be simpler.

So why do we have the 1=1 or 1=0 in the first place? It seems like this should go away. I want the query to be this:

SELECT [Extent1].[Id] AS [Id], [Extent1].[Name] AS [Name] FROM [dbo].[Person] AS [Extent1] WHERE ([Extent1].[Name] = @p__linq__2) OR ([Extent1].[Name] = @p__linq__4)

Why was LinqKit implemented with this extra check? Well, the answer is because Expression<Func<T,bool>> cannot be empty and doesn't have a public constructor, so naturally, a query has to start with something, right?

So then I thought, well, why not have a PredicateBuilder.New<T>() that just returns null. Extension methods still work with an object even if its value is null. We could simply return the passed in expression to And/Or if the left expression is null. However, I decided against null. Null could cause issues. A lot of code might not be expecting null. I need an object that could use 1=1 or 1=0 if null, but doesn't use 1=1 or 1=0 if not null.

So I created an ExpressionStart<T> that wraps an Expression<Func<T,bool>> and acts for all intents and purposes as if it were an Expression<Func<T,bool>>. Using ExpressionStarter<T>, we can eliminate all the unnecessary 1=1 or 1=0 calls. I can also support null, or a default 1=1 or 1=0 calls if desired.

I depricate True and False and added a PredicateBuilder.New<T>() that returns an empty Expression<Func<T,bool>> with a default of 1=0 that is only used if null and otherwise never used.

I unit tested it pretty thoroughly to make sure ExpressionStarter<T> really acted just like Expression<Func<T,bool>>. My query is now exactly as expected. All unit tests pass. Nothing is broken.

I have created a pull request linked directly above. Give it a look, let me know what you think. I worked hard to make sure there were no breaking changes.

[jblacker]

I hope I'm posting this in the right place, but I figured since this isn't closed yet it's OK to post here. I updated LinqKit to the current version and changed my True() & False() methods to use New() instead. This caused the functionality to break wherever I use New() instead of having an initial lambda when the query was issued via EF.

I have since reverted back to using the True() & False() starter methods instead and everything works as intended in the current release. I'm not 100% sure why it caused all of my search logic to break, but there is definitely something wrong and they probably shouldn't be depreciated just yet...

[rhyous]

If you use the Visual Studio Diagnostic Tools (Debug | Windows | Show Diagnostic Tools) or SQL Profiler, you will be able to and get the actual SQL query.

It is all about the WHERE statement in the actual SQL query EF generates.

Do this twice: Once with New() and once with your query using True() or False(). Compare the two queries.

New() does have a default lambda, it just only uses it if no additional lambda was provided. Also, the New() has an overload so you put in a bool value, so if you don't add any other expression, it will be 1=1 or 1=0. However, on adding of the first "real" expression, the 1=1 or 1=0 isn't included.

There are differences between New and True() or False().

For example, you can create this expression with True(), which one might consider a bug or invalid:

WHERE 1=1 or (<expression>)

The 1=1 is always true, so the additional expression is never actually evaluated. Was that intended? This was happening to me, so it turned out that I was always getting all items from a table, because 1=1 is always true. I thought I had coded it correctly, but once I got rid of the 1=1, my query didn't bring back any results and it turned out my second expression had a bug. I fixed it an now I get the appropriate results.

Similarly, with False() you can create this expression, which one might consider a bug or invalid:

WHERE 1=0 AND (<expression>)

The 1=0 is always false, so again, the additional expression is never actually evaluated.

Using New() these would never happen, because the 1=1 or 1=0 are removed leaving only:

WHERE (<expression>)

So. again, evaluate the actual queries. If after this evaluation, you find a bug, post it, and I will look at it.

[jblacker]

@rhyous You are absolutely correct, and I'm a dumbass. Thanks for the clarification though. I'll leave my comment up there so others can see your clarification comment.

[Mardoxx]

Very eloquent way of describing your issue! Very brief but here:

I still am not sure if I understand correctly why there are two.

Initially includes or excludes all results (true and false respectively)

1. Predicate returns either true of false. Needs to be one or the other initially depending on what you're trying to do with it
2. It's not a query it's a predicate
3. It is if you see it is a predicate builder not a query builder.

Isn't this rudimentary logic? I don't think this is an issue really. But I can see how it would be confusing for some people at first, but nothing that wouldn't be overcome with experimentation.

Also what happens if e.g. I want to exclude deleted items when we aren't an admin. Using the following (assuming I read the patched docs correctly) would yield no results as an admin (.new() is shorthand for falsey predicate isn't it?.. pseudo code)

var pred = PredicateBuilder.new();
// exclude deleted if not an admin
if(!User.IsAdmin)
{
    pred = pred.And(x=>x.IsDeleted == false);
}
var items = db.Items.AsExpandable().Where(pred).ToList();

This also doesn't read nicely because what exactly are we anding with? Nothing AND something always gives nothing (where nothing isn't evaluated as truthy, of course)! Although in this case it doesn't because you remove the false statement behind the scenes.

[davisford]

What if I want to return all items if no other expressions are added? None of the options above work for me.

For example, if I have a search class like this:

public class Search<T> {
   private Expression<Func<T,bool>> predicate = PredicateBuilder.New<T>();
   public Expression<Func<T,bool>> Expression { get; }
}

// concrete type
public class FooSearch : Search<Foo> {

   public FooSearch HasID(long id) {
      predicate = predicate.Or(f => f.ID == id);
      return this;
   }

   public FooSearch HasName(string name) {
      predicate = predicate.Or(f => f.Name == name);
      return this;
   }
}

Now, I want to use it like this:

// finds all foos that have id = 3 or name = "bar" --- this works 
var search = new FooSearch().HasID(3).HasName("bar").Expression;

// here I *want* to find all foo records --- this does not work
var search = new FooSearch().Expression;

The first example works b/c as you stated the .New() drops the first predicate and only uses the last.

The second example does not work, because the predicate ends up being 1 = 0 which evaluates to false, which is not what I want.

So, I'm trying to figure out how to solve this. If I can somehow tell in the Expression getter that the predicate is equal to the StarterExpression then I could just return PredicateBuilder.True<T>(); but so far it isn't clear to me how to deduce that?

[davisford]

So, right now, the only way I can tell to do that (which is not ideal):

        private const string FalsePredicate = "f => False";
        public Expression<Func<T, bool>> Expression {
            get
            {
                // if this is the StarterExpression (1 = 0) then we want the True predicate to return all records 
                if (FalsePredicate.Equals(predicate.ToString())) { return PredicateBuilder.True<T>();  }
                return predicate;
            }
        }

I wish the predicate had some kind of boolean indicator to tell me whether it was the starter expression.

[rhyous]

I had to choose one of either true or false as the default for empty PredicateBuilder.New(). I chose false. The reason I chose false is because with huge tables, you don't want the default to be true. Also, with the default of true, you might think your query works when it doesn't as it returns data. With false, you get nothing.

However, you can easily change it to true.

Use this overload:

public static ExpressionStarter<T> New<T>(bool defaultExpression);

So like this:

public class Search<T> {
   private Expression<Func<T,bool>> predicate = PredicateBuilder.New<T>(true);
   public Expression<Func<T,bool>> Expression { get; }
}

[davisford]

@rhyous the issue isn't really so much whether the default is false or true. I can easily create my starter expression as true, but that breaks the other case where I add more predicates b/c it always returns all records.

What I'm looking for instead is a way to introspect the predicate so it can tell me if it is empty or not, and then I can dynamically decide what to do to fit the use case.

Right now, after introspecting the predicate in the debugger and comparing the case where it was just the starter expression vs. if it had several predicates tacked on, I could not see any robust way to do so.

My hacked solution was to dump it ToString() and compare ....not ideal.

[rhyous]

You may benefit from storing the ExpressionStarter<T> in your variable. In your current code, the ExpressionStarter<T> is created in memory by PredicateBuilder.New<T>(), but then the code assigns it to an Expression<Func<T,bool>>, which will use the implicit cast, so the ExpressionStarter<T> is never stored and left to be garbage collected. You never actually have a variable holding onto the ExpressionStarter<T>. You need that variable. That is what gives you all the features.

public class Search<T> {
   protected Expression<Func<T,bool>> predicate = PredicateBuilder.New<T>(true);
   public Expression<Func<T,bool>> Expression { get; }
}

If you use ExpressionStarter<T> this is solved for you.

public class Search<T> {
   protected ExpressionStarter<T> Starter = PredicateBuilder.New<T>(true);
   public Expression<Func<T,bool>> Expression { get { Starter.Predicate } }
}

Now you can use ExpressionStart<T> in your code.

public class FooSearch : Search<Foo> {

   public FooSearch HasID(long id) {
      Starter.Or(f => f.ID == id);
      return this;
   }

   public FooSearch HasName(string name) {
      Starter.Or(f => f.Name == name);
      return this;
   }
}

Now, if you want to control the default expression if not started, you can.

if (!Starter.IsStarted)
{
    Starter.Start(whateverStartExpressionYouWant) ;
}

[tmc101]

I've just moved from LinqKit 1.1.3.1 to LinqKit 1.1.16.0

Switching to using Predicate.New() broke my code.

Replacing PredicateBuilder.True and PredicateBuilder.False with PredicateBuilder.New feels very counter-intuitive to me.

The AND and OR operations on predicates form a monoid with TRUE and FALSE being the identity values respectively.

i.e. It makes sense from a Boolean logic viewpoint to build expressions such as:

True AND (x==y) AND (p!=q)

or

False OR (x==y) OR (p!=q)

The True value for the AND expression doesn't change the logical value of the expression, and neither does the FALSE value in the OR expression.

I don't know what 'New' is supposed to mean in the context of Boolean logic?

The average user of LinqKit is going to have no clue whether to use True or False methods. I know I sure didn't.

I'm certainly not convinced that the 'average user of LinqKit' has so little knowledge of basic Boolean algebra.

I had to choose one of either true or false as the default for empty PredicateBuilder.New(). I chose false. The reason I chose false is because with huge tables, you don't want the default to be true. Also, with the default of true, you might think your query works when it doesn't as it returns data. With false, you get nothing.

This reasoning seems questionable to me and based around limited use-cases.

The semantics of PredicateBuilder.True and PredicateBuilder.False are crystal clear, I find the use of PredicateBuilder.New() to be confusing.

For instance, in the tests below the value created using PredicateBuilder.New(true) seems to mutate when it is ANDed with another expression.

Whereas the value of PredicateBuilder.True() remains constant:

       [Test]
        public void TestPredicateNew()
        {
            var newTrue = PredicateBuilder.New<String>(true);

            Console.WriteLine(newTrue);  // f => True

            var value1 = newTrue.Compile()("");

            var andWithNewTrue = newTrue.And(x => false);

            Console.WriteLine(newTrue); // x => False

            var value2 = newTrue.Compile()("");

            // Fails
            Assert.True(value1 == value2);
        }

        [Test]
        public void TestPredicateTrue()
        {
            var predicateTrue = PredicateBuilder.True<String>();

            Console.WriteLine(predicateTrue); // f => True

            var value1 = predicateTrue.Compile()("");

            var andWithOldTrue = predicateTrue.And(x => false);

            Console.WriteLine(predicateTrue); // f => True

            var value2 = predicateTrue.Compile()("");

            // Succeeds
            Assert.True(value1 == value2);
        }

So objects returned by PredicateBuilder.New and PredicateBuilder.True/PredicateBuilder.False have fundamentally different behaviour.

But the code for PredicateBuilder.True says "Use PredicateBuilder.New() instead.":

        /// <summary> Always true </summary>
        [Obsolete("Use PredicateBuilder.New() instead.")]
        public static Expression<Func<T, bool>> True<T>() { return new ExpressionStarter<T>(true); }

And the documentation says:

PredicateBuilder.New() creates an object called ExpressionStarter, which acts for all intents and purposes as an Expression<Func<T, bool>> object.

Anyone attempting to simply replace PredicateBuilder.True() with PredicateBuilder.New(true) is going to introduce subtle bugs into their code which will be very difficult to diagnose.

PredicateBuilder.True and PredicateBuilder.False should not be deprecated.

[rhyous]

@tmc101

Wow. The build breaking can be annoying. Did it break because of the obsolete call? Or did it break/break because of code behavior?

Code can always get better. What about PredicateBuilder.New() is confusing?

[tmc101]

It originally broke the compile because of the deprecation.

So I changed my calls from PredicateBuilder.True to use PredicateBuilder.New(true).

Then it broke the tests with:

System.InvalidOperationException : Internal .NET Framework Data Provider error 1025.

These two tests illustrate the difference (they are a cut-down version of my actual tests):

        [Test]
        public void PassingTest()
        {
            var predicate = PredicateBuilder.True<Journal>();

            Expression<Func<Person, bool>> journalPredicate = p => p.Journals.Any(predicate.Compile());

            var query = dbContext.Persons.AsExpandable().Where(journalPredicate);

            Console.WriteLine(query.ToString());
        }

        [Test]
        public void FailingTest()
        {
            var predicate = PredicateBuilder.New<Journal>(true);

            Expression<Func<Person, bool>> journalPredicate = p => p.Journals.Any(predicate.Compile());

            var query = dbContext.Persons.AsExpandable().Where(journalPredicate);

            Console.WriteLine(query.ToString());
        }

The model is:

    public class Person
    {
        public virtual ICollection<Journal> Journals { get; set; }
    }

The things I find confusing about PredicateBuilder.New() are:

1. It returns a mutable object - so where previously I was able to create a singleton expression.

   var alwaysTrue = PredicateBuilder.True<Journal>();

this always remained x=> true.

If I try the same with PredicateBuilder.New();

   var maybeTrue = PredicateBuilder.New<Journal>(true);

I can change the expression in maybeTrue to x => false using:

   var definitelyfalse = maybeTrue.And(x=> false);

It is not obvious that maybeTrue is now x=>false - even though I have not assigned to it again.

So the behaviour is not the same as PredicateBuilder.True<Journal>() - which the documentation implies.

2. PredicateBuilder.New() returns an ExpressionStarter which defaults to x=> false.

   This is not obvious from the call. It would be just as valid to default it to x=> true.

   The argument that your are more likely to want a false result than a true seems to be based on a single use case.

   The call to PredicateBuilder.New() should take a mandatory argument so the user is forced to make it specific that they require a true or false expression. This is what PredicateBuilder.True and PredicateBuilder.False already do.

3. The main argument for PredicateBuilder.New() is that a developer may write a predicate like:

   false AND (x>1)

   or

   true OR (x>1)

   But if people are going to do this, then they are going to do lots of other silly things and introducing PredicateBuilder.New() just feels like dumbing down.

PredicateBuilder.True and PredicateBuilder.False are not the same as PredicateBuilder.New(true) and PredicateBuilder.New(false).

They should be un-deprecated so existing developers don't have to change their code, potentially introducing bugs.

[rhyous]

Let me give your failing test a look.

[rhyous]

I looked at this more thoroughly for you.

Unit Tests

If you ToString() Expression<Fun<Journal,bool> or ExpressionStarter<Journal> both ToString to the same string. However, if you call ToString on a subexpression to which you have passed in the build predicate, you will see the subexpression know about the wrapper. The ExpressionStarter.Compile() becomes wrapped in a Value() call.

The ToString of the child functions are different because they are working with different objects: ExpressionStarter<T> vs Expression<Func<T, bool>>. If you want them to be the same, you can easily make them the same by not using var.

var predicate1 = PredicateBuilder.True<Journal>(); // This is an Expression<Func<Journal,bool>> of 
var predicate2 = PredicateBuilder.New<Journal>(true) // This is an ExpressionStarter<Journal>
Expression<Fun<Journal,bool> predicate2 = PredicateBuilder.New<Journal>(true) // This casts from ExpressionStarter<Journal> to Expression<Func<Journal,bool>> and leads to the same behavior as before.

You are welcome to cast away the ExpressionStarter<T> and only use the Expression<Func<T,bool>> if you desire.

Remember, Expression<Func<Journal,bool>> was sealed, so we couldn't use inheritance. So instead, we had to create an ExpressionStarter that acts like an Expression<Func<Journal,bool>> but it acts as Expression<Func<Journal,bool>> by using a mix of implicit casts and wrapping. It is still a different object.

Also, you should avoid using this argument:

But if people are going to do this, then they are going to do lots of other silly things and introducing PredicateBuilder.New() just feels like dumbing down.

I hear this invalid argument a lot. You repeated it twice. I let it go the first time. Please take a moment to realize why this type of thinking is elitist and has no place in our industry.

[tmc101]

Thanks for the explanation.

I'm going to stick with using True() and False() for the time being.

I genuinely feel that they are easier to understand.