-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Repositories: Quote table, column and alias names (closes #21451) #21577
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
Repositories: Quote table, column and alias names (closes #21451) #21577
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 🤖 🙂 |
refactor new extensions into another file
|
I did some little refactoring by moving private methods in a new static class and making them internal. I moved new methods into a new static class and commented them for review purpose. Hopefully that helps reviewing the changes. |
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 pull request addresses SQL syntax issues for case-sensitive databases like PostgreSQL by quoting table, column, and alias names using SqlSyntaxProvider methods in raw SQL queries. It's part of the ongoing effort to fix issue #20453 and depends on PR #21552.
Changes:
- Adds proper SQL quoting methods to RepositoryBase and refactors numerous repository implementations to use quoted identifiers
- Introduces new extension methods for SQL operations (WhereIn with case-insensitive string comparison, SelectMax with COALESCE)
- Updates test expectations to reflect case-insensitive string comparisons using LOWER()
- Replaces some ExecuteScalar calls with FirstOrDefault to ensure proper mapper pipeline invocation
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Persistence/NPocoTests/NPocoSqlExtensionsTests.cs | Updates test to expect LOWER() wrapping for string WhereIn comparisons |
| tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ExecuteScalarVsFirstOrDefaultTests.cs | Adds new integration tests demonstrating ExecuteScalar vs FirstOrDefault behavior differences |
| src/Umbraco.Infrastructure/Persistence/SqlSyntaxExtensions.cs | Changes GetColumnName visibility from private to internal for reuse |
| src/Umbraco.Infrastructure/Persistence/NPocoSqlWhereExtensions.cs | New file with WhereParam extension method |
| src/Umbraco.Infrastructure/Persistence/NPocoSqlSelectExtensions.cs | New file with SelectMax overload |
| src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensionsInternal.cs | New file with refactored internal helper methods |
| src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs | Refactors by moving code to new files, adds case-insensitive WhereIn for strings, updates AppendSubQuery to quote aliases |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RepositoryBase.cs | Adds QuoteColumnName overload and QuoteName helper methods |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs | Major refactoring with extraction of filtering logic into separate methods, adds proper column quoting and boolean value conversion |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserGroupRepository.cs | Uses DTO constant instead of hardcoded "Key" string |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TrackedReferencesRepository.cs | Removes unnecessary nullable annotation |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TagRepository.cs | Adds null cast suffix, adds OrderBy clauses, fixes column name inconsistencies |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RedirectUrlRepository.cs | Replaces raw SQL with WhereLike extension method |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberTypeRepository.cs | Uses DTO constants for table/column names in delete clauses |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs | Improves query construction with proper quoting, replaces Database.Delete with Sql().Delete() for consistency |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MediaRepository.cs | Adds column name quoting in delete clause |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LastSyncedRepository.cs | Quotes all column and table names in raw SQL statements |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LanguageRepository.cs | Replaces SqlSyntax.GetQuotedName with QuoteName helper and fixes parameter array usage |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/EntityRepository.cs | Uses lowercase alias names, quotes column names in subqueries, changes ExecuteScalar to FirstOrDefault, improves ordering logic |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs | Adds null cast suffix for NULL literals, uses DTO constants for column names in delete clauses |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepository.cs | Uses DTO constants for column names in delete clauses |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentRepositoryBase.cs | Quotes column names in custom ordering SQL |
| src/Umbraco.Infrastructure/Persistence/Repositories/Implement/AuditRepository.cs | Quotes column name in WHERE clause |
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LastSyncedRepository.cs
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TagRepository.cs
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs
Show resolved
Hide resolved
…xProvider-methods
…ethods' of https://github.com/idseefeld/Umbraco-CMS into v173/21451-quote-raw-sql-names-with-SqlSyntaxProvider-methods
|
@AndyButland Copilot found a lot to complain, but it was mostly wrong or against my intention. Especially one suggestion lead to decreased code health in CodeScene test. |
|
I think you would be better to revert the refactoring you have done to try to address CodeScene remarks please @idseefeld. Whilst this feedback can be useful for new code, it's not so important to adhere to it fully when working on existing classes, as they will often have existing issues that this check will complain about. We can and will merge PRs with failing CodeScene reports. It doesn't look to me that the refactoring you have done to try to avoid the concerns raised by this check are necessary, and they make the PR harder to review. In other words, it might well be a good idea to break down So if you could please prepare this so it has only the updates you need to address the issue you are tackling please...
... that'll be easier to review, and more likely we can merge without impacting other developers working on PRs that might overlap with this. Thanks. |
|
Thanks @AndyButland! |
AndyButland
left a comment
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.
I spotted the following issues in review @idseefeld - please can you have a look and consider and/or action? Then I think it's good to merge in.
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LastSyncedRepository.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RedirectUrlRepository.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs
Outdated
Show resolved
Hide resolved
AndyButland
left a comment
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.
All looks good to me now @idseefeld, thanks. I added a few tests just around areas you had refactored, just as an extra check that nothing of significance to behaviour has changed.
Quote table, column and alias names with
SqlSyntaxProvidermethods in raw sql.All test succeed
This is another part to fix #20453.
PR #21552 should be merge before.