Solution of Compute - Include - Decompile problem

[mayorovp]

Issue #52

Added VisitConstant method to DecompileExpressionVisitor class. If constant is IQueryable it value's expression should be visited.

Added some logic to avoid infinite recursion (because EF DbSet expression is a value(ObjectQuery<...>).MergeAs(AppendData) where first constant is a internal DbSet query itself with same expression in).

---

Добавил метод VisitConstant в класс DecompileExpressionVisitor. Если константа является IQueryable - выражение, хранящееся в ее значении, также будет преобразовано.

Также добавил логику, необходимую для того, чтобы избежать вечной рекурсии. Это необходимо, потому что выражение, содержащееся в DbSet, определяется как value(ObjectQuery<...>).MergeAs(AppendData), где первая константа - это внутренний запрос самого DbSet, с точно таким же выражением внутри.

[hazzik]

@daveaglick @JonPSmith can you guys take a look?

[JonPSmith]

As I wrote the test harness then my comment is that Pavel's test does not follow the normal conventions. The test harness expects a test to a) run the test with LINQ without DelegateDecompiler (DD) to prove that it is possible and then b) run the same test, but using DD. This approach stops us chasing problems in DD which are really EF problems. There are plenty of examples to follow, e.g. Select Tests

Pavel.Mayorov's tests may be useful to him, but because they do not use the test harness its results won't be added to the automatically generated text documentation.

As to the fix then I will let @daveaglick comment on that :)

[mayorovp]

Ok, let me explain my test case.

Look at this code:

db.EfParents.Where(p => ComputedSample()).Decompile().Load();

This code works as expected but does not load children. Let's try to add Include call:

db.EfParents.Include(p => p.Children).Where(p => ComputedSample()).Decompile().Load();

This code works but i cannot use this sequence in my project because of reasons. This code

db.EfParents.Where(p => ComputedSample()).Decompile().Include(p => p.Children).Load();

does not throw any exception but does not work as expected due to #38. The readme says: "Decompile method should be called after all ORM specific methods, otherwise it may not work". Ok, let's move Include right before Decompile:

db.EfParents.Where(p => ComputedSample()).Include(p => p.Children).Decompile().Load();

Oops, now we got an exception.

So, now readme has the mistake, right sentence is "Decompile method or any methods that references computed properties or functions should be called after all ORM specific methods, otherwise it may not work". In my pull request i try to fix behavior according to current version of documentation.

[mayorovp]

PS ComputeDoubleIncludeDecompileNotFail is just a test of recursion.

[daveaglick]

I'll try and take a look at this tomorrow - I'm swamped today.

[daveaglick]

First off, I'm sure I speak for @hazzik when I say thanks for the PR!

I'd like to drop some reference information in here. I got curious what was really going on (why the heck are we getting a ConstantExpression when calling Include()?), and if checking for a ConstantExpression was solving the underlying issue or just a symptom. I traced the EF Include() call and found that the "problem" is happening in the internal ELinqQueryState class source here:

internal override ObjectQueryState Include<TElementType>(ObjectQuery<TElementType> sourceQuery, string includePath)
{
    var includeMethod = GetIncludeMethod(sourceQuery);
    Debug.Assert(includeMethod != null, "Unable to find ObjectQuery.Include method?");

    Expression includeCall = Expression.Call(
        Expression.Constant(sourceQuery), includeMethod, new Expression[] { Expression.Constant(includePath, typeof(string)) });
    ObjectQueryState retState = new ELinqQueryState(ElementType, ObjectContext, includeCall);
    ApplySettingsTo(retState);
    return retState;
}

Notice that it (indirectly) returns an Expression.Constant() - so that's where the constant expression is coming from. I wasn't aware EF would wrap queries like this, and it might not be the only situation in which it does. Therefore, I like the general approach of checking for IQueryable wrapped in a ConstantExpression and then decompiling the inner queryable - well done.

As for the code, it looks fine to me. The tests need some cleanup to use the harness that @JonPSmith developed, but I don't mind taking care of that after this is merged. FYI - this should also resolve #38. I'll add some additional tests for AsNoTracking() after merge as well, but some adhoc testing showed success (I don't think it wraps with a ConstantExpression anyway).

TL;DR:

[mayorovp]

No, this changes cannot resolve #38. Because EF use this code for third-party IQueryable providers:

    private static T CommonInclude<T>(T source, string path)
    {
        DebugCheck.NotNull((object)source);

        var includeMethod = source.GetType().GetRuntimeMethod(
            "Include", 
            p => p.IsPublic && !p.IsStatic,
            new[] { typeof(string) },
            new[] { typeof(IComparable) },
            new[] { typeof(ICloneable) },
            new[] { typeof(IComparable<string>) },
            new[] { typeof(IEnumerable<char>) },
            new[] { typeof(IEnumerable) },
            new[] { typeof(IEquatable<string>) },
            new[] { typeof(object) });

        if (includeMethod != null
            && typeof(T).IsAssignableFrom(includeMethod.ReturnType))
        {
            return (T)includeMethod.Invoke(source, new object[] { path });
        }
        return source;
    }

If DecompiledQueryable has no Include method, Include extension does nothing.

[hazzik]

@mayorovp can you change the test to use @JonPSmith framework?

[daveaglick]

@mayorovp Okay, I think I get what you're saying. This PR does fix the problem with crashing, but because we've wrapped the EF query provider by calling .Decompile(), when the wrapped .Include() call gets evaluated, EF runs the CommonInclude<T>(...) method above and instead of invoking the .Include() method of DbQuery<T> it just invokes...what? Are you thinking includeMethod would be null above? Do I understand your concern with #38?

I'm not sure this is the case, though. I ran your ComputeIncludeDecompileNotFail() test through a debugger to see what actually happens when we evaluate the .Include(). See the following screen shot (sorry for it being so wide):

Notice how when we evaluate the QueryableExtensions.Include<T>(...) method (in the right file), we still have a DbQuery<T> as the source. Because of that, we end up executing dbQuery.Include(path) instead of QueryableExtensions.CommonInclude<IQueryable<T>>(source, path). As far as I can tell, the include call goes to the DbQuery<T> as it should and we never even execute CommonInclude.

[mayorovp]

Sorry, i misunderstood #38 because it was just a link to some comment in other PR. Yes, my changes resolves this issue:

Unfortunately moving Decompile to the end is not enough. I have a query with Where, OrderByDescending, Take and couple of Includes after it and moving Decompile to the end causes NotSupportedException exception so it looks like the Where expression isn't decompiled any more.

[hazzik]

@mayorovp can you create test for AsNoTracking, it might be fixed by your code.

[txavier]

Any hope in getting this merged in?

[mayorovp]

@txavier if someone [re]write tests...

[hazzik]

Can you please enable Allow edits from maintainers option on this PR?

[mayorovp]

@hazzik enabled

[marcoz-tsn]

anyone please have news?