dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.52k stars 3.13k forks source link

Allow FKs to exist in the model but avoid creating them in the database #15854

Open chrsas opened 5 years ago

chrsas commented 5 years ago

Note: This issue is about only not creating the constraint in the database. EF will still treat the relationship as constrained. If you want an unconstrained relationship--that is, a relationship where an FK value is non-null but there is no corresponding PK in the database--then please vote for #13146.


I try to use RemoveForeignKey to remove all foreign keys from my DbContext, but it is not usable. Test DbContext

public class Order
{
    public Guid Id { get; set; }

    public string Code { get; set; }

    public IList<OrderDetail> OrderDetails { get; set; }
}
public class OrderDetail
{
    public Guid Id { get; set; }

    public Guid OrderId { get; set; }

    public int Quantity { get; set; }
}
public class BloggingContextFactory : IDesignTimeDbContextFactory<ConsoleDbContext>
{
    public ConsoleDbContext CreateDbContext(string[] args)
    {
        var optionsBuilder = new DbContextOptionsBuilder<ConsoleDbContext>();
        optionsBuilder.UseSqlServer("Server=.;Database=Blogging;Integrated Security=True");

        return new ConsoleDbContext(optionsBuilder.Options);
    }
}

public class ConsoleDbContext : DbContext
{
    public DbSet<Order> Orders { get; set; }

    public DbSet<OrderDetail> OrderDetails { get; set; }

    public ConsoleDbContext(DbContextOptions<ConsoleDbContext> dbContextOptions) : base(dbContextOptions)
    {
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        foreach (var mutableEntityType in modelBuilder.Model.GetEntityTypes())
        {
            if (mutableEntityType.ClrType == null)
                continue;
            // delete all foreign key
            foreach (var foreignKey in mutableEntityType.GetForeignKeys().ToList())
            {
                foreignKey.DeclaringEntityType.RemoveForeignKey(foreignKey.Properties, foreignKey.PrincipalKey,
                    foreignKey.PrincipalEntityType);
            }
        }       
    }
}

The foreign key is still in the generated Snapshot.

[DbContext(typeof(ConsoleDbContext))]
partial class ConsoleDbContextModelSnapshot : ModelSnapshot
{
    protected override void BuildModel(ModelBuilder modelBuilder)
    {
gma warning disable 612, 618
        modelBuilder
            .HasAnnotation("ProductVersion", "2.2.4-servicing-10062")
            .HasAnnotation("Relational:MaxIdentifierLength", 128)
            .HasAnnotation("SqlServer:ValueGenerationStrategy", SqlServerValueGenerationStrategy.IdentityColumn);

        modelBuilder.Entity("ConsoleApp.Order", b =>
            {
                b.Property<Guid>("Id")
                    .ValueGeneratedOnAdd();

                b.Property<string>("Code");

                b.HasKey("Id");

                b.ToTable("Orders");
            });

        modelBuilder.Entity("ConsoleApp.OrderDetail", b =>
            {
                b.Property<Guid>("Id")
                    .ValueGeneratedOnAdd();

                b.Property<Guid>("OrderId");

                b.Property<int>("Quantity");

                b.HasKey("Id");

                b.HasIndex("OrderId");

                b.ToTable("OrderDetails");
            });

        modelBuilder.Entity("ConsoleApp.OrderDetail", b =>
            {
                b.HasOne("ConsoleApp.Order")
                    .WithMany("OrderDetails")
                    .HasForeignKey("OrderId")
                    .OnDelete(DeleteBehavior.Cascade);
            });
gma warning restore 612, 618
    }
}

Further technical details

EF Core version: .Net Core 2.2 Database Provider: Microsoft.EntityFrameworkCore.SqlServer 2.2.4 Operating system: IDE: Visual Studio 2019 16.1.1

ajcvickers commented 1 year ago

@houbi56 It's not committed for 8.0. At this point, I think it's unlikely to make the cut.

atrauzzi commented 1 year ago

I wish the quality of life and improvement of existing functionality wouldn't always get punted.

ErikEJ commented 1 year ago

@atrauzzi many quality of life items have been added to EF Core 8! Just not this one.

atrauzzi commented 1 year ago

That's great, but it seems like too many features are developed (or reimplemented from EF6) with an "MVP" mindset. This doesn't take into consideration some degree of predictable flexibility that will users are inevitably going to need.

I find it similar to the Azure mentality at Microsoft which is "get to GA at all costs" and you end up with way too many gaps. EF Core just has the benefit and past experiences of <= EF6 behind it.

I wish more effort was done in the research and planning phase of things to ensure at least some corner scenarios ship, versus backlogging all the quality of life and now it's death from a thousand cuts...

roji commented 1 year ago

@atrauzzi we really do care about corner cases and quality of life issues, and try very hard not to release "minimally-viable" features - try looking at the issues fixed e.g. for 7.0 and I think you'll see that.

At the end of the day, everyone has their own particular set of features they care about for their particular scenario, and if those don't get implemented in a timely fashion, the subjective impression is that the team is neglecting or mis-prioritizing things. We're a small team and we do our best when prioritizing - some users are bound to be disappointed.

atrauzzi commented 1 year ago

I know @roji -- It's not a commentary on the overall quality, but more I guess of quantity, haha.

I wish they'd give you more resources. Send a link to this comment up the chain... :wink:

roji commented 1 year ago

You and me both :)

sertifiscott commented 1 year ago

Adding to the chain of this is needed.

roji commented 1 year ago

Everyone, please refrain from posting more comments saying "I need this" - these don't help us prioritize this issue. Make sure you vote (:+1:) on the top post, we do look at that.

berhir commented 1 year ago

We manually removed all the foreign keys from the migrations and it worked fine at first glance. But we are using an SQLite DB and it turned out that there are many cases when EF migrations must rebuild a table. And when the table gets rebuilt, all foreign keys get added again and there is no way to customize the migration because the rebuild happens magically behind the scenes. In such cases, a lot of migration code must be written manually. See dotnet/EntityFramework.Docs#4429

OpenSpacesAndPlaces commented 1 year ago

@berhir See if the following helps your use case: https://github.com/dotnet/efcore/issues/15854#issuecomment-850671874

bricelam commented 1 year ago

Yes, on SQLite, you can disable all the database logic for foreign keys by adding Foreign Keys=False (added in EF Core 3.0) to your connection string. Note, this is really only necessary when the native library was compiled with SQLITE_DEFAULT_FOREIGN_KEYS.

The foreign keys will still be created in the SQL, but SQLite will simply ignore them.

berhir commented 11 months ago

Thanks, I am aware of this flag, and I have already used it successfully. But in our current project I don't want to disable ALL foreign keys, just some of them. Seems this case doesn't work well with EF Core and SQLite and requires a lot of manual migration code in case a table rebuild is required.

bricelam commented 11 months ago

Our implementation of table rebuilds assumes that your EF model is in sync with the database. Instead of altering the database schema after a migration is generated, you should alter the EF model to reflect what you want the database to look like. But alas, removing the foreign keys from the EF model would require this issue to be implemented.

berhir commented 11 months ago

@bricelam I have the same problem with triggers, they get removed too. Unfortunately, my suggestion I mentioned above to make the table rebuilds transparent by adding the code to the generated migration was moved to the docs repo. Not sure how better docs can solve this. It's off topic for this issue, so maybe you can add a comment there why this is not possible/planned to implement and if there are workarounds for triggers, thanks: https://github.com/dotnet/EntityFramework.Docs/issues/4429

marchy commented 6 months ago

You and me both :)

@roji If you need some ammo, we are a native, mobile-first tech startup, who have left the Windows ecosystem over a decade ago, run Swift and Kotlin as our primary languages, and have every reason to have our back-end stack be anything OTHER than MS/.NET.

EXCEPT for the fact that we value domain-driven design and the best technology to meet that promise in any ecosystem is EF Core. Therefore fine, we take on requiring a different tech stack, language, IDE, cloud provider (.NET, C#, Rider and Azure respectively) all so that we can access this unique ORM - itself made possible by C#’s equally unique Expressions API (which has become very stagnant unfortunately and stuck in pre-nullable era).

I am the CEO of this company, and have had every chance to choose otherwise, despite the recruitment challenges of running a .NET backend stack in the startup world.

THAT is how paramount ORM technology is. Is the very lifeblood of the backend stack - MUCH more than the endpoint wrappers (ASP.NET/Functions) or infrastructure components (Azure) that support/expose it.

Please DO pass this message up the chain and seriously ask for a doubling/tripling of the team in the year ahead - because we would not be on Azure and we would not be on .NET if it wasn’t for Entity Framework. Fact.

You guys need more people like us succeeding, spreading the word and getting others to incorporate the best-of-each-world paradigm for each piece of the tech stack —> particularly why we run iOS/Swift native, Kotlin/Android native, and C#/.NET/Azure for back-end, because for crunching data and modeling your business domain it has the potential to be the best technology in any tech stack period. In a world where MS lost on the entire mobile front and cloud is the best strategic focus for the company as a whole - it is in Microsoft’s core interest to win as a leader in the cloud space, with EF as the heart of that differentiation in the market.

But you guys badly need more throughput and to fill in all the holes in the promises to actually fulfill the standout potential at hand.

PS: This is NOT a vote for the importance of this particular issue, we don’t have need for this specific scenario. It is a comment on the very valid and broad gaps on edge case / quality of life / MVP mindset which has been consistently prominent in the development of EF Core. I don’t think you guys are necessarily off to hyper-prioritize across horizontally rather than knocking one single feature completely out of the park (though arguments could likely be made there), but there IS a big disconnect between the resources the team actually has VS what is intrinsically needed to tackle the underlying challenges at hand. The result is that feature after feature for the past 3+ EF releases we cannot adopt any of it because no POC of major new EF functionality ever makes it to production-grade readiness. Not one.

Please make the case - and go get - a doubling/tripling of the EF team. It’s that simple.

💪 🚀

roji commented 6 months ago

@marchy thank you for writing all that - it is greatly appreciated - really (and may indeed be helpful in internal discussions). We get surprisingly little messages like this one, explaining the actual place of EF in users' actual, concrete projects and considerations - I wish we had more like these, both positive and negative.

We're indeed aware that there are quite a few rough edges, and various features aren't "complete" (or don't always interoperate well with other features).. That's indeed something we'd like to make better. But I'd be interested in knowing more specifics about recent EF features which you consider unusable/not production-grade; if you're interested in having that conversation, please write to the email on my github profile (so as to prevent over-spamming this issue).

Thanks again for taking the time to write the above! ❤️

atrauzzi commented 6 months ago

Holy wow, yeah agreed on all counts @marchy.

@roji - If you want business cases... Hi, I'm Alex :tm:. As of this comment, I'm an architect at one of the worlds largest travel companies. In the past, I've held similar roles at smaller, successful companies where I've done everything from migrate to .NET Core, to introducing .NET anew. I've also moved companies away from Azure as a consequence of the sloppy out-of-our-hands issues and gaps we faced. My background prior to .NET was in the very strong Symfony/Laravel ecosystems, so the bar is very high. Just like marchy mentioned, EF and all the existing features he mentionied are enough of a game changer to legitimize the platform. But man, EF is often just coasting on its past success and as good as it is, if EF or LINQ just suddenly stopped tomorrow, I would probably pick up something on the JVM with Kotlin, or switch over to Erlang based backends.

Wearing my psuedo-anthropological hat, I think the model Microsoft has adopted today is far too press-release vs. product-quality oriented. The MVP approach eluded to by marchy actually plagues all of Microsoft. The consistency in gaps between teams and departments makes it obvious that there's something top-down going on here and the reputational cost isn't being factored for.

I've become fairly accustomed to .NET features and technologies only getting enough depth to pass a stage demo. The consequence? Most new EF features aren't having a big impact on making my life as a developer easier. How many stale topics are festering, waiting for enough votes on to get certain JSON or polymorphism features in place? Yet release after release, I see them "pushed to 6, pushed to 7, pushed to 8, pushed to 9". I couldn't disagree more with the mentality that these things should be decided by popular vote. Don't farm out your products vision to the masses, and if you're stretched thin, don't pick things that aren't improving quality of life.

And I get it, 'softies probably grumble when I show up, poking holes in your product strategies and trying to offer insights. I even bet some think I'm ungrateful. But the treatment I receive sometimes and the gaps are just so consistently similar in nature and origin. We're your customers and I've absolutely made decisions based on my experiences that impact your bottom line.

pellet commented 6 months ago

If anyone runs into this issue when trying to disable the foreign key constraints whilst using SQLite(I did when unit testing) the following code worked for me:

var connection = new SqliteConnection("Data Source=:memory:;Foreign Keys=False");
connection.Open();
var contextOptions = new DbContextOptionsBuilder<DbContext>()
    .UseSqlite(connection)
    .Options;
var context = new DbContext(contextOptions)

If you'd prefer use a mssql db, the following c#/sql code drops all the foreign keys in the db:

var q = @"declare @sql nvarchar(max) = (
    select 
        'alter table ' + quotename(schema_name(schema_id)) + '.' +
        quotename(object_name(parent_object_id)) +
        ' drop constraint '+quotename(name) + ';'
    from sys.foreign_keys
    for xml path('')
);
exec sp_executesql @sql;";
context.Connection.Query(q);
robkeimig commented 6 months ago

I found my way here after about an hour of banging my head against:

'Introducing FOREIGN KEY constraint 'FK_Lol_No_Sql_For_You' on table 'ForeignKeysSuck' may cause cycles or multiple cascade paths. 
Specify ON DELETE NO ACTION or ON UPDATE NO ACTION, or modify other FOREIGN KEY constraints.
Could not create constraint or index. See previous errors.'

This cropped up in what feels like a stupid-simple, convention-based model.

To be clear, I do NOT want to use foreign keys in my database. I actually don't understand why they are even truly required to enable the navigations feature.

I feel if the EFCore team had the same convinctions some of us have against FKs we could very quickly find a happy path to make this work.

We are going to start hedging back towards common queries in an extension method w/ Dapper. I can tell that a "no FK" branch of EFCore is going take longer than we are willing to wait.

wdhenrik commented 5 months ago

Please implement this with an option to create the FK index but without the constraint. I don't want the DB to enforce the relationship, but I still want the performance benefits of the index.

My application contains supporting data brought in from external integrations. That supporting data is fully modeled in our application so it can be queried using EF. However, data in the external systems has reached retention age and the old data must be purged from our system. The constraints make that process very inefficient.

Currently, with the FKs constrained:

  1. Add a supporting column to PK table, e.g. bool SourceDeleted. An index on SourceDeleted column is necessary for 3 and 4 below, but has no value otherwise.
  2. When a record is purged from the external system, the supporting data must be updated to set SourceDeleted = true (ETL).
  3. Walk the FK tables and delete records dependent on the data Where SourceDeleted = true.
  4. Once all FK tables are updated, delete data from the PK table Where SourceDeleted = true.

If the FK could generate the related index without the constraint, this process is simpler:

  1. Delete data PK records when purged from external system (ETL)
  2. Walk the FK tables and delete dependent records where FK Not Exist in PK table Since the related FK columns are still indexed, performance should be very good.

I would like to see a configuration syntax of something like this:

builder.HasOne(x => x.Position)
    .WithMany(y => y.AssignedPersons)
    .HasForeignKey(x => x.PositionCode)
    .DoNotEnforce();

That would not create the FK constraint in the database, but will tell EF to use the index for navigation work and would create an index similar to this:

builder.HasIndex(x => x.PositionCode);
TsengSR commented 4 months ago

Gotta join the party too. I have a similar use case where I need to setup a navigation property w/o foreign key constrains.

The particular use case is, that we sync some data via message bus across services when ever they change in the authoriatve service, for performance reason (its hard to make an efficient join with an data that only exists in a different microservice which has its own database).

The issue is, some entities synchronized that way have references to other objects, that can be changed on their own (and hence need a separate event triggered, because EF core do not flag the parent entity as changed), so we have subscribes for that type of entity too

public class ParentEntity 
{
    public int Id { get; set;}
    public string Name { get; set;}
    public ReferencedEntity Reference { get; set;}
}

public class ReferencedEntity
{
    public int Id { get; set;}
    public string Name { get; set;}
    public int? ParentId { get; set;}
    public ParentEntity Parent { get; set;}
}

Now due to nature of message buses, sending messages and re-queuing when an error happens, the messages can arrive out of order, i.e. the ReferencedEntity can be inserted prior to its ParentEntity. If the FK is present on ParentId property, it will fail, since the parent isn't inserted yet.

So we'd like to not create the foreign key in the first place (but still have the relationship configured, so include can be used), then ReferencedEntity will be inserted first, with a "valid" ParentId (for which the parent just doesn't exist yet) and will be a full relationship once the ParentEntity is processed. A simple index on it would be sufficient (for join performance), but no foreign key with validation etc.

Alternatively, I'd be happy if there'd be an easy mechanism to notify when any referenced entity of a specific parent entity (aggregate root) changes, i.e. when ReferenceEntity is modified to have some kind of notification on ParentEntity that it or one of its references has been modified.

Then we could of course also change the messaging to only be based on the aggregate/root entity that contains the full object graph, but I have no idea on how to attempt that w/o tons of reflection and additional tracking

@ajcvickers @roji Any directions?

roji commented 4 months ago

@TsengSR it sounds like if your messages really arrive out of order and you just apply the changes, your database really will be in an inconsistent state (at least temporarily); that means that if someone queries the database at that moment, they'll see an incorrect view of things, and EF may actually completely fail (e.g. since there's no principal although there must be one).

I generally really encourage people to think carefully before asking for "no foreign keys"; in many cases, that indicates a problematic design that should be addressed elsewhere. Note that database also sometimes support "deferrable" transactions, where foreign key constraints are only enforced when the transaction is actually commit; that can allow a temporary inconsistent state, but only within the transaction itself (and the inconsistency is therefore invisible outside of it).

I don't know enough about your application to give any advice here - but changing "the messaging to only be based on the aggregate/root entity that contains the full object graph" sounds like it's too much (or missing the main thing). The point is that each change you apply (via your message) should always lead to a consistent state in your data - this is even regardless of whether foreign keys are enforced in your database or not. Disabling foreign keys just pushes the problem down further down to whoever is reading from your database, as they now have to deal with inconsistent states.

TsengSR commented 4 months ago

@TsengSR it sounds like if your messages really arrive out of order and you just apply the changes, your database really will be in an inconsistent state (at least temporarily); that means that if someone queries the database at that moment, they'll see an incorrect view of things, and EF may actually completely fail (e.g. since there's no principal although there must be one).

Isn't an issue for me in this case, since the property will be unreachable anyways until the parent property gets processed/synched. But as far as I see it, there won't even be any exceptions, since the relation is just used to build joins when doing includes and then either the parent or child entity is missing it gives no result or null result, depending on whether or not its an inner, outer or left/right join.

The synched data is assumed as eventual consistent (no different then any other eventually consistent system really), so it is made with the assumption that there may be some time where its not 100% consistent, as long as it gets eventually consistent its fine, since its not authoritative data. The authoritative data (source of truth) is the original service

I generally really encourage people to think carefully before asking for "no foreign keys"; in many cases, that indicates a problematic design that should be addressed elsewhere. Note that database also sometimes support "deferrable" transactions, where foreign key constraints are only enforced when the transaction is actually commit; that can allow a temporary inconsistent state, but only within the transaction itself (and the inconsistency is therefore invisible outside of it).

Yea, but this doesn't work with messaging, where a message is processed one after other. For regular transactions this approach is fine and all of the a services authoritative tables have and use foreign keys and enforce them on database level. Its just in that special case where I need to synchronize some entities (or just part of them with a minimal set of properties) across another service to be able to perform queries.

Assume a service which manages storage (how much of each material is located in which part of the storage hall, which shelve and which location within the shelf) and another service with material (materialnumber, description, weight, dimensions etc.) and now every time a material is added or changed, the accordingly data in the storage service needs to be updated too. Why need the data there? Well, people want to be able to query for a specific material, material type or just when listing whats currently in the storage want to know which material is there and also have the changed in the material service reflected. Since its a lot of data, the only efficient way to include this data into the query to display for the user is by joining it on the database rather than have to fetch each material from the material service. Aside of being awfully slow, it prevents certain things (pagination, sorting or filtering on properties that are not inside the storage service)

I don't know enough about your application to give any advice here - but changing "the messaging to only be based on the aggregate/root entity that contains the full object graph" sounds like it's too much (or missing the main thing).

Well if it would be an object database, it would be easy really, since the material and all of its references would be a single object. The problem is, its not a document database and how EF Core tracks modified entities.

Using the example above, when someone updates ReferenceEntity, there will be no indication on ParentEntity that there has been a change, i.e.

ParentEntity entity = await dbContext.Parents.Include(p => p.Reference).SingleOrDefault(p => p.Id == 1);
entity.Reference.Name = "New Name";
await dBContext.SaveChangesAsync();

In this, only ReferenceEntity will be marked as modified in the change tracker, since well nothing was changed in the parent and I use the change tracker to detect changes and send messages.

That's why for now, I send for every changed entity a message. However, when doing an add (adding new parent and new reference) the reference, two messages are sent and the reference one is more often than not the first one to arrive. And I can't think of any way for a generic solution that would properly and automatically determine the parent entity (or aggregate root more specifically) to do notifications via this

wdhenrik commented 4 months ago

Something else to consider is what is the alternative solution for this issue.

For my system:

  1. Write queries without the benefit of navigations and explicitly declare indexes for performance. or
  2. Engineer a process to sequence external data load in relational order, and in reverse order for deleting

1 must be done for every new relationship and query. That adds development and maintenance overhead, and is easily overlooked. 2 can be very complicated. It requires keeping the EF Core app in sync with the source system relationships (order must be maintained). For deleting data, as in my scenario, it involves adding an additional column to every table so I can queue obsolete data for deletion. (At which point I can either let EF core use change tracker RBAR, or walk the navigation structure to use the bulk delete methods. RBAR has performance overhead, the other requires additional development and maintenance.)

There are number of EF features and functionality that come with warnings about their use. I would put this feature into the same category. A big yellow warning box on the feature documentation mentioning the concerns about data integrity is sufficient. It should only be used when needed, and the implementer should understand the risks.

ajcvickers commented 4 months ago

Note: This issue is about only not creating the constraint in the database. EF will still treat the relationship as constrained. If you want an unconstrained relationship--that is, a relationship where an FK value is non-null but there is no corresponding PK in the database--then please vote for #13146.

statler commented 1 month ago

It has been noted in this thread that the upvotes count is considered when implemented 'quality of life' improvements to .EF, and that these are important to the team. Given that this issue has over 200 upvotes it has to be one of the most in demand requests for the framework on the metric of Popularity / [Required Effort]. At first blush it appears that providing a property/attribute to exclude a key from the migration wouldn't be a major task compared to many low value additions that do make the cut for new versions. Can we please just get it done? The community has been giving clear feedback over 5 years as it has been asked to do, but it doesn't appear that anyone is really reviewing the guidance users are providing as to important issues for us through the only platform we really have - even though we try to do this [generally] with respect and in accordance with the directions we are given.