efcore / EFCore.FSharp

Adds F# design-time support to EF Core
MIT License
228 stars 26 forks source link

Migrations don't compile #121

Closed LiteracyFanatic closed 2 years ago

LiteracyFanatic commented 2 years ago

Describe the bug In previous versions, migrations were generating code like this

b.Property<int>("Id")
    .IsRequired(true)
    .ValueGeneratedOnAdd()
    .HasColumnType("int")
    .UseIdentityColumn() |> ignore

but in v6.0.2 it now generates the following

b.Property<int>("Id")
    .IsRequired(true)
    .ValueGeneratedOnAdd()
    .HasColumnType("int")
    .HasDefaultValue(0) |> ignore

    SqlServerPropertyBuilderExtensions.UseIdentityColumn(b, 1L, 1) |> ignore

which fails to compile

error FS0597: Successive arguments should be separated by spaces or tupled, and arguments involving function or method applications should be parenthesized

Code using Navigation also fails to restore the indentation level correctly

modelBuilder.Entity("Server.LetsConnectDomain+LCAccount", (fun b ->

    b.Navigation("LCCustomer") |> ignore

    b.Navigation("LCCustomerAutoMatchFilter") |> ignore

    b.Navigation("LCEvents") |> ignore

    b.Navigation("LCProjectViews") |> ignore

    b.Navigation("LCRefreshTokens") |> ignore

    b.Navigation("LCVendors") |> ignore

    b.Navigation("OwnedSocialGroups") |> ignore
    )) |> ignore

    modelBuilder.Entity("Server.LetsConnectDomain+LCCustomerProjectPost", (fun b ->

        b.Navigation("LCProjectViews") |> ignore
        )) |> ignore

        modelBuilder.Entity("Server.LetsConnectDomain+LCRole", (fun b ->

            b.Navigation("LCAccounts") |> ignore
            )) |> ignore

            modelBuilder.Entity("Server.LetsConnectDomain+LCVendor", (fun b ->

                b.Navigation("LCVendorAnnouncements") |> ignore

                b.Navigation("LCVendorEmployees") |> ignore

                b.Navigation("LCVendorJobPosts") |> ignore
                )) |> ignore

                modelBuilder.Entity("Server.LetsConnectDomain+LCVendorJobPost", (fun b ->

                    b.Navigation("Applications") |> ignore
                    )) |> ignore

                    modelBuilder.Entity("Server.LetsConnectDomain+Resume", (fun b ->

                        b.Navigation("WorkHistoryItems") |> ignore
                        )) |> ignore

To Reproduce Steps to reproduce the behavior: Run a migration after updating to .NET 6.

Expected behavior Migrations should compile successfully.

LiteracyFanatic commented 2 years ago

Found one more code pattern that is broken

b.HasKey("LCVendorID")
    .HasName("LCVendor_PK")
    .IsClustered(false) |> ignore
b.HasKey("LCVendorID")
    .HasName("LCVendor_PK") |> ignore

    SqlServerKeyBuilderExtensions.IsClustered(b.HasKey("LCVendorID"), false) |> ignore
simon-reynolds commented 2 years ago

Hi @LiteracyFanatic Thank you for reporting this. If at all possible, can you provide the definition of LCAccount and any related entities needed to reproduce the issue?

LiteracyFanatic commented 2 years ago

Here is a minimal piece of code that reproduces the UseIdentityColumn and Navigation issues when running an initial migration. Unfortunately, I haven't been able to reproduce the IsClustered issue in a clean project yet.

namespace Test

open System
open System.Collections.Generic
open Microsoft.EntityFrameworkCore
open Microsoft.EntityFrameworkCore.Design
open Microsoft.EntityFrameworkCore.Metadata
open Microsoft.EntityFrameworkCore.Metadata.Builders
open EntityFrameworkCore.FSharp.Extensions

module rec Domain =
    [<AllowNullLiteral>]
    type Account() =
        [<DefaultValue>] val mutable private _Id: int
        [<DefaultValue>] val mutable private _Customer: Customer
        member this.Id with get() = this._Id and set v = this._Id <- v
        member this.Customer with get() = this._Customer and set v = this._Customer <- v

    [<AllowNullLiteral>]
    type Customer() =
        [<DefaultValue>] val mutable private _Id: int
        [<DefaultValue>] val mutable private _AccountId: int
        [<DefaultValue>] val mutable private _Account: Account
        [<DefaultValue>] val mutable private _Profile: Profile
        member this.Id with get() = this._Id and set v = this._Id <- v
        member this.AccountId with get() = this._AccountId and set v = this._AccountId <- v
        member this.Account with get() = this._Account and set v = this._Account <- v
        member this.Profile with get() = this._Profile and set v = this._Profile <- v

    [<AllowNullLiteral>]
    type Profile() =
        [<DefaultValue>] val mutable private _Id: int
        [<DefaultValue>] val mutable private _CustomerId: int
        [<DefaultValue>] val mutable private _Customer: Customer
        member this.Id with get() = this._Id and set v = this._Id <- v
        member this.CustomerId with get() = this._CustomerId and set v = this._CustomerId <- v
        member this.Customer with get() = this._Customer and set v = this._Customer <- v

    type LetsConnectContext() =
        inherit DbContext()

        [<DefaultValue>] val mutable private _Accounts : DbSet<Account>
        member this.Accounts with get() = this._Accounts and set v = this._Accounts <- v

        [<DefaultValue>] val mutable private _Customers : DbSet<Customer>
        member this.Customers with get() = this._Customers and set v = this._Customers <- v

        [<DefaultValue>] val mutable private _Profiles : DbSet<Profile>
        member this.Profiles with get() = this._Profiles and set v = this._Profiles <- v

        override _.OnModelCreating builder =
            builder.RegisterOptionTypes() // enables option values for all entities

        override __.OnConfiguring(options: DbContextOptionsBuilder) : unit =
            options.UseSqlServer(Environment.GetEnvironmentVariable("CONNECTION_STRING")) |> ignore
simon-reynolds commented 2 years ago

Hi @LiteracyFanatic This should be address in v6.0.3 just pushed to NuGet. Looks like the IsClustered issue had the same root cause but let me know if you find any other issues

LiteracyFanatic commented 2 years ago

Thanks for looking at this. I can confirm that this fixed the IsClustered and UseIdentityColumn problems. It seems that there is still a problem when an entity has more than one navigation property though. I've added the navigation Address to Customer from my previous example.

namespace Test

open System
open System.Collections.Generic
open Microsoft.EntityFrameworkCore
open Microsoft.EntityFrameworkCore.Design
open Microsoft.EntityFrameworkCore.Metadata
open Microsoft.EntityFrameworkCore.Metadata.Builders
open EntityFrameworkCore.FSharp.Extensions

module rec Domain =
    [<AllowNullLiteral>]
    type Account() =
        [<DefaultValue>] val mutable private _Id: int
        [<DefaultValue>] val mutable private _Customer: Customer
        member this.Id with get() = this._Id and set v = this._Id <- v
        member this.Customer with get() = this._Customer and set v = this._Customer <- v

    [<AllowNullLiteral>]
    type Customer() =
        [<DefaultValue>] val mutable private _Id: int
        [<DefaultValue>] val mutable private _AccountId: int
        [<DefaultValue>] val mutable private _Account: Account
        [<DefaultValue>] val mutable private _Profile: Profile
        [<DefaultValue>] val mutable private _Address: Address
        member this.Id with get() = this._Id and set v = this._Id <- v
        member this.AccountId with get() = this._AccountId and set v = this._AccountId <- v
        member this.Account with get() = this._Account and set v = this._Account <- v
        member this.Profile with get() = this._Profile and set v = this._Profile <- v
        member this.Address with get() = this._Address and set v = this._Address <- v

    [<AllowNullLiteral>]
    type Profile() =
        [<DefaultValue>] val mutable private _Id: int
        [<DefaultValue>] val mutable private _CustomerId: int
        [<DefaultValue>] val mutable private _Customer: Customer
        member this.Id with get() = this._Id and set v = this._Id <- v
        member this.CustomerId with get() = this._CustomerId and set v = this._CustomerId <- v
        member this.Customer with get() = this._Customer and set v = this._Customer <- v

    [<AllowNullLiteral>]
    type Address() =
        [<DefaultValue>] val mutable private _Id: int
        [<DefaultValue>] val mutable private _CustomerId: int
        [<DefaultValue>] val mutable private _Customer: Customer
        member this.Id with get() = this._Id and set v = this._Id <- v
        member this.CustomerId with get() = this._CustomerId and set v = this._CustomerId <- v
        member this.Customer with get() = this._Customer and set v = this._Customer <- v

    type LetsConnectContext() =
        inherit DbContext()

        [<DefaultValue>] val mutable private _Accounts : DbSet<Account>
        member this.Accounts with get() = this._Accounts and set v = this._Accounts <- v

        [<DefaultValue>] val mutable private _Customers : DbSet<Customer>
        member this.Customers with get() = this._Customers and set v = this._Customers <- v

        [<DefaultValue>] val mutable private _Profiles : DbSet<Profile>
        member this.Profiles with get() = this._Profiles and set v = this._Profiles <- v

        override _.OnModelCreating builder =
            builder.RegisterOptionTypes() // enables option values for all entities

        override __.OnConfiguring(options: DbContextOptionsBuilder) : unit =
            options.UseSqlServer(Environment.GetEnvironmentVariable("CONNECTION_STRING")) |> ignore

The generated code unidents for each call to Navigate like follows

        modelBuilder.Entity("Test.Domain+Customer", (fun b ->

            b.Navigation("Address") |> ignore

        b.Navigation("Profile") |> ignore
    )) |> ignore

Instead it should only unindent for the last call inside the Entity call.

simon-reynolds commented 2 years ago

Resolved as part of work done in #123 v6.0.4 pushed to NuGet

LiteracyFanatic commented 2 years ago

Thanks! I can confirm that calls to Navigation are now indented correctly and the initial migration builds successfully.

It looks like #123 has introduced some other issues that appear in subsequent migrations, but everything mentioned in this issue has been addressed so I've opened #124.