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

Improve exception message when reconfiguring an owned type #27705

Closed
ghost opened this issue Mar 25, 2022 · 10 comments · Fixed by #32260
Closed

Improve exception message when reconfiguring an owned type #27705

ghost opened this issue Mar 25, 2022 · 10 comments · Fixed by #32260
Labels
area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@ghost
Copy link

ghost commented Mar 25, 2022

It seems like the configuration (Fluent API) for an Owned type entity is ignored. I've change the configuration for the UserData entity from normal (User and UserData had their own configs) to owned type (now owned by User entity).

Strangely it's now changing all columntypes and some other stuff which was configured correctly before to "longtext" and also ignoring additional attributes like non-nullable (IsRequired()).

I couldn't find helpful solutions and tried different ways (except Attributes inside the class) without luck. Am I missing something here?

EF Core entity configuration inside User entity

internal class UserConfiguration : IEntityTypeConfiguration<User>
{
  public void Configure(EntityTypeBuilder<User> builder)
  {
    // additional code removed for brevity

    builder.OwnsOne(user => user.UserData, build =>
    {
      build.ToTable("UserData");

      build.HasKey(x => x.Id);
      build.Property(x => x.Id).ValueGeneratedOnAdd();

      // every config here results in change to "longtext" inside migration script
      build.Property(x => x.Salutation).HasColumnType("varchar(15)").IsRequired();
      build.Property(x => x.FirstName).HasColumnName("FirstName").HasColumnType("varchar(50)").IsRequired();
      build.Property(x => x.Gender).HasColumnName("Gender").HasMaxLength(50).IsRequired();
      build.Property(x => x.FamilyName).HasMaxLength(50).IsRequired();
      build.Property(x => x.BirthName).HasMaxLength(50);
      });
    }
  }
}

EF Core created migration script (copy&paste of related parts)

 migrationBuilder.AlterColumn<string>(
   name: "UserId",
  table: "UserData",
  type: "varchar(22)",
  maxLength: 22,
  nullable: false,
  oldClrType: typeof(int),
  oldType: "int")
  .Annotation("MySql:CharSet", "utf8mb4");

migrationBuilder.AlterColumn<string>(
  name: "Salutation",
  table: "UserData",
  type: "longtext",
  nullable: true,
  oldClrType: typeof(string),
  oldType: "varchar(15)",
  oldMaxLength: 15)
  .Annotation("MySql:CharSet", "utf8mb4")
  .OldAnnotation("MySql:CharSet", "utf8mb4");

migrationBuilder.AlterColumn<string>(
  name: "FirstName",
  table: "UserData",
  type: "longtext",
  nullable: true,
  oldClrType: typeof(string),
  oldType: "varchar(50)",
  oldMaxLength: 50)
  .Annotation("MySql:CharSet", "utf8mb4")
  .OldAnnotation("MySql:CharSet", "utf8mb4");

migrationBuilder.AlterColumn<string>(
  name: "Gender",
  table: "UserData",
  type: "longtext",
  nullable: true,
  oldClrType: typeof(string),
  oldType: "varchar(100)",
  oldMaxLength: 100)
  .Annotation("MySql:CharSet", "utf8mb4")
  .OldAnnotation("MySql:CharSet", "utf8mb4");

migrationBuilder.AlterColumn<string>(
  name: "FamilyName",
  table: "UserData",
  type: "longtext",
  nullable: true,
  oldClrType: typeof(string),
  oldType: "varchar(50)",
  oldMaxLength: 50)
  .Annotation("MySql:CharSet", "utf8mb4")
  .OldAnnotation("MySql:CharSet", "utf8mb4");

migrationBuilder.AlterColumn<string>(
  name: "BirthName",
  table: "UserData",
  type: "longtext",
  nullable: true,
  oldClrType: typeof(string),
  oldType: "varchar(50)",
  oldMaxLength: 50,
  oldNullable: true)
  .Annotation("MySql:CharSet", "utf8mb4")
  .OldAnnotation("MySql:CharSet", "utf8mb4");

EF Core version: 6.0.3 (latest)
Database provider: Pomelo.EntityFrameworkCore.MySql (latest)
Target framework: .NET 6.0
Operating system: Wndows 11
IDE: JetBrains Rider 2021.3.3 or VS 2022 17.1.2 (doesn't matter)

@ghost ghost added the customer-reported label Mar 25, 2022
@ajcvickers
Copy link
Member

@DanielDudda I am not able to reproduce this. Please attach a small, runnable project or post a small, runnable code listing that reproduces what you are seeing so that we can investigate.

@ddudda174
Copy link

ddudda174 commented Apr 5, 2022

@ajcvickers thanks for looking at it.

I've reduced the original project's code to only contain the required parts to show this bug. I've already run the dotnet ef migration command and still the properties are set as longtext. Here's a link.

I'm using docker compose to run the MariaDB server instance and connect to the .net project. Maybe it has something to do with it or some configurations I've set?

@ajcvickers
Copy link
Member

@ddudda174 Given that GitHub repo, what are the steps to reproduce what you are seeing? (I get unrelated errors attempting to start the app.)

@ddudda174
Copy link

@ajcvickers Given the current state of the GitHub repo I would do the following steps to reproduce it (nothing special here):

  • remove last migration
  • update database
  • make some pseudo changes (e.g. change MaxLength value on some property)
  • add new migration with specific name
  • look into newly created migration class inside Migrations folder
  • owned type properties will then be listed with 'longtext'

I'm currently using JetBrains Rider IDE with the Entity Framework Core UI Plugin, which simply provides a more convenient way to call the dotnet ef-core commands and filling parameters like "-project *.csproj" for me. Also tried using the Package Manager Console and the "Add-Migration " command inside Visual Studio 2022, same result.

Just noticed that the max-length value for the custom-id I'm using is correct. I've configured it via the "ConfigureConventions" method, but configuring the max-length inside the EntityTypeConfiguration class has no effect.

protected override void ConfigureConventions(ModelConfigurationBuilder configurationBuilder)
        {
            configurationBuilder.Properties<DbId>()
                                .HaveConversion<DbIdConverter>()
                                .HaveMaxLength(DbId.MAX_LENGTH);
        }

(What errors do you receive, maybe we can fix 'em?)

@ajcvickers
Copy link
Member

@ddudda174 Does that bad model get created when you run the application, or only when you create the migration?

@ddudda174
Copy link

ddudda174 commented Apr 20, 2022

@ajcvickers what do you mean by bad model? The entity PersonalData was created by me, and setup inside the UserConfig class with OwnsOne. After that I run the ef core command (e.g. Add-Migration TestOwnedTypeConfig) to generate the migration file which comes up with the longtext column types instead of for example varchar(64) for property "FirstName". When I simply run the app(without creating a migration and running Update-Database) then it will cause exceptions which lead to "the database does not fit the current model" error message. Am i missing something here?

I've also deleted everything and started from scratch to generate a new snaptshot class, but without success. As a side note, this was also a former .NET 5 project that was upgraded to latest .NET 6 versions including nuget packages.

@ajcvickers
Copy link
Member

@ddudda174 The issue here is that the code is configuring PersonalData as an owned type, but then also configuring it as a non-owned type in SeedAdmin, and then attempting to suppress the error that this generates by completely ignoring it in AppDbContext. Since it is ignored, it probably shouldn't be included in the model at all, and hence there should be no table creation for it in the migration--so this is likely a bug. However, the real issue here is that PersonalData can't be both configured as an owned type, and then later used as a non-owned type for seeding.

@ddudda174
Copy link

@ajcvickers Just saw that owned types should be seeded after a OwnsOne call, correct? So my common seeding inside SeedAdmin with HasData causes it to be recognized as non-owned type (thought using anonymous object on seeding is enough/ok)? EF Core came up with the error that it is configured as non-owned type and cannot be used as owned type and I should add ignore if I don't want to use it as non-owned type. That's why I added it as I couldn't see why it was using it as non-owned type. So I try to change it to the correct seed method, but will be annoying to scather around seed data across all entities instead of using centralized methods.

Since it is ignored, it probably shouldn't be included in the model at all, and hence there should be no table creation for it in the migration--so this is likely a bug

At least I could unveal a probably real bug :D

@ajcvickers
Copy link
Member

Note for triage: we should look at the working of the owned and non-owned message, since Ignore is very rarely the way to solve this.

The entity type '{entityType}' cannot be configured as owned because it has already been configured as a non-owned. If you want to override previous configuration first remove the entity type from the model by calling 'Ignore'. See https://aka.ms/efcore-docs-owned for more information and examples.

@ddudda174

At least I could unveal a probably real bug :D

I doubt we will fix this, since it requires a completely invalid configuration in order to get into this situation.

@ddudda174
Copy link

ddudda174 commented Apr 27, 2022

@ajcvickers you can close this issue if you want (or keep it open for the non-owned message rework), I fixed the configuration and it's now working. Thanks for your help!

Just one more question: Is it ok to use OwnsOne twice to separate configuration and data seeding or should I better merge the calls (already tested and it's working, just wondering for best practice)?

@ajcvickers ajcvickers added this to the MQ milestone May 2, 2022
@AndriySvyryd AndriySvyryd changed the title Can not specify Properties of Owned type via Fluent API Improve exception message when reconfiguring an owned type Mar 8, 2023
AndriySvyryd added a commit that referenced this issue Nov 8, 2023
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 8, 2023
AndriySvyryd added a commit that referenced this issue Nov 8, 2023
AndriySvyryd added a commit that referenced this issue Nov 14, 2023
@ajcvickers ajcvickers modified the milestones: MQ, Backlog, 9.0.0 Nov 14, 2023
@ajcvickers ajcvickers modified the milestones: 9.0.0, 9.0.0-preview1 Jan 31, 2024
@roji roji removed this from the 9.0.0-preview1 milestone Oct 12, 2024
@roji roji added this to the 9.0.0 milestone Oct 12, 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. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants