Management API: Replace nameof() by constants of DTO (closes #21303)#21344
Management API: Replace nameof() by constants of DTO (closes #21303)#21344AndyButland merged 52 commits intoumbraco:mainfrom
Conversation
…v173/20453-fix-more-sql-syntax-issues-squash (copy of main)
…ng issues for case sensitive databses
|
Hi there @idseefeld, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
There was a problem hiding this comment.
Pull request overview
This pull request addresses PostgreSQL case sensitivity issues by replacing hard-coded string column names in NPoco DTO attributes with constants. This is part of addressing issue #20453 regarding SQL syntax issues for case-sensitive databases.
Key Changes
- Introduced a new
Constants.DatabaseSchema.Columnsclass containing common database column name constants (PrimaryKeyNameId,PrimaryKeyNamePk,PrimaryKeyNameKey,NodeIdName,UniqueIdName) - Updated 60+ DTO classes to replace hard-coded strings in NPoco attributes (
[PrimaryKey],[Column],[ForeignKey],[Index],[PrimaryKeyColumn],[Reference]) with constant references - Updated API Management controller classes to use DTO constants instead of
nameof()for ordering specifications - Fixed
TableNamePrefixreference inUmbracoDbContext.csto use the relocated constant
Reviewed changes
Copilot reviewed 76 out of 76 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Constants-DatabaseSchema.cs | Added new Columns nested class with common column name constants; moved TableNamePrefix into Tables class |
| 60+ DTO files | Replaced hard-coded column name strings with constant references in NPoco attributes |
| UmbracoDbContext.cs | Updated TableNamePrefix reference to use relocated constant path |
| FolderTreeControllerBase.cs | Replaced nameof() with NodeDto constants for ordering |
| EntityTreeControllerBase.cs | Replaced nameof() with NodeDto constants for ordering |
| RecycleBinControllerBase.cs | Replaced nameof() with NodeDto constants for ordering |
| MediaTreeControllerBase.cs | Replaced nameof() with NodeDto constants for ordering |
| DocumentBlueprintTreeControllerBase.cs | Replaced nameof() with NodeDto constants for ordering |
| DocumentTreeControllerBase.cs | Replaced nameof() with NodeDto constants for ordering |
| DataTypeTreeControllerBase.cs | Replaced nameof() with NodeDto constants for ordering |
| internal sealed class PropertyTypeReadOnlyDto | ||
| { | ||
| public const string TableName = Constants.DatabaseSchema.Tables.PropertyType; | ||
| public const string PrimaryKeyName = "PropertyTypeId"; // there is an inconsistency in main branch between atributtes |
There was a problem hiding this comment.
Spelling error in comment: "atributtes" should be "attributes"
| internal sealed class PropertyTypeGroupReadOnlyDto | ||
| { | ||
| public const string TableName = Constants.DatabaseSchema.Tables.PropertyTypeGroup; | ||
| public const string PrimaryKeyName = "PropertyTypeGroupId"; // there is an inconsistency in main branch between atributtes |
There was a problem hiding this comment.
Spelling error in comment: "atributtes" should be "attributes"
| // ToDo: Here we have an incosistent column name in DatabaseSchema. It should be Constants.DatabaseSchema.Columns.PrimaryKeyNameId; ("id") | ||
| // For now we leave the databse schema as is to avoid breaking changes. |
There was a problem hiding this comment.
Spelling error in comment: "incosistent" should be "inconsistent"
| // ToDo: Here we have an incosistent column name in DatabaseSchema. It should be Constants.DatabaseSchema.Columns.PrimaryKeyNameId; ("id") | |
| // For now we leave the databse schema as is to avoid breaking changes. | |
| // ToDo: Here we have an inconsistent column name in DatabaseSchema. It should be Constants.DatabaseSchema.Columns.PrimaryKeyNameId; ("id") | |
| // For now we leave the database schema as is to avoid breaking changes. |
| [ForeignKey(typeof(ContentTypeDto), Name = "FK_cmsContentTypeAllowedContentType_cmsContentType", Column = "nodeId")] | ||
| [PrimaryKeyColumn(AutoIncrement = false, Clustered = true, Name = "PK_cmsContentTypeAllowedContentType", OnColumns = "Id, AllowedId")] | ||
| // ToDo: Here we have an incosistent column name in DatabaseSchema. It should be Constants.DatabaseSchema.Columns.PrimaryKeyNameId; ("id") | ||
| // For now we leave the databse schema as is to avoid breaking changes. |
There was a problem hiding this comment.
Spelling error in comment: "databse" should be "database"
| // For now we leave the databse schema as is to avoid breaking changes. | |
| // For now we leave the database schema as is to avoid breaking changes. |
…ContentTypeDto.cs Co-authored-by: Andy Butland <abutland73@gmail.com>
…utes again, because some integration tests for PostgreSQL provider fail without them. Again a case sensitivty issue.
make all column name const consistent
AndyButland
left a comment
There was a problem hiding this comment.
This PR reveals a minor architectural concern, in that the API Management layer controllers directly reference Infrastructure.Persistence.Dtos constants. This couples the presentation layer to the data access layer. However that's not introduced in this PR - it already existed from the nameof property names.
So ideally we would abstract that at the service layer rather than exposing the column names to controllers.
As I say though, that's not made better or worse by this PR, so just noting in passing. Approving and merging, thanks @idseefeld.
Prerequisites
Replace nameof(DTO.COLUMN_NAME) by constant of the DTO, because it leads to casing issues for case sensitive databases like PostgreSQL (NPoco implementation only!).
This is another part of issue #20453 and can only be merged after PR #21327