From 4a412bb432a9dd05629bec3daa8fe7c86573ea63 Mon Sep 17 00:00:00 2001 From: Jacob Overgaard <752371+iOvergaard@users.noreply.github.com> Date: Wed, 12 Oct 2022 12:39:11 +0200 Subject: [PATCH 1/5] Fix for potential race condition in packages search (#13153) * search on input allowing to wait for copy/paste etc * invoke resourcePromise() with correct parameters * return the xhrStatus allowing the caller to check if the request was aborted * fix: send in canceler.promise to allow the timeout to work * catch any errors and ignore aborts if they happen * move the logic to handle cancellations outside Angulars $scope.$apply * remove file accidentally committed --- .../ourpackagerrepository.resource.js | 8 ++-- .../services/umbrequesthelper.service.js | 3 +- .../views/packages/views/repo.controller.js | 38 +++++++++++++------ .../src/views/packages/views/repo.html | 2 +- 4 files changed, 34 insertions(+), 17 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/resources/ourpackagerrepository.resource.js b/src/Umbraco.Web.UI.Client/src/common/resources/ourpackagerrepository.resource.js index be13e6d0ec60..430f05c2c42b 100644 --- a/src/Umbraco.Web.UI.Client/src/common/resources/ourpackagerrepository.resource.js +++ b/src/Umbraco.Web.UI.Client/src/common/resources/ourpackagerrepository.resource.js @@ -57,7 +57,7 @@ function ourPackageRepositoryResource($q, $http, umbDataFormatter, umbRequestHel if (canceler) { httpConfig["timeout"] = canceler; } - + if (category === undefined) { category = ""; } @@ -69,8 +69,10 @@ function ourPackageRepositoryResource($q, $http, umbDataFormatter, umbRequestHel var order = !orderBy ? "&order=Default" : ("&order=" + orderBy); return umbRequestHelper.resourcePromise( - $http.get(baseurl + "?pageIndex=" + pageIndex + "&pageSize=" + pageSize + "&category=" + category + "&query=" + query + order + "&version=" + Umbraco.Sys.ServerVariables.application.version), - httpConfig, + $http.get( + baseurl + "?pageIndex=" + pageIndex + "&pageSize=" + pageSize + "&category=" + category + "&query=" + query + order + "&version=" + Umbraco.Sys.ServerVariables.application.version, + httpConfig + ), 'Failed to query packages'); } diff --git a/src/Umbraco.Web.UI.Client/src/common/services/umbrequesthelper.service.js b/src/Umbraco.Web.UI.Client/src/common/services/umbrequesthelper.service.js index 2b5447cdf681..b419600653ed 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/umbrequesthelper.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/umbrequesthelper.service.js @@ -197,7 +197,8 @@ function umbRequestHelper($http, $q, notificationsService, eventsService, formHe return $q.reject({ errorMsg: result.errorMsg, data: result.data, - status: result.status + status: result.status, + xhrStatus: response.xhrStatus }); }); diff --git a/src/Umbraco.Web.UI.Client/src/views/packages/views/repo.controller.js b/src/Umbraco.Web.UI.Client/src/views/packages/views/repo.controller.js index 8360663be621..b690efacdfb6 100644 --- a/src/Umbraco.Web.UI.Client/src/views/packages/views/repo.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/packages/views/repo.controller.js @@ -1,7 +1,7 @@ (function () { "use strict"; - function PackagesRepoController($scope, $timeout, ourPackageRepositoryResource, $q, localizationService) { + function PackagesRepoController($scope, $timeout, ourPackageRepositoryResource, $q, localizationService, notificationsService) { var vm = this; @@ -197,18 +197,14 @@ var searchDebounced = _.debounce(function (e) { + //a canceler exists, so perform the cancelation operation and reset + if (canceler) { + canceler.resolve(); + } - $scope.$apply(function () { - - //a canceler exists, so perform the cancelation operation and reset - if (canceler) { - canceler.resolve(); - canceler = $q.defer(); - } - else { - canceler = $q.defer(); - } + canceler = $q.defer(); + $scope.$apply(function () { currSort = vm.searchQuery ? "Default" : "Latest"; ourPackageRepositoryResource.search(vm.pagination.pageNumber - 1, @@ -216,7 +212,7 @@ currSort, "", vm.searchQuery, - canceler) + canceler.promise) .then(function (pack) { vm.packages = pack.packages; vm.pagination.totalPages = Math.ceil(pack.total / vm.pagination.pageSize); @@ -224,6 +220,24 @@ vm.loading = false; //set back to null so it can be re-created canceler = null; + }) + .catch(function (err) { + canceler = null; + + if (err) { + // If an abort happened, ignore it since it happened because of a new search + if (err.xhrStatus === 'abort') { + return; + } + + // Otherwise, show the error + if (err.errorMsg) { + notificationsService.error(err.errorMsg); + return; + } + } + + console.error(err); }); }); diff --git a/src/Umbraco.Web.UI.Client/src/views/packages/views/repo.html b/src/Umbraco.Web.UI.Client/src/views/packages/views/repo.html index f7e976de1ec5..1c3e3ec5d82c 100644 --- a/src/Umbraco.Web.UI.Client/src/views/packages/views/repo.html +++ b/src/Umbraco.Web.UI.Client/src/views/packages/views/repo.html @@ -17,7 +17,7 @@ localize="placeholder" placeholder="@packager_packageSearch" ng-model="vm.searchQuery" - ng-change="vm.search()" + ng-on-input="vm.search()" no-dirty-check /> From 29ae87ec6129387cba4a0c81c3402a85ccefc502 Mon Sep 17 00:00:00 2001 From: Mole Date: Thu, 13 Oct 2022 08:17:02 +0200 Subject: [PATCH 2/5] V10: Fix request accessor memory leak (#13152) * Dispose OnChange event registration when disposing the notification handler * Ensure that the ApplicationUrl is only initialized once Since notifications handlers are transient,_hasAppUrl and _isInit defaults to false on every request causing it to always be called. * Make notification handler and EnsureApplicationUrl internal --- ...ationUrlRequestBeginNotificationHandler.cs | 26 ++++++++++++++ .../AspNetCore/AspNetCoreRequestAccessor.cs | 35 +++++++++++++------ .../UmbracoBuilderExtensions.cs | 1 + 3 files changed, 51 insertions(+), 11 deletions(-) create mode 100644 src/Umbraco.Web.Common/AspNetCore/ApplicationUrlRequestBeginNotificationHandler.cs diff --git a/src/Umbraco.Web.Common/AspNetCore/ApplicationUrlRequestBeginNotificationHandler.cs b/src/Umbraco.Web.Common/AspNetCore/ApplicationUrlRequestBeginNotificationHandler.cs new file mode 100644 index 000000000000..0472d315923a --- /dev/null +++ b/src/Umbraco.Web.Common/AspNetCore/ApplicationUrlRequestBeginNotificationHandler.cs @@ -0,0 +1,26 @@ +using Umbraco.Cms.Core.Events; +using Umbraco.Cms.Core.Notifications; +using Umbraco.Cms.Core.Web; + +namespace Umbraco.Cms.Web.Common.AspNetCore; + +/// +/// Notification handler which will listen to the , and ensure that +/// the applicationUrl is set on the first request. +/// +internal class ApplicationUrlRequestBeginNotificationHandler : INotificationHandler +{ + private readonly IRequestAccessor _requestAccessor; + + public ApplicationUrlRequestBeginNotificationHandler(IRequestAccessor requestAccessor) => + _requestAccessor = requestAccessor; + + public void Handle(UmbracoRequestBeginNotification notification) + { + // If someone has replaced the AspNetCoreRequestAccessor we'll do nothing and assume they handle it themselves. + if (_requestAccessor is AspNetCoreRequestAccessor accessor) + { + accessor.EnsureApplicationUrl(); + } + } +} diff --git a/src/Umbraco.Web.Common/AspNetCore/AspNetCoreRequestAccessor.cs b/src/Umbraco.Web.Common/AspNetCore/AspNetCoreRequestAccessor.cs index 38d67ff2f08e..de47999835c4 100644 --- a/src/Umbraco.Web.Common/AspNetCore/AspNetCoreRequestAccessor.cs +++ b/src/Umbraco.Web.Common/AspNetCore/AspNetCoreRequestAccessor.cs @@ -9,7 +9,7 @@ namespace Umbraco.Cms.Web.Common.AspNetCore; -public class AspNetCoreRequestAccessor : IRequestAccessor, INotificationHandler +public class AspNetCoreRequestAccessor : IRequestAccessor, INotificationHandler, IDisposable { private readonly ISet _applicationUrls = new HashSet(); private readonly IHttpContextAccessor _httpContextAccessor; @@ -18,6 +18,7 @@ public class AspNetCoreRequestAccessor : IRequestAccessor, INotificationHandler< private object _initLocker = new(); private bool _isInit; private WebRoutingSettings _webRoutingSettings; + private readonly IDisposable? _onChangeDisposable; /// /// Initializes a new instance of the class. @@ -28,20 +29,19 @@ public AspNetCoreRequestAccessor( { _httpContextAccessor = httpContextAccessor; _webRoutingSettings = webRoutingSettings.CurrentValue; - webRoutingSettings.OnChange(x => _webRoutingSettings = x); + _onChangeDisposable = webRoutingSettings.OnChange(x => _webRoutingSettings = x); } /// - /// This just initializes the application URL on first request attempt - /// TODO: This doesn't belong here, the GetApplicationUrl doesn't belong to IRequestAccessor - /// this should be part of middleware not a lazy init based on an INotification + /// + /// This is now a NoOp, and is no longer used, instead ApplicationUrlRequestBeginNotificationHandler is used + /// /// + [Obsolete("This is no longer used, AspNetCoreRequestAccessor will no longer implement INotificationHandler in V12, see ApplicationUrlRequestBeginNotificationHandler instead.")] public void Handle(UmbracoRequestBeginNotification notification) - => LazyInitializer.EnsureInitialized(ref _hasAppUrl, ref _isInit, ref _initLocker, () => - { - GetApplicationUrl(); - return true; - }); + { + // NoOP + } /// public string GetRequestValue(string name) => GetFormValue(name) ?? GetQueryStringValue(name); @@ -54,6 +54,17 @@ public void Handle(UmbracoRequestBeginNotification notification) ? new Uri(_httpContextAccessor.HttpContext.Request.GetEncodedUrl()) : null; + /// + /// Ensure that the ApplicationUrl is set on the first request, this is using a LazyInitializer, so the code will only be run the first time + /// + /// TODO: This doesn't belong here, the GetApplicationUrl doesn't belong to IRequestAccessor + internal void EnsureApplicationUrl() => + LazyInitializer.EnsureInitialized(ref _hasAppUrl, ref _isInit, ref _initLocker, () => + { + GetApplicationUrl(); + return true; + }); + public Uri? GetApplicationUrl() { // Fixme: This causes problems with site swap on azure because azure pre-warms a site by calling into `localhost` and when it does that @@ -63,7 +74,7 @@ public void Handle(UmbracoRequestBeginNotification notification) // see U4-10626 - in some cases we want to reset the application url // (this is a simplified version of what was in 7.x) // note: should this be optional? is it expensive? - if (!(_webRoutingSettings.UmbracoApplicationUrl is null)) + if (_webRoutingSettings.UmbracoApplicationUrl is not null) { return new Uri(_webRoutingSettings.UmbracoApplicationUrl); } @@ -96,4 +107,6 @@ public void Handle(UmbracoRequestBeginNotification notification) return request.Form[name]; } + + public void Dispose() => _onChangeDisposable?.Dispose(); } diff --git a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs index 40b84a0987e3..d454e5d6c56e 100644 --- a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs +++ b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs @@ -298,6 +298,7 @@ public static IUmbracoBuilder AddWebComponents(this IUmbracoBuilder builder) // AspNetCore specific services builder.Services.AddUnique(); builder.AddNotificationHandler(); + builder.AddNotificationHandler(); // Password hasher builder.Services.AddUnique(); From 9f6d5e954340ca737294450b6355390647d07693 Mon Sep 17 00:00:00 2001 From: Mole Date: Thu, 13 Oct 2022 08:24:50 +0200 Subject: [PATCH 3/5] Add missing ForceLeft and ForceRight (#13190) --- .../Models/Blocks/BlockGridLayoutItem.cs | 6 ++++++ .../ValueConverters/BlockGridPropertyValueConverter.cs | 2 ++ 2 files changed, 8 insertions(+) diff --git a/src/Umbraco.Infrastructure/Models/Blocks/BlockGridLayoutItem.cs b/src/Umbraco.Infrastructure/Models/Blocks/BlockGridLayoutItem.cs index 30494b03f7d3..9014e2789a97 100644 --- a/src/Umbraco.Infrastructure/Models/Blocks/BlockGridLayoutItem.cs +++ b/src/Umbraco.Infrastructure/Models/Blocks/BlockGridLayoutItem.cs @@ -25,6 +25,12 @@ public class BlockGridLayoutItem : IBlockLayoutItem [JsonProperty("rowSpan", NullValueHandling = NullValueHandling.Ignore)] public int? RowSpan { get; set; } + [JsonProperty("forceLeft")] + public bool ForceLeft { get; set; } + + [JsonProperty("forceRight")] + public bool ForceRight { get; set; } + [JsonProperty("areas", NullValueHandling = NullValueHandling.Ignore)] public BlockGridLayoutAreaItem[] Areas { get; set; } = Array.Empty(); } diff --git a/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/BlockGridPropertyValueConverter.cs b/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/BlockGridPropertyValueConverter.cs index ae330870fac9..6cd2d4bd3820 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/BlockGridPropertyValueConverter.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/BlockGridPropertyValueConverter.cs @@ -49,6 +49,8 @@ public override bool IsConverter(IPublishedPropertyType propertyType) blockItem.RowSpan = layoutItem.RowSpan!.Value; blockItem.ColumnSpan = layoutItem.ColumnSpan!.Value; + blockItem.ForceLeft = layoutItem.ForceLeft; + blockItem.ForceRight = layoutItem.ForceRight; blockItem.AreaGridColumns = blockConfig.AreaGridColumns; blockItem.GridColumns = configuration.GridColumns; blockItem.Areas = layoutItem.Areas.Select(area => From cdc994dd8ee16f539e0115c514f8b1168f48fd10 Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska <21998037+elit0451@users.noreply.github.com> Date: Thu, 13 Oct 2022 08:59:53 +0200 Subject: [PATCH 4/5] Fix tags with CSV storage type (#13188) * Fixing null check as default(NRT) is null => default(configuration?.Delimiter) is also null and we were counting on it being the same as default(char) * Adding tests to check cases with multiple tags (or tag made of comma separated values) --- .../Models/PropertyTagsExtensions.cs | 2 +- .../Services/ContentServiceTagsTests.cs | 73 +++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Models/PropertyTagsExtensions.cs b/src/Umbraco.Core/Models/PropertyTagsExtensions.cs index 9ad98d66c0ac..d36baed60401 100644 --- a/src/Umbraco.Core/Models/PropertyTagsExtensions.cs +++ b/src/Umbraco.Core/Models/PropertyTagsExtensions.cs @@ -33,7 +33,7 @@ public static class PropertyTagsExtensions : dataTypeService.GetDataType(property.PropertyType.DataTypeId)?.Configuration; TagConfiguration? configuration = ConfigurationEditor.ConfigurationAs(configurationObject); - if (configuration?.Delimiter == default && configuration?.Delimiter is not null) + if (configuration is not null && configuration.Delimiter == default) { configuration.Delimiter = tagAttribute.Delimiter; } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentServiceTagsTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentServiceTagsTests.cs index 25e7aa10097e..9179b6cd69fb 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentServiceTagsTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentServiceTagsTests.cs @@ -638,6 +638,9 @@ public void Create_Tag_Data_Bulk_Publish_Operation() var dataType = DataTypeService.GetDataType(1041); dataType.Configuration = new TagConfiguration { Group = "test", StorageType = TagsStorageType.Csv }; + // updating the data type with the new configuration + DataTypeService.Save(dataType); + var template = TemplateBuilder.CreateTextPageTemplate(); FileService.SaveTemplate(template); @@ -822,6 +825,76 @@ public void Can_Remove_Tag_Data_To_Published_Content() } } + [Test] + public void Does_Not_Save_Multiple_Tags_As_One_When_CSV_Storage() + { + // Arrange + // set configuration + var dataType = DataTypeService.GetDataType(1041); + dataType.Configuration = new TagConfiguration { Group = "test", StorageType = TagsStorageType.Csv }; + + // updating the data type with the new configuration + DataTypeService.Save(dataType); + + var template = TemplateBuilder.CreateTextPageTemplate(); + FileService.SaveTemplate(template); + + var contentType = ContentTypeBuilder.CreateSimpleContentType("umbMandatory", "Mandatory Doc Type", + mandatoryProperties: true, defaultTemplateId: template.Id); + CreateAndAddTagsPropertyType(contentType); + + ContentTypeService.Save(contentType); + + IContent content = ContentBuilder.CreateSimpleContent(contentType, "Tagged content"); + content.AssignTags(PropertyEditorCollection, DataTypeService, Serializer, "tags", + new[] { "hello,world,tags", "new"}); + + ContentService.SaveAndPublish(content); + + // Act + content = ContentService.GetById(content.Id); + var savedTags = content.Properties["tags"].GetTagsValue(PropertyEditorCollection, DataTypeService, Serializer) + .ToArray(); + + // Assert + Assert.AreEqual(4, savedTags.Length); + } + + [Test] + public void Can_Save_Tag_With_Comma_Separated_Values_As_One_When_JSON_Storage() + { + // Arrange + // set configuration + var dataType = DataTypeService.GetDataType(1041); + dataType.Configuration = new TagConfiguration { Group = "test", StorageType = TagsStorageType.Json }; + + // updating the data type with the new configuration + DataTypeService.Save(dataType); + + var template = TemplateBuilder.CreateTextPageTemplate(); + FileService.SaveTemplate(template); + + var contentType = ContentTypeBuilder.CreateSimpleContentType("umbMandatory", "Mandatory Doc Type", + mandatoryProperties: true, defaultTemplateId: template.Id); + CreateAndAddTagsPropertyType(contentType); + + ContentTypeService.Save(contentType); + + IContent content = ContentBuilder.CreateSimpleContent(contentType, "Tagged content"); + content.AssignTags(PropertyEditorCollection, DataTypeService, Serializer, "tags", + new[] { "hello,world,tags", "new"}); + + ContentService.SaveAndPublish(content); + + // Act + content = ContentService.GetById(content.Id); + var savedTags = content.Properties["tags"].GetTagsValue(PropertyEditorCollection, DataTypeService, Serializer) + .ToArray(); + + // Assert + Assert.AreEqual(2, savedTags.Length); + } + private PropertyType CreateAndAddTagsPropertyType(ContentType contentType, ContentVariation variations = ContentVariation.Nothing) { From 2fa61fe811ef3fd1411cc96bb017588e4394fb44 Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Thu, 13 Oct 2022 10:29:50 +0200 Subject: [PATCH 5/5] Add data-element to umb property so we can find it (#13199) Co-authored-by: Zeegaan --- .../src/views/components/property/umb-property.html | 6 +++--- .../src/views/macros/views/settings.html | 1 + .../tests/DefaultConfig/Settings/macro.spec.ts | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/views/components/property/umb-property.html b/src/Umbraco.Web.UI.Client/src/views/components/property/umb-property.html index 8d0087b395f3..f00ba725b251 100644 --- a/src/Umbraco.Web.UI.Client/src/views/components/property/umb-property.html +++ b/src/Umbraco.Web.UI.Client/src/views/components/property/umb-property.html @@ -9,7 +9,7 @@
- + @@ -41,11 +41,11 @@ {{ vm.property.culture }} - + - + {{ vm.property.segment }} Default diff --git a/src/Umbraco.Web.UI.Client/src/views/macros/views/settings.html b/src/Umbraco.Web.UI.Client/src/views/macros/views/settings.html index 9b7906165497..0975da878ab8 100644 --- a/src/Umbraco.Web.UI.Client/src/views/macros/views/settings.html +++ b/src/Umbraco.Web.UI.Client/src/views/macros/views/settings.html @@ -21,6 +21,7 @@