Skip to content

fix sql syntax in DatabaseCacheRepository #19955

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix sql syntax in DatabaseCacheRepository #19955

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1edf68e
fix sql syntax issues
idseefeld Aug 20, 2025
9608ca1
Merge branch 'main' into v16/19954-fix-DatabaseCacheRepository
idseefeld Aug 21, 2025
9a7f826
fix some Copilote comments
idseefeld Aug 21, 2025
1772d86
fix another Copilot comment removing raw sql strings
idseefeld Aug 21, 2025
32e2e0c
Merge branch 'main' into v16/19954-fix-DatabaseCacheRepository
idseefeld Aug 22, 2025
7fe5933
Merge branch 'main' into v16/19954-fix-DatabaseCacheRepository
idseefeld Aug 22, 2025
74591e2
Merge branch 'main' into v16/19954-fix-DatabaseCacheRepository
idseefeld Aug 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 94 additions & 84 deletions src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Diagnostics;

Check notice on line 1 in src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

✅ Getting better: Code Duplication 

reduced similar code in: SqlOrderByLevelIdSortOrder,SqlWhereNodeKey. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using NPoco;
Expand Down Expand Up @@ -61,7 +61,12 @@

/// <inheritdoc/>
public async Task DeleteContentItemAsync(int id)
=> await Database.ExecuteAsync("DELETE FROM cmsContentNu WHERE nodeId = @id", new { id });
{
Sql<ISqlContext> sql = Sql()
.Delete<ContentNuDto>()
.Where<ContentNuDto>(x => x.NodeId == id);
_ = await Database.ExecuteAsync(sql);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_ = 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.

}

/// <inheritdoc/>
public async Task RefreshContentAsync(ContentCacheNode contentCacheNode, PublishedState publishedState)
Expand All @@ -83,7 +88,10 @@
await OnRepositoryRefreshed(serializer, contentCacheNode, false);
break;
case PublishedState.Unpublishing:
await Database.ExecuteAsync("DELETE FROM cmsContentNu WHERE nodeId = @id AND published = 1", new { id = contentCacheNode.Id });
Sql<ISqlContext> sql = Sql()
.Delete<ContentNuDto>()
.Where<ContentNuDto>(x => x.NodeId == contentCacheNode.Id && x.Published);
_ = await Database.ExecuteAsync(sql);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_ = await Database.ExecuteAsync(sql);
await Database.ExecuteAsync(sql);

break;
}
}
Expand Down Expand Up @@ -137,9 +145,9 @@
public async Task<ContentCacheNode?> GetContentSourceAsync(Guid key, bool preview = false)
{
Sql<ISqlContext>? sql = SqlContentSourcesSelect()
.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));
Comment on lines +148 to +150
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.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.


ContentSourceDto? dto = await Database.FirstOrDefaultAsync<ContentSourceDto>(sql);

Expand Down Expand Up @@ -193,11 +201,11 @@
? SqlMediaSourcesSelect()
: throw new ArgumentOutOfRangeException(nameof(objectType), objectType, null);

sql.InnerJoin<NodeDto>("n")
.On<NodeDto, ContentDto>((n, c) => n.NodeId == c.ContentTypeId, "n", "umbracoContent")
.Append(SqlObjectTypeNotTrashed(SqlContext, objectType))
.WhereIn<NodeDto>(x => x.UniqueId, keys,"n")
.Append(SqlOrderByLevelIdSortOrder(SqlContext));
sql.InnerJoin<NodeDto>("n")
.On<NodeDto, ContentDto>((n, c) => n.NodeId == c.ContentTypeId, "n", "umbracoContent")
.Append(SqlObjectTypeNotTrashed(SqlContext, objectType))
.WhereIn<NodeDto>(x => x.UniqueId, keys, "n")
.Append(SqlOrderByLevelIdSortOrder(SqlContext));

return GetContentNodeDtos(sql);
}
Expand Down Expand Up @@ -277,19 +285,25 @@

private async Task OnRepositoryRefreshed(IContentCacheDataSerializer serializer, ContentCacheNode content, bool preview)
{

ContentNuDto dto = GetDtoFromCacheNode(content, !preview, serializer);

await Database.InsertOrUpdateAsync(
dto,
"SET data = @data, dataRaw = @dataRaw, rv = rv + 1 WHERE nodeId = @id AND published = @published",
new
{
dataRaw = dto.RawData ?? Array.Empty<byte>(),
data = dto.Data,
id = dto.NodeId,
published = dto.Published,
});
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);
}
Comment on lines +290 to +306
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/// <summary>
Expand All @@ -308,14 +322,7 @@
Guid contentObjectType = Constants.ObjectTypes.Document;

// Remove all - if anything fails the transaction will rollback.
if (contentTypeIds.Count == 0)
{
DeleteForObjectType(contentObjectType);
}
else
{
DeleteForObjectTypeAndContentTypes(contentObjectType, contentTypeIds);
}
RemoveByObjectType(contentObjectType, contentTypeIds);

// Insert back - if anything fails the transaction will rollback.
IQuery<IContent> query = GetInsertQuery<IContent>(contentTypeIds);
Expand Down Expand Up @@ -351,12 +358,20 @@
}

/// <summary>
/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice comment added, but if we are including, could we add the similar on RebuildContentDbCache and RebuildMemberDbCache for consistency please?

/// Assumes media tree lock.
/// </remarks>
private void RebuildMediaDbCache(IContentCacheDataSerializer serializer, int groupSize, IReadOnlyCollection<int>? contentTypeIds)
/// </summary>
/// <remarks>This method assumes that the media tree is locked during execution to ensure consistency.
/// The operation is performed in two stages: first, the existing cache entries are removed, and then the cache is
/// repopulated with the latest media data. The process is batched to handle large datasets efficiently.</remarks>
/// <param name="serializer">The serializer used to convert media content into a format suitable for database storage.</param>
/// <param name="groupSize">The number of media items to process in each batch during the cache rebuild operation.</param>
/// <param name="contentTypeIds">A collection of content type IDs to filter the media items being processed. If null or empty, all media items
/// are processed.</param>
private void RebuildMediaDbCache(
IContentCacheDataSerializer serializer,
int groupSize,
IReadOnlyCollection<int>? contentTypeIds)
{
if (contentTypeIds is null)
{
Expand All @@ -366,14 +381,7 @@
Guid mediaObjectType = Constants.ObjectTypes.Media;

// Remove all - if anything fails the transaction will rollback.
if (contentTypeIds.Count == 0)
{
DeleteForObjectType(mediaObjectType);
}
else
{
DeleteForObjectTypeAndContentTypes(mediaObjectType, contentTypeIds);
}
RemoveByObjectType(mediaObjectType, contentTypeIds);

// Insert back - if anything fails the transaction will rollback.
IQuery<IMedia> query = GetInsertQuery<IMedia>(contentTypeIds);
Expand Down Expand Up @@ -405,18 +413,10 @@
{
return;
}

Guid memberObjectType = Constants.ObjectTypes.Member;

// Remove all - if anything fails the transaction will rollback.
if (contentTypeIds.Count == 0)
{
DeleteForObjectType(memberObjectType);
}
else
{
DeleteForObjectTypeAndContentTypes(memberObjectType, contentTypeIds);
}
RemoveByObjectType(memberObjectType, contentTypeIds);

// Insert back - if anything fails the transaction will rollback.
IQuery<IMember> query = GetInsertQuery<IMember>(contentTypeIds);
Expand All @@ -429,32 +429,46 @@
IEnumerable<IMember> descendants =
_memberRepository.GetPage(query, pageIndex++, groupSize, out total, null, Ordering.By("Path"));
ContentNuDto[] items = descendants.Select(m => GetDtoFromContent(m, false, serializer)).ToArray();
Database.BulkInsertRecords(items);
_ = Database.BulkInsertRecords(items);
processed += items.Length;
}
while (processed < total);
}

private void DeleteForObjectType(Guid nodeObjectType) =>
Database.Execute(
@"
DELETE FROM cmsContentNu
WHERE cmsContentNu.nodeId IN (
SELECT id FROM umbracoNode WHERE umbracoNode.nodeObjectType = @objType
)",
new { objType = nodeObjectType });

private void DeleteForObjectTypeAndContentTypes(Guid nodeObjectType, IReadOnlyCollection<int> contentTypeIds) =>
Database.Execute(
$@"
DELETE FROM cmsContentNu
WHERE cmsContentNu.nodeId IN (
SELECT id FROM umbracoNode
JOIN {Constants.DatabaseSchema.Tables.Content} ON {Constants.DatabaseSchema.Tables.Content}.nodeId=umbracoNode.id
WHERE umbracoNode.nodeObjectType = @objType
AND {Constants.DatabaseSchema.Tables.Content}.contentTypeId IN (@ctypes)
)",
new { objType = nodeObjectType, ctypes = contentTypeIds });
private void RemoveByObjectType(Guid objectType, IReadOnlyCollection<int>? contentTypeIds)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// remove all - if anything fails the transaction will rollback
// If the provided contentTypeIds collection is empty, remove all records for the provided object type.

Let's update these comments to better describe what's going on in this method.

Sql<ISqlContext> sql;
if (contentTypeIds is null || contentTypeIds.Count == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (contentTypeIds is null || contentTypeIds.Count == 0)
if (contentTypeIds.Count == 0)

{
sql = Sql()
.Delete<ContentNuDto>()
.WhereIn<ContentNuDto>(
x => x.NodeId,
Sql().Select<NodeDto>(n => n.NodeId)
.From<NodeDto>()
.Where<NodeDto>(n => n.NodeObjectType == objectType));
_ = Database.Execute(sql);
Comment on lines +450 to +451
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
else
{
// assume number of ctypes won't blow IN(...)
// must support SQL-CE
Comment on lines +455 to +456
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// assume number of ctypes won't blow IN(...)
// must support SQL-CE
// Otherwise, if contentTypeIds are provided remove only those records that match the object type and one of the content types.

sql = Sql()
.Delete<ContentNuDto>()
.WhereIn<ContentNuDto>(
x => x.NodeId,
Sql()
.Select<NodeDto>(n => n.NodeId)
.From<NodeDto>()
.InnerJoin<ContentDto>()
.On<NodeDto, ContentDto>((n, c) => n.NodeId == c.NodeId)
.Where<NodeDto, ContentDto>((n, c) =>
n.NodeObjectType == objectType
&& contentTypeIds.Contains(c.ContentTypeId)));
Comment on lines +467 to +468
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

_ = Database.Execute(sql);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_ = Database.Execute(sql);
Database.Execute(sql);

}
}

private IQuery<TContent> GetInsertQuery<TContent>(IReadOnlyCollection<int> contentTypeIds)
where TContent : IContentBase
Expand Down Expand Up @@ -484,7 +498,10 @@

var dto = new ContentNuDto
{
NodeId = cacheNode.Id, Published = published, Data = serialized.StringData, RawData = serialized.ByteData,
NodeId = cacheNode.Id,
Published = published,
Data = serialized.StringData,
RawData = serialized.ByteData,
};

return dto;
Expand Down Expand Up @@ -560,7 +577,10 @@

var dto = new ContentNuDto
{
NodeId = content.Id, Published = published, Data = serialized.StringData, RawData = serialized.ByteData,
NodeId = content.Id,
Published = published,
Data = serialized.StringData,
RawData = serialized.ByteData,
};

return dto;
Expand Down Expand Up @@ -633,39 +653,30 @@

private Sql<ISqlContext> SqlWhereNodeKey(ISqlContext sqlContext, Guid key)
{
ISqlSyntaxProvider syntax = sqlContext.SqlSyntax;

SqlTemplate sqlTemplate = sqlContext.Templates.Get(
Constants.SqlTemplates.NuCacheDatabaseDataSource.WhereNodeKey,
builder =>
builder.Where<NodeDto>(x => x.UniqueId == SqlTemplate.Arg<Guid>("key")));

Sql<ISqlContext> sql = sqlTemplate.Sql(key);
return sql;
}

private Sql<ISqlContext> SqlOrderByLevelIdSortOrder(ISqlContext sqlContext)
{
ISqlSyntaxProvider syntax = sqlContext.SqlSyntax;

SqlTemplate sqlTemplate = sqlContext.Templates.Get(
Constants.SqlTemplates.NuCacheDatabaseDataSource.OrderByLevelIdSortOrder, s =>
s.OrderBy<NodeDto>(x => x.Level, x => x.ParentId, x => x.SortOrder));

Sql<ISqlContext> sql = sqlTemplate.Sql();
return sql;
}

private Sql<ISqlContext> SqlObjectTypeNotTrashed(ISqlContext sqlContext, Guid nodeObjectType)
{
ISqlSyntaxProvider syntax = sqlContext.SqlSyntax;

SqlTemplate sqlTemplate = sqlContext.Templates.Get(
Constants.SqlTemplates.NuCacheDatabaseDataSource.ObjectTypeNotTrashedFilter, s =>
s.Where<NodeDto>(x =>
x.NodeObjectType == SqlTemplate.Arg<Guid?>("nodeObjectType") &&
x.Trashed == SqlTemplate.Arg<bool>("trashed")));

Sql<ISqlContext> sql = sqlTemplate.Sql(nodeObjectType, false);
return sql;
}
Expand All @@ -681,7 +692,6 @@
.From<NodeDto>()
.InnerJoin<ContentDto>().On<NodeDto, ContentDto>((left, right) => left.NodeId == right.NodeId)
.InnerJoin<DocumentDto>().On<NodeDto, DocumentDto>((left, right) => left.NodeId == right.NodeId));

Sql<ISqlContext>? sql = sqlTemplate.Sql();

if (joins != null)
Expand Down