Efcore: EF Core 1.1.1 has introduced a new ArgumentException for complex queries with with subqueries, if the same subquery is present multiple times in the query model

[shaulbehr]

I saw today that EF Core 1.1 has been released. I made a new branch and upgraded from 1.0. Now several of our integration tests are failing with the below error. I'm not going to attempt to give you the full code base that raises this error; our codebase is massive. Rather I will just paste the line of code that throws the error along with the stack trace and hope this will be helpful to you. In the meantime I'm abandoning my attempt to upgrade until I hear back that this bug has been repaired.

The line of code that raises the exception:

                            var result = pagedResult
                                .Select(bt => new LibraryManagerSubShelf
                                {
                                    TitleStartingLetter = bt.titleStartingLetter.ToUpper()
                                })
                                .ToList();

where pagedResult is an IQueryable<> that has had a .Skip().Take() applied to it. I verified that if the skip+take is removed, the exception is not thrown.

Stack trace:
`` System.ArgumentException: An item with the same key has already been added. at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource) at System.Collections.Generic.Dictionary2.Insert(TKey key, TValue value, Boolean add)
at Microsoft.EntityFrameworkCore.Query.Internal.QueryModelExtensions.QueryModelMappingPopulatingVisitor.VisitSubQuery(SubQueryExpression expression)
at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.ExpressionVisitorBase.Visit(Expression node)
at System.Linq.Expressions.ExpressionVisitor.VisitAndConvertT
at Remotion.Linq.QueryModel.TransformExpressions(Func2 transformation) at Microsoft.EntityFrameworkCore.Query.Internal.QueryModelExtensions.QueryModelMappingPopulatingVisitor.VisitSubQuery(SubQueryExpression expression) at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.ExpressionVisitorBase.Visit(Expression node) at Microsoft.EntityFrameworkCore.Query.Internal.QueryModelExtensions.PopulateQueryModelMapping(QueryModel queryModel, Dictionary2 mapping)
at Microsoft.EntityFrameworkCore.Query.RelationalQueryModelVisitor.LiftSubQuery(IQuerySource querySource, Expression itemsExpression, Expression expression)
at Microsoft.EntityFrameworkCore.Query.EntityQueryModelVisitor.VisitMainFromClause(MainFromClause fromClause, QueryModel queryModel)
at Remotion.Linq.QueryModelVisitorBase.VisitQueryModel(QueryModel queryModel)
at Microsoft.EntityFrameworkCore.Query.EntityQueryModelVisitor.VisitQueryModel(QueryModel queryModel)
at Microsoft.EntityFrameworkCore.Query.RelationalQueryModelVisitor.VisitQueryModel(QueryModel queryModel)
at Microsoft.EntityFrameworkCore.Query.Internal.SqlServerQueryModelVisitor.VisitQueryModel(QueryModel queryModel)
at Microsoft.EntityFrameworkCore.Query.EntityQueryModelVisitor.CreateQueryExecutorTResult
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.CompileQueryCoreTResult
at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.<>c__DisplayClass19_01.<CompileQuery>b__0() at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQueryCore[TFunc](Object cacheKey, Func1 compiler)
at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.ExecuteTResult
at Remotion.Linq.QueryableBase1.GetEnumerator() at System.Collections.Generic.List1..ctor(IEnumerable1 collection) at System.Linq.Enumerable.ToList[TSource](IEnumerable1 source)
at MyCode.MyRepository.<>c__DisplayClass1_0.b__0(MyDbContext db) in C:src\MyCode\MyRepository.cs:line 63

[smitpatel]

@maumar - Did you investigate this?

[maumar]

In QueryModelMappingPopulatingVisitor before adding model to the dictionary we should check if it's already been added. This _should_ be safe to do, but will do some more testing to make sure.

[smitpatel]

While having defensive add to dictionary is safe to do, we should also investigate the reason for it to arise and evaluate if we want to preserve the first value or last value. Also if such cases should be impossible (but happening due to bug somewhere else) then defensive check would make it harder for us to catch the issue.

[maumar]

@shaulbehr are there other operations applied on the IQueryable in the pagedResult (filters, orderbys etc) or is it just entities.Skip(..).Take(...)?

[maumar]

standalone repro:

    class Program
    {
        static void Main(string[] args)
        {
            using (var ctx = new MyContext())
            {
                ctx.Database.EnsureCreated();

                var innerQuery = from c in ctx.Customers
                            let customers = ctx.Customers.Select(cc => cc)
                            where customers.Any()
                            select customers;

                var outerQuery = ctx.Customers.Where(c => innerQuery.Any()).ToList();
            }
        }
    }

    public class MyContext : DbContext
    {
        public DbSet<Customer> Customers { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseSqlServer(@"Server=.;Database=Repro7944;Trusted_Connection=True;MultipleActiveResultSets=true");
        }
    }

    public class Customer
    {
        public int Id { get; set; }        
    }

[maumar]

Fixed in 0594995d21b3706821107b84cf35dfe372210add

[maumar]

Justification to include this in 1.1.2 release:

Impact: Medium. This is a regression introduced by patch 1.1.1 and was reported by a customer. However the scenario affected is quite complex - it requires a subquery nested within another subquery.

Risk: Low. Fix is very local - basically we are adding defensive check when trying to add an item to a dictionary. They only scenarios that could possibly be affected by the fix are currently broken due to this bug.

[Eilon]

This patch fix is approved. Please follow the normal code review / pull request process and make sure you make the change in the correct branch.

[Eilon]

Preview builds of this patch fix should be available on the following feeds:

• https://dotnet.myget.org/gallery/aspnet-1-0-5-may2017-patch-public (for fixes in the 1.0.x release)
  
  
  
  • Feed URL to add to NuGet.config: https://dotnet.myget.org/F/aspnet-1-0-5-may2017-patch-public/api/v3/index.json
  
  
• https://dotnet.myget.org/gallery/aspnet-1-1-2-may2017-patch-public (for fixes in the 1.1.x release)
  
  
  
  • Feed URL to add to NuGet.config: https://dotnet.myget.org/F/aspnet-1-1-2-may2017-patch-public/api/v3/index.json
  
  

If you have a chance, please try out these preview builds and let us know if you have any feedback!

[ajcvickers]

@shaulbehr Could you please check that this issue is fixed for you using the feed posted above? It would be really great to confirm that the issue is fixed in a real-world scenario and that it doesn't regress functionality in some other way. It would be greatly appreciated.

[shaulbehr]

@ajcvickers : For our actual production code we decided only to use release code (not previews). But for the sake of this exercise I branched off and upgraded to preview 24623 (which was latest available at this time of writing). Regrettably I couldn't get past my integration test setup because of some other changes that appear to be broken by the way I'm declaring my relationships:

public class UserGroupOrganization
{
    // obviously there's other schema info here...
    public virtual ICollection<UserGroupOrganizationCrossReference> ParentOrganizations { get; set; }
    public virtual ICollection<UserGroupOrganizationCrossReference> ChildOrganizations { get; set; }
}

public class UserGroupOrganizationCrossReference
{
    // other stuff
    public Guid ChildUserGroupOrganizationId { get; set; }
    public Guid ParentUserGroupOrganizationId { get; set; }

    public virtual UserGroupOrganization ChildOrganization { get; set; }
    public virtual UserGroupOrganization ParentOrganization { get; set; }
}

// and inside OnModelCreating():
    modelBuilder.Entity<UserGroupOrganization>(entity =>
    {
        // other stuff
        entity.HasMany(o => o.ParentOrganizations)
              .WithOne(p => p.ChildOrganization)
              .HasForeignKey(p => p.ChildUserGroupOrganizationId);
        entity.HasMany(o => o.ChildOrganizations)
              .WithOne(c => c.ParentOrganization)
              .HasForeignKey(c => c.ParentUserGroupOrganizationId);
    });

    modelBuilder.Entity<UserGroupOrganizationCrossReference>(entity =>
    {
        // other stuff
        entity.HasOne(x => x.ParentOrganization)
              .WithMany(o => o.ChildOrganizations)
              .HasForeignKey(x => x.ParentUserGroupOrganizationId);

        entity.HasOne(x => x.ChildOrganization)
              .WithMany(o => o.ParentOrganizations)
              .HasForeignKey(x => x.ChildUserGroupOrganizationId);
    });

Firstly, I know I shouldn't have to declare those relationships in both entities, but I've tried all combinations. And I still get the following error:

System.InvalidOperationException : Unable to determine the relationship represented by navigation property 'UserGroupOrganization.ParentOrganizations' of type 'ICollection'. Either manually configure the relationship, or ignore this property using the '[NotMapped]' attribute or by using 'EntityTypeBuilder.Ignore' in 'OnModelCreating'.

Should I log a new bug for this?

[ajcvickers]

@shaulbehr Thanks for trying it. Yes, please file an issue for the new bug. We will investigate.

[ajcvickers]

@shaulbehr How are the primary keys defined on those two entity types?

[ajcvickers]

@AndriySvyryd Can you investigate this as a possible regression from 1.1.1 to 1.1.2?

[ajcvickers]

@shaulbehr Did you test this with the latest nightly build, or with the 1.1.2 preview feed posted in the comments above?

[AndriySvyryd]

@shaulbehr I wasn't able to repro the new issue using the 1.1.2 nightly. Please provide a complete code listing when filing a new issue for this.

[shaulbehr]

@ajcvickers It was the daily build. I'll try again with the 1.1.2 preview.

[ErikEJ]

You made a typo, I think.

https://dotnet.myget.org/gallery/aspnet-1-0-5-may2017-patch-publics

Should be:

https://dotnet.myget.org/F/aspnet-1-0-5-may2017-patch-public/api/v3/index.json

(and public, not publics)

[shaulbehr]

@ErikEJ I deleted my previous comment - was my bad.

[shaulbehr]

Confirmed: patch has fixed my problem. Thanks!

[ajcvickers]

@shaulbehr Thanks for checking! Much appreciated. Can you still file the issue for what you are seeing on dev?

[shaulbehr]

@ajcvickers Done
https://github.com/aspnet/EntityFramework/issues/8302

[anurse]

Verified fixed in 1.1.2 candidate build.