From bb3ac682411847bcba95b9b6279262574e425b03 Mon Sep 17 00:00:00 2001 From: Ronald Barendse Date: Fri, 31 May 2024 09:02:36 +0200 Subject: [PATCH] Include umbraco-package.json manifests in telemetry data (#16430) * Include umbraco-package.json manifests in package telemetry and add ID * Add package ID to ManifestResponseModel * Use async service method and HTTP client factory * Avoid breaking changes --- .../ManifestViewModelMapDefinition.cs | 3 +- .../Manifest/ManifestResponseModel.cs | 4 +- src/Umbraco.Core/Manifest/PackageManifest.cs | 4 +- .../Services/IPackagingService.cs | 10 ++ .../Telemetry/TelemetryService.cs | 10 +- .../BackgroundJobs/Jobs/ReportSiteJob.cs | 57 +++++---- .../Services/Implement/PackagingService.cs | 119 +++++++++++++++--- .../Telemetry/TelemetryServiceTests.cs | 3 +- 8 files changed, 156 insertions(+), 54 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Mapping/Manifest/ManifestViewModelMapDefinition.cs b/src/Umbraco.Cms.Api.Management/Mapping/Manifest/ManifestViewModelMapDefinition.cs index df18c71bdf..5dbf5c1835 100644 --- a/src/Umbraco.Cms.Api.Management/Mapping/Manifest/ManifestViewModelMapDefinition.cs +++ b/src/Umbraco.Cms.Api.Management/Mapping/Manifest/ManifestViewModelMapDefinition.cs @@ -1,4 +1,4 @@ -using Umbraco.Cms.Api.Management.ViewModels.Manifest; +using Umbraco.Cms.Api.Management.ViewModels.Manifest; using Umbraco.Cms.Core.Manifest; using Umbraco.Cms.Core.Mapping; @@ -13,6 +13,7 @@ public class ManifestViewModelMapDefinition : IMapDefinition private static void Map(PackageManifest source, ManifestResponseModel target, MapperContext context) { target.Name = source.Name; + target.Id = source.Id; target.Version = source.Version; target.Extensions = source.Extensions; } diff --git a/src/Umbraco.Cms.Api.Management/ViewModels/Manifest/ManifestResponseModel.cs b/src/Umbraco.Cms.Api.Management/ViewModels/Manifest/ManifestResponseModel.cs index 6e17fcedb2..beb72d605a 100644 --- a/src/Umbraco.Cms.Api.Management/ViewModels/Manifest/ManifestResponseModel.cs +++ b/src/Umbraco.Cms.Api.Management/ViewModels/Manifest/ManifestResponseModel.cs @@ -1,4 +1,4 @@ -using System.ComponentModel.DataAnnotations; +using System.ComponentModel.DataAnnotations; namespace Umbraco.Cms.Api.Management.ViewModels.Manifest; @@ -7,6 +7,8 @@ public class ManifestResponseModel [Required] public string Name { get; set; } = string.Empty; + public string? Id { get; set; } + public string? Version { get; set; } public object[] Extensions { get; set; } = Array.Empty(); diff --git a/src/Umbraco.Core/Manifest/PackageManifest.cs b/src/Umbraco.Core/Manifest/PackageManifest.cs index a38bd83290..8ff77729b0 100644 --- a/src/Umbraco.Core/Manifest/PackageManifest.cs +++ b/src/Umbraco.Core/Manifest/PackageManifest.cs @@ -1,9 +1,11 @@ -namespace Umbraco.Cms.Core.Manifest; +namespace Umbraco.Cms.Core.Manifest; public class PackageManifest { public required string Name { get; set; } + public string? Id { get; set; } + public string? Version { get; set; } public bool AllowPublicAccess { get; set; } diff --git a/src/Umbraco.Core/Services/IPackagingService.cs b/src/Umbraco.Core/Services/IPackagingService.cs index beec6325d8..e4f71a85ac 100644 --- a/src/Umbraco.Core/Services/IPackagingService.cs +++ b/src/Umbraco.Core/Services/IPackagingService.cs @@ -29,8 +29,18 @@ public interface IPackagingService : IService /// Returns the advertised installed packages /// /// + [Obsolete("Use GetAllInstalledPackagesAsync instead. Scheduled for removal in Umbraco 15.")] IEnumerable GetAllInstalledPackages(); + /// + /// Returns the advertised installed packages + /// + /// + Task> GetAllInstalledPackagesAsync() +#pragma warning disable CS0618 // Type or member is obsolete + => Task.FromResult(GetAllInstalledPackages()); +#pragma warning restore CS0618 // Type or member is obsolete + /// /// Returns installed packages collected from the package migration plans. /// diff --git a/src/Umbraco.Core/Telemetry/TelemetryService.cs b/src/Umbraco.Core/Telemetry/TelemetryService.cs index 5efc37c662..ee57c4deab 100644 --- a/src/Umbraco.Core/Telemetry/TelemetryService.cs +++ b/src/Umbraco.Core/Telemetry/TelemetryService.cs @@ -1,10 +1,7 @@ // Copyright (c) Umbraco. // See LICENSE for more details. -using Microsoft.Extensions.DependencyInjection; using Umbraco.Cms.Core.Configuration; -using Umbraco.Cms.Core.DependencyInjection; -using Umbraco.Cms.Core.Manifest; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Packaging; using Umbraco.Cms.Core.Services; @@ -43,6 +40,7 @@ internal class TelemetryService : ITelemetryService public bool TryGetTelemetryReportData(out TelemetryReportData? telemetryReportData) { telemetryReportData = GetTelemetryReportDataAsync().GetAwaiter().GetResult(); + return telemetryReportData != null; } @@ -58,7 +56,7 @@ internal class TelemetryService : ITelemetryService { Id = telemetryId, Version = GetVersion(), - Packages = await GetPackageTelemetryAsync(), + Packages = await GetPackageTelemetryAsync().ConfigureAwait(false), Detailed = _usageInformationService.GetDetailed(), }; } @@ -67,8 +65,6 @@ internal class TelemetryService : ITelemetryService ? null : _umbracoVersion.SemanticVersion.ToSemanticStringWithoutBuild(); - - private async Task?> GetPackageTelemetryAsync() { if (_metricsConsentService.GetConsentLevel() == TelemetryLevel.Minimal) @@ -77,7 +73,7 @@ internal class TelemetryService : ITelemetryService } List packages = new(); - IEnumerable installedPackages = _packagingService.GetAllInstalledPackages(); + IEnumerable installedPackages = await _packagingService.GetAllInstalledPackagesAsync().ConfigureAwait(false); foreach (InstalledPackage installedPackage in installedPackages) { diff --git a/src/Umbraco.Infrastructure/BackgroundJobs/Jobs/ReportSiteJob.cs b/src/Umbraco.Infrastructure/BackgroundJobs/Jobs/ReportSiteJob.cs index b3de6fa906..c04a876f6c 100644 --- a/src/Umbraco.Infrastructure/BackgroundJobs/Jobs/ReportSiteJob.cs +++ b/src/Umbraco.Infrastructure/BackgroundJobs/Jobs/ReportSiteJob.cs @@ -1,5 +1,7 @@ using System.Text; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Sync; using Umbraco.Cms.Core.Telemetry; @@ -9,7 +11,6 @@ namespace Umbraco.Cms.Infrastructure.BackgroundJobs.Jobs; public class ReportSiteJob : IRecurringBackgroundJob { - public TimeSpan Period => TimeSpan.FromDays(1); public TimeSpan Delay => TimeSpan.FromMinutes(5); @@ -19,25 +20,37 @@ public class ReportSiteJob : IRecurringBackgroundJob // No-op event as the period never changes on this job public event EventHandler PeriodChanged { - add { } remove { } + add { } + remove { } } - private static HttpClient _httpClient = new(); - private readonly ILogger _logger; - private readonly ITelemetryService _telemetryService; -private readonly IJsonSerializer _jsonSerializer; + private readonly IJsonSerializer _jsonSerializer; + private readonly IHttpClientFactory _httpClientFactory; + [Obsolete("Use the constructor with IHttpClientFactory instead.")] public ReportSiteJob( ILogger logger, ITelemetryService telemetryService, IJsonSerializer jsonSerializer) + : this( + logger, + telemetryService, + jsonSerializer, + StaticServiceProvider.Instance.GetRequiredService()) + { } + + public ReportSiteJob( + ILogger logger, + ITelemetryService telemetryService, + IJsonSerializer jsonSerializer, + IHttpClientFactory httpClientFactory) { _logger = logger; _telemetryService = telemetryService; _jsonSerializer = jsonSerializer; - _httpClient = new HttpClient(); + _httpClientFactory = httpClientFactory; } /// @@ -46,7 +59,8 @@ private readonly IJsonSerializer _jsonSerializer; /// public async Task RunJobAsync() { - if (_telemetryService.TryGetTelemetryReportData(out TelemetryReportData? telemetryReportData) is false) + TelemetryReportData? telemetryReportData = await _telemetryService.GetTelemetryReportDataAsync().ConfigureAwait(false); + if (telemetryReportData is null) { _logger.LogWarning("No telemetry marker found"); @@ -55,31 +69,28 @@ private readonly IJsonSerializer _jsonSerializer; try { - if (_httpClient.BaseAddress is null) + HttpClient httpClient = _httpClientFactory.CreateClient(); + if (httpClient.BaseAddress is null) { // Send data to LIVE telemetry - _httpClient.BaseAddress = new Uri("https://telemetry.umbraco.com/"); + httpClient.BaseAddress = new Uri("https://telemetry.umbraco.com/"); #if DEBUG // Send data to DEBUG telemetry service - _httpClient.BaseAddress = new Uri("https://telemetry.rainbowsrock.net/"); + httpClient.BaseAddress = new Uri("https://telemetry.rainbowsrock.net/"); #endif } - _httpClient.DefaultRequestHeaders.TryAddWithoutValidation("Content-Type", "application/json"); + httpClient.DefaultRequestHeaders.TryAddWithoutValidation("Content-Type", "application/json"); - using (var request = new HttpRequestMessage(HttpMethod.Post, "installs/")) - { - request.Content = new StringContent(_jsonSerializer.Serialize(telemetryReportData), Encoding.UTF8, - "application/json"); + using var request = new HttpRequestMessage(HttpMethod.Post, "installs/"); + request.Content = new StringContent(_jsonSerializer.Serialize(telemetryReportData), Encoding.UTF8, "application/json"); - // Make a HTTP Post to telemetry service - // https://telemetry.umbraco.com/installs/ - // Fire & Forget, do not need to know if its a 200, 500 etc - using (await _httpClient.SendAsync(request)) - { - } - } + // Make a HTTP Post to telemetry service + // https://telemetry.umbraco.com/installs/ + // Fire & Forget, do not need to know if its a 200, 500 etc + using (await httpClient.SendAsync(request)) + { } } catch { diff --git a/src/Umbraco.Infrastructure/Services/Implement/PackagingService.cs b/src/Umbraco.Infrastructure/Services/Implement/PackagingService.cs index ad8de35609..c4e152e0b5 100644 --- a/src/Umbraco.Infrastructure/Services/Implement/PackagingService.cs +++ b/src/Umbraco.Infrastructure/Services/Implement/PackagingService.cs @@ -1,7 +1,5 @@ using System.Xml.Linq; using Microsoft.Extensions.DependencyInjection; -using Umbraco.Cms.Core.DependencyInjection; -using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Hosting; using Umbraco.Cms.Core.DependencyInjection; @@ -14,6 +12,7 @@ using Umbraco.Cms.Core.Notifications; using Umbraco.Cms.Core.Packaging; using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Services.OperationStatus; +using Umbraco.Cms.Infrastructure.Manifest; using Umbraco.Extensions; using File = System.IO.File; @@ -31,10 +30,12 @@ public class PackagingService : IPackagingService private readonly IKeyValueService _keyValueService; private readonly IPackageInstallation _packageInstallation; private readonly PackageMigrationPlanCollection _packageMigrationPlans; + private readonly IPackageManifestReader _packageManifestReader; private readonly ICoreScopeProvider _coreScopeProvider; private readonly IHostEnvironment _hostEnvironment; private readonly IUserService _userService; + [Obsolete("Use the constructor with IPackageManifestReader instead.")] public PackagingService( IAuditService auditService, ICreatedPackagesRepository createdPackages, @@ -45,6 +46,30 @@ public class PackagingService : IPackagingService PackageMigrationPlanCollection packageMigrationPlans, IHostEnvironment hostEnvironment, IUserService userService) + : this( + auditService, + createdPackages, + packageInstallation, + eventAggregator, + keyValueService, + coreScopeProvider, + packageMigrationPlans, + StaticServiceProvider.Instance.GetRequiredService(), + hostEnvironment, + userService) + { } + + public PackagingService( + IAuditService auditService, + ICreatedPackagesRepository createdPackages, + IPackageInstallation packageInstallation, + IEventAggregator eventAggregator, + IKeyValueService keyValueService, + ICoreScopeProvider coreScopeProvider, + PackageMigrationPlanCollection packageMigrationPlans, + IPackageManifestReader packageManifestReader, + IHostEnvironment hostEnvironment, + IUserService userService) { _auditService = auditService; _createdPackages = createdPackages; @@ -52,6 +77,7 @@ public class PackagingService : IPackagingService _eventAggregator = eventAggregator; _keyValueService = keyValueService; _packageMigrationPlans = packageMigrationPlans; + _packageManifestReader = packageManifestReader; _coreScopeProvider = coreScopeProvider; _hostEnvironment = hostEnvironment; _userService = userService; @@ -61,9 +87,7 @@ public class PackagingService : IPackagingService public CompiledPackage GetCompiledPackageInfo(XDocument? xml) => _packageInstallation.ReadPackage(xml); - public InstallationSummary InstallCompiledPackageData( - XDocument? packageXml, - int userId = Constants.Security.SuperUserId) + public InstallationSummary InstallCompiledPackageData(XDocument? packageXml, int userId = Constants.Security.SuperUserId) { CompiledPackage compiledPackage = GetCompiledPackageInfo(packageXml); @@ -89,9 +113,7 @@ public class PackagingService : IPackagingService return summary; } - public InstallationSummary InstallCompiledPackageData( - FileInfo packageXmlFile, - int userId = Constants.Security.SuperUserId) + public InstallationSummary InstallCompiledPackageData(FileInfo packageXmlFile, int userId = Constants.Security.SuperUserId) { XDocument xml; using (StreamReader streamReader = File.OpenText(packageXmlFile.FullName)) @@ -109,10 +131,14 @@ public class PackagingService : IPackagingService [Obsolete("Use DeleteCreatedPackageAsync instead. Scheduled for removal in Umbraco 15.")] public void DeleteCreatedPackage(int id, int userId = Constants.Security.SuperUserId) { - using ICoreScope scope = _coreScopeProvider.CreateCoreScope(); - PackageDefinition? package = GetCreatedPackageById(id); - Guid key = package?.PackageId ?? Guid.Empty; - Guid currentUserKey = _userService.GetUserById(id)?.Key ?? Constants.Security.SuperUserKey; + Guid key, currentUserKey; + + using (ICoreScope scope = _coreScopeProvider.CreateCoreScope(autoComplete: true)) + { + PackageDefinition? package = GetCreatedPackageById(id); + key = package?.PackageId ?? Guid.Empty; + currentUserKey = _userService.GetUserById(id)?.Key ?? Constants.Security.SuperUserKey; + } DeleteCreatedPackageAsync(key, currentUserKey).GetAwaiter().GetResult(); } @@ -139,6 +165,7 @@ public class PackagingService : IPackagingService public IEnumerable GetAllCreatedPackages() { using ICoreScope scope = _coreScopeProvider.CreateCoreScope(autoComplete: true); + return _createdPackages.GetAll(); } @@ -146,6 +173,7 @@ public class PackagingService : IPackagingService public PackageDefinition? GetCreatedPackageById(int id) { using ICoreScope scope = _coreScopeProvider.CreateCoreScope(autoComplete: true); + return _createdPackages.GetById(id); } @@ -155,7 +183,8 @@ public class PackagingService : IPackagingService using ICoreScope scope = _coreScopeProvider.CreateCoreScope(autoComplete: true); PackageDefinition[] packages = _createdPackages.GetAll().WhereNotNull().ToArray(); var pagedModel = new PagedModel(packages.Length, packages.Skip(skip).Take(take)); - return await Task.FromResult(pagedModel); + + return pagedModel; } /// @@ -173,6 +202,7 @@ public class PackagingService : IPackagingService var success = _createdPackages.SavePackage(definition); scope.Complete(); + return success; } @@ -192,9 +222,10 @@ public class PackagingService : IPackagingService int currentUserId = (await _userService.GetRequiredUserAsync(userKey)).Id; _auditService.Add(AuditType.New, currentUserId, -1, "Package", $"Created package '{package.Name}' created. Package key: {package.PackageId}"); - scope.Complete(); - return await Task.FromResult(Attempt.SucceedWithStatus(PackageOperationStatus.Success, package)); + scope.Complete(); + + return Attempt.SucceedWithStatus(PackageOperationStatus.Success, package); } /// @@ -208,20 +239,27 @@ public class PackagingService : IPackagingService int currentUserId = (await _userService.GetRequiredUserAsync(userKey)).Id; _auditService.Add(AuditType.New, currentUserId, -1, "Package", $"Created package '{package.Name}' updated. Package key: {package.PackageId}"); + scope.Complete(); - return await Task.FromResult(Attempt.SucceedWithStatus(PackageOperationStatus.Success, package)); + + return Attempt.SucceedWithStatus(PackageOperationStatus.Success, package); } public string ExportCreatedPackage(PackageDefinition definition) { using ICoreScope scope = _coreScopeProvider.CreateCoreScope(autoComplete: true); + return _createdPackages.ExportPackage(definition); } public InstalledPackage? GetInstalledPackageByName(string packageName) => GetAllInstalledPackages().Where(x => x.PackageName?.InvariantEquals(packageName) ?? false).FirstOrDefault(); + [Obsolete("Use GetAllInstalledPackagesAsync instead. Scheduled for removal in Umbraco 15.")] public IEnumerable GetAllInstalledPackages() + => GetAllInstalledPackagesAsync().GetAwaiter().GetResult(); + + public async Task> GetAllInstalledPackagesAsync() { using ICoreScope scope = _coreScopeProvider.CreateCoreScope(autoComplete: true); @@ -277,14 +315,57 @@ public class PackagingService : IPackagingService installedPackage.PackageMigrationPlans = currentPlans; } - // Return all packages with an ID or name in the package.manifest or package migrations + // Collect and merge the packages from the manifests + foreach (PackageManifest packageManifest in await _packageManifestReader.ReadPackageManifestsAsync().ConfigureAwait(false)) + { + if (packageManifest.Id is null && string.IsNullOrEmpty(packageManifest.Name)) + { + continue; + } + + InstalledPackage installedPackage; + if (packageManifest.Id is not null && installedPackages.FirstOrDefault(x => x.PackageId == packageManifest.Id) is InstalledPackage installedPackageById) + { + installedPackage = installedPackageById; + + // Always use package name from manifest + installedPackage.PackageName = packageManifest.Name; + } + else if (installedPackages.FirstOrDefault(x => x.PackageName == packageManifest.Name) is InstalledPackage installedPackageByName) + { + installedPackage = installedPackageByName; + + // Ensure package ID is set + installedPackage.PackageId ??= packageManifest.Id; + } + else + { + installedPackage = new InstalledPackage + { + PackageId = packageManifest.Id, + PackageName = packageManifest.Name, + }; + + installedPackages.Add(installedPackage); + } + + // Set additional values + installedPackage.AllowPackageTelemetry = packageManifest.AllowTelemetry; + + if (!string.IsNullOrEmpty(packageManifest.Version)) + { + installedPackage.Version = packageManifest.Version; + } + } + + // Return all packages with an ID or name in the package manifest or package migrations return installedPackages; } #endregion /// - public async Task> GetInstalledPackagesFromMigrationPlansAsync(int skip, int take) + public Task> GetInstalledPackagesFromMigrationPlansAsync(int skip, int take) { IReadOnlyDictionary? keyValues = _keyValueService.FindByKeyPrefix(Constants.Conventions.Migrations.KeyValuePrefix); @@ -311,7 +392,7 @@ public class PackagingService : IPackagingService return package; }).ToArray(); - return await Task.FromResult(new PagedModel + return Task.FromResult(new PagedModel { Total = installedPackages.Count(), Items = installedPackages.Skip(skip).Take(take), diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/TelemetryServiceTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/TelemetryServiceTests.cs index f1cc43a7c0..36195a2f91 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/TelemetryServiceTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/TelemetryServiceTests.cs @@ -1,7 +1,6 @@ using Moq; using NUnit.Framework; using Umbraco.Cms.Core.Configuration; -using Umbraco.Cms.Core.Manifest; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Packaging; using Umbraco.Cms.Core.Semver; @@ -139,7 +138,7 @@ public class TelemetryServiceTests private IPackagingService CreatePackagingService(IEnumerable installedPackages) { var packagingServiceMock = new Mock(); - packagingServiceMock.Setup(x => x.GetAllInstalledPackages()).Returns(installedPackages); + packagingServiceMock.Setup(x => x.GetAllInstalledPackagesAsync()).ReturnsAsync(installedPackages); return packagingServiceMock.Object; }