feat(DataElement): Allow updating filename property#958
Conversation
📝 WalkthroughWalkthroughThe changes add support for updating the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
test/UnitTest/TestingControllers/DataControllerTests.cs (1)
1168-1179: Strengthen Update test to assert exact property set and filename valueThis currently verifies only key presence. To protect the contract better, assert the exact key set and validate the
/filenamevalue passed to repository.Suggested test tightening
+ const string expectedFilename = "name+v1.txt"; dataRepositoryMock .Setup(dr => dr.Update( It.IsAny<Guid>(), It.IsAny<Guid>(), It.Is<Dictionary<string, object>>(propertyList => - propertyList.ContainsKey("/locked") - && propertyList.ContainsKey("/refs") - && propertyList.ContainsKey("/references") - && propertyList.ContainsKey("/tags") - && propertyList.ContainsKey("/userDefinedMetadata") - && propertyList.ContainsKey("/metadata") - && propertyList.ContainsKey("/deleteStatus") - && propertyList.ContainsKey("/lastChanged") - && propertyList.ContainsKey("/lastChangedBy") - && propertyList.ContainsKey("/filename") + new HashSet<string> + { + "/locked", + "/refs", + "/references", + "/tags", + "/userDefinedMetadata", + "/metadata", + "/deleteStatus", + "/lastChanged", + "/lastChangedBy", + "/filename", + }.SetEquals(propertyList.Keys) + && Equals(propertyList["/filename"], expectedFilename) ), It.IsAny<CancellationToken>() ) ) .ReturnsAsync(new DataElement()); @@ DataElement dataElementBody = new() { Id = dataGuid, InstanceGuid = instanceGuid, DataType = "default", - Filename = "filename.txt", + Filename = expectedFilename, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/UnitTest/TestingControllers/DataControllerTests.cs` around lines 1168 - 1179, The test's predicate using It.Is<Dictionary<string, object>>(propertyList => ...) only checks for presence of keys; update that predicate to assert the exact key set and the filename value: build the expected key set (the nine keys shown) and compare propertyList.Keys to that expected set (ensure counts match and all expected keys are present), and additionally assert propertyList["/filename"] (or propertyList.GetValueOrDefault("/filename")) equals the expected filename passed into the controller; modify the It.Is<Dictionary<string, object>> predicate in DataControllerTests.cs so the mock verification enforces both exact keys and the exact "/filename" value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Storage/Controllers/DataController.cs`:
- Line 777: In the Update handler in DataController, remove the URL-decoding
call around dataElement.Filename (the { "/filename",
HttpUtility.UrlDecode(dataElement.Filename) } entry) so the controller persists
the raw deserialized filename from the JSON body; simply use
dataElement.Filename directly (no HttpUtility.UrlDecode) because ASP.NET already
binds the JSON string and decoding corrupts names with '+' characters.
---
Nitpick comments:
In `@test/UnitTest/TestingControllers/DataControllerTests.cs`:
- Around line 1168-1179: The test's predicate using It.Is<Dictionary<string,
object>>(propertyList => ...) only checks for presence of keys; update that
predicate to assert the exact key set and the filename value: build the expected
key set (the nine keys shown) and compare propertyList.Keys to that expected set
(ensure counts match and all expected keys are present), and additionally assert
propertyList["/filename"] (or propertyList.GetValueOrDefault("/filename"))
equals the expected filename passed into the controller; modify the
It.Is<Dictionary<string, object>> predicate in DataControllerTests.cs so the
mock verification enforces both exact keys and the exact "/filename" value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8424e11e-2ad7-4ba8-8c7a-cbefc7103353
📒 Files selected for processing (2)
src/Storage/Controllers/DataController.cstest/UnitTest/TestingControllers/DataControllerTests.cs
| { "/deleteStatus", dataElement.DeleteStatus }, | ||
| { "/lastChanged", dataElement.LastChanged }, | ||
| { "/lastChangedBy", dataElement.LastChangedBy }, | ||
| { "/filename", HttpUtility.UrlDecode(dataElement.Filename) }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In .NET, does System.Web.HttpUtility.UrlDecode convert '+' to a space character, and is UrlDecode recommended for values already parsed from JSON request bodies?
💡 Result:
Yes, System.Web.HttpUtility.UrlDecode converts '+' to a space character. No, it is not recommended for values already parsed from JSON request bodies.
Citations:
- 1: https://stackoverflow.com/questions/6753901/httputility-urldecode-turns-2b-into-space-but-i-need-it-to-be
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.web.httputility.urldecode?view=net-10.0
- 3: https://stackoverflow.com/questions/39331176/httputility-urldecode-is-replacing-with-space
- 4: https://stackoverflow.com/questions/25118369/how-to-stop-httputility-urlencode-from-changing-to-space
- 5: https://stackoverflow.com/questions/46255160/c-sharp-web-api-action-method-automatically-decoding-query-parameter
- 6: https://weblog.west-wind.com/posts/2017/sep/14/accepting-raw-request-body-content-in-aspnet-core-api-controllers
- 7: https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding
- 8: https://andrewlock.net/model-binding-json-posts-in-asp-net-core/
🏁 Script executed:
# Locate and examine DataController.cs
find . -type f -name "DataController.cs" | head -20Repository: Altinn/altinn-storage
Length of output: 108
🏁 Script executed:
# Get context around line 777 in DataController.cs
if [ -f "src/Storage/Controllers/DataController.cs" ]; then
sed -n '765,790p' "src/Storage/Controllers/DataController.cs" | cat -n
fiRepository: Altinn/altinn-storage
Length of output: 1139
🏁 Script executed:
# Find the Update method signature with context
if [ -f "src/Storage/Controllers/DataController.cs" ]; then
rg -n "public.*Update" "src/Storage/Controllers/DataController.cs" -A 5
fiRepository: Altinn/altinn-storage
Length of output: 317
🏁 Script executed:
# Check for [Consumes] attribute on Update method
if [ -f "src/Storage/Controllers/DataController.cs" ]; then
rg -n "\[Consumes" "src/Storage/Controllers/DataController.cs" -B 5 -A 10
fiRepository: Altinn/altinn-storage
Length of output: 1621
🏁 Script executed:
# Examine PgDataRepository.cs around line 203-204 to see filename usage
if [ -f "src/Storage/Repository/PgDataRepository.cs" ]; then
sed -n '195,215p' "src/Storage/Repository/PgDataRepository.cs" | cat -n
fiRepository: Altinn/altinn-storage
Length of output: 1229
Remove URL-decoding of filename from JSON body in Update
Line 777 applies HttpUtility.UrlDecode to dataElement.Filename, which corrupts filenames containing + characters (e.g., a+b.txt becomes a b.txt). This endpoint declares [Consumes("application/json")] and binds from the request body—the filename is already deserialized as a plain string by ASP.NET model binding and should be persisted as-is.
Proposed fix
- { "/filename", HttpUtility.UrlDecode(dataElement.Filename) },
+ { "/filename", dataElement.Filename },📝 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.
| { "/filename", HttpUtility.UrlDecode(dataElement.Filename) }, | |
| { "/filename", dataElement.Filename }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Storage/Controllers/DataController.cs` at line 777, In the Update handler
in DataController, remove the URL-decoding call around dataElement.Filename (the
{ "/filename", HttpUtility.UrlDecode(dataElement.Filename) } entry) so the
controller persists the raw deserialized filename from the JSON body; simply use
dataElement.Filename directly (no HttpUtility.UrlDecode) because ASP.NET already
binds the JSON string and decoding corrupts names with '+' characters.
1 participant
Description
Include filename in the list of properties that are allowed to be updated on a DataElement. This property is already included in the OverwriteData-method in the DataController, but is missing from the more general Update-method. This allows updating of filenames after initial upload.
Verification
Documentation
Summary by CodeRabbit
New Features
Tests