Skip to content

Feat/add metadata #1531

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+42 −0
Open

Feat/add metadata #1531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46a1556
addMetadata
Sep 25, 2025
5ebd0d9
Merge branch 'main' of https://github.com/walldenfilippa/app-lib-dotn…
walldenfilippa Oct 21, 2025
6382df0
minor update to merge
walldenfilippa Oct 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,14 @@ DataElementChange change
new MemoryAsStream(bytes),
authenticationMethod: GetAuthenticationMethod(change.DataType)
);

if (change.Metadata is not null)
{
dataElement.Metadata = [.. change.Metadata];
change.Lock = true;
await _dataClient.Update(Instance, dataElement);
}
Comment on lines +452 to +457
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify if metadata can be included in the initial Insert call.

The current implementation makes two sequential async calls when metadata is present: InsertBinaryData followed by Update. This introduces additional latency and a potential inconsistency if the Update fails (the data element is created but metadata is not persisted, yet Lock is already set to true).

Run the following script to check if the IDataClient.InsertBinaryData method signature or overloads support metadata:

If the API does not support metadata on creation, consider adding error handling:

 if (change.Metadata is not null)
 {
     dataElement.Metadata = [.. change.Metadata];
-    change.Lock = true;
-    await _dataClient.Update(Instance, dataElement);
+    try
+    {
+        await _dataClient.Update(Instance, dataElement);
+        change.Lock = true;
+    }
+    catch
+    {
+        // Metadata update failed; consider logging or cleanup
+        throw;
+    }
 }

🏁 Script executed:

#!/bin/bash
# Check if InsertBinaryData has metadata support
ast-grep --pattern $'interface IDataClient {
  $$$
  InsertBinaryData($$$) {
    $$$
  }
  $$$
}'

# Also search for any overloads or related methods
rg -nP --type=cs -C3 '\bInsertBinaryData\s*\(' -g '!**/test/**'

Length of output: 8577


---

🏁 Script executed:

```shell
#!/bin/bash
# Get the full signature of the second InsertBinaryData overload used in InstanceDataUnitOfWork
sed -n '281,295p' src/Altinn.App.Core/Internal/Data/IDataClient.cs

echo "---"

# Also check the implementation
sed -n '442,455p' src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs

echo "---"

# Check the context around lines 452-457 in InstanceDataUnitOfWork.cs
sed -n '440,465p' src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs

Length of output: 310


---

🏁 Script executed:

```shell
#!/bin/bash
# Get the full signature of the second InsertBinaryData overload used in InstanceDataUnitOfWork
sed -n '281,295p' src/Altinn.App.Core/Internal/Data/IDataClient.cs

echo "---"

# Also check the implementation
sed -n '442,455p' src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs

echo "---"

# Check the context around lines 452-457 in InstanceDataUnitOfWork.cs
sed -n '440,465p' src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs

Length of output: 2241

Move change.Lock = true after the successful Update call.

The InsertBinaryData method does not support a metadata parameter, so the two-call approach is necessary. However, setting change.Lock = true before awaiting the Update call (line 453) risks leaving the object in an inconsistent state if the update fails—the lock would be set while metadata persistence failed.

Move the lock assignment after the successful update completes:

 if (change.Metadata is not null)
 {
     dataElement.Metadata = [.. change.Metadata];
-    change.Lock = true;
-    await _dataClient.Update(Instance, dataElement);
+    await _dataClient.Update(Instance, dataElement);
+    change.Lock = true;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (change.Metadata is not null)
{
dataElement.Metadata = [.. change.Metadata];
change.Lock = true;
await _dataClient.Update(Instance, dataElement);
}
if (change.Metadata is not null)
{
dataElement.Metadata = [.. change.Metadata];
await _dataClient.Update(Instance, dataElement);
change.Lock = true;
}
🤖 Prompt for AI Agents 
In src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs around lines 452
to 457, move the assignment of change.Lock = true so it occurs after the awaited
_dataClient.Update(Instance, dataElement) completes successfully; specifically,
leave dataElement.Metadata assignment and the await _dataClient.Update call
as-is, then set change.Lock = true only after the await returns (so the lock is
not set if the update throws).


// Update caches
_binaryCache.Set(dataElement, bytes);
change.DataElement = dataElement; // Set the data element so that it can be referenced later in the save process
Expand Down
33 changes: 33 additions & 0 deletions src/Altinn.App.Core/Models/DataElementChanges.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ internal DataElementChange(ChangeType type, DataType dataType, string contentTyp
ContentType = contentType;
}

/// <summary>
/// The metadata of the data element
/// </summary>
private List<KeyValueEntry>? _metadata = [];
Comment on lines +45 to +48
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Contradictory initialization logic.

The field is declared nullable and initialized to an empty list, but line 100 uses lazy initialization (_metadata ??= []). Either remove the initializer here (for lazy init) or remove the nullable annotation and line 100's null-coalescing (for eager init).

Apply this diff for lazy initialization (recommended):

-    private List<KeyValueEntry>? _metadata = [];
+    private List<KeyValueEntry>? _metadata;

Or apply this diff for eager initialization:

-    private List<KeyValueEntry>? _metadata = [];
+    private List<KeyValueEntry> _metadata = [];

And remove line 100's null-coalescing:

-        _metadata ??= [];
-        _metadata.Add(new KeyValueEntry { Key = key, Value = value });
+        _metadata.Add(new KeyValueEntry { Key = key, Value = value });

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/Altinn.App.Core/Models/DataElementChanges.cs around lines 45-48 (and note
usage at line 100), the private field _metadata is declared nullable but eagerly
initialized to an empty list which conflicts with the lazy-init pattern used at
line 100; remove the initializer (leave "private List<KeyValueEntry>?
_metadata;") so the field remains nullable and the existing "_metadata ??= []"
lazy initialization at line 100 works as intended.


/// <summary>
/// The type of update: Create, Update or Delete
/// </summary>
Expand All @@ -67,6 +72,34 @@ internal DataElementChange(ChangeType type, DataType dataType, string contentTyp
/// The contentType of an element in storage
/// </summary>
public string ContentType { get; }

/// <summary>
/// The metadata of the data element
/// </summary>
internal IReadOnlyCollection<KeyValueEntry>? Metadata => _metadata;

/// <summary>
/// If true, no more metadata can be added
/// </summary>
internal bool Lock { get; set; }

/// <summary>
/// Add metadata to a created data element
/// </summary>
public void AddMetadata(string key, string value)
{
if (Type != ChangeType.Created)
{
throw new InvalidOperationException("Metadata can only be added to created data elements");
}

if (Lock)
{
throw new InvalidOperationException("Metadata already locked");
}
_metadata ??= [];
_metadata.Add(new KeyValueEntry { Key = key, Value = value });
}
Comment on lines +89 to +102
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add parameter validation for key and value.

The method lacks validation to ensure key and value are not null, empty, or whitespace. Invalid metadata entries could be added to the data element.

Apply this diff to add validation:

 public void AddMetadata(string key, string value)
 {
+    ArgumentException.ThrowIfNullOrWhiteSpace(key);
+    ArgumentException.ThrowIfNullOrWhiteSpace(value);
+
     if (Type != ChangeType.Created)
     {
         throw new InvalidOperationException("Metadata can only be added to created data elements");
     }

     if (Lock)
     {
         throw new InvalidOperationException("Metadata already locked");
     }
     _metadata ??= [];
     _metadata.Add(new KeyValueEntry { Key = key, Value = value });
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void AddMetadata(string key, string value)
{
if (Type != ChangeType.Created)
{
throw new InvalidOperationException("Metadata can only be added to created data elements");
}
if (Lock)
{
throw new InvalidOperationException("Metadata already locked");
}
_metadata ??= [];
_metadata.Add(new KeyValueEntry { Key = key, Value = value });
}
public void AddMetadata(string key, string value)
{
ArgumentException.ThrowIfNullOrWhiteSpace(key);
ArgumentException.ThrowIfNullOrWhiteSpace(value);
if (Type != ChangeType.Created)
{
throw new InvalidOperationException("Metadata can only be added to created data elements");
}
if (Lock)
{
throw new InvalidOperationException("Metadata already locked");
}
_metadata ??= [];
_metadata.Add(new KeyValueEntry { Key = key, Value = value });
}
🤖 Prompt for AI Agents 
In src/Altinn.App.Core/Models/DataElementChanges.cs around lines 89–102,
AddMetadata currently allows null/empty/whitespace keys and values and also
incorrectly initializes _metadata with []; add parameter validation at the top
of the method: throw ArgumentNullException for key or value if null, and throw
ArgumentException if key or value is empty or whitespace (use nameof for
parameter names); also replace the invalid _metadata ??= [] with proper
collection initialization (e.g. new List<KeyValueEntry>()) before adding the new
KeyValueEntry.

}

/// <summary>
Expand Down
1 change: 1 addition & 0 deletions ...tinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3961,6 +3961,7 @@ namespace Altinn.App.Core.Models
public Altinn.App.Core.Models.DataElementIdentifier DataElementIdentifier { get; }
public Altinn.Platform.Storage.Interface.Models.DataType DataType { get; }
public Altinn.App.Core.Models.ChangeType Type { get; }
public void AddMetadata(string key, string value) { }
}
public sealed class DataElementChanges
{
Expand Down
Loading