Implement file type properties - #271
Conversation
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.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
- 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
Builds domResourceIdByCoreId using three error lists via BuildDomResourceIdByCoreId. Passes the mapping to HandleReservationUpdateCausedReservationsToGoToQuarantine for further processing.
- 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
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.
…racking removed file settings Co-authored-by: JensVandewalle <102030104+JensVandewalle@users.noreply.github.com>
…eview-suggestion Reconcile property attachments against stored names to prevent orphans on file setting removal
Co-authored-by: JensVandewalle <102030104+JensVandewalle@users.noreply.github.com>
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.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Builds domResourceIdByCoreId using three error lists via BuildDomResourceIdByCoreId. Passes the mapping to HandleReservationUpdateCausedReservationsToGoToQuarantine for further processing.
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.
- 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
- 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
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.
…racking removed file settings Co-authored-by: JensVandewalle <102030104+JensVandewalle@users.noreply.github.com>
Co-authored-by: JensVandewalle <102030104+JensVandewalle@users.noreply.github.com>
This reverts commit 3acaec0.
….com/SkylineCommunications/Skyline.DataMiner.MediaOps.Plan into 43980_Implement-file-type-properties
There was a problem hiding this comment.
Pull request overview
Extends the MediaOps Plan property API with attachment-backed file values, including validation, persistence synchronization, simulation support, and tests. It also completes the job ResourceManager error translation implementation.
Changes:
- Adds file property settings and DOM attachment storage/synchronization.
- Adds file count/size validation using property and server configuration.
- Adds simulation-backed tests and ResourceManager trace-data translation tests.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
DevPack/Storage/DOM/Helpers/PropertyAttachmentStore.cs |
Implements the DataMiner attachment storage adapter. |
DevPack/Storage/DOM/Helpers/IPropertyAttachmentStore.cs |
Defines the testable attachment storage abstraction. |
DevPack/Exceptions/TraceData/Properties/Definitions/PropertyInvalidFileSizeLimitError.cs |
Adds structured file-size-limit validation errors. |
DevPack/API/Validators/PropertySettingValidator.cs |
Validates file multiplicity and upload sizes. |
DevPack/API/Objects/Properties/Values/Public/PropertySettingCollection.cs |
Integrates file settings into property value collections. |
DevPack/API/Objects/Properties/Values/Public/FilePropertySetting.cs |
Adds the public file property value API and pending attachment state. |
DevPack/API/Objects/Properties/Values/Internal/InnerFilePropertySetting.cs |
Serializes and parses file property DOM sections. |
DevPack/API/Objects/Properties/Definitions/FileProperty.cs |
Persists server-default versus explicit file limits. |
DevPack/API/MediaOpsPlanApi.cs |
Exposes the internal attachment store seam. |
DevPack/API/Handlers/Workflow/Jobs/ResourceManagerTraceDataHandler.cs |
Translates ResourceManager failures into job resource errors. |
DevPack/API/Handlers/Workflow/Jobs/CoreJobHandler.cs |
Extracts the ResourceManager translator from the handler. |
DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs |
Synchronizes file attachments during collection persistence. |
DevPack/API/Handlers/Properties/DomPropertyHandler.cs |
Validates file limits against the agent configuration. |
DevPack.UnitTesting/Simulation/SimulatedDms.cs |
Simulates the agent maximum document size response. |
DevPack.Tests/Workflow/Jobs/ResourceManagerTraceDataHandlerTests.cs |
Tests typed and fallback ResourceManager error translation. |
DevPack.Tests/Properties/Values/FilePropertySimulationTests.cs |
Tests file persistence, attachment handling, and validation. |
DevPack.Tests/Properties/Values/FilePropertySettingTests.cs |
Tests the standalone file setting API. |
Suppressed comments (3)
DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs:128
- Orphaned attachments are deleted before the replacement uploads run. During a rename, the old attachment is removed here; if the later upload fails, both the old content and the new content are gone even though the API only reports an upload error. Upload all required content first and only delete obsolete attachments after every upload for the collection has succeeded.
foreach (var attachmentName in planApi.PropertyAttachments.GetNames(instanceId).Where(x => !expectedAttachments.Contains(x)).ToList())
{
try
{
planApi.PropertyAttachments.Delete(instanceId, attachmentName);
DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs:133
- A failed attachment deletion is only logged, after which the collection can still be reported successful and its pending file state is cleared below. This leaves leaked content while telling the caller synchronization succeeded. Record a structured error for the collection and retain/reconstruct the pending cleanup so the operation is not marked successful.
catch (Exception ex)
{
planApi.Logger.Error(this, $"Failed to delete orphaned attachment '{attachmentName}': {ex}");
}
DevPack/API/Handlers/Properties/DomPropertySettingCollectionHandler.cs:164
- A content-only replacement keeps the same file names, so the collection hash is unchanged and
GetItemsWithChangeshas already marked the item successful. If this upload then fails,ReportErrorthrowsInvalidOperationExceptionbecause the same ID is both successful and unsuccessful, instead of returning the structured attachment error. Defer the unchanged-item success decision until attachment synchronization completes, or explicitly treat pending file content as a change.
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,
});
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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()); |
There was a problem hiding this comment.
Implemented as a staged workflow in 017393c: the first write keeps the file names that are already stored as attachments, the content is uploaded next, and only then are the new file names committed in a second write. When an upload or that second write fails, the file names are left at their previous value and the uploaded content is removed again by the attachment reconciliation, so a stored name always refers to content that can be read.
| private string StripAttachmentPrefix(string entry) | ||
| { | ||
| var prefix = $"{Id}_"; | ||
|
|
||
| return entry.StartsWith(prefix, StringComparison.OrdinalIgnoreCase) ? entry.Substring(prefix.Length) : entry; |
There was a problem hiding this comment.
@copilot what do you mean? The prefix is to allow using the same name for different properties added to the same DOM instance. We strip the ID because the consumer doesn't need to see this.
There was a problem hiding this comment.
The prefix on the attachment name is indeed there to keep the same file name usable for different properties on one DOM instance, and the stored DOM value only contains the plain file name, so nothing has to be stripped for values written by this version.
StripAttachmentPrefix only exists for values that were written earlier in this branch, when the full attachment name was stored in the value. The remark is about the edge case where a user uploads a file that is literally named <propertyId>_report.pdf: that raw name is stored in the value, and on the next read the prefix is stripped, so the setting reports report.pdf and ReadContent looks for the attachment <propertyId>_report.pdf while the content was uploaded as <propertyId>_<propertyId>_report.pdf.
If no values in the old format exist anywhere, the cleanest fix is to drop StripAttachmentPrefix entirely. Let me know and I will remove it.
There was a problem hiding this comment.
Removed StripAttachmentPrefix in fd7e229, so the stored value is always used as the plain file name and the prefix is only added when resolving the attachment name.
There was a problem hiding this comment.
@copilot when the consumer looks in the list, will he see __report.pdf or _report.pdf? The latter is correct, the first one is wrong
There was a problem hiding this comment.
Your comment looks cut off, so if you meant something else let me know.
If the question is what Files exposes: the consumer sees the plain file name only (e.g. report.pdf). The DOM value stores just that plain name, and the <propertyId>_ prefix is added only in GetAttachmentName when reading/writing the attachment, so the property ID is never visible to the consumer.
There was a problem hiding this comment.
@copilot won't in this case the prefix always be visible when the user looks at the files list?
There was a problem hiding this comment.
No. The prefix is never written into the DOM value: GetSectionWithChanges joins Files, which only contains the plain names given to AddFile (or parsed back from that same value). The prefix is added only in GetAttachmentName, i.e. when talking to the attachment API, so the list a consumer sees stays report.pdf.
Stripping was only relevant for values written by earlier commits on this branch, which stored the full attachment name; since those only exist in intermediate builds of this branch, dropping it is safe.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: JensVandewalle <102030104+JensVandewalle@users.noreply.github.com>
Co-authored-by: JensVandewalle <102030104+JensVandewalle@users.noreply.github.com>
…values Co-authored-by: JensVandewalle <102030104+JensVandewalle@users.noreply.github.com>
Skyline Quality Gate: ✅ Passed
SonarCloud: new-code dashboard for See the Actions run for full logs. |
PR Classification
New feature and API enhancement to support file property values in the MediaOps Plan API.
PR Summary
This pull request adds support for file property values, enabling properties to store files and introducing infrastructure for file management, validation, and testing. It also refactors related classes to integrate the new file property type.
AB#43980