Features/loan#10
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe changes refactor the loan management model from ID-based to object-based relationships. Book gains accessors for its Loan relationship. Loan replaces primitive ID fields with User and Book object references through JPA annotations. A new LoanServices class provides business logic for loan operations. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant LS as LoanServices
participant EM as EntityManager
participant DB as Database
participant B as Book
participant U as User
participant L as Loan
App->>LS: loanBook(bookId, userId)
LS->>LS: isBookLoaned(bookId)
LS->>EM: Query Loan entities
EM->>DB: SELECT loan WHERE bookId=? AND returnDate=null
DB-->>EM: Result
alt Book already loaned
LS-->>App: false
else Book available
LS->>EM: Find User by userId
EM->>DB: SELECT user WHERE id=userId
DB-->>EM: User entity
LS->>EM: Find Book by bookId
EM->>DB: SELECT book WHERE id=bookId
DB-->>EM: Book entity
LS->>L: Create Loan(loanDate=now, returnDate=null)
L->>B: Set book reference
B->>L: Set loan reference
LS->>EM: Persist Loan
EM->>DB: INSERT into Loan
DB-->>EM: Success
LS-->>App: true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In @src/main/java/org/example/LoanServices.java:
- Around line 53-75: The returnBook method currently starts a transaction with
em.getTransaction().begin() but does not rollback on exception and also uses
entity parameters unlike loanBook(Long, Long); update returnBook(User, Book) to
wrap the transaction in a try-catch-finally (or use a transaction helper) so
that on any exception you call em.getTransaction().rollback() before rethrowing
or returning false, and ensure the transaction is committed only on success;
additionally align the API by either adding a returnBook(Long userId, Long
bookId) overload (mirroring loanBook) or change loanBook to accept entities so
both methods use the same parameter style and lookup logic (refer to returnBook,
loanBook, em, Loan, setReturnDate).
- Around line 33-50: In loanBook(Long bookId, Long userId) ensure you 1)
validate em.find results (User user and Book book) and return false or throw if
either is null; 2) start and commit/rollback a transaction around the
persistence (use EntityTransaction tx =
em.getTransaction()/tx.begin()/tx.commit() and tx.rollback() on exceptions); and
3) update the owning side before persisting by calling book.setLoan(loan) (in
addition to loan.setBook(book)) so the @JoinColumn is saved; also wrap
persistence in try/catch to rollback the transaction on error.
🧹 Nitpick comments (1)
src/main/java/org/example/LoanServices.java (1)
16-30: Simplify the boolean return.The conditional block can be reduced to a single expression.
♻️ Suggested simplification
- if (loans.isEmpty()) { - return false; - } else { - return true; - } + return !loans.isEmpty();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/example/Book.javasrc/main/java/org/example/Loan.javasrc/main/java/org/example/LoanServices.java
🔇 Additional comments (3)
src/main/java/org/example/Book.java (1)
67-85: LGTM with a note on field ordering.The accessors are correctly implemented. Note that
Bookis the owning side of the@OneToOnerelationship (it has@JoinColumn), sobook.setLoan(loan)must be called to persist the association—setting onlyloan.setBook(book)won't update the foreign key.Consider moving the
loanfield declaration (lines 84-85) to be grouped with other fields at the top of the class for consistency.src/main/java/org/example/Loan.java (2)
55-59: Verify the relationship ownership design.The
@OneToOne(mappedBy = "loan")makesLoanthe inverse side of the Book-Loan relationship. This means:
Bookowns the foreign key (loaned_bookcolumn)- Updates to
loan.bookalone won't persist—book.loanmust also be setThis is semantically unusual (typically a Loan would "own" its Book reference). If intentional, ensure all service code sets the owning side (
book.setLoan(loan)). Consider also moving field declarations above accessors for conventional ordering.
19-33: Accessors look correct.The object-based getters/setters properly replace the previous ID-based accessors.
Comment on lines
+33
to
+50
| public boolean loanBook(Long bookId, Long userId) { | ||
|
|
||
| if (isBookLoaned(bookId)) { | ||
| return false; | ||
| } | ||
|
|
||
| User user = em.find(User.class, userId); | ||
| Book book = em.find(Book.class, bookId); | ||
|
|
||
| Loan loan = new Loan(); | ||
|
|
||
| loan.setUser(user); | ||
| loan.setBook(book); | ||
| loan.setLoanDate(ZonedDateTime.now()); | ||
| loan.setReturnDate(null); | ||
| em.persist(loan); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Critical: Owning-side not updated, missing transaction and null checks.
Several issues in this method:
- Relationship not persisted:
Bookis the owning side (@JoinColumn), so you must callbook.setLoan(loan)for the FK to be saved—loan.setBook(book)alone won't persist the association. - No transaction:
em.persist()requires an active transaction. - No null checks:
em.find()returnsnullif the entity doesn't exist—dereferencing will cause NPE.
🐛 Proposed fix
public boolean loanBook(Long bookId, Long userId) {
if (isBookLoaned(bookId)) {
return false;
}
User user = em.find(User.class, userId);
Book book = em.find(Book.class, bookId);
+ if (user == null || book == null) {
+ return false;
+ }
+
+ em.getTransaction().begin();
+
Loan loan = new Loan();
loan.setUser(user);
loan.setBook(book);
loan.setLoanDate(ZonedDateTime.now());
- loan.setReturnDate(null);
+ book.setLoan(loan); // Set owning side to persist the FK
+
em.persist(loan);
+ em.getTransaction().commit();
return true;
}🤖 Prompt for AI Agents
In @src/main/java/org/example/LoanServices.java around lines 33 - 50, In
loanBook(Long bookId, Long userId) ensure you 1) validate em.find results (User
user and Book book) and return false or throw if either is null; 2) start and
commit/rollback a transaction around the persistence (use EntityTransaction tx =
em.getTransaction()/tx.begin()/tx.commit() and tx.rollback() on exceptions); and
3) update the owning side before persisting by calling book.setLoan(loan) (in
addition to loan.setBook(book)) so the @JoinColumn is saved; also wrap
persistence in try/catch to rollback the transaction on error.
Comment on lines
+53
to
+75
| public boolean returnBook(User user, Book book) { | ||
|
|
||
| Loan loan = em.createQuery( | ||
| "SELECT l FROM Loan l WHERE l.user = :user AND l.book = :book AND l.returnDate IS NULL", | ||
| Loan.class | ||
| ) | ||
| .setParameter("user", user) | ||
| .setParameter("book", book) | ||
| .getResultStream() | ||
| .findFirst() | ||
| .orElse(null); | ||
|
|
||
| if (loan == null) { | ||
| return false; | ||
| } | ||
|
|
||
| em.getTransaction().begin(); | ||
|
|
||
| loan.setReturnDate(ZonedDateTime.now()); | ||
| book.setLoan(null); | ||
| em.getTransaction().commit(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Missing rollback handling and inconsistent API design.
- No rollback on failure: If an exception occurs between
begin()andcommit(), the transaction is left dangling. Wrap in try-catch or use try-with-resources with a transaction wrapper. - Inconsistent API:
loanBook(Long, Long)takes IDs whilereturnBook(User, Book)takes entities. Consider aligning the signatures for consistency.
🔧 Suggested fix with rollback handling
public boolean returnBook(User user, Book book) {
Loan loan = em.createQuery(
"SELECT l FROM Loan l WHERE l.user = :user AND l.book = :book AND l.returnDate IS NULL",
Loan.class
)
.setParameter("user", user)
.setParameter("book", book)
.getResultStream()
.findFirst()
.orElse(null);
if (loan == null) {
return false;
}
- em.getTransaction().begin();
-
- loan.setReturnDate(ZonedDateTime.now());
- book.setLoan(null);
- em.getTransaction().commit();
- return true;
+ try {
+ em.getTransaction().begin();
+ loan.setReturnDate(ZonedDateTime.now());
+ book.setLoan(null);
+ em.getTransaction().commit();
+ return true;
+ } catch (Exception e) {
+ if (em.getTransaction().isActive()) {
+ em.getTransaction().rollback();
+ }
+ throw e;
+ }
}🤖 Prompt for AI Agents
In @src/main/java/org/example/LoanServices.java around lines 53 - 75, The
returnBook method currently starts a transaction with
em.getTransaction().begin() but does not rollback on exception and also uses
entity parameters unlike loanBook(Long, Long); update returnBook(User, Book) to
wrap the transaction in a try-catch-finally (or use a transaction helper) so
that on any exception you call em.getTransaction().rollback() before rethrowing
or returning false, and ensure the transaction is committed only on success;
additionally align the API by either adding a returnBook(Long userId, Long
bookId) overload (mirroring loanBook) or change loanBook to accept entities so
both methods use the same parameter style and lookup logic (refer to returnBook,
loanBook, em, Loan, setReturnDate).
This was referenced Jan 13, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
✏️ Tip: You can customize this high-level summary in your review settings.