feat/add metadata #1598
feat/add metadata #1598
Conversation
… into Feat/AddMetadata
…Feat/AddMetadata
walldenfilippa
added
feature
📝 WalkthroughWalkthroughThe change adds metadata persistence for newly created data elements. When a data element is created, if metadata is present, it's assigned to the element and updated via the data client before the change is locked to prevent further modifications. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
/publish |
PR release:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs:
- Around line 482-487: The code currently calls _dataClient.Update(Instance,
dataElement) after InsertBinaryData and doesn't handle failures, risking
orphaned binary data and extra round-trips; modify the logic in
InstanceDataUnitOfWork where InsertBinaryData and the metadata handling occur so
that: if change.Metadata is not null you either (a) call a single insert API
that accepts metadata (investigate and use InsertBinaryData overload that
accepts metadata) to avoid two calls, or (b) wrap the
_dataClient.Update(Instance, dataElement) in a try-catch and on failure perform
compensating cleanup by deleting the previously inserted binary (use the same
identifier on dataElement/InsertBinaryData) and rethrow or translate the
exception; ensure change.Lock is only set to true after a successful metadata
persist and reference the symbols InsertBinaryData, dataElement,
change.Metadata, _dataClient.Update, change.Lock and Instance when making the
changes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Models/DataElementChanges.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Models/DataElementChanges.cs
🧠 Learnings (6)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 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.
📚 Learning: 2025-09-18T07:40:01.660Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 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
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🔇 Additional comments (2)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
4160-4160: Generated public API snapshot – no code review needed hereThis file is an auto-generated public API snapshot (
*.verified.txt). Per project conventions, I’m not reviewing or suggesting code changes based on this line; correctness should instead be ensured in the corresponding implementation (DataElementChange.AddMetadata) and its tests.src/Altinn.App.Core/Models/DataElementChanges.cs (1)
91-110: LGTM! Metadata addition logic is well-implemented.The method correctly validates inputs, enforces that metadata can only be added to newly created elements, and checks the Lock flag. The lazy initialization pattern is appropriate here.
| if (change.Metadata is not null) | ||
| { | ||
| dataElement.Metadata = [.. change.Metadata]; | ||
| await _dataClient.Update(Instance, dataElement); | ||
| change.Lock = true; | ||
| } |
There was a problem hiding this comment.
Consider error handling for metadata Update failure.
If _dataClient.Update fails after InsertBinaryData succeeds, the data element will exist in storage without the intended metadata. While the exception will propagate and fail the operation, there's no cleanup or rollback of the already-created data element.
Additionally, this approach makes two sequential storage calls for each new data element with metadata. Consider whether the storage API could support inserting with metadata in a single operation to improve reliability and performance.
Suggested approach for error handling
Consider wrapping the metadata persistence in a try-catch to handle failures gracefully:
if (change.Metadata is not null)
{
dataElement.Metadata = [.. change.Metadata];
- await _dataClient.Update(Instance, dataElement);
- change.Lock = true;
+ try
+ {
+ await _dataClient.Update(Instance, dataElement);
+ change.Lock = true;
+ }
+ catch (Exception ex)
+ {
+ // Log the failure and consider whether to:
+ // 1. Delete the created data element (rollback)
+ // 2. Continue without metadata and log a warning
+ // 3. Rethrow and fail the entire operation (current behavior)
+ throw;
+ }
}For the performance concern, investigate whether InsertBinaryData can accept metadata as a parameter to avoid the second call.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs around lines
482-487, The code currently calls _dataClient.Update(Instance, dataElement)
after InsertBinaryData and doesn't handle failures, risking orphaned binary data
and extra round-trips; modify the logic in InstanceDataUnitOfWork where
InsertBinaryData and the metadata handling occur so that: if change.Metadata is
not null you either (a) call a single insert API that accepts metadata
(investigate and use InsertBinaryData overload that accepts metadata) to avoid
two calls, or (b) wrap the _dataClient.Update(Instance, dataElement) in a
try-catch and on failure perform compensating cleanup by deleting the previously
inserted binary (use the same identifier on dataElement/InsertBinaryData) and
rethrow or translate the exception; ensure change.Lock is only set to true after
a successful metadata persist and reference the symbols InsertBinaryData,
dataElement, change.Metadata, _dataClient.Update, change.Lock and Instance when
making the changes.
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#3898) and localtest (Altinn/app-localtest#183). Storage branch: https://github.com/Altinn/altinn-storage/tree/filippa2
TEST APP: https://altinn.studio/repos/ttd/test-app-filippa
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.