Names for the primary key would be generated the same in all cases #1803
Conversation
| @@ -29,23 +32,24 @@ public class DefaultPrimaryKeyNameConvention : IColumnsConvention | |||
| /// <inheritdoc /> | |||
| public IColumnsExpression Apply(IColumnsExpression expression) | |||
| { | |||
| if (expression.Columns.Count(x => x.IsPrimaryKey) > 1) | |||
There was a problem hiding this comment.
I don't understand why this is something that should be discouraged. It is perfectly valid for a table to have a primary key with multiple columns.
There was a problem hiding this comment.
One primary key on many columns is good. But here we are talking specifically about multiple primary keys, each with its own name, am I wrong?
There was a problem hiding this comment.
I am pretty sure you are wrong. But your questioning my statement honestly gives me pause. But I'm 99% sure if I integration test this on a compound key, it will fail.
| foreach (var columnDefinition in expression.Columns) | ||
| { | ||
| if (string.IsNullOrEmpty(columnDefinition.Name)) |
There was a problem hiding this comment.
This should be handled through validation logic. There is a separate layer in the framework for validation of expressions.
There was a problem hiding this comment.
This part of the code is about use column names. Isn't it good to recognize the error as early as possible? To which specific class do you propose to move this check?
There was a problem hiding this comment.
Why separate the two? Same reason you would separate input validation from a Controller Action method in ASP.NET MVC - it keeps the core business logic focused on the actual problem domain and allows for more precise errors / warnings if done as a ValidationResult rather than throwing an exception. If you throw, you lose Metadata that could be used to build even more nicer error messages, too - similar to what Patrik Svensson's SpectreConsole does. In general, throwing exceptions for input validation also prevents "railway oriented programming" where the client gets back a object representing success or failure.
That being said, our Validation layer isn't the best. I realize that's frustrating feedback considering I just told you to use it, but it was pretty good 7 years ago - but third party libraries like FluentValidation have made insane leaps forward since then. We currently pattern the existing one of off System.ComponentModel.DataAnnotations.Validator. I need to delete the areas of the code that still reference CollectErrors, as they have been marked obsolete for awhile.
But:
1.The interface for adding validation is here but doesn't let you explicitly handle each expression root type - you would have to do type tests and dispatch off a Type-switch (probably to something like FluentValidation): https://github.com/fluentmigrator/fluentmigrator/blob/main/src%2FFluentMigrator.Abstractions%2FValidation%2FIMigrationExpressionValidator.cs
2. The default implementation should be replaced here: https://github.com/fluentmigrator/fluentmigrator/blob/main/src%2FFluentMigrator.Abstractions%2FValidation%2FDefaultMigrationExpressionValidator.cs
- The CLI tool supports a Validate command. https://github.com/fluentmigrator/fluentmigrator/blob/main/src%2FFluentMigrator.DotNet.Cli%2FCommands%2FValidate.cs
- Each Model XxxDefinition class and XxxExpression class has a Validate member.
| if (columnDefinition.IsPrimaryKey && string.IsNullOrEmpty(columnDefinition.PrimaryKeyName)) | ||
| { | ||
| var tableName = string.IsNullOrEmpty(columnDefinition.TableName) | ||
| ? expression.TableName | ||
| : columnDefinition.TableName; | ||
| columnDefinition.PrimaryKeyName = GetPrimaryKeyName(tableName); | ||
| columnDefinition.PrimaryKeyName = $"PK_{tableName}_{columnDefinition.Name}"; |
There was a problem hiding this comment.
I think the current behavior is what most users prefer. The actual column name(s) don't really matter to most developers except when needing to change the data type, as that requires dropping and recreating the column name.
Additionally, most DB engines like SQL Server have limit of, say, 255 characters for system object names.
Awhile ago I had the idea of an Opinions assembly that has a different set of default conventions and even some syntax shortcuts. This could live in something like that.
There was a problem hiding this comment.
I think the current behavior is what most users prefer. The actual column name(s) don't really matter to most developers except when needing to change the data type, as that requires dropping and recreating the column name.
If the column name doesn't matter then why shouldn't it be deterministic?
Additionally, most DB engines like SQL Server have limit of, say, 255 characters for system object names.
If the name is too long the user will just get an exception, as in the case of DefaultConstraintNameConvention.
Awhile ago I had the idea of an Opinions assembly that has a different set of default conventions and even some syntax shortcuts. This could live in something like that.
I don't think this is a good enough reason to complicate the project's ecosystem.
There was a problem hiding this comment.
From reading the original issue, it sounds like Create.PrimaryKey() uses DefaultConstraintNameConvention - this is a bug. It should use DefauktPrimaryKeyNameConvention.
Default constraints should just be for default values specified by the ANSI SQL DDL DEFAULT keyword.
Eliminated the difference between classes
DefaultConstraintNameConventionandDefaultPrimaryKeyNameConventionin the default generated primary key name. See issue #1800.