Caching: Fixes regression of the caching of null representations for missing dictionary items (closes #20336 for 13) #20344
Conversation
AndyButland
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a regression in null value caching for the dictionary repository. The issue involved the handling of a special null representation string in the cache that was incorrectly being treated as null in all contexts, breaking backoffice translations when dictionary keys didn't exist.
- Refactored cache item retrieval logic to distinguish between actual null and the special null representation string
- Added comprehensive integration tests to verify caching behavior for both existing and non-existing dictionary items
- Separated null checking from null representation checking in cache extensions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Umbraco.Core/Cache/AppCacheExtensions.cs | Fixed cache retrieval logic to properly handle null representation string vs actual null values |
| tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DictionaryRepositoryTest.cs | Added comprehensive integration tests for dictionary repository caching behavior |
...ests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DictionaryRepositoryTest.cs
Show resolved
Hide resolved
...ests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DictionaryRepositoryTest.cs
Show resolved
Hide resolved
kjac
left a comment
kjac
left a comment
There was a problem hiding this comment.
@AndyButland I have added a bit more to the tests, to verify that cached null values are also flushed as expected.
If you think it makes sense, feel free to merge 👍
|
It does - thanks. Will do (and I'll cherry-pick for the 13.11 release and 16). |
AndyButland
deleted the
v13/bugfix/fix-handling-of-null-representation-in-cache
branch
…missing dictionary items (#20344) * Fixed regression with handling of null representation in cache. * Added integration test to verify behaviour. * Use helper methods. * Amend tests so they verify that cached null values are also removed --------- Co-authored-by: kjac <[email protected]>
|
Port to 16 is in #20349 - I'll merge when the build checks are happy, as it's effectively a cherry-pick. |
AndyButland
changed the title
|
This pull request has been mentioned on Umbraco community forum. There might be relevant details there: https://forum.umbraco.com/t/dictionary-cache-regression/6041/6 |
|
This pull request has been mentioned on Umbraco community forum. There might be relevant details there: https://forum.umbraco.com/t/website-significantly-slower-since-upgrading-from-v13-to-v16/6049/9 |
Prerequisites
Addresses #20336
Description
In an earlier PR, we added an update to the dictionary repository such that it cached null values, and prevented multiple database hits for dictionary keys that did not exist: #15576
We then resolved this issue raised that identified some issues resulting when using localised text in the backoffice (supported in Umbraco 13): #19350
Unfortunately it seems this caused a regression on the original feature.
In this PR I've resolved the regression and tested that the backoffice translations work as expected.
I've also added integration tests to verify the cache behaviour.
Testing
See replication steps described in #20336
With the code in this PR in place, verify that you no longer see the database requests for missing dictionary items.
Release
Once approved and merged for 13.12, this will need cherry-picking up for 16.4.