From 7a5cb7058b798820a598294fe683f1ca5dc7a2fe Mon Sep 17 00:00:00 2001 From: Claus Date: Mon, 14 Aug 2017 11:38:13 +0200 Subject: [PATCH] U4-10289 IdkMap cache can cause the entity service to return invalid results --- src/Umbraco.Core/Services/IdkMap.cs | 53 +++++++++++++------ .../Services/EntityServiceTests.cs | 14 +++-- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/src/Umbraco.Core/Services/IdkMap.cs b/src/Umbraco.Core/Services/IdkMap.cs index 63b203aa0f..af3e4ff50b 100644 --- a/src/Umbraco.Core/Services/IdkMap.cs +++ b/src/Umbraco.Core/Services/IdkMap.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Threading; using Umbraco.Core.Models; using Umbraco.Core.Persistence.UnitOfWork; @@ -10,8 +11,8 @@ namespace Umbraco.Core.Services { private readonly IDatabaseUnitOfWorkProvider _uowProvider; private readonly ReaderWriterLockSlim _locker = new ReaderWriterLockSlim(); - private readonly Dictionary _id2Key = new Dictionary(); - private readonly Dictionary _key2Id = new Dictionary(); + private readonly Dictionary _id2Key = new Dictionary(); + private readonly Dictionary _key2Id = new Dictionary(); public IdkMap(IDatabaseUnitOfWorkProvider uowProvider) { @@ -23,11 +24,12 @@ namespace Umbraco.Core.Services public Attempt GetIdForKey(Guid key, UmbracoObjectTypes umbracoObjectType) { + var compositeKey = new Key2IdCompositeKey() { Key = key, UmbracoObjectType = umbracoObjectType }; int id; try { _locker.EnterReadLock(); - if (_key2Id.TryGetValue(key, out id)) return Attempt.Succeed(id); + if (_key2Id.TryGetValue(compositeKey, out id)) return Attempt.Succeed(id); } finally { @@ -48,9 +50,10 @@ namespace Umbraco.Core.Services try { + var reversedCompositeKey = new Id2KeyCompositeKey { Id = id, UmbracoObjectType = umbracoObjectType }; _locker.EnterWriteLock(); - _id2Key[id] = key; - _key2Id[key] = id; + _id2Key[reversedCompositeKey] = key; + _key2Id[compositeKey] = id; } finally { @@ -73,11 +76,12 @@ namespace Umbraco.Core.Services public Attempt GetKeyForId(int id, UmbracoObjectTypes umbracoObjectType) { + var compositeKey = new Id2KeyCompositeKey() {Id = id, UmbracoObjectType = umbracoObjectType}; Guid key; try { _locker.EnterReadLock(); - if (_id2Key.TryGetValue(id, out key)) return Attempt.Succeed(key); + if (_id2Key.TryGetValue(compositeKey, out key)) return Attempt.Succeed(key); } finally { @@ -98,9 +102,10 @@ namespace Umbraco.Core.Services try { + var reversedCompositeKey = new Key2IdCompositeKey {Key = key, UmbracoObjectType = umbracoObjectType}; _locker.EnterWriteLock(); - _id2Key[id] = key; - _key2Id[key] = id; + _id2Key[compositeKey] = key; + _key2Id[reversedCompositeKey] = id; } finally { @@ -139,10 +144,12 @@ namespace Umbraco.Core.Services try { _locker.EnterWriteLock(); - Guid key; - if (_id2Key.TryGetValue(id, out key) == false) return; - _id2Key.Remove(id); - _key2Id.Remove(key); + var match = _id2Key.Keys.SingleOrDefault(x => x.Id == id); + if (match == null) return; + var key = _id2Key[match]; + var reversedCompositeKey = new Key2IdCompositeKey { Key = key, UmbracoObjectType = match.UmbracoObjectType }; + _id2Key.Remove(match); + _key2Id.Remove(reversedCompositeKey); } finally { @@ -156,10 +163,12 @@ namespace Umbraco.Core.Services try { _locker.EnterWriteLock(); - int id; - if (_key2Id.TryGetValue(key, out id) == false) return; - _id2Key.Remove(id); - _key2Id.Remove(key); + var match = _key2Id.Keys.SingleOrDefault(x => x.Key == key); + if (match == null) return; + var id = _key2Id[match]; + var reversedCompositeKey = new Id2KeyCompositeKey {Id = id, UmbracoObjectType = match.UmbracoObjectType}; + _id2Key.Remove(reversedCompositeKey); + _key2Id.Remove(match); } finally { @@ -167,5 +176,17 @@ namespace Umbraco.Core.Services _locker.ExitWriteLock(); } } + + internal class Id2KeyCompositeKey + { + public int Id { get; set; } + public UmbracoObjectTypes UmbracoObjectType { get; set; } + } + + internal class Key2IdCompositeKey + { + public Guid Key { get; set; } + public UmbracoObjectTypes UmbracoObjectType { get; set; } + } } } \ No newline at end of file diff --git a/src/Umbraco.Tests/Services/EntityServiceTests.cs b/src/Umbraco.Tests/Services/EntityServiceTests.cs index 656915c3f9..818e0099e8 100644 --- a/src/Umbraco.Tests/Services/EntityServiceTests.cs +++ b/src/Umbraco.Tests/Services/EntityServiceTests.cs @@ -547,9 +547,11 @@ namespace Umbraco.Tests.Services public void EntityService_Cannot_Get_Key_For_Id_With_Incorrect_Object_Type() { var service = ServiceContext.EntityService; - var result = service.GetKeyForId(1060, UmbracoObjectTypes.MediaType); - - Assert.IsFalse(result.Success); + var result1 = service.GetKeyForId(1060, UmbracoObjectTypes.DocumentType); + var result2 = service.GetKeyForId(1060, UmbracoObjectTypes.MediaType); + + Assert.IsTrue(result1.Success); + Assert.IsFalse(result2.Success); } [Test] @@ -566,9 +568,11 @@ namespace Umbraco.Tests.Services public void EntityService_Cannot_Get_Id_For_Key_With_Incorrect_Object_Type() { var service = ServiceContext.EntityService; - var result = service.GetIdForKey(Guid.Parse("1D3A8E6E-2EA9-4CC1-B229-1AEE19821522"), UmbracoObjectTypes.MediaType); + var result1 = service.GetIdForKey(Guid.Parse("1D3A8E6E-2EA9-4CC1-B229-1AEE19821522"), UmbracoObjectTypes.DocumentType); + var result2 = service.GetIdForKey(Guid.Parse("1D3A8E6E-2EA9-4CC1-B229-1AEE19821522"), UmbracoObjectTypes.MediaType); - Assert.IsFalse(result.Success); + Assert.IsTrue(result1.Success); + Assert.IsFalse(result2.Success); } private static bool _isSetup = false;