From e54506ea9cf39b4a2cbf9de30d84b083ba8df1ab Mon Sep 17 00:00:00 2001 From: Jens Vandewalle Date: Wed, 12 Aug 2026 11:35:29 +0200 Subject: [PATCH 01/12] Refactor ResourceManagerTraceDataHandler implementation Replaced obsolete ResourceManagerTraceDataHandler in CoreJobHandler.cs with a new, robust version in its own file. The new handler translates core ResourceManager errors to DevPack job resource errors, mapping resource, capacity, and capability IDs to Resource Studio (DOM) equivalents. It handles specific error types and falls back to raw messages for unknown errors. Added deterministic, simulation-backed unit tests to verify translation logic and fallback behavior. --- .../ResourceManagerTraceDataHandlerTests.cs | 188 ++++++++++++ .../Handlers/Workflow/Jobs/CoreJobHandler.cs | 67 ----- .../Jobs/ResourceManagerTraceDataHandler.cs | 273 ++++++++++++++++++ 3 files changed, 461 insertions(+), 67 deletions(-) create mode 100644 DevPack.Tests/Workflow/Jobs/ResourceManagerTraceDataHandlerTests.cs create mode 100644 DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs diff --git a/DevPack.Tests/Workflow/Jobs/ResourceManagerTraceDataHandlerTests.cs b/DevPack.Tests/Workflow/Jobs/ResourceManagerTraceDataHandlerTests.cs new file mode 100644 index 00000000..46d9df5f --- /dev/null +++ b/DevPack.Tests/Workflow/Jobs/ResourceManagerTraceDataHandlerTests.cs @@ -0,0 +1,188 @@ +namespace RT_MediaOps.Plan.Workflow.Jobs +{ + using System; + using System.Collections.Generic; + using System.Linq; + + using Skyline.DataMiner.Net.Messages; + using Skyline.DataMiner.Net.ResourceManager.Helpers; + using Skyline.DataMiner.Net.ResponseErrorData; + using Skyline.DataMiner.Net.SRM.Capabilities; + using Skyline.DataMiner.Net.SRM.Capacities; + using Skyline.DataMiner.Net.SRM.Quarantine; + using Skyline.DataMiner.Solutions.MediaOps.Plan.API; + using Skyline.DataMiner.Solutions.MediaOps.Plan.Exceptions; + using Skyline.DataMiner.Solutions.MediaOps.Plan.UnitTesting.Simulation; + + using ResourcePool = Skyline.DataMiner.Solutions.MediaOps.Plan.API.ResourcePool; + using PlanResource = Skyline.DataMiner.Solutions.MediaOps.Plan.API.Resource; + + /// + /// Deterministic, simulation-backed tests for translating core ResourceManager errors into DevPack job resource errors. + /// A real Resource Studio resource is created so the handler can resolve the core resource id to its DOM counterpart. + /// + [TestClass] + public sealed class ResourceManagerTraceDataHandlerTests + { + private static (IMediaOpsPlanApi Api, PlanResource Resource) CreateContextWithResource() + { + var dms = MediaOpsPlanSimulation.Create(); + var connection = dms.CreateConnection(); + var api = connection.GetMediaOpsPlanApi(); + + var prefix = Guid.NewGuid(); + + var pool = api.ResourcePools.Create(new ResourcePool { Name = $"{prefix}_Pool" }); + pool = api.ResourcePools.Complete(pool); + + var resource = new UnmanagedResource { Name = $"{prefix}_Resource" }.AssignToPool(pool); + resource = api.Resources.Create(resource); + resource = api.Resources.Complete(resource); + + return (api, resource); + } + + [TestMethod] + public void Translate_QuarantineError_EmitsResourceNotAvailableWithDomResourceId() + { + var (api, resource) = CreateContextWithResource(); + var reservationId = Guid.NewGuid(); + + var error = new ResourceManagerErrorData( + ResourceManagerErrorData.Reason.ReservationUpdateCausedReservationsToGoToQuarantine, + reservationId, + (Guid?)null, + new List()) + { + MustBeMovedToQuarantine = new List + { + new QuarantinedUsagesOnSingleReservation + { + QuarantinedUsages = new List + { + new QuarantinedResourceUsageDefinition + { + QuarantinedResourceUsage = new ResourceUsageDefinition(resource.CoreResourceId), + }, + }, + }, + }, + }; + + var handler = new ResourceManagerTraceDataHandler((MediaOpsPlanApi)api); + + var result = handler.Translate(new[] { error }); + + Assert.IsTrue(result.ContainsKey(reservationId), "Expected the translated errors to be keyed by the reservation id."); + var notAvailable = result[reservationId].ErrorData.OfType().Single(); + Assert.AreEqual(resource.Id, notAvailable.ResourceId, "Expected the DOM resource id to be reported."); + } + + [TestMethod] + public void Translate_ResourceCapacityInvalid_EmitsInvalidCapacityWithIds() + { + var (api, resource) = CreateContextWithResource(); + var reservationId = Guid.NewGuid(); + var capacityId = Guid.NewGuid(); + + var error = new ResourceManagerErrorData( + ResourceManagerErrorData.Reason.ResourceCapacityInvalid, + reservationId, + resource.CoreResourceId, + new MultiResourceCapacityUsage { CapacityProfileID = capacityId }); + + var handler = new ResourceManagerTraceDataHandler((MediaOpsPlanApi)api); + + var result = handler.Translate(new[] { error }); + + Assert.IsTrue(result.ContainsKey(reservationId), "Expected the translated errors to be keyed by the reservation id."); + var capacityError = result[reservationId].ErrorData.OfType().Single(); + Assert.AreEqual(resource.Id, capacityError.ResourceId, "Expected the DOM resource id to be reported."); + Assert.AreEqual(capacityId, capacityError.CapacityId, "Expected the capacity profile id to be reported."); + } + + [TestMethod] + public void Translate_ResourceCapabilityInvalid_EmitsInvalidCapabilityWithIds() + { + var (api, resource) = CreateContextWithResource(); + var reservationId = Guid.NewGuid(); + var capabilityId = Guid.NewGuid(); + + var error = new ResourceManagerErrorData( + ResourceManagerErrorData.Reason.ResourceCapabilityInvalid, + reservationId, + resource.CoreResourceId, + new ResourceCapabilityUsage { CapabilityProfileID = capabilityId }, + "Capability not available."); + + var handler = new ResourceManagerTraceDataHandler((MediaOpsPlanApi)api); + + var result = handler.Translate(new[] { error }); + + Assert.IsTrue(result.ContainsKey(reservationId), "Expected the translated errors to be keyed by the reservation id."); + var capabilityError = result[reservationId].ErrorData.OfType().Single(); + Assert.AreEqual(resource.Id, capabilityError.ResourceId, "Expected the DOM resource id to be reported."); + Assert.AreEqual(capabilityId, capabilityError.CapabilityId, "Expected the capability profile id to be reported."); + } + + [TestMethod] + public void Translate_UncategorizedError_FallsBackToRawMessage() + { + var (api, _) = CreateContextWithResource(); + var reservationId = Guid.NewGuid(); + + var error = new ResourceManagerErrorData( + ResourceManagerErrorData.Reason.UnknownError, + reservationId, + (Guid?)null, + new List()); + + var handler = new ResourceManagerTraceDataHandler((MediaOpsPlanApi)api); + + var result = handler.Translate(new[] { error }); + + Assert.IsTrue(result.ContainsKey(reservationId), "Expected the raw fallback to be keyed by the reservation id."); + var errorData = result[reservationId].ErrorData.ToList(); + Assert.IsFalse( + errorData.OfType().Any(), + "Expected no typed job resource error for an uncategorized reason."); + Assert.IsTrue( + errorData.Any(x => x.GetType() == typeof(MediaOpsErrorData)), + "Expected a raw MediaOpsErrorData fallback for an uncategorized reason."); + } + + [TestMethod] + public void Translate_KnownAndUnknownErrors_MapsKnownAndAddsRawForUnknown() + { + var (api, resource) = CreateContextWithResource(); + var reservationId = Guid.NewGuid(); + var capacityId = Guid.NewGuid(); + + var capacityError = new ResourceManagerErrorData( + ResourceManagerErrorData.Reason.ResourceCapacityInvalid, + reservationId, + resource.CoreResourceId, + new MultiResourceCapacityUsage { CapacityProfileID = capacityId }); + + var unknownError = new ResourceManagerErrorData( + ResourceManagerErrorData.Reason.UnknownError, + reservationId, + (Guid?)null, + new List()); + + var handler = new ResourceManagerTraceDataHandler((MediaOpsPlanApi)api); + + var result = handler.Translate(new[] { capacityError, unknownError }); + + Assert.IsTrue(result.ContainsKey(reservationId), "Expected the translated errors to be keyed by the reservation id."); + var errorData = result[reservationId].ErrorData.ToList(); + Assert.AreEqual( + capacityId, + errorData.OfType().Single().CapacityId, + "Expected the known capacity error to be translated."); + Assert.IsTrue( + errorData.Any(x => x.GetType() == typeof(MediaOpsErrorData)), + "Expected the unknown error to be added as a raw default alongside the translated one."); + } + } +} diff --git a/DevPack/API/Handlers/Workflow/Jobs/CoreJobHandler.cs b/DevPack/API/Handlers/Workflow/Jobs/CoreJobHandler.cs index 5f5370dd..684f8494 100644 --- a/DevPack/API/Handlers/Workflow/Jobs/CoreJobHandler.cs +++ b/DevPack/API/Handlers/Workflow/Jobs/CoreJobHandler.cs @@ -10,7 +10,6 @@ using Skyline.DataMiner.Net.Jobs; using Skyline.DataMiner.Net.Messages.SLDataGateway; using Skyline.DataMiner.Net.ResourceManager.Objects; - using Skyline.DataMiner.Net.ResponseErrorData; using Skyline.DataMiner.Net.SRM.Capabilities; using Skyline.DataMiner.Net.SRM.Capacities; using Skyline.DataMiner.Solutions.MediaOps.Plan.Exceptions; @@ -1076,71 +1075,5 @@ private static CoreReservation BuildCoreReservation() }; } } - - private sealed class ResourceManagerTraceDataHandler : ITraceDataHandler - { - private readonly MediaOpsPlanApi planApi; - - private readonly Dictionary traceDataPerReservationId = new Dictionary(); - - public ResourceManagerTraceDataHandler(MediaOpsPlanApi planApi) - { - this.planApi = planApi ?? throw new ArgumentNullException(nameof(planApi)); - } - - public IReadOnlyDictionary Translate(ICollection resourceManagerErrors) - { - if (resourceManagerErrors == null) - { - throw new ArgumentNullException(nameof(resourceManagerErrors)); - } - - if (resourceManagerErrors.Count == 0) - { - return new Dictionary(); - } - - var reservationUpdateCausedReservationsToGoToQuarantineErrors = resourceManagerErrors.Where(x => x.ErrorReason == ResourceManagerErrorData.Reason.ReservationUpdateCausedReservationsToGoToQuarantine).ToList(); - var resourceCapacityInvalidErrors = resourceManagerErrors.Where(x => x.ErrorReason == ResourceManagerErrorData.Reason.ResourceCapacityInvalid).ToList(); - var resourceCapabilityInvalidErrors = resourceManagerErrors.Where(x => x.ErrorReason == ResourceManagerErrorData.Reason.ResourceCapabilityInvalid).ToList(); - if ((reservationUpdateCausedReservationsToGoToQuarantineErrors.Count + resourceCapacityInvalidErrors.Count + resourceCapabilityInvalidErrors.Count) != resourceManagerErrors.Count) - { - return ReturnDefaultTraceData(resourceManagerErrors); - } - - throw new NotImplementedException(); - } - - private MediaOpsTraceData GetOrCreateTraceData(Guid id) - { - if (!traceDataPerReservationId.TryGetValue(id, out var traceData)) - { - traceData = new MediaOpsTraceData(); - traceDataPerReservationId[id] = traceData; - } - - return traceData; - } - - private Dictionary ReturnDefaultTraceData(ICollection resourceManagerErrors) - { - foreach (var error in resourceManagerErrors) - { - if (error.SubjectId == Guid.Empty) - { - planApi.Logger.Error(this, $"Error with reason {error.ErrorReason} has empty SubjectId. This should not happen. Error message: {error.Message}"); - continue; - } - - var traceData = GetOrCreateTraceData(error.SubjectId.Value); - traceData.Add(new MediaOpsErrorData - { - ErrorMessage = error.ToString(), - }); - } - - return traceDataPerReservationId; - } - } } } diff --git a/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs b/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs new file mode 100644 index 00000000..7d42b459 --- /dev/null +++ b/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs @@ -0,0 +1,273 @@ +namespace Skyline.DataMiner.Solutions.MediaOps.Plan.API +{ + using System; + using System.Collections.Generic; + using System.Linq; + + using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; + using Skyline.DataMiner.Net.Messages.SLDataGateway; + using Skyline.DataMiner.Net.ResponseErrorData; + using Skyline.DataMiner.Solutions.MediaOps.Plan.Exceptions; + using Skyline.DataMiner.Solutions.MediaOps.Plan.Storage.DOM.SlcResource_Studio; + + // Translates core ResourceManager errors into DevPack job resource errors keyed per reservation (SubjectId). Only the + // resource, capacity and capability ids are surfaced; the consumer is responsible for resolving them to names. + internal sealed class ResourceManagerTraceDataHandler : ITraceDataHandler + { + private readonly MediaOpsPlanApi planApi; + + private readonly Dictionary traceDataPerReservationId = new Dictionary(); + + public ResourceManagerTraceDataHandler(MediaOpsPlanApi planApi) + { + this.planApi = planApi ?? throw new ArgumentNullException(nameof(planApi)); + } + + public IReadOnlyDictionary Translate(ICollection resourceManagerErrors) + { + if (resourceManagerErrors == null) + { + throw new ArgumentNullException(nameof(resourceManagerErrors)); + } + + if (resourceManagerErrors.Count == 0) + { + return new Dictionary(); + } + + var reservationUpdateCausedReservationsToGoToQuarantineErrors = resourceManagerErrors.Where(x => x.ErrorReason == ResourceManagerErrorData.Reason.ReservationUpdateCausedReservationsToGoToQuarantine).ToList(); + var resourceCapacityInvalidErrors = resourceManagerErrors.Where(x => x.ErrorReason == ResourceManagerErrorData.Reason.ResourceCapacityInvalid).ToList(); + var resourceCapabilityInvalidErrors = resourceManagerErrors.Where(x => x.ErrorReason == ResourceManagerErrorData.Reason.ResourceCapabilityInvalid).ToList(); + + // The DevPack surfaces resources by their Resource Studio (DOM) id, so the core resource ids in the errors are + // resolved to their DOM counterparts in a single query. + var domResourceIdByCoreId = BuildDomResourceIdByCoreId( + reservationUpdateCausedReservationsToGoToQuarantineErrors, + resourceCapacityInvalidErrors, + resourceCapabilityInvalidErrors); + + HandleReservationUpdateCausedReservationsToGoToQuarantine(reservationUpdateCausedReservationsToGoToQuarantineErrors, domResourceIdByCoreId); + HandleResourceCapacityInvalid(resourceCapacityInvalidErrors, domResourceIdByCoreId); + HandleResourceCapabilityInvalid(resourceCapabilityInvalidErrors, domResourceIdByCoreId); + + // Errors that are not one of the known types are added to their reservation's trace data as raw defaults. + AddDefaultTraceData(GetUnknownErrors(resourceManagerErrors)); + + return traceDataPerReservationId; + } + + private static List GetUnknownErrors(ICollection resourceManagerErrors) + { + return resourceManagerErrors + .Where(x => x.ErrorReason != ResourceManagerErrorData.Reason.ReservationUpdateCausedReservationsToGoToQuarantine + && x.ErrorReason != ResourceManagerErrorData.Reason.ResourceCapacityInvalid + && x.ErrorReason != ResourceManagerErrorData.Reason.ResourceCapabilityInvalid) + .ToList(); + } + + private Dictionary BuildDomResourceIdByCoreId( + IEnumerable quarantineErrors, + IEnumerable capacityErrors, + IEnumerable capabilityErrors) + { + var coreResourceIds = new HashSet(); + + foreach (var error in quarantineErrors) + { + foreach (var coreResourceId in GetQuarantinedCoreResourceIds(error)) + { + coreResourceIds.Add(coreResourceId); + } + } + + foreach (var error in capacityErrors.Concat(capabilityErrors)) + { + if (error.ResourceId.HasValue) + { + coreResourceIds.Add(error.ResourceId.Value); + } + } + + if (coreResourceIds.Count == 0) + { + return new Dictionary(); + } + + FilterElement Filter(Guid coreResourceId) => + DomInstanceExposers.DomDefinitionId.Equal(SlcResource_StudioIds.Definitions.Resource.Id) + .AND(DomInstanceExposers.FieldValues.DomInstanceField(SlcResource_StudioIds.Sections.ResourceInternalProperties.Resource_Id).Equal(coreResourceId)); + + var domResourceIdByCoreId = new Dictionary(); + foreach (var domResource in planApi.DomHelpers.SlcResourceStudioHelper.GetResources(coreResourceIds, Filter)) + { + var coreId = domResource.ResourceInternalProperties.Resource_Id.GetValueOrDefault(); + if (coreId == Guid.Empty) + { + continue; + } + + domResourceIdByCoreId[coreId] = domResource.ID.Id; + } + + return domResourceIdByCoreId; + } + + private void HandleReservationUpdateCausedReservationsToGoToQuarantine( + IReadOnlyCollection quarantineErrors, + IReadOnlyDictionary domResourceIdByCoreId) + { + foreach (var error in quarantineErrors) + { + if (!TryGetReservationId(error, out var reservationId)) + { + continue; + } + + var traceData = GetOrCreateTraceData(reservationId); + + var emittedAny = false; + foreach (var coreResourceId in GetQuarantinedCoreResourceIds(error).Distinct()) + { + if (!domResourceIdByCoreId.TryGetValue(coreResourceId, out var domResourceId)) + { + planApi.Logger.Error(this, $"Could not resolve a Resource Studio resource for core resource {coreResourceId}."); + continue; + } + + traceData.Add(new JobResourceNotAvailableError + { + ErrorMessage = error.Message, + ResourceId = domResourceId, + }); + emittedAny = true; + } + + if (!emittedAny) + { + AddRawFallback(traceData, error); + } + } + } + + private void HandleResourceCapacityInvalid( + IReadOnlyCollection capacityErrors, + IReadOnlyDictionary domResourceIdByCoreId) + { + foreach (var error in capacityErrors) + { + if (!TryGetReservationId(error, out var reservationId)) + { + continue; + } + + var traceData = GetOrCreateTraceData(reservationId); + + if (!TryGetDomResourceId(error, domResourceIdByCoreId, out var domResourceId)) + { + AddRawFallback(traceData, error); + continue; + } + + traceData.Add(new JobResourceInvalidCapacityError + { + ErrorMessage = error.Message, + ResourceId = domResourceId, + CapacityId = error.ResourceCapacityUsage.CapacityProfileID, + }); + } + } + + private void HandleResourceCapabilityInvalid( + IReadOnlyCollection capabilityErrors, + IReadOnlyDictionary domResourceIdByCoreId) + { + foreach (var error in capabilityErrors) + { + if (!TryGetReservationId(error, out var reservationId)) + { + continue; + } + + var traceData = GetOrCreateTraceData(reservationId); + + if (!TryGetDomResourceId(error, domResourceIdByCoreId, out var domResourceId)) + { + AddRawFallback(traceData, error); + continue; + } + + traceData.Add(new JobResourceInvalidCapabilityError + { + ErrorMessage = error.Message, + ResourceId = domResourceId, + CapabilityId = error.ResourceCapabilityUsage.CapabilityProfileID, + }); + } + } + + private static IEnumerable GetQuarantinedCoreResourceIds(ResourceManagerErrorData error) + { + return error.MustBeMovedToQuarantine + .SelectMany(x => x.QuarantinedUsages) + .Select(x => x.QuarantinedResourceUsage.GUID); + } + + private bool TryGetDomResourceId(ResourceManagerErrorData error, IReadOnlyDictionary domResourceIdByCoreId, out Guid domResourceId) + { + domResourceId = Guid.Empty; + + if (!error.ResourceId.HasValue || !domResourceIdByCoreId.TryGetValue(error.ResourceId.Value, out domResourceId)) + { + planApi.Logger.Error(this, $"Could not resolve a Resource Studio resource for core resource {error.ResourceId}."); + return false; + } + + return true; + } + + private bool TryGetReservationId(ResourceManagerErrorData error, out Guid reservationId) + { + reservationId = error.SubjectId.GetValueOrDefault(); + if (reservationId == Guid.Empty) + { + planApi.Logger.Error(this, $"Error with reason {error.ErrorReason} has empty SubjectId. This should not happen. Error message: {error.Message}"); + return false; + } + + return true; + } + + private void AddRawFallback(MediaOpsTraceData traceData, ResourceManagerErrorData error) + { + planApi.Logger.Error(this, $"Falling back to the raw error message for an error with reason {error.ErrorReason}."); + traceData.Add(new MediaOpsErrorData { ErrorMessage = error.ToString() }); + } + + private MediaOpsTraceData GetOrCreateTraceData(Guid id) + { + if (!traceDataPerReservationId.TryGetValue(id, out var traceData)) + { + traceData = new MediaOpsTraceData(); + traceDataPerReservationId[id] = traceData; + } + + return traceData; + } + + private void AddDefaultTraceData(ICollection resourceManagerErrors) + { + foreach (var error in resourceManagerErrors) + { + if (!TryGetReservationId(error, out var reservationId)) + { + continue; + } + + GetOrCreateTraceData(reservationId).Add(new MediaOpsErrorData + { + ErrorMessage = error.ToString(), + }); + } + } + } +} From abafb7f34dbaf3de94d6033b46b25a70bf90a899 Mon Sep 17 00:00:00 2001 From: Jens Vandewalle <102030104+JensVandewalle@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:37:29 +0200 Subject: [PATCH 02/12] Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../ResourceManagerTraceDataHandlerTests.cs | 27 +++++++++++++++++++ .../Jobs/ResourceManagerTraceDataHandler.cs | 9 ++++--- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/DevPack.Tests/Workflow/Jobs/ResourceManagerTraceDataHandlerTests.cs b/DevPack.Tests/Workflow/Jobs/ResourceManagerTraceDataHandlerTests.cs index 46d9df5f..de62bddb 100644 --- a/DevPack.Tests/Workflow/Jobs/ResourceManagerTraceDataHandlerTests.cs +++ b/DevPack.Tests/Workflow/Jobs/ResourceManagerTraceDataHandlerTests.cs @@ -184,5 +184,32 @@ public void Translate_KnownAndUnknownErrors_MapsKnownAndAddsRawForUnknown() errorData.Any(x => x.GetType() == typeof(MediaOpsErrorData)), "Expected the unknown error to be added as a raw default alongside the translated one."); } + [TestMethod] + public void Translate_WhenCalledMultipleTimes_DoesNotReturnPreviousResults() + { + var (api, resource) = CreateContextWithResource(); + var handler = new ResourceManagerTraceDataHandler((MediaOpsPlanApi)api); + + var reservationId1 = Guid.NewGuid(); + var reservationId2 = Guid.NewGuid(); + + var first = new ResourceManagerErrorData( + ResourceManagerErrorData.Reason.ResourceCapacityInvalid, + reservationId1, + resource.CoreResourceId, + new MultiResourceCapacityUsage { CapacityProfileID = Guid.NewGuid() }); + + var second = new ResourceManagerErrorData( + ResourceManagerErrorData.Reason.UnknownError, + reservationId2, + (Guid?)null, + new List()); + + Assert.IsTrue(handler.Translate(new[] { first }).ContainsKey(reservationId1)); + + var secondResult = handler.Translate(new[] { second }); + Assert.IsFalse(secondResult.ContainsKey(reservationId1), "Expected previous translation output not to leak into subsequent calls."); + Assert.IsTrue(secondResult.ContainsKey(reservationId2)); + } } } diff --git a/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs b/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs index 7d42b459..a959ee5f 100644 --- a/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs +++ b/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs @@ -30,9 +30,11 @@ public IReadOnlyDictionary Translate(ICollection(); + return traceDataPerReservationId; } var reservationUpdateCausedReservationsToGoToQuarantineErrors = resourceManagerErrors.Where(x => x.ErrorReason == ResourceManagerErrorData.Reason.ReservationUpdateCausedReservationsToGoToQuarantine).ToList(); @@ -40,8 +42,7 @@ public IReadOnlyDictionary Translate(ICollection x.ErrorReason == ResourceManagerErrorData.Reason.ResourceCapabilityInvalid).ToList(); // The DevPack surfaces resources by their Resource Studio (DOM) id, so the core resource ids in the errors are - // resolved to their DOM counterparts in a single query. - var domResourceIdByCoreId = BuildDomResourceIdByCoreId( + // resolved to their DOM counterparts in a batched query (one or more backend calls, depending on filter size). reservationUpdateCausedReservationsToGoToQuarantineErrors, resourceCapacityInvalidErrors, resourceCapabilityInvalidErrors); @@ -239,7 +240,7 @@ private bool TryGetReservationId(ResourceManagerErrorData error, out Guid reserv private void AddRawFallback(MediaOpsTraceData traceData, ResourceManagerErrorData error) { - planApi.Logger.Error(this, $"Falling back to the raw error message for an error with reason {error.ErrorReason}."); + planApi.Logger.Error(this, $"Falling back to the raw error data (ToString) for an error with reason {error.ErrorReason}."); traceData.Add(new MediaOpsErrorData { ErrorMessage = error.ToString() }); } From 70511c246277132227918f2a648258e0b3981c6e Mon Sep 17 00:00:00 2001 From: Jens Vandewalle Date: Wed, 12 Aug 2026 15:39:12 +0200 Subject: [PATCH 03/12] Add file property support to MediaOps Plan API Added FilePropertySetting for file value management, including attachment handling via IPropertyAttachmentStore. Updated property collections for file support, serialization, and validation (size, count, type). Exposed configurable max document size. Added tests for file properties and error cases. Refactored persistence and cleanup logic for attachments. --- .../Values/FilePropertySettingTests.cs | 156 +++++++++ .../Values/FilePropertySimulationTests.cs | 307 ++++++++++++++++++ .../Simulation/SimulatedDms.cs | 12 + .../Handlers/Properties/DomPropertyHandler.cs | 50 ++- .../DomPropertySettingCollectionHandler.cs | 78 +++++ DevPack/API/MediaOpsPlanApi.cs | 9 + .../Properties/Definitions/FileProperty.cs | 4 +- .../Internal/InnerFilePropertySetting.cs | 67 ++++ .../Values/Public/FilePropertySetting.cs | 243 ++++++++++++++ .../Public/PropertySettingCollection.cs | 35 +- .../Validators/PropertySettingValidator.cs | 39 +++ .../PropertyInvalidFileSizeLimitError.cs | 13 + .../DOM/Helpers/IPropertyAttachmentStore.cs | 18 + .../DOM/Helpers/PropertyAttachmentStore.cs | 37 +++ 14 files changed, 1063 insertions(+), 5 deletions(-) create mode 100644 DevPack.Tests/Properties/Values/FilePropertySettingTests.cs create mode 100644 DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs create mode 100644 DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs create mode 100644 DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs create mode 100644 DevPack/Exceptions/TraceData/Properties/Definitions/PropertyInvalidFileSizeLimitError.cs create mode 100644 DevPack/Storage/DOM/Helpers/IPropertyAttachmentStore.cs create mode 100644 DevPack/Storage/DOM/Helpers/PropertyAttachmentStore.cs diff --git a/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs b/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs new file mode 100644 index 00000000..14083670 --- /dev/null +++ b/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs @@ -0,0 +1,156 @@ +namespace RT_MediaOps.Plan.Properties.Values +{ + using System; + using System.Linq; + using System.Text; + + using Skyline.DataMiner.Solutions.MediaOps.Plan.API; + + [TestClass] + public sealed class FilePropertySettingTests + { + private static byte[] Content(string value) => Encoding.UTF8.GetBytes(value); + + [TestMethod] + public void Constructor_SetsPropertyId() + { + var id = Guid.NewGuid(); + + var setting = new FilePropertySetting(new FileProperty(id)); + + Assert.AreEqual(id, setting.Id); + } + + [TestMethod] + public void Constructor_NullProperty_Throws() + { + Assert.ThrowsException(() => new FilePropertySetting((FileProperty)null!)); + } + + [TestMethod] + public void NewSetting_HasNoValue() + { + var setting = new FilePropertySetting(new FileProperty()); + + Assert.IsFalse(setting.HasValue); + Assert.AreEqual(0, setting.Files.Count); + } + + [TestMethod] + public void AddFile_FileIsTrackedForUpload() + { + var setting = new FilePropertySetting(new FileProperty()); + + setting.AddFile("document.pdf", Content("abc")); + + Assert.IsTrue(setting.HasValue); + CollectionAssert.AreEqual(new[] { "document.pdf" }, setting.Files.ToArray()); + } + + [TestMethod] + public void AddFile_StripsDirectoryInformation() + { + var setting = new FilePropertySetting(new FileProperty()); + + setting.AddFile(@"C:\temp\..\document.pdf", Content("abc")); + + CollectionAssert.AreEqual(new[] { "document.pdf" }, setting.Files.ToArray()); + } + + [TestMethod] + public void AddFile_SameNameTwice_KeepsSingleEntry() + { + var setting = new FilePropertySetting(new FileProperty()); + + setting.AddFile("document.pdf", Content("first")); + setting.AddFile("document.pdf", Content("second")); + + Assert.AreEqual(1, setting.Files.Count); + } + + [TestMethod] + public void AddFile_NullContent_Throws() + { + var setting = new FilePropertySetting(new FileProperty()); + + Assert.ThrowsException(() => setting.AddFile("document.pdf", null!)); + } + + [TestMethod] + public void AddFile_EmptyName_Throws() + { + var setting = new FilePropertySetting(new FileProperty()); + + Assert.ThrowsException(() => setting.AddFile(" ", Content("abc"))); + } + + [TestMethod] + public void RemoveFile_RemovesFile() + { + var setting = new FilePropertySetting(new FileProperty()); + setting.AddFile("document.pdf", Content("abc")); + + setting.RemoveFile("document.pdf"); + + Assert.IsFalse(setting.HasValue); + Assert.AreEqual(0, setting.Files.Count); + } + + [TestMethod] + public void RemoveFile_UnknownFile_DoesNothing() + { + var setting = new FilePropertySetting(new FileProperty()); + setting.AddFile("document.pdf", Content("abc")); + + setting.RemoveFile("other.pdf"); + + CollectionAssert.AreEqual(new[] { "document.pdf" }, setting.Files.ToArray()); + } + + [TestMethod] + public void ClearFiles_RemovesAllFiles() + { + var setting = new FilePropertySetting(new FileProperty()); + setting.AddFile("first.pdf", Content("abc")); + setting.AddFile("second.pdf", Content("def")); + + setting.ClearFiles(); + + Assert.AreEqual(0, setting.Files.Count); + } + + [TestMethod] + public void Equals_SameFiles_ReturnsTrue() + { + var property = new FileProperty(Guid.NewGuid()); + + var first = new FilePropertySetting(property).AddFile("a.pdf", Content("a")); + var second = new FilePropertySetting(property).AddFile("a.pdf", Content("a")); + + Assert.AreEqual(first, second); + Assert.AreEqual(first.GetHashCode(), second.GetHashCode()); + } + + [TestMethod] + public void Equals_DifferentFiles_ReturnsFalse() + { + var property = new FileProperty(Guid.NewGuid()); + + var first = new FilePropertySetting(property).AddFile("a.pdf", Content("a")); + var second = new FilePropertySetting(property).AddFile("b.pdf", Content("a")); + + Assert.AreNotEqual(first, second); + } + + [TestMethod] + public void Equals_IgnoresFileOrder() + { + var property = new FileProperty(Guid.NewGuid()); + + var first = new FilePropertySetting(property).AddFile("a.pdf", Content("a")).AddFile("b.pdf", Content("b")); + var second = new FilePropertySetting(property).AddFile("b.pdf", Content("b")).AddFile("a.pdf", Content("a")); + + Assert.AreEqual(first, second); + } + } +} diff --git a/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs b/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs new file mode 100644 index 00000000..64770d74 --- /dev/null +++ b/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs @@ -0,0 +1,307 @@ +namespace RT_MediaOps.Plan.Properties.Values +{ + using System; + using System.Collections.Generic; + using System.Linq; + using System.Text; + + using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; + using Skyline.DataMiner.Solutions.MediaOps.Plan.API; + using Skyline.DataMiner.Solutions.MediaOps.Plan.Exceptions; + using Skyline.DataMiner.Solutions.MediaOps.Plan.Storage.DOM; + using Skyline.DataMiner.Solutions.MediaOps.Plan.UnitTesting.Simulation; + + /// + /// Deterministic, simulation-backed tests for file property values. The file content is stored through the attachment + /// API, which cannot be simulated, so a fake attachment store is used to verify what is uploaded and deleted. + /// + [TestClass] + public sealed class FilePropertySimulationTests + { + private const string Scope = "global"; + + private static byte[] Content(string value) => Encoding.UTF8.GetBytes(value); + + private static (IMediaOpsPlanApi Api, FakePropertyAttachmentStore Attachments) CreateContext() + { + var dms = MediaOpsPlanSimulation.Create(); + var connection = dms.CreateConnection(); + var api = connection.GetMediaOpsPlanApi(); + + var attachments = new FakePropertyAttachmentStore(); + ((MediaOpsPlanApi)api).PropertyAttachments = attachments; + + return (api, attachments); + } + + private static FileProperty CreateFileProperty(IMediaOpsPlanApi api, bool allowMultiple = false, bool hasSizeLimit = false, long sizeLimit = 20) + { + var property = new FileProperty(new PropertyData { Scope = Scope }) + { + Name = $"{Guid.NewGuid()}_Prop", + SectionName = "General", + AllowMultiple = allowMultiple, + HasSizeLimit = hasSizeLimit, + SizeLimit = sizeLimit, + }; + + return (FileProperty)api.Properties.Create(property); + } + + private static PropertySettingCollection CreateCollection() + { + return new PropertySettingCollection(new PropertySettingCollectionData + { + LinkedObjectId = $"obj-{Guid.NewGuid()}", + Scope = Scope, + SubId = string.Empty, + }); + } + + [TestMethod] + public void Create_WithFile_UploadsAttachmentAndStoresFileName() + { + var (api, attachments) = CreateContext(); + var property = CreateFileProperty(api); + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); + + var created = api.PropertySettingCollections.Create(collection); + + Assert.AreEqual(1, created.FileSettings.Count, "Expected the file setting to be stored."); + CollectionAssert.AreEqual(new[] { "document.pdf" }, created.FileSettings.Single().Files.ToArray()); + + var attachmentName = $"{property.Id}_document.pdf"; + Assert.IsTrue(attachments.Contains(collection.Id, attachmentName), "Expected the file content to be uploaded as an attachment."); + CollectionAssert.AreEqual(Content("hello"), attachments.Get(new DomInstanceId(collection.Id), attachmentName)); + } + + [TestMethod] + public void Read_AfterCreate_ReturnsFileNameWithoutAttachmentPrefix() + { + var (api, _) = CreateContext(); + var property = CreateFileProperty(api); + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); + api.PropertySettingCollections.Create(collection); + + var read = api.PropertySettingCollections.Read(collection.Id); + + Assert.IsNotNull(read); + CollectionAssert.AreEqual(new[] { "document.pdf" }, read.FileSettings.Single().Files.ToArray()); + } + + [TestMethod] + public void ReadContent_ReturnsUploadedContent() + { + var (api, _) = CreateContext(); + var property = CreateFileProperty(api); + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); + api.PropertySettingCollections.Create(collection); + + var read = api.PropertySettingCollections.Read(collection.Id); + var setting = read.FileSettings.Single(); + + CollectionAssert.AreEqual(Content("hello"), setting.ReadContent("document.pdf")); + } + + [TestMethod] + public void ReadContent_BeforeSaving_ReturnsPendingContent() + { + var (api, _) = CreateContext(); + var property = CreateFileProperty(api); + + var setting = new FilePropertySetting(property).AddFile("document.pdf", Content("hello")); + + CollectionAssert.AreEqual(Content("hello"), setting.ReadContent("document.pdf"), "Expected content that is not stored yet to be returned from memory."); + } + + [TestMethod] + public void ReadContent_UnknownFile_Throws() + { + var (api, _) = CreateContext(); + var property = CreateFileProperty(api); + + var setting = new FilePropertySetting(property).AddFile("document.pdf", Content("hello")); + + Assert.ThrowsException(() => setting.ReadContent("other.pdf")); + } + + [TestMethod] + public void ReadContent_OnReturnedCollection_UsesStoredContent() + { + var (api, attachments) = CreateContext(); + var property = CreateFileProperty(api); + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); + + var created = api.PropertySettingCollections.Create(collection); + + // The pending content is cleared once it is stored, so this has to come from the attachment. + CollectionAssert.AreEqual(Content("hello"), created.FileSettings.Single().ReadContent("document.pdf")); + Assert.IsTrue(attachments.Contains(collection.Id, $"{property.Id}_document.pdf")); + } + + [TestMethod] + public void Update_RemovingFile_DeletesAttachment() + { + var (api, attachments) = CreateContext(); + var property = CreateFileProperty(api); + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); + api.PropertySettingCollections.Create(collection); + + var read = api.PropertySettingCollections.Read(collection.Id); + read.FileSettings.Single().RemoveFile("document.pdf"); + var updated = api.PropertySettingCollections.Update(read); + + Assert.AreEqual(0, updated.FileSettings.Single().Files.Count, "Expected the file to be removed from the property."); + Assert.IsFalse(attachments.Contains(collection.Id, $"{property.Id}_document.pdf"), "Expected the attachment to be deleted."); + } + + [TestMethod] + public void Delete_RemovesAttachments() + { + var (api, attachments) = CreateContext(); + var property = CreateFileProperty(api); + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); + api.PropertySettingCollections.Create(collection); + + var read = api.PropertySettingCollections.Read(collection.Id); + api.PropertySettingCollections.Delete(read); + + Assert.IsFalse(attachments.Contains(collection.Id, $"{property.Id}_document.pdf"), "Expected the attachments to be removed together with the collection."); + } + + [TestMethod] + public void Create_MultipleFilesWhileNotAllowed_ThrowsInvalidPropertySettingsError() + { + var (api, _) = CreateContext(); + var property = CreateFileProperty(api, allowMultiple: false); + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property) + .AddFile("first.pdf", Content("a")) + .AddFile("second.pdf", Content("b"))); + + var exception = Assert.ThrowsException(() => api.PropertySettingCollections.Create(collection)); + Assert.IsTrue( + exception.TraceData.ErrorData.OfType().Any(), + "Expected an invalid property settings error when multiple files are not allowed."); + } + + [TestMethod] + public void Create_FileExceedingSizeLimit_ThrowsInvalidPropertySettingsError() + { + var (api, _) = CreateContext(); + var property = CreateFileProperty(api, hasSizeLimit: true, sizeLimit: 1); + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property).AddFile("big.bin", new byte[2 * 1024 * 1024])); + + var exception = Assert.ThrowsException(() => api.PropertySettingCollections.Create(collection)); + Assert.IsTrue( + exception.TraceData.ErrorData.OfType().Any(), + "Expected an invalid property settings error when a file exceeds the size limit."); + } + + [TestMethod] + public void CreateProperty_SizeLimitBelowOne_ThrowsInvalidFileSizeLimitError() + { + var (api, _) = CreateContext(); + + var property = new FileProperty(new PropertyData { Scope = Scope }) + { + Name = $"{Guid.NewGuid()}_Prop", + SectionName = "General", + HasSizeLimit = true, + SizeLimit = 0, + }; + + var exception = Assert.ThrowsException(() => api.Properties.Create(property)); + Assert.IsTrue( + exception.TraceData.ErrorData.OfType().Any(), + "Expected a PropertyInvalidFileSizeLimitError when the size limit is not positive."); + } + + [TestMethod] + public void CreateProperty_SizeLimitAboveServerMaximum_ThrowsInvalidFileSizeLimitError() + { + var (api, _) = CreateContext(); + + var property = new FileProperty(new PropertyData { Scope = Scope }) + { + Name = $"{Guid.NewGuid()}_Prop", + SectionName = "General", + HasSizeLimit = true, + SizeLimit = 10000, + }; + + var exception = Assert.ThrowsException(() => api.Properties.Create(property)); + Assert.IsTrue( + exception.TraceData.ErrorData.OfType().Any(), + "Expected a PropertyInvalidFileSizeLimitError when the size limit exceeds the server maximum."); + } + + [TestMethod] + public void CreateProperty_WithoutSizeLimit_RoundTripsHasSizeLimit() + { + var (api, _) = CreateContext(); + + var property = CreateFileProperty(api, hasSizeLimit: false); + + var read = (FileProperty)api.Properties.Read(property.Id); + + Assert.IsFalse(read.HasSizeLimit, "Expected a property without its own size limit to keep using the server limit."); + } + + private sealed class FakePropertyAttachmentStore : IPropertyAttachmentStore + { + private readonly Dictionary> attachments = new Dictionary>(); + + public void Add(DomInstanceId instanceId, string attachmentName, byte[] content) + { + if (!attachments.TryGetValue(instanceId.Id, out var perInstance)) + { + perInstance = new Dictionary(StringComparer.OrdinalIgnoreCase); + attachments[instanceId.Id] = perInstance; + } + + perInstance[attachmentName] = content; + } + + public byte[] Get(DomInstanceId instanceId, string attachmentName) + { + return attachments.TryGetValue(instanceId.Id, out var perInstance) && perInstance.TryGetValue(attachmentName, out var content) + ? content + : throw new InvalidOperationException($"Attachment '{attachmentName}' was not found."); + } + + public void Delete(DomInstanceId instanceId, string attachmentName) + { + if (attachments.TryGetValue(instanceId.Id, out var perInstance)) + { + perInstance.Remove(attachmentName); + } + } + + public IReadOnlyCollection GetNames(DomInstanceId instanceId) + { + return attachments.TryGetValue(instanceId.Id, out var perInstance) ? perInstance.Keys.ToList() : new List(); + } + + public bool Contains(Guid instanceId, string attachmentName) + { + return attachments.TryGetValue(instanceId, out var perInstance) && perInstance.ContainsKey(attachmentName); + } + } + } +} diff --git a/DevPack.UnitTesting/Simulation/SimulatedDms.cs b/DevPack.UnitTesting/Simulation/SimulatedDms.cs index 9c6f3486..80960a6c 100644 --- a/DevPack.UnitTesting/Simulation/SimulatedDms.cs +++ b/DevPack.UnitTesting/Simulation/SimulatedDms.cs @@ -51,6 +51,11 @@ public SimulatedDms() /// public DomSLNetMessageHandler DomHandler => _domSlNetMessageHandler; + /// + /// Gets or sets the maximum document size, in MB, reported by the simulated agent. + /// + public int MaxDocumentSizeInMegaBytes { get; set; } = 100; + /// /// Registers an installed application package so that installation checks succeed. /// @@ -433,6 +438,13 @@ private IEnumerable HandleMessage(GetInfoMessage msg) break; + case InfoType.GeneralInfoMessage: + yield return new GeneralInfoEventMessage + { + MaxDocumentSize = MaxDocumentSizeInMegaBytes, + }; + break; + default: throw new NotSupportedException($"Unsupported InfoType: {msg.Type}"); } diff --git a/DevPack/API/Handlers/Properties/DomPropertyHandler.cs b/DevPack/API/Handlers/Properties/DomPropertyHandler.cs index 1f230c7e..e0f22e79 100644 --- a/DevPack/API/Handlers/Properties/DomPropertyHandler.cs +++ b/DevPack/API/Handlers/Properties/DomPropertyHandler.cs @@ -5,6 +5,7 @@ namespace Skyline.DataMiner.Solutions.MediaOps.Plan.API using System.Linq; using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; + using Skyline.DataMiner.Net.Messages; using Skyline.DataMiner.Net.Messages.SLDataGateway; using Skyline.DataMiner.Solutions.MediaOps.Plan.Exceptions; using Skyline.DataMiner.Solutions.MediaOps.Plan.Storage.DOM; @@ -556,7 +557,54 @@ private void ValidateFileProperties(ICollection apiProperties) return; } - throw new NotImplementedException(); + // Only properties that define their own limit are validated; the others fall back to the server limit. + var propertiesWithSizeLimit = apiProperties.Where(x => x.HasSizeLimit).ToList(); + if (propertiesWithSizeLimit.Count == 0) + { + return; + } + + var maxSizeLimit = GetMaxDocumentSizeInMegaBytes(); + + foreach (var property in propertiesWithSizeLimit) + { + if (property.SizeLimit <= 0) + { + var error = new PropertyInvalidFileSizeLimitError + { + ErrorMessage = "Size limit must be greater than 0.", + SizeLimit = property.SizeLimit, + Id = property.Id, + }; + + ReportError(property.Id, error); + continue; + } + + if (maxSizeLimit.HasValue && property.SizeLimit > maxSizeLimit.Value) + { + var error = new PropertyInvalidFileSizeLimitError + { + ErrorMessage = $"Size limit cannot exceed the maximum file size allowed by DataMiner ({maxSizeLimit.Value} MB).", + SizeLimit = property.SizeLimit, + Id = property.Id, + }; + + ReportError(property.Id, error); + } + } + } + + // Returns the file size limit configured on the server, in MB, or null when it is not exposed. + private long? GetMaxDocumentSizeInMegaBytes() + { + var response = planApi.Connection.HandleSingleResponseMessage(new GetInfoMessage(InfoType.GeneralInfoMessage)) as GeneralInfoEventMessage; + if (response == null || response.MaxDocumentSize <= 0) + { + return null; + } + + return response.MaxDocumentSize; } private void ValidateStateForDeleteAction(ICollection apiProperties) diff --git a/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs b/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs index 75a31b59..6e2157b7 100644 --- a/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs +++ b/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs @@ -101,6 +101,52 @@ private void CreateOrUpdateLocked(ICollection apiSett .Select(x => new DomPropertySettingCollection(x.Instance)) .ToList(); CreateOrUpdateDomPropertySettingCollections(toCreateDomInstances.Concat(toUpdateDomInstances).ToList()); + + // The file content is attached to the DOM instance, so it can only be stored once that instance exists. + SyncAttachments(apiSettingCollections.Where(IsValid).ToList()); + } + + private void SyncAttachments(ICollection apiSettingCollections) + { + foreach (var settingCollection in apiSettingCollections) + { + var instanceId = new DomInstanceId(settingCollection.Id); + + foreach (var setting in settingCollection.FileSettings) + { + // The collection exists from here on, so the content of its files can be read on demand. + setting.SetStorageContext(planApi, settingCollection.Id); + + if (setting.FilesToUpload.Count == 0 && setting.FilesToDelete.Count == 0) + { + continue; + } + + try + { + foreach (var fileName in setting.FilesToDelete) + { + planApi.PropertyAttachments.Delete(instanceId, FilePropertySetting.GetAttachmentName(setting.Id, fileName)); + } + + foreach (var fileToUpload in setting.FilesToUpload) + { + planApi.PropertyAttachments.Add(instanceId, FilePropertySetting.GetAttachmentName(setting.Id, fileToUpload.Key), fileToUpload.Value); + } + + setting.ClearPendingFileChanges(); + } + catch (Exception ex) + { + ReportError(settingCollection.Id, new PropertySettingCollectionInvalidPropertySettingsError + { + ErrorMessage = $"The files of the property could not be stored: {ex.Message}", + PropertyId = setting.Id, + Id = settingCollection.Id, + }); + } + } + } } private void CreateOrUpdateDomPropertySettingCollections(ICollection domValueCollections) @@ -168,6 +214,9 @@ private void DeleteLocked(ICollection apiSettingColle throw new ArgumentException($"Not all provided property value collections are valid", nameof(apiSettingCollections)); } + // The attachments are removed while the instance still exists, so no file content is left behind. + DeleteAttachments(apiSettingCollections); + var toDelete = apiSettingCollections.Select(x => x.OriginalInstance.ToInstance()); planApi.DomHelpers.SlcPropertiesHelper.DomHelper.DomInstances.TryDeleteInBatches(toDelete, out var domResult); @@ -187,6 +236,35 @@ private void DeleteLocked(ICollection apiSettingColle ReportSuccess(toDelete.Where(x => domResult.SuccessfulIds.Contains(x.ID)).Select(x => new DomPropertySettingCollection(x))); } + private void DeleteAttachments(ICollection apiSettingCollections) + { + foreach (var settingCollection in apiSettingCollections) + { + var settingsWithFiles = settingCollection.FileSettings.Where(x => x.Files.Count != 0).ToList(); + if (settingsWithFiles.Count == 0) + { + continue; + } + + var instanceId = new DomInstanceId(settingCollection.Id); + + foreach (var setting in settingsWithFiles) + { + foreach (var fileName in setting.Files) + { + try + { + planApi.PropertyAttachments.Delete(instanceId, FilePropertySetting.GetAttachmentName(setting.Id, fileName)); + } + catch (Exception ex) + { + planApi.Logger.Error(this, $"Failed to delete attachment '{fileName}' of property '{setting.Id}': {ex}"); + } + } + } + } + } + private void ValidateIdsNotInUse(ICollection apiSettingCollections) { if (apiSettingCollections == null) diff --git a/DevPack/API/MediaOpsPlanApi.cs b/DevPack/API/MediaOpsPlanApi.cs index fe511dad..2141d98a 100644 --- a/DevPack/API/MediaOpsPlanApi.cs +++ b/DevPack/API/MediaOpsPlanApi.cs @@ -49,6 +49,8 @@ public class MediaOpsPlanApi : IMediaOpsPlanApi private ILogger logger; + private IPropertyAttachmentStore propertyAttachments; + /// /// Initializes a new instance of the class. /// @@ -151,6 +153,13 @@ internal MediaOpsPlanApi(IConnection connection) internal DomHelpers DomHelpers => domHelpers; + // Settable so the attachment handling can be verified without a DataMiner Agent. + internal IPropertyAttachmentStore PropertyAttachments + { + get => propertyAttachments ?? (propertyAttachments = new PropertyAttachmentStore(domHelpers.SlcPropertiesHelper)); + set => propertyAttachments = value; + } + internal CoreHelpers CoreHelpers => coreHelpers; internal IDms Dms => lazyDms.Value; diff --git a/DevPack/API/Objects/Properties/Definitions/FileProperty.cs b/DevPack/API/Objects/Properties/Definitions/FileProperty.cs index 2cddff0d..047eabb5 100644 --- a/DevPack/API/Objects/Properties/Definitions/FileProperty.cs +++ b/DevPack/API/Objects/Properties/Definitions/FileProperty.cs @@ -96,7 +96,9 @@ public override bool Equals(object obj) internal override void ApplyChanges(StorageProperties.PropertyInstance instance) { instance.PropertyInfo.PropertyType = StorageProperties.SlcPropertiesIds.Enums.PropertytypeEnum.File; - instance.PropertyInfo.FileSizeLimit = SizeLimit; + + // A zero size limit signals that the limit configured on the server applies. + instance.PropertyInfo.FileSizeLimit = HasSizeLimit ? SizeLimit : 0; instance.PropertyInfo.AllowMultipleFiles = AllowMultiple; } diff --git a/DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs b/DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs new file mode 100644 index 00000000..e6b5544b --- /dev/null +++ b/DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs @@ -0,0 +1,67 @@ +namespace Skyline.DataMiner.Solutions.MediaOps.Plan.API +{ + using System; + using System.Linq; + + using StorageProperties = Storage.DOM.SlcProperties; + + internal class InnerFilePropertySetting : FilePropertySetting + { + private const char FileSeparator = '|'; + + private StorageProperties.PropertyValueSection originalSection; + private StorageProperties.PropertyValueSection updatedSection; + + internal InnerFilePropertySetting(FilePropertySetting filePropertySetting) + : base(filePropertySetting) + { + } + + internal InnerFilePropertySetting(MediaOpsPlanApi planApi, Guid settingCollectionId, StorageProperties.PropertyValueSection section) + { + ParseSection(section); + SetStorageContext(planApi, settingCollectionId); + InitTracking(); + } + + internal override Storage.DOM.DomSectionBase OriginalSection => originalSection; + + internal StorageProperties.PropertyValueSection GetSectionWithChanges() + { + if (updatedSection == null) + { + updatedSection = IsNew ? new StorageProperties.PropertyValueSection() : originalSection.Clone(); + } + + updatedSection.PropertyID = Id; + updatedSection.Value = string.Join(FileSeparator.ToString(), Files); + + return updatedSection; + } + + private void ParseSection(StorageProperties.PropertyValueSection section) + { + originalSection = section ?? throw new ArgumentNullException(nameof(section)); + + Id = section.PropertyID.Value; + + if (string.IsNullOrEmpty(section.Value)) + { + return; + } + + foreach (var entry in section.Value.Split(new[] { FileSeparator }, StringSplitOptions.RemoveEmptyEntries)) + { + AddParsedFile(StripAttachmentPrefix(entry)); + } + } + + // Values written by older versions store the attachment name instead of the file name. + private string StripAttachmentPrefix(string entry) + { + var prefix = $"{Id}_"; + + return entry.StartsWith(prefix, StringComparison.OrdinalIgnoreCase) ? entry.Substring(prefix.Length) : entry; + } + } +} diff --git a/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs b/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs new file mode 100644 index 00000000..446d627a --- /dev/null +++ b/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs @@ -0,0 +1,243 @@ +namespace Skyline.DataMiner.Solutions.MediaOps.Plan.API +{ + using System; + using System.Collections.Generic; + using System.IO; + using System.Linq; + + using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; + using Skyline.DataMiner.Net.Helper; + + /// + /// Represents a property value that holds one or more files. + /// + /// + /// The file content is stored as an attachment on the property value collection. Content that is added or removed is + /// only persisted when the property value collection is saved. + /// + public class FilePropertySetting : PropertySetting + { + private readonly List files = new List(); + + private readonly Dictionary filesToUpload = new Dictionary(StringComparer.OrdinalIgnoreCase); + + private readonly HashSet filesToDelete = new HashSet(StringComparer.OrdinalIgnoreCase); + + private MediaOpsPlanApi planApi; + + private Guid settingCollectionId; + + /// + /// Initializes a new instance of the class linked to the specified file property. + /// + /// The definition to link to. + public FilePropertySetting(FileProperty property) + : base(property) + { + } + + internal FilePropertySetting() + { + } + + internal FilePropertySetting(FilePropertySetting filePropertySetting) + : base(filePropertySetting) + { + files.AddRange(filePropertySetting.files); + + foreach (var fileToUpload in filePropertySetting.filesToUpload) + { + filesToUpload[fileToUpload.Key] = fileToUpload.Value; + } + + foreach (var fileToDelete in filePropertySetting.filesToDelete) + { + filesToDelete.Add(fileToDelete); + } + } + + /// + /// Gets the names of the files of this property. + /// + public IReadOnlyCollection Files => files; + + /// + public override bool HasValue => files.Count > 0; + + internal IReadOnlyDictionary FilesToUpload => filesToUpload; + + internal IReadOnlyCollection FilesToDelete => filesToDelete; + + // The attachment holding the content of a file is named after the property it belongs to. + internal static string GetAttachmentName(Guid propertyId, string fileName) + { + return $"{propertyId}_{fileName}"; + } + + /// + /// Reads the content of the specified file. + /// + /// The name of the file to read. + /// The content of the file. + /// The content of a stored file is retrieved on demand, so it is not held in memory while the file names are used. + /// Thrown when is or white space, or is not a file of this property. + /// Thrown when the property value collection holding this file was never read or saved. + public byte[] ReadContent(string fileName) + { + var name = NormalizeFileName(fileName); + + // A file that is not stored yet is still held in memory. + if (filesToUpload.TryGetValue(name, out var content)) + { + return content; + } + + if (!files.Contains(name, StringComparer.OrdinalIgnoreCase)) + { + throw new ArgumentException($"File '{name}' is not a file of this property.", nameof(fileName)); + } + + if (planApi == null || settingCollectionId == Guid.Empty) + { + throw new InvalidOperationException("The content can only be read for a property value collection that was read or saved."); + } + + return planApi.PropertyAttachments.Get(new DomInstanceId(settingCollectionId), GetAttachmentName(Id, name)); + } + + /// + /// Adds a file to this property, or replaces the content when a file with the same name was already added. + /// + /// The name of the file. + /// The content of the file. + /// This , so calls can be chained. + /// Thrown when is or white space. + /// Thrown when is . + public FilePropertySetting AddFile(string fileName, byte[] content) + { + var name = NormalizeFileName(fileName); + + if (content == null) + { + throw new ArgumentNullException(nameof(content)); + } + + filesToDelete.Remove(name); + filesToUpload[name] = content; + + if (!files.Contains(name, StringComparer.OrdinalIgnoreCase)) + { + files.Add(name); + } + + return this; + } + + /// + /// Removes the specified file from this property. + /// + /// The name of the file to remove. + /// This , so calls can be chained. + /// Thrown when is or white space. + public FilePropertySetting RemoveFile(string fileName) + { + var name = NormalizeFileName(fileName); + + var storedName = files.FirstOrDefault(x => string.Equals(x, name, StringComparison.OrdinalIgnoreCase)); + if (storedName == null) + { + return this; + } + + files.Remove(storedName); + + // A file that was never uploaded only has to be forgotten, not deleted. + if (!filesToUpload.Remove(storedName)) + { + filesToDelete.Add(storedName); + } + + return this; + } + + /// + /// Removes all files from this property. + /// + /// This , so calls can be chained. + public FilePropertySetting ClearFiles() + { + foreach (var fileName in files.ToArray()) + { + RemoveFile(fileName); + } + + return this; + } + + /// + public override int GetHashCode() + { + unchecked + { + var hash = base.GetHashCode(); + + foreach (var fileName in files.OrderBy(x => x).ToArray()) + { + hash = (hash * 23) + (fileName != null ? fileName.GetHashCode() : 0); + } + + return hash; + } + } + + /// + public override bool Equals(object obj) + { + if (obj is not FilePropertySetting other) + { + return false; + } + + return base.Equals(other) + && files.ScrambledEquals(other.files); + } + + internal void AddParsedFile(string fileName) + { + if (!files.Contains(fileName, StringComparer.OrdinalIgnoreCase)) + { + files.Add(fileName); + } + } + + internal void ClearPendingFileChanges() + { + filesToUpload.Clear(); + filesToDelete.Clear(); + } + + // Captures where the content of the files is stored, so it can be read on demand. + internal void SetStorageContext(MediaOpsPlanApi planApi, Guid settingCollectionId) + { + this.planApi = planApi; + this.settingCollectionId = settingCollectionId; + } + + // The file name is used as an attachment name, so any directory information is stripped off. + private static string NormalizeFileName(string fileName) + { + if (string.IsNullOrWhiteSpace(fileName)) + { + throw new ArgumentException("The file name cannot be null or white space.", nameof(fileName)); + } + + var name = Path.GetFileName(fileName.Trim()); + if (string.IsNullOrWhiteSpace(name)) + { + throw new ArgumentException($"'{fileName}' is not a valid file name.", nameof(fileName)); + } + + return name; + } + } +} diff --git a/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs b/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs index 4dbafbfa..600cd4dd 100644 --- a/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs +++ b/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs @@ -18,6 +18,7 @@ public class PropertySettingCollection : ApiObject, ICollection stringSettings = []; private readonly List booleanSettings = []; private readonly List discreteSettings = []; + private readonly List fileSettings = []; private StorageProperties.PropertyValuesInstance originalInstance; private StorageProperties.PropertyValuesInstance updatedInstance; @@ -115,7 +116,7 @@ internal PropertySettingCollection(MediaOpsPlanApi planApi, StorageProperties.Pr /// /// Gets the collection of property settings linked to a property definition. /// - public IReadOnlyCollection PropertySettings => stringSettings.Cast().Concat(booleanSettings).Concat(discreteSettings).ToList(); + public IReadOnlyCollection PropertySettings => stringSettings.Cast().Concat(booleanSettings).Concat(discreteSettings).Concat(fileSettings).ToList(); /// /// Gets the collection of string property settings. @@ -132,8 +133,13 @@ internal PropertySettingCollection(MediaOpsPlanApi planApi, StorageProperties.Pr /// public IReadOnlyCollection DiscreteSettings => discreteSettings; + /// + /// Gets the collection of file property settings. + /// + public IReadOnlyCollection FileSettings => fileSettings; + /// - public int Count => customSettings.Count + stringSettings.Count + booleanSettings.Count + discreteSettings.Count; + public int Count => customSettings.Count + stringSettings.Count + booleanSettings.Count + discreteSettings.Count + fileSettings.Count; /// public bool IsReadOnly => false; @@ -171,6 +177,11 @@ public override int GetHashCode() hash = (hash * 23) + value.GetHashCode(); } + foreach (var value in fileSettings.OrderBy(x => x.Id)) + { + hash = (hash * 23) + value.GetHashCode(); + } + return hash; } } @@ -190,7 +201,8 @@ public override bool Equals(object obj) && customSettings.ScrambledEquals(other.customSettings) && stringSettings.ScrambledEquals(other.stringSettings) && booleanSettings.ScrambledEquals(other.booleanSettings) - && discreteSettings.ScrambledEquals(other.discreteSettings); + && discreteSettings.ScrambledEquals(other.discreteSettings) + && fileSettings.ScrambledEquals(other.fileSettings); } /// @@ -215,6 +227,9 @@ public void Add(PropertySettingBase item) case DiscretePropertySetting discreteVal: discreteSettings.Add(new InnerDiscretePropertySetting(discreteVal)); break; + case FilePropertySetting fileVal: + fileSettings.Add(new InnerFilePropertySetting(fileVal)); + break; default: throw new ArgumentException($"Unsupported property setting type '{item.GetType().Name}'.", nameof(item)); } @@ -227,6 +242,7 @@ public void Clear() stringSettings.Clear(); booleanSettings.Clear(); discreteSettings.Clear(); + fileSettings.Clear(); } /// @@ -261,6 +277,7 @@ public void SetPropertySettings(IEnumerable settings) stringSettings.Clear(); booleanSettings.Clear(); discreteSettings.Clear(); + fileSettings.Clear(); if (settings == null) { @@ -287,6 +304,7 @@ public bool Contains(PropertySettingBase item) StringPropertySetting stringVal => stringSettings.Contains(stringVal), BooleanPropertySetting boolVal => booleanSettings.Contains(boolVal), DiscretePropertySetting discreteVal => discreteSettings.Contains(discreteVal), + FilePropertySetting fileVal => fileSettings.Contains(fileVal), _ => false, }; } @@ -329,6 +347,7 @@ public bool Remove(PropertySettingBase item) StringPropertySetting stringVal => stringSettings.RemoveAll(x => x.Equals(stringVal)) > 0, BooleanPropertySetting boolVal => booleanSettings.RemoveAll(x => x.Equals(boolVal)) > 0, DiscretePropertySetting discreteVal => discreteSettings.RemoveAll(x => x.Equals(discreteVal)) > 0, + FilePropertySetting fileVal => fileSettings.RemoveAll(x => x.Equals(fileVal)) > 0, _ => false, }; } @@ -341,6 +360,7 @@ public IEnumerator GetEnumerator() .Concat(stringSettings) .Concat(booleanSettings) .Concat(discreteSettings) + .Concat(fileSettings) .GetEnumerator(); } @@ -382,6 +402,11 @@ internal StorageProperties.PropertyValuesInstance GetInstanceWithChanges() updatedInstance.PropertyValue.Add(discreteSetting.GetSectionWithChanges()); } + foreach (var fileSetting in fileSettings) + { + updatedInstance.PropertyValue.Add(fileSetting.GetSectionWithChanges()); + } + return updatedInstance; } @@ -435,6 +460,10 @@ private void ParsePropertyValues(MediaOpsPlanApi planApi, IList 1) + { + ReportError(apiObjectId, ComposePropertySettingError(propertySetting.Id, "This property does not allow multiple files.")); + } + + if (!fileProperty.HasSizeLimit) + { + return; + } + + var sizeLimitInBytes = fileProperty.SizeLimit * 1024L * 1024L; + foreach (var fileToUpload in setting.FilesToUpload.Where(x => x.Value.LongLength > sizeLimitInBytes)) + { + ReportError(apiObjectId, ComposePropertySettingError(propertySetting.Id, $"File '{fileToUpload.Key}' exceeds the maximum file size of {fileProperty.SizeLimit} MB.")); + } + } + private MediaOpsErrorData ComposePropertySettingError(Guid propertyId, string errorMessage) { return new PropertySettingCollectionInvalidPropertySettingsError diff --git a/DevPack/Exceptions/TraceData/Properties/Definitions/PropertyInvalidFileSizeLimitError.cs b/DevPack/Exceptions/TraceData/Properties/Definitions/PropertyInvalidFileSizeLimitError.cs new file mode 100644 index 00000000..f69a517b --- /dev/null +++ b/DevPack/Exceptions/TraceData/Properties/Definitions/PropertyInvalidFileSizeLimitError.cs @@ -0,0 +1,13 @@ +namespace Skyline.DataMiner.Solutions.MediaOps.Plan.Exceptions +{ + /// + /// Represents an error that occurs when a file property is configured with an invalid size limit, such as a value that is not positive or a value that exceeds the maximum file size allowed by DataMiner. + /// + public sealed class PropertyInvalidFileSizeLimitError : PropertyError + { + /// + /// Gets the configured size limit, in MB, that caused the error. + /// + public long SizeLimit { get; internal set; } + } +} diff --git a/DevPack/Storage/DOM/Helpers/IPropertyAttachmentStore.cs b/DevPack/Storage/DOM/Helpers/IPropertyAttachmentStore.cs new file mode 100644 index 00000000..de3324c5 --- /dev/null +++ b/DevPack/Storage/DOM/Helpers/IPropertyAttachmentStore.cs @@ -0,0 +1,18 @@ +namespace Skyline.DataMiner.Solutions.MediaOps.Plan.Storage.DOM +{ + using System.Collections.Generic; + + using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; + + // Seam around the DataMiner attachment API so the attachment handling can be verified without a DataMiner Agent. + internal interface IPropertyAttachmentStore + { + void Add(DomInstanceId instanceId, string attachmentName, byte[] content); + + byte[] Get(DomInstanceId instanceId, string attachmentName); + + void Delete(DomInstanceId instanceId, string attachmentName); + + IReadOnlyCollection GetNames(DomInstanceId instanceId); + } +} diff --git a/DevPack/Storage/DOM/Helpers/PropertyAttachmentStore.cs b/DevPack/Storage/DOM/Helpers/PropertyAttachmentStore.cs new file mode 100644 index 00000000..1baff4ba --- /dev/null +++ b/DevPack/Storage/DOM/Helpers/PropertyAttachmentStore.cs @@ -0,0 +1,37 @@ +namespace Skyline.DataMiner.Solutions.MediaOps.Plan.Storage.DOM +{ + using System; + using System.Collections.Generic; + + using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; + + internal sealed class PropertyAttachmentStore : IPropertyAttachmentStore + { + private readonly SlcPropertiesHelper propertiesHelper; + + public PropertyAttachmentStore(SlcPropertiesHelper propertiesHelper) + { + this.propertiesHelper = propertiesHelper ?? throw new ArgumentNullException(nameof(propertiesHelper)); + } + + public void Add(DomInstanceId instanceId, string attachmentName, byte[] content) + { + propertiesHelper.DomHelper.DomInstances.Attachments.Add(instanceId, attachmentName, content); + } + + public byte[] Get(DomInstanceId instanceId, string attachmentName) + { + return propertiesHelper.DomHelper.DomInstances.Attachments.Get(instanceId, attachmentName); + } + + public void Delete(DomInstanceId instanceId, string attachmentName) + { + propertiesHelper.DomHelper.DomInstances.Attachments.Delete(instanceId, attachmentName); + } + + public IReadOnlyCollection GetNames(DomInstanceId instanceId) + { + return propertiesHelper.DomHelper.DomInstances.Attachments.GetFileNames(instanceId); + } + } +} From 4e1be163d86d365af6cb678c17adb0062f1a3705 Mon Sep 17 00:00:00 2001 From: Jens Vandewalle Date: Wed, 12 Aug 2026 16:00:51 +0200 Subject: [PATCH 04/12] Stricter file name validation and improved attachment cleanup - Disallow separator and invalid characters in file names; add unit tests - Track stored files in FilePropertySetting to delete only uploaded files - Track removed file settings in PropertySettingCollection for cleanup - Update cleanup logic to handle current and removed file settings, clear removed list after cleanup - Return persisted instances after create/update for accurate reporting - Add tests for attachment deletion and structured error reporting on upload failure --- .../Values/FilePropertySettingTests.cs | 16 ++++ .../Values/FilePropertySimulationTests.cs | 82 +++++++++++++++++++ .../DomPropertySettingCollectionHandler.cs | 57 ++++++++----- .../Jobs/ResourceManagerTraceDataHandler.cs | 1 + .../Internal/InnerFilePropertySetting.cs | 2 - .../Values/Public/FilePropertySetting.cs | 32 +++++++- .../Public/PropertySettingCollection.cs | 33 +++++++- 7 files changed, 196 insertions(+), 27 deletions(-) diff --git a/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs b/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs index 14083670..27114b3e 100644 --- a/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs +++ b/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs @@ -84,6 +84,22 @@ public void AddFile_EmptyName_Throws() Assert.ThrowsException(() => setting.AddFile(" ", Content("abc"))); } + [TestMethod] + public void AddFile_NameContainingSeparator_Throws() + { + var setting = new FilePropertySetting(new FileProperty()); + + Assert.ThrowsException(() => setting.AddFile("first|second.pdf", Content("abc"))); + } + + [TestMethod] + public void AddFile_NameContainingInvalidCharacter_Throws() + { + var setting = new FilePropertySetting(new FileProperty()); + + Assert.ThrowsException(() => setting.AddFile("do.pdf", Content("abc"))); + } + [TestMethod] public void RemoveFile_RemovesFile() { diff --git a/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs b/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs index 64770d74..31eb5bb4 100644 --- a/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs +++ b/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs @@ -181,6 +181,81 @@ public void Delete_RemovesAttachments() Assert.IsFalse(attachments.Contains(collection.Id, $"{property.Id}_document.pdf"), "Expected the attachments to be removed together with the collection."); } + [TestMethod] + public void Update_ReplacedThenRemovedFile_DeletesStoredAttachment() + { + var (api, attachments) = CreateContext(); + var property = CreateFileProperty(api); + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); + api.PropertySettingCollections.Create(collection); + + var read = api.PropertySettingCollections.Read(collection.Id); + var setting = read.FileSettings.Single(); + + // Replacing the content and removing it again must still delete the file that is already stored. + setting.AddFile("document.pdf", Content("replaced")); + setting.RemoveFile("document.pdf"); + + api.PropertySettingCollections.Update(read); + + Assert.IsFalse(attachments.Contains(collection.Id, $"{property.Id}_document.pdf"), "Expected the stored attachment to be deleted."); + } + + [TestMethod] + public void Update_RemovingWholeFileSetting_DeletesAttachments() + { + var (api, attachments) = CreateContext(); + var property = CreateFileProperty(api); + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); + api.PropertySettingCollections.Create(collection); + + var read = api.PropertySettingCollections.Read(collection.Id); + read.Remove(read.FileSettings.Single()); + + api.PropertySettingCollections.Update(read); + + Assert.IsFalse(attachments.Contains(collection.Id, $"{property.Id}_document.pdf"), "Expected the attachments of a removed file setting to be deleted."); + } + + [TestMethod] + public void Update_ClearingSettings_DeletesAttachments() + { + var (api, attachments) = CreateContext(); + var property = CreateFileProperty(api); + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); + api.PropertySettingCollections.Create(collection); + + var read = api.PropertySettingCollections.Read(collection.Id); + read.SetPropertySettings(null); + + api.PropertySettingCollections.Update(read); + + Assert.IsFalse(attachments.Contains(collection.Id, $"{property.Id}_document.pdf"), "Expected the attachments to be deleted when the settings are replaced."); + } + + [TestMethod] + public void Create_WhenAttachmentUploadFails_ReportsErrorInsteadOfThrowingUnexpectedly() + { + var (api, attachments) = CreateContext(); + var property = CreateFileProperty(api); + + attachments.FailOnAdd = true; + + var collection = CreateCollection(); + collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); + + var exception = Assert.ThrowsException(() => api.PropertySettingCollections.Create(collection)); + Assert.IsTrue( + exception.TraceData.ErrorData.OfType().Any(), + "Expected the attachment failure to be reported as a structured error."); + } + [TestMethod] public void Create_MultipleFilesWhileNotAllowed_ThrowsInvalidPropertySettingsError() { @@ -267,8 +342,15 @@ private sealed class FakePropertyAttachmentStore : IPropertyAttachmentStore { private readonly Dictionary> attachments = new Dictionary>(); + public bool FailOnAdd { get; set; } + public void Add(DomInstanceId instanceId, string attachmentName, byte[] content) { + if (FailOnAdd) + { + throw new InvalidOperationException("Simulated upload failure."); + } + if (!attachments.TryGetValue(instanceId.Id, out var perInstance)) { perInstance = new Dictionary(StringComparer.OrdinalIgnoreCase); diff --git a/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs b/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs index 6e2157b7..add4d5f0 100644 --- a/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs +++ b/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs @@ -100,10 +100,13 @@ private void CreateOrUpdateLocked(ICollection apiSett .Where(IsValid) .Select(x => new DomPropertySettingCollection(x.Instance)) .ToList(); - CreateOrUpdateDomPropertySettingCollections(toCreateDomInstances.Concat(toUpdateDomInstances).ToList()); + var persistedInstances = CreateOrUpdateDomPropertySettingCollections(toCreateDomInstances.Concat(toUpdateDomInstances).ToList()); // The file content is attached to the DOM instance, so it can only be stored once that instance exists. SyncAttachments(apiSettingCollections.Where(IsValid).ToList()); + + // Reported last, because a collection whose files could not be stored must not be reported as successful. + ReportSuccess(persistedInstances.Where(x => !TraceDataPerItem.ContainsKey(x.ID.Id))); } private void SyncAttachments(ICollection apiSettingCollections) @@ -112,6 +115,14 @@ private void SyncAttachments(ICollection apiSettingCo { var instanceId = new DomInstanceId(settingCollection.Id); + // A file setting that was dropped from the collection still has attachments that must be cleaned up. + foreach (var removedSetting in settingCollection.RemovedFileSettings) + { + DeleteAttachments(instanceId, removedSetting, removedSetting.StoredFiles); + } + + settingCollection.ClearRemovedFileSettings(); + foreach (var setting in settingCollection.FileSettings) { // The collection exists from here on, so the content of its files can be read on demand. @@ -149,7 +160,23 @@ private void SyncAttachments(ICollection apiSettingCo } } - private void CreateOrUpdateDomPropertySettingCollections(ICollection domValueCollections) + // Cleaning up attachments must never fail the operation itself, so failures are only logged. + private void DeleteAttachments(DomInstanceId instanceId, FilePropertySetting setting, IEnumerable fileNames) + { + foreach (var fileName in fileNames) + { + try + { + planApi.PropertyAttachments.Delete(instanceId, FilePropertySetting.GetAttachmentName(setting.Id, fileName)); + } + catch (Exception ex) + { + planApi.Logger.Error(this, $"Failed to delete attachment '{fileName}' of property '{setting.Id}': {ex}"); + } + } + } + + private ICollection CreateOrUpdateDomPropertySettingCollections(ICollection domValueCollections) { if (domValueCollections == null) { @@ -158,7 +185,7 @@ private void CreateOrUpdateDomPropertySettingCollections(ICollection(); } planApi.DomHelpers.SlcPropertiesHelper.DomHelper.DomInstances.TryCreateOrUpdateInBatches(domValueCollections.Select(x => x.ToInstance()), out var domResult); @@ -176,7 +203,7 @@ private void CreateOrUpdateDomPropertySettingCollections(ICollection new DomPropertySettingCollection(x))); + return domResult.SuccessfulItems.Select(x => new DomPropertySettingCollection(x)).ToList(); } private void Delete(ICollection apiSettingCollections) @@ -240,28 +267,14 @@ private void DeleteAttachments(ICollection apiSetting { foreach (var settingCollection in apiSettingCollections) { - var settingsWithFiles = settingCollection.FileSettings.Where(x => x.Files.Count != 0).ToList(); - if (settingsWithFiles.Count == 0) - { - continue; - } - var instanceId = new DomInstanceId(settingCollection.Id); - foreach (var setting in settingsWithFiles) + foreach (var setting in settingCollection.FileSettings.Concat(settingCollection.RemovedFileSettings)) { - foreach (var fileName in setting.Files) - { - try - { - planApi.PropertyAttachments.Delete(instanceId, FilePropertySetting.GetAttachmentName(setting.Id, fileName)); - } - catch (Exception ex) - { - planApi.Logger.Error(this, $"Failed to delete attachment '{fileName}' of property '{setting.Id}': {ex}"); - } - } + DeleteAttachments(instanceId, setting, setting.StoredFiles); } + + settingCollection.ClearRemovedFileSettings(); } } diff --git a/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs b/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs index a959ee5f..0305850c 100644 --- a/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs +++ b/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs @@ -43,6 +43,7 @@ public IReadOnlyDictionary Translate(ICollection public class FilePropertySetting : PropertySetting { + // The file names are stored as a single separated value, so the separator cannot be part of a file name. + internal const char FileSeparator = '|'; + private readonly List files = new List(); private readonly Dictionary filesToUpload = new Dictionary(StringComparer.OrdinalIgnoreCase); private readonly HashSet filesToDelete = new HashSet(StringComparer.OrdinalIgnoreCase); + // The files that are known to be stored as an attachment, so a removal knows whether it has to delete one. + private readonly HashSet storedFiles = new HashSet(StringComparer.OrdinalIgnoreCase); + private MediaOpsPlanApi planApi; private Guid settingCollectionId; @@ -68,6 +74,8 @@ internal FilePropertySetting(FilePropertySetting filePropertySetting) internal IReadOnlyCollection FilesToDelete => filesToDelete; + internal IReadOnlyCollection StoredFiles => storedFiles; + // The attachment holding the content of a file is named after the property it belongs to. internal static string GetAttachmentName(Guid propertyId, string fileName) { @@ -150,9 +158,10 @@ public FilePropertySetting RemoveFile(string fileName) } files.Remove(storedName); + filesToUpload.Remove(storedName); - // A file that was never uploaded only has to be forgotten, not deleted. - if (!filesToUpload.Remove(storedName)) + // Only a file that is actually stored has an attachment that must be deleted. + if (storedFiles.Contains(storedName)) { filesToDelete.Add(storedName); } @@ -208,12 +217,21 @@ internal void AddParsedFile(string fileName) { files.Add(fileName); } + + storedFiles.Add(fileName); } internal void ClearPendingFileChanges() { filesToUpload.Clear(); filesToDelete.Clear(); + + // Everything that is left is stored from here on. + storedFiles.Clear(); + foreach (var fileName in files) + { + storedFiles.Add(fileName); + } } // Captures where the content of the files is stored, so it can be read on demand. @@ -237,6 +255,16 @@ private static string NormalizeFileName(string fileName) throw new ArgumentException($"'{fileName}' is not a valid file name.", nameof(fileName)); } + if (name.IndexOf(FileSeparator) != -1) + { + throw new ArgumentException($"The file name cannot contain '{FileSeparator}'.", nameof(fileName)); + } + + if (name.IndexOfAny(Path.GetInvalidFileNameChars()) != -1) + { + throw new ArgumentException($"'{fileName}' is not a valid file name.", nameof(fileName)); + } + return name; } } diff --git a/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs b/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs index 600cd4dd..f6feb6dc 100644 --- a/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs +++ b/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs @@ -20,6 +20,9 @@ public class PropertySettingCollection : ApiObject, ICollection discreteSettings = []; private readonly List fileSettings = []; + // File settings that are dropped still own attachments, so they are kept until those are deleted. + private readonly List removedFileSettings = []; + private StorageProperties.PropertyValuesInstance originalInstance; private StorageProperties.PropertyValuesInstance updatedInstance; @@ -146,6 +149,8 @@ internal PropertySettingCollection(MediaOpsPlanApi planApi, StorageProperties.Pr internal StorageProperties.PropertyValuesInstance OriginalInstance => originalInstance; + internal IReadOnlyCollection RemovedFileSettings => removedFileSettings; + /// public override int GetHashCode() { @@ -242,6 +247,7 @@ public void Clear() stringSettings.Clear(); booleanSettings.Clear(); discreteSettings.Clear(); + TrackRemovedFileSettings(fileSettings); fileSettings.Clear(); } @@ -277,6 +283,7 @@ public void SetPropertySettings(IEnumerable settings) stringSettings.Clear(); booleanSettings.Clear(); discreteSettings.Clear(); + TrackRemovedFileSettings(fileSettings); fileSettings.Clear(); if (settings == null) @@ -347,11 +354,30 @@ public bool Remove(PropertySettingBase item) StringPropertySetting stringVal => stringSettings.RemoveAll(x => x.Equals(stringVal)) > 0, BooleanPropertySetting boolVal => booleanSettings.RemoveAll(x => x.Equals(boolVal)) > 0, DiscretePropertySetting discreteVal => discreteSettings.RemoveAll(x => x.Equals(discreteVal)) > 0, - FilePropertySetting fileVal => fileSettings.RemoveAll(x => x.Equals(fileVal)) > 0, + FilePropertySetting fileVal => RemoveFileSetting(fileVal), _ => false, }; } + private bool RemoveFileSetting(FilePropertySetting fileSetting) + { + var toRemove = fileSettings.Where(x => x.Equals(fileSetting)).ToList(); + if (toRemove.Count == 0) + { + return false; + } + + TrackRemovedFileSettings(toRemove); + toRemove.ForEach(x => fileSettings.Remove(x)); + + return true; + } + + private void TrackRemovedFileSettings(IEnumerable settings) + { + removedFileSettings.AddRange(settings.Where(x => x.StoredFiles.Count != 0)); + } + /// public IEnumerator GetEnumerator() { @@ -410,6 +436,11 @@ internal StorageProperties.PropertyValuesInstance GetInstanceWithChanges() return updatedInstance; } + internal void ClearRemovedFileSettings() + { + removedFileSettings.Clear(); + } + private void ParseInstance(MediaOpsPlanApi planApi, StorageProperties.PropertyValuesInstance instance) { originalInstance = instance ?? throw new ArgumentNullException(nameof(instance)); From b32d166c04a8a283c46fd7a261e3ee0b7704c0aa Mon Sep 17 00:00:00 2001 From: Jens Vandewalle Date: Wed, 12 Aug 2026 16:01:29 +0200 Subject: [PATCH 05/12] Build DOM resource ID map from error lists for quarantine Builds domResourceIdByCoreId using three error lists via BuildDomResourceIdByCoreId. Passes the mapping to HandleReservationUpdateCausedReservationsToGoToQuarantine for further processing. --- .../Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs b/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs index a959ee5f..0305850c 100644 --- a/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs +++ b/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs @@ -43,6 +43,7 @@ public IReadOnlyDictionary Translate(ICollection Date: Wed, 12 Aug 2026 16:30:51 +0200 Subject: [PATCH 06/12] Improve file attachment handling in property settings - Retain attachments when updating collections with same file property - Copy file content when copying settings between collections - Refine cleanup to delete only orphaned attachments - Update constructors to accept destination collection ID - Add and update tests for attachment retention and copying - Remove explicit attachment deletion on collection removal --- .../Values/FilePropertySimulationTests.cs | 32 +++++++++++++++++-- .../DomPropertySettingCollectionHandler.cs | 31 +++++++----------- .../Internal/InnerFilePropertySetting.cs | 4 +-- .../Values/Public/FilePropertySetting.cs | 29 ++++++++++++++++- .../Public/PropertySettingCollection.cs | 2 +- 5 files changed, 71 insertions(+), 27 deletions(-) diff --git a/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs b/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs index 31eb5bb4..cc29c65d 100644 --- a/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs +++ b/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs @@ -166,7 +166,7 @@ public void Update_RemovingFile_DeletesAttachment() } [TestMethod] - public void Delete_RemovesAttachments() + public void Update_RetainingSettingsOfSameCollection_KeepsAttachmentsWithoutReupload() { var (api, attachments) = CreateContext(); var property = CreateFileProperty(api); @@ -176,9 +176,35 @@ public void Delete_RemovesAttachments() api.PropertySettingCollections.Create(collection); var read = api.PropertySettingCollections.Read(collection.Id); - api.PropertySettingCollections.Delete(read); + read.SetPropertySettings(read.PropertySettings.ToList()); - Assert.IsFalse(attachments.Contains(collection.Id, $"{property.Id}_document.pdf"), "Expected the attachments to be removed together with the collection."); + var updated = api.PropertySettingCollections.Update(read); + + CollectionAssert.AreEqual(new[] { "document.pdf" }, updated.FileSettings.Single().Files.ToArray()); + CollectionAssert.AreEqual(Content("hello"), updated.FileSettings.Single().ReadContent("document.pdf")); + Assert.IsTrue(attachments.Contains(collection.Id, $"{property.Id}_document.pdf")); + } + + [TestMethod] + public void Create_CopyingSettingFromOtherCollection_CopiesStoredContent() + { + var (api, attachments) = CreateContext(); + var property = CreateFileProperty(api); + + var source = CreateCollection(); + source.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); + api.PropertySettingCollections.Create(source); + + var readSource = api.PropertySettingCollections.Read(source.Id); + + var destination = CreateCollection(); + destination.Add(readSource.FileSettings.Single()); + var created = api.PropertySettingCollections.Create(destination); + + Assert.IsTrue( + attachments.Contains(destination.Id, $"{property.Id}_document.pdf"), + "Expected the stored content to be copied to the destination collection."); + CollectionAssert.AreEqual(Content("hello"), created.FileSettings.Single().ReadContent("document.pdf")); } [TestMethod] diff --git a/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs b/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs index add4d5f0..f19a2c05 100644 --- a/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs +++ b/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs @@ -115,10 +115,18 @@ private void SyncAttachments(ICollection apiSettingCo { var instanceId = new DomInstanceId(settingCollection.Id); - // A file setting that was dropped from the collection still has attachments that must be cleaned up. + // A dropped file setting can be re-added as a copy, so only the attachments that nothing refers to anymore are removed. + var expectedAttachments = new HashSet( + settingCollection.FileSettings.SelectMany(x => x.Files.Select(f => FilePropertySetting.GetAttachmentName(x.Id, f))), + StringComparer.OrdinalIgnoreCase); + foreach (var removedSetting in settingCollection.RemovedFileSettings) { - DeleteAttachments(instanceId, removedSetting, removedSetting.StoredFiles); + var orphaned = removedSetting.StoredFiles + .Where(x => !expectedAttachments.Contains(FilePropertySetting.GetAttachmentName(removedSetting.Id, x))) + .ToList(); + + DeleteAttachments(instanceId, removedSetting, orphaned); } settingCollection.ClearRemovedFileSettings(); @@ -241,9 +249,7 @@ private void DeleteLocked(ICollection apiSettingColle throw new ArgumentException($"Not all provided property value collections are valid", nameof(apiSettingCollections)); } - // The attachments are removed while the instance still exists, so no file content is left behind. - DeleteAttachments(apiSettingCollections); - + // The server removes the attachments together with the instance, so they need no separate cleanup here. var toDelete = apiSettingCollections.Select(x => x.OriginalInstance.ToInstance()); planApi.DomHelpers.SlcPropertiesHelper.DomHelper.DomInstances.TryDeleteInBatches(toDelete, out var domResult); @@ -263,21 +269,6 @@ private void DeleteLocked(ICollection apiSettingColle ReportSuccess(toDelete.Where(x => domResult.SuccessfulIds.Contains(x.ID)).Select(x => new DomPropertySettingCollection(x))); } - private void DeleteAttachments(ICollection apiSettingCollections) - { - foreach (var settingCollection in apiSettingCollections) - { - var instanceId = new DomInstanceId(settingCollection.Id); - - foreach (var setting in settingCollection.FileSettings.Concat(settingCollection.RemovedFileSettings)) - { - DeleteAttachments(instanceId, setting, setting.StoredFiles); - } - - settingCollection.ClearRemovedFileSettings(); - } - } - private void ValidateIdsNotInUse(ICollection apiSettingCollections) { if (apiSettingCollections == null) diff --git a/DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs b/DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs index 5fb4271e..a233a8b2 100644 --- a/DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs +++ b/DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs @@ -10,8 +10,8 @@ internal class InnerFilePropertySetting : FilePropertySetting private StorageProperties.PropertyValueSection originalSection; private StorageProperties.PropertyValueSection updatedSection; - internal InnerFilePropertySetting(FilePropertySetting filePropertySetting) - : base(filePropertySetting) + internal InnerFilePropertySetting(FilePropertySetting filePropertySetting, Guid destinationCollectionId) + : base(filePropertySetting, destinationCollectionId) { } diff --git a/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs b/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs index 964efec9..bf81693c 100644 --- a/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs +++ b/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs @@ -46,7 +46,7 @@ internal FilePropertySetting() { } - internal FilePropertySetting(FilePropertySetting filePropertySetting) + internal FilePropertySetting(FilePropertySetting filePropertySetting, Guid destinationCollectionId) : base(filePropertySetting) { files.AddRange(filePropertySetting.files); @@ -60,6 +60,8 @@ internal FilePropertySetting(FilePropertySetting filePropertySetting) { filesToDelete.Add(fileToDelete); } + + CopyStoredFiles(filePropertySetting, destinationCollectionId); } /// @@ -241,6 +243,31 @@ internal void SetStorageContext(MediaOpsPlanApi planApi, Guid settingCollectionI this.settingCollectionId = settingCollectionId; } + // A setting that stays in the collection holding its attachments keeps them; anywhere else the content has to travel along. + private void CopyStoredFiles(FilePropertySetting source, Guid destinationCollectionId) + { + if (source.storedFiles.Count == 0) + { + return; + } + + if (source.settingCollectionId == destinationCollectionId) + { + foreach (var storedFile in source.storedFiles) + { + storedFiles.Add(storedFile); + } + + SetStorageContext(source.planApi, source.settingCollectionId); + return; + } + + foreach (var storedFile in source.storedFiles.Where(x => !filesToUpload.ContainsKey(x))) + { + filesToUpload[storedFile] = source.ReadContent(storedFile); + } + } + // The file name is used as an attachment name, so any directory information is stripped off. private static string NormalizeFileName(string fileName) { diff --git a/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs b/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs index f6feb6dc..2647538d 100644 --- a/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs +++ b/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs @@ -233,7 +233,7 @@ public void Add(PropertySettingBase item) discreteSettings.Add(new InnerDiscretePropertySetting(discreteVal)); break; case FilePropertySetting fileVal: - fileSettings.Add(new InnerFilePropertySetting(fileVal)); + fileSettings.Add(new InnerFilePropertySetting(fileVal, Id)); break; default: throw new ArgumentException($"Unsupported property setting type '{item.GetType().Name}'.", nameof(item)); From 3054119991590073c2d87bfc4f0d0ea49328d8eb Mon Sep 17 00:00:00 2001 From: Jens Vandewalle Date: Wed, 12 Aug 2026 16:45:09 +0200 Subject: [PATCH 07/12] Improve file name validation and add unit tests Replaced Path methods with explicit handling of '/' and '\' as directory separators and a custom invalid character set for cross-platform consistency. Enhanced validation to reject empty, ".", "..", and control character file names. Added unit tests to verify stripping of directory info and exception handling for invalid names. --- .../Values/FilePropertySettingTests.cs | 18 ++++++++++++++++++ .../Values/Public/FilePropertySetting.cs | 17 +++++++++++++---- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs b/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs index 27114b3e..6548a6b1 100644 --- a/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs +++ b/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs @@ -57,6 +57,24 @@ public void AddFile_StripsDirectoryInformation() CollectionAssert.AreEqual(new[] { "document.pdf" }, setting.Files.ToArray()); } + [TestMethod] + public void AddFile_StripsUnixDirectoryInformation() + { + var setting = new FilePropertySetting(new FileProperty()); + + setting.AddFile("/tmp/../document.pdf", Content("abc")); + + CollectionAssert.AreEqual(new[] { "document.pdf" }, setting.Files.ToArray()); + } + + [TestMethod] + public void AddFile_RelativeName_Throws() + { + var setting = new FilePropertySetting(new FileProperty()); + + Assert.ThrowsException(() => setting.AddFile("..", Content("abc"))); + } + [TestMethod] public void AddFile_SameNameTwice_KeepsSingleEntry() { diff --git a/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs b/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs index bf81693c..673fd55d 100644 --- a/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs +++ b/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs @@ -2,7 +2,6 @@ namespace Skyline.DataMiner.Solutions.MediaOps.Plan.API { using System; using System.Collections.Generic; - using System.IO; using System.Linq; using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; @@ -20,6 +19,11 @@ public class FilePropertySetting : PropertySetting // The file names are stored as a single separated value, so the separator cannot be part of a file name. internal const char FileSeparator = '|'; + private static readonly char[] DirectorySeparators = new[] { '/', '\\' }; + + // Checked explicitly instead of through Path, because those characters depend on the platform this runs on. + private static readonly char[] InvalidFileNameCharacters = new[] { '<', '>', ':', '"', '/', '\\', '|', '?', '*' }; + private readonly List files = new List(); private readonly Dictionary filesToUpload = new Dictionary(StringComparer.OrdinalIgnoreCase); @@ -276,8 +280,13 @@ private static string NormalizeFileName(string fileName) throw new ArgumentException("The file name cannot be null or white space.", nameof(fileName)); } - var name = Path.GetFileName(fileName.Trim()); - if (string.IsNullOrWhiteSpace(name)) + var trimmed = fileName.Trim(); + + // Both separators are handled explicitly, because the attachment is stored on the server regardless of the platform this runs on. + var separatorIndex = trimmed.LastIndexOfAny(DirectorySeparators); + var name = separatorIndex == -1 ? trimmed : trimmed.Substring(separatorIndex + 1); + + if (string.IsNullOrWhiteSpace(name) || name == "." || name == "..") { throw new ArgumentException($"'{fileName}' is not a valid file name.", nameof(fileName)); } @@ -287,7 +296,7 @@ private static string NormalizeFileName(string fileName) throw new ArgumentException($"The file name cannot contain '{FileSeparator}'.", nameof(fileName)); } - if (name.IndexOfAny(Path.GetInvalidFileNameChars()) != -1) + if (name.IndexOfAny(InvalidFileNameCharacters) != -1 || name.Any(char.IsControl)) { throw new ArgumentException($"'{fileName}' is not a valid file name.", nameof(fileName)); } From 94b2fcde8947c22d9c92b5e0b155832ed6957375 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 12 Aug 2026 15:04:22 +0000 Subject: [PATCH 08/12] Initial plan From 357fbd7d5b0c3741055e6236b7329b4dbda2be2b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 12 Aug 2026 15:19:18 +0000 Subject: [PATCH 09/12] Fix orphaned attachments by reconciling against GetNames instead of tracking removed file settings Co-authored-by: JensVandewalle <102030104+JensVandewalle@users.noreply.github.com> --- .../DomPropertySettingCollectionHandler.cs | 39 ++++++------------- 1 file changed, 11 insertions(+), 28 deletions(-) diff --git a/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs b/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs index f19a2c05..16e40d3b 100644 --- a/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs +++ b/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs @@ -115,18 +115,22 @@ private void SyncAttachments(ICollection apiSettingCo { var instanceId = new DomInstanceId(settingCollection.Id); - // A dropped file setting can be re-added as a copy, so only the attachments that nothing refers to anymore are removed. + // Only the attachments that nothing refers to anymore are removed. Reconciling against the full stored + // attachment list catches orphans from removed file settings as well as settings that were never loaded. var expectedAttachments = new HashSet( settingCollection.FileSettings.SelectMany(x => x.Files.Select(f => FilePropertySetting.GetAttachmentName(x.Id, f))), StringComparer.OrdinalIgnoreCase); - foreach (var removedSetting in settingCollection.RemovedFileSettings) + foreach (var attachmentName in planApi.PropertyAttachments.GetNames(instanceId).Where(x => !expectedAttachments.Contains(x)).ToList()) { - var orphaned = removedSetting.StoredFiles - .Where(x => !expectedAttachments.Contains(FilePropertySetting.GetAttachmentName(removedSetting.Id, x))) - .ToList(); - - DeleteAttachments(instanceId, removedSetting, orphaned); + try + { + planApi.PropertyAttachments.Delete(instanceId, attachmentName); + } + catch (Exception ex) + { + planApi.Logger.Error(this, $"Failed to delete orphaned attachment '{attachmentName}': {ex}"); + } } settingCollection.ClearRemovedFileSettings(); @@ -143,11 +147,6 @@ private void SyncAttachments(ICollection apiSettingCo try { - foreach (var fileName in setting.FilesToDelete) - { - planApi.PropertyAttachments.Delete(instanceId, FilePropertySetting.GetAttachmentName(setting.Id, fileName)); - } - foreach (var fileToUpload in setting.FilesToUpload) { planApi.PropertyAttachments.Add(instanceId, FilePropertySetting.GetAttachmentName(setting.Id, fileToUpload.Key), fileToUpload.Value); @@ -168,22 +167,6 @@ private void SyncAttachments(ICollection apiSettingCo } } - // Cleaning up attachments must never fail the operation itself, so failures are only logged. - private void DeleteAttachments(DomInstanceId instanceId, FilePropertySetting setting, IEnumerable fileNames) - { - foreach (var fileName in fileNames) - { - try - { - planApi.PropertyAttachments.Delete(instanceId, FilePropertySetting.GetAttachmentName(setting.Id, fileName)); - } - catch (Exception ex) - { - planApi.Logger.Error(this, $"Failed to delete attachment '{fileName}' of property '{setting.Id}': {ex}"); - } - } - } - private ICollection CreateOrUpdateDomPropertySettingCollections(ICollection domValueCollections) { if (domValueCollections == null) From 3acaec05fa098bf8e1c5c048f97ebff11f0eef47 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 06:35:34 +0000 Subject: [PATCH 10/12] Revert merge of PR #269 (fix-code-review-suggestion) Co-authored-by: JensVandewalle <102030104+JensVandewalle@users.noreply.github.com> --- .../Values/FilePropertySettingTests.cs | 190 -------- .../Values/FilePropertySimulationTests.cs | 415 ------------------ .../Simulation/SimulatedDms.cs | 12 - .../Handlers/Properties/DomPropertyHandler.cs | 50 +-- .../DomPropertySettingCollectionHandler.cs | 73 +-- DevPack/API/MediaOpsPlanApi.cs | 9 - .../Properties/Definitions/FileProperty.cs | 4 +- .../Internal/InnerFilePropertySetting.cs | 65 --- .../Values/Public/FilePropertySetting.cs | 307 ------------- .../Public/PropertySettingCollection.cs | 66 +-- .../Validators/PropertySettingValidator.cs | 39 -- .../PropertyInvalidFileSizeLimitError.cs | 13 - .../DOM/Helpers/IPropertyAttachmentStore.cs | 18 - .../DOM/Helpers/PropertyAttachmentStore.cs | 37 -- 14 files changed, 9 insertions(+), 1289 deletions(-) delete mode 100644 DevPack.Tests/Properties/Values/FilePropertySettingTests.cs delete mode 100644 DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs delete mode 100644 DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs delete mode 100644 DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs delete mode 100644 DevPack/Exceptions/TraceData/Properties/Definitions/PropertyInvalidFileSizeLimitError.cs delete mode 100644 DevPack/Storage/DOM/Helpers/IPropertyAttachmentStore.cs delete mode 100644 DevPack/Storage/DOM/Helpers/PropertyAttachmentStore.cs diff --git a/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs b/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs deleted file mode 100644 index 6548a6b1..00000000 --- a/DevPack.Tests/Properties/Values/FilePropertySettingTests.cs +++ /dev/null @@ -1,190 +0,0 @@ -namespace RT_MediaOps.Plan.Properties.Values -{ - using System; - using System.Linq; - using System.Text; - - using Skyline.DataMiner.Solutions.MediaOps.Plan.API; - - [TestClass] - public sealed class FilePropertySettingTests - { - private static byte[] Content(string value) => Encoding.UTF8.GetBytes(value); - - [TestMethod] - public void Constructor_SetsPropertyId() - { - var id = Guid.NewGuid(); - - var setting = new FilePropertySetting(new FileProperty(id)); - - Assert.AreEqual(id, setting.Id); - } - - [TestMethod] - public void Constructor_NullProperty_Throws() - { - Assert.ThrowsException(() => new FilePropertySetting((FileProperty)null!)); - } - - [TestMethod] - public void NewSetting_HasNoValue() - { - var setting = new FilePropertySetting(new FileProperty()); - - Assert.IsFalse(setting.HasValue); - Assert.AreEqual(0, setting.Files.Count); - } - - [TestMethod] - public void AddFile_FileIsTrackedForUpload() - { - var setting = new FilePropertySetting(new FileProperty()); - - setting.AddFile("document.pdf", Content("abc")); - - Assert.IsTrue(setting.HasValue); - CollectionAssert.AreEqual(new[] { "document.pdf" }, setting.Files.ToArray()); - } - - [TestMethod] - public void AddFile_StripsDirectoryInformation() - { - var setting = new FilePropertySetting(new FileProperty()); - - setting.AddFile(@"C:\temp\..\document.pdf", Content("abc")); - - CollectionAssert.AreEqual(new[] { "document.pdf" }, setting.Files.ToArray()); - } - - [TestMethod] - public void AddFile_StripsUnixDirectoryInformation() - { - var setting = new FilePropertySetting(new FileProperty()); - - setting.AddFile("/tmp/../document.pdf", Content("abc")); - - CollectionAssert.AreEqual(new[] { "document.pdf" }, setting.Files.ToArray()); - } - - [TestMethod] - public void AddFile_RelativeName_Throws() - { - var setting = new FilePropertySetting(new FileProperty()); - - Assert.ThrowsException(() => setting.AddFile("..", Content("abc"))); - } - - [TestMethod] - public void AddFile_SameNameTwice_KeepsSingleEntry() - { - var setting = new FilePropertySetting(new FileProperty()); - - setting.AddFile("document.pdf", Content("first")); - setting.AddFile("document.pdf", Content("second")); - - Assert.AreEqual(1, setting.Files.Count); - } - - [TestMethod] - public void AddFile_NullContent_Throws() - { - var setting = new FilePropertySetting(new FileProperty()); - - Assert.ThrowsException(() => setting.AddFile("document.pdf", null!)); - } - - [TestMethod] - public void AddFile_EmptyName_Throws() - { - var setting = new FilePropertySetting(new FileProperty()); - - Assert.ThrowsException(() => setting.AddFile(" ", Content("abc"))); - } - - [TestMethod] - public void AddFile_NameContainingSeparator_Throws() - { - var setting = new FilePropertySetting(new FileProperty()); - - Assert.ThrowsException(() => setting.AddFile("first|second.pdf", Content("abc"))); - } - - [TestMethod] - public void AddFile_NameContainingInvalidCharacter_Throws() - { - var setting = new FilePropertySetting(new FileProperty()); - - Assert.ThrowsException(() => setting.AddFile("do.pdf", Content("abc"))); - } - - [TestMethod] - public void RemoveFile_RemovesFile() - { - var setting = new FilePropertySetting(new FileProperty()); - setting.AddFile("document.pdf", Content("abc")); - - setting.RemoveFile("document.pdf"); - - Assert.IsFalse(setting.HasValue); - Assert.AreEqual(0, setting.Files.Count); - } - - [TestMethod] - public void RemoveFile_UnknownFile_DoesNothing() - { - var setting = new FilePropertySetting(new FileProperty()); - setting.AddFile("document.pdf", Content("abc")); - - setting.RemoveFile("other.pdf"); - - CollectionAssert.AreEqual(new[] { "document.pdf" }, setting.Files.ToArray()); - } - - [TestMethod] - public void ClearFiles_RemovesAllFiles() - { - var setting = new FilePropertySetting(new FileProperty()); - setting.AddFile("first.pdf", Content("abc")); - setting.AddFile("second.pdf", Content("def")); - - setting.ClearFiles(); - - Assert.AreEqual(0, setting.Files.Count); - } - - [TestMethod] - public void Equals_SameFiles_ReturnsTrue() - { - var property = new FileProperty(Guid.NewGuid()); - - var first = new FilePropertySetting(property).AddFile("a.pdf", Content("a")); - var second = new FilePropertySetting(property).AddFile("a.pdf", Content("a")); - - Assert.AreEqual(first, second); - Assert.AreEqual(first.GetHashCode(), second.GetHashCode()); - } - - [TestMethod] - public void Equals_DifferentFiles_ReturnsFalse() - { - var property = new FileProperty(Guid.NewGuid()); - - var first = new FilePropertySetting(property).AddFile("a.pdf", Content("a")); - var second = new FilePropertySetting(property).AddFile("b.pdf", Content("a")); - - Assert.AreNotEqual(first, second); - } - - [TestMethod] - public void Equals_IgnoresFileOrder() - { - var property = new FileProperty(Guid.NewGuid()); - - var first = new FilePropertySetting(property).AddFile("a.pdf", Content("a")).AddFile("b.pdf", Content("b")); - var second = new FilePropertySetting(property).AddFile("b.pdf", Content("b")).AddFile("a.pdf", Content("a")); - - Assert.AreEqual(first, second); - } - } -} diff --git a/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs b/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs deleted file mode 100644 index cc29c65d..00000000 --- a/DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs +++ /dev/null @@ -1,415 +0,0 @@ -namespace RT_MediaOps.Plan.Properties.Values -{ - using System; - using System.Collections.Generic; - using System.Linq; - using System.Text; - - using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; - using Skyline.DataMiner.Solutions.MediaOps.Plan.API; - using Skyline.DataMiner.Solutions.MediaOps.Plan.Exceptions; - using Skyline.DataMiner.Solutions.MediaOps.Plan.Storage.DOM; - using Skyline.DataMiner.Solutions.MediaOps.Plan.UnitTesting.Simulation; - - /// - /// Deterministic, simulation-backed tests for file property values. The file content is stored through the attachment - /// API, which cannot be simulated, so a fake attachment store is used to verify what is uploaded and deleted. - /// - [TestClass] - public sealed class FilePropertySimulationTests - { - private const string Scope = "global"; - - private static byte[] Content(string value) => Encoding.UTF8.GetBytes(value); - - private static (IMediaOpsPlanApi Api, FakePropertyAttachmentStore Attachments) CreateContext() - { - var dms = MediaOpsPlanSimulation.Create(); - var connection = dms.CreateConnection(); - var api = connection.GetMediaOpsPlanApi(); - - var attachments = new FakePropertyAttachmentStore(); - ((MediaOpsPlanApi)api).PropertyAttachments = attachments; - - return (api, attachments); - } - - private static FileProperty CreateFileProperty(IMediaOpsPlanApi api, bool allowMultiple = false, bool hasSizeLimit = false, long sizeLimit = 20) - { - var property = new FileProperty(new PropertyData { Scope = Scope }) - { - Name = $"{Guid.NewGuid()}_Prop", - SectionName = "General", - AllowMultiple = allowMultiple, - HasSizeLimit = hasSizeLimit, - SizeLimit = sizeLimit, - }; - - return (FileProperty)api.Properties.Create(property); - } - - private static PropertySettingCollection CreateCollection() - { - return new PropertySettingCollection(new PropertySettingCollectionData - { - LinkedObjectId = $"obj-{Guid.NewGuid()}", - Scope = Scope, - SubId = string.Empty, - }); - } - - [TestMethod] - public void Create_WithFile_UploadsAttachmentAndStoresFileName() - { - var (api, attachments) = CreateContext(); - var property = CreateFileProperty(api); - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); - - var created = api.PropertySettingCollections.Create(collection); - - Assert.AreEqual(1, created.FileSettings.Count, "Expected the file setting to be stored."); - CollectionAssert.AreEqual(new[] { "document.pdf" }, created.FileSettings.Single().Files.ToArray()); - - var attachmentName = $"{property.Id}_document.pdf"; - Assert.IsTrue(attachments.Contains(collection.Id, attachmentName), "Expected the file content to be uploaded as an attachment."); - CollectionAssert.AreEqual(Content("hello"), attachments.Get(new DomInstanceId(collection.Id), attachmentName)); - } - - [TestMethod] - public void Read_AfterCreate_ReturnsFileNameWithoutAttachmentPrefix() - { - var (api, _) = CreateContext(); - var property = CreateFileProperty(api); - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); - api.PropertySettingCollections.Create(collection); - - var read = api.PropertySettingCollections.Read(collection.Id); - - Assert.IsNotNull(read); - CollectionAssert.AreEqual(new[] { "document.pdf" }, read.FileSettings.Single().Files.ToArray()); - } - - [TestMethod] - public void ReadContent_ReturnsUploadedContent() - { - var (api, _) = CreateContext(); - var property = CreateFileProperty(api); - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); - api.PropertySettingCollections.Create(collection); - - var read = api.PropertySettingCollections.Read(collection.Id); - var setting = read.FileSettings.Single(); - - CollectionAssert.AreEqual(Content("hello"), setting.ReadContent("document.pdf")); - } - - [TestMethod] - public void ReadContent_BeforeSaving_ReturnsPendingContent() - { - var (api, _) = CreateContext(); - var property = CreateFileProperty(api); - - var setting = new FilePropertySetting(property).AddFile("document.pdf", Content("hello")); - - CollectionAssert.AreEqual(Content("hello"), setting.ReadContent("document.pdf"), "Expected content that is not stored yet to be returned from memory."); - } - - [TestMethod] - public void ReadContent_UnknownFile_Throws() - { - var (api, _) = CreateContext(); - var property = CreateFileProperty(api); - - var setting = new FilePropertySetting(property).AddFile("document.pdf", Content("hello")); - - Assert.ThrowsException(() => setting.ReadContent("other.pdf")); - } - - [TestMethod] - public void ReadContent_OnReturnedCollection_UsesStoredContent() - { - var (api, attachments) = CreateContext(); - var property = CreateFileProperty(api); - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); - - var created = api.PropertySettingCollections.Create(collection); - - // The pending content is cleared once it is stored, so this has to come from the attachment. - CollectionAssert.AreEqual(Content("hello"), created.FileSettings.Single().ReadContent("document.pdf")); - Assert.IsTrue(attachments.Contains(collection.Id, $"{property.Id}_document.pdf")); - } - - [TestMethod] - public void Update_RemovingFile_DeletesAttachment() - { - var (api, attachments) = CreateContext(); - var property = CreateFileProperty(api); - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); - api.PropertySettingCollections.Create(collection); - - var read = api.PropertySettingCollections.Read(collection.Id); - read.FileSettings.Single().RemoveFile("document.pdf"); - var updated = api.PropertySettingCollections.Update(read); - - Assert.AreEqual(0, updated.FileSettings.Single().Files.Count, "Expected the file to be removed from the property."); - Assert.IsFalse(attachments.Contains(collection.Id, $"{property.Id}_document.pdf"), "Expected the attachment to be deleted."); - } - - [TestMethod] - public void Update_RetainingSettingsOfSameCollection_KeepsAttachmentsWithoutReupload() - { - var (api, attachments) = CreateContext(); - var property = CreateFileProperty(api); - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); - api.PropertySettingCollections.Create(collection); - - var read = api.PropertySettingCollections.Read(collection.Id); - read.SetPropertySettings(read.PropertySettings.ToList()); - - var updated = api.PropertySettingCollections.Update(read); - - CollectionAssert.AreEqual(new[] { "document.pdf" }, updated.FileSettings.Single().Files.ToArray()); - CollectionAssert.AreEqual(Content("hello"), updated.FileSettings.Single().ReadContent("document.pdf")); - Assert.IsTrue(attachments.Contains(collection.Id, $"{property.Id}_document.pdf")); - } - - [TestMethod] - public void Create_CopyingSettingFromOtherCollection_CopiesStoredContent() - { - var (api, attachments) = CreateContext(); - var property = CreateFileProperty(api); - - var source = CreateCollection(); - source.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); - api.PropertySettingCollections.Create(source); - - var readSource = api.PropertySettingCollections.Read(source.Id); - - var destination = CreateCollection(); - destination.Add(readSource.FileSettings.Single()); - var created = api.PropertySettingCollections.Create(destination); - - Assert.IsTrue( - attachments.Contains(destination.Id, $"{property.Id}_document.pdf"), - "Expected the stored content to be copied to the destination collection."); - CollectionAssert.AreEqual(Content("hello"), created.FileSettings.Single().ReadContent("document.pdf")); - } - - [TestMethod] - public void Update_ReplacedThenRemovedFile_DeletesStoredAttachment() - { - var (api, attachments) = CreateContext(); - var property = CreateFileProperty(api); - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); - api.PropertySettingCollections.Create(collection); - - var read = api.PropertySettingCollections.Read(collection.Id); - var setting = read.FileSettings.Single(); - - // Replacing the content and removing it again must still delete the file that is already stored. - setting.AddFile("document.pdf", Content("replaced")); - setting.RemoveFile("document.pdf"); - - api.PropertySettingCollections.Update(read); - - Assert.IsFalse(attachments.Contains(collection.Id, $"{property.Id}_document.pdf"), "Expected the stored attachment to be deleted."); - } - - [TestMethod] - public void Update_RemovingWholeFileSetting_DeletesAttachments() - { - var (api, attachments) = CreateContext(); - var property = CreateFileProperty(api); - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); - api.PropertySettingCollections.Create(collection); - - var read = api.PropertySettingCollections.Read(collection.Id); - read.Remove(read.FileSettings.Single()); - - api.PropertySettingCollections.Update(read); - - Assert.IsFalse(attachments.Contains(collection.Id, $"{property.Id}_document.pdf"), "Expected the attachments of a removed file setting to be deleted."); - } - - [TestMethod] - public void Update_ClearingSettings_DeletesAttachments() - { - var (api, attachments) = CreateContext(); - var property = CreateFileProperty(api); - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); - api.PropertySettingCollections.Create(collection); - - var read = api.PropertySettingCollections.Read(collection.Id); - read.SetPropertySettings(null); - - api.PropertySettingCollections.Update(read); - - Assert.IsFalse(attachments.Contains(collection.Id, $"{property.Id}_document.pdf"), "Expected the attachments to be deleted when the settings are replaced."); - } - - [TestMethod] - public void Create_WhenAttachmentUploadFails_ReportsErrorInsteadOfThrowingUnexpectedly() - { - var (api, attachments) = CreateContext(); - var property = CreateFileProperty(api); - - attachments.FailOnAdd = true; - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property).AddFile("document.pdf", Content("hello"))); - - var exception = Assert.ThrowsException(() => api.PropertySettingCollections.Create(collection)); - Assert.IsTrue( - exception.TraceData.ErrorData.OfType().Any(), - "Expected the attachment failure to be reported as a structured error."); - } - - [TestMethod] - public void Create_MultipleFilesWhileNotAllowed_ThrowsInvalidPropertySettingsError() - { - var (api, _) = CreateContext(); - var property = CreateFileProperty(api, allowMultiple: false); - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property) - .AddFile("first.pdf", Content("a")) - .AddFile("second.pdf", Content("b"))); - - var exception = Assert.ThrowsException(() => api.PropertySettingCollections.Create(collection)); - Assert.IsTrue( - exception.TraceData.ErrorData.OfType().Any(), - "Expected an invalid property settings error when multiple files are not allowed."); - } - - [TestMethod] - public void Create_FileExceedingSizeLimit_ThrowsInvalidPropertySettingsError() - { - var (api, _) = CreateContext(); - var property = CreateFileProperty(api, hasSizeLimit: true, sizeLimit: 1); - - var collection = CreateCollection(); - collection.Add(new FilePropertySetting(property).AddFile("big.bin", new byte[2 * 1024 * 1024])); - - var exception = Assert.ThrowsException(() => api.PropertySettingCollections.Create(collection)); - Assert.IsTrue( - exception.TraceData.ErrorData.OfType().Any(), - "Expected an invalid property settings error when a file exceeds the size limit."); - } - - [TestMethod] - public void CreateProperty_SizeLimitBelowOne_ThrowsInvalidFileSizeLimitError() - { - var (api, _) = CreateContext(); - - var property = new FileProperty(new PropertyData { Scope = Scope }) - { - Name = $"{Guid.NewGuid()}_Prop", - SectionName = "General", - HasSizeLimit = true, - SizeLimit = 0, - }; - - var exception = Assert.ThrowsException(() => api.Properties.Create(property)); - Assert.IsTrue( - exception.TraceData.ErrorData.OfType().Any(), - "Expected a PropertyInvalidFileSizeLimitError when the size limit is not positive."); - } - - [TestMethod] - public void CreateProperty_SizeLimitAboveServerMaximum_ThrowsInvalidFileSizeLimitError() - { - var (api, _) = CreateContext(); - - var property = new FileProperty(new PropertyData { Scope = Scope }) - { - Name = $"{Guid.NewGuid()}_Prop", - SectionName = "General", - HasSizeLimit = true, - SizeLimit = 10000, - }; - - var exception = Assert.ThrowsException(() => api.Properties.Create(property)); - Assert.IsTrue( - exception.TraceData.ErrorData.OfType().Any(), - "Expected a PropertyInvalidFileSizeLimitError when the size limit exceeds the server maximum."); - } - - [TestMethod] - public void CreateProperty_WithoutSizeLimit_RoundTripsHasSizeLimit() - { - var (api, _) = CreateContext(); - - var property = CreateFileProperty(api, hasSizeLimit: false); - - var read = (FileProperty)api.Properties.Read(property.Id); - - Assert.IsFalse(read.HasSizeLimit, "Expected a property without its own size limit to keep using the server limit."); - } - - private sealed class FakePropertyAttachmentStore : IPropertyAttachmentStore - { - private readonly Dictionary> attachments = new Dictionary>(); - - public bool FailOnAdd { get; set; } - - public void Add(DomInstanceId instanceId, string attachmentName, byte[] content) - { - if (FailOnAdd) - { - throw new InvalidOperationException("Simulated upload failure."); - } - - if (!attachments.TryGetValue(instanceId.Id, out var perInstance)) - { - perInstance = new Dictionary(StringComparer.OrdinalIgnoreCase); - attachments[instanceId.Id] = perInstance; - } - - perInstance[attachmentName] = content; - } - - public byte[] Get(DomInstanceId instanceId, string attachmentName) - { - return attachments.TryGetValue(instanceId.Id, out var perInstance) && perInstance.TryGetValue(attachmentName, out var content) - ? content - : throw new InvalidOperationException($"Attachment '{attachmentName}' was not found."); - } - - public void Delete(DomInstanceId instanceId, string attachmentName) - { - if (attachments.TryGetValue(instanceId.Id, out var perInstance)) - { - perInstance.Remove(attachmentName); - } - } - - public IReadOnlyCollection GetNames(DomInstanceId instanceId) - { - return attachments.TryGetValue(instanceId.Id, out var perInstance) ? perInstance.Keys.ToList() : new List(); - } - - public bool Contains(Guid instanceId, string attachmentName) - { - return attachments.TryGetValue(instanceId, out var perInstance) && perInstance.ContainsKey(attachmentName); - } - } - } -} diff --git a/DevPack.UnitTesting/Simulation/SimulatedDms.cs b/DevPack.UnitTesting/Simulation/SimulatedDms.cs index 80960a6c..9c6f3486 100644 --- a/DevPack.UnitTesting/Simulation/SimulatedDms.cs +++ b/DevPack.UnitTesting/Simulation/SimulatedDms.cs @@ -51,11 +51,6 @@ public SimulatedDms() /// public DomSLNetMessageHandler DomHandler => _domSlNetMessageHandler; - /// - /// Gets or sets the maximum document size, in MB, reported by the simulated agent. - /// - public int MaxDocumentSizeInMegaBytes { get; set; } = 100; - /// /// Registers an installed application package so that installation checks succeed. /// @@ -438,13 +433,6 @@ private IEnumerable HandleMessage(GetInfoMessage msg) break; - case InfoType.GeneralInfoMessage: - yield return new GeneralInfoEventMessage - { - MaxDocumentSize = MaxDocumentSizeInMegaBytes, - }; - break; - default: throw new NotSupportedException($"Unsupported InfoType: {msg.Type}"); } diff --git a/DevPack/API/Handlers/Properties/DomPropertyHandler.cs b/DevPack/API/Handlers/Properties/DomPropertyHandler.cs index e0f22e79..1f230c7e 100644 --- a/DevPack/API/Handlers/Properties/DomPropertyHandler.cs +++ b/DevPack/API/Handlers/Properties/DomPropertyHandler.cs @@ -5,7 +5,6 @@ namespace Skyline.DataMiner.Solutions.MediaOps.Plan.API using System.Linq; using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; - using Skyline.DataMiner.Net.Messages; using Skyline.DataMiner.Net.Messages.SLDataGateway; using Skyline.DataMiner.Solutions.MediaOps.Plan.Exceptions; using Skyline.DataMiner.Solutions.MediaOps.Plan.Storage.DOM; @@ -557,54 +556,7 @@ private void ValidateFileProperties(ICollection apiProperties) return; } - // Only properties that define their own limit are validated; the others fall back to the server limit. - var propertiesWithSizeLimit = apiProperties.Where(x => x.HasSizeLimit).ToList(); - if (propertiesWithSizeLimit.Count == 0) - { - return; - } - - var maxSizeLimit = GetMaxDocumentSizeInMegaBytes(); - - foreach (var property in propertiesWithSizeLimit) - { - if (property.SizeLimit <= 0) - { - var error = new PropertyInvalidFileSizeLimitError - { - ErrorMessage = "Size limit must be greater than 0.", - SizeLimit = property.SizeLimit, - Id = property.Id, - }; - - ReportError(property.Id, error); - continue; - } - - if (maxSizeLimit.HasValue && property.SizeLimit > maxSizeLimit.Value) - { - var error = new PropertyInvalidFileSizeLimitError - { - ErrorMessage = $"Size limit cannot exceed the maximum file size allowed by DataMiner ({maxSizeLimit.Value} MB).", - SizeLimit = property.SizeLimit, - Id = property.Id, - }; - - ReportError(property.Id, error); - } - } - } - - // Returns the file size limit configured on the server, in MB, or null when it is not exposed. - private long? GetMaxDocumentSizeInMegaBytes() - { - var response = planApi.Connection.HandleSingleResponseMessage(new GetInfoMessage(InfoType.GeneralInfoMessage)) as GeneralInfoEventMessage; - if (response == null || response.MaxDocumentSize <= 0) - { - return null; - } - - return response.MaxDocumentSize; + throw new NotImplementedException(); } private void ValidateStateForDeleteAction(ICollection apiProperties) diff --git a/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs b/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs index 16e40d3b..75a31b59 100644 --- a/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs +++ b/DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs @@ -100,74 +100,10 @@ private void CreateOrUpdateLocked(ICollection apiSett .Where(IsValid) .Select(x => new DomPropertySettingCollection(x.Instance)) .ToList(); - var persistedInstances = CreateOrUpdateDomPropertySettingCollections(toCreateDomInstances.Concat(toUpdateDomInstances).ToList()); - - // The file content is attached to the DOM instance, so it can only be stored once that instance exists. - SyncAttachments(apiSettingCollections.Where(IsValid).ToList()); - - // Reported last, because a collection whose files could not be stored must not be reported as successful. - ReportSuccess(persistedInstances.Where(x => !TraceDataPerItem.ContainsKey(x.ID.Id))); - } - - private void SyncAttachments(ICollection apiSettingCollections) - { - foreach (var settingCollection in apiSettingCollections) - { - var instanceId = new DomInstanceId(settingCollection.Id); - - // Only the attachments that nothing refers to anymore are removed. Reconciling against the full stored - // attachment list catches orphans from removed file settings as well as settings that were never loaded. - var expectedAttachments = new HashSet( - settingCollection.FileSettings.SelectMany(x => x.Files.Select(f => FilePropertySetting.GetAttachmentName(x.Id, f))), - StringComparer.OrdinalIgnoreCase); - - foreach (var attachmentName in planApi.PropertyAttachments.GetNames(instanceId).Where(x => !expectedAttachments.Contains(x)).ToList()) - { - try - { - planApi.PropertyAttachments.Delete(instanceId, attachmentName); - } - catch (Exception ex) - { - planApi.Logger.Error(this, $"Failed to delete orphaned attachment '{attachmentName}': {ex}"); - } - } - - settingCollection.ClearRemovedFileSettings(); - - foreach (var setting in settingCollection.FileSettings) - { - // The collection exists from here on, so the content of its files can be read on demand. - setting.SetStorageContext(planApi, settingCollection.Id); - - if (setting.FilesToUpload.Count == 0 && setting.FilesToDelete.Count == 0) - { - continue; - } - - try - { - foreach (var fileToUpload in setting.FilesToUpload) - { - planApi.PropertyAttachments.Add(instanceId, FilePropertySetting.GetAttachmentName(setting.Id, fileToUpload.Key), fileToUpload.Value); - } - - setting.ClearPendingFileChanges(); - } - catch (Exception ex) - { - ReportError(settingCollection.Id, new PropertySettingCollectionInvalidPropertySettingsError - { - ErrorMessage = $"The files of the property could not be stored: {ex.Message}", - PropertyId = setting.Id, - Id = settingCollection.Id, - }); - } - } - } + CreateOrUpdateDomPropertySettingCollections(toCreateDomInstances.Concat(toUpdateDomInstances).ToList()); } - private ICollection CreateOrUpdateDomPropertySettingCollections(ICollection domValueCollections) + private void CreateOrUpdateDomPropertySettingCollections(ICollection domValueCollections) { if (domValueCollections == null) { @@ -176,7 +112,7 @@ private ICollection CreateOrUpdateDomPropertySetti if (domValueCollections.Count == 0) { - return new List(); + return; } planApi.DomHelpers.SlcPropertiesHelper.DomHelper.DomInstances.TryCreateOrUpdateInBatches(domValueCollections.Select(x => x.ToInstance()), out var domResult); @@ -194,7 +130,7 @@ private ICollection CreateOrUpdateDomPropertySetti } } - return domResult.SuccessfulItems.Select(x => new DomPropertySettingCollection(x)).ToList(); + ReportSuccess(domResult.SuccessfulItems.Select(x => new DomPropertySettingCollection(x))); } private void Delete(ICollection apiSettingCollections) @@ -232,7 +168,6 @@ private void DeleteLocked(ICollection apiSettingColle throw new ArgumentException($"Not all provided property value collections are valid", nameof(apiSettingCollections)); } - // The server removes the attachments together with the instance, so they need no separate cleanup here. var toDelete = apiSettingCollections.Select(x => x.OriginalInstance.ToInstance()); planApi.DomHelpers.SlcPropertiesHelper.DomHelper.DomInstances.TryDeleteInBatches(toDelete, out var domResult); diff --git a/DevPack/API/MediaOpsPlanApi.cs b/DevPack/API/MediaOpsPlanApi.cs index 2141d98a..fe511dad 100644 --- a/DevPack/API/MediaOpsPlanApi.cs +++ b/DevPack/API/MediaOpsPlanApi.cs @@ -49,8 +49,6 @@ public class MediaOpsPlanApi : IMediaOpsPlanApi private ILogger logger; - private IPropertyAttachmentStore propertyAttachments; - /// /// Initializes a new instance of the class. /// @@ -153,13 +151,6 @@ internal MediaOpsPlanApi(IConnection connection) internal DomHelpers DomHelpers => domHelpers; - // Settable so the attachment handling can be verified without a DataMiner Agent. - internal IPropertyAttachmentStore PropertyAttachments - { - get => propertyAttachments ?? (propertyAttachments = new PropertyAttachmentStore(domHelpers.SlcPropertiesHelper)); - set => propertyAttachments = value; - } - internal CoreHelpers CoreHelpers => coreHelpers; internal IDms Dms => lazyDms.Value; diff --git a/DevPack/API/Objects/Properties/Definitions/FileProperty.cs b/DevPack/API/Objects/Properties/Definitions/FileProperty.cs index 047eabb5..2cddff0d 100644 --- a/DevPack/API/Objects/Properties/Definitions/FileProperty.cs +++ b/DevPack/API/Objects/Properties/Definitions/FileProperty.cs @@ -96,9 +96,7 @@ public override bool Equals(object obj) internal override void ApplyChanges(StorageProperties.PropertyInstance instance) { instance.PropertyInfo.PropertyType = StorageProperties.SlcPropertiesIds.Enums.PropertytypeEnum.File; - - // A zero size limit signals that the limit configured on the server applies. - instance.PropertyInfo.FileSizeLimit = HasSizeLimit ? SizeLimit : 0; + instance.PropertyInfo.FileSizeLimit = SizeLimit; instance.PropertyInfo.AllowMultipleFiles = AllowMultiple; } diff --git a/DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs b/DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs deleted file mode 100644 index a233a8b2..00000000 --- a/DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs +++ /dev/null @@ -1,65 +0,0 @@ -namespace Skyline.DataMiner.Solutions.MediaOps.Plan.API -{ - using System; - using System.Linq; - - using StorageProperties = Storage.DOM.SlcProperties; - - internal class InnerFilePropertySetting : FilePropertySetting - { - private StorageProperties.PropertyValueSection originalSection; - private StorageProperties.PropertyValueSection updatedSection; - - internal InnerFilePropertySetting(FilePropertySetting filePropertySetting, Guid destinationCollectionId) - : base(filePropertySetting, destinationCollectionId) - { - } - - internal InnerFilePropertySetting(MediaOpsPlanApi planApi, Guid settingCollectionId, StorageProperties.PropertyValueSection section) - { - ParseSection(section); - SetStorageContext(planApi, settingCollectionId); - InitTracking(); - } - - internal override Storage.DOM.DomSectionBase OriginalSection => originalSection; - - internal StorageProperties.PropertyValueSection GetSectionWithChanges() - { - if (updatedSection == null) - { - updatedSection = IsNew ? new StorageProperties.PropertyValueSection() : originalSection.Clone(); - } - - updatedSection.PropertyID = Id; - updatedSection.Value = string.Join(FileSeparator.ToString(), Files); - - return updatedSection; - } - - private void ParseSection(StorageProperties.PropertyValueSection section) - { - originalSection = section ?? throw new ArgumentNullException(nameof(section)); - - Id = section.PropertyID.Value; - - if (string.IsNullOrEmpty(section.Value)) - { - return; - } - - foreach (var entry in section.Value.Split(new[] { FileSeparator }, StringSplitOptions.RemoveEmptyEntries)) - { - AddParsedFile(StripAttachmentPrefix(entry)); - } - } - - // Values written by older versions store the attachment name instead of the file name. - private string StripAttachmentPrefix(string entry) - { - var prefix = $"{Id}_"; - - return entry.StartsWith(prefix, StringComparison.OrdinalIgnoreCase) ? entry.Substring(prefix.Length) : entry; - } - } -} diff --git a/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs b/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs deleted file mode 100644 index 673fd55d..00000000 --- a/DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs +++ /dev/null @@ -1,307 +0,0 @@ -namespace Skyline.DataMiner.Solutions.MediaOps.Plan.API -{ - using System; - using System.Collections.Generic; - using System.Linq; - - using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; - using Skyline.DataMiner.Net.Helper; - - /// - /// Represents a property value that holds one or more files. - /// - /// - /// The file content is stored as an attachment on the property value collection. Content that is added or removed is - /// only persisted when the property value collection is saved. - /// - public class FilePropertySetting : PropertySetting - { - // The file names are stored as a single separated value, so the separator cannot be part of a file name. - internal const char FileSeparator = '|'; - - private static readonly char[] DirectorySeparators = new[] { '/', '\\' }; - - // Checked explicitly instead of through Path, because those characters depend on the platform this runs on. - private static readonly char[] InvalidFileNameCharacters = new[] { '<', '>', ':', '"', '/', '\\', '|', '?', '*' }; - - private readonly List files = new List(); - - private readonly Dictionary filesToUpload = new Dictionary(StringComparer.OrdinalIgnoreCase); - - private readonly HashSet filesToDelete = new HashSet(StringComparer.OrdinalIgnoreCase); - - // The files that are known to be stored as an attachment, so a removal knows whether it has to delete one. - private readonly HashSet storedFiles = new HashSet(StringComparer.OrdinalIgnoreCase); - - private MediaOpsPlanApi planApi; - - private Guid settingCollectionId; - - /// - /// Initializes a new instance of the class linked to the specified file property. - /// - /// The definition to link to. - public FilePropertySetting(FileProperty property) - : base(property) - { - } - - internal FilePropertySetting() - { - } - - internal FilePropertySetting(FilePropertySetting filePropertySetting, Guid destinationCollectionId) - : base(filePropertySetting) - { - files.AddRange(filePropertySetting.files); - - foreach (var fileToUpload in filePropertySetting.filesToUpload) - { - filesToUpload[fileToUpload.Key] = fileToUpload.Value; - } - - foreach (var fileToDelete in filePropertySetting.filesToDelete) - { - filesToDelete.Add(fileToDelete); - } - - CopyStoredFiles(filePropertySetting, destinationCollectionId); - } - - /// - /// Gets the names of the files of this property. - /// - public IReadOnlyCollection Files => files; - - /// - public override bool HasValue => files.Count > 0; - - internal IReadOnlyDictionary FilesToUpload => filesToUpload; - - internal IReadOnlyCollection FilesToDelete => filesToDelete; - - internal IReadOnlyCollection StoredFiles => storedFiles; - - // The attachment holding the content of a file is named after the property it belongs to. - internal static string GetAttachmentName(Guid propertyId, string fileName) - { - return $"{propertyId}_{fileName}"; - } - - /// - /// Reads the content of the specified file. - /// - /// The name of the file to read. - /// The content of the file. - /// The content of a stored file is retrieved on demand, so it is not held in memory while the file names are used. - /// Thrown when is or white space, or is not a file of this property. - /// Thrown when the property value collection holding this file was never read or saved. - public byte[] ReadContent(string fileName) - { - var name = NormalizeFileName(fileName); - - // A file that is not stored yet is still held in memory. - if (filesToUpload.TryGetValue(name, out var content)) - { - return content; - } - - if (!files.Contains(name, StringComparer.OrdinalIgnoreCase)) - { - throw new ArgumentException($"File '{name}' is not a file of this property.", nameof(fileName)); - } - - if (planApi == null || settingCollectionId == Guid.Empty) - { - throw new InvalidOperationException("The content can only be read for a property value collection that was read or saved."); - } - - return planApi.PropertyAttachments.Get(new DomInstanceId(settingCollectionId), GetAttachmentName(Id, name)); - } - - /// - /// Adds a file to this property, or replaces the content when a file with the same name was already added. - /// - /// The name of the file. - /// The content of the file. - /// This , so calls can be chained. - /// Thrown when is or white space. - /// Thrown when is . - public FilePropertySetting AddFile(string fileName, byte[] content) - { - var name = NormalizeFileName(fileName); - - if (content == null) - { - throw new ArgumentNullException(nameof(content)); - } - - filesToDelete.Remove(name); - filesToUpload[name] = content; - - if (!files.Contains(name, StringComparer.OrdinalIgnoreCase)) - { - files.Add(name); - } - - return this; - } - - /// - /// Removes the specified file from this property. - /// - /// The name of the file to remove. - /// This , so calls can be chained. - /// Thrown when is or white space. - public FilePropertySetting RemoveFile(string fileName) - { - var name = NormalizeFileName(fileName); - - var storedName = files.FirstOrDefault(x => string.Equals(x, name, StringComparison.OrdinalIgnoreCase)); - if (storedName == null) - { - return this; - } - - files.Remove(storedName); - filesToUpload.Remove(storedName); - - // Only a file that is actually stored has an attachment that must be deleted. - if (storedFiles.Contains(storedName)) - { - filesToDelete.Add(storedName); - } - - return this; - } - - /// - /// Removes all files from this property. - /// - /// This , so calls can be chained. - public FilePropertySetting ClearFiles() - { - foreach (var fileName in files.ToArray()) - { - RemoveFile(fileName); - } - - return this; - } - - /// - public override int GetHashCode() - { - unchecked - { - var hash = base.GetHashCode(); - - foreach (var fileName in files.OrderBy(x => x).ToArray()) - { - hash = (hash * 23) + (fileName != null ? fileName.GetHashCode() : 0); - } - - return hash; - } - } - - /// - public override bool Equals(object obj) - { - if (obj is not FilePropertySetting other) - { - return false; - } - - return base.Equals(other) - && files.ScrambledEquals(other.files); - } - - internal void AddParsedFile(string fileName) - { - if (!files.Contains(fileName, StringComparer.OrdinalIgnoreCase)) - { - files.Add(fileName); - } - - storedFiles.Add(fileName); - } - - internal void ClearPendingFileChanges() - { - filesToUpload.Clear(); - filesToDelete.Clear(); - - // Everything that is left is stored from here on. - storedFiles.Clear(); - foreach (var fileName in files) - { - storedFiles.Add(fileName); - } - } - - // Captures where the content of the files is stored, so it can be read on demand. - internal void SetStorageContext(MediaOpsPlanApi planApi, Guid settingCollectionId) - { - this.planApi = planApi; - this.settingCollectionId = settingCollectionId; - } - - // A setting that stays in the collection holding its attachments keeps them; anywhere else the content has to travel along. - private void CopyStoredFiles(FilePropertySetting source, Guid destinationCollectionId) - { - if (source.storedFiles.Count == 0) - { - return; - } - - if (source.settingCollectionId == destinationCollectionId) - { - foreach (var storedFile in source.storedFiles) - { - storedFiles.Add(storedFile); - } - - SetStorageContext(source.planApi, source.settingCollectionId); - return; - } - - foreach (var storedFile in source.storedFiles.Where(x => !filesToUpload.ContainsKey(x))) - { - filesToUpload[storedFile] = source.ReadContent(storedFile); - } - } - - // The file name is used as an attachment name, so any directory information is stripped off. - private static string NormalizeFileName(string fileName) - { - if (string.IsNullOrWhiteSpace(fileName)) - { - throw new ArgumentException("The file name cannot be null or white space.", nameof(fileName)); - } - - var trimmed = fileName.Trim(); - - // Both separators are handled explicitly, because the attachment is stored on the server regardless of the platform this runs on. - var separatorIndex = trimmed.LastIndexOfAny(DirectorySeparators); - var name = separatorIndex == -1 ? trimmed : trimmed.Substring(separatorIndex + 1); - - if (string.IsNullOrWhiteSpace(name) || name == "." || name == "..") - { - throw new ArgumentException($"'{fileName}' is not a valid file name.", nameof(fileName)); - } - - if (name.IndexOf(FileSeparator) != -1) - { - throw new ArgumentException($"The file name cannot contain '{FileSeparator}'.", nameof(fileName)); - } - - if (name.IndexOfAny(InvalidFileNameCharacters) != -1 || name.Any(char.IsControl)) - { - throw new ArgumentException($"'{fileName}' is not a valid file name.", nameof(fileName)); - } - - return name; - } - } -} diff --git a/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs b/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs index 2647538d..4dbafbfa 100644 --- a/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs +++ b/DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs @@ -18,10 +18,6 @@ public class PropertySettingCollection : ApiObject, ICollection stringSettings = []; private readonly List booleanSettings = []; private readonly List discreteSettings = []; - private readonly List fileSettings = []; - - // File settings that are dropped still own attachments, so they are kept until those are deleted. - private readonly List removedFileSettings = []; private StorageProperties.PropertyValuesInstance originalInstance; private StorageProperties.PropertyValuesInstance updatedInstance; @@ -119,7 +115,7 @@ internal PropertySettingCollection(MediaOpsPlanApi planApi, StorageProperties.Pr /// /// Gets the collection of property settings linked to a property definition. /// - public IReadOnlyCollection PropertySettings => stringSettings.Cast().Concat(booleanSettings).Concat(discreteSettings).Concat(fileSettings).ToList(); + public IReadOnlyCollection PropertySettings => stringSettings.Cast().Concat(booleanSettings).Concat(discreteSettings).ToList(); /// /// Gets the collection of string property settings. @@ -136,21 +132,14 @@ internal PropertySettingCollection(MediaOpsPlanApi planApi, StorageProperties.Pr /// public IReadOnlyCollection DiscreteSettings => discreteSettings; - /// - /// Gets the collection of file property settings. - /// - public IReadOnlyCollection FileSettings => fileSettings; - /// - public int Count => customSettings.Count + stringSettings.Count + booleanSettings.Count + discreteSettings.Count + fileSettings.Count; + public int Count => customSettings.Count + stringSettings.Count + booleanSettings.Count + discreteSettings.Count; /// public bool IsReadOnly => false; internal StorageProperties.PropertyValuesInstance OriginalInstance => originalInstance; - internal IReadOnlyCollection RemovedFileSettings => removedFileSettings; - /// public override int GetHashCode() { @@ -182,11 +171,6 @@ public override int GetHashCode() hash = (hash * 23) + value.GetHashCode(); } - foreach (var value in fileSettings.OrderBy(x => x.Id)) - { - hash = (hash * 23) + value.GetHashCode(); - } - return hash; } } @@ -206,8 +190,7 @@ public override bool Equals(object obj) && customSettings.ScrambledEquals(other.customSettings) && stringSettings.ScrambledEquals(other.stringSettings) && booleanSettings.ScrambledEquals(other.booleanSettings) - && discreteSettings.ScrambledEquals(other.discreteSettings) - && fileSettings.ScrambledEquals(other.fileSettings); + && discreteSettings.ScrambledEquals(other.discreteSettings); } /// @@ -232,9 +215,6 @@ public void Add(PropertySettingBase item) case DiscretePropertySetting discreteVal: discreteSettings.Add(new InnerDiscretePropertySetting(discreteVal)); break; - case FilePropertySetting fileVal: - fileSettings.Add(new InnerFilePropertySetting(fileVal, Id)); - break; default: throw new ArgumentException($"Unsupported property setting type '{item.GetType().Name}'.", nameof(item)); } @@ -247,8 +227,6 @@ public void Clear() stringSettings.Clear(); booleanSettings.Clear(); discreteSettings.Clear(); - TrackRemovedFileSettings(fileSettings); - fileSettings.Clear(); } /// @@ -283,8 +261,6 @@ public void SetPropertySettings(IEnumerable settings) stringSettings.Clear(); booleanSettings.Clear(); discreteSettings.Clear(); - TrackRemovedFileSettings(fileSettings); - fileSettings.Clear(); if (settings == null) { @@ -311,7 +287,6 @@ public bool Contains(PropertySettingBase item) StringPropertySetting stringVal => stringSettings.Contains(stringVal), BooleanPropertySetting boolVal => booleanSettings.Contains(boolVal), DiscretePropertySetting discreteVal => discreteSettings.Contains(discreteVal), - FilePropertySetting fileVal => fileSettings.Contains(fileVal), _ => false, }; } @@ -354,30 +329,10 @@ public bool Remove(PropertySettingBase item) StringPropertySetting stringVal => stringSettings.RemoveAll(x => x.Equals(stringVal)) > 0, BooleanPropertySetting boolVal => booleanSettings.RemoveAll(x => x.Equals(boolVal)) > 0, DiscretePropertySetting discreteVal => discreteSettings.RemoveAll(x => x.Equals(discreteVal)) > 0, - FilePropertySetting fileVal => RemoveFileSetting(fileVal), _ => false, }; } - private bool RemoveFileSetting(FilePropertySetting fileSetting) - { - var toRemove = fileSettings.Where(x => x.Equals(fileSetting)).ToList(); - if (toRemove.Count == 0) - { - return false; - } - - TrackRemovedFileSettings(toRemove); - toRemove.ForEach(x => fileSettings.Remove(x)); - - return true; - } - - private void TrackRemovedFileSettings(IEnumerable settings) - { - removedFileSettings.AddRange(settings.Where(x => x.StoredFiles.Count != 0)); - } - /// public IEnumerator GetEnumerator() { @@ -386,7 +341,6 @@ public IEnumerator GetEnumerator() .Concat(stringSettings) .Concat(booleanSettings) .Concat(discreteSettings) - .Concat(fileSettings) .GetEnumerator(); } @@ -428,19 +382,9 @@ internal StorageProperties.PropertyValuesInstance GetInstanceWithChanges() updatedInstance.PropertyValue.Add(discreteSetting.GetSectionWithChanges()); } - foreach (var fileSetting in fileSettings) - { - updatedInstance.PropertyValue.Add(fileSetting.GetSectionWithChanges()); - } - return updatedInstance; } - internal void ClearRemovedFileSettings() - { - removedFileSettings.Clear(); - } - private void ParseInstance(MediaOpsPlanApi planApi, StorageProperties.PropertyValuesInstance instance) { originalInstance = instance ?? throw new ArgumentNullException(nameof(instance)); @@ -491,10 +435,6 @@ private void ParsePropertyValues(MediaOpsPlanApi planApi, IList 1) - { - ReportError(apiObjectId, ComposePropertySettingError(propertySetting.Id, "This property does not allow multiple files.")); - } - - if (!fileProperty.HasSizeLimit) - { - return; - } - - var sizeLimitInBytes = fileProperty.SizeLimit * 1024L * 1024L; - foreach (var fileToUpload in setting.FilesToUpload.Where(x => x.Value.LongLength > sizeLimitInBytes)) - { - ReportError(apiObjectId, ComposePropertySettingError(propertySetting.Id, $"File '{fileToUpload.Key}' exceeds the maximum file size of {fileProperty.SizeLimit} MB.")); - } - } - private MediaOpsErrorData ComposePropertySettingError(Guid propertyId, string errorMessage) { return new PropertySettingCollectionInvalidPropertySettingsError diff --git a/DevPack/Exceptions/TraceData/Properties/Definitions/PropertyInvalidFileSizeLimitError.cs b/DevPack/Exceptions/TraceData/Properties/Definitions/PropertyInvalidFileSizeLimitError.cs deleted file mode 100644 index f69a517b..00000000 --- a/DevPack/Exceptions/TraceData/Properties/Definitions/PropertyInvalidFileSizeLimitError.cs +++ /dev/null @@ -1,13 +0,0 @@ -namespace Skyline.DataMiner.Solutions.MediaOps.Plan.Exceptions -{ - /// - /// Represents an error that occurs when a file property is configured with an invalid size limit, such as a value that is not positive or a value that exceeds the maximum file size allowed by DataMiner. - /// - public sealed class PropertyInvalidFileSizeLimitError : PropertyError - { - /// - /// Gets the configured size limit, in MB, that caused the error. - /// - public long SizeLimit { get; internal set; } - } -} diff --git a/DevPack/Storage/DOM/Helpers/IPropertyAttachmentStore.cs b/DevPack/Storage/DOM/Helpers/IPropertyAttachmentStore.cs deleted file mode 100644 index de3324c5..00000000 --- a/DevPack/Storage/DOM/Helpers/IPropertyAttachmentStore.cs +++ /dev/null @@ -1,18 +0,0 @@ -namespace Skyline.DataMiner.Solutions.MediaOps.Plan.Storage.DOM -{ - using System.Collections.Generic; - - using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; - - // Seam around the DataMiner attachment API so the attachment handling can be verified without a DataMiner Agent. - internal interface IPropertyAttachmentStore - { - void Add(DomInstanceId instanceId, string attachmentName, byte[] content); - - byte[] Get(DomInstanceId instanceId, string attachmentName); - - void Delete(DomInstanceId instanceId, string attachmentName); - - IReadOnlyCollection GetNames(DomInstanceId instanceId); - } -} diff --git a/DevPack/Storage/DOM/Helpers/PropertyAttachmentStore.cs b/DevPack/Storage/DOM/Helpers/PropertyAttachmentStore.cs deleted file mode 100644 index 1baff4ba..00000000 --- a/DevPack/Storage/DOM/Helpers/PropertyAttachmentStore.cs +++ /dev/null @@ -1,37 +0,0 @@ -namespace Skyline.DataMiner.Solutions.MediaOps.Plan.Storage.DOM -{ - using System; - using System.Collections.Generic; - - using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; - - internal sealed class PropertyAttachmentStore : IPropertyAttachmentStore - { - private readonly SlcPropertiesHelper propertiesHelper; - - public PropertyAttachmentStore(SlcPropertiesHelper propertiesHelper) - { - this.propertiesHelper = propertiesHelper ?? throw new ArgumentNullException(nameof(propertiesHelper)); - } - - public void Add(DomInstanceId instanceId, string attachmentName, byte[] content) - { - propertiesHelper.DomHelper.DomInstances.Attachments.Add(instanceId, attachmentName, content); - } - - public byte[] Get(DomInstanceId instanceId, string attachmentName) - { - return propertiesHelper.DomHelper.DomInstances.Attachments.Get(instanceId, attachmentName); - } - - public void Delete(DomInstanceId instanceId, string attachmentName) - { - propertiesHelper.DomHelper.DomInstances.Attachments.Delete(instanceId, attachmentName); - } - - public IReadOnlyCollection GetNames(DomInstanceId instanceId) - { - return propertiesHelper.DomHelper.DomInstances.Attachments.GetFileNames(instanceId); - } - } -} From 3df31e44abab9c0a1f19a44cfaed31a451d8fc05 Mon Sep 17 00:00:00 2001 From: Jens Vandewalle Date: Thu, 13 Aug 2026 10:59:54 +0200 Subject: [PATCH 11/12] Add GetResourceStudioFields for partial resource retrieval Update SlcResourceStudioHelper with GetResourceStudioFields to fetch partial DOM instances based on selected fields and filters. Refactor ResourceManagerTraceDataHandler to use this method, retrieving only required fields (e.g., Resource_Id) for improved performance. Add null checks and support for SelectedFields and PartialObject types. --- .../Jobs/ResourceManagerTraceDataHandler.cs | 12 ++++++--- .../DOM/Helpers/SlcResourceStudioHelper.cs | 25 +++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs b/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs index 0305850c..cf6f06c1 100644 --- a/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs +++ b/DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs @@ -5,6 +5,7 @@ namespace Skyline.DataMiner.Solutions.MediaOps.Plan.API using System.Linq; using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; + using Skyline.DataMiner.Net.Apps.ManagerStore.Select; using Skyline.DataMiner.Net.Messages.SLDataGateway; using Skyline.DataMiner.Net.ResponseErrorData; using Skyline.DataMiner.Solutions.MediaOps.Plan.Exceptions; @@ -95,20 +96,23 @@ private Dictionary BuildDomResourceIdByCoreId( return new Dictionary(); } + var selectedFields = new SelectedFields() + .Add(SlcResource_StudioIds.Sections.ResourceInternalProperties.Resource_Id); + FilterElement Filter(Guid coreResourceId) => DomInstanceExposers.DomDefinitionId.Equal(SlcResource_StudioIds.Definitions.Resource.Id) .AND(DomInstanceExposers.FieldValues.DomInstanceField(SlcResource_StudioIds.Sections.ResourceInternalProperties.Resource_Id).Equal(coreResourceId)); var domResourceIdByCoreId = new Dictionary(); - foreach (var domResource in planApi.DomHelpers.SlcResourceStudioHelper.GetResources(coreResourceIds, Filter)) + foreach (var partialObject in planApi.DomHelpers.SlcResourceStudioHelper.GetResourceStudioFields(coreResourceIds, Filter, selectedFields)) { - var coreId = domResource.ResourceInternalProperties.Resource_Id.GetValueOrDefault(); - if (coreId == Guid.Empty) + if (!partialObject.TryGetValue(SlcResource_StudioIds.Sections.ResourceInternalProperties.Resource_Id, out var coreId) + || coreId == Guid.Empty) { continue; } - domResourceIdByCoreId[coreId] = domResource.ID.Id; + domResourceIdByCoreId[coreId] = partialObject.Id.Id; } return domResourceIdByCoreId; diff --git a/DevPack/Storage/DOM/Helpers/SlcResourceStudioHelper.cs b/DevPack/Storage/DOM/Helpers/SlcResourceStudioHelper.cs index 2bbae41c..c709ec65 100644 --- a/DevPack/Storage/DOM/Helpers/SlcResourceStudioHelper.cs +++ b/DevPack/Storage/DOM/Helpers/SlcResourceStudioHelper.cs @@ -6,10 +6,12 @@ using Skyline.DataMiner.Net; using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; + using Skyline.DataMiner.Net.Apps.ManagerStore.Select; using Skyline.DataMiner.Net.Messages.SLDataGateway; using Skyline.DataMiner.Solutions.MediaOps.Plan.Storage.DOM.SlcResource_Studio; using Skyline.DataMiner.Utils.DOM.Extensions; + using SLDataGateway.API.Querying; using SLDataGateway.API.Types.Querying; internal class SlcResourceStudioHelper : DomModuleHelperBase @@ -230,6 +232,29 @@ public IEnumerable GetResourceStudioInstances(IEnumerable ids x => DomHelper.DomInstances.Read(x)); } + public IEnumerable> GetResourceStudioFields(IEnumerable values, Func> filter, SelectedFields selectedFields) + { + if (values == null) + { + throw new ArgumentNullException(nameof(values)); + } + + if (filter == null) + { + throw new ArgumentNullException(nameof(filter)); + } + + if (selectedFields == null) + { + throw new ArgumentNullException(nameof(selectedFields)); + } + + return FilterQueryExecutor.RetrieveFilteredItems( + values.Distinct(), + x => filter(x), + x => DomHelper.DomInstances.Read(x.ToQuery(), selectedFields)); + } + public DomInstance TransitionResourceToComplete(Guid resourceId) { var transitionId = SlcResource_StudioIds.Behaviors.Resource_Behavior.Transitions.Draft_To_Complete; From 527918c574eb246df8dfccb29bf255c3d13963d1 Mon Sep 17 00:00:00 2001 From: Jens Vandewalle Date: Thu, 13 Aug 2026 12:12:43 +0200 Subject: [PATCH 12/12] Add DomInstanceSelectStore for select read in unit tests Introduce DomInstanceSelectStore to handle partial object select read requests in unit tests, simulating DataMiner Agent behavior. Update SimulatedDms.cs to use this handler before the default DOM handler. Update package references and versions; remove dependency on Skyline.DataMiner.Files.SLNetTypes. --- ...Utils.Solutions.MediaOps.Plan.Tests.csproj | 1 - .../Simulation/SimulatedDms.cs | 8 ++ .../Stores/DomInstanceSelectStore.cs | 101 ++++++++++++++++++ Directory.Packages.props | 7 +- 4 files changed, 112 insertions(+), 5 deletions(-) create mode 100644 DevPack.UnitTesting/Stores/DomInstanceSelectStore.cs diff --git a/DevPack.Tests/Skyline.DataMiner.Dev.Utils.Solutions.MediaOps.Plan.Tests.csproj b/DevPack.Tests/Skyline.DataMiner.Dev.Utils.Solutions.MediaOps.Plan.Tests.csproj index bb7a3b9e..1ec5510c 100644 --- a/DevPack.Tests/Skyline.DataMiner.Dev.Utils.Solutions.MediaOps.Plan.Tests.csproj +++ b/DevPack.Tests/Skyline.DataMiner.Dev.Utils.Solutions.MediaOps.Plan.Tests.csproj @@ -15,7 +15,6 @@ - all runtime; build; native; contentfiles; analyzers; buildtransitive diff --git a/DevPack.UnitTesting/Simulation/SimulatedDms.cs b/DevPack.UnitTesting/Simulation/SimulatedDms.cs index 9c6f3486..34206964 100644 --- a/DevPack.UnitTesting/Simulation/SimulatedDms.cs +++ b/DevPack.UnitTesting/Simulation/SimulatedDms.cs @@ -264,6 +264,14 @@ internal bool TryHandleMessage(DMSMessage message, out IEnumerable r throw new ArgumentNullException(nameof(message)); } + // A DOM select read only asks for specific fields. The DOM message handler does not support it, + // so it is translated into a regular read and then reduced to the requested fields. + if (DomInstanceSelectStore.TryHandleMessage(message, _domSlNetMessageHandler, out var selectResponse)) + { + responses = new[] { selectResponse }; + return true; + } + if (_domSlNetMessageHandler.TryHandleMessage(message, out var domResponse)) { responses = new[] { domResponse }; diff --git a/DevPack.UnitTesting/Stores/DomInstanceSelectStore.cs b/DevPack.UnitTesting/Stores/DomInstanceSelectStore.cs new file mode 100644 index 00000000..8b79a4c1 --- /dev/null +++ b/DevPack.UnitTesting/Stores/DomInstanceSelectStore.cs @@ -0,0 +1,101 @@ +namespace Skyline.DataMiner.Solutions.MediaOps.Plan.UnitTesting.Stores +{ + using System; + using System.Collections.Generic; + + using Skyline.DataMiner.Net.Apps.DataMinerObjectModel; + using Skyline.DataMiner.Net.Apps.DataMinerObjectModel.Select; + using Skyline.DataMiner.Net.Apps.ManagerStore.Select; + using Skyline.DataMiner.Net.Messages; + using Skyline.DataMiner.Utils.DOM.UnitTesting; + + /// + /// Handles DOM instance select (partial object) read requests, mirroring how a real DataMiner Agent + /// answers a . + /// + /// + /// A select read returns only the requested fields instead of full objects. The agent replies with a + /// whose custom response data holds a . + /// does not support these requests and returns , + /// so the request is translated into a regular read whose results are reduced to the requested fields. + /// + internal static class DomInstanceSelectStore + { + public static bool TryHandleMessage(DMSMessage message, DomSLNetMessageHandler domHandler, out DMSMessage response) + { + if (domHandler is null) + { + throw new ArgumentNullException(nameof(domHandler)); + } + + response = null; + + if (!(message is ManagerStoreSelectReadRequest request)) + { + return false; + } + + var readRequest = new ManagerStoreReadRequest(request.Query) + { + ModuleId = request.ModuleId, + }; + + if (!domHandler.TryHandleMessage(readRequest, out var readResponse) + || !(readResponse is ManagerStoreCrudResponse crudResponse)) + { + return false; + } + + response = CreateResponse(request, crudResponse.Objects ?? new List()); + return true; + } + + private static DMSMessage CreateResponse(ManagerStoreSelectReadRequest request, IEnumerable instances) + { + if (request is null) + { + throw new ArgumentNullException(nameof(request)); + } + + var selectedFields = request.SelectedFields ?? new List(); + var objects = new List(); + + foreach (var instance in instances) + { + if (instance?.ID is null) + { + continue; + } + + var factory = new PartialDomInstanceFactory(instance.ID.ModuleId); + var values = new List(selectedFields.Count); + + foreach (var selectedField in selectedFields) + { + values.Add(new PartialObjectValue + { + FieldReferenceId = selectedField.Id, + Value = Execute(selectedField, instance), + }); + } + + objects.Add(factory.GetPartialObjectData(instance.ID, values)); + } + + return new ManagerStoreCrudResponse((object)new SelectResult { Objects = objects }); + } + + private static object Execute(SelectedFieldReference selectedField, DomInstance instance) + { + try + { + return selectedField.SerializableExposer?.Exposer?.execute(instance); + } + catch + { + // Mirror the agent's fail-safe behavior: a field that cannot be read is returned as no value. + return null; + } + } + } +} diff --git a/Directory.Packages.props b/Directory.Packages.props index 19a60bee..14617079 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -13,13 +13,12 @@ - - - + + + -