-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix sql syntax issues (closes #20453) #21307
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
Fix sql syntax issues (closes #20453) #21307
Conversation
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses SQL syntax issues to support a PostgreSQL provider for Umbraco CMS. The main changes focus on case-sensitive column naming (PostgreSQL requirement via NPoco configuration), adding sequence support for databases that use sequences instead of identity columns, and improving SQL query consistency across different database providers.
Key changes include:
- Standardizing column name references using constants instead of hard-coded strings to ensure consistent casing
- Adding sequence support methods to SQL syntax providers (for PostgreSQL's sequence-based auto-increment)
- Converting hard-coded SQL queries to use typed query builders with proper column name quoting
- Fixing case-insensitive string comparisons in WHERE IN clauses for PostgreSQL compatibility
Reviewed changes
Copilot reviewed 110 out of 110 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ContentTypeRepositorySqlClausesTest.cs | Fixed column name casing in test expectations (Id → id, UniqueId → uniqueId) |
| NPocoSqlExtensionsTests.cs | Updated test to expect LOWER() wrapper for case-insensitive string comparisons |
| DictionaryTranslationMapperTest.cs | Fixed expected column name casing (UniqueId → uniqueId) |
| DatabaseCacheRepository.cs | Replaced InsertOrUpdate with explicit FirstOrDefault/Insert/Update pattern for better compatibility |
| UserIdKeyResolver.cs | Changed ExecuteScalar to FirstOrDefault for consistency |
| UmbracoDatabaseFactory.cs | Made DbProviderFactory and CreateDatabaseInstance protected/virtual for extensibility |
| UmbracoDatabaseExtensions.cs | Changed ExecuteScalar to FirstOrDefault for COUNT queries |
| SqlSyntaxProviderBase.cs | Added GetNullExtension, SupportsSequences, and AlterSequences methods for PostgreSQL support |
| ISqlSyntaxProvider.cs | Added interface definitions for new sequence and null extension methods |
| Multiple Repository files | Replaced hard-coded column names with QuoteColumnName calls and DTO constants |
| Query.cs | Added FixCompareCasing to wrap string comparisons in LOWER() for case-insensitive queries |
| NPocoSqlExtensions.cs | Added LOWER() wrapping for string values in WhereIn clauses |
| Multiple DTO files | Added PrimaryKeyName constants and replaced hard-coded column names |
| DatabaseSchemaCreator.cs | Added sequence support after table creation with identity columns |
| DatabaseDataCreator.cs | Major refactoring to use centralized Insert method with auto-increment detection |
| EntityService.cs | Added default ordering by path for deterministic descendant results |
| Constants-DatabaseSchema.cs | Added common primary key and column name constants |
| Multiple API Controller files | Changed from nameof() to DTO column name constants for ordering |
Comments suppressed due to low confidence (3)
src/Umbraco.Infrastructure/Migrations/Install/DatabaseDataCreator.cs:2362
- 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:2367
- 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:2373
- Condition is always not null because of access to local variable alwaysInsert.
if (!alwaysInsert && installDefaultDataSettings?.InstallData == InstallDefaultDataOption.ExceptValues &&
Prerequisites
This PR is required for a PostgreSQL provider. The main issue is NPocos case sensitive configuration for PostgreSQL (It is not a PostgreSQL issue!)
Some other fixes address slightly different SQL statements or behaviour of PostgreSQL.
For this PR is mostly important, that all Unit and Integration test are successful.
For a complete check of these changes you could checkout https://github.com/idseefeld/Umbraco-CMS/tree/v173/postgreSqlProvider which is a complete PostgreSQL provider implementation with additional Unit tests and updated Integration tests (see:
\tests\Umbraco.Tests.Integration\appsettings.Tests.json)This PR solves issue #20453