Add file size restriction for legacy API #561
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Ceredron
added
the
kind/user-story
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
tests/Altinn.Broker.Tests/Helpers/FakeFileStream.cs (2)
1-11: Add XML documentation to improve test helper clarity.Consider adding XML documentation to describe the purpose and usage of this test helper class, which will improve maintainability and help other developers understand how to use it in their tests.
namespace Altinn.Broker.Tests.Helpers; +/// <summary> +/// A fake stream implementation that simulates a file of a specific size for testing purposes. +/// This helper is particularly useful for testing file size validation scenarios without actual file I/O. +/// </summary> public class FakeFileStream : Stream { private readonly long _length; private long _position; + /// <summary> + /// Initializes a new instance of the <see cref="FakeFileStream"/> class. + /// </summary> + /// <param name="length">The total length of the fake stream in bytes.</param> public FakeFileStream(long length) { _length = length; _position = 0; }
56-58: Add descriptive exception messages.Consider adding more descriptive exception messages to help developers understand why these operations are not supported.
public override void Flush() { } - public override void SetLength(long value) => throw new NotImplementedException(); - public override void Write(byte[] buffer, int offset, int count) => throw new NotImplementedException(); + public override void SetLength(long value) => throw new NotImplementedException("SetLength is not supported in this read-only test stream"); + public override void Write(byte[] buffer, int offset, int count) => throw new NotImplementedException("Write operations are not supported in this read-only test stream");src/Altinn.Broker.API/Controllers/LegacyFileController.cs (1)
80-81: Add XML documentation for the size restriction.Since this is a breaking change that adds file size restrictions, it should be documented in the method's XML documentation to inform API consumers.
Add this to the method's XML documentation:
/// Upload to an initialized file using a binary stream. /// </summary> + /// <remarks> + /// The file size is restricted according to the resource configuration. + /// The Content-Length header is required for all uploads. + /// </remarks> /// <returns></returns>tests/Altinn.Broker.Tests/LegacyFileControllerTests.cs (1)
449-469: LGTM with suggestions for improvement!The test effectively validates the file size restriction for the legacy API. A few suggestions to enhance the test:
- Extract the file size limit as a constant for better maintainability
- Consider verifying the error message content to ensure the correct validation error is returned
Consider applying these improvements:
+ private const long MAX_FILE_SIZE = 1024 * 1000 * 1000; // 1GB + private const long OVERSIZED_FILE = MAX_FILE_SIZE + 1; [Fact] public async Task UploadTooBigFile_ToLegacyApi_FailsWithValidationError() { // Initialize var initializeFileResponse = await _legacyClient.PostAsJsonAsync("broker/api/legacy/v1/file", FileTransferInitializeExtTestFactory.BasicFileTransfer()); string onBehalfOfConsumer = FileTransferInitializeExtTestFactory.BasicFileTransfer().Sender; Assert.True(initializeFileResponse.IsSuccessStatusCode, await initializeFileResponse.Content.ReadAsStringAsync()); var fileId = await initializeFileResponse.Content.ReadAsStringAsync(); var fileAfterInitialize = await _legacyClient.GetFromJsonAsync<LegacyFileOverviewExt>($"broker/api/legacy/v1/file/{fileId}?onBehalfOfConsumer={onBehalfOfConsumer}", _responseSerializerOptions); Assert.NotNull(fileAfterInitialize); Assert.Equal(LegacyFileStatusExt.Initialized, fileAfterInitialize.FileStatus); // Upload - var fileSize = 1024 * 1000 * 1000 + 1; + var fileSize = OVERSIZED_FILE; var content = new FakeFileStream(fileSize); var streamContent = new StreamContent(content); streamContent.Headers.ContentType = new MediaTypeHeaderValue("application/octet-stream"); streamContent.Headers.ContentLength = fileSize; var uploadResponse = await _legacyClient.PostAsync($"broker/api/legacy/v1/file/{fileId}/upload?onBehalfOfConsumer={onBehalfOfConsumer}", streamContent); Assert.Equal(HttpStatusCode.BadRequest, uploadResponse.StatusCode); + var errorContent = await uploadResponse.Content.ReadAsStringAsync(); + Assert.Contains("file size exceeds the maximum allowed limit", errorContent.ToLower()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/Altinn.Broker.API/Controllers/LegacyFileController.cs (1 hunks)
- src/Altinn.Broker.Application/UploadFile/UploadFileHandler.cs (3 hunks)
- tests/Altinn.Broker.Tests/Helpers/FakeFileStream.cs (1 hunks)
- tests/Altinn.Broker.Tests/LegacyFileControllerTests.cs (1 hunks)
🔇 Additional comments (3)
tests/Altinn.Broker.Tests/Helpers/FakeFileStream.cs (1)
13-21: LGTM! Stream properties are correctly implemented.The properties are properly overridden and their implementations align with the intended read-only behavior of this test helper.
src/Altinn.Broker.Application/UploadFile/UploadFileHandler.cs (1)
54-57: LGTM! File size validation is properly implemented.The validation is correctly placed before the file upload process begins, ensuring early failure for oversized files.
src/Altinn.Broker.API/Controllers/LegacyFileController.cs (1)
80-81: Verify the impact on existing clients.The addition of file size restrictions is a breaking change that could affect existing clients of the legacy API.
Let's analyze the potential impact:
| IsLegacy = true, | ||
| ContentLength = Request.ContentLength ?? Request.Body.Length |
There was a problem hiding this comment.
Consider potential memory issues with Request.Body.Length.
The fallback to Request.Body.Length when Request.ContentLength is null could cause memory issues with large files, as it requires reading the entire stream into memory.
Consider this alternative implementation:
- IsLegacy = true,
- ContentLength = Request.ContentLength ?? Request.Body.Length
+ IsLegacy = true,
+ ContentLength = Request.ContentLength ??
+ throw new ValidationException("Content-Length header is required for file uploads")📝 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.
| IsLegacy = true, | |
| ContentLength = Request.ContentLength ?? Request.Body.Length | |
| IsLegacy = true, | |
| ContentLength = Request.ContentLength ?? | |
| throw new ValidationException("Content-Length header is required for file uploads") |
There was a problem hiding this comment.
Should we consider this or some other error handling?
May be unneccessary if we know all filesizes is smaller
There was a problem hiding this comment.
@Andreass2, understood. If we can ensure that all file sizes are small and the Content-Length header is always present, then additional error handling may not be necessary.
✏️ Learnings added
Learnt from: Andreass2
PR: Altinn/altinn-broker#561
File: src/Altinn.Broker.API/Controllers/LegacyFileController.cs:80-81
Timestamp: 2024-10-29T10:43:15.889Z
Learning: In the Altinn Broker API, specifically in `src/Altinn.Broker.API/Controllers/LegacyFileController.cs`, when handling file uploads in the `UploadFileStreamed` method, we can assume that all uploaded files are small and that the `Content-Length` header is always provided. Therefore, additional error handling for missing `Content-Length` is not required.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
All the other comments were a bit stupid, but this one is very valid. I'm inclined to close this Pull Request as this is the Legacy API calling us (and file size is restricted there). Main purpose of this PR was to have it explicit in our code too, but not really worth it if it causes memory issues itself.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
On the other side, we should require content-length header to be set for the normal upload operation.
| IsLegacy = true, | ||
| ContentLength = Request.ContentLength ?? Request.Body.Length |
There was a problem hiding this comment.
Should we consider this or some other error handling?
May be unneccessary if we know all filesizes is smaller
Description
Add file size restriction for legacy API. Same as A2 file size restriction.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
ContentLengthproperty to enhance file upload information.Bug Fixes
Tests
FakeFileStreamclass for simulating file streams in tests.