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

Composite primary key with foreign keys -- zero is treated as non-existing #31212

Closed
macias opened this issue Jul 9, 2023 · 5 comments
Closed
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@macias
Copy link

macias commented Jul 9, 2023

File a bug

Maybe it is related to: #23730

Include your code

Full project: https://codeberg.org/macias/lala/src/branch/master/app/Playground/DbDemo

Short description -- I have User type and its primary key is composite one depending on foreign keys. When I used valid zero value EF reports the value is missing.

Shortened code -- models:

public sealed class IntCompany
{
    public int Id { get; set; }
    public string Name { get; set; } = null!;
}

public sealed class IntLanguage
{
    public int Id { get; set; }
    public string Name { get; set; } = null!;
}

public sealed class IntUser
{
    public string Name { get; set; } = null!;
    public int LanguageId { get; set; }
    public int CompanyId { get; set; }
}

Relations and so on:

        modelBuilder.Entity<IntCompany>(entity =>
        {
            entity.HasKey(e => e.Id);

            entity.Property(e => e.Id)
                .ValueGeneratedOnAdd();
        });
        
        modelBuilder.Entity<IntLanguage>(entity =>
        {
            entity.HasKey(e => e.Id);

            entity.Property(e => e.Id)
                .ValueGeneratedOnAdd();
        });
        
        modelBuilder.Entity<IntUser>(entity =>
        {
            entity.HasKey(e => new { e.LanguageId, e.CompanyId });

            entity.Property(e => e.LanguageId);

            entity.Property(e => e.CompanyId);


            entity.HasOne<IntLanguage>()
                .WithMany()
                .HasForeignKey(p => p.LanguageId)
                .OnDelete( DeleteBehavior.Restrict);
            
            entity.HasOne<IntCompany>()
                .WithMany()
                .HasForeignKey(p => p.CompanyId)
                .OnDelete( DeleteBehavior.Restrict);
        });

Execution part:

                ctx.Database.ExecuteSqlRaw("INSERT INTO IntLanguages (Id,Name) VALUES (0,'some');");

                ctx.IntLanguages.Add(new IntLanguage() {Name = "hello"});

                ctx.SaveChanges();

                ctx.IntCompanies.Add(new IntCompany() {Name = "mine"});

                ctx.SaveChanges();

                var company_id = ctx.IntCompanies.Single().Id;

                Console.WriteLine($"Languages {(String.Join(", ", ctx.IntLanguages.Select(it => it.Id)))}");

                Console.WriteLine("Creating first user");
                ctx.IntUsers.Add(new IntUser() {LanguageId = 1, CompanyId = company_id, Name = "joe"});
                ctx.SaveChanges();

                Console.WriteLine("Creating second user");
                ctx.IntUsers.Add(new IntUser() {LanguageId = 0, CompanyId = company_id, Name = "jane"});
                ctx.SaveChanges();

The second user which tries to bind to language 0 is not saved.

Include stack traces

Unhandled exception. System.InvalidOperationException: The value of 'IntUser.LanguageId' is unknown when attempting to save changes. This is because the property is also part of a foreign key for which the principal entity in the relationship is not known.
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.PrepareToSave()
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.GetEntriesToSave(Boolean cascadeChanges)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(StateManager stateManager, Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.<>c.<SaveChanges>b__107_0(DbContext _, ValueTuple`2 t)
   at Microsoft.EntityFrameworkCore.Storage.NonRetryingExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges()
   at DbDemo.Program.WithInts() in /home/mist/Projekty/lala/app/Playground/DbDemo/Program.cs:line 44
   at DbDemo.Program.Main(String[] args) in /home/mist/Projekty/lala/app/Playground/DbDemo/Program.cs:line 14
Aborted (core dumped)

Include version information

Microsoft.Data.Sqlite version: 7.0.8
Target framework: .NET 7.0
Operating system: openSUSE 15.3

@ajcvickers
Copy link
Contributor

@macias The way the code is set up there will never be a zero IntCompany.Id or IntLanguage.Id, because these properties are configured to have generated values, which means if the value is zero on insert, then EF will make sure that a non-zero value is generated. If you really need to use zero as a valid key value, then you probably can't use EF value generation.

@macias
Copy link
Author

macias commented Jul 13, 2023

@ajcvickers Thank you for your answer.

I can see few things here.

When you are generating keys, this strategy is OK. It is simply a rule in EF Core, because EF core in such scenario plays an active role.

But this case is not about generating identifiers. It is about referring to some values, and currently (by your comment) I assume EF Core does some eager validation and reject the incoming data. In my opinion it is not only not consistent but wrong approach.

First -- consistency:

The record which is about to be saved:

PK: autoinc | Field1 (FK against table A) : 0 | Field2 (FK against table B) : 0

What EF Core says "Both table A and B have their PK auto-generated, nevertheless let those 0s pass, let database decide".

And now let's cut PK and make both fields composite PK:

(PK)Field1 (FK against table A) : 0 | (PK)Field2 (FK against table B) : 0

What EF Core says "By EF Core rules 0 is invalid value for tables A and B so let's throw an error right now, no point relying on database".

So in first case EF Core relies on DB, in the it knows better. It is surprising and not consistent.

But please do not make it consistent by clipping non-PK usage :-).

The second issue is this -- any relational database checks its relations (or in general sense constraints). If you start to replacing this logic on client side it would be best to have 1:1 coverage, if not -- and here it is the case, it means you are limiting greatly usage of EF Core. For example EF Core cannot work valid data.

So in short, please do not interfere with relational constraints of database, leave it do database, if foreign key (for example) is wrong, DB will tell about it for sure. If FK is hitting against autoinc PK with value "-5" it is not valid or invalid because it is less than zero, but because there is or there is not such PK value in the database.

@ajcvickers
Copy link
Contributor

@macias Thanks for your thoughts. However, EF Core doesn't work this way. It relies on an accurate model of the database. We then use that to our best to implement useful behaviors based on the semantics of that model. One of those behaviors is not saving an FK that has not been explicitly set, which is a common source of errors.

@macias
Copy link
Author

macias commented Jul 19, 2023

@ajcvickers
' One of those behaviors is not saving an FK that has not been explicitly set'

I gave you an example that this statement is not true.

Secondly, not "set" but "assumed to be not set", because I am by all means setting it, it is just EF Core assumes it is not set.

To sum this up, I don't see anything wrong on relying on DB to sort out references, this is what DB is for. And with just FK this is how it works, but when FK is also part of PK, EF Core is more suspicious and relies on its own logic making using existing data in some cases impossible.

Update Given the fact that obviously I am failure when it comes to convincing ;-) I am afraid that you notice the FK alone does not behave according to your statement and the "fix" would be to even tighter grip over the data. So taking my original report both two cases would fail, not only the one when PK is composed out of two FK. Thus -- how about option which would control it? Default would be current one, EF Core additionally controls FK values, with option like allowing only database enforce FK constraints, EF Core would rely on DB. Would it be possible?

@ajcvickers
Copy link
Contributor

@macias This is not something we plan to implement.

@ajcvickers ajcvickers added closed-no-further-action The issue is closed and no further action is planned. and removed area-adonet-sqlite labels Jul 26, 2023
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

2 participants