From a8e43a39088cdc0b31a6b5a14b65aa1baf4594e5 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 21 Jan 2016 11:39:46 +0100 Subject: [PATCH 1/2] U4-7716 Content type editor - YSOD when adding a property with the same alias as it's parent --- .../Editors/ContentTypeControllerBase.cs | 212 ++++++++++++------ .../Editors/MemberTypeController.cs | 3 +- .../Models/ContentEditing/ContentTypeSave.cs | 12 +- 3 files changed, 148 insertions(+), 79 deletions(-) diff --git a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs index de7835e0d5..ae2e7f1afb 100644 --- a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs +++ b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs @@ -11,6 +11,7 @@ using Newtonsoft.Json; using Umbraco.Core; using Umbraco.Core.Configuration; using Umbraco.Core.Dictionary; +using Umbraco.Core.Exceptions; using Umbraco.Core.Models; using Umbraco.Core.PropertyEditors; using Umbraco.Core.Services; @@ -131,41 +132,7 @@ namespace Umbraco.Web.Editors }) .ToList(); } - - /// - /// Validates the composition and adds errors to the model state if any are found then throws an error response if there are errors - /// - /// - /// - /// - protected void ValidateComposition(ContentTypeSave contentTypeSave, IContentTypeComposition composition) - { - var validateAttempt = Services.ContentTypeService.ValidateComposition(composition); - if (validateAttempt == false) - { - //if it's not successful then we need to return some model state for the property aliases that - // are duplicated - var propertyAliases = validateAttempt.Result.Distinct(); - foreach (var propertyAlias in propertyAliases) - { - //find the property relating to these - var prop = contentTypeSave.Groups.SelectMany(x => x.Properties).Single(x => x.Alias == propertyAlias); - var group = contentTypeSave.Groups.Single(x => x.Properties.Contains(prop)); - var propIndex = group.Properties.IndexOf(prop); - var groupIndex = contentTypeSave.Groups.IndexOf(group); - - var key = string.Format("Groups[{0}].Properties[{1}].Alias", groupIndex, propIndex); - ModelState.AddModelError(key, "Duplicate property aliases not allowed between compositions"); - } - - var display = Mapper.Map(composition); - //map the 'save' data on top - display = Mapper.Map(contentTypeSave, display); - display.Errors = ModelState.ToErrorDictionary(); - throw new HttpResponseException(Request.CreateValidationErrorResponse(display)); - } - - } + protected string TranslateItem(string text) { @@ -185,7 +152,6 @@ namespace Umbraco.Web.Editors ContentTypeSave contentTypeSave, Func getContentType, Action saveContentType, - bool validateComposition = true, Action beforeCreateNew = null) where TContentType : class, IContentTypeComposition where TContentTypeDisplay : ContentTypeCompositionDisplay @@ -210,22 +176,7 @@ namespace Umbraco.Web.Editors if (ModelState.IsValid == false) { - TContentTypeDisplay forDisplay; - if (ctId > 0) - { - //Required data is invalid so we cannot continue - forDisplay = Mapper.Map(ct); - //map the 'save' data on top - forDisplay = Mapper.Map(contentTypeSave, forDisplay); - } - else - { - //map the 'save' data to display - forDisplay = Mapper.Map(contentTypeSave); - } - - forDisplay.Errors = ModelState.ToErrorDictionary(); - throw new HttpResponseException(Request.CreateValidationErrorResponse(forDisplay)); + throw CreateModelStateValidationException(ctId, contentTypeSave, ct); } //filter out empty properties @@ -237,15 +188,21 @@ namespace Umbraco.Web.Editors if (ctId > 0) { - //its an update to an existing + //its an update to an existing content type - Mapper.Map(contentTypeSave, ct); - - if (validateComposition) + //This mapping will cause a lot of content type validation to occur which we need to deal with + try { - //NOTE: this throws an error response if it is not valid - ValidateComposition(contentTypeSave, ct); + Mapper.Map(contentTypeSave, ct); } + catch (Exception ex) + { + var responseEx = CreateInvalidCompositionResponseException(ex, contentTypeSave, ct, ctId); + if (responseEx != null) throw responseEx; + } + + var exResult = CreateCompositionValidationExceptionIfInvalid(contentTypeSave, ct); + if (exResult != null) throw exResult; saveContentType(ct); @@ -257,12 +214,7 @@ namespace Umbraco.Web.Editors { beforeCreateNew(contentTypeSave); } - - //set id to null to ensure its handled as a new type - contentTypeSave.Id = null; - contentTypeSave.CreateDate = DateTime.Now; - contentTypeSave.UpdateDate = DateTime.Now; - + //check if the type is trying to allow type 0 below itself - id zero refers to the currently unsaved type //always filter these 0 types out var allowItselfAsChild = false; @@ -273,13 +225,26 @@ namespace Umbraco.Web.Editors } //save as new - var newCt = Mapper.Map(contentTypeSave); - - if (validateComposition) + + TContentType newCt = null; + try { - //NOTE: this throws an error response if it is not valid - ValidateComposition(contentTypeSave, newCt); + //This mapping will cause a lot of content type validation to occur which we need to deal with + newCt = Mapper.Map(contentTypeSave); } + catch (Exception ex) + { + var responseEx = CreateInvalidCompositionResponseException(ex, contentTypeSave, ct, ctId); + if (responseEx != null) throw responseEx; + } + + var exResult = CreateCompositionValidationExceptionIfInvalid(contentTypeSave, newCt); + if (exResult != null) throw exResult; + + //set id to null to ensure its handled as a new type + contentTypeSave.Id = null; + contentTypeSave.CreateDate = DateTime.Now; + contentTypeSave.UpdateDate = DateTime.Now; saveContentType(newCt); @@ -293,7 +258,7 @@ namespace Umbraco.Web.Editors return newCt; } } - + /// /// Change the sort order for media /// @@ -338,6 +303,112 @@ namespace Umbraco.Web.Editors } } + /// + /// Validates the composition and adds errors to the model state if any are found then throws an error response if there are errors + /// + /// + /// + /// + private HttpResponseException CreateCompositionValidationExceptionIfInvalid(ContentTypeSave contentTypeSave, IContentTypeComposition composition) + { + var validateAttempt = Services.ContentTypeService.ValidateComposition(composition); + if (validateAttempt == false) + { + //if it's not successful then we need to return some model state for the property aliases that + // are duplicated + var invalidPropertyAliases = validateAttempt.Result.Distinct(); + AddCompositionValidationErrors(contentTypeSave, invalidPropertyAliases); + + var display = Mapper.Map(composition); + //map the 'save' data on top + display = Mapper.Map(contentTypeSave, display); + display.Errors = ModelState.ToErrorDictionary(); + throw new HttpResponseException(Request.CreateValidationErrorResponse(display)); + } + return null; + } + + /// + /// Adds errors to the model state if any invalid aliases are found then throws an error response if there are errors + /// + /// + /// + /// + private void AddCompositionValidationErrors(ContentTypeSave contentTypeSave, IEnumerable invalidPropertyAliases) + { + foreach (var propertyAlias in invalidPropertyAliases) + { + //find the property relating to these + var prop = contentTypeSave.Groups.SelectMany(x => x.Properties).Single(x => x.Alias == propertyAlias); + var group = contentTypeSave.Groups.Single(x => x.Properties.Contains(prop)); + + var key = string.Format("Groups[{0}].Properties[{1}].Alias", group.SortOrder, prop.SortOrder); + ModelState.AddModelError(key, "Duplicate property aliases not allowed between compositions"); + } + } + + /// + /// If the exception is an InvalidCompositionException create a response exception to be thrown for validation errors + /// + /// + /// + /// + /// + /// + /// + /// + private HttpResponseException CreateInvalidCompositionResponseException( + Exception ex, ContentTypeSave contentTypeSave, TContentType ct, int ctId) + where TContentType : class, IContentTypeComposition + where TContentTypeDisplay : ContentTypeCompositionDisplay + { + InvalidCompositionException invalidCompositionException = null; + if (ex is AutoMapperMappingException && ex.InnerException is InvalidCompositionException) + { + invalidCompositionException = (InvalidCompositionException)ex.InnerException; + } + else if (ex.InnerException is InvalidCompositionException) + { + invalidCompositionException = (InvalidCompositionException)ex; + } + if (invalidCompositionException != null) + { + AddCompositionValidationErrors(contentTypeSave, invalidCompositionException.PropertyTypeAliases); + return CreateModelStateValidationException(ctId, contentTypeSave, ct); + } + return null; + } + + /// + /// Used to throw the ModelState validation results when the ModelState is invalid + /// + /// + /// + /// + /// + /// + private HttpResponseException CreateModelStateValidationException(int ctId, ContentTypeSave contentTypeSave, TContentType ct) + where TContentType : class, IContentTypeComposition + where TContentTypeDisplay : ContentTypeCompositionDisplay + { + TContentTypeDisplay forDisplay; + if (ctId > 0) + { + //Required data is invalid so we cannot continue + forDisplay = Mapper.Map(ct); + //map the 'save' data on top + forDisplay = Mapper.Map(contentTypeSave, forDisplay); + } + else + { + //map the 'save' data to display + forDisplay = Mapper.Map(contentTypeSave); + } + + forDisplay.Errors = ModelState.ToErrorDictionary(); + return new HttpResponseException(Request.CreateValidationErrorResponse(forDisplay)); + } + private ICultureDictionary CultureDictionary { get @@ -348,5 +419,6 @@ namespace Umbraco.Web.Editors } } + } } \ No newline at end of file diff --git a/src/Umbraco.Web/Editors/MemberTypeController.cs b/src/Umbraco.Web/Editors/MemberTypeController.cs index 5b95318dc1..ccd321d53a 100644 --- a/src/Umbraco.Web/Editors/MemberTypeController.cs +++ b/src/Umbraco.Web/Editors/MemberTypeController.cs @@ -134,8 +134,7 @@ namespace Umbraco.Web.Editors var savedCt = PerformPostSave( contentTypeSave: contentTypeSave, getContentType: i => Services.MemberTypeService.Get(i), - saveContentType: type => Services.MemberTypeService.Save(type), - validateComposition: false); + saveContentType: type => Services.MemberTypeService.Save(type)); var display = Mapper.Map(savedCt); diff --git a/src/Umbraco.Web/Models/ContentEditing/ContentTypeSave.cs b/src/Umbraco.Web/Models/ContentEditing/ContentTypeSave.cs index 17eb6bb35e..99d094af7b 100644 --- a/src/Umbraco.Web/Models/ContentEditing/ContentTypeSave.cs +++ b/src/Umbraco.Web/Models/ContentEditing/ContentTypeSave.cs @@ -64,10 +64,10 @@ namespace Umbraco.Web.Models.ContentEditing if (duplicateGroups.Any()) { //we need to return the field name with an index so it's wired up correctly - var firstIndex = Groups.IndexOf(duplicateGroups.First().First()); + var lastIndex = Groups.IndexOf(duplicateGroups.Last().Last()); yield return new ValidationResult("Duplicate group names not allowed", new[] { - string.Format("Groups[{0}].Name", firstIndex) + string.Format("Groups[{0}].Name", lastIndex) }); } @@ -75,14 +75,12 @@ namespace Umbraco.Web.Models.ContentEditing if (duplicateProperties.Any()) { //we need to return the field name with an index so it's wired up correctly - var firstProperty = duplicateProperties.First().First(); - var propertyGroup = Groups.Single(x => x.Properties.Contains(firstProperty)); - var groupIndex = Groups.IndexOf(propertyGroup); - var propertyIndex = propertyGroup.Properties.IndexOf(firstProperty); + var lastProperty = duplicateProperties.Last().Last(); + var propertyGroup = Groups.Single(x => x.Properties.Contains(lastProperty)); yield return new ValidationResult("Duplicate property aliases not allowed", new[] { - string.Format("Groups[{0}].Properties[{1}].Alias", groupIndex, propertyIndex) + string.Format("Groups[{0}].Properties[{1}].Alias", propertyGroup.SortOrder, lastProperty.SortOrder) }); } From 95eb5f88f36fac3b16379700d87077448403424d Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 25 Jan 2016 16:07:57 +0100 Subject: [PATCH 2/2] fixes merge issues --- .../Editors/ContentTypeControllerBase.cs | 33 ++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs index 5a861589a2..49829d984e 100644 --- a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs +++ b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs @@ -199,11 +199,11 @@ namespace Umbraco.Web.Editors } catch (Exception ex) { - var responseEx = CreateInvalidCompositionResponseException(ex, contentTypeSave, ct, ctId); + var responseEx = CreateInvalidCompositionResponseException(ex, contentTypeSave, ct, ctId); if (responseEx != null) throw responseEx; } - var exResult = CreateCompositionValidationExceptionIfInvalid(contentTypeSave, ct); + var exResult = CreateCompositionValidationExceptionIfInvalid(contentTypeSave, ct); if (exResult != null) throw exResult; saveContentType(ct); @@ -236,11 +236,11 @@ namespace Umbraco.Web.Editors } catch (Exception ex) { - var responseEx = CreateInvalidCompositionResponseException(ex, contentTypeSave, ct, ctId); + var responseEx = CreateInvalidCompositionResponseException(ex, contentTypeSave, ct, ctId); if (responseEx != null) throw responseEx; } - var exResult = CreateCompositionValidationExceptionIfInvalid(contentTypeSave, newCt); + var exResult = CreateCompositionValidationExceptionIfInvalid(contentTypeSave, newCt); if (exResult != null) throw exResult; //set id to null to ensure its handled as a new type @@ -311,7 +311,10 @@ namespace Umbraco.Web.Editors /// /// /// - private HttpResponseException CreateCompositionValidationExceptionIfInvalid(ContentTypeSave contentTypeSave, IContentTypeComposition composition) + private HttpResponseException CreateCompositionValidationExceptionIfInvalid(TContentTypeSave contentTypeSave, IContentTypeComposition composition) + where TContentTypeSave : ContentTypeSave + where TPropertyType : PropertyTypeBasic + where TContentTypeDisplay : ContentTypeCompositionDisplay { var validateAttempt = Services.ContentTypeService.ValidateComposition(composition); if (validateAttempt == false) @@ -319,9 +322,9 @@ namespace Umbraco.Web.Editors //if it's not successful then we need to return some model state for the property aliases that // are duplicated var invalidPropertyAliases = validateAttempt.Result.Distinct(); - AddCompositionValidationErrors(contentTypeSave, invalidPropertyAliases); + AddCompositionValidationErrors(contentTypeSave, invalidPropertyAliases); - var display = Mapper.Map(composition); + var display = Mapper.Map(composition); //map the 'save' data on top display = Mapper.Map(contentTypeSave, display); display.Errors = ModelState.ToErrorDictionary(); @@ -336,7 +339,9 @@ namespace Umbraco.Web.Editors /// /// /// - private void AddCompositionValidationErrors(ContentTypeSave contentTypeSave, IEnumerable invalidPropertyAliases) + private void AddCompositionValidationErrors(TContentTypeSave contentTypeSave, IEnumerable invalidPropertyAliases) + where TContentTypeSave : ContentTypeSave + where TPropertyType : PropertyTypeBasic { foreach (var propertyAlias in invalidPropertyAliases) { @@ -354,15 +359,19 @@ namespace Umbraco.Web.Editors /// /// /// + /// + /// /// /// /// /// /// - private HttpResponseException CreateInvalidCompositionResponseException( - Exception ex, ContentTypeSave contentTypeSave, TContentType ct, int ctId) + private HttpResponseException CreateInvalidCompositionResponseException( + Exception ex, TContentTypeSave contentTypeSave, TContentType ct, int ctId) where TContentType : class, IContentTypeComposition - where TContentTypeDisplay : ContentTypeCompositionDisplay + where TContentTypeDisplay : ContentTypeCompositionDisplay + where TContentTypeSave : ContentTypeSave + where TPropertyType : PropertyTypeBasic { InvalidCompositionException invalidCompositionException = null; if (ex is AutoMapperMappingException && ex.InnerException is InvalidCompositionException) @@ -375,7 +384,7 @@ namespace Umbraco.Web.Editors } if (invalidCompositionException != null) { - AddCompositionValidationErrors(contentTypeSave, invalidCompositionException.PropertyTypeAliases); + AddCompositionValidationErrors(contentTypeSave, invalidCompositionException.PropertyTypeAliases); return CreateModelStateValidationException(ctId, contentTypeSave, ct); } return null;