dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.63k stars 3.15k forks source link

Auto-creation of DbSet does not work with F# properties #34356

Closed isaacabraham closed 1 week ago

isaacabraham commented 1 month ago

The following code does not work:

[<CLIMutable>]
type Todo = { Id: Guid; Title: string }

type TodoContext() =
    inherit DbContext()

    member val Todos : DbSet<Todo> = null with get, set

let ctx = new TodoContext()

ctx.Todos // null :(

After construction, Todos is still null.

Workarounds

  1. Manually set Todos:
type TodoContext() =
    inherit DbContext()

    member val Todos = base.Set<Todo>() with get, set // note explicitly setting Todos.

However this is undesirable:

  1. Use verbose field / property syntax. However this is very ugly. I can't even remember having to ever use this syntax before and had to look it up in the docs:
type TodoContext() =
    inherit DbContext()

    [<DefaultValue>]
    val mutable todos: DbSet<Todo>

    member this.Todos
        with get () = this.todos
        and set (value) = this.todos <- value

Suggestion:

I think that the issue is that EF expects the generated field to be a plain text name e.g. todos like in example 2 above. However, F# auto-generated backing-fields have a different naming convention:

(typeof<TodoContext>)
    .GetMembers(Reflection.BindingFlags.Public ||| Reflection.BindingFlags.NonPublic ||| Reflection.BindingFlags.Instance)
|> Seq.filter _.Name.ToLower().Contains("todo")
|> Seq.map (fun x -> x.Name, x.MemberType)
|> Seq.toList

// [("get_todos", Method); ("set_todos", Method); ("todos", Property); ("todos@", Field)]

Note the name of the generated backing field: todos@. I imagine that this is what's causing EF is miss the field out.

This is unfortunate because it's the first thing that someone uses EF in F# will fall foul of, leaving a negative impression. There's no documentation on this, and it won't cause a compile time error - just a runtime null ref exception.

roji commented 1 month ago

Makes sense, thanks for the report and the analysis.

We're unfortunately not F# experts, and have received little requests for improve F# support in EF over the years (of course, that may be a vicious cycle where support isn't good enough for F# users to use EF).

We'd be happy to receive some help here, if you (or anyone else) is willing to chip in. We basically need a new EFCore.FSharp.FunctionalTests F# project where we can start adding test coverage for various F# scenarios (it can be modeled on the existing EFCore.VisualBasic.FunctionalTests project). You're very welcome to send a PR doing that; if doing the actual fix is too much, it's OK to just add the testing infrastructure with the failing test, and we can do the actual fix.

How does that sound?

isaacabraham commented 1 month ago

Sure, I'll give it a go - it'll probably just have a single unit test in there to start with.

roji commented 1 month ago

@isaacabraham thanks for the PR - I can see the test failing. However, I looked into it, and this doesn't seem to have anything to do with the backing field or anything like that; I can clearly see the Todos property being injected with a DbSet by EF's DbSetInitializer, but then the initialization of the Todos property in your test code overwrites that:

member val Todos : DbSet<CliMutableRecord> = null with get, set

I know very little about F#, but this seems like a simple initialization ordering issue that has nothing to do with EF...

isaacabraham commented 1 month ago

Thanks for looking at this. That's good to know. My understanding is that "auto properties" in F# always need a default value to be provided; I believed that they are baked into the constructor of the type: see here and see the description for member val. Evidently, that initialisation is happening after EF does its thing. I'm sure someone with a better understanding of the underlying implementation of auto-properties in F# can confirm this or not - @vzarytovskii / @cartermp / @dsyme ?

baronfel commented 4 weeks ago

Here's a quick sharplab showing the decompiled C# version of an F# type with a member initializer:

The generated code projected as C# is:

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Reflection;
using System.Runtime.CompilerServices;
using Microsoft.FSharp.Core;

[assembly: FSharpInterfaceDataVersion(2, 0, 0)]
[assembly: AssemblyVersion("0.0.0.0")]

[CompilationMapping(SourceConstructFlags.Module)]
public static class @_
{
    [Serializable]
    [CompilationMapping(SourceConstructFlags.ObjectType)]
    public class MyDb
    {
        internal HashSet<string> Todos@;

        public HashSet<string> Todos
        {
            [CompilerGenerated]
            [DebuggerNonUserCode]
            get
            {
                return Todos@;
            }
            [CompilerGenerated]
            [DebuggerNonUserCode]
            set
            {
                Todos@ = value;
            }
        }

        public MyDb()
        {
            Todos@ = null;
        }
    }
}

namespace <StartupCode$_>
{
    internal static class $_
    {
        public static void main@()
        {
        }
    }
}

So in the constructor of the type the F# compiler does emit an explicit set of null here.

isaacabraham commented 4 weeks ago

Thank you @baronfel - much appreciated. So, this would imply that EF is doing something (somehow?) before the ctor of the MyDb object fires? @roji can you elaborate a little on how EF goes about finding and setting DbSet properties on an object?

ajcvickers commented 4 weeks ago

@isaacabraham EF uses reflection to generate and compile a dynamic method that initializes the DbSet properties. But its entirely optional; I rarely use it these days. Instead I just call DbContext.Set() to get a cached DbSet when I need it.

roji commented 4 weeks ago

Yeah, as @ajcvickers says finding a fix/workaround for this isn't really necessary - F# users can simply explicitly assign the properties (as in the first code sample above, as "Workaround"). Though there's lots of EF sample code out there which doesn't do this, and it's true that a naive F# user starting out would probably hit this problem.

Will keep this open a little longer for discussion, but otherwise I'm not sure there's anything we can do here...

baronfel commented 4 weeks ago

So to be clear, you're recommending something like the following?

member val Todos : DbSet<CliMutableRecord> = DbContext.Set() with get, set

What's DbContext here, the 'this' parameter for this newly type, to a static member on the DbContext base class, or something else?

roji commented 4 weeks ago

Set<T>() is a just a method on DbContext, which is the superclass of the type we're discussing here. See these C# docs to see a pattern being recommended - note that in that code it's a getter-only property (so Set() gets called every time), if high performance is really important it's also possible to initialize auto-properties by calling Set() in the constructor.

isaacabraham commented 4 weeks ago

Yeah, as @ajcvickers says finding a fix/workaround for this isn't really necessary - F# users can simply explicitly assign the properties (as in the first code sample above, as "Workaround"). Though there's lots of EF sample code out there which doesn't do this, and it's true that a naive F# user starting out would probably hit this problem.

Will keep this open a little longer for discussion, but otherwise I'm not sure there's anything we can do here...

This would be really unfortunate IMHO. I'd love to get parity here with the C# experience and the standard docs. I'm still unclear as to why the standard route doesn't work when it's being set to null in the ctor?

I appreciate the "use the workaround" but that feels as though we're doing that because the standard way doesn't work, rather than that being the optimal way to go. Are we really saying there's nothing that can be done?

roji commented 4 weeks ago

@isaacabraham unless I'm mistaken, the situation is as follows... The constructor of EF's DbContext class uses reflection to discover DbSet properties, and populate them with non-null DbSets; this works well with C#. But F#, the compiler emits an explicit setting of the property to null, overwriting the value set by the base class constructor; this does not occur in C#, which explains why the pattern works in C# but not in F#. As this is a language/compiler difference, I don't see what can be done here - but I'm open to suggestions.

Note that as @ajcvickers wrote above, this runtime reflection-based method of discovering and initializing DbSets... isn't great; it causes issues in other places as well (e.g. nullable reference types), and is generally quite "magical". I think that if we were designing EF Core from scratch today, we probably wouldn't implement it (keep in mind that EF Core needed to be largely compatible with the previous non-core EF6). So I don't think it's a huge loss if this doesn't work in F# - we just need to document it properly. A dedicated page in the EF docs detailing usage with F# could help.

isaacabraham commented 4 weeks ago

Ok, that makes much more sense now: the base constructor is doing all the work which is then being "overwritten" by the child constructor (which obviously hasn't been run yet when the base ctor is running). As you have alluded to, I guess this was always a bit of an anti-pattern i.e. a base-class constructor modifying data of the derived class which may or may not have been initialised yet.

I'm all for removing magic anyway so this seems like a good way forward.

I don't believe that there is any other option in terms of F# object construction, so yes, it seems like documentation and a "don't follow this guide, it won't work with F#" is the best way forward.

@baronfel you can also just call base.Set(), that works.

isaacabraham commented 4 weeks ago

Given all of this - @roji - what would you suggest we do with the PR?

  1. Can it completely.
  2. Fix it (I can change that to a test proving that it is always null and add another test showing the "correct" way of doing it)?
roji commented 4 weeks ago

@isaacabraham I'd be happy if we merged even a minimal working F# test - that would at least get us started with an F# test suite which we could evolve as needed. I wouldn't bother with a test showing that it's null - one minimal/simple working test would be great. If you can update the PR for that, that would be great.

isaacabraham commented 4 weeks ago

Done

roji commented 1 week ago

Closing this as the problem with DbSet isn't related to EF, but rather to F# itself.

I've opened https://github.com/dotnet/EntityFramework.Docs/issues/4789 to track an F# documentation page to point out all the gotchas a beginner may need to know - please don't hesitate to post there with similar addition difficulties.