perf(api): Enable Http Caching & stream Book Content instead of load to memory #2494
Conversation
loading to memory
# Conflicts: # booklore-api/src/main/java/com/adityachandel/booklore/controller/BookController.java # booklore-api/src/main/java/com/adityachandel/booklore/service/book/BookService.java
There was a problem hiding this comment.
Pull request overview
This PR aims to improve the performance of the "Read Book" feature by enabling HTTP caching and switching from loading book content into memory (ByteArrayResource) to streaming it (InputStreamResource). However, the implementation has critical issues that need to be addressed.
Changes:
- Replaced
ByteArrayResourcewithInputStreamResourcefor streaming book content - Added HTTP caching headers (
Cache-Control: no-cacheandLast-Modified) - Updated tests to reflect the new resource type
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| BookService.java | Modified getBookContent methods to use InputStreamResource and add caching headers |
| BookController.java | Updated return type from ByteArrayResource to InputStreamResource and added import |
| BookServiceTest.java | Updated test to verify InputStreamResource instead of ByteArrayResource |
Comments suppressed due to low confidence (1)
booklore-api/src/test/java/com/adityachandel/booklore/service/book/BookServiceTest.java:333
- The test doesn't verify that the HTTP caching headers (Cache-Control and Last-Modified) are correctly set in the response. Since this PR specifically adds caching support as a key feature, the test should validate:
- The Cache-Control header is present and set correctly
- The Last-Modified header is present when the file exists
- The Last-Modified timestamp matches the actual file modification time
Consider adding assertions to verify these headers are properly configured.
void getBookContent_returnsInputStreamResource() throws Exception {
BookEntity entity = new BookEntity();
entity.setId(10L);
when(bookRepository.findById(10L)).thenReturn(Optional.of(entity));
Path path = Paths.get("/tmp/bookcontent.txt");
Files.write(path, "hello".getBytes());
try (MockedStatic<FileUtils> fileUtilsMock = mockStatic(FileUtils.class)) {
fileUtilsMock.when(() -> FileUtils.getBookFullPath(entity)).thenReturn(path.toString());
ResponseEntity<InputStreamResource> response = bookService.getBookContent(10L);
assertEquals(HttpStatus.OK, response.getStatusCode());
assertNotNull(response.getBody());
assertArrayEquals("hello".getBytes(), response.getBody().getContentAsByteArray());
} finally {
Files.deleteIfExists(path);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .cacheControl(CacheControl.noCache()) | ||
| .lastModified(lastModified != null ? lastModified : Instant.now()) // Fallback to now | ||
| .contentType(MediaType.APPLICATION_OCTET_STREAM) | ||
| .body(new InputStreamResource(new FileInputStream(filePath))); |
There was a problem hiding this comment.
The codebase has an established convention of using FileSystemResource for serving files from disk, as seen in BookDownloadService.java:76-77, BookDropService.java:102, and CustomFontService.java:179. FileSystemResource provides the same streaming benefits as InputStreamResource but with automatic resource management and cleanup.
Consider using FileSystemResource instead of InputStreamResource to maintain consistency with the codebase and avoid resource leaks.
| } catch (IOException ignored) {} | ||
|
|
||
| return ResponseEntity.ok() | ||
| .cacheControl(CacheControl.noCache()) | ||
| .lastModified(lastModified != null ? lastModified : Instant.now()) // Fallback to now |
There was a problem hiding this comment.
Silently ignoring IOException when fetching file modification time could hide legitimate issues like permission problems or file system errors. If the lastModified value is critical for caching (which it should be given the PR's goals), consider logging this exception or handling it more explicitly.
Additionally, using Instant.now() as a fallback creates inconsistent caching behavior - every request will have a different "last modified" timestamp, preventing any caching benefits from working correctly.
| } catch (IOException ignored) {} | |
| return ResponseEntity.ok() | |
| .cacheControl(CacheControl.noCache()) | |
| .lastModified(lastModified != null ? lastModified : Instant.now()) // Fallback to now | |
| } catch (IOException e) { | |
| log.warn("Failed to get last modified time for file: {}", filePath, e); | |
| } | |
| ResponseEntity.BodyBuilder responseBuilder = ResponseEntity.ok() | |
| .cacheControl(CacheControl.noCache()); | |
| if (lastModified != null) { | |
| responseBuilder = responseBuilder.lastModified(lastModified); | |
| } | |
| return responseBuilder |
| @@ -33,6 +33,7 @@ | |||
| import jakarta.validation.constraints.Min; | |||
| import lombok.AllArgsConstructor; | |||
| import org.springframework.core.io.ByteArrayResource; | |||
There was a problem hiding this comment.
This import is no longer used after switching to InputStreamResource and should be removed.
| import org.springframework.core.io.ByteArrayResource; |
| .cacheControl(CacheControl.noCache()) | ||
| .lastModified(lastModified != null ? lastModified : Instant.now()) // Fallback to now | ||
| .contentType(MediaType.APPLICATION_OCTET_STREAM) | ||
| .body(new InputStreamResource(new FileInputStream(filePath))); |
There was a problem hiding this comment.
Critical resource leak: The FileInputStream created here is never closed. When using InputStreamResource, Spring does not automatically close the underlying stream, which will lead to file descriptor leaks over time.
The codebase already has an established pattern for serving file resources using FileSystemResource, which automatically handles resource cleanup. See BookDownloadService.java:76-77, BookDropService.java:102, and CustomFontService.java:179 for examples.
Instead of InputStreamResource with a raw FileInputStream, use FileSystemResource which properly manages the file resource lifecycle and will still provide streaming behavior without loading the entire file into memory.
| .cacheControl(CacheControl.noCache()) | ||
| .lastModified(lastModified != null ? lastModified : Instant.now()) // Fallback to now |
There was a problem hiding this comment.
This approach contradicts the stated performance goals of the PR. Using Cache-Control: no-cache means the client must revalidate with the server on every request, which doesn't provide caching benefits. Additionally, using Last-Modified with Instant.now() as a fallback defeats the purpose of conditional requests since it will always appear modified.
For better performance, consider:
- Using CacheControl.maxAge() with a reasonable duration to allow client-side caching
- Only setting lastModified when the actual file modification time is available (don't use Instant.now() as fallback)
- Supporting If-Modified-Since headers for true conditional requests
Note: If the intention is to ensure fresh content due to potential file updates, document this explicitly and consider if ETag headers would be more appropriate than Last-Modified.
| assertNotNull(response.getBody()); | ||
| assertArrayEquals("hello".getBytes(), response.getBody().getContentAsByteArray()); |
There was a problem hiding this comment.
This test defeats the entire purpose of streaming by calling getContentAsByteArray(), which loads the entire file content into memory. This is exactly what the PR aims to avoid in production.
The test should verify that:
- The response body is an InputStreamResource (or better, FileSystemResource as per codebase conventions)
- The resource is properly configured
- HTTP headers are correctly set (cache control, last modified, content type)
Avoid reading the entire stream content in the test, as this doesn't reflect real-world usage where the framework streams the content to the client.
1 participant
🚀 Pull Request
📝 Description
This PR enhances
Read Bookperformance by enable HTTP Caching and employsInputStreamResourcefor memory efficiency🛠️ Changes Implemented
ByteArrayResourcewithInputStreamResourcefor book content delivery.Cache-Control: no-cacheandLast-Modifiedheaders to responses.🧪 Testing Strategy
📸 Visual Changes (if applicable)
Please Read - This Checklist is Mandatory
Mandatory Requirements (please check ALL boxes):
developbranch (please resolve any merge conflicts)./gradlew testfor Spring Boot backend, andng testfor Angular frontend - NO EXCEPTIONS)Why This Matters:
Recent production incidents have been traced back to:
Backend changes without tests will not be accepted. By completing this checklist thoroughly, you're helping maintain the quality and stability of Booklore for all users.
Note to Reviewers: Please verify the checklist is complete before beginning your review. If items are unchecked, kindly ask the contributor to complete them first.
💬 Additional Context (optional)