From 436be6ec3f83eb080566bad99655729fbc80f2b4 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 2 Oct 2025 13:03:00 +0200 Subject: [PATCH] Exception handling: Improve error messaging on invalid `umbraco-package.json` file (#20332) * Improve error messaging on invalid umbraco-package.json file. * Adjust failing unit tests --------- Co-authored-by: Laura Neto <12862535+lauraneto@users.noreply.github.com> --- .../HtmlHelperBackOfficeExtensions.cs | 33 +++++++------------ .../Manifest/PackageManifestReader.cs | 12 +++++-- .../Manifest/PackageManifestReaderTests.cs | 30 +++++++---------- 3 files changed, 32 insertions(+), 43 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Extensions/HtmlHelperBackOfficeExtensions.cs b/src/Umbraco.Cms.Api.Management/Extensions/HtmlHelperBackOfficeExtensions.cs index f501b7f312..963b176018 100644 --- a/src/Umbraco.Cms.Api.Management/Extensions/HtmlHelperBackOfficeExtensions.cs +++ b/src/Umbraco.Cms.Api.Management/Extensions/HtmlHelperBackOfficeExtensions.cs @@ -1,4 +1,4 @@ -using System.Text; +using System.Text; using Microsoft.AspNetCore.Html; using Microsoft.AspNetCore.Mvc.Rendering; using Umbraco.Cms.Core; @@ -24,29 +24,18 @@ public static class HtmlHelperBackOfficeExtensions IBackOfficePathGenerator backOfficePathGenerator, IPackageManifestService packageManifestService) { - try - { - PackageManifestImportmap packageImports = await packageManifestService.GetPackageManifestImportmapAsync(); + PackageManifestImportmap packageImports = await packageManifestService.GetPackageManifestImportmapAsync(); - var sb = new StringBuilder(); - sb.AppendLine(""""); + var sb = new StringBuilder(); + sb.AppendLine(""""); - // Inject the BackOffice cache buster into the import string to handle BackOffice assets - var importmapScript = sb.ToString() - .Replace(backOfficePathGenerator.BackOfficeVirtualDirectory, backOfficePathGenerator.BackOfficeAssetsPath) - .Replace(Constants.Web.CacheBusterToken, backOfficePathGenerator.BackOfficeCacheBustHash); + // Inject the BackOffice cache buster into the import string to handle BackOffice assets + var importmapScript = sb.ToString() + .Replace(backOfficePathGenerator.BackOfficeVirtualDirectory, backOfficePathGenerator.BackOfficeAssetsPath) + .Replace(Constants.Web.CacheBusterToken, backOfficePathGenerator.BackOfficeCacheBustHash); - return html.Raw(importmapScript); - } - catch (NotSupportedException ex) - { - throw new NotSupportedException("Failed to serialize the BackOffice import map", ex); - } - catch (Exception ex) - { - throw new Exception("Failed to generate the BackOffice import map", ex); - } + return html.Raw(importmapScript); } } diff --git a/src/Umbraco.Infrastructure/Manifest/PackageManifestReader.cs b/src/Umbraco.Infrastructure/Manifest/PackageManifestReader.cs index 7d4a502a35..c188694bfb 100644 --- a/src/Umbraco.Infrastructure/Manifest/PackageManifestReader.cs +++ b/src/Umbraco.Infrastructure/Manifest/PackageManifestReader.cs @@ -1,4 +1,5 @@ -using System.Text; +using System.Text; +using System.Text.Json; using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Logging; using Umbraco.Cms.Core.IO; @@ -92,10 +93,15 @@ internal class PackageManifestReader : IPackageManifestReader packageManifests.Add(packageManifest); } } + catch (JsonException ex) + { + throw new InvalidOperationException( + $"The package manifest file {fileInfo.PhysicalPath} could not be parsed as it does not contain valid JSON. Please see the inner exception for details.", ex); + } catch (Exception ex) { - _logger.LogError(ex, "Unable to load package manifest file: {FileName}", fileInfo.Name); - throw; + throw new InvalidOperationException( + $"The package manifest file {fileInfo.PhysicalPath} could not be parsed due to an unexpected error. Please see the inner exception for details.", ex); } } diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Manifest/PackageManifestReaderTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Manifest/PackageManifestReaderTests.cs index db23529c67..ff69213b3e 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Manifest/PackageManifestReaderTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Manifest/PackageManifestReaderTests.cs @@ -186,8 +186,9 @@ public class PackageManifestReaderTests .Setup(f => f.GetEnumerator()) .Returns(new List { CreatePackageManifestFile(content) }.GetEnumerator()); - Assert.ThrowsAsync(() => _reader.ReadPackageManifestsAsync()); - EnsureLogErrorWasCalled(); + var exception = Assert.ThrowsAsync(() => _reader.ReadPackageManifestsAsync()); + Assert.NotNull(exception); + Assert.IsInstanceOf(exception.InnerException); } [Test] @@ -202,8 +203,9 @@ public class PackageManifestReaderTests .Setup(f => f.GetEnumerator()) .Returns(new List { CreatePackageManifestFile(content) }.GetEnumerator()); - Assert.ThrowsAsync(() => _reader.ReadPackageManifestsAsync()); - EnsureLogErrorWasCalled(); + var exception = Assert.ThrowsAsync(() => _reader.ReadPackageManifestsAsync()); + Assert.NotNull(exception); + Assert.IsInstanceOf(exception.InnerException); } [Test] @@ -224,8 +226,9 @@ public class PackageManifestReaderTests .Setup(f => f.GetEnumerator()) .Returns(new List { CreatePackageManifestFile(content) }.GetEnumerator()); - Assert.ThrowsAsync(() => _reader.ReadPackageManifestsAsync()); - EnsureLogErrorWasCalled(); + var exception = Assert.ThrowsAsync(() => _reader.ReadPackageManifestsAsync()); + Assert.NotNull(exception); + Assert.IsInstanceOf(exception.InnerException); } [TestCase("This is not JSON")] @@ -236,20 +239,11 @@ public class PackageManifestReaderTests .Setup(f => f.GetEnumerator()) .Returns(new List { CreatePackageManifestFile(content) }.GetEnumerator()); - Assert.ThrowsAsync(() => _reader.ReadPackageManifestsAsync()); - EnsureLogErrorWasCalled(); + var exception = Assert.ThrowsAsync(() => _reader.ReadPackageManifestsAsync()); + Assert.NotNull(exception); + Assert.IsInstanceOf(exception.InnerException); } - private void EnsureLogErrorWasCalled(int numberOfTimes = 1) => - _loggerMock.Verify( - x => x.Log( - It.Is(l => l == LogLevel.Error), - It.IsAny(), - It.Is((v, t) => true), - It.IsAny(), - It.Is>((v, t) => true)), - Times.Exactly(numberOfTimes)); - private IFileInfo CreateDirectoryMock(string path, params IFileInfo[] children) { var directoryContentsMock = new Mock();