npgsql / efcore.pg

Entity Framework Core provider for PostgreSQL
PostgreSQL License
1.53k stars 223 forks source link

Null values if a property not change value, Default value and Scaffold-DBContext #1758

Closed Morhpeuss closed 3 years ago

Morhpeuss commented 3 years ago

Hi,

I have a table with two integer columns Opens and UniqueOpens. They have a default value set to 0. I scaffold DB with Scaffold-DBContext. I don't know why, but the default value is not set on these columns.

I create a new empty DB, applied migrations, tables were created (without default values on these columns).

I want to insert a data:

es = new EmailDeviceStatistic()
                        {
                            Date = deviceStatisticResult.StatisticDate,
                            Id = Guid.NewGuid(),
                            Device = ds.Name
                        };

                        es.Opens = 0;
                        es.UniqueOpens = 0;

                        counter++;
                        this.Context.EmailDeviceStatistics.Add(es);
                        await this.Context.SaveChangesAsync();`

Nothing special. As you can see I set values of Opens and UniqueOpens to 0 (default value of integer). By saving I get an error: "Failing row contains (4f43b05f-82a1-412b-93bb-f137c2545a52, 2021-02-13, Desktop, null, null)."

If I set these properties to 1 (other from zero), there is no problem and a record is created. It looks like, If you don't change the property value, the provider decides that I want to insert NULL and because the column does not have a default value and is set as NOT NULL, the save fails.

So I have 2 questions

  1. why Scaffold-Context does not create a default value on a column
  2. why if I don't change a default value is there a null
Morhpeuss commented 3 years ago

@roji could you please check this issue? thank you.

Morhpeuss commented 3 years ago

@roji Is there any support, please?

vonzshik commented 3 years ago

@Morhpeuss Hello, sorry it took so long: @roji and I were a little busy with a new feature for the provider.

I'm afraid I has not been able to reproduce the problem - is it possible to provide a migration with the model (or even better, a full repro)?

Morhpeuss commented 3 years ago

@roji Thank you for a response.

I found that the problem is in Scaffolding.

The repro steps:

I created (was generated) a model in my primary project with this properties:

builder.Entity<EmailMarketing.Models.Data.EmailDeviceStatistic>()
              .Property(p => p.UniqueOpens)
              .HasDefaultValueSql("0");

        builder.Entity<EmailMarketing.Models.Data.EmailDeviceStatistic>()
              .Property(p => p.Opens)
              .HasDefaultValueSql("0");

In my second project which I would like to use for migrations, I scaffolded a migration context (Scaffold-DbContext "connection string" Npgsql.EntityFrameworkCore.PostgreSQL -OutputDir Models -force -Context DataContext).

Generated model:

modelBuilder.Entity<EmailDeviceStatistic>(entity =>
            {
                entity.ToTable("EmailDeviceStatistic");

                entity.HasIndex(e => e.Date, "ix_emaildevicestatistic_date")
                    .HasNullSortOrder(new[] { NullSortOrder.NullsLast })
                    .HasSortOrder(new[] { SortOrder.Descending });

                entity.HasIndex(e => new { e.Date, e.Device }, "ix_emaildevicestatistic_date_device");

                entity.Property(e => e.Id).HasDefaultValueSql("uuid_generate_v1()");

                entity.Property(e => e.Date).HasColumnType("date");

                entity.Property(e => e.Device)
                    .IsRequired()
                    .HasMaxLength(150);
            });

As you can see, in this context is not the default value for columns Opens and UniqueOpens don't have a default value.

In the migration project, I added migration and I apply this migration in the primary project. Then I have a database without default values for columns Opens and UniqueOpens, but the context in the primary project expects that these columns have default values.

In this case. If I set 0 to the fields Opens and UniqueOpens, the context ignores (value has not been changed) this change and expects that the DB inserts the default value.

If the ScaffoldDB generates correctly default values to the model, it won't happen.

roji commented 3 years ago

It seems like you are trying to mix two different DbContexts (one written manually, one scaffolded), each with its own corresponding database schema - that's not really expected to work. Is there any reason why you're scaffolding the second context rather than always just using the first one, which is manually written?

When EF Core scaffolds an existing database, it by design does not scaffold default column values which correspond to CLR defaults - so the default value of 0 does not get scaffolded.

Morhpeuss commented 3 years ago

The problem is, that the columns in the database have the default value, but Scaffold-DB ignores these default values.

I will explain it. The first context is generated by Radzen. He scaffolds the database with the default values and generates right model definitions:

builder.Entity<EmailMarketing.Models.Data.EmailDeviceStatistic>()
              .Property(p => p.UniqueOpens)
              .HasDefaultValueSql("0");

        builder.Entity<EmailMarketing.Models.Data.EmailDeviceStatistic>()
              .Property(p => p.Opens)
              .HasDefaultValueSql("0");

The second is scaffolded by Scaffold-DB

modelBuilder.Entity<EmailDeviceStatistic>(entity =>
            {
                entity.ToTable("EmailDeviceStatistic");

                entity.HasIndex(e => e.Date, "ix_emaildevicestatistic_date")
                    .HasNullSortOrder(new[] { NullSortOrder.NullsLast })
                    .HasSortOrder(new[] { SortOrder.Descending });

                entity.HasIndex(e => new { e.Date, e.Device }, "ix_emaildevicestatistic_date_device");

                entity.Property(e => e.Id).HasDefaultValueSql("uuid_generate_v1()");

                entity.Property(e => e.Date).HasColumnType("date");

                entity.Property(e => e.Device)
                    .IsRequired()
                    .HasMaxLength(150);
            });

There are no default values.

roji commented 3 years ago

@Morhpeuss for why the default value of 0 isn't scaffolded, see https://github.com/dotnet/efcore/issues/9627.

What I'm still not sure about, is my question above:

Is there any reason why you're scaffolding the second context rather than always just using the first one, which is manually written?

Morhpeuss commented 3 years ago

@roji I believe more related is https://github.com/dotnet/efcore/issues/11735 .

I am confused about the EF core team approached. They wrote: "the meaning of "HasDefaultValueSql" is that there is a default constraint in the database which the EF runtime should make use of. In this case, the change to reverse engineering was explicit in that the EF runtime should not make use of the default constraint."

That means, we can not rely on the scaffolded context that he is a "copy" of a database. Very often we design the database, where we define constraints, default values, etc. Then we scaffold the database, add the migrations, and assume that the application is able to create the DB, with constraints, default values, etc. But now, it is not true.

So we have to go back and again, create a backup of the database, restore and then start the application because we can not rely on the migration, that he will create the database with all constraints, etc.

roji commented 3 years ago

I believe more related is dotnet/efcore#11735

Yep, that issue refers back to dotnet/efcore#9627, they are pretty much dups.

That means, we can not rely on the scaffolded context that he is a "copy" of a database. Very often we design the database, where we define constraints, default values, etc. Then we scaffold the database, add the migrations, and assume that the application is able to create the DB, with constraints, default values, etc. But now, it is not true.

EF Core intentionally does not attempt to provide database "round-trippability" - if you scaffold a database and then use migrations to recreate it, the resulting schema isn't guaranteed are supposed to be identical to the source schema. You can create an issue on the EF Core repo to open this discussion if you wish, but it's not something that's specifically related to the PostgreSQL provider.

However, I still don't exactly understand why your flow requires you to round-trip your database in this way... You may be doing things in a way which could be simplified.