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

Stop configuring lazy-loading for properties that will always be eager-loaded #12462

Closed
ajcvickers opened this issue Jun 25, 2018 · 5 comments
Closed
Assignees
Labels
area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@ajcvickers
Copy link
Member

For example, for owner to owned navigation properties in aggregates.

See also #10787 (comment)

@ajcvickers ajcvickers added this to the 2.2.0 milestone Jun 29, 2018
@ajcvickers ajcvickers self-assigned this Jun 29, 2018
@ajcvickers ajcvickers changed the title Stop configuring lazy-loading for properties that will always been eager-loaded Stop configuring lazy-loading for properties that will always be eager-loaded Jun 29, 2018
@ajcvickers ajcvickers modified the milestones: 2.2.0, 3.0.0, Backlog Aug 3, 2018
@ajcvickers ajcvickers modified the milestone: Backlog Jun 3, 2019
@ajcvickers ajcvickers added the good first issue This issue should be relatively straightforward to fix. label Sep 2, 2019
@ajcvickers ajcvickers modified the milestones: Backlog, 5.0.0 Feb 18, 2020
@lajones lajones self-assigned this Feb 18, 2020
@lajones
Copy link
Contributor

lajones commented Feb 18, 2020

Notes from discussion with @ajcvickers and @smitpatel:

Define "properties that will always be eager loaded" as an INavigation (either reference or collection) which:

  • has IsEagerLoaded == true
  • has GetTargetType().IsOwned == true

In that case set up such that a) we do not need the virtual keyword, and b) check that attempting to lazy-load should be a no-op as we should already have eager-loaded.

@AndriySvyryd
Copy link
Member

  • has IsEagerLoaded == true

This isn't safe as we'll add an opt-out

@lajones
Copy link
Contributor

lajones commented Feb 19, 2020

@AndriySvyryd @smitpatel What would be safe?

@AndriySvyryd
Copy link
Member

@lajones

  • navigation.ForeignKey.IsOwnership == true for tracked enitites
  • navigation.ForeignKey.IsOwnership == true && navigation.IsOnDependent == false for not tracked

@lajones
Copy link
Contributor

lajones commented Feb 20, 2020

We don't know whether the entity is being tracked at model validation time (which is when the current error is thrown). After more discussion with @AndriySvyryd and @ajcvickers we'll do something like:

if (navigation.ForeignKey.IsOwnership)
{
    if (!navigation.IsOnDependent)
    {
        // no-op (do not throw and do not warn)
    }
    else
    {
        // do not throw - this will be eager loaded
    }
}
else
{
    // throw as normal
}

@lajones lajones added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed needs-design labels Feb 27, 2020
@lajones lajones removed the good first issue This issue should be relatively straightforward to fix. label Feb 27, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview2 Mar 13, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview2, 5.0.0 Nov 7, 2020
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

4 participants