Skip to content

perf(api): Enable Http Caching & stream Book Content instead of load to memory #2494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
NamLe029 wants to merge 6 commits into from
Closed

perf(api): Enable Http Caching & stream Book Content instead of load to memory #2494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
33a98c4
perf(api): Enable Http Caching & Streaming Book Content instead of
NamLe029 Jan 26, 2026
fe6a831
Merge branch 'develop' into readBookService
NamLe029 Jan 28, 2026
ef517a8
rename test case
NamLe029 Jan 28, 2026
dd12b1c
oops, forgot
NamLe029 Jan 28, 2026
95e94f8
Merge branch 'upstream' into readBookService
NamLe029 Feb 4, 2026
db4c404
private cache & merge upstream
NamLe029 Feb 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions booklore-api/src/main/java/org/booklore/controller/BookController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import jakarta.validation.constraints.Max;
import jakarta.validation.constraints.Min;
import lombok.AllArgsConstructor;
import org.springframework.core.io.ByteArrayResource;
import org.springframework.core.io.Resource;
import org.springframework.http.ResponseEntity;
import org.springframework.security.access.prepost.PreAuthorize;
Expand Down Expand Up @@ -125,7 +124,7 @@ public ResponseEntity<?> getComicInfoMetadata(
@ApiResponse(responseCode = "200", description = "Book content returned successfully")
@GetMapping("/{bookId}/content")
@CheckBookAccess(bookIdParam = "bookId")
public ResponseEntity<ByteArrayResource> getBookContent(
public ResponseEntity<Resource> getBookContent(
@Parameter(description = "ID of the book") @PathVariable long bookId,
@Parameter(description = "Optional book type for alternative format (e.g., EPUB, PDF, MOBI)") @RequestParam(required = false) String bookType) throws IOException {
return bookService.getBookContent(bookId, bookType);
Expand Down
27 changes: 17 additions & 10 deletions booklore-api/src/main/java/org/booklore/service/book/BookService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,8 @@
import jakarta.servlet.http.HttpServletResponse;
import lombok.AllArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.core.io.ByteArrayResource;
import org.springframework.core.io.ClassPathResource;
import org.springframework.core.io.Resource;
import org.springframework.core.io.UrlResource;
import org.springframework.core.io.*;
import org.springframework.http.CacheControl;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.http.ResponseEntity;
Expand All @@ -39,6 +37,7 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.Instant;
import java.util.*;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -265,13 +264,14 @@ public void downloadAllBookFiles(Long bookId, HttpServletResponse response) {
bookDownloadService.downloadAllBookFiles(bookId, response);
}

public ResponseEntity<ByteArrayResource> getBookContent(long bookId) throws IOException {
public ResponseEntity<Resource> getBookContent(long bookId) throws IOException {
return getBookContent(bookId, null);
}

public ResponseEntity<ByteArrayResource> getBookContent(long bookId, String bookType) throws IOException {
public ResponseEntity<Resource> getBookContent(long bookId, String bookType) throws IOException {
BookEntity bookEntity = bookRepository.findById(bookId).orElseThrow(() -> ApiError.BOOK_NOT_FOUND.createException(bookId));
String filePath;
Instant lastModified = null;
if (bookType != null) {
BookFileType requestedType = BookFileType.valueOf(bookType.toUpperCase());
BookFileEntity bookFile = bookEntity.getBookFiles().stream()
Expand All @@ -282,11 +282,18 @@ public ResponseEntity<ByteArrayResource> getBookContent(long bookId, String book
} else {
filePath = FileUtils.getBookFullPath(bookEntity);
}
try (FileInputStream inputStream = new FileInputStream(filePath)) {
return ResponseEntity.ok()
.contentType(MediaType.APPLICATION_OCTET_STREAM)
.body(new ByteArrayResource(inputStream.readAllBytes()));

try {
lastModified = Files.getLastModifiedTime(Paths.get(filePath)).toInstant();
} catch (IOException e) {
log.warn("Failed to get last modified time for: {}", filePath, e);
}

return ResponseEntity.ok()
.cacheControl(CacheControl.noCache().cachePrivate())
.lastModified(lastModified != null ? lastModified : Instant.now()) // Fallback to now
.contentType(MediaType.APPLICATION_OCTET_STREAM)
.body(new InputStreamResource(new FileInputStream(filePath)));
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

@Transactional
Expand Down
12 changes: 5 additions & 7 deletions booklore-api/src/test/java/org/booklore/service/book/BookServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,7 @@
import org.mockito.Mock;
import org.mockito.MockedStatic;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.core.io.ByteArrayResource;
import org.springframework.core.io.ClassPathResource;
import org.springframework.core.io.Resource;
import org.springframework.core.io.UrlResource;
import org.springframework.core.io.*;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;

Expand Down Expand Up @@ -318,17 +315,18 @@ void downloadBook_delegatesToDownloadService() {
}

@Test
void getBookContent_returnsByteArrayResource() throws Exception {
void getBookContent_returnsResource() 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<ByteArrayResource> response = bookService.getBookContent(10L);
ResponseEntity<Resource> response = bookService.getBookContent(10L);
assertEquals(HttpStatus.OK, response.getStatusCode());
assertArrayEquals("hello".getBytes(), response.getBody().getByteArray());
assertNotNull(response.getBody());
assertArrayEquals("hello".getBytes(), response.getBody().getContentAsByteArray());
Comment on lines +328 to +329
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The response body is an InputStreamResource (or better, FileSystemResource as per codebase conventions)
  2. The resource is properly configured
  3. 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.

Copilot uses AI. Check for mistakes.
} finally {
Files.deleteIfExists(path);
}
Expand Down