Entity equality support for non-anonymous DTOs

[roji]

Also various modifications to support MemberMemberBinding better (RelationalProjectionBindingExpressionVisitor, ReplacingExpressionVisitor, ExpressionEqualityComparer)

Fixes #16789

There's probably some specific support for MemberMemberBinding missing from RelationalProjectionBindingExpressionVisitor but I think we can take care of that later.

Note that we still don't support MemberListBinding.

[roji]

PS there seems to be considerable potential for DRY (e.g. repeated code between RelationalProjectionBindingExpressionVisitor, InMemoryProjectionBindingExpressionVisitor, CosmosProjectionBindingExpressionVisitor). Any objection to me taking a look?

[ajcvickers]

Remember to re-target to release/3.0 before merging.

[roji]

@ajcvickers done

[smitpatel]

While changings look fine, we are not adding support for MemberMemberBinding in 3.0. It was not supported in past. There is only 1 test. In order to add support for whole new expression structure like that, we need to test and make sure it works with

• EE
• Nav expansion
• Translation of non-select in terms of rebinding
• Projection bindings
• Materialization

It would be a risky change to put it in for RTM given lack of time and testing.

cc: @ajcvickers

[smitpatel]

PS there seems to be considerable potential for DRY (e.g. repeated code between RelationalProjectionBindingExpressionVisitor, InMemoryProjectionBindingExpressionVisitor, CosmosProjectionBindingExpressionVisitor). Any objection to me taking a look?

A lot of internal states. By making it DRY we may put ourselves in corner in terms of breaking changes.

[roji]

While changings look fine, we are not adding support for MemberMemberBinding in 3.0. It was not supported in past. There is only 1 test. In order to add support for whole new expression structure like that, we need to test and make sure it works with

But at this point nothing is working with MemberMemberBinding anyway, so we wouldn't be risking or breaking anything, right? Whatever scenarios this unblocks - great - and what it doesn't wouldn't have worked anyway, so we can take care of issues as they come? Put another way, is there a downside/risk to adding partial support for this, even if it's not complete?

@ajcvickers @smitpatel let me know what you decide, I can back out this work.

PS there seems to be considerable potential for DRY (e.g. repeated code between RelationalProjectionBindingExpressionVisitor, InMemoryProjectionBindingExpressionVisitor, CosmosProjectionBindingExpressionVisitor). Any objection to me taking a look?

A lot of internal states. By making it DRY we may put ourselves in corner in terms of breaking changes.

All I meant was moving duplicated methods and state to a single internal base class - since everything stays internal I don't think there's a problem with breaking changes. It seems a bit dangerous to have to do identical changes in three different places.

[smitpatel]

Internal code in EF core which providers depend on? We don't do such patterns.

[ajcvickers]

@roji @smitpatel I chatted with Smit and I think if there are only specific case(s) of MemberMemberBinding that the code knows how to deal with, and we have tests for those cases, then it's fine to keep as long as we are confident that the cases we don't support don't just do the wrong thing, but instead throw. That being said, supporting types of expression that we didn't support before is not a priority--we could easily punt it to after 3.0.

[smitpatel]

MemberAssignment supported
Throw for MemberMemberBinding & MemberListBinding

[roji]

@ajcvickers @smitpatel OK, not sure why we're throwing away valid code already written but I'll back out all support for MemberMemberBinding.

Internal code in EF core which providers depend on? We don't do such patterns.

Seems strange to me - everything is already internal now, so refactoring internal with internal to avoid duplication is only an improvement, no? Unless we were planning to make this visitor public?

[ajcvickers]

@roji I obviously misunderstand. Why are you backing this out?

[smitpatel]

Provider.Internal does not depend on EFCore.Internal.

[ajcvickers]

@smitpatel So you're saying for a provider to use this it must depend on EF internal code?

[ajcvickers]

@roji Are we talking about two different things here? DRYing up is fine, but public surface has to be reviewed and signed off this late in the game, so doesn't seem worth it to me. We can do this later.

With regard to code that understands different bindings, I'm fine with that if we are confident in the change and it doesn't introduce new, untested paths.

[roji]

@ajcvickers unless I'm misunderstanding, @smitpatel is asking that I back out all changes that I did to support MemberMemberBinding, because they are not complete support for this expression type and not fully-tested. I personally believe we should consider merging, but I'm OK to back out and revisit later. You can discuss with him.

So you're saying for a provider to use this it must depend on EF internal code?

Providers can reimplement their own ProjectionBindingExpressionVisitor from scratch - that's what Cosmos, InMemory and Relational are doing. However, there's considerable code duplication between them.

I'm not proposing adding anything public, just DRYing common code from three largely-duplicated classes in providers into a common base class in EFCore.Internal.

Provider.Internal does not depend on EFCore.Internal.

I'm not sure where these rules come from :)

[smitpatel]

Making anything DRY between different providers require to put the code in EF.Core assembly. Anything in EFCore which providers depend on needs to be made public. Providers don't depend on types inside .Internal namespace in base EFCore provider. Provider code being inside Internal namespace is irrelevant.

[ajcvickers]

@roji I pinged you on Teams. This is quite concerning to me--we should follow up as a priority.

[roji]

@ajcvickers explained the situation in teams.

To sum up the way I understand it:

1. We have substantial duplication across the three current implementations we have (Relational, Cosoms, InMemory)
2. A new non-relational provider would likely need to duplicate the code once again (but without depending on anything internal).
3. Therefore, there could be a case for making this public at some point. I don't think we absolutely have to spend time on an public API right now, or lock ourselves in at the last minute.
4. Assuming we don't make this public right now (for pragmatic reasons), why make our lives difficult and have lots of duplicated code, when it can be refactored?

[roji]

After offline discussion, we'll keep the code duplication to avoid having our providers depend on internal EFCore code.

Regardless, updated this PR to not add any MemberMemberBinding support.

Re-targeted

[roji]

Thanks @smitpatel

[roji]

Note: opened #16867 to track adding MemberMemberBinding and linked to the work already done, which was backed out of this PR.