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

Revisit navigation naming convention when reverse-engineering composite keys #16711

Open
Tracked by #22948
cgountanis opened this issue Jul 23, 2019 · 15 comments
Open
Tracked by #22948

Comments

@cgountanis
Copy link

cgountanis commented Jul 23, 2019

I have a database first solution core 2.2.4. Scaffolding does work and it creates a navigation property. The issue I'm having is how the scaffolding names that property. It causes parent table navigation when really it should call it the child table navigation. What if for some reason there was two composite keys. It wouldn't be able to use the same name.

Is there any way to customize the name does it use something from the foreign key description or anything where it would name the navigation property that it automatically creates a little more smartly?

@cgountanis
Copy link
Author

cgountanis commented Jul 24, 2019

To add more information, I feel this is a bug due to the way db-first scaffold names the property [ParentTableName]Navigation when it should name it [ChildTable]Navigation. If you have a composite FK from parent to child on say 2 fields why would it name the "Navigation" property prefixed with the parent table and not the object it is navigating to?

In this example Entity is the parent and the EntitySubType is the child with the composite FK, mainly for data integrity reasons, although I was not sold on this concept, came from the DBA that way.

image

@cgountanis
Copy link
Author

There has to be a way it can use the child name + "Navigation", you know something that would make more sense from the API side when using the models logically. Right now it would seem every time I scaffold I have to rename a bunch of properties.

@ajcvickers
Copy link
Member

@cgountanis First, if you are using EF Core 2.0, then you should upgrade. That release has been out-of-support for some time now.

Following that, if you are still seeing issues, then please post the tables and the generated classes so we can fully investigate.

@cgountanis
Copy link
Author

cgountanis commented Jul 25, 2019

Sorry I should have been more clear. Easy to reproduce, make a parent table and a child table with composite key (2 columns PK on parent > child as FK), then do database-first scaffold.

Scaffold-DbContext "Server=localhost;Database=DBNAME;Trusted_Connection=True;" Microsoft.EntityFrameworkCore.SqlServer -OutputDir Models -Context Context -ContextDir Models -Force

The models when it creates the virtual property, the property name is backwards.

In Parent model: Named ParentTableNameNavigation when it should be ChildTableNameNavigation, to make any sense. Plus, if you ever had 2 children with comp from same parent, my guess is it would really get funky. Have not tested that yet.

image

@cgountanis cgountanis changed the title Database First Composite Keys Naming Database First Composite Keys Naming (Navigation) Jul 25, 2019
@cgountanis cgountanis changed the title Database First Composite Keys Naming (Navigation) Database First Composite Keys Property Naming (Navigation) Jul 25, 2019
@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 25, 2019

Please post the CREATE TABLE statements and the generated model code (DbContext and POCO classes)

@cgountanis
Copy link
Author

cgountanis commented Jul 25, 2019

using System;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Metadata;

namespace CaseManagerData.TestModels
{
    public partial class TestContext : DbContext
    {
        public TestContext()
        {
        }

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

        public virtual DbSet<Book> Book { get; set; }
        public virtual DbSet<BookType> BookType { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            if (!optionsBuilder.IsConfigured)
            {
#warning To protect potentially sensitive information in your connection string, you should move it out of source code. See http://go.microsoft.com/fwlink/?LinkId=723263 for guidance on storing connection strings.
                optionsBuilder.UseSqlServer("Server=localhost;Database=Test;Trusted_Connection=True;");
            }
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.HasAnnotation("ProductVersion", "2.2.6-servicing-10079");

            modelBuilder.Entity<Book>(entity =>
            {
                entity.Property(e => e.BookId).ValueGeneratedNever();

                entity.Property(e => e.Name).HasMaxLength(50);

                entity.HasOne(d => d.BookNavigation)
                    .WithMany(p => p.Book)
                    .HasForeignKey(d => new { d.BookTypeId, d.BookSubTypeId })
                    .OnDelete(DeleteBehavior.ClientSetNull)
                    .HasConstraintName("FK_Book_BookType");
            });

            modelBuilder.Entity<BookType>(entity =>
            {
                entity.HasKey(e => new { e.BookTypeId, e.BookSubTypeId });

                entity.Property(e => e.Name).HasMaxLength(50);
            });
        }
    }
}
using System;
using System.Collections.Generic;

namespace CaseManagerData.TestModels
{
    public partial class Book
    {
        public int BookId { get; set; }
        public string Name { get; set; }
        public int BookTypeId { get; set; }
        public int BookSubTypeId { get; set; }

        public virtual BookType BookNavigation { get; set; }
    }
}
using System;
using System.Collections.Generic;

namespace CaseManagerData.TestModels
{
    public partial class BookType
    {
        public BookType()
        {
            Book = new HashSet<Book>();
        }

        public int BookTypeId { get; set; }
        public int BookSubTypeId { get; set; }
        public string Name { get; set; }

        public virtual ICollection<Book> Book { get; set; }
    }
}
USE [Test]
GO

/****** Object:  Table [dbo].[Book]    Script Date: 7/25/2019 1:33:23 PM ******/
SET ANSI_NULLS ON
GO

SET QUOTED_IDENTIFIER ON
GO

CREATE TABLE [dbo].[Book](
	[BookId] [int] NOT NULL,
	[Name] [nvarchar](50) NULL,
	[BookTypeId] [int] NOT NULL,
	[BookSubTypeId] [int] NOT NULL,
 CONSTRAINT [PK_Book] PRIMARY KEY CLUSTERED 
(
	[BookId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY]
GO

ALTER TABLE [dbo].[Book]  WITH CHECK ADD  CONSTRAINT [FK_Book_BookType] FOREIGN KEY([BookTypeId], [BookSubTypeId])
REFERENCES [dbo].[BookType] ([BookTypeId], [BookSubTypeId])
GO

ALTER TABLE [dbo].[Book] CHECK CONSTRAINT [FK_Book_BookType]
GO


USE [Test]
GO

/****** Object:  Table [dbo].[BookType]    Script Date: 7/25/2019 1:33:35 PM ******/
SET ANSI_NULLS ON
GO

SET QUOTED_IDENTIFIER ON
GO

CREATE TABLE [dbo].[BookType](
	[BookTypeId] [int] NOT NULL,
	[BookSubTypeId] [int] NOT NULL,
	[Name] [nvarchar](50) NULL,
 CONSTRAINT [PK_BookType] PRIMARY KEY CLUSTERED 
(
	[BookTypeId] ASC,
	[BookSubTypeId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY]
GO

Notice the ParentTable "Book" virtual property named as BookNavigation even though it is of type BookType. The reason for this FK is to make sure the parent table (in this example Book) can not set TypeId/SubTypeId without it being in the BookType child table for obvious reasons.

image

@cgountanis
Copy link
Author

I made sure I was 100% up-to-date on the latest stable version just to make sure I did my part here.

Microsoft Visual Studio Professional 2019 Version 16.2.0
.NET Core SDK 2.2.401
.NET Core Runtime 2.2.6
ASP.NET Core Runtime 2.2.6

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netcoreapp2.2</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="2.2.6" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="2.2.6" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Tools" Version="2.2.6">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
    </PackageReference>
  </ItemGroup>
  
</Project>

Same results, though. Sorry I thought I was on the latest and greatest (stable).

image

@smitpatel
Copy link
Contributor

For the case of composite key, the dependent to principal navigation name is generated using common prefix of foreign key properties. It is common practice to write the code that way. In your case it turns out to be Book. Since the class itself is named book, it added BookNavigation.

@cgountanis
Copy link
Author

cgountanis commented Jul 25, 2019

The FK is from Book > BookType, look at the virtual BookType BookNavigation. The property should be named "what it is, no?" BookTypeNavigation. That would make more sense from the front-end auto-complete API side of things. Now what if you had two composite keys? You could not name them both BookNavigation. Sorry, respectfully, but that answer does not compute.

@smitpatel
Copy link
Contributor

I believe an example would clarify.

public class Blog
{
    public int Id1 { get; set; }
    public int Id2 { get; set; }
}

public class Post
{
    public int Id { get; set; }
    public int BlogId1 { get; set; }
    public int BlogId2 { get; set; }
    public Blog Blog { get; set; }
}

Above is the general pattern we recognize. Of course people can write their code according to their preference. We follow heuristic to arrive at the name. You are free to change it to whatever you prefer.

@cgountanis
Copy link
Author

To add, we have solved this by NOT using FK and just using CHECK CONTRAINTS within the DBMS so EF can stay happy. I posted this in an effort to shed some light on Composite Keys property naming conventions as it would seem from my examples you would not name the property the parent but the "TYPE" it is representing.

@ajcvickers
Copy link
Member

Triage: we will move this to the backlog to consider changing the default pattern.

/cc @ErikEJ in case he has thoughts.

@ajcvickers ajcvickers added this to the Backlog milestone Jul 26, 2019
@ajcvickers ajcvickers changed the title Database First Composite Keys Property Naming (Navigation) Revisit navigation naming convention when reverse-engineering composite keys Jul 26, 2019
@cgountanis
Copy link
Author

I would appreciate it I think other people with composite keys will run into something like this at some point database first.

@jzabroski
Copy link

The point should not be to "change the default pattern".

The point should be to enable users to control the pattern.

I gave up on using EFCore and have been pounding the table at work for us to move to either EF6 or NHibernate. For any reasonably complicated database architecture, the pattern-based approach tightly couples you to a specific version of EFCore. It is not even guaranteed the patterns wont break from release to release, making you stuck without an upgrade path and piles of EFCore linq queries to port to fix the issue.

@cgountanis
Copy link
Author

cgountanis commented Aug 1, 2019

@jzabroski

That is a shame. We have been using EF Core for simple to complex designs and it overall has been working well. Very fast and it really simplifies the business data layer for us personally. As long as your DB design is solid, be it relational or complex relational (transaction design), it DOES work. Maybe revisit it in a few months after a couple 3.0 releases. Sounds like the 3.0 release will really help, including the SqlFrom for people that must use SPROCS, etc... Would be nice if it supported views but I would much rather use the relational tracking object model approach honestly. (KISS)

Let's be honest database-first was never a priority and I am sure it will come around. Composite keys mentioned my post are replaced with CHECK CONSTRAINTS because it was not really even needed for the parent > child connections, more of a constraint issue anyway for good data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants