Conversation
Basic tests also implmented. Co-authored-by: Oscar Nidemar <oscar.nidemar@gmail.com> Co-authored-by: Oskar Lundqvist <oskar.lundqvist3@icloud.com> Co-authored-by: Mark Cavric <cavric_123@hotmail.se> Co-authored-by: Linus Westling <westling.linus@gmail.com>
Co-authored-by: Oscar Nidemar <oscar.nidemar@gmail.com> Co-authored-by: Oskar Lundqvist <oskar.lundqvist3@icloud.com> Co-authored-by: Mark Cavric <cavric_123@hotmail.se> Co-authored-by: Linus Westling <westling.linus@gmail.com>
…ontiued implementation of switch menue's. Co-authored-by: Oscar Nidemar <oscar.nidemar@gmail.com> Co-authored-by: Oskar Lundqvist <oskar.lundqvist3@icloud.com> Co-authored-by: Mark Cavric <cavric_123@hotmail.se> Co-authored-by: Linus Westling <westling.linus@gmail.com>
WalkthroughBootstraps runtime JPA seeding with a DB-empty check, adds JPA utilities and a transaction runner, introduces interactive CLI (user + admin), extends repository APIs and implementations (queries and signatures), updates seed data/tests, and adds H2 test dependency plus Surefire config. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant App as App.main
participant JpaR as JpaRunner
participant EM as EntityManager / DB
participant Seeder as DatabaseFiller
participant Repo as Repositories
User->>App: start
App->>JpaR: runInTransaction(isDatabaseEmpty)
JpaR->>EM: obtain EM, begin tx
EM->>Repo: count Movies
Repo-->>EM: result
EM-->>JpaR: commit
JpaR-->>App: returns empty?
alt DB empty
App->>JpaR: runInTransaction(seedAll)
JpaR->>EM: begin tx
Seeder->>Repo: persist genres, actors, directors, movies, users, relations
Repo-->>EM: persist ops
EM-->>JpaR: commit
JpaR-->>App: seed completed
end
App->>User: prompt login
User->>App: credentials
App->>JpaR: runInTransaction(findUser)
JpaR->>EM: query user
EM-->>JpaR: user result
JpaR-->>App: user (or none)
alt admin
App->>User: dispatch to CliAdminApp (interactive transactions)
else normal user
App->>User: dispatch to CliApp (menus & repo queries)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
src/main/java/org/example/jpaimpl/DirectorRepoJpa.java-49-62 (1)
49-62: Fix copy-paste errors in comments.The comments at lines 57 and 61 incorrectly refer to "actor" instead of "director".
🔎 Apply this diff to correct the comments:
if (director == null) { - return false; // ingen actor hittades + return false; // ingen director hittades } em.remove(director); - return true; // actor togs bort + return true; // director togs bort }src/test/java/org/example/AppTest.java-236-237 (1)
236-237: Pass actual user ID instead of null.Line 237 calls
app.optionsUser(userRepoMock, movieRepoMock, null)withnullfor the User parameter, but the test setup doesn't show what user should be passed. This could cause issues ifoptionsUserexpects a non-null user.src/main/java/org/example/CliApp.java-331-331 (1)
331-331: Display rating result to user.Line 331 calls
getRatingForMovieByUserbut doesn't capture or display the result. The user won't see their rating.🔎 Suggested fix:
- movieOpt.ifPresent(movie -> userRatingRepoJpa.getRatingForMovieByUser(user, movie)); + movieOpt.ifPresent(movie -> { + Optional<Float> rating = userRatingRepoJpa.getRatingForMovieByUser(user, movie); + if (rating.isPresent()) { + System.out.println("Your rating for '" + movie.getTitle() + "': " + rating.get()); + } else { + System.out.println("You haven't rated '" + movie.getTitle() + "' yet."); + } + });src/main/java/org/example/CliApp.java-220-221 (1)
220-221: Unused Genre entity creation.A new
Genreentity is created but never used. The method call on line 223 usesgenreName(String) directly, not the createdGenreobject.Consider removing the unused entity creation:
System.out.println("***** You have selected find a movie by genre *****"); System.out.println("Enter Genre: "); String genreName = sc.nextLine(); - Genre genre = new Genre(); - genre.setName(genreName); - List<Movie> moviesByGenre = movieRepoJpa.getMovieByGenre(genreName); if (moviesByGenre.isEmpty()) { - System.out.println("No movies found in genre: " + genre.getName()); + System.out.println("No movies found in genre: " + genreName);
🧹 Nitpick comments (7)
src/main/java/org/example/jpaimpl/DirectorRepoJpa.java (1)
5-5: Remove unused import.The
Actorimport is not used in this file.🔎 Apply this diff to remove the unused import:
-import org.example.pojo.Actor; import org.example.pojo.Director;src/main/java/org/example/DatabaseFiller.java (1)
23-27: Consider checking multiple tables in isDatabaseEmpty.Currently,
isDatabaseEmptyonly checks theMovietable. If other tables (User, Actor, Director, Genre) contain data while Movie is empty, re-seeding could cause issues. While this might work given the current seeding strategy, it's fragile and could break if the seeding logic changes.🔎 Consider this more robust implementation:
public static boolean isDatabaseEmpty(EntityManager em) { Long movieCount = em.createQuery("SELECT COUNT(m) FROM Movie m", Long.class) .getSingleResult(); - return movieCount == 0; + Long userCount = em.createQuery("SELECT COUNT(u) FROM User u", Long.class) + .getSingleResult(); + return movieCount == 0 && userCount == 0; }src/test/java/org/example/AppTest.java (1)
32-39: Consider extracting persistence configuration to a helper method.The inline Map configuration in the test setup is verbose and could be reused. Consider extracting it to a test utility method for better maintainability.
src/main/java/org/example/CliAdminApp.java (1)
51-240: Consider extracting transaction management to reduce duplication.Each case block manually manages transactions with repetitive
begin()andcommit()calls. Consider using theJpaRunner.runInTransactionutility introduced in this PR to reduce duplication.src/main/java/org/example/CliApp.java (2)
296-296: Consider checking rateMovie return value.The
rateMoviemethod now returns a boolean to indicate success/failure, but the return value is ignored here. Consider checking it to provide user feedback.
24-109: Consider extracting menu loop pattern.The menu handling pattern (print options, loop with switch, handle exit) is repeated in all three option methods. Consider extracting this to a helper method to reduce duplication.
src/main/java/org/example/jpaimpl/MovieRepoJpa.java (1)
186-197: Add @OverRide annotation for consistency.The
getMovieByGenremethod appears to implement an interface method (per the AI summary), but is missing the@Overrideannotation unlike other interface methods in this class.+@Override public List<Movie> getMovieByGenre (String genre) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
NOTES.md(1 hunks)README.md(1 hunks)pom.xml(1 hunks)src/main/java/org/example/App.java(1 hunks)src/main/java/org/example/CliAdminApp.java(1 hunks)src/main/java/org/example/CliApp.java(1 hunks)src/main/java/org/example/DatabaseFiller.java(2 hunks)src/main/java/org/example/JpaRunner.java(1 hunks)src/main/java/org/example/JpaUtil.java(1 hunks)src/main/java/org/example/jpaimpl/ActorRepoJpa.java(1 hunks)src/main/java/org/example/jpaimpl/DirectorRepoJpa.java(2 hunks)src/main/java/org/example/jpaimpl/MovieRepoJpa.java(5 hunks)src/main/java/org/example/jpaimpl/UserRatingRepoJpa.java(2 hunks)src/main/java/org/example/repo/MovieRepo.java(1 hunks)src/main/java/org/example/repo/UserRatingRepo.java(1 hunks)src/main/java/org/example/seed/SeedMovieRelations.java(0 hunks)src/main/java/org/example/seed/SeedMovies.java(0 hunks)src/main/java/org/example/seed/SeedUsers.java(1 hunks)src/test/java/org/example/AppTest.java(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/org/example/seed/SeedMovies.java
- src/main/java/org/example/seed/SeedMovieRelations.java
🧰 Additional context used
🧬 Code graph analysis (4)
src/main/java/org/example/CliAdminApp.java (1)
src/main/java/org/example/seed/SeedUsers.java (1)
SeedUsers(9-153)
src/main/java/org/example/CliApp.java (3)
src/main/java/org/example/jpaimpl/MovieRepoJpa.java (1)
MovieRepoJpa(20-224)src/main/java/org/example/jpaimpl/UserRatingRepoJpa.java (1)
UserRatingRepoJpa(12-124)src/main/java/org/example/jpaimpl/UserRepoJpa.java (1)
UserRepoJpa(13-135)
src/main/java/org/example/JpaRunner.java (1)
src/main/java/org/example/JpaUtil.java (1)
JpaUtil(9-34)
src/test/java/org/example/AppTest.java (2)
src/main/java/org/example/jpaimpl/MovieRepoJpa.java (1)
MovieRepoJpa(20-224)src/main/java/org/example/jpaimpl/UserRepoJpa.java (1)
UserRepoJpa(13-135)
🪛 GitHub Actions: Java Maven Build
pom.xml
[error] 1-1: Maven Surefire test run failed. 10 errors detected during tests; build failed.
src/test/java/org/example/AppTest.java
[error] 44-44: NullPointerException: emf is null in setup(AppTest.java:44) when trying to create EntityManager from EntityManagerFactory.
[error] 49-49: NullPointerException: emf is null in tearDown(AppTest.java:49) when attempting to close EntityManager.
[error] 54-54: NullPointerException: emf is null in closeFactory(AppTest.java:54) when attempting to close EntityManagerFactory.
🔇 Additional comments (16)
pom.xml (1)
53-58: LGTM! H2 test dependency added appropriately.The H2 in-memory database dependency is correctly scoped for testing and supports the integration test suite.
NOTES.md (1)
87-129: LGTM! Clear documentation of domain model and use cases.The added documentation provides a comprehensive overview of the entities and their operations, which will help with understanding the system design.
README.md (1)
23-23: LGTM! Progress tracking updated.The checkbox update accurately reflects the completion of documenting design decisions.
src/main/java/org/example/DatabaseFiller.java (1)
29-54: LGTM! Well-structured seeding orchestration.The
seedAllmethod follows a logical progression: base entities first, then relations. This separation of concerns makes the seeding process clear and maintainable.src/main/java/org/example/JpaRunner.java (1)
11-22: LGTM: Transaction lifecycle is correctly managed.The transaction handling follows best practices: begins transaction, executes action, commits on success, rolls back on exception when active, and ensures EntityManager is closed in the finally block.
src/main/java/org/example/repo/MovieRepo.java (1)
28-28: LGTM: Genre-based query method added.The new
getMovieByGenremethod provides a useful query capability for filtering movies by genre.src/main/java/org/example/JpaUtil.java (1)
27-29: LGTM: EntityManager lifecycle methods are clean.The
getEntityManager()andclose()methods provide straightforward access to JPA resources.src/main/java/org/example/jpaimpl/UserRatingRepoJpa.java (1)
85-121: LGTM: Boolean return type improves API usability.The change from
voidtobooleanreturn type allows callers to detect success/failure. The null handling is now more graceful (returnsfalseinstead of throwing an exception), and the movie ranking recalculation logic is correct.src/main/java/org/example/CliAdminApp.java (1)
47-48: LGTM: Input handling is correct here.The
nextInt()followed bynextLine()pattern correctly consumes the newline character.src/main/java/org/example/App.java (1)
21-27: LGTM: Database seeding with empty check is well-designed.The conditional seeding approach prevents duplicate data and provides a helpful message when the database is already populated.
src/main/java/org/example/jpaimpl/MovieRepoJpa.java (6)
20-26: LGTM! Repository pattern correctly implemented.The class properly implements the
MovieRepointerface and follows standard dependency injection withEntityManager.
97-107: LGTM! Better exception handling with stream API.The refactoring from
getSingleResult()togetResultStream().findFirst()properly avoidsNoResultExceptionand provides cleaner Optional handling.
130-139: LGTM! Query logic is correct.The length validation and query implementation are appropriate.
154-165: LGTM! Language query is correctly implemented.The null check and query logic are appropriate.
167-184: LGTM! Standard query implementations.Both
getAllMovies()andgetById()follow JPA best practices.
172-179: This is a pure JPA application using resource-local transactions, not a Spring application.The methods
deleteMovie,setDirector,addActors, andaddGenresperform persistence operations (em.remove(),em.merge()) and are correctly used within transaction contexts. All current callers inCliAdminAppandDatabaseFillerexplicitly wrap these method calls withem.getTransaction().begin()andem.getTransaction().commit(), ensuring changes are persisted.However, the current design relies on caller responsibility to manage transactions. Consider either (1) documenting this transaction requirement clearly in method javadocs, or (2) wrapping these methods with a helper similar to
JpaRunner.runInTranscation()to enforce transaction boundaries at the method level.Likely an incorrect or invalid review comment.
| .jdbcUrl("jdbc:mysql://localhost:3306/codeTraumaTeam") | ||
| .jdbcUsername("user") | ||
| .jdbcPassword("password") |
There was a problem hiding this comment.
Externalize database credentials immediately.
Database credentials are hard-coded in source code, exposing them in version control. This is a critical security vulnerability that could lead to unauthorized database access.
🔎 Recommended approach:
Store credentials in environment variables or a configuration file outside version control:
- .jdbcUrl("jdbc:mysql://localhost:3306/codeTraumaTeam")
- .jdbcUsername("user")
- .jdbcPassword("password")
+ .jdbcUrl(System.getenv("DB_URL"))
+ .jdbcUsername(System.getenv("DB_USERNAME"))
+ .jdbcPassword(System.getenv("DB_PASSWORD"))Add a .env file to .gitignore and document the required environment variables in the README.
📝 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.
| .jdbcUrl("jdbc:mysql://localhost:3306/codeTraumaTeam") | |
| .jdbcUsername("user") | |
| .jdbcPassword("password") | |
| .jdbcUrl(System.getenv("DB_URL")) | |
| .jdbcUsername(System.getenv("DB_USERNAME")) | |
| .jdbcPassword(System.getenv("DB_PASSWORD")) |
🤖 Prompt for AI Agents
In src/main/java/org/example/JpaUtil.java around lines 15 to 17, the database
URL, username and password are hard-coded which leaks credentials; change the
code to read these values from environment variables or an external config
(e.g., System.getenv or a properties file loaded from outside version control),
add instructions to document required env vars in the README, and ensure any
.env or config file is added to .gitignore so secrets are never committed.
| Optional<Double> getAverageRatingForMovie(Movie movie); | ||
|
|
||
| void rateMovie(User user, Movie movie, float rating); | ||
| boolean rateMovie(User user, Movie movie, float rating); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the UserRatingRepo interface definition
echo "=== Interface Definition ==="
find . -type f -name "*.java" | xargs grep -l "interface UserRatingRepo" 2>/dev/null
# Find the interface file and show its content
echo -e "\n=== UserRatingRepo Interface ==="
find . -type f -name "UserRatingRepo.java" | head -1 | xargs cat -n
# Find all implementations of UserRatingRepo
echo -e "\n=== Implementations of UserRatingRepo ==="
rg -nP --type=java "implements.*UserRatingRepo|class \w+ implements" -A 20
# Find all call sites of rateMovie
echo -e "\n=== Call sites of rateMovie ==="
rg -nP --type=java "\.rateMovie\s*\(" -B 2 -A 3
# Find method implementations within classes
echo -e "\n=== rateMovie implementations ==="
ast-grep --pattern $'rateMovie($$$) {
$$$
}'Repository: ithsjava25/project-jpa-grupp-5-d
Length of output: 13193
🏁 Script executed:
#!/bin/bash
# Show the complete UserRatingRepoJpa file to find rateMovie implementation
echo "=== UserRatingRepoJpa full content ==="
cat -n src/main/java/org/example/jpaimpl/UserRatingRepoJpa.java
# Also search for the rateMovie method specifically
echo -e "\n=== rateMovie method in detail ==="
rg -nP --type=java -A 15 "public.*rateMovie" src/main/java/org/example/jpaimpl/UserRatingRepoJpa.javaRepository: ithsjava25/project-jpa-grupp-5-d
Length of output: 5413
Update call sites to handle the boolean return value.
The interface and implementation correctly return boolean, but call sites ignore the return value:
CliApp.java:296prints success unconditionally without checking the resultSeedUserRelations.java:38discards the return value without error handling
Update both call sites to check the return value and handle failure cases (e.g., when user or movie is null).
🤖 Prompt for AI Agents
In src/main/java/org/example/repo/UserRatingRepo.java around line 15 and at the
call sites src/main/java/org/example/cli/CliApp.java line ~296 and
src/main/java/org/example/seed/SeedUserRelations.java line ~38, the rateMovie
method returns boolean but callers ignore it; update both call sites to check
the returned boolean and handle failures: after calling rateMovie, if it returns
true proceed with the success flow, if false log/report an error (e.g., print a
failure message, return an error code, or throw a controlled exception), and
optionally pre-check for null user or movie before calling to give a clearer
error message. Ensure CliApp only prints success when rateMovie returns true,
and SeedUserRelations handles false by logging or retrying as appropriate.
| UserRepoJpa userRepo = new UserRepoJpa(em); | ||
| Map<String, User> users = new HashMap<>(); | ||
|
|
||
| users.put("Admin", userRepo.addUser("Admin", "Admin")); |
There was a problem hiding this comment.
Security concern: Weak hardcoded Admin credentials.
The Admin user is created with the password "Admin", which is extremely weak and predictable. While this may be acceptable for development seeding, it poses a security risk if this code is ever used in a production environment.
Consider one of these approaches:
- Use environment variables for admin credentials:
String adminPassword = System.getenv("ADMIN_PASSWORD");
if (adminPassword == null || adminPassword.isEmpty()) {
adminPassword = "Admin"; // fallback for dev/test only
}
users.put("Admin", userRepo.addUser("Admin", adminPassword));- At minimum, add a warning comment:
// WARNING: This is test data only. Never use these credentials in production!
users.put("Admin", userRepo.addUser("Admin", "Admin"));- Consider not seeding admin users and instead requiring admins to be created through a secure setup process.
🤖 Prompt for AI Agents
In src/main/java/org/example/seed/SeedUsers.java around line 21, the Admin user
is created with a weak hardcoded password "Admin"; replace this with a secure
approach: read the admin password from an environment variable (with a clear
dev-only fallback) or remove seeding of admin accounts entirely, and if you must
keep a hardcoded value add an explicit warning comment and ensure the repository
never ships that value to production; implement the env-var read and fallback
logic, or remove the seed and document the secure admin creation process in
deployment notes.
Co-authored-by: Oscar Nidemar <oscar.nidemar@gmail.com> Co-authored-by: Oskar Lundqvist <oskar.lundqvist3@icloud.com> Co-authored-by: Mark Cavric <cavric_123@hotmail.se> Co-authored-by: Linus Westling <westling.linus@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/org/example/jpaimpl/MovieRepoJpa.java (1)
109-117: Stub implementations still return empty lists.These methods are called by
CliAppbut always return empty results, causing silent failures. This was flagged in a previous review and remains unimplemented.
🧹 Nitpick comments (1)
src/main/java/org/example/App.java (1)
63-67: Consider using a role field instead of hardcoded username check.Checking
user.getUserName().equals("Admin")is brittle—it's case-sensitive and ties admin detection to a specific username string. If theUserentity has (or could have) anisAdminflag or role field, use that instead for more robust authorization.🔎 Suggested approach
- if (user.getUserName().equals("Admin")) { + if (user.isAdmin()) { // or user.getRole() == Role.ADMIN new CliAdminApp(em).printOptions(); } else { new CliApp().runUserMenu(sc, userRepoJpa, userRatingRepoJpa, movieRepoJpa, user); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/example/App.java(1 hunks)src/main/java/org/example/CliApp.java(1 hunks)src/main/java/org/example/JpaRunner.java(1 hunks)src/main/java/org/example/jpaimpl/MovieRepoJpa.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/CliApp.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/org/example/JpaRunner.java (1)
src/main/java/org/example/JpaUtil.java (1)
JpaUtil(9-34)
🔇 Additional comments (5)
src/main/java/org/example/JpaRunner.java (1)
10-23: LGTM! Standard JPA transaction wrapper pattern.The implementation correctly handles the transaction lifecycle: begin, execute action, commit on success, rollback on
RuntimeException, and close theEntityManagerinfinally. The typo from the previous review has been fixed.src/main/java/org/example/App.java (1)
31-57: Login flow implementation is correct.The try-with-resources ensures the
EntityManageris closed, and the retry loop with user confirmation provides good UX. The previous scope issues from past reviews have been addressed—optionalUseris now properly scoped anduseris extracted after the loop.src/main/java/org/example/jpaimpl/MovieRepoJpa.java (3)
97-107: LGTM! Clean implementation offindByTitle.Properly validates input, uses stream to safely return first match as
Optional, avoidingNoResultExceptionhandling.
175-178: LGTM! Query methods are correctly implemented.
getAllMovies(),getById(), andgetMovieByGenre()follow proper JPA patterns with appropriate null checks and query structures.Also applies to: 189-192, 194-205
138-160: LGTM! Length and ranking queries are properly implemented.Both methods validate input bounds and construct correct JPQL queries with ordering.
| @Override | ||
| public List<Movie> getMovieByLanguage(Language language) { | ||
| if (language == null){ | ||
| return List.of(); | ||
| } | ||
|
|
||
| return em.createQuery( | ||
| "select m from Movie m where language = :language", Movie.class) | ||
| .setParameter("language", language) | ||
| .getResultList(); | ||
|
|
||
| } |
There was a problem hiding this comment.
Add entity alias to the language field in the query.
The query uses language = :language but should use m.language = :language for consistency with other queries and to avoid potential ambiguity.
🔎 Proposed fix
return em.createQuery(
- "select m from Movie m where language = :language", Movie.class)
+ "select m from Movie m where m.language = :language", Movie.class)
.setParameter("language", language)
.getResultList();📝 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.
| @Override | |
| public List<Movie> getMovieByLanguage(Language language) { | |
| if (language == null){ | |
| return List.of(); | |
| } | |
| return em.createQuery( | |
| "select m from Movie m where language = :language", Movie.class) | |
| .setParameter("language", language) | |
| .getResultList(); | |
| } | |
| @Override | |
| public List<Movie> getMovieByLanguage(Language language) { | |
| if (language == null){ | |
| return List.of(); | |
| } | |
| return em.createQuery( | |
| "select m from Movie m where m.language = :language", Movie.class) | |
| .setParameter("language", language) | |
| .getResultList(); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/example/jpaimpl/MovieRepoJpa.java around lines 162 to 173,
the JPQL query uses an unqualified language field ("language = :language") which
can be ambiguous; update the query to use the entity alias by changing the
predicate to "m.language = :language" so it consistently references the Movie
alias, keep the rest of the query and parameter setting the same.
Co-authored-by: Oscar Nidemar <oscar.nidemar@gmail.com> Co-authored-by: Oskar Lundqvist <oskar.lundqvist3@icloud.com> Co-authored-by: Mark Cavric <cavric_123@hotmail.se> Co-authored-by: Linus Westling <westling.linus@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/main/java/org/example/jpaimpl/MovieRepoJpa.java (2)
180-184: Add entity alias to thelanguagefield in the query.The query uses
language = :languagebut should usem.language = :languagefor consistency with other queries and to avoid potential ambiguity.
133-146: Handle potentialDateTimeParseExceptionfrom date parsing.
LocalDate.parse()with the custom formatter will throwDateTimeParseExceptionif the input doesn't match the expected "yyyyMMdd" format.
🧹 Nitpick comments (8)
src/main/java/org/example/CliAdminApp.java (3)
66-77: MissingnextLine()afternextLong()may cause issues in subsequent cases.After
scan.nextLong()at line 68, the newline character remains in the buffer. While this case doesn't read another line immediately, if the user continues using the menu, input behavior could become inconsistent. Consider addingscan.nextLine()after line 68 for consistency with other cases.🔎 Suggested fix
case 2 -> { System.out.println("Enter userId to delete:"); long userId = scan.nextLong(); + scan.nextLine(); // consume newline UserRepoJpa userRepo = new UserRepoJpa(em);
104-112: MissingnextLine()afternextLong().Same issue as case 2 - after
scan.nextLong()at line 106, the newline remains in the buffer.🔎 Suggested fix
case 4 -> { System.out.println("Enter movieId to delete:"); long movieId = scan.nextLong(); + scan.nextLine(); // consume newline MovieRepoJpa movieRepo = new MovieRepoJpa(em);
37-37: Remove or translate Swedish comment in menu text.The menu item contains "***** ta bort? enum?" which appears to be a development note in Swedish. This should be removed before release.
src/main/java/org/example/jpaimpl/MovieRepoJpa.java (1)
219-240: Silent failures when movie is not found.The helper methods
setDirector,addActors, andaddGenressilently do nothing when the movie title is not found. Consider returning a boolean or throwing an exception to inform callers of failure, especially since these are used in CLI flows where user feedback is important.🔎 Suggested approach
- public void setDirector(String title, Director director) { + public boolean setDirector(String title, Director director) { Movie m = findByTitle(title).orElse(null); if (m != null) { m.setDirector(director); em.merge(m); + return true; } + return false; }Apply similar changes to
addActorsandaddGenres.src/main/java/org/example/App.java (1)
65-66: Hardcoded admin username is fragile.Checking for admin status via
user.getUserName().equals("Admin")is case-sensitive and hardcoded. Consider using a role-based approach or at minimum case-insensitive comparison.🔎 Suggested improvement
- if (user.getUserName().equals("Admin")) { + if (user.getUserName().equalsIgnoreCase("Admin")) { new CliAdminApp(em).printOptions();Better long-term: Add an
isAdmin()method or role field to the User entity.src/main/java/org/example/CliApp.java (3)
378-378: Creating newScannerinstead of using passedScanner.The
optionsUserRatingmethod creates a newScanner(System.in)at line 378 instead of accepting aScannerparameter like other methods. This is inconsistent and could cause resource management issues.🔎 Suggested approach
public void optionsUserRating(MovieRepoJpa movieRepoJpa, UserRatingRepoJpa userRatingRepoJpa, - User user){ + User user, + Scanner sc){ printOptionsUserRating(); - Scanner sc = new Scanner(System.in); boolean running = true;Update the call site at line 49 accordingly.
17-17: Unused import:DirectorRepo.The
DirectorRepointerface is imported but never used.
242-243: Creating duplicate repository instances instead of using injected ones.Lines 242, 266, and 291 create new repository instances (
ActorRepoJpa,DirectorRepoJpa,GenreRepoJpa) even though equivalent repos are passed as parameters tooptionsMovies. Use the injected instances for consistency.🔎 Example fix for case 6
//hämtar actor-objektet - ActorRepoJpa actorRepo = new ActorRepoJpa(em); - Optional<Actor> actorOpt = actorRepo.findByName(actorName); + Optional<Actor> actorOpt = actorRepoJpa.findByName(actorName);Also applies to: 266-267, 291-292
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/example/App.java(1 hunks)src/main/java/org/example/CliAdminApp.java(1 hunks)src/main/java/org/example/CliApp.java(1 hunks)src/main/java/org/example/jpaimpl/MovieRepoJpa.java(4 hunks)src/test/java/org/example/AppTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/example/AppTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/org/example/CliAdminApp.java (1)
src/main/java/org/example/seed/SeedUsers.java (1)
SeedUsers(9-153)
src/main/java/org/example/App.java (3)
src/main/java/org/example/DatabaseFiller.java (1)
DatabaseFiller(15-55)src/main/java/org/example/JpaRunner.java (1)
JpaRunner(8-24)src/main/java/org/example/JpaUtil.java (1)
JpaUtil(9-34)
🔇 Additional comments (5)
src/main/java/org/example/jpaimpl/MovieRepoJpa.java (2)
112-130: LGTM! Proper JPA implementations forgetByDirectorandgetByActor.The stub implementations have been replaced with proper JPQL queries. The
getByDirectorquery correctly matches on the director entity, andgetByActorproperly uses a join on the actors collection.
100-109: Good refactoring offindByTitleto useOptionalwith stream.The implementation correctly handles null/blank input and uses
getResultStream().findFirst()which is cleaner than catchingNoResultException.src/main/java/org/example/App.java (2)
19-26: Good use of transactional seeding with database emptiness check.The seeding logic correctly uses
JpaRunner.runInTransactionfor atomic seeding and guards withisDatabaseEmptyto avoid duplicate seeding.
29-77: LGTM! Clean resource management with try-with-resources.The EntityManager lifecycle is properly managed, and the login flow with retry logic is well-structured.
src/main/java/org/example/CliAdminApp.java (1)
222-234: Logic error: inconsistentOptionalusage.Line 230 checks
users.isEmpty()treatingusersas a collection, but line 233 callsusers.ifPresent()treating it as anOptional. Based on thefindByUserNamemethod likely returningOptional<User>, the correct check should beusers.isEmpty()→users.isPresent()inverted, or simply use theOptionalpattern consistently.🔎 Proposed fix
var users = userRepo.findByUserName(userName); em.getTransaction().commit(); - if (users.isEmpty()) { + if (users.isEmpty()) { System.out.println("No users found with username '" + userName + "'"); } else { - users.ifPresent(u -> System.out.println("Found user: " + u.getUserName())); + System.out.println("Found user: " + users.get().getUserName()); }Likely an incorrect or invalid review comment.
| case 2 -> { | ||
| System.out.println("***** You have selected to list movies by language *****"); | ||
| System.out.println("Enter Language: "); | ||
| String langInput = sc.nextLine().trim().toUpperCase(); | ||
| Language lang = Language.valueOf(langInput); |
There was a problem hiding this comment.
Add error handling for enum parsing.
Language.valueOf(langInput) will throw IllegalArgumentException for invalid input, crashing the application.
🔎 Proposed fix
System.out.println("Enter Language: ");
String langInput = sc.nextLine().trim().toUpperCase();
- Language lang = Language.valueOf(langInput);
- movieRepoJpa.getMovieByLanguage(lang)
- .forEach(m-> System.out.println("- " + m.getTitle()));
+ try {
+ Language lang = Language.valueOf(langInput);
+ movieRepoJpa.getMovieByLanguage(lang)
+ .forEach(m-> System.out.println("- " + m.getTitle()));
+ } catch (IllegalArgumentException e) {
+ System.out.println("Invalid language: " + langInput);
+ }📝 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.
| case 2 -> { | |
| System.out.println("***** You have selected to list movies by language *****"); | |
| System.out.println("Enter Language: "); | |
| String langInput = sc.nextLine().trim().toUpperCase(); | |
| Language lang = Language.valueOf(langInput); | |
| case 2 -> { | |
| System.out.println("***** You have selected to list movies by language *****"); | |
| System.out.println("Enter Language: "); | |
| String langInput = sc.nextLine().trim().toUpperCase(); | |
| try { | |
| Language lang = Language.valueOf(langInput); | |
| movieRepoJpa.getMovieByLanguage(lang) | |
| .forEach(m-> System.out.println("- " + m.getTitle())); | |
| } catch (IllegalArgumentException e) { | |
| System.out.println("Invalid language: " + langInput); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/example/CliApp.java around lines 178 to 182, the call
Language.valueOf(langInput) can throw IllegalArgumentException for invalid
input; wrap the parsing in a try-catch (or validate against Language.values()
first) to handle invalid inputs gracefully: catch IllegalArgumentException to
print a user-friendly error message and re-prompt or return to the menu, or use
a utility method that maps input to an Optional<Language> and handles absent
values without throwing.
| System.out.println("Enter Genre: "); | ||
| String genreName = sc.nextLine(); | ||
|
|
||
| GenreRepoJpa directorRepo = new GenreRepoJpa(em); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Misleading variable name: directorRepo is actually a GenreRepoJpa.
The variable is named directorRepo but holds a GenreRepoJpa instance.
🔎 Proposed fix
- GenreRepoJpa directorRepo = new GenreRepoJpa(em);
- Optional<Genre> genreOpt = directorRepo.findByName(genreName);
+ GenreRepoJpa genreRepo = new GenreRepoJpa(em);
+ Optional<Genre> genreOpt = genreRepo.findByName(genreName);📝 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.
| GenreRepoJpa directorRepo = new GenreRepoJpa(em); | |
| GenreRepoJpa genreRepo = new GenreRepoJpa(em); | |
| Optional<Genre> genreOpt = genreRepo.findByName(genreName); |
🤖 Prompt for AI Agents
In src/main/java/org/example/CliApp.java around line 291, the local variable is
named directorRepo while it is instantiated as a GenreRepoJpa, which is
misleading; rename the variable to genreRepo (or another name reflecting
GenreRepoJpa) and update all subsequent usages at/after this line to use the new
name so the variable type and identifier match.
Simplified the EntityManager mess for CliAdminApp. TODO: Simplify em for CliApp.
Co-authored-by: Oscar Nidemar <oscar.nidemar@gmail.com> Co-authored-by: Oskar Lundqvist <oskar.lundqvist3@icloud.com> Co-authored-by: Mark Cavric <cavric_123@hotmail.se> Co-authored-by: Linus Westling <westling.linus@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/org/example/jpaimpl/MovieRepoJpa.java (1)
192-203: Add entity alias to thelanguagefield in the query.Line 199 uses
language = :languagebut should usem.language = :languagefor consistency with other queries and to avoid potential ambiguity.🔎 Proposed fix
return em.createQuery( - "select m from Movie m where language = :language", Movie.class) + "select m from Movie m where m.language = :language", Movie.class) .setParameter("language", language) .getResultList();src/main/java/org/example/CliApp.java (2)
199-207: Add error handling for enum parsing.Line 204 uses
Language.valueOf(langInput)without error handling. Invalid input will throwIllegalArgumentExceptionand crash the application.🔎 Proposed fix
case 2 -> { System.out.println("***** You have selected to list movies by language *****"); System.out.println("Enter Language: "); String langInput = sc.nextLine().trim().toUpperCase(); - Language lang = Language.valueOf(langInput); - movieRepoJpa.getMovieByLanguage(lang) - .forEach(m -> System.out.println("- " + m.getTitle())); + try { + Language lang = Language.valueOf(langInput); + movieRepoJpa.getMovieByLanguage(lang) + .forEach(m -> System.out.println("- " + m.getTitle())); + } catch (IllegalArgumentException e) { + System.out.println("Invalid language: " + langInput); + } }
321-340: Critical:NoSuchElementExceptionwhen genre not found.Lines 328-330 check if
genreOpt.isEmpty()and print a message, but execution continues to line 332 which unconditionally callsgenreOpt.get(). When the genre is not found, this throwsNoSuchElementException.🔎 Proposed fix
case 8 -> { System.out.println("***** You have selected find a movie by genre *****"); System.out.println("Enter Genre: "); String genreName = sc.nextLine(); Optional<Genre> genreOpt = genreRepoJpa.findByName(genreName); if (genreOpt.isEmpty()) { System.out.println("No genre found with name: " + genreName); + break; } Genre genre = genreOpt.get(); List<Movie> moviesByGenre = movieRepoJpa.getMovieByGenre(genre.getName()); if (moviesByGenre.isEmpty()) { System.out.println("No movies found in genre: " + genre.getName()); } else { moviesByGenre.forEach(m -> System.out.println("- " + m.getTitle())); } }
🧹 Nitpick comments (1)
src/main/java/org/example/CliApp.java (1)
439-439: Use double literal for consistency.Line 439 initializes a
doublevariable with afloatliteral (-1.0f). While Java auto-converts this, using a double literal (-1.0or-1.0d) would be more consistent with the variable type and rating operations throughout the codebase.🔎 Proposed fix
- double rating = -1.0f; + double rating = -1.0;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/org/example/App.java(1 hunks)src/main/java/org/example/CliAdminApp.java(1 hunks)src/main/java/org/example/CliApp.java(1 hunks)src/main/java/org/example/JpaUtil.java(1 hunks)src/main/java/org/example/jpaimpl/MovieRepoJpa.java(4 hunks)src/main/java/org/example/jpaimpl/UserRatingRepoJpa.java(4 hunks)src/main/java/org/example/pojo/UserRating.java(2 hunks)src/main/java/org/example/repo/UserRatingRepo.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/example/repo/UserRatingRepo.java
- src/main/java/org/example/JpaUtil.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/org/example/CliAdminApp.java (1)
src/main/java/org/example/JpaRunner.java (1)
JpaRunner(8-24)
src/main/java/org/example/App.java (3)
src/main/java/org/example/DatabaseFiller.java (1)
DatabaseFiller(15-55)src/main/java/org/example/JpaRunner.java (1)
JpaRunner(8-24)src/main/java/org/example/JpaUtil.java (1)
JpaUtil(9-34)
🔇 Additional comments (4)
src/main/java/org/example/pojo/UserRating.java (1)
27-27: LGTM!The parameter type change from
floattodoubleimproves precision for rating values and is consistent with the repository layer changes in UserRatingRepoJpa.Also applies to: 55-55
src/main/java/org/example/jpaimpl/UserRatingRepoJpa.java (1)
21-21: LGTM!The changes improve the API by:
- Using
doubleinstead offloatfor better precision in rating values.- Returning
booleanfromrateMovieto indicate success/failure instead ofvoid.- Handling null inputs gracefully by returning
falseinstead of throwing exceptions.These changes are consistent across the codebase and align with the corresponding entity changes in UserRating.java.
Also applies to: 60-70, 85-121
src/main/java/org/example/App.java (1)
14-80: LGTM!The refactored main method improves the architecture by:
- Runtime seeding with database emptiness check to avoid duplicate seeding.
- Using
JpaRunnerfor clean transactional boundaries.- Proper EntityManager lifecycle management with try-with-resources.
- Clear separation between admin and user CLI flows.
- Login retry loop with user-friendly exit option.
src/main/java/org/example/CliAdminApp.java (1)
15-348: LGTM!The admin menu implementation demonstrates excellent practices:
- Comprehensive input validation with clear error messages for numeric, date, and enum parsing.
- Graceful handling of missing entities using Optional pattern without crashing.
- Consistent transactional boundaries via
JpaRunner.runInTransaction.- User-friendly feedback for all operations.
- Proper handling of empty/invalid inputs throughout.
All previously identified issues have been addressed.
| public List<Movie> getMovieByReleaseDate(String from, String to) { | ||
| if (from == null || to == null || from.isEmpty() || to.isEmpty()) return List.of(); | ||
|
|
||
| LocalDate fromDate; | ||
| LocalDate toDate; | ||
|
|
||
| try { | ||
| fromDate = LocalDate.parse(from, PLAIN_DATE); | ||
| toDate = LocalDate.parse(from, PLAIN_DATE); | ||
| } catch (DateTimeParseException e){ | ||
| System.out.print("Invalid date format. Expected pattern: " + PLAIN_DATE); | ||
| return List.of(); | ||
| } | ||
|
|
||
| if (fromDate.isAfter(toDate)) { | ||
| System.out.println("From-date cannot be after to-date"); | ||
| return List.of(); | ||
| } | ||
|
|
||
| try { | ||
| return em.createQuery( | ||
| "select m from Movie m where m.releaseDate between :from and :to order by m.releaseDate asc", | ||
| Movie.class) | ||
| .setParameter("from", fromDate) | ||
| .setParameter("to", toDate) | ||
| .getResultList(); | ||
| } catch (Exception e) { | ||
| System.out.println("Error executing query: " + e.getMessage()); | ||
| return List.of(); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Critical bug: parsing from parameter twice instead of to.
Line 143 parses the from parameter again instead of parsing the to parameter, causing both fromDate and toDate to have the same value. This breaks all date range queries.
🔎 Proposed fix
try {
fromDate = LocalDate.parse(from, PLAIN_DATE);
- toDate = LocalDate.parse(from, PLAIN_DATE);
+ toDate = LocalDate.parse(to, PLAIN_DATE);
} catch (DateTimeParseException e){📝 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.
| public List<Movie> getMovieByReleaseDate(String from, String to) { | |
| if (from == null || to == null || from.isEmpty() || to.isEmpty()) return List.of(); | |
| LocalDate fromDate; | |
| LocalDate toDate; | |
| try { | |
| fromDate = LocalDate.parse(from, PLAIN_DATE); | |
| toDate = LocalDate.parse(from, PLAIN_DATE); | |
| } catch (DateTimeParseException e){ | |
| System.out.print("Invalid date format. Expected pattern: " + PLAIN_DATE); | |
| return List.of(); | |
| } | |
| if (fromDate.isAfter(toDate)) { | |
| System.out.println("From-date cannot be after to-date"); | |
| return List.of(); | |
| } | |
| try { | |
| return em.createQuery( | |
| "select m from Movie m where m.releaseDate between :from and :to order by m.releaseDate asc", | |
| Movie.class) | |
| .setParameter("from", fromDate) | |
| .setParameter("to", toDate) | |
| .getResultList(); | |
| } catch (Exception e) { | |
| System.out.println("Error executing query: " + e.getMessage()); | |
| return List.of(); | |
| } | |
| } | |
| public List<Movie> getMovieByReleaseDate(String from, String to) { | |
| if (from == null || to == null || from.isEmpty() || to.isEmpty()) return List.of(); | |
| LocalDate fromDate; | |
| LocalDate toDate; | |
| try { | |
| fromDate = LocalDate.parse(from, PLAIN_DATE); | |
| toDate = LocalDate.parse(to, PLAIN_DATE); | |
| } catch (DateTimeParseException e){ | |
| System.out.print("Invalid date format. Expected pattern: " + PLAIN_DATE); | |
| return List.of(); | |
| } | |
| if (fromDate.isAfter(toDate)) { | |
| System.out.println("From-date cannot be after to-date"); | |
| return List.of(); | |
| } | |
| try { | |
| return em.createQuery( | |
| "select m from Movie m where m.releaseDate between :from and :to order by m.releaseDate asc", | |
| Movie.class) | |
| .setParameter("from", fromDate) | |
| .setParameter("to", toDate) | |
| .getResultList(); | |
| } catch (Exception e) { | |
| System.out.println("Error executing query: " + e.getMessage()); | |
| return List.of(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/example/jpaimpl/MovieRepoJpa.java around lines 135 to 166,
the code mistakenly parses the "from" string twice so toDate is set from the
"from" parameter; change the second parse to use the "to" parameter (i.e.,
LocalDate.parse(to, PLAIN_DATE)) so fromDate and toDate reflect the intended
range, keep the existing DateTimeParseException handling and query logic
unchanged, and run unit/integration tests for date-range queries to verify the
fix.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.