Feat/add metadata #1531
Feat/add metadata #1531
Conversation
📝 WalkthroughWalkthroughThe pull request adds metadata support to the DataElementChange model. A new AddMetadata method enables attaching metadata key-value pairs to created data elements. During data element creation, any attached metadata is persisted to storage and the change is locked to prevent subsequent modifications. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs(1 hunks)src/Altinn.App.Core/Models/DataElementChanges.cs(2 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Models/DataElementChanges.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Models/DataElementChanges.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Models/DataElementChanges.cs
🧠 Learnings (1)
📚 Learning: 2025-09-18T07:40:01.660Z
Learnt from: ivarne
PR: Altinn/app-lib-dotnet#1470
File: src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs:411-418
Timestamp: 2025-09-18T07:40:01.660Z
Learning: In InstanceDataUnitOfWork.UpdateDataElement, the _binaryCache is intentionally NOT updated after successful UpdateBinaryData calls. This preserves the ability to repeatedly detect which elements were changed during a session, even after they've been saved to storage. The "dirty" detection is by design for audit/tracking purposes.
Applied to files:
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and Test
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
src/Altinn.App.Core/Models/DataElementChanges.cs (2)
76-79: LGTM!The internal
Metadataproperty correctly exposes the metadata collection as read-only.
81-84: LGTM!The
Lockflag is appropriately scoped as internal and provides a simple mechanism to prevent further metadata changes.test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
3964-3964: Public API baseline update acknowledged.The new
AddMetadata(string key, string value)signature matches the intended metadata workflow for created data elements. No further action needed.
| if (change.Metadata is not null) | ||
| { | ||
| dataElement.Metadata = [.. change.Metadata]; | ||
| change.Lock = true; | ||
| await _dataClient.Update(Instance, dataElement); | ||
| } |
There was a problem hiding this comment.
🧩 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.
| 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).
| /// <summary> | ||
| /// The metadata of the data element | ||
| /// </summary> | ||
| private List<KeyValueEntry>? _metadata = []; |
There was a problem hiding this comment.
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.
| 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 }); | ||
| } |
There was a problem hiding this comment.
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.
| 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.
1 participant
Description
Added metadata to the data element, along with functionality to store the metadata within it.
This is part of the new feature thumbnail in the fileupload component, related PR in frontend (Altinn/app-frontend-react#3797) and localtest. Storage (in process).
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit