booklore-api/src/main/java/org/booklore/service/AuthorMetadataService.java (4)

192-212: ⚠️ Potential issue | 🟡 Minor

Rollback semantics: file deletion is not transactional.

fileService.deleteAuthorImages(authorId) is called at line 206 before authorRepository.delete(author). If the subsequent delete (or commit, e.g., FK/constraint violation from the BookMetadataEntity association cleanup at 200–204) fails, the images are already gone on disk but the row remains — permanent data inconsistency. Consider deleting files only after the transaction commits (e.g., via TransactionSynchronizationManager.registerSynchronization / @TransactionalEventListener(phase = AFTER_COMMIT)), or reorder so the DB delete is flushed first and files are removed post-commit.

The same observation applies to unmatchAuthors at line 185.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@booklore-api/src/main/java/org/booklore/service/AuthorMetadataService.java`
around lines 192 - 212, The deleteAuthors method currently calls
fileService.deleteAuthorImages(authorId) before the DB delete, which can leave
files removed if the transaction later rolls back; move file deletion to after
successful commit by deferring deletion (either register a post-commit callback
with TransactionSynchronizationManager.registerSynchronization or publish an
event handled by a `@TransactionalEventListener`(phase = AFTER_COMMIT)) so that
fileService.deleteAuthorImages runs only after authorRepository.delete(...) has
committed; apply the same deferral pattern to unmatchAuthors where
fileService.deleteAuthorImages is invoked.

---

214-278: ⚠️ Potential issue | 🟠 Major

Same tx-scope concern for photo upload methods.

uploadAuthorPhoto holds the write transaction across FileService.readImage, fileService.saveAuthorImages (and image.flush()), and uploadAuthorPhotoFromUrl holds it across fileService.createAuthorThumbnailFromUrl (which typically downloads from a remote URL and writes to disk). No DB mutation actually happens after the initial lookup in either method — only the findById needs the persistence context.

Suggested narrowing: drop @Transactional here and either (a) rely on a readOnly = true lookup via an extracted loader bean, or (b) move the lookup into a short @Transactional(readOnly = true) method and do all I/O afterward. This also avoids holding a DB connection for the duration of potentially large image decodes and network fetches.

Based on learnings: "Option B: extract the ... lookup into a dedicated Service bean ... annotated with Transactional(readOnly = true), so that the proxy always narrows the transaction to the DB access only and all filesystem I/O runs outside the persistence context."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@booklore-api/src/main/java/org/booklore/service/AuthorMetadataService.java`
around lines 214 - 278, Both uploadAuthorPhoto and uploadAuthorPhotoFromUrl keep
a write transaction open for long-running I/O (FileService.readImage,
fileService.saveAuthorImages, image.flush,
fileService.createAuthorThumbnailFromUrl) even though only the initial
authorRepository.findById call needs DB access; extract the DB lookup into a
small transactional(readOnly = true) helper (e.g., loadAuthorOrThrow(Long)
annotated with `@Transactional`(readOnly = true)) or move the findById call into a
dedicated loader bean, then remove `@Transactional` from uploadAuthorPhoto and
uploadAuthorPhotoFromUrl so all image decoding/networking
(FileService.readImage, fileService.saveAuthorImages,
fileService.createAuthorThumbnailFromUrl) runs outside the persistence context
and the connection is not held during I/O.

---

98-149: ⚠️ Potential issue | 🟠 Major

External HTTP and file I/O executed inside the @Transactional boundary.

Both matchAuthor and quickMatchAuthor perform remote provider calls (provider.getAuthorByAsin / provider.quickSearch) and filesystem/URL I/O (fileService.createAuthorThumbnailFromUrl) while a write transaction — and therefore a JDBC connection — is held open. Slow/hung upstreams will pin connections from the pool and can cascade into outages; this is exactly the anti-pattern called out for this codebase.

Preferred refactor (Option B from prior learnings): keep the transactional boundary narrow to DB access only. Extract an AuthorEntityLoader/writer bean with @Transactional(readOnly = true) for the lookup and a separate @Transactional method for the save, and run the HTTP fetch and thumbnail download between them, outside any persistence context.

Based on learnings: "for the AudiobookReaderService pattern of wrapping a DB lookup + file I/O under a single Transactional boundary, the preferred refactoring approach is Option B: extract the ... lookup into a dedicated Service bean ... annotated with Transactional(readOnly = true), so that the proxy always narrows the transaction to the DB access only and all filesystem I/O runs outside the persistence context."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@booklore-api/src/main/java/org/booklore/service/AuthorMetadataService.java`
around lines 98 - 149, Both matchAuthor and quickMatchAuthor hold a DB
transaction while calling external HTTP (provider.getAuthorByAsin /
provider.quickSearch) and doing file I/O
(fileService.createAuthorThumbnailFromUrl); refactor to narrow transactions by
extracting a new AuthorEntityLoader bean with a `@Transactional`(readOnly = true)
method (e.g., loadAuthorForMatch(Long authorId) that returns AuthorEntity or
throws ApiError.AUTHOR_NOT_FOUND) and a separate `@Transactional` writer method
(e.g., saveAuthorMetadata(AuthorEntity) or in AuthorMetadataService.saveAuthor).
Change matchAuthor and quickMatchAuthor to: call the loader to fetch the
AuthorEntity (within readOnly tx), then perform provider calls and thumbnail
download outside any transaction, then call the transactional save method to
persist changes (use applyMetadataResult before save); keep auditService.log
after successful save. Ensure you reference and update usages of
provider.getAuthorByAsin, provider.quickSearch,
fileService.createAuthorThumbnailFromUrl, applyMetadataResult,
authorRepository.save and auditService.log accordingly.

---

151-174: ⚠️ Potential issue | 🔴 Critical

Self-invocation in autoMatchAuthors bypasses @Transactional on quickMatchAuthor, and transaction wraps file I/O and external HTTP calls.

At line 157, autoMatchAuthors calls quickMatchAuthor(authorId, "us") within a Mono.fromCallable lambda. The Spring proxy is not invoked for this self-call, so the @Transactional annotation on quickMatchAuthor (line 127) is silently bypassed. Additionally, even if the proxy were used, the execution runs on boundedElastic scheduler, which executes on a worker thread not bound to the transaction context.

More critically, quickMatchAuthor wraps multiple concerns in a single @Transactional boundary:

• External HTTP calls via provider.quickSearch() (line 132)
• File I/O via fileService.createAuthorThumbnailFromUrl() (line 137)
• DB writes via authorRepository.save() (line 135)

This pattern holds a DB connection throughout external I/O and network operations, which is the anti-pattern documented in the learning (PR 513): extract the DB lookup into a separate @Service bean with @Transactional(readOnly = true) so file I/O and external calls run outside the persistence context. Do not use TransactionTemplate or @Lazy self-references for this.

Required: Extract a new @Service bean (e.g., AuthorEntityLoader) to handle the entity lookup and metadata apply in isolation under a read-only transaction. Call this bean from both matchAuthor and autoMatchAuthors, and move all file I/O and external provider calls outside the transaction.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@booklore-api/src/main/java/org/booklore/service/AuthorMetadataService.java`
around lines 151 - 174, autoMatchAuthors is self-calling quickMatchAuthor and
running it on boundedElastic which bypasses the Spring transactional proxy and
holds a transaction across I/O and HTTP calls; fix by extracting the DB lookup
into a new `@Service` bean (e.g., AuthorEntityLoader) with a method like
loadAuthorEntityForMatch(Long authorId) annotated `@Transactional`(readOnly =
true) that fetches and returns the AuthorEntity (or a detached DTO) only; update
quickMatchAuthor and autoMatchAuthors to call
AuthorEntityLoader.loadAuthorEntityForMatch(authorId) first, then perform
provider.quickSearch(...), fileService.createAuthorThumbnailFromUrl(...) and
Files.exists(...) outside that transaction on the boundedElastic scheduler, and
only persist changes with a separate transactional save call (or by moving
repository.save into a brief `@Transactional` method) after external I/O
completes.