Fix Raw SQL Issues and Refactor DatabaseDataCreator (closes #21607)#21590
Fix Raw SQL Issues and Refactor DatabaseDataCreator (closes #21607)#21590idseefeld wants to merge 30 commits intoumbraco:mainfrom
Conversation
refactor new extensions into another file
…xProvider-methods
…xProvider-methods
…ethods' of https://github.com/idseefeld/Umbraco-CMS into v173/21451-quote-raw-sql-names-with-SqlSyntaxProvider-methods
…n UmbracoDatabaseFactory
|
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 PR refactors database seeding and database factory behavior to reduce raw NPoco insert boilerplate, align table-name usage with DTOs, and address PostgreSQL-specific issues around auto-increment/identity columns.
Changes:
- Makes
UmbracoDatabaseFactory.DbProviderFactoryprotected andCreateDatabaseInstance()protected virtual to allow extension and test customization of database creation. - Refactors
DatabaseDataCreatorto use DTOTableNameconstants, centralizes insert logic via a new genericInsert<T>helper usingPocoData, and simplifiesConditionalInsert<TDto>to depend only on configuration and the DTO instance. - Fixes the content-type-allowed-content-type initialization to use
ContentTypeAllowedContentTypeDto.TableNameand updates the seed methods to rely on DTO-based inserts instead of hard-coded table/PK names, with additional logic to toggle auto-increment behavior based on database type.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Umbraco.Infrastructure/Persistence/UmbracoDatabaseFactory.cs |
Exposes DbProviderFactory as protected and CreateDatabaseInstance as protected virtual so subclasses or tests can customize how UmbracoDatabase instances are created. |
src/Umbraco.Infrastructure/Migrations/Install/DatabaseDataCreator.cs |
Refactors install-time seed data creation to use DTO table constants and a shared Insert<T> based on PocoData, updates ConditionalInsert to no longer require table/PK names, corrects the content child-type table mapping to ContentTypeAllowedContentTypeDto.TableName, and introduces PostgreSQL-aware auto-increment handling for inserts. |
Comments suppressed due to low confidence (3)
src/Umbraco.Infrastructure/Migrations/Install/DatabaseDataCreator.cs:2425
- Condition is always not null because of access to local variable alwaysInsert.
if (!alwaysInsert && installDefaultDataSettings?.InstallData == InstallDefaultDataOption.None)
src/Umbraco.Infrastructure/Migrations/Install/DatabaseDataCreator.cs:2430
- Condition is always not null because of access to local variable alwaysInsert.
if (!alwaysInsert && installDefaultDataSettings?.InstallData == InstallDefaultDataOption.Values &&
src/Umbraco.Infrastructure/Migrations/Install/DatabaseDataCreator.cs:2436
- Condition is always not null because of access to local variable alwaysInsert.
if (!alwaysInsert && installDefaultDataSettings?.InstallData == InstallDefaultDataOption.ExceptValues &&
src/Umbraco.Infrastructure/Migrations/Install/DatabaseDataCreator.cs
Outdated
Show resolved
Hide resolved
…hub.com/idseefeld/Umbraco-CMS into v173/21516-DatabaseDataCreator-refactor
…tabaseDataCreator-refactor
…tabaseDataCreator-refactor
Refactoring Hint
This looks like another case of working just for CodeScene, but actually I did this before I was aware of CodeScene.
And I think these would be acceptable for code review 😥
Details to Help in Code Review
There seem to be many changes, but actually there are only 4 principals in many places.
So, what I basically did:
Updated method
private void ConditionalInsert<TDto>(string configKey, string id, TDto dto)by reducing input parameters).Added method
private object Insert<T>(T poco).That reduces constant input parameter e.g.:
Replace constants by table properties e.g.:
Constants.DatabaseSchema.Tables.NodebecomesNodeDto.TableName. I think this makes the code cleaner, more OO 😉_database.Insert(Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Constants.Locks.Servers, Name = "Servers" });becomesInsert(new LockDto { Id = Constants.Locks.Servers, Name = "Servers" });all in all 19 lines changed like this in method
private void CreateLockData().The refactoring decreases the total number of lines from 2758 to 2468 and removes a lot of repetitive code.
And it solves another issue for auto increment columns in PostgreSqlProvider and therefor it is another part of issue #20453.
I hope this summary makes it much easier to review the changes.
This is another PR for issue #20453.