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

One-to-one relationship with bad data behaviors differently for bi-directional verses uni-directional relationships #8137

Closed
JonPSmith opened this issue Apr 12, 2017 · 20 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@JonPSmith
Copy link

JonPSmith commented Apr 12, 2017

I am building some tests/examples for one-to-one relationships, and I found that EF Core differs in its handling of the relationship if an inverse navigational relationship exists or not:

  1. One-to-one relationship without a inverse navigational relationship correctly saves data
  2. One-to-one relationship with a inverse navigational relationship detaches data that has a duplicate one-to-one entity

I found this with a test to show that duplicate one-on-one entities should throw an exception: in case 1 an exception is thrown if I have a duplicate. In case 2 no exception is thrown.

UPDATE: I have found change in behaviour is due to all but one of the entities that has a duplicate one-to-one relationship are set to Detached after SaveChanges is called. They have not been saved to the database, so they don't trigger the exception on a UNIQUE constraint.

NOTE: I agree this seems very strange and it could be my mistake, but I have looked at it and can't see anything wrong with my code. Let me know if you spot anything.

Steps to reproduce

My main entity

public class Attendee 
{
    public int AttendeeId { get; set; }
    public string Name { get; set; }

    public int TicketId { get; set; } 
    public Ticket Ticket { get; set; }
}

My sub-entity

public class Ticket
{
    public enum TicketTypes : byte { Guest = 0, VIP = 1, Staff = 3}

    public int TicketId { get; set; }
    public TicketTypes TicketType { get; set; }

    public Attendee Attendee { get; set; }
}

My configuration
NOTE: The configuration shown does NOT work (case 2), but if I replace the WithOne(p => p.Attendee) with .WithOne() then it works.

public static class AttendeeConfig
{
    public static void Configure
        (this EntityTypeBuilder<Attendee> entity)
    {
        entity.HasOne(p => p.Ticket) //#A
            .WithOne(p => p.Attendee)
            //.WithOne()
            .HasForeignKey<Attendee>(p => p.TicketId);
    }
}

My DbContext

public class Chapter07DbContext : DbContext
{
    public DbSet<Employee> Employees { get; set; }
    public DbSet<EmployeeShortFk> EmployeeShortFks { get; set; }
    public DbSet<Person> People { get; set; }
    public DbSet<LibraryBook> LibraryBooks { get; set; }

    //One-to-One versions
    public DbSet<Attendee> Attendees { get; set; }
    public DbSet<Ticket> Tickets { get; set; }

    public Chapter07DbContext(
        DbContextOptions<Chapter07DbContext> options)
        : base(options)
    { }

    protected override void OnModelCreating
        (ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Attendee>().Configure();
    }
}

My unit test is

[Fact]
public void TestOption1OneToOneDuplicateTicketBad()
{
    //SETUP
    using (var context = new Chapter07DbContext(
        SqliteInMemory.CreateOptions<Chapter07DbContext>()))
    {
        {
            var logs = new List<string>();
            SqliteInMemory.SetupLogging(context, logs);
            context.Database.EnsureCreated();

            //ATTEMPT
            var dupTicket = new Ticket {TicketType = Ticket.TicketTypes.Guest};
            var attendees = new List<Attendee>
            {
                new Attendee {Name = "Person1", Ticket = dupTicket, },
                new Attendee {Name = "Person2", Ticket = dupTicket, },
                new Attendee {Name = "Person3", Ticket = new Ticket()}
            };
//The following code has been added to diagnose the issue
            context.AddRange(attendees);
            ListStates(context, "After AddRange", attendees);
            context.SaveChanges();
            ListStates(context, "After SaveChanges", attendees);

            //var ex = Assert.Throws<DbUpdateException>(() => context.SaveChanges());

            //VERIFY
            //ex.InnerException.Message.ShouldEqual("SQLite Error 19: 'UNIQUE constraint failed: Attendees.TicketId'.");
        }
    }
}

private void ListStates(DbContext context, string message,  List<Attendee> entities)
{
    var line = message + ": " + string.Join(", ", entities.Select(x => context.Entry(x).State));
    _output.WriteLine(line);
}

Notes

The output of state of the Attendee entities in case 2 (the failure state) are:

After AddRange: Added, Added, Added
After SaveChanges: Detached, Unchanged, Unchanged

Further technical details

EF Core version: 1.1.1
Database Provider: Microsoft.EntityFrameworkCore.Sqlite
Operating system: Windows 10
IDE: Visual Studio 2017

@MaherJendoubi
Copy link

@JonPSmith Please share your Chapter07DbContext and your ConfigureServices method in Startup.cs file.

@JonPSmith
Copy link
Author

@MaherJendoubi . I have added my Chapter07DbContext to the original post.

@MaherJendoubi
Copy link

@JonPSmith How about ConfigureServices()? to reproduce the issue.

@JonPSmith
Copy link
Author

I don't have a ConfigureServices(). This is a unit test using in-memory Sqlite, which I have used in other issues I have posted here.

@MaherJendoubi
Copy link

@JonPSmith Would you please share a repro project sample?

@MaherJendoubi
Copy link

@JonPSmith I didn't succeed to reproduce the issue because of this :
image

@MaherJendoubi
Copy link

image

@JonPSmith
Copy link
Author

JonPSmith commented Apr 12, 2017

OK. The repo is at https://github.com/JonPSmith/EfCoreInAction and you need branch Chapter07. Be aware I am adding other items, so it may get out of date in weeks to come. It good as of today. look for unit test TestOption1OneToOneDuplicateTicketBad in test class Ch07_OneToOneRelationships.

Some of the things you need are already available.

  • The modelBuilder.Entity<Attendee>().Configure(); is in the first post (I hadn't put the outer static class on it, but its there now)
  • ShouldEqual is from the XUnit fluent assertions

About the only thing you need is the SqlLite in-memory, but you can use SqlServer. Its just some code to set up the SqlLite in-memory.

PS. You don't need the logging. I use that to check how EF Core builds the database.

@ajcvickers
Copy link
Member

@JonPSmith I'm finding it a little hard to understand exactly what you are describing, but two things may help clarify the behaviors of EF:

  • If EF finds a reference navigation property pointing from one type to another but does not find an inverse navigation pointing the other way, then EF doesn't know for sure whether this is a one-to-one or a one-two-many relationship. Depending on the semantics of the domain model (which, of course, EF doesn't know about) either could make sense. Based on experience about what people do in real world apps and what works more often than not we went with a convention to make this a one-to-many relationship. So, based on your model above, if you remove one of the navigation properties, then you get a one-to-many relationship.
  • You say the "configuration does not work" for case two, but if you remove the lambda that specifies the missing navigation property then it does work. I'm not sure what your expectations are here, but this is the way the API is designed. This is how the multiplicity of the relationship is specified explicitly, and therefore how the code overrides the convention described above to make the relationship one-to-one. It might help to read through the Getting Started documentation on relationships here: https://docs.microsoft.com/en-us/ef/core/modeling/relationships

@ajcvickers ajcvickers self-assigned this Apr 12, 2017
@ajcvickers ajcvickers added this to the 2.0.0 milestone Apr 12, 2017
@JonPSmith JonPSmith changed the title One-to-one relationship stops working if inverse navigational relationship is added One-to-one relationship with inverse navigational relationship added stops saving of data Apr 13, 2017
@JonPSmith
Copy link
Author

@ajcvickers,

Thanks for your comments - they make me go back to the unit test and look more closely at the database etc. It turns out there is not difference in the database/EF Core configuration between both cases - they both create a one-to-one relationship with a UNIQUE constraint on the FK.

This made me look further and I found that not all of the entities were being written to the database. I have updated my initial post with the new information, but basically the call to SaveChanges detaches all but one of the entities that has a duplicate one-on-one relationship. I didn't expect that!

I have no idea what is happening here, but that is what the problem is. I have created a branch Chapter07Bug which has the updated test - Look for unit test TestOption1OneToOneDuplicateTicketBad in test class Ch07_OneToOneRelationships.

@ajcvickers
Copy link
Member

@JonPSmith This is what is happening:

  • The first Attendee is tracked by the context and brings in the associated Ticket. Fixup happens to make sure the FKs an navigation properties are consistent. Everything is in the Added state--nothing exists in the database.
  • The second Attendee is tracked. It points to the existing Ticket, so it "steals" this Ticket from the first Attendee. Navigation properties and FKs are fixed up appropriately for this new relationship. Since the relationship is required and the first Attendee is in the Added state it cannot be deleted (because it doesn't exist in the database) so it is detached. In other words, you first told EF to insert this new Attendee, but now you are telling EF to insert a different Attendee, so EF knows not to insert the first one and detaches it.
  • The same thing happens with the third Attendee.

The result is that, as part of tracking the entities, the relationship was changed twice to associate different Tickets and Attendees.

@JonPSmith
Copy link
Author

@ajcvickers,

OK, that was not obvious, but I can see how that works. Another learning point.

Not sure I see why the two cases - with inverse navigation and without inverse navigation affect that, but don't bother to explain as you have far better things to do.

@ajcvickers
Copy link
Member

@JonPSmith The two cases should behave the same way, so something is not working correctly. I will investigate further.

@ajcvickers ajcvickers reopened this Apr 13, 2017
@ajcvickers ajcvickers changed the title One-to-one relationship with inverse navigational relationship added stops saving of data One-to-one relationship with bad data behaviors differently for bi-directional verses uni-directional relationships Apr 13, 2017
@JonPSmith
Copy link
Author

JonPSmith commented Apr 18, 2017

@ajcvickers

I found a difference between the two. The SQL tables creation is identical, but...

  1. Case 1 (partially defined) - catches duplicate tickets and throws exception
    A Index is created for the AttendeeId foreign key in the Ticket entity. This is the Sqlite code:
CREATE INDEX "IX_Tickets_AttendeeId" ON "Tickets" ("AttendeeId");
  1. Case 2 (fully defined) - does not throw an exception on duplicate ticket, but detaches one of the attendees with a duplicate ticket.
    No index is created.

Because EF Core makes decisions to detach an entity prior to writing the data to the database I assume this is a symptom and not the cause. But maybe this will help diagnose the issue, as I would expected a required relationship like this should have a unique index.

PS. I'm glad you have marked this as a bug, as I was tearing my hair out trying to understand why it was working this way!

@ajcvickers ajcvickers modified the milestones: 2.0.0-preview1, 2.0.0 Apr 19, 2017
@JonPSmith
Copy link
Author

Hi @ajcvickers,

I see you have marked this as a bug, and the milestone is 2.0. My chapter 7, on configuring relationships, will come out before Version 2.0 is out, so can I check on what is wrong with the relationship so I can note this.

My tests show that this issue arises if a fully defined one-to-one relationship is configured by convention or via Fluent API. The only difference I can see between what I expect (and see in a partially defined one-to-one relationship) is that a SaveChange does not fail on a duplicate principal, but all preceding dependent entities that contains the duplicate principal will not be written to the database (they are detached).

Is there anything else I missed?

@ajcvickers
Copy link
Member

ajcvickers commented Apr 20, 2017

@JonPSmith The general concept here is that once you have a one-to-one relationship, then assigning any entity to that relationship will cause the newly assigned entity to replace any entity that is already there. Replacing the entity that is already there can mean different things:

  • If cascade deletes are enabled (normal for required relationships), then it is deleted
  • Without cascade deletes with an optional relationship, then the FK set to null
  • Without cascade deletes with a required relationship, then an exception should be thrown

However, if the entity has not yet been saved (i.e. it is in the Added state), then there is nothing to be deleted from the entity, so deleting really means "not saving", and hence it will be detached.

I think a principle here is that EF will attempt to do what makes sense given that your model is correct and your use of EF is geared towards doing valid things. Trying to do invalid things (like trying to create multiple conflicting dependents) and then trying to have EF tell you this isn't really an appropriate use of EF.

@JonPSmith
Copy link
Author

@ajcvickers,

Thanks for that. I agree that EF Core does a sensible (actually quite clever) action in that case. But you have marked this as a bug - I assume because the fully defined relationship exhibits a different action to the partially defined relationship. I just wanted to know how you plan to change it, again I assume you will change it to work the same as the partially defined relationship.

Further tests shows that the most probable duplicate error, which is a duplicate being added in another DbContext session, does throw an exception. I therefore think the difference between the two cases is quite small. I therefore think I won't worry about this in the book, but I will mark the failing unit test with this issue number.

I'm happy with that.

@ajcvickers
Copy link
Member

@JonPSmith Yes, the reason the issue is open is because of the different behaviors, but I won't have a crisp idea of what is going on until I investigate more.

@AndriySvyryd AndriySvyryd self-assigned this Jun 29, 2017
AndriySvyryd added a commit that referenced this issue Jul 1, 2017
…stealing using navigations and/or FK properties

Mark the dependent as detached/deleted when breaking a required relationship even if the FK doesn't overlap PK

Fixes #8137
AndriySvyryd added a commit that referenced this issue Jul 1, 2017
… new one is reference stealing using navigations and/or FK properties

If the FK was non-identifying make sure the properties are set to null instead of the dependent being removed.

Fixes #8137
@ajcvickers ajcvickers removed their assignment Jul 3, 2017
AndriySvyryd added a commit that referenced this issue Jul 3, 2017
… new one is reference stealing using navigations and/or FK properties

If the FK was non-identifying make sure the properties are set to null instead of the dependent being removed.

Fixes #8137
@AndriySvyryd AndriySvyryd removed their assignment Jul 3, 2017
@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 Jul 3, 2017
@ajcvickers
Copy link
Member

@AndriySvyryd Wait, throw where? We should only throw if you attempt to null out an FK property on a required relationship without cascade delete and then call SaveChanges. Is that what you mean?

@AndriySvyryd
Copy link
Member

@ajcvickers Yes, seems that my comment doesn't actually clarify anything, so I'll delete it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

4 participants