Fix sparse fields with DefaultEntityRepository.Get() override

[milosloub]

Closes #475

Current implementation:

• sparse fields calls Select() query first, than follows Filter() (and other methods in IQueryableExtensions). In case Select() (with internal Provider.CreateQuery() call) is applied like that, LINQ Where() method is reduced in runtime and has no info about other properties outside Select() => this is exactly the use-case, when overriding repository Get() method. And this leads to broken sql translation.

Requested implementation:

• Select() call is moved from DefaultEntityRepository.Get() to EntityResourceService.GetAsync() before Skip() and Take(). There is no "reduce" danger.

What do you think about this change?

[jaredcnance]

Thanks for the PR as always. This is a pretty big breaking change. I need to think about it a little more. We should be able to deprecate the existing Get API without causing a breaking change right away. But, I’d like to spend some more time understanding the problem. I know that we override this method in many places and haven’t run into this issue. I’m wondering if there is an issue here that needs to be pushed back onto EF? But anyways, I need to work on a minimum reproduction of the issue so I can understand it better.

[milosloub]

@jaredcnance To understand the problem, you can simply try this two use-cases in TodoItemRepository with basic query [GET] api/todo-items
a)

        public override IQueryable<TodoItem> Get()
            => base.Get()
                .Select(i => new TodoItem() { Description = i.Description }) // this line comes from IQueryableExtensions.Select() in runtime
                .Where(e => e.OwnerId == 1);

b)

        public override IQueryable<TodoItem> Get()
            => base.Get()
                .Where(e => e.OwnerId == 1)
                .Select(i => new TodoItem() { Description = i.Description });

Only order of Select() and Where() is changed.
In a) example you will notice this line in console:

The LINQ expression 'where (new TodoItem() {Description = [i].Description }.OwnerId == 1)' could not be translated and will be evaluated locally.

In b) everything is OK

I think this is not EF Core issue, because every LINQ query (method or query syntax) should end with select clause.

And if you modify example a) and add OwnerId to Select, than everything works as expected.

        public override IQueryable<TodoItem> Get()
            => base.Get()
                // OwnerId provided extra, than it is working
                .Select(i => new TodoItem() { Description = i.Description, OwnerId = i.OwnerId })
                .Where(e => e.OwnerId == 1);

That was the reason I was talking about "reducing" properties -> Where() doesn't see property, which is not provided in previous Select()

Is it more clear, now?

[milosloub]

@jaredcnance hey man, or you can try simply this (without any Repository Layer and overrides)

[GET] api/todo-items?filter[ordinal]=1&fields[todo-items]=description
-> you will get empty array
[GET] api/todo-items?filter[ordinal]=1&fields[todo-items]=ordinal
-> you will get some values

The problem is same - applying filters and it's Where() method is called after Select()

[jaredcnance]

Yes, I see. You're correct. And I don't think moving the order alone should be a breaking change. I think we can probably use C# 8 default interface methods to keep this from being a breaking change entirely. Later this week I may put together a draft document that describes our approach and philosophy around these kinds of breaking changes so that users have clear expectations.

But, other than that, this approach looks good to me. Thanks!

[milosloub]

@jaredcnance What about new GetQueryable() method like you mentioned here #404 (comment) ?
Then deprecate Get() (in DefaultEntityRepository and IEntityReadRepository), then EntityResourceService can call this new method and add that Select() line.

This is breaking change only for those, who implements custom ResourceService by override (without base.GetAsync call) or implement IEntityReadRepository. They will see this deprecation with clear description. If developer let deprecation be, function is the same. If not he has to change to GetQueryable() and provide Select() row to his custom service.
Maybe I missed something that blocks all of this, I don't know - just some ideas.
What do you think?

[jaredcnance]

No, I like the idea. However, for the first rollout I think I would prefer to try using default interface methods and deprecation warnings. This lets us release it as a minor version first so users don’t have to fix all overrides in one go (some projects make heavy use of overrides and the work will take a while). But, overall, I like the proposal. 👍

[milosloub]

Ok, no problem. But can you give me some explained guidance, where to put changes? Because I'm not familiar with default interface methods, yet. Right now I can't imagine the right approach.

[jaredcnance]

Sure. Tomorrow evening I’ll extend this PR with what I’m thinking if that’s alright

[milosloub]

Ok, thanks.

[jaredcnance]

Ah never mind, I was think that default interface methods were already available in the 3.0.0-preview SDK. But, apparently it's still at the proof-of-concept phase. So, let's bump the major version and we can release this as alpha.

[milosloub]

Yeah I see, never mind. Thanks for approval, but do you want to do it like this "first pass" way? What about deprecating Get() and introduce new GetQueryable() to minimize posible breaking changes? Just asking for sure

[milosloub]

@jaredcnance Can you look at the question above? This is quite blocking feature for my company at this moment and I don't know if I can realease as alpha...

[maurei]

What about deprecating Get() and introduce new GetQueryable() to minimize possible breaking changes?

I think we should do this. We'll completely drop Get() in future releases. But adding a new method to an existing interface is still a breaking change though so we'll have to bump the major version anyway.

[maurei]

@milosloub see #498