Working on U4-4701 Fixup DataTypeService and repository with cache, pre-values are now persisted properly using units of work with it's own mini internal repo.

src/Umbraco.Core/Models/PreValue.cs

@@ -19,7 +19,7 @@
         /// <summary>
         /// The value stored for the pre-value field
         /// </summary>
-        public string Value { get; private set; }
+        public string Value { get; set; }
 
         /// <summary>
         /// The database id for the pre-value field value

src/Umbraco.Core/Persistence/Repositories/DataTypeDefinitionRepository.cs

@@ -14,6 +14,7 @@ using Umbraco.Core.Persistence.Querying;
 using Umbraco.Core.Persistence.SqlSyntax;
 using Umbraco.Core.Persistence.UnitOfWork;
 using Umbraco.Core.Services;
+using NullCacheProvider = Umbraco.Core.Persistence.Caching.NullCacheProvider;
 
 namespace Umbraco.Core.Persistence.Repositories
 {
@@ -23,17 +24,20 @@ namespace Umbraco.Core.Persistence.Repositories
     internal class DataTypeDefinitionRepository : PetaPocoRepositoryBase<int, IDataTypeDefinition>, IDataTypeDefinitionRepository
     {
         private readonly CacheHelper _cacheHelper;
+        private DataTypePreValueRepository _preValRepository;
 
         public DataTypeDefinitionRepository(IDatabaseUnitOfWork work, CacheHelper cacheHelper)
 			: base(work)
         {
             _cacheHelper = cacheHelper;
+            _preValRepository = new DataTypePreValueRepository(work, NullCacheProvider.Current);
         }
 
         public DataTypeDefinitionRepository(IDatabaseUnitOfWork work, IRepositoryCacheProvider cache, CacheHelper cacheHelper)
             : base(work, cache)
         {
             _cacheHelper = cacheHelper;
+            _preValRepository = new DataTypePreValueRepository(work, NullCacheProvider.Current);
         }
 
         private readonly ReaderWriterLockSlim Locker = new ReaderWriterLockSlim();
@@ -126,6 +130,42 @@ namespace Umbraco.Core.Persistence.Repositories
 
         #region Unit of Work Implementation
 
+        public override void PersistUpdatedItem(IEntity entity)
+        {
+            if (entity is PreValue)
+            {
+                _preValRepository.PersistUpdatedItem(entity);
+            }
+            else
+            {
+                base.PersistUpdatedItem(entity);    
+            }
+        }
+
+        public override void PersistNewItem(IEntity entity)
+        {
+            if (entity is PreValue)
+            {
+                _preValRepository.PersistNewItem(entity);
+            }
+            else
+            {
+                base.PersistNewItem(entity);
+            }
+        }
+
+        public override void PersistDeletedItem(IEntity entity)
+        {
+            if (entity is PreValue)
+            {
+                _preValRepository.PersistDeletedItem(entity);
+            }
+            else
+            {
+                base.PersistDeletedItem(entity);
+            }
+        }
+
         protected override void PersistNewItem(IDataTypeDefinition entity)
         {
             ((DataTypeDefinition)entity).AddingEntity();
@@ -300,6 +340,82 @@ AND umbracoNode.id <> @id",
             }
         }
 
+        public void AddOrUpdatePreValues(int dataTypeId, IDictionary<string, PreValue> values)
+        {
+            var dtd = Get(dataTypeId);
+            if (dtd == null)
+            {
+                throw new InvalidOperationException("No data type found with id " + dataTypeId);
+            }
+            AddOrUpdatePreValues(dtd, values);
+        }
+
+        public void AddOrUpdatePreValues(IDataTypeDefinition dataType, IDictionary<string, PreValue> values)
+        {
+            var currentVals = new DataTypePreValueDto[]{};
+            if (dataType.HasIdentity)
+            {
+                //first just get all pre-values for this data type so we can compare them to see if we need to insert or update or replace
+                var sql = new Sql().Select("*")
+                                   .From<DataTypePreValueDto>()
+                                   .Where<DataTypePreValueDto>(dto => dto.DataTypeNodeId == dataType.Id)
+                                   .OrderBy<DataTypePreValueDto>(dto => dto.SortOrder);
+                currentVals = Database.Fetch<DataTypePreValueDto>(sql).ToArray();    
+            }
+
+            //already existing, need to be updated
+            var valueIds = values.Where(x => x.Value.Id > 0).Select(x => x.Value.Id).ToArray();
+            var existingByIds = currentVals.Where(x => valueIds.Contains(x.Id)).ToArray();
+
+            //These ones need to be removed from the db, they no longer exist in the new values
+            var deleteById = currentVals.Where(x => valueIds.Contains(x.Id) == false);
+
+            foreach (var d in deleteById)
+            {
+                _preValRepository.Delete(new PreValueEntity
+                {
+                    Alias = d.Alias,
+                    Id = d.Id,
+                    Value = d.Value,
+                    DataType = dataType,
+                    SortOrder = d.SortOrder
+                });
+            }
+
+            var sortOrder = 1;
+
+            foreach (var pre in values)
+            {
+                var existing = existingByIds.FirstOrDefault(valueDto => valueDto.Id == pre.Value.Id);
+                if (existing != null)
+                {
+                    existing.Value = pre.Value.Value;
+                    existing.SortOrder = sortOrder;
+                    _preValRepository.AddOrUpdate(new PreValueEntity
+                    {
+                        Alias = existing.Alias,
+                        Id = existing.Id,
+                        SortOrder = existing.SortOrder,
+                        Value = existing.Value,
+                        DataType = dataType,
+                    });
+                }
+                else
+                {
+                    _preValRepository.AddOrUpdate(new PreValueEntity
+                    {
+                        Alias = pre.Key,
+                        SortOrder = sortOrder,
+                        Value = pre.Value.Value,
+                        DataType = dataType,
+                    });
+                }
+
+                sortOrder++;
+            }
+
+        }
+
         private string GetPrefixedCacheKey(int dataTypeId)
         {
             return CacheKeys.DataTypePreValuesCacheKey + dataTypeId + "-";
@@ -327,5 +443,129 @@ AND umbracoNode.id <> @id",
 
             return collection;
         }
+
+        /// <summary>
+        /// Private class to handle pre-value crud based on units of work with transactions
+        /// </summary>
+        public class PreValueEntity : Entity, IAggregateRoot
+        {
+            public string Value { get; set; }
+            public string Alias { get; set; }
+            public IDataTypeDefinition DataType { get; set; }
+            public int SortOrder { get; set; }         
+        }
+
+        /// <summary>
+        /// Private class to handle pre-value crud based on standard principles and units of work with transactions
+        /// </summary>
+        private class DataTypePreValueRepository : PetaPocoRepositoryBase<int, PreValueEntity>
+        {
+            public DataTypePreValueRepository(IDatabaseUnitOfWork work, IRepositoryCacheProvider cache) : base(work, cache)
+            {
+            }
+
+            #region Not implemented (don't need to for the purposes of this repo)
+            protected override PreValueEntity PerformGet(int id)
+            {
+                throw new NotImplementedException();
+            }
+
+            protected override IEnumerable<PreValueEntity> PerformGetAll(params int[] ids)
+            {
+                throw new NotImplementedException();
+            }
+
+            protected override IEnumerable<PreValueEntity> PerformGetByQuery(IQuery<PreValueEntity> query)
+            {
+                throw new NotImplementedException();
+            }
+
+            protected override Sql GetBaseQuery(bool isCount)
+            {
+                throw new NotImplementedException();
+            }
+
+            protected override string GetBaseWhereClause()
+            {
+                throw new NotImplementedException();
+            }
+
+            protected override IEnumerable<string> GetDeleteClauses()
+            {
+                return new List<string>();
+            }
+
+            protected override Guid NodeObjectTypeId
+            {
+                get { throw new NotImplementedException(); }
+            } 
+            #endregion
+
+            protected override void PersistDeletedItem(PreValueEntity entity)
+            {
+                Database.Execute(
+                    "DELETE FROM cmsDataTypePreValues WHERE id=@Id",
+                    new { Id = entity.Id });
+            }
+
+            protected override void PersistNewItem(PreValueEntity entity)
+            {
+                if (entity.DataType.HasIdentity == false)
+                {
+                    throw new InvalidOperationException("Cannot insert a pre value for a data type that has no identity");
+                }
+
+                //Cannot add a duplicate alias
+                var exists = Database.ExecuteScalar<int>(@"SELECT COUNT(*) FROM cmsDataTypePreValues
+WHERE alias = @alias
+AND datatypeNodeId = @dtdid",
+                        new { alias = entity.Alias, dtdid = entity.DataType.Id });
+                if (exists > 0)
+                {
+                    throw new DuplicateNameException("A pre value with the alias " + entity.Alias + " already exists for this data type");
+                }
+
+                var dto = new DataTypePreValueDto
+                {
+                    DataTypeNodeId = entity.DataType.Id,
+                    Value = entity.Value,
+                    SortOrder = entity.SortOrder,
+                    Alias = entity.Alias
+                };
+                Database.Insert(dto);
+            }
+
+            protected override void PersistUpdatedItem(PreValueEntity entity)
+            {
+                if (entity.DataType.HasIdentity == false)
+                {
+                    throw new InvalidOperationException("Cannot update a pre value for a data type that has no identity");
+                }
+
+                //Cannot change to a duplicate alias
+                var exists = Database.ExecuteScalar<int>(@"SELECT COUNT(*) FROM cmsDataTypePreValues
+WHERE alias = @alias
+AND datatypeNodeId = @dtdid
+AND id <> @id",
+                        new { id = entity.Id, alias = entity.Alias, dtdid = entity.DataType.Id });
+                if (exists > 0)
+                {
+                    throw new DuplicateNameException("A pre value with the alias " + entity.Alias + " already exists for this data type");
+                }
+
+                var dto = new DataTypePreValueDto
+                {
+                    DataTypeNodeId = entity.DataType.Id,
+                    Id = entity.Id,
+                    Value = entity.Value,
+                    SortOrder = entity.SortOrder,
+                    Alias = entity.Alias
+                };
+                Database.Update(dto);
+            }
+        }
+    
     }
+
+
 }

src/Umbraco.Core/Persistence/Repositories/Interfaces/IDataTypeDefinitionRepository.cs

@@ -1,4 +1,6 @@
-using Umbraco.Core.Models;
+using System.Collections.Generic;
+using Umbraco.Core.Models;
+using Umbraco.Core.Persistence.UnitOfWork;
 
 namespace Umbraco.Core.Persistence.Repositories
 {
@@ -6,5 +8,8 @@ namespace Umbraco.Core.Persistence.Repositories
     {
         PreValueCollection GetPreValuesCollectionByDataTypeId(int dataTypeId);
         string GetPreValueAsString(int preValueId);
+
+        void AddOrUpdatePreValues(IDataTypeDefinition dataType, IDictionary<string, PreValue> values);
+        void AddOrUpdatePreValues(int dataTypeId, IDictionary<string, PreValue> values);
     }
 }

src/Umbraco.Core/Services/DataTypeService.cs

@@ -153,17 +153,14 @@ namespace Umbraco.Core.Services
             if (Saving.IsRaisedEventCancelled(new SaveEventArgs<IDataTypeDefinition>(dataTypeDefinition), this))
                 return;
 
-            using (new WriteLock(Locker))
+            var uow = _uowProvider.GetUnitOfWork();
+            using (var repository = _repositoryFactory.CreateDataTypeDefinitionRepository(uow))
             {
-                var uow = _uowProvider.GetUnitOfWork();
-                using (var repository = _repositoryFactory.CreateDataTypeDefinitionRepository(uow))
-                {
-                    dataTypeDefinition.CreatorId = userId;
-                    repository.AddOrUpdate(dataTypeDefinition);
-                    uow.Commit();
+                dataTypeDefinition.CreatorId = userId;
+                repository.AddOrUpdate(dataTypeDefinition);
+                uow.Commit();
 
-                    Saved.RaiseEvent(new SaveEventArgs<IDataTypeDefinition>(dataTypeDefinition, false), this);
-                }
+                Saved.RaiseEvent(new SaveEventArgs<IDataTypeDefinition>(dataTypeDefinition, false), this);
             }
 
             Audit.Add(AuditTypes.Save, string.Format("Save DataTypeDefinition performed by user"), userId, dataTypeDefinition.Id);
@@ -179,31 +176,29 @@ namespace Umbraco.Core.Services
             if (Saving.IsRaisedEventCancelled(new SaveEventArgs<IDataTypeDefinition>(dataTypeDefinitions), this))
                 return;
 
-            using (new WriteLock(Locker))
+            var uow = _uowProvider.GetUnitOfWork();
+            using (var repository = _repositoryFactory.CreateDataTypeDefinitionRepository(uow))
             {
-                var uow = _uowProvider.GetUnitOfWork();
-                using (var repository = _repositoryFactory.CreateDataTypeDefinitionRepository(uow))
+                foreach (var dataTypeDefinition in dataTypeDefinitions)
                 {
-                    foreach (var dataTypeDefinition in dataTypeDefinitions)
-                    {
-                        dataTypeDefinition.CreatorId = userId;
-                        repository.AddOrUpdate(dataTypeDefinition);
-                    }
-                    uow.Commit();
-
-                    Saved.RaiseEvent(new SaveEventArgs<IDataTypeDefinition>(dataTypeDefinitions, false), this);
+                    dataTypeDefinition.CreatorId = userId;
+                    repository.AddOrUpdate(dataTypeDefinition);
                 }
+                uow.Commit();
+
+                Saved.RaiseEvent(new SaveEventArgs<IDataTypeDefinition>(dataTypeDefinitions, false), this);
             }
+
             Audit.Add(AuditTypes.Save, string.Format("Save DataTypeDefinition performed by user"), userId, -1);
         }
 
         /// <summary>
         /// Saves a list of PreValues for a given DataTypeDefinition
         /// </summary>
-        /// <param name="id">Id of the DataTypeDefinition to save PreValues for</param>
+        /// <param name="dataTypeId">Id of the DataTypeDefinition to save PreValues for</param>
         /// <param name="values">List of string values to save</param>
         [Obsolete("This should no longer be used, use the alternative SavePreValues or SaveDataTypeAndPreValues methods instead. This will only insert pre-values without keys")]
-        public void SavePreValues(int id, IEnumerable<string> values)
+        public void SavePreValues(int dataTypeId, IEnumerable<string> values)
         {
             //TODO: Should we raise an event here since we are really saving values for the data type?
 
@@ -215,7 +210,7 @@ namespace Umbraco.Core.Services
                     {
                         var sortOrderObj =
                         uow.Database.ExecuteScalar<object>(
-                            "SELECT max(sortorder) FROM cmsDataTypePreValues WHERE datatypeNodeId = @DataTypeId", new { DataTypeId = id });
+                            "SELECT max(sortorder) FROM cmsDataTypePreValues WHERE datatypeNodeId = @DataTypeId", new { DataTypeId = dataTypeId });
                         int sortOrder;
                         if (sortOrderObj == null || int.TryParse(sortOrderObj.ToString(), out sortOrder) == false)
                         {
@@ -224,7 +219,7 @@ namespace Umbraco.Core.Services
 
                         foreach (var value in values)
                         {
-                            var dto = new DataTypePreValueDto { DataTypeNodeId = id, Value = value, SortOrder = sortOrder };
+                            var dto = new DataTypePreValueDto { DataTypeNodeId = dataTypeId, Value = value, SortOrder = sortOrder };
                             uow.Database.Insert(dto);
                             sortOrder++;
                         }
@@ -238,26 +233,40 @@ namespace Umbraco.Core.Services
         /// <summary>
         /// Saves/updates the pre-values
         /// </summary>
-        /// <param name="id"></param>
+        /// <param name="dataTypeId"></param>
         /// <param name="values"></param>
         /// <remarks>
         /// We need to actually look up each pre-value and maintain it's id if possible - this is because of silly property editors
         /// like 'dropdown list publishing keys'
         /// </remarks>
-        public void SavePreValues(int id, IDictionary<string, PreValue> values)
+        public void SavePreValues(int dataTypeId, IDictionary<string, PreValue> values)
+        {
+            var dtd = this.GetDataTypeDefinitionById(dataTypeId);
+            if (dtd == null)
+            {
+                throw new InvalidOperationException("No data type found for id " + dataTypeId);
+            }
+            SavePreValues(dtd, values);
+        }
+
+        /// <summary>
+        /// Saves/updates the pre-values
+        /// </summary>
+        /// <param name="dataTypeDefinition"></param>
+        /// <param name="values"></param>
+        /// <remarks>
+        /// We need to actually look up each pre-value and maintain it's id if possible - this is because of silly property editors
+        /// like 'dropdown list publishing keys'
+        /// </remarks>
+        public void SavePreValues(IDataTypeDefinition dataTypeDefinition, IDictionary<string, PreValue> values)
         {
             //TODO: Should we raise an event here since we are really saving values for the data type?
 
-            using (new WriteLock(Locker))
+            var uow = _uowProvider.GetUnitOfWork();
+            using (var repository = _repositoryFactory.CreateDataTypeDefinitionRepository(uow))
             {
-                using (var uow = _uowProvider.GetUnitOfWork())
-                {
-                    using (var transaction = uow.Database.GetTransaction())
-                    {
-                        AddOrUpdatePreValues(id, values, uow);
-                        transaction.Complete();
-                    }
-                }
+                repository.AddOrUpdatePreValues(dataTypeDefinition, values);
+                uow.Commit();
             }
         }
 
@@ -272,73 +281,25 @@ namespace Umbraco.Core.Services
             if (Saving.IsRaisedEventCancelled(new SaveEventArgs<IDataTypeDefinition>(dataTypeDefinition), this))
                 return;
 
-            using (new WriteLock(Locker))
+            var uow = _uowProvider.GetUnitOfWork();
+            using (var repository = _repositoryFactory.CreateDataTypeDefinitionRepository(uow))
             {
-                var uow = (PetaPocoUnitOfWork)_uowProvider.GetUnitOfWork();
-                using (var repository = _repositoryFactory.CreateDataTypeDefinitionRepository(uow))
-                {
-                    dataTypeDefinition.CreatorId = userId;
-                    repository.AddOrUpdate(dataTypeDefinition);
+                dataTypeDefinition.CreatorId = userId;
 
-                    //complete the transaction, but run the delegate before the db transaction is finalized
-                    uow.Commit(database => AddOrUpdatePreValues(dataTypeDefinition.Id, values, uow));
+                //add/update the dtd
+                repository.AddOrUpdate(dataTypeDefinition);
 
-                    Saved.RaiseEvent(new SaveEventArgs<IDataTypeDefinition>(dataTypeDefinition, false), this);
-                }
+                //add/update the prevalues
+                repository.AddOrUpdatePreValues(dataTypeDefinition, values);
+
+                uow.Commit();
+
+                Saved.RaiseEvent(new SaveEventArgs<IDataTypeDefinition>(dataTypeDefinition, false), this);
             }
 
             Audit.Add(AuditTypes.Save, string.Format("Save DataTypeDefinition performed by user"), userId, dataTypeDefinition.Id);
         }
 
-        private void AddOrUpdatePreValues(int id, IDictionary<string, PreValue> preValueCollection, IDatabaseUnitOfWork uow)
-        {
-            //first just get all pre-values for this data type so we can compare them to see if we need to insert or update or replace
-            var sql = new Sql().Select("*")
-                               .From<DataTypePreValueDto>()
-                               .Where<DataTypePreValueDto>(dto => dto.DataTypeNodeId == id)
-                               .OrderBy<DataTypePreValueDto>(dto => dto.SortOrder);
-            var currentVals = uow.Database.Fetch<DataTypePreValueDto>(sql).ToArray();
-
-            //already existing, need to be updated
-            var valueIds = preValueCollection.Where(x => x.Value.Id > 0).Select(x => x.Value.Id).ToArray();
-            var existingByIds = currentVals.Where(x => valueIds.Contains(x.Id)).ToArray();
-
-            //These ones need to be removed from the db, they no longer exist in the new values
-            var deleteById = currentVals.Where(x => valueIds.Contains(x.Id) == false);
-
-            foreach (var d in deleteById)
-            {
-                uow.Database.Execute(
-                    "DELETE FROM cmsDataTypePreValues WHERE datatypeNodeId = @DataTypeId AND id=@Id",
-                    new { DataTypeId = id, Id = d.Id });
-            }
-
-            var sortOrder = 1;
-
-            foreach (var pre in preValueCollection)
-            {
-                var existing = existingByIds.FirstOrDefault(valueDto => valueDto.Id == pre.Value.Id);
-                if (existing != null)
-                {
-                    existing.Value = pre.Value.Value;
-                    existing.SortOrder = sortOrder;
-                    uow.Database.Update(existing);
-                }
-                else
-                {
-                    var dto = new DataTypePreValueDto
-                    {
-                        DataTypeNodeId = id,
-                        Value = pre.Value.Value,
-                        SortOrder = sortOrder,
-                        Alias = pre.Key
-                    };
-                    uow.Database.Insert(dto);
-                }
-
-                sortOrder++;
-            }
-        }
 
         /// <summary>
         /// Deletes an <see cref="IDataTypeDefinition"/>

src/Umbraco.Core/Services/IDataTypeService.cs

@@ -93,17 +93,24 @@ namespace Umbraco.Core.Services
         /// <summary>
         /// Saves a list of PreValues for a given DataTypeDefinition
         /// </summary>
-        /// <param name="id">Id of the DataTypeDefinition to save PreValues for</param>
+        /// <param name="dataTypeId">Id of the DataTypeDefinition to save PreValues for</param>
         /// <param name="values">List of string values to save</param>
         [Obsolete("This should no longer be used, use the alternative SavePreValues or SaveDataTypeAndPreValues methods instead. This will only insert pre-values without keys")]
-        void SavePreValues(int id, IEnumerable<string> values);
+        void SavePreValues(int dataTypeId, IEnumerable<string> values);
 
         /// <summary>
         /// Saves a list of PreValues for a given DataTypeDefinition
         /// </summary>
-        /// <param name="id">Id of the DataTypeDefinition to save PreValues for</param>
+        /// <param name="dataTypeId">Id of the DataTypeDefinition to save PreValues for</param>
         /// <param name="values">List of key/value pairs to save</param>
-        void SavePreValues(int id, IDictionary<string, PreValue> values);
+        void SavePreValues(int dataTypeId, IDictionary<string, PreValue> values);
+
+        /// <summary>
+        /// Saves a list of PreValues for a given DataTypeDefinition
+        /// </summary>
+        /// <param name="dataTypeDefinition">The DataTypeDefinition to save PreValues for</param>
+        /// <param name="values">List of key/value pairs to save</param>
+        void SavePreValues(IDataTypeDefinition dataTypeDefinition, IDictionary<string, PreValue> values);
 
         /// <summary>
         /// Saves the data type and it's prevalues

src/Umbraco.Core/Services/PackagingService.cs

@@ -786,11 +786,13 @@ namespace Umbraco.Core.Services
                                                         .Select(x => x.Attribute("Value").Value);
 
                 var valuesWithKeys = prevaluesElement.Elements("PreValue")
-                                                     .Where(x => ((string) x.Attribute("Alias")).IsNullOrWhiteSpace() == false)
-                                                     .ToDictionary(key => (string) key.Attribute("Alias"), val => new PreValue((string) val.Attribute("Value")));
+                    .Where(x => ((string) x.Attribute("Alias")).IsNullOrWhiteSpace() == false)
+                    .ToDictionary(
+                        key => (string) key.Attribute("Alias"),
+                        val => new PreValue((string) val.Attribute("Value")));
                 
                 //save the values with keys
-                _dataTypeService.SavePreValues(dataTypeDefinition.Id, valuesWithKeys);
+                _dataTypeService.SavePreValues(dataTypeDefinition, valuesWithKeys);
 
                 //save the values without keys (this is legacy)
                 _dataTypeService.SavePreValues(dataTypeDefinition.Id, valuesWithoutKeys);

src/Umbraco.Tests/Services/DataTypeServiceTests.cs

@@ -74,6 +74,36 @@ namespace Umbraco.Tests.Services
             var dataTypeService = ServiceContext.DataTypeService;
             var textfieldId = new Guid("ec15c1e5-9d90-422a-aa52-4f7622c63bea");
 
+            // Act
+            IDataTypeDefinition dataTypeDefinition = new DataTypeDefinition(-1, textfieldId) { Name = "Testing prevals", DatabaseType = DataTypeDatabaseType.Ntext };
+            dataTypeService.Save(dataTypeDefinition);
+            dataTypeService.SavePreValues(dataTypeDefinition, new Dictionary<string, PreValue>
+                {
+                    {"preVal1", new PreValue("Hello")},
+                    {"preVal2", new PreValue("World")}
+                });
+            //re-get
+            dataTypeDefinition = dataTypeService.GetDataTypeDefinitionById(dataTypeDefinition.Id);
+            var preVals = dataTypeService.GetPreValuesCollectionByDataTypeId(dataTypeDefinition.Id);
+
+            // Assert
+            Assert.That(dataTypeDefinition, Is.Not.Null);
+            Assert.That(dataTypeDefinition.HasIdentity, Is.True);
+            Assert.AreEqual(true, preVals.IsDictionaryBased);
+            Assert.AreEqual(2, preVals.PreValuesAsDictionary.Keys.Count);
+            Assert.AreEqual("preVal1", preVals.PreValuesAsDictionary.Keys.First());
+            Assert.AreEqual("preVal2", preVals.PreValuesAsDictionary.Keys.Last());
+            Assert.AreEqual("Hello", preVals.PreValuesAsDictionary["preVal1"].Value);
+            Assert.AreEqual("World", preVals.PreValuesAsDictionary["preVal2"].Value);
+        }
+
+        [Test]
+        public void DataTypeService_Can_Persist_Dtd_And_Dictionary_Based_Pre_Values()
+        {
+            // Arrange
+            var dataTypeService = ServiceContext.DataTypeService;
+            var textfieldId = new Guid("ec15c1e5-9d90-422a-aa52-4f7622c63bea");
+
             // Act
             IDataTypeDefinition dataTypeDefinition = new DataTypeDefinition(-1, textfieldId) { Name = "Testing prevals", DatabaseType = DataTypeDatabaseType.Ntext };
             dataTypeService.SaveDataTypeAndPreValues(dataTypeDefinition, new Dictionary<string, PreValue>
@@ -96,6 +126,77 @@ namespace Umbraco.Tests.Services
             Assert.AreEqual("World", preVals.PreValuesAsDictionary["preVal2"].Value);
         }
 
+        [Test]
+        public void DataTypeService_Can_Update_Pre_Values()
+        {
+            // Arrange
+            var dataTypeService = ServiceContext.DataTypeService;
+            var textfieldId = new Guid("ec15c1e5-9d90-422a-aa52-4f7622c63bea");
+
+            // Act
+            IDataTypeDefinition dataTypeDefinition = new DataTypeDefinition(-1, textfieldId) { Name = "Testing prevals", DatabaseType = DataTypeDatabaseType.Ntext };
+            dataTypeService.SaveDataTypeAndPreValues(dataTypeDefinition, new Dictionary<string, PreValue>
+                {
+                    {"preVal1", new PreValue("Hello")},
+                    {"preVal2", new PreValue("World")}
+                });
+            //re-get
+            dataTypeDefinition = dataTypeService.GetDataTypeDefinitionById(dataTypeDefinition.Id);
+            var preVals = dataTypeService.GetPreValuesCollectionByDataTypeId(dataTypeDefinition.Id);
+
+            //update them (ensure Ids are there!)
+            var asDictionary = preVals.FormatAsDictionary();
+            asDictionary["preVal1"].Value = "Hello2";
+            asDictionary["preVal2"].Value = "World2";
+
+            dataTypeService.SavePreValues(dataTypeDefinition, asDictionary);
+
+            var preValsAgain = dataTypeService.GetPreValuesCollectionByDataTypeId(dataTypeDefinition.Id);
+
+            // Assert
+
+            Assert.AreEqual(preVals.PreValuesAsDictionary.Values.First().Id, preValsAgain.PreValuesAsDictionary.Values.First().Id);
+            Assert.AreEqual(preVals.PreValuesAsDictionary.Values.Last().Id, preValsAgain.PreValuesAsDictionary.Values.Last().Id);
+            Assert.AreEqual("preVal1", preValsAgain.PreValuesAsDictionary.Keys.First());
+            Assert.AreEqual("preVal2", preValsAgain.PreValuesAsDictionary.Keys.Last());
+            Assert.AreEqual("Hello2", preValsAgain.PreValuesAsDictionary["preVal1"].Value);
+            Assert.AreEqual("World2", preValsAgain.PreValuesAsDictionary["preVal2"].Value);
+        }
+
+        [Test]
+        public void DataTypeService_Can_Remove_Pre_Value()
+        {
+            // Arrange
+            var dataTypeService = ServiceContext.DataTypeService;
+            var textfieldId = new Guid("ec15c1e5-9d90-422a-aa52-4f7622c63bea");
+
+            // Act
+            IDataTypeDefinition dataTypeDefinition = new DataTypeDefinition(-1, textfieldId) { Name = "Testing prevals", DatabaseType = DataTypeDatabaseType.Ntext };
+            dataTypeService.SaveDataTypeAndPreValues(dataTypeDefinition, new Dictionary<string, PreValue>
+                {
+                    {"preVal1", new PreValue("Hello")},
+                    {"preVal2", new PreValue("World")}
+                });
+            //re-get
+            dataTypeDefinition = dataTypeService.GetDataTypeDefinitionById(dataTypeDefinition.Id);
+            var preVals = dataTypeService.GetPreValuesCollectionByDataTypeId(dataTypeDefinition.Id);
+
+            //update them (ensure Ids are there!)
+            var asDictionary = preVals.FormatAsDictionary();
+            asDictionary.Remove("preVal2");
+
+            dataTypeService.SavePreValues(dataTypeDefinition, asDictionary);
+
+            var preValsAgain = dataTypeService.GetPreValuesCollectionByDataTypeId(dataTypeDefinition.Id);
+
+            // Assert
+
+            Assert.AreEqual(1, preValsAgain.FormatAsDictionary().Count);
+            Assert.AreEqual(preVals.PreValuesAsDictionary.Values.First().Id, preValsAgain.PreValuesAsDictionary.Values.First().Id);            
+            Assert.AreEqual("preVal1", preValsAgain.PreValuesAsDictionary.Keys.First());
+
+        }
+
         [Test]
         public void DataTypeService_Can_Persist_Array_Based_Pre_Values()
         {