Skip to content

"non-nullable property with a nullable backing field" example doesn't work in practice #1994

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Costo opened this issue Dec 17, 2019 — with docs.microsoft.com · 9 comments · Fixed by #2024
Closed
Assignees
Milestone

Comments

Copy link
Contributor

Costo commented Dec 17, 2019

Note: I'm using EF Core 2.2

I tried to apply the pattern described in this article of using a backing field for non-nullable navigation properties:

public Address ShippingAddress
{
    set => _shippingAddress = value;
    get => _shippingAddress
           ?? throw new InvalidOperationException("Uninitialized property: " + nameof(ShippingAddress));
}

When I do this, I get an error because EF reads the property when materializing entities: "System.InvalidOperationException: Uninitialized property: RouteDetail"

Call stack:

System.InvalidOperationException: Uninitialized property: RouteDetail
   at ***.get_RouteDetail() in ***
   at Microsoft.EntityFrameworkCore.Metadata.Internal.ClrPropertyGetter`2.GetClrValue(Object instance)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.ReadPropertyValue(IPropertyBase propertyBase)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.SetProperty(IPropertyBase propertyBase, Object value, Boolean setModified)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.NavigationFixer.SetNavigation(InternalEntityEntry entry, INavigation navigation, InternalEntityEntry value)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.NavigationFixer.InitialFixup(InternalEntityEntry entry, ISet`1 handledForeignKeys, Boolean fromQuery)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.NavigationFixer.TrackedFromQuery(InternalEntityEntry entry, ISet`1 handledForeignKeys)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntryNotifier.TrackedFromQuery(InternalEntityEntry entry, ISet`1 handledForeignKeys)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.MarkUnchangedFromQuery(ISet`1 handledForeignKeys)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.StartTrackingFromQuery(IEntityType baseEntityType, Object entity, ValueBuffer& valueBuffer, ISet`1 handledForeignKeys)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryBuffer.StartTracking(Object entity, IEntityType entityType)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryBuffer.IncludeCollectionAsync[TEntity,TRelated,TElement](Int32 includeId, INavigation navigation, INavigation inverseNavigation, IEntityType targetEntityType, IClrCollectionAccessor clrCollectionAccessor, IClrPropertySetter inverseClrPropertySetter, Boolean tracking, TEntity entity, Func`1 relatedEntitiesFactory, Func`3 joinPredicate, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.Internal.IncludeCompiler._IncludeAsync[TEntity](QueryContext queryContext, TEntity entity, Object[] included, Func`5 fixup, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.TaskLiftingExpressionVisitor._ExecuteAsync[T](IReadOnlyList`1 taskFactories, Func`2 selector)
   at Microsoft.EntityFrameworkCore.Query.Internal.AsyncLinqOperatorProvider.AsyncSelectEnumerable`2.AsyncSelectEnumerator.MoveNext(CancellationToken cancellationToken)
   at System.Linq.AsyncEnumerable.SelectEnumerableAsyncIterator`2.MoveNextCore(CancellationToken cancellationToken) in D:\a\1\s\Ix.NET\Source\System.Interactive.Async\Select.cs:line 106
   at System.Linq.AsyncEnumerable.AsyncIterator`1.MoveNext(CancellationToken cancellationToken) in D:\a\1\s\Ix.NET\Source\System.Interactive.Async\AsyncIterator.cs:line 98
   at Microsoft.EntityFrameworkCore.Query.Internal.AsyncLinqOperatorProvider.ExceptionInterceptor`1.EnumeratorExceptionInterceptor.MoveNext(CancellationToken cancellationToken)
   at System.Collections.Generic.AsyncEnumerableHelpers.ToArrayWithLength[T](IAsyncEnumerable`1 source, CancellationToken cancellationToken) in D:\a\1\s\Ix.NET\Source\System.Interactive.Async\AsyncEnumerableHelpers.cs:line 48
   at System.Collections.Generic.AsyncEnumerableHelpers.ToArray[T](IAsyncEnumerable`1 source, CancellationToken cancellationToken) in D:\a\1\s\Ix.NET\Source\System.Interactive.Async\AsyncEnumerableHelpers.cs:line 16
   at ***.GetRoutes(Int32 routePlanId, DateTimeOffset startTime) in ***
***snip***

Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@ajcvickers
Copy link
Contributor

@roji It looks like the guidance for non-nullable navigations is incorrect; As far as I can tell, the pattern described will not work most of the time.

@roji
Copy link
Member

roji commented Dec 17, 2019

@Costo can you please try with EF Core 3.1? At least with the following trivial sample everything seems to work well, if you're still hitting and issue could you please post a similar code sample?

class Program
{
    static void Main(string[] args)
    {
        using (var ctx = new BlogContext())
        {
            ctx.Database.EnsureDeleted();
            ctx.Database.EnsureCreated();
            ctx.Blogs.Add(new Blog { Name = "Blog1" });
            ctx.SaveChanges();
        }

        using (var ctx = new BlogContext())
        {
            var blogs = ctx.Blogs.ToList();
        }
    }
}

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder.UseSqlServer(@"...");
}

public class Blog
{
    string _name;
    
    public int Id { get; set; }

    public string Name
    {
        get => _name ?? throw new InvalidOperationException("Uninitialized property: " + nameof(Name));
        set => _name = value;
    }
}

@ajcvickers in the latest EF source code I can't see ReadPropertyValue being called by SetProperty, so I'm guessing this is maybe a 2.2 issue?

@ajcvickers
Copy link
Contributor

@roji There are plenty of places we read the property when it could be null--for example, in fixup. I just be missing something about why this pattern is expected to work for anything but trivial graphs.

@roji
Copy link
Member

roji commented Dec 17, 2019

OK. We can discuss briefly in design, but if that's the way things work we should remove this guidance and just recommend to annotate the property as non-nullable (and initialize to null), with the assumption it will always be populated on entities returned by EF Core. It's not ideal but I guess that's that.

@Costo
Copy link
Contributor Author

Costo commented Dec 19, 2019

@roji I modified your example slightly to add a Post entity and introduced a navigation property: Blog <-> Post.

I can confirm that this program works with EF Core 3.1 (no error), but when switching to EF Core 2.2, I get the exception "System.InvalidOperationException: Uninitialized property: Blog".

#nullable enable
using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.EntityFrameworkCore;

class Program
{
    static void Main(string[] args)
    {
        using (var ctx = new BlogContext())
        {
            ctx.Database.EnsureDeleted();
            ctx.Database.EnsureCreated();
            var blog = new Blog { Name = "Blog1" };
            var post = new Post { Title = "Post Title", Blog = blog };
            ctx.AddRange(blog, post);
            ctx.SaveChanges();
        }

        using (var ctx = new BlogContext())
        {
            var blogs = ctx
                .Blogs
                .Include(x => x.Posts)
                .ToList();
        }
    }
}

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; } = default!;
    public DbSet<Post> Posts { get; set; } = default!;

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder.UseSqlite("Data Source=mydb.db;");
}

public class Blog
{
    public int Id { get; set; }
    public string Name { get; set; } = default!;
    public IList<Post> Posts { get; set; } = new List<Post>();
}

public class Post
{
    private Blog? _blog;
    public int Id { get; set; }
    public string Title { get; set; } = default!;
    public Blog Blog
    {
        get => _blog ?? throw new InvalidOperationException("Uninitialized property: " + nameof(Blog));
        set => _blog = value;
    } 
}

@roji
Copy link
Member

roji commented Dec 19, 2019

@Costo thanks for testing - so do I understand correctly that you have no issues whatsoever on 3.1?

@ajcvickers I understand you're thinking of our scenarios where this would be a problem - even in 3.1 - so I'll keep this open for changing the guidance (out of curiosity, though, can you provide a quick example?).

@Costo
Copy link
Contributor Author

Costo commented Dec 19, 2019

@roji Correct, I have no issues with this sample program on 3.1.
I want to convert a big, less trivial application to 3.1 in Q1 2020. I'll keep you updated if I run into this issue again with 3.1.

@ajcvickers
Copy link
Contributor

@roji I missed that this is explicitly making use of the change we made in 3.0 to use backing fields directly. The reason EF is not calling the property getter here is because it knows the backing field and is configured to use it directly. It would probably be useful to update the documentation to make it explicit that:

  • EF needs to know the backing field. In this case EF finds it by convention. In other cases it may need to be specified explicitly.
  • EF needs to be configured to use the backing field. 2.x doesn't do this by default. 3.x does, but only when the backing field is known.
  • Suggest using PropertyAccessMode.Field since this causes EF to generate an error if no field is found.

@roji
Copy link
Member

roji commented Jan 3, 2020

@ajcvickers that makes perfect sense, submitted #2024 to add a note on this.

roji added a commit that referenced this issue Jan 14, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants