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

[3.0] Owned types are null when Select, Skip and Take are used with Sqlite #18734

Closed
BrightSoul opened this issue Nov 4, 2019 · 3 comments · Fixed by #19377
Closed

[3.0] Owned types are null when Select, Skip and Take are used with Sqlite #18734

BrightSoul opened this issue Nov 4, 2019 · 3 comments · Fixed by #19377
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@BrightSoul
Copy link

BrightSoul commented Nov 4, 2019

Hello there, I've found an eerie behavior with EFCore 3.0 and Sqlite. Owned types are null when I use Select, Skip and Take because their fields do not appear in the SQL query. Here's a repro project.
https://github.com/BrightSoul/repro-skip-take-issue

Here's the gist of it: a simple entity having a CurrentPrice property of type Money.

    public class Course
    {
        public int Id { get; private set; }
        public string Title { get; private set; }
        public Money CurrentPrice { get; private set; }
    }

And here's Money, very simple.

    public class Money
    {
        public decimal Amount { get; set; }
        public Currency Currency { get; set; } //Currency is just an enum type
        
        public override string ToString()
        {
            return $"{Currency} {Amount:#.00}";
        }
    }

Here's the mapping for Course. As you can see, Money is an owned type.

modelBuilder.Entity<Course>(entity =>
            {
                entity.OwnsOne(course => course.CurrentPrice, builder => {
                    builder.Property(money => money.Currency).HasConversion<string>();
                });
            });

Now I run this LINQ query. As you can see there's a Select invoking a static mapping method (because reasons) and Skip and Take are used.

var courseList1 = await dbContext.Courses
                    .Select(course => CourseViewModel.FromEntity(course))
                    .Skip(0).Take(3)
                    .AsNoTracking()
                    .ToListAsync();

Here's the issue: all items in courseList1 have a null CurrentPrice property. I had no problem with ASP.NET Core 2.2.

If, instead, I omit Skip and Take, everything is working fine. All of the items in courseList2 have a proper value for the CurrentPrice property.

var courseList2 = await dbContext.Courses
                    .Select(course => CourseViewModel.FromEntity(course))
                    //.Skip(0).Take(3) //Omit this
                    .AsNoTracking()
                    .ToListAsync();

Again, here's the repro. It's a very simple ASP.NET Core application.
https://github.com/BrightSoul/repro-skip-take-issue

I know I should have made a Unit Test project but it's 1 AM and I'm about to fall asleep on the keyboard after trying to understand this problem for 3 hours. Sorry.

Would you please take a look?

Thanks,
Moreno

@ajcvickers
Copy link
Member

@smitpatel Is this a duplicate of #18672 If so, maybe we should prepare a patch.

@smitpatel
Copy link
Member

Both of them happens in same area but this has different root cause. Above case specifically is about doing client projection in non-last operation. Any operation which does pushdown can cause not loading the properties.
#18672, we still have entity references just in a form which we did not recognize. Above case, we don't even have entity left after client projection. Nav expansion moves skip/take before select, and that is where things are going wrong. If we don't move then above query is non-translatable as is.

The fix would be somewhat non-trivial because we are going to change what we have been doing in processing skip/take in nav expansion since we have moved to new pipeline. It can make some queries which are getting translated right now fail.

@ajcvickers
Copy link
Member

Note from triage: Putting this in 5.0 for the same reason as #18672

@ajcvickers ajcvickers added this to the 5.0.0 milestone Nov 8, 2019
smitpatel added a commit that referenced this issue Dec 20, 2019
smitpatel added a commit that referenced this issue Dec 27, 2019
@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Dec 30, 2019
smitpatel added a commit that referenced this issue Dec 30, 2019
Resolves #18140
Resolves #18374
Resolves #18672
Resolves #18734
Resolves #19138
Resolves #19207
smitpatel added a commit that referenced this issue Dec 30, 2019
- Skip/Take does not force applying pending selector and changing shape.
- Throw translation failure message for Querayble methods which we don't translate (hence we don't process in navigation expansion). Earlier we threw query failed message. Now Navigation Expansion does not throw QueryFailed error message from any place.
- Unwrap type conversion for validating member access during include expansion so that we don't generate include when derived type's member is accessed.

Resolves #18140
Resolves #18374
Resolves #18672
Resolves #18734
Resolves #19138
Resolves #19207
smitpatel added a commit that referenced this issue Dec 30, 2019
- Skip/Take does not force applying pending selector and changing shape.
- Throw translation failure message for Querayble methods which we don't translate (hence we don't process in navigation expansion). Earlier we threw query failed message. Now Navigation Expansion does not throw QueryFailed error message from any place.
- Unwrap type conversion for validating member access during include expansion so that we don't generate include when derived type's member is accessed.

Resolves #18140
Resolves #18374
Resolves #18672
Resolves #18734
Resolves #19138
Resolves #19207
smitpatel added a commit that referenced this issue Jan 1, 2020
- Skip/Take does not force applying pending selector and changing shape.
- Throw translation failure message for Querayble methods which we don't translate (hence we don't process in navigation expansion). Earlier we threw query failed message. Now Navigation Expansion does not throw QueryFailed error message from any place.
- Unwrap type conversion for validating member access during include expansion so that we don't generate include when derived type's member is accessed.

Resolves #18140
Resolves #18374
Resolves #18672
Resolves #18734
Resolves #19138
Resolves #19207
smitpatel added a commit that referenced this issue Jan 1, 2020
- Skip/Take does not force applying pending selector and changing shape.
- Throw translation failure message for Querayble methods which we don't translate (hence we don't process in navigation expansion). Earlier we threw query failed message. Now Navigation Expansion does not throw QueryFailed error message from any place.
- Unwrap type conversion for validating member access during include expansion so that we don't generate include when derived type's member is accessed.

Resolves #18140
Resolves #18374
Resolves #18672
Resolves #18734
Resolves #19138
Resolves #19207
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview1 Mar 13, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview1, 5.0.0 Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants