fix sql syntax in DatabaseCacheRepository #19955
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.
Pull Request Overview
This PR fixes SQL syntax issues in the DatabaseCacheRepository.cs file by replacing raw SQL strings with strongly-typed SQL builder methods and improving SQL syntax compatibility across different database providers.
Key Changes
- Replaced raw SQL DELETE statements with strongly-typed SQL builder pattern
- Updated column name quoting to use proper SQL syntax provider methods
- Consolidated duplicate deletion logic into a single reusable method
src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs
Show resolved
Hide resolved
idseefeld
changed the title
idseefeld
changed the title
There was a problem hiding this comment.
Thanks for this @idseefeld. I'm mostly happy with the clean-up apart from a few nit-picky comments. The main concern I have is the complexity introduced by by-passing the InsertOrUpdateAsync method, so please see my inline comment on that.
| Sql<ISqlContext> sql = Sql() | ||
| .Delete<ContentNuDto>() | ||
| .Where<ContentNuDto>(x => x.NodeId == id); | ||
| _ = await Database.ExecuteAsync(sql); |
There was a problem hiding this comment.
| _ = await Database.ExecuteAsync(sql); | |
| await Database.ExecuteAsync(sql); |
Minor nit-pick just for consistency: we tend not use the discard for these types of method calls where don't do anything with the returned value.
| Sql<ISqlContext> sql = Sql() | ||
| .Delete<ContentNuDto>() | ||
| .Where<ContentNuDto>(x => x.NodeId == contentCacheNode.Id && x.Published); | ||
| _ = await Database.ExecuteAsync(sql); |
There was a problem hiding this comment.
| _ = await Database.ExecuteAsync(sql); | |
| await Database.ExecuteAsync(sql); |
| .Append(SqlObjectTypeNotTrashed(SqlContext, Constants.ObjectTypes.Document)) | ||
| .Append(SqlWhereNodeKey(SqlContext, key)) | ||
| .Append(SqlOrderByLevelIdSortOrder(SqlContext)); |
There was a problem hiding this comment.
| .Append(SqlObjectTypeNotTrashed(SqlContext, Constants.ObjectTypes.Document)) | |
| .Append(SqlWhereNodeKey(SqlContext, key)) | |
| .Append(SqlOrderByLevelIdSortOrder(SqlContext)); | |
| .Append(SqlObjectTypeNotTrashed(SqlContext, Constants.ObjectTypes.Document)) | |
| .Append(SqlWhereNodeKey(SqlContext, key)) | |
| .Append(SqlOrderByLevelIdSortOrder(SqlContext)); |
Would prefer to keep the indentation here.
| AND {Constants.DatabaseSchema.Tables.Content}.contentTypeId IN (@ctypes) | ||
| )", | ||
| new { objType = nodeObjectType, ctypes = contentTypeIds }); | ||
| private void RemoveByObjectType(Guid objectType, IReadOnlyCollection<int>? contentTypeIds) |
There was a problem hiding this comment.
| private void RemoveByObjectType(Guid objectType, IReadOnlyCollection<int>? contentTypeIds) | |
| private void RemoveByObjectType(Guid objectType, IReadOnlyCollection<int> contentTypeIds) |
We have early returns for null, so can update this parameter.
| { | ||
| // remove all - if anything fails the transaction will rollback | ||
| Sql<ISqlContext> sql; | ||
| if (contentTypeIds is null || contentTypeIds.Count == 0) |
There was a problem hiding this comment.
| if (contentTypeIds is null || contentTypeIds.Count == 0) | |
| if (contentTypeIds.Count == 0) |
| .Where<NodeDto, ContentDto>((n, c) => | ||
| n.NodeObjectType == objectType | ||
| && contentTypeIds.Contains(c.ContentTypeId))); | ||
| _ = Database.Execute(sql); |
There was a problem hiding this comment.
| _ = Database.Execute(sql); | |
| Database.Execute(sql); |
| .Where<NodeDto>(n => n.NodeObjectType == objectType)); | ||
| _ = Database.Execute(sql); |
There was a problem hiding this comment.
Would it be better to define a parameter in Where and pass that parameter as the second argument to Execute? Maybe it doesn't really matter, so just flagging for you to consider. If you do update, there are further instances of this in the proposed changes.
| n.NodeObjectType == objectType | ||
| && contentTypeIds.Contains(c.ContentTypeId))); |
There was a problem hiding this comment.
| n.NodeObjectType == objectType | |
| && contentTypeIds.Contains(c.ContentTypeId))); | |
| n.NodeObjectType == objectType && | |
| contentTypeIds.Contains(c.ContentTypeId))); |
Nit-pick: I just more typically see code with && and || laid out this way.
| /// Rebuilds the content database cache for media. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Rebuilds the media database cache by clearing and repopulating the cache with the latest media data. |
There was a problem hiding this comment.
This is a nice comment added, but if we are including, could we add the similar on RebuildContentDbCache and RebuildMemberDbCache for consistency please?
| dto.RawData ??= []; | ||
| dto.Rv++; | ||
| ContentNuDto? existing = await Database.FirstOrDefaultAsync<ContentNuDto>( | ||
| Sql() | ||
| .SelectAll() | ||
| .From<ContentNuDto>() | ||
| .Where<ContentNuDto>(x => x.NodeId == dto.NodeId && x.Published == dto.Published)); | ||
| if (existing is null) | ||
| { | ||
| _ = await Database.InsertAsync(dto); | ||
| } | ||
| else | ||
| { | ||
| Sql<ISqlContext> updateSql = Sql().Update<ContentNuDto>(u => u.Set(d => d.Data, dto.Data).Set(rd => rd.RawData, dto.RawData).Set(v => v.Rv, dto.Rv)) | ||
| .Where<ContentNuDto>(x => x.NodeId == dto.NodeId && x.Published == dto.Published); | ||
| _ = await Database.ExecuteAsync(updateSql); | ||
| } |
There was a problem hiding this comment.
I'm not really too keen on this update, as it by-passes the helper we have for InsertOrUpdateAsync.
Firstly, why is is necessary to do this and not just use the helper? I assume you are looking to replace the SQL statement for the SET, but this looks to be standard SQL - is it a problem for particular database providers?
If it is necessary to change, could you look to amend the InsertOrUpdateAsync helper itself? That way we'd fix all instances of it's usage, and not have to apply the more complicated code you've added here in potentially other places. Perhaps an overload taking a Sql parameter instead of a string for the SET statement could work.
Prerequisites
All test have to be successful.
Find details in issue #19954