From 3f3262eb0115a2b92fad8d9b8d1bc78cc24d399e Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Thu, 14 Oct 2021 09:54:14 +0200 Subject: [PATCH 1/7] Adding property group aliases to ex.message --- .../Exceptions/InvalidCompositionException.cs | 51 ++++++++++++++----- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/src/Umbraco.Core/Exceptions/InvalidCompositionException.cs b/src/Umbraco.Core/Exceptions/InvalidCompositionException.cs index 95cd27b2cd..f1bb4d2dd5 100644 --- a/src/Umbraco.Core/Exceptions/InvalidCompositionException.cs +++ b/src/Umbraco.Core/Exceptions/InvalidCompositionException.cs @@ -85,18 +85,45 @@ namespace Umbraco.Core.Exceptions private static string FormatMessage(string contentTypeAlias, string addedCompositionAlias, string[] propertyTypeAliases, string[] propertyGroupAliases) { - // TODO Add property group aliases to message - return addedCompositionAlias.IsNullOrWhiteSpace() - ? string.Format( - "ContentType with alias '{0}' has an invalid composition " + - "and there was a conflict on the following PropertyTypes: '{1}'. " + - "PropertyTypes must have a unique alias across all Compositions in order to compose a valid ContentType Composition.", - contentTypeAlias, string.Join(", ", propertyTypeAliases)) - : string.Format( - "ContentType with alias '{0}' was added as a Composition to ContentType with alias '{1}', " + - "but there was a conflict on the following PropertyTypes: '{2}'. " + - "PropertyTypes must have a unique alias across all Compositions in order to compose a valid ContentType Composition.", - addedCompositionAlias, contentTypeAlias, string.Join(", ", propertyTypeAliases)); + // list both propertyTypeAliases and propertyGroupAliases + var customMsg = string.Format("PropertyTypes: '{0}' and PropertyGroups: '{1}'. PropertyTypes and PropertyGroups", + string.Join(", ", propertyTypeAliases), string.Join(", ", propertyGroupAliases)); + + var endMsg = " must have a unique alias across all Compositions in order to compose a valid ContentType Composition."; + + // list only propertyGroupAliases when there are no property type aliases + if (propertyTypeAliases.Length == 0) + { + customMsg = string.Format("PropertyGroups: '{0}'. PropertyGroups", + string.Join(", ", propertyGroupAliases)); + } + else + { + // list only propertyTypeAliases when there are no property group aliases + if (propertyGroupAliases.Length == 0) + { + customMsg = string.Format("PropertyTypes: '{0}'. PropertyTypes", + string.Join(", ", propertyTypeAliases)); + } + } + + string message; + if (addedCompositionAlias.IsNullOrWhiteSpace()) + { + var startMsg = "ContentType with alias '{0}' has an invalid composition " + + "and there was a conflict on the following "; + + message = string.Format(startMsg + customMsg + endMsg, contentTypeAlias); + } + else + { + var startMsg = "ContentType with alias '{0}' was added as a Composition to ContentType with alias '{1}', " + + "but there was a conflict on the following "; + + message = string.Format(startMsg + customMsg + endMsg, addedCompositionAlias, contentTypeAlias); + } + + return message; } /// From ccd9013d30accdb1ea69d3f0eb0b332d03bc6141 Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Thu, 14 Oct 2021 09:57:13 +0200 Subject: [PATCH 2/7] Adding invalid prop group aliases as ModelState errors, so we don't introduce breaking changes --- .../Editors/ContentTypeControllerBase.cs | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs index 6ac936961f..e4a5b2e417 100644 --- a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs +++ b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs @@ -454,10 +454,12 @@ namespace Umbraco.Web.Editors var validateAttempt = service.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); + // if it's not successful then we need to return some model state for the property type and property group + // aliases that are duplicated + var invalidPropertyTypeAliases = validateAttempt.Result.Distinct(); + var invalidPropertyGroupAliases = (validateAttempt.Exception as InvalidCompositionException)?.PropertyGroupAliases ?? Array.Empty(); + + AddCompositionValidationErrors(contentTypeSave, invalidPropertyTypeAliases, invalidPropertyGroupAliases); var display = Mapper.Map(composition); //map the 'save' data on top @@ -472,22 +474,32 @@ namespace Umbraco.Web.Editors /// Adds errors to the model state if any invalid aliases are found then throws an error response if there are errors /// /// - /// + /// + /// /// - private void AddCompositionValidationErrors(TContentTypeSave contentTypeSave, IEnumerable invalidPropertyAliases) + private void AddCompositionValidationErrors(TContentTypeSave contentTypeSave, IEnumerable invalidPropertyTypeAliases, IEnumerable invalidPropertyGroupAliases) where TContentTypeSave : ContentTypeSave where TPropertyType : PropertyTypeBasic { - foreach (var propertyAlias in invalidPropertyAliases) + foreach (var propertyTypeAlias in invalidPropertyTypeAliases) { - // Find the property relating to these - var property = contentTypeSave.Groups.SelectMany(x => x.Properties).Single(x => x.Alias == propertyAlias); + // Find the property type relating to these + var property = contentTypeSave.Groups.SelectMany(x => x.Properties).Single(x => x.Alias == propertyTypeAlias); var group = contentTypeSave.Groups.Single(x => x.Properties.Contains(property)); var propertyIndex = group.Properties.IndexOf(property); var groupIndex = contentTypeSave.Groups.IndexOf(group); var key = $"Groups[{groupIndex}].Properties[{propertyIndex}].Alias"; - ModelState.AddModelError(key, "Duplicate property aliases not allowed between compositions"); + ModelState.AddModelError(key, "Duplicate property aliases are not allowed between compositions"); + } + + foreach (var propertyGroupAlias in invalidPropertyGroupAliases) + { + // Find the property group relating to these + var group = contentTypeSave.Groups.Single(x => x.Alias == propertyGroupAlias); + var groupIndex = contentTypeSave.Groups.IndexOf(group); + var key = $"Groups[{groupIndex}].Name"; + ModelState.AddModelError(key, "Duplicate property group aliases are not allowed between compositions"); } } @@ -519,7 +531,7 @@ namespace Umbraco.Web.Editors } if (invalidCompositionException != null) { - AddCompositionValidationErrors(contentTypeSave, invalidCompositionException.PropertyTypeAliases); + AddCompositionValidationErrors(contentTypeSave, invalidCompositionException.PropertyTypeAliases, invalidCompositionException.PropertyGroupAliases); return CreateModelStateValidationException(ctId, contentTypeSave, ct); } return null; From b8ecc17140f93e9b528445609a5ea185b87e30ca Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Thu, 14 Oct 2021 13:41:56 +0200 Subject: [PATCH 3/7] Pointing the actual reason for invalidating composition --- src/Umbraco.Web/Editors/ContentTypeControllerBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs index e4a5b2e417..60e10313ca 100644 --- a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs +++ b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs @@ -499,7 +499,7 @@ namespace Umbraco.Web.Editors var group = contentTypeSave.Groups.Single(x => x.Alias == propertyGroupAlias); var groupIndex = contentTypeSave.Groups.IndexOf(group); var key = $"Groups[{groupIndex}].Name"; - ModelState.AddModelError(key, "Duplicate property group aliases are not allowed between compositions"); + ModelState.AddModelError(key, "Same alias for tab and group is not allowed between compositions"); } } From 1b06db8d5cc18c7813afbf0d1505d7d4bb1982e3 Mon Sep 17 00:00:00 2001 From: Ronald Barendse Date: Thu, 14 Oct 2021 14:47:57 +0200 Subject: [PATCH 4/7] Validate all content type dependencies and throw a single InvalidCompositionException --- ...entTypeServiceBaseOfTRepositoryTItemTService.cs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Core/Services/Implement/ContentTypeServiceBaseOfTRepositoryTItemTService.cs b/src/Umbraco.Core/Services/Implement/ContentTypeServiceBaseOfTRepositoryTItemTService.cs index 11142ad96e..385bfec923 100644 --- a/src/Umbraco.Core/Services/Implement/ContentTypeServiceBaseOfTRepositoryTItemTService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentTypeServiceBaseOfTRepositoryTItemTService.cs @@ -97,18 +97,22 @@ namespace Umbraco.Core.Services.Implement stack.Push(c); } + var duplicatePropertyTypeAliases = new List(); + var invalidPropertyGroupAliases = new List(); + foreach (var dependency in dependencies) { if (dependency.Id == compositionContentType.Id) continue; var contentTypeDependency = allContentTypes.FirstOrDefault(x => x.Alias.Equals(dependency.Alias, StringComparison.InvariantCultureIgnoreCase)); if (contentTypeDependency == null) continue; - var duplicatePropertyTypeAliases = contentTypeDependency.PropertyTypes.Select(x => x.Alias).Intersect(propertyTypeAliases, StringComparer.InvariantCultureIgnoreCase).ToArray(); - var invalidPropertyGroupAliases = contentTypeDependency.PropertyGroups.Where(x => propertyGroupAliases.TryGetValue(x.Alias, out var type) && type != x.Type).Select(x => x.Alias).ToArray(); + duplicatePropertyTypeAliases.AddRange(contentTypeDependency.PropertyTypes.Select(x => x.Alias).Intersect(propertyTypeAliases, StringComparer.InvariantCultureIgnoreCase)); + invalidPropertyGroupAliases.AddRange(contentTypeDependency.PropertyGroups.Where(x => propertyGroupAliases.TryGetValue(x.Alias, out var type) && type != x.Type).Select(x => x.Alias)); + } - if (duplicatePropertyTypeAliases.Length == 0 && invalidPropertyGroupAliases.Length == 0) continue; - - throw new InvalidCompositionException(compositionContentType.Alias, null, duplicatePropertyTypeAliases, invalidPropertyGroupAliases); + if (duplicatePropertyTypeAliases.Count > 0 || invalidPropertyGroupAliases.Count > 0) + { + throw new InvalidCompositionException(compositionContentType.Alias, null, duplicatePropertyTypeAliases.Distinct().ToArray(), invalidPropertyGroupAliases.Distinct().ToArray()); } } From 6666f6aab7c3f62eefb52609a2889148c3517c53 Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska Date: Thu, 14 Oct 2021 14:57:52 +0200 Subject: [PATCH 5/7] Rename based on review comments --- src/Umbraco.Web/Editors/ContentTypeControllerBase.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs index 60e10313ca..b38e2b619f 100644 --- a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs +++ b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs @@ -456,10 +456,10 @@ namespace Umbraco.Web.Editors { // if it's not successful then we need to return some model state for the property type and property group // aliases that are duplicated - var invalidPropertyTypeAliases = validateAttempt.Result.Distinct(); + var duplicatePropertyTypeAliases = validateAttempt.Result.Distinct(); var invalidPropertyGroupAliases = (validateAttempt.Exception as InvalidCompositionException)?.PropertyGroupAliases ?? Array.Empty(); - AddCompositionValidationErrors(contentTypeSave, invalidPropertyTypeAliases, invalidPropertyGroupAliases); + AddCompositionValidationErrors(contentTypeSave, duplicatePropertyTypeAliases, invalidPropertyGroupAliases); var display = Mapper.Map(composition); //map the 'save' data on top @@ -474,14 +474,14 @@ namespace Umbraco.Web.Editors /// Adds errors to the model state if any invalid aliases are found then throws an error response if there are errors /// /// - /// + /// /// /// - private void AddCompositionValidationErrors(TContentTypeSave contentTypeSave, IEnumerable invalidPropertyTypeAliases, IEnumerable invalidPropertyGroupAliases) + private void AddCompositionValidationErrors(TContentTypeSave contentTypeSave, IEnumerable duplicatePropertyTypeAliases, IEnumerable invalidPropertyGroupAliases) where TContentTypeSave : ContentTypeSave where TPropertyType : PropertyTypeBasic { - foreach (var propertyTypeAlias in invalidPropertyTypeAliases) + foreach (var propertyTypeAlias in duplicatePropertyTypeAliases) { // Find the property type relating to these var property = contentTypeSave.Groups.SelectMany(x => x.Properties).Single(x => x.Alias == propertyTypeAlias); From 705a3ed4e86cc2df630eab239fc93bea9ad613f3 Mon Sep 17 00:00:00 2001 From: Ronald Barendse Date: Thu, 14 Oct 2021 15:06:28 +0200 Subject: [PATCH 6/7] Update composition validation error messages --- src/Umbraco.Web/Editors/ContentTypeControllerBase.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs index 60e10313ca..232ca92234 100644 --- a/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs +++ b/src/Umbraco.Web/Editors/ContentTypeControllerBase.cs @@ -490,7 +490,7 @@ namespace Umbraco.Web.Editors var groupIndex = contentTypeSave.Groups.IndexOf(group); var key = $"Groups[{groupIndex}].Properties[{propertyIndex}].Alias"; - ModelState.AddModelError(key, "Duplicate property aliases are not allowed between compositions"); + ModelState.AddModelError(key, "Duplicate property aliases aren't allowed between compositions"); } foreach (var propertyGroupAlias in invalidPropertyGroupAliases) @@ -499,7 +499,7 @@ namespace Umbraco.Web.Editors var group = contentTypeSave.Groups.Single(x => x.Alias == propertyGroupAlias); var groupIndex = contentTypeSave.Groups.IndexOf(group); var key = $"Groups[{groupIndex}].Name"; - ModelState.AddModelError(key, "Same alias for tab and group is not allowed between compositions"); + ModelState.AddModelError(key, "Different group types aren't allowed between compositions"); } } From b73ab255e47fe1a821a2b214c5fe9efeaca203c1 Mon Sep 17 00:00:00 2001 From: Ronald Barendse Date: Thu, 14 Oct 2021 15:50:29 +0200 Subject: [PATCH 7/7] Update InvalidCompositionException message --- .../Exceptions/InvalidCompositionException.cs | 46 ++++++------------- 1 file changed, 15 insertions(+), 31 deletions(-) diff --git a/src/Umbraco.Core/Exceptions/InvalidCompositionException.cs b/src/Umbraco.Core/Exceptions/InvalidCompositionException.cs index f1bb4d2dd5..5d2beaa387 100644 --- a/src/Umbraco.Core/Exceptions/InvalidCompositionException.cs +++ b/src/Umbraco.Core/Exceptions/InvalidCompositionException.cs @@ -1,5 +1,6 @@ using System; using System.Runtime.Serialization; +using System.Text; namespace Umbraco.Core.Exceptions { @@ -85,45 +86,28 @@ namespace Umbraco.Core.Exceptions private static string FormatMessage(string contentTypeAlias, string addedCompositionAlias, string[] propertyTypeAliases, string[] propertyGroupAliases) { - // list both propertyTypeAliases and propertyGroupAliases - var customMsg = string.Format("PropertyTypes: '{0}' and PropertyGroups: '{1}'. PropertyTypes and PropertyGroups", - string.Join(", ", propertyTypeAliases), string.Join(", ", propertyGroupAliases)); + var sb = new StringBuilder(); - var endMsg = " must have a unique alias across all Compositions in order to compose a valid ContentType Composition."; - - // list only propertyGroupAliases when there are no property type aliases - if (propertyTypeAliases.Length == 0) - { - customMsg = string.Format("PropertyGroups: '{0}'. PropertyGroups", - string.Join(", ", propertyGroupAliases)); - } - else - { - // list only propertyTypeAliases when there are no property group aliases - if (propertyGroupAliases.Length == 0) - { - customMsg = string.Format("PropertyTypes: '{0}'. PropertyTypes", - string.Join(", ", propertyTypeAliases)); - } - } - - string message; if (addedCompositionAlias.IsNullOrWhiteSpace()) { - var startMsg = "ContentType with alias '{0}' has an invalid composition " + - "and there was a conflict on the following "; - - message = string.Format(startMsg + customMsg + endMsg, contentTypeAlias); + sb.AppendFormat("Content type with alias '{0}' has an invalid composition.", contentTypeAlias); } else { - var startMsg = "ContentType with alias '{0}' was added as a Composition to ContentType with alias '{1}', " + - "but there was a conflict on the following "; - - message = string.Format(startMsg + customMsg + endMsg, addedCompositionAlias, contentTypeAlias); + sb.AppendFormat("Content type with alias '{0}' was added as a composition to content type with alias '{1}', but there was a conflict.", addedCompositionAlias, contentTypeAlias); } - return message; + if (propertyTypeAliases.Length > 0) + { + sb.AppendFormat(" Property types must have a unique alias across all compositions, these aliases are duplicate: {0}.", string.Join(", ", propertyTypeAliases)); + } + + if (propertyGroupAliases.Length > 0) + { + sb.AppendFormat(" Property groups with the same alias must also have the same type across all compositions, these aliases have different types: {0}.", string.Join(", ", propertyGroupAliases)); + } + + return sb.ToString(); } ///