Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Number of existing media items is counted wrong in NuCacheContentRepository which leads to media not being cached properly #15205

Closed
karl-sjogren opened this issue Nov 14, 2023 · 3 comments

Comments

@karl-sjogren
Copy link

karl-sjogren commented Nov 14, 2023

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

12.3.2

Bug summary

This is a follow up issue to #15195 where not all media was populated into the cache and thus not available on the site. A fix for #15195 was made in release 12.3.2 but it didn't help.

The work around from the previous issue still works, ie disabling the new feature that uses a paged query when populating the cache.

  "Umbraco": {
    "CMS": {
      "NuCache": {
        "UsePagedSqlQuery" : false
      },

Specifics

As the initial reporter of #15195 I took some time to debug this again on 12.3.2 and the "Count" query just doesn't work for media items it seems. In my tests it always returns 0 items, but since the check is made after loading the first page at least some stuff still loaded.

I'm still something of a beginner with the low level Umbraco (and NPoco) stuff but the count query is waaay different then the actual select in this case.

This is the generated query to select all media items:

SELECT 
  [umbracoNode].[id] AS [Id], 
  [umbracoNode].[uniqueId] AS [Key], 
  [umbracoNode].[level] AS [Level], 
  [umbracoNode].[path] AS [Path], 
  [umbracoNode].[sortOrder] AS [SortOrder], 
  [umbracoNode].[parentId] AS [ParentId], 
  [umbracoNode].[createDate] AS [CreateDate], 
  [umbracoNode].[nodeUser] AS [CreatorId], 
  [umbracoContent].[contentTypeId] AS [ContentTypeId], 
  [umbracoContentVersion].[id] AS [VersionId], 
  [umbracoContentVersion].[text] AS [EditName], 
  [umbracoContentVersion].[versionDate] AS [EditVersionDate], 
  [umbracoContentVersion].[userId] AS [EditWriterId], 
  [nuEdit].[data] AS [EditData], 
  [nuEdit].[dataRaw] AS [EditDataRaw] 
FROM 
  [umbracoNode] 
  INNER JOIN [umbracoContent] ON (
    [umbracoNode].[id] = [umbracoContent].[nodeId]
  ) 
  INNER JOIN [umbracoContentVersion] ON (
    (
      [umbracoNode].[id] = [umbracoContentVersion].[nodeId]
    ) 
    AND [umbracoContentVersion].[current] = 1
  ) 
  LEFT JOIN [cmsContentNu] [nuEdit] ON (
    (
      [umbracoNode].[id] = [nuEdit].[nodeId]
    ) 
    AND [nuEdit].[published] = 0
  ) 
WHERE 
  (
    (
      (
        [umbracoNode].[nodeObjectType] = 'b796f64c-1f99-4ffb-b886-4bf4bc011a9c'
      ) 
      AND ([umbracoNode].[trashed] = 0)
    )
  ) 
ORDER BY 
  [umbracoNode].[level], 
  [umbracoNode].[parentId], 
  [umbracoNode].[sortOrder]

And this is the (supposedly optimized) query to count the items which returns 0.

SELECT 
  COUNT(*) 
FROM 
  (
    SELECT 
      [umbracoNode].[id] AS [Id] 
    FROM 
      [umbracoNode] 
      INNER JOIN [umbracoContent] ON (
        [umbracoNode].[id] = [umbracoContent].[nodeId]
      ) 
      INNER JOIN [umbracoDocument] ON (
        [umbracoNode].[id] = [umbracoDocument].[nodeId]
      ) 
      INNER JOIN [umbracoContentVersion] ON (
        (
          [umbracoNode].[id] = [umbracoContentVersion].[nodeId]
        ) 
        AND [umbracoContentVersion].[current] = 1
      ) 
      INNER JOIN [umbracoDocumentVersion] ON (
        [umbracoContentVersion].[id] = [umbracoDocumentVersion].[id]
      ) 
      LEFT JOIN [umbracoContentVersion] [pcver] 
      INNER JOIN [umbracoDocumentVersion] [pdver] ON (
        ([pcver].[id] = [pdver].[id]) 
        AND [pdver].[published] = 0
      ) ON (
        [umbracoNode].[id] = [pcver].[nodeId]
      ) 
    WHERE 
      (
        (
          (
            [umbracoNode].[nodeObjectType] = 'b796f64c-1f99-4ffb-b886-4bf4bc011a9c'
          ) 
          AND ([umbracoNode].[trashed] = 0)
        )
      )
  ) npoco_tbl

If we were to skip this optimized count query for Media the result is correct. Something like this.

    private IEnumerable<ContentSourceDto> GetContentNodeDtos(Sql<ISqlContext> sql, Guid nodeObjectType)
    {
        // We need to page here. We don't want to iterate over every single row in one connection cuz this can cause an SQL Timeout.
        // We also want to read with a db reader and not load everything into memory, QueryPaged lets us do that.
        // QueryPaged is very slow on large sites however, so use fetch if UsePagedSqlQuery is disabled.
        IEnumerable<ContentSourceDto> dtos;
        if (_nucacheSettings.Value.UsePagedSqlQuery)
        {
            Sql<ISqlContext>? sqlCount;

            if(nodeObjectType == Constants.ObjectTypes.Media) {
                // Use the default COUNT query
                sqlCount = null;
            } else {
                // Use a more efficient COUNT query
                Sql<ISqlContext>? sqlCountQuery = SqlContentSourcesCount()
                    .Append(SqlObjectTypeNotTrashed(SqlContext, nodeObjectType));

                 sqlCount = SqlContext.Sql("SELECT COUNT(*) FROM (").Append(sqlCountQuery).Append(") npoco_tbl");
            }

            dtos = Database.QueryPaged<ContentSourceDto>(_nucacheSettings.Value.SqlPageSize, sql, sqlCount);
        }
        else
        {
            dtos = Database.Fetch<ContentSourceDto>(sql);
        }

        return dtos;
    }

I have no idea if there are other types that needs to be handled here as well but this solved my images not loading. Since it is only for the count query I can't see that this affects the performance to badly, at least for short term fix.

Steps to reproduce

Add more then 1000 media items and less then 1000 documents, only the first 1000 media items will be loaded into the cache.

Expected result / actual result

All media items should be loaded into the cache no matter how many documents there are.

Copy link

Hi there @karl-sjogren!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

  • We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
  • If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
  • We'll replicate the issue to ensure that the problem is as described.
  • We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@bergmania
Copy link
Member

Hi @karl-sjogren

Can you confirm this query returns the expected number of media

SELECT COUNT(*) FROM (
                         SELECT "umbracoNode"."id" AS "Id"
                         FROM "umbracoNode"
                                  INNER JOIN "umbracoContent"
                                             ON ("umbracoNode"."id" = "umbracoContent"."nodeId")
                                  INNER JOIN "umbracoContentVersion"
                                             ON (("umbracoNode"."id" = "umbracoContentVersion"."nodeId") AND "umbracoContentVersion"."current" = 1)
                         WHERE ((("umbracoNode"."nodeObjectType" = 'B796F64C-1F99-4FFB-B886-4BF4BC011A9C') AND ("umbracoNode"."trashed" = 0)))
                     ) npoco_tbl

@karl-sjogren
Copy link
Author

Yes that returns the correct number when I run it against my database.

Zeegaan pushed a commit that referenced this issue Nov 15, 2023
* #15205
Count media correct

* Fix content should be document
bergmania added a commit that referenced this issue Dec 7, 2023
* #15205
Count media correct

* Fix content should be document

(cherry picked from commit d2ff2ea)
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants