From e2bcf59b0f2df1333630895079cd1f52d3893f80 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 17 Jul 2018 15:33:57 +1000 Subject: [PATCH] Fixes and documents the odd async usage for reading multipart in a model binder, fixes up navigation.service checks and adds unit tests for that --- .../src/common/services/navigation.service.js | 6 ++- .../services/navigation-service.spec.js | 50 ++++++++++++++++++ .../WebApi/Binders/ContentItemBaseBinder.cs | 52 +++++++++++++++---- 3 files changed, 95 insertions(+), 13 deletions(-) create mode 100644 src/Umbraco.Web.UI.Client/test/unit/common/services/navigation-service.spec.js diff --git a/src/Umbraco.Web.UI.Client/src/common/services/navigation.service.js b/src/Umbraco.Web.UI.Client/src/common/services/navigation.service.js index 275c9ca242..74e8f7d1a5 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/navigation.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/navigation.service.js @@ -153,9 +153,11 @@ function navigationService($rootScope, $route, $routeParams, $log, $location, $q //If any of the other parts have changed we do not cancel var currRoutingKeys = _.difference(_.keys(currUrlParams), nonRoutingQueryStrings); var nextRoutingKeys = _.difference(_.keys(nextUrlParams), nonRoutingQueryStrings); - var diff = _.difference(currRoutingKeys, nextRoutingKeys); + var diff1 = _.difference(currRoutingKeys, nextRoutingKeys); + var diff2 = _.difference(nextRoutingKeys, currRoutingKeys); + //if the routing parameter keys are the same, we'll compare their values to see if any have changed and if so then the routing will be allowed. - if (diff.length == 0) { + if (diff1.length === 0 && diff2.length === 0) { var partsChanged = 0; _.each(currRoutingKeys, function (k) { if (currUrlParams[k] != nextUrlParams[k]) { diff --git a/src/Umbraco.Web.UI.Client/test/unit/common/services/navigation-service.spec.js b/src/Umbraco.Web.UI.Client/test/unit/common/services/navigation-service.spec.js new file mode 100644 index 0000000000..b09d867d8d --- /dev/null +++ b/src/Umbraco.Web.UI.Client/test/unit/common/services/navigation-service.spec.js @@ -0,0 +1,50 @@ +describe('navigation services tests', function () { + var navigationService; + + beforeEach(module('umbraco.services')); + beforeEach(module('ngRoute')); + + beforeEach(inject(function ($injector) { + navigationService = $injector.get('navigationService'); + })); + + describe('determine if route change causes navigation', function () { + + it('navigates when parameters added', function () { + var currParams = { section: "content", id: 123 }; + var nextParams = { section: "content", id: 123, create: true }; + var result = navigationService.isRouteChangingNavigation(currParams, nextParams); + expect(result).toBe(true); + }); + + it('navigates when parameters removed', function () { + var currParams = { section: "content", id: 123, create: true }; + var nextParams = { section: "content", id: 123 }; + var result = navigationService.isRouteChangingNavigation(currParams, nextParams); + expect(result).toBe(true); + }); + + it('does not navigate when non routing parameters added', function () { + var currParams = { section: "content", id: 123 }; + var nextParams = { section: "content", id: 123, mculture: "abc", cculture: "xyz" }; + var result = navigationService.isRouteChangingNavigation(currParams, nextParams); + expect(result).toBe(false); + }); + + it('does not navigate when non routing parameters changed', function () { + var currParams = { section: "content", id: 123, mculture: "abc" }; + var nextParams = { section: "content", id: 123, mculture: "ooo", cculture: "xyz" }; + var result = navigationService.isRouteChangingNavigation(currParams, nextParams); + expect(result).toBe(false); + }); + + it('does not navigate when non routing parameters removed', function () { + var currParams = { section: "content", id: 123, mculture: "abc", cculture: "xyz" }; + var nextParams = { section: "content", id: 123, cculture: "xyz" }; + var result = navigationService.isRouteChangingNavigation(currParams, nextParams); + expect(result).toBe(false); + }); + + }); + +}); diff --git a/src/Umbraco.Web/WebApi/Binders/ContentItemBaseBinder.cs b/src/Umbraco.Web/WebApi/Binders/ContentItemBaseBinder.cs index 0607b0157e..f7af56d2b9 100644 --- a/src/Umbraco.Web/WebApi/Binders/ContentItemBaseBinder.cs +++ b/src/Umbraco.Web/WebApi/Binders/ContentItemBaseBinder.cs @@ -66,12 +66,9 @@ namespace Umbraco.Web.WebApi.Binders throw new HttpResponseException(HttpStatusCode.UnsupportedMediaType); } - var root = IOHelper.MapPath("~/App_Data/TEMP/FileUploads"); - //ensure it exists - Directory.CreateDirectory(root); - var provider = new MultipartFormDataStreamProvider(root); + var result = GetMultiPartResult(actionContext); - var model = GetModel(actionContext, bindingContext, provider); + var model = GetModel(actionContext, bindingContext, result); //now that everything is binded, validate the properties var contentItemValidator = GetValidationHelper(); contentItemValidator.ValidateItem(actionContext, model); @@ -85,6 +82,41 @@ namespace Umbraco.Web.WebApi.Binders return new ContentItemValidationHelper(Logger, UmbracoContextAccessor); } + private MultipartFormDataStreamProvider GetMultiPartResult(HttpActionContext actionContext) + { + var root = IOHelper.MapPath("~/App_Data/TEMP/FileUploads"); + //ensure it exists + Directory.CreateDirectory(root); + var provider = new MultipartFormDataStreamProvider(root); + + var request = actionContext.Request; + var content = request.Content; + + //NOTE: Yes this is super strange, ugly and weird. One would think that you could 'just' + // use a .Return on the ReadAsMultipartAsync but that absolutely doesn't work, it immediately deadlocks. + // We've tried all things like .Result, ConfigureAwait(false), GetAwaiter().GetResult(), a combination of + // everything and they all immediately deadlock. + // This issue has been documented in various .NET releases but there's really no info about the best way + // to do this. Ideally we'd use true async/await but in .NET Framework, model binders don't support async :( + // For some reason wrapping the operation in our own task works without deadlocking. + MultipartFormDataStreamProvider result = null; + var task = Task.Run(() => content.ReadAsMultipartAsync(provider)) + .ContinueWith(x => + { + if (x.IsFaulted && x.Exception != null) + { + throw x.Exception; + } + result = x.ConfigureAwait(false).GetAwaiter().GetResult(); + }); + task.Wait(); + + if (result == null) + throw new InvalidOperationException("Could not read multi-part message"); + + return result; + } + /// /// Builds the model from the request contents /// @@ -92,13 +124,11 @@ namespace Umbraco.Web.WebApi.Binders /// /// /// - private TModelSave GetModel(HttpActionContext actionContext, ModelBindingContext bindingContext, MultipartFormDataStreamProvider provider) + private TModelSave GetModel(HttpActionContext actionContext, ModelBindingContext bindingContext, MultipartFormDataStreamProvider result) { - var request = actionContext.Request; - - var content = request.Content; - - var result = content.ReadAsMultipartAsync(provider).Result; + //var request = actionContext.Request; + //var content = request.Content; + //var result = content.ReadAsMultipartAsync(provider).ConfigureAwait(false).GetAwaiter().GetResult(); if (result.FormData["contentItem"] == null) {