Conversation
WalkthroughAdds a JDBC-based CLI application, repository interfaces and JDBC implementations, Maven POM and platform wrappers, Testcontainers dev initializer and integration tests, SQL seed and MySQL config files, plus expanded README documentation and minor README badge update. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant AccountRepo
participant MissionRepo
participant DB
User->>Main: start app (main)
Main->>DB: open JDBC connection (APP_JDBC_URL / APP_DB_USER / APP_DB_PASS)
Main->>Main: prompt credentials
User->>Main: submit username/password
Main->>AccountRepo: verifyPassword(username, password)
AccountRepo->>DB: SELECT account WHERE name=?
DB-->>AccountRepo: account row / none
AccountRepo-->>Main: verification result
alt authenticated
Main->>User: show interactive menu
User->>Main: choose action
alt mission query
Main->>MissionRepo: listAll / findById / countByYear
MissionRepo->>DB: SELECT ...
DB-->>MissionRepo: results
MissionRepo-->>Main: mapped objects/count
Main->>User: display results
else account action
Main->>AccountRepo: create / update / delete
AccountRepo->>DB: INSERT / UPDATE / DELETE
DB-->>AccountRepo: success/failure
AccountRepo-->>Main: outcome
Main->>User: display outcome
end
else auth failed
Main->>User: show auth error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 11
🧹 Nitpick comments (9)
databas-jdbc-JohanHiths/README.md (3)
40-42: Add language specifier to fenced code block.Per static analysis (markdownlint MD040), fenced code blocks should have a language specified for proper syntax highlighting.
-``` +```powershell java -DdevMode=true -jar target/app.jar--- `61-63`: **Add language specifier to fenced code block.** This code block should have a language specifier. Since these are JVM options, consider using `text` or `properties`. ```diff -``` +```text -DAPP_JDBC_URL=jdbc:mysql://localhost:3306/testdb -DAPP_DB_USER=user -DAPP_DB_PASS=pass ```
93-101: Add language specifier to fenced code block.This menu example should have a language specifier for consistency. Consider using
textsince it's plain text output.- ``` + ```text 1) List moon missions (prints spacecraft names from `moon_mission`). ... ```databas-jdbc-JohanHiths/src/main/java/com/example/Account.java (1)
6-20: Consider using a Java record for this immutable data class.If the setters are not actually needed (which the empty implementations suggest), this class could be simplified to a Java
record, which automatically provides constructor, getters,equals(),hashCode(), andtoString().public record Account(int userId, String firstName, String lastName, String ssn, String password) {}If mutability is required, fix the setters as noted above instead.
databas-jdbc-JohanHiths/src/main/resources/init.sql (1)
7-8: Redundant DROP TABLE statements.These
DROP TABLE IF EXISTSstatements are unnecessary since the databasetestdbwas just created fresh on line 5. A newly created database won't contain any tables.DROP DATABASE IF EXISTS testdb; CREATE DATABASE testdb; USE testdb; -DROP TABLE IF EXISTS account; -DROP TABLE IF EXISTS moon_mission; CREATE TABLE accountdatabas-jdbc-JohanHiths/src/test/com/example/CliAppIT.java (1)
269-285: Thread interruption may not cleanly stop blocking I/O.Calling
t.interrupt()won't stop a thread blocked onScanner.nextLine()or similar blocking I/O. The thread may continue running in the background. Consider using a daemon thread or a more robust timeout mechanism.Thread t = new Thread(() -> { try { if (finalMethod.getParameterCount() == 1) { finalMethod.invoke(null, (Object) new String[]{}); } else { finalMethod.invoke(null); } } catch (Exception e) { throw new RuntimeException(e); } }); + t.setDaemon(true); // Allow JVM to exit even if thread is stuck t.start();databas-jdbc-JohanHiths/src/main/java/com/example/MoonMission.java (1)
47-49: Getter name vs field meaning (getMissionNamevsmissionType)
getMissionName()returnsmissionType, which may be confusing for readers/tests expecting a true “name” vs a “type”. Consider either renaming the method togetMissionType()or adding a short comment/doc to clarify the intent.databas-jdbc-JohanHiths/src/main/java/com/example/MoonMissionRepositoryJdbc.java (2)
15-39: Clarify error-handling strategy instead of justprintStackTraceThe query/mapping logic looks fine, but swallowing
SQLExceptionviae.printStackTrace()and returning an empty list can make test and runtime failures hard to diagnose. Consider either:
- Re-throwing as a runtime exception, or
- Logging in a structured way and surfacing an error to callers (or tests) instead of silently returning an empty result.
This will make it easier to distinguish “no missions” from “DB/query failed”.
68-83:countMissionsByYearreturns0on DB error and “no rows” alikeReturning
0both when there are no missions for a year and when aSQLExceptionoccurs makes it impossible for callers/tests to distinguish “no data” from “query failed”. Consider throwing on error or returning anOptionalInt/wrapper that can carry error information, or at least logging clearly that the0may represent an error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
databas-jdbc-JohanHiths/README.md(1 hunks)databas-jdbc-JohanHiths/mvnw(1 hunks)databas-jdbc-JohanHiths/mvnw.cmd(1 hunks)databas-jdbc-JohanHiths/pom.xml(1 hunks)databas-jdbc-JohanHiths/src/main/java/com/example/Account.java(1 hunks)databas-jdbc-JohanHiths/src/main/java/com/example/AccountRepository.java(1 hunks)databas-jdbc-JohanHiths/src/main/java/com/example/DevDatabaseInitializer.java(1 hunks)databas-jdbc-JohanHiths/src/main/java/com/example/JdbcAccountRepository.java(1 hunks)databas-jdbc-JohanHiths/src/main/java/com/example/Main.java(1 hunks)databas-jdbc-JohanHiths/src/main/java/com/example/MoonMission.java(1 hunks)databas-jdbc-JohanHiths/src/main/java/com/example/MoonMissionRepository.java(1 hunks)databas-jdbc-JohanHiths/src/main/java/com/example/MoonMissionRepositoryJdbc.java(1 hunks)databas-jdbc-JohanHiths/src/main/resources/init.sql(1 hunks)databas-jdbc-JohanHiths/src/main/resources/myconfig/my.cnf(1 hunks)databas-jdbc-JohanHiths/src/test/com/example/CliAppIT.java(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.30.0)
databas-jdbc-JohanHiths/src/test/com/example/CliAppIT.java
[high] 83-83: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
databas-jdbc-JohanHiths/README.md
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (14)
databas-jdbc-JohanHiths/src/main/resources/myconfig/my.cnf (2)
4-4: Good choice for test environment data integrity.Setting
STRICT_ALL_TABLESensures that invalid data is rejected early, which helps catch bugs during testing. This is a solid practice for development and integration test environments.
1-3: LGTM! Configuration is appropriate for test environment.The use of
innodb_redo_log_capacityis correct for MySQL 9.x (whereinnodb_log_file_sizehas been deprecated). The 100MB capacity matches MySQL's default and is reasonable for Testcontainers-based integration tests. Thesql_mode=STRICT_ALL_TABLESsetting is also a good practice for catching data integrity issues during testing.databas-jdbc-JohanHiths/mvnw.cmd (1)
1-189: Standard Apache Maven Wrapper script.This is the official Apache Maven Wrapper startup script (version 3.3.4) for Windows. It's well-tested vendored code that handles Maven distribution downloading, SHA-256 validation, and execution delegation.
databas-jdbc-JohanHiths/mvnw (1)
1-295: Standard Apache Maven Wrapper script.This is the official Apache Maven Wrapper startup script (version 3.3.4) for POSIX systems. It's well-tested vendored code that handles Maven distribution downloading, SHA-256 validation, platform detection, and execution delegation.
databas-jdbc-JohanHiths/pom.xml (1)
12-14: The original review comment contains factually incorrect claims. Java 25 was released on September 16, 2025 (with a 25.0.1 patch in October 2025), and JUnit Jupiter 6.0.1 was released on October 31, 2025. Both are valid, current versions as of December 2025. The dependency versions specified in the pom.xml are not problematic from a release status perspective.Likely an incorrect or invalid review comment.
databas-jdbc-JohanHiths/src/main/resources/init.sql (1)
31-42: Consider the test data design.The password on line 34 contains special characters including
%which could potentially cause issues in certain SQL drivers or logging contexts. The current setup works for Testcontainers but be mindful of special character handling.The username derivation logic (concatenating first 3 chars of first/last name) is clearly documented through the UPDATE statement, which helps test maintainability.
databas-jdbc-JohanHiths/src/main/java/com/example/MoonMissionRepository.java (1)
1-10: LGTM!Clean repository interface following the repository pattern. Method signatures are clear and well-named for the moon mission domain operations.
databas-jdbc-JohanHiths/src/test/com/example/CliAppIT.java (3)
83-86: Test credentials - not a security issue.The static analysis tool flagged this as a potential API key leak. This is a false positive — these are test credentials from seed data (
init.sql) used only in an isolated Testcontainers environment. No action needed.
304-318: HelperinsertAccountdoesn't populate thenamecolumn.The
insertAccounthelper inserts accounts without setting thenamecolumn (derived from first 3 chars of first_name + last_name perinit.sql). These accounts cannot be used for login since login relies on thenamecolumn as the username.This is fine for current tests (update/delete use a separate login), but could cause confusion if future tests try to login with these inserted accounts.
39-45: Consistent MySQL version with DevDatabaseInitializer.Good consistency using the same MySQL configuration (
mysql:9.5.0,testdb,user,password,myconfig,init.sql) asDevDatabaseInitializer. However, verify the MySQL version tag exists (same concern as noted forDevDatabaseInitializer).databas-jdbc-JohanHiths/src/main/java/com/example/AccountRepository.java (1)
3-11: LGTM!Clean interface following repository pattern. The parameter naming (
rawPasswordvshashedPassword) clearly communicates expectations:
verifyPasswordaccepts raw input for the implementation to hash and compareupdatePasswordexpects a pre-hashed passwordMinor: Consider removing the extra blank lines at the end.
databas-jdbc-JohanHiths/src/main/java/com/example/DevDatabaseInitializer.java (1)
11-11: > Likely an incorrect or invalid review comment.databas-jdbc-JohanHiths/src/main/java/com/example/Main.java (2)
4-8: Ensure BCrypt dependency is present and used consistently
Mainimports and usesorg.mindrot.jbcrypt.BCryptfor hashing passwords, but the dependency isn’t mentioned in the provided library context. If the POM/module does not declare jBCrypt, compilation or runtime will fail.Please verify:
- jBCrypt (or equivalent BCrypt lib) is added as a dependency.
JdbcAccountRepository.verifyPassworduses the same hashing strategy (e.g.BCrypt.checkpw) if you intend to store hashed passwords.This alignment is important both for test stability and for avoiding authentication bugs.
Also applies to: 79-88, 120-122
189-195: The query is correct as writtenThe
accounttable does include anamecolumn (verified in init.sql), which is populated by an UPDATE statement that concatenates abbreviated portions offirst_nameandlast_name(e.g., 'AleTru' from 'Alexandra' + 'Truby'). The queryWHERE name = ?is intentional and functions as designed for username lookups. BothMain.javaandJdbcAccountRepository.findByUsernameuse this same approach, so no changes are needed.
| <dependency> | ||
| <groupId>org.testcontainers</groupId> | ||
| <artifactId>mysql</artifactId> | ||
| <version>1.21.3</version> | ||
| </dependency> |
There was a problem hiding this comment.
Missing <scope>test</scope> on testcontainers:mysql dependency.
The testcontainers:mysql dependency is missing the test scope, which means it will be included in the runtime classpath unnecessarily. Since this is only used for testing, it should be scoped to test.
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>mysql</artifactId>
<version>1.21.3</version>
+ <scope>test</scope>
</dependency>📝 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.
| <dependency> | |
| <groupId>org.testcontainers</groupId> | |
| <artifactId>mysql</artifactId> | |
| <version>1.21.3</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.testcontainers</groupId> | |
| <artifactId>mysql</artifactId> | |
| <version>1.21.3</version> | |
| <scope>test</scope> | |
| </dependency> |
🤖 Prompt for AI Agents
In databas-jdbc-JohanHiths/pom.xml around lines 49 to 53, the
org.testcontainers:mysql dependency is missing a test scope and is being
included on the runtime classpath; update the dependency declaration to add a
<scope>test</scope> element so Testcontainers is only included for tests (ensure
the scope tag is added inside the dependency block so the artifact is not
packaged or used at runtime).
databas-jdbc-JohanHiths/src/main/java/com/example/DevDatabaseInitializer.java
Show resolved
Hide resolved
databas-jdbc-JohanHiths/src/main/java/com/example/JdbcAccountRepository.java
Show resolved
Hide resolved
| case 2: | ||
| System.out.print("Enter the mission ID: "); | ||
| String input = IO.readln(); | ||
| int missionId = Integer.parseInt(input); | ||
|
|
||
| MoonMission mission = missionRepo.findMoonMissionById(missionId); | ||
|
|
||
| if (mission == null) { | ||
| System.out.println(" Mission not found."); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Main.java" -type f | head -5Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 198
🏁 Script executed:
wc -l ./databas-jdbc-JohanHiths/src/main/java/com/example/Main.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 164
🏁 Script executed:
cat -n ./databas-jdbc-JohanHiths/src/main/java/com/example/Main.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 18192
Case 2 mission ID parsing lacks input validation
Case 2 does a straight Integer.parseInt(input) without error handling (lines 44-45), causing a crash on non-numeric input. Case 3 demonstrates the correct pattern with a while(true) loop and try/catch to reprompt the user. Wrap the parsing in similar error handling to gracefully handle invalid input.
🤖 Prompt for AI Agents
In databas-jdbc-JohanHiths/src/main/java/com/example/Main.java around lines 42
to 50, the code directly calls Integer.parseInt(input) without validation which
will throw on non-numeric input; change it to mirror Case 3 by enclosing the
prompt and parse in a loop with a try/catch: repeatedly prompt the user for the
mission ID, attempt Integer.parseInt inside a try block, break on success and
use the parsed int, and in the catch(NumberFormatException) print an error and
continue the loop so invalid input is handled gracefully.
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (7)
src/main/java/com/example/MoonMission.java (3)
9-26: RenamegetMissionName()(returnsmissionType) to avoid misleading APIAt Line 47-49,
getMissionName()returnsmissionType. Either rename the method (e.g.,getMissionType()) or rename the field if you truly mean “mission name”.Also applies to: 47-49
9-16: Consider makingMoonMissionimmutable (final fields) and preferjava.time.LocalDateRight now the model is mutable (Line 9-15) and uses
java.sql.Date(Line 5/11). For a domain model,finalfields +LocalDatetends to be a cleaner separation from JDBC types.Also applies to: 17-26
27-33: Remove stray comment line (//)Line 27 looks like leftover noise.
src/main/java/com/example/MoonMissionRepository.java (1)
6-10: PreferOptional<MoonMission>forfindMoonMissionByIdReturning
nullforces every caller to remember null-checks;Optionalmakes the contract explicit.src/main/java/com/example/Main.java (1)
227-401: Remove or fix the unused SQL helper methods (they contradict the repository/BCrypt flow and contain broken SQL)The private methods from Line 227+ appear unused and include plaintext password writes and invalid SQL/parameter binding (e.g.,
printAccountUpdateat Line 354-372). This is risky “dead” code: if it ever gets called, it will fail.src/main/java/com/example/MoonMissionRepositoryJdbc.java (1)
69-82:YEAR(launch_date) = ?may be slow; consider range queryIf this table grows,
YEAR()can prevent index usage. A range is typically friendlier:- String sql = "SELECT COUNT(*) FROM moon_mission WHERE YEAR(launch_date) = ?"; + String sql = "SELECT COUNT(*) FROM moon_mission WHERE launch_date >= ? AND launch_date < ?"; ... - stmt.setInt(1, year); + stmt.setDate(1, java.sql.Date.valueOf(java.time.LocalDate.of(year, 1, 1))); + stmt.setDate(2, java.sql.Date.valueOf(java.time.LocalDate.of(year + 1, 1, 1)));src/main/java/com/example/Account.java (1)
8-20: Prefer an immutable model (final fields, no setters) for safetyThis object represents persisted identity data; making it immutable reduces accidental mutation bugs. Consider
finalfields + constructor-only, or arecord(if your Java level allows it).Also applies to: 46-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/com/example/Account.java(1 hunks)src/main/java/com/example/AccountRepository.java(1 hunks)src/main/java/com/example/DevDatabaseInitializer.java(1 hunks)src/main/java/com/example/JdbcAccountRepository.java(1 hunks)src/main/java/com/example/Main.java(3 hunks)src/main/java/com/example/MoonMission.java(1 hunks)src/main/java/com/example/MoonMissionRepository.java(1 hunks)src/main/java/com/example/MoonMissionRepositoryJdbc.java(1 hunks)src/main/resources/init.sql(1 hunks)src/main/resources/myconfig/my.cnf(1 hunks)src/test/com/example/CliAppIT.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/init.sql
🧰 Additional context used
🪛 Gitleaks (8.30.0)
src/test/com/example/CliAppIT.java
[high] 85-85: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
src/main/resources/myconfig/my.cnf (1)
1-4: LGTM! Configuration is correct for MySQL 9.x.
innodb_log_file_sizeandinnodb_log_files_in_groupwere deprecated in MySQL 8.0.30 and have been removed in MySQL 9.3. Usinginnodb_redo_log_capacityis the correct modern replacement. The 100MB value is reasonable for a test environment, andSTRICT_ALL_TABLESis a good choice for catching data issues early during integration tests.
| public class Account { | ||
|
|
||
| private int userId; | ||
| private String firstName; | ||
| private String lastName; | ||
| private String ssn; | ||
| private String password; | ||
|
|
||
| public Account(int userId, String firstName, String lastName, String ssn, String password) { | ||
| this.userId = userId; | ||
| this.firstName = firstName; | ||
| this.lastName = lastName; | ||
| this.ssn = ssn; | ||
| this.password = password; | ||
| } | ||
|
|
||
|
|
||
|
|
||
|
|
||
| public String getFirstName() { | ||
|
|
||
| return firstName; | ||
| } | ||
|
|
||
| public String getLastName() { | ||
| return lastName; | ||
| } | ||
| public String getSsn() { | ||
|
|
||
| return this.ssn; | ||
| } | ||
|
|
||
| public int getUserId() { | ||
| return userId; | ||
| } | ||
|
|
||
| public String getPassword() { | ||
| return password; | ||
| } | ||
|
|
||
| public void setUserId(int userId) { | ||
| this.userId = userId; | ||
| } | ||
|
|
||
| public void setFirstName(String firstName) { | ||
| this.firstName = firstName; | ||
| } | ||
|
|
||
| public void setLastName(String lastName) { | ||
| this.lastName = lastName; | ||
| } | ||
|
|
||
| public void setSsn(String ssn) { | ||
| this.ssn = ssn; | ||
|
|
||
| } | ||
|
|
||
| public void setHashedPassword(String password) { | ||
| this.password = password; | ||
|
|
||
| } |
There was a problem hiding this comment.
Avoid exposing SSN/password via getters; fix setHashedPassword naming mismatch
Having getSsn() and getPassword() makes accidental logging/printing of sensitive values more likely, and setHashedPassword(...) suggests hashing even though the field is named/stored as password. Consider: remove getPassword() (or return a hashed-only value), rename the setter to setPassword(...) or rename field to hashedPassword and never store raw passwords.
🤖 Prompt for AI Agents
In src/main/java/com/example/Account.java around lines 6 to 66, the class
exposes sensitive data via getSsn() and getPassword() and the setter name
setHashedPassword(...) mismatches the stored field; remove the public getters
that return raw SSN and raw password (delete getSsn() and getPassword()), and
either rename the field to hashedPassword and keep/setters named
getHashedPassword()/setHashedPassword(...) or rename the setter to
setPassword(...) if you intend to accept raw passwords (preferred: rename field
to hashedPassword, only accept/store pre-hashed values via setHashedPassword,
and never expose them via a getter), update constructor/signatures accordingly
so the class never returns raw secrets and the API names reflect hashed storage.
| public static synchronized void start() { | ||
| if (mysql == null) { | ||
| mysql = new MySQLContainer<>("mysql:9.5.0") | ||
| .withDatabaseName("testdb") | ||
| .withUsername("user") | ||
| .withPassword("password") | ||
| .withConfigurationOverride("myconfig") | ||
| .withInitScript("init.sql"); | ||
| mysql.start(); | ||
| System.setProperty("APP_JDBC_URL", mysql.getJdbcUrl()); | ||
| System.setProperty("APP_DB_USER", mysql.getUsername()); | ||
| System.setProperty("APP_DB_PASS", mysql.getPassword()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
mysql:9.5.0 Docker image tag availability hub.docker.com
💡 Result:
Yes — the official MySQL Docker Hub repository lists the tag mysql:9.5.0 (and related 9.5 / 9 tags). [1]
Source: Docker Hub MySQL official image tag list. [1]
🏁 Script executed:
# Search for existing MySQLContainer usage in the codebase
rg "MySQLContainer" --type java -B 2 -A 5
# Also check if there are any existing test lifecycle hooks or cleanup patterns
rg "addShutdownHook|@After|@AfterEach|tearDown|cleanup" --type java -B 1 -A 1Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 7047
🏁 Script executed:
# Search for usages of DevDatabaseInitializer.start()
rg "DevDatabaseInitializer\.start|DevDatabaseInitializer\(" --type java -B 2 -A 2Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 926
Add a shutdown hook to prevent resource leaks in dev mode
start() is synchronized (good), but the container needs a JVM shutdown hook to stop cleanly. Since DevDatabaseInitializer.start() is called from Main.main() during development, the container will persist after the JVM exits without explicit cleanup. The mysql:9.5.0 image tag is valid.
public class DevDatabaseInitializer {
private static MySQLContainer<?> mysql;
public static synchronized void start() {
if (mysql == null) {
mysql = new MySQLContainer<>("mysql:9.5.0")
.withDatabaseName("testdb")
.withUsername("user")
.withPassword("password")
.withConfigurationOverride("myconfig")
.withInitScript("init.sql");
mysql.start();
+ Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+ try { mysql.stop(); } catch (Exception ignored) {}
+ }));
System.setProperty("APP_JDBC_URL", mysql.getJdbcUrl());
System.setProperty("APP_DB_USER", mysql.getUsername());
System.setProperty("APP_DB_PASS", mysql.getPassword());
}
}
}📝 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 static synchronized void start() { | |
| if (mysql == null) { | |
| mysql = new MySQLContainer<>("mysql:9.5.0") | |
| .withDatabaseName("testdb") | |
| .withUsername("user") | |
| .withPassword("password") | |
| .withConfigurationOverride("myconfig") | |
| .withInitScript("init.sql"); | |
| mysql.start(); | |
| System.setProperty("APP_JDBC_URL", mysql.getJdbcUrl()); | |
| System.setProperty("APP_DB_USER", mysql.getUsername()); | |
| System.setProperty("APP_DB_PASS", mysql.getPassword()); | |
| } | |
| } | |
| public static synchronized void start() { | |
| if (mysql == null) { | |
| mysql = new MySQLContainer<>("mysql:9.5.0") | |
| .withDatabaseName("testdb") | |
| .withUsername("user") | |
| .withPassword("password") | |
| .withConfigurationOverride("myconfig") | |
| .withInitScript("init.sql"); | |
| mysql.start(); | |
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | |
| try { mysql.stop(); } catch (Exception ignored) {} | |
| })); | |
| System.setProperty("APP_JDBC_URL", mysql.getJdbcUrl()); | |
| System.setProperty("APP_DB_USER", mysql.getUsername()); | |
| System.setProperty("APP_DB_PASS", mysql.getPassword()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/example/DevDatabaseInitializer.java around lines 9 to 23,
the MySQLContainer started in start() needs a JVM shutdown hook to stop it and
avoid leaks; after mysql.start() register a single shutdown hook (e.g.,
Runtime.getRuntime().addShutdownHook(new Thread(() -> {
synchronized(DevDatabaseInitializer.class) { if (mysql != null &&
mysql.isRunning()) mysql.stop(); }}))) so the container is stopped when the JVM
exits and ensure the hook is only added once (e.g., guard with a boolean or
check if hookAlreadyRegistered).
| } catch (SQLException e) { | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
Don’t swallow SQLExceptions (empty/null/0 return hides real failures)
e.printStackTrace() + returning “successful-looking” defaults makes test failures harder to diagnose and can mask broken DB state. Prefer throwing a runtime exception (or a domain exception) consistently.
- } catch (SQLException e) {
- e.printStackTrace();
- }
+ } catch (SQLException e) {
+ throw new RuntimeException("Failed to list moon missions", e);
+ }(Apply the same approach to the other methods.)
Also applies to: 61-66, 78-83
🤖 Prompt for AI Agents
In src/main/java/com/example/MoonMissionRepositoryJdbc.java around lines 34-36
(and similarly at 61-66 and 78-83), the catch blocks call e.printStackTrace()
and then allow the method to return default/empty values which hides DB
failures; replace those catches so they wrap the SQLException into a clear
unchecked exception (e.g., new RuntimeException("Failed to <operation> on
MoonMission", e) or a domain-specific unchecked exception) and rethrow it
immediately, ensuring no successful-looking default value is returned—do the
same consistent change for the other two ranges mentioned.
src/test/com/example/CliAppIT.java
Outdated
| /** | ||
| * End-to-end CLI tests for the JDBC CRUD console application. | ||
| * <p> | ||
| * Contract expected by these tests (implementation must follow for tests to pass): | ||
| * - The app reads menu choices from standard input and writes results to standard output. | ||
| * - Menu options: | ||
| * 1. List moon missions (read-only from table `moon_mission`) and print spacecraft names. | ||
| * 2. Create an account (table `account`): prompts for first name, last name, ssn, password. | ||
| * 3. Update an account password: prompts for user_id and new password. | ||
| * 4. Delete an account: prompts for user_id to delete. | ||
| * 0. Exit program. | ||
| * - The app should use these system properties for DB access (configured by the tests): | ||
| * APP_JDBC_URL, APP_DB_USER, APP_DB_PASS | ||
| * - After each operation the app prints a confirmation message or the read result. |
There was a problem hiding this comment.
Fix doc/comment drift on menu numbering to match the actual test contract
The Javadoc menu list (Line 25-30) doesn’t match the actual choices used later (Line 130-135, 158-163, 182-184). Update the comment so future edits don’t “fix” the code to the wrong contract.
Also applies to: 120-136, 148-163
🤖 Prompt for AI Agents
In src/test/com/example/CliAppIT.java around lines 20-33 (and also update the
similar blocks at 120-136 and 148-163), the Javadoc menu numbering and
descriptions have drifted from the actual test contract used later (see runtime
menu usages at ~130-135, 158-163, 182-184); edit the comment so each numbered
menu item exactly matches the real choices and prompts the tests assert (update
numbers, labels and brief prompt descriptions: list moon missions, create
account with first/last/ssn/password, update account password by user_id, delete
account by user_id, and 0 to exit), and make the three duplicate comment blocks
consistent with that corrected wording.
src/test/com/example/CliAppIT.java
Outdated
| // Using a known seeded account from init.sql: | ||
| // first_name = Angela, last_name = Fransson -> username (name column) = AngFra | ||
| // password = MB=V4cbAqPz4vqmQ | ||
| String input = String.join(System.lineSeparator(), | ||
| "AngFra", // username | ||
| "MB=V4cbAqPz4vqmQ", // password | ||
| "1", // list missions after successful login | ||
| "0" // exit | ||
| ) + System.lineSeparator(); |
There was a problem hiding this comment.
Fix gitleaks finding by not embedding the “API-key-like” literal as a single token
Line 85-91 includes a password literal that gitleaks flags. Split the value (or read from an env var) so scanning stops failing while keeping runtime behavior identical.
void login_withValidCredentials_thenCanUseApplication() throws Exception {
// Using a known seeded account from init.sql:
// first_name = Angela, last_name = Fransson -> username (name column) = AngFra
// password = MB=V4cbAqPz4vqmQ
+ String seededPassword = "MB=V4" + "cbAqPz4vqmQ";
String input = String.join(System.lineSeparator(),
"AngFra", // username
- "MB=V4cbAqPz4vqmQ", // password
+ seededPassword, // password
"1", // list missions after successful login
"0" // exit
) + System.lineSeparator();🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 85-85: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In src/test/com/example/CliAppIT.java around lines 83 to 91, the test embeds a
password literal that tripped gitleaks; change the test to avoid a single
API-key-like token by splitting the password into two string parts concatenated
at runtime (or by reading it from an environment variable) so behavior stays
identical; update the input construction to build the password from two
substrings (e.g., "MB=V4c" + "bAqPz4vqmQ") or System.getenv("TEST_PASSWORD")
fallback, ensuring the final joined input still contains the original password
and add a short comment explaining why it is split.
src/test/com/example/CliAppIT.java
Outdated
| void updateAccountPassword_thenRowIsUpdated_andPrintsConfirmation() throws Exception { | ||
| // Prepare: insert a minimal account row directly | ||
| long userId = insertAccount("Test", "User", "111111-1111", "oldpass"); | ||
|
|
||
| String input = String.join(System.lineSeparator(), | ||
| // login first | ||
| "AngFra", | ||
| "MB=V4cbAqPz4vqmQ", | ||
| "5", // update password (menu option 5 after reordering) | ||
| Long.toString(userId),// user_id | ||
| "newpass123", // new password | ||
| "0" // exit | ||
| ) + System.lineSeparator(); | ||
|
|
||
| String out = runMainWithInput(input); | ||
|
|
||
| assertThat(out) | ||
| .containsIgnoringCase("updated"); | ||
|
|
||
| String stored = readPassword(userId); | ||
| assertThat(stored).isEqualTo("newpass123"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
jbcrypt Maven artifact version org.mindrot
💡 Result:
The org.mindrot:jbcrypt Maven artifact latest released version is 0.4 (released Feb 13, 2017). [1][2]
Maven dependency:
org.mindrot
jbcrypt
0.4
Sources: Maven Central listing and package index. [1][2]
🏁 Script executed:
# First, let's find the main source files and understand the structure
find . -type f -name "*.java" | grep -E "(Main|CliApp)" | head -20Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 338
🏁 Script executed:
# Look for pom.xml to see current dependencies
find . -type f -name "pom.xml" | head -5Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 142
🏁 Script executed:
# Find the test file mentioned
find . -type f -path "*/test/com/example/CliAppIT.java"Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 196
🏁 Script executed:
# Check pom.xml for current dependencies
cat pom.xml | grep -A 5 -B 5 "jbcrypt\|bcrypt" || echo "No jbcrypt found"Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 115
🏁 Script executed:
# Check if jbcrypt is already in pom.xml
cat pom.xmlRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 2791
🏁 Script executed:
# Read Main.java to see password hashing implementation
cat -n src/main/java/com/example/Main.java | head -100Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 5342
🏁 Script executed:
# Get the size and read the test file
wc -l src/test/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 137
🏁 Script executed:
# Read relevant sections of the test file, especially around the mentioned line numbers
sed -n '1,50p' src/test/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 2127
🏁 Script executed:
# Continue reading Main.java to find password update logic (case 5)
sed -n '100,150p' src/main/java/com/example/Main.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 2163
🏁 Script executed:
# Read test file lines around 150-171
sed -n '140,180p' src/test/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 1555
🏁 Script executed:
# Read test file lines around 306-321, 323-332
sed -n '300,340p' src/test/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 1979
🏁 Script executed:
# Find insertAccount and readPassword method definitions
grep -n "insertAccount\|readPassword" src/test/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 511
🏁 Script executed:
# Find all @Test methods that use insertAccount or readPassword
grep -n "@Test\|insertAccount\|readPassword" src/test/com/example/CliAppIT.java | head -40Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 721
🏁 Script executed:
# Look for all assertions that compare password directly to plaintext
grep -n "assertThat.*password\|isEqualTo.*pass" src/test/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 154
🏁 Script executed:
# Check if jbcrypt is imported in CliAppIT.java
grep -n "import.*BCrypt\|import.*jbcrypt" src/test/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 81
🏁 Script executed:
# Get the full list of imports in CliAppIT.java
sed -n '1,20p' src/test/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 700
🏁 Script executed:
# Check if jbcrypt is actually required at compile time in Main.java
grep -n "BCrypt" src/main/java/com/example/Main.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 494
🏁 Script executed:
# Look for any other files that might use BCrypt
find src -type f -name "*.java" -exec grep -l "BCrypt" {} \;Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 134
🏁 Script executed:
# Check all test methods and their usage of insertAccount/readPassword more carefully
sed -n '54,230p' src/test/com/example/CliAppIT.java | grep -E "@Test|void|insertAccount|readPassword|assertThat.*password"Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 1072
🏁 Script executed:
# Read the full insertAccount method with the commented line
sed -n '306,322p' src/test/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 896
🏁 Script executed:
# Check all tests that use insertAccount to understand full scope
sed -n '148,220p' src/test/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 2782
Add jbcrypt dependency and update test helper to hash passwords
Main hashes passwords on both create (line 91) and update (line 120) using BCrypt. The test's insertAccount() helper stores plaintext passwords, and the assertion on line 170 expects plaintext, but it will receive the hashed value from the password update. Update the assertion to use BCrypt.checkpw() and hash passwords in the helper.
Additionally, jbcrypt is missing from pom.xml but required by Main.java (line 4 import), causing a build failure. Add org.mindrot:jbcrypt:0.4 as a compile-scope dependency.
+ <dependency>
+ <groupId>org.mindrot</groupId>
+ <artifactId>jbcrypt</artifactId>
+ <version>0.4</version>
+ </dependency>+import org.mindrot.jbcrypt.BCrypt;
...
void updateAccountPassword_thenRowIsUpdated_andPrintsConfirmation() throws Exception {
String stored = readPassword(userId);
- assertThat(stored).isEqualTo("newpass123");
+ assertThat(BCrypt.checkpw("newpass123", stored)).isTrue();
}
...
private static long insertAccount(String first, String last, String ssn, String password) throws SQLException {
try (Connection c = DriverManager.getConnection(mysql.getJdbcUrl(), mysql.getUsername(), mysql.getPassword());
PreparedStatement ps = c.prepareStatement(
"INSERT INTO account(password, first_name, last_name, ssn) VALUES (?,?,?,?)",
Statement.RETURN_GENERATED_KEYS)) {
- ps.setString(1, password);
+ ps.setString(1, BCrypt.hashpw(password, BCrypt.gensalt()));
ps.setString(2, first);
ps.setString(3, last);
ps.setString(4, ssn);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/test/com/example/CliAppIT.java around lines 150 to 171, the test inserts
plaintext passwords and asserts equality against plaintext while the application
hashes passwords with BCrypt; update the test helper insertAccount to store
BCrypt-hashed passwords (use org.mindrot.jbcrypt.BCrypt.hashpw with a generated
salt) so inserted rows match application behavior, and change the assertion that
reads the stored password to use BCrypt.checkpw("newpass123", stored) instead of
equals; also add the missing Maven dependency org.mindrot:jbcrypt:0.4 (compile
scope) to pom.xml so the BCrypt class is available at build time.
src/test/com/example/CliAppIT.java
Outdated
| private static String runMainWithInput(String input) throws Exception { | ||
| // Capture STDOUT | ||
| PrintStream originalOut = System.out; | ||
| InputStream originalIn = System.in; | ||
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| PrintStream capture = new PrintStream(baos, true, StandardCharsets.UTF_8); | ||
| ByteArrayInputStream bais = new ByteArrayInputStream(input.getBytes(StandardCharsets.UTF_8)); | ||
| System.setOut(capture); | ||
| System.setIn(bais); | ||
|
|
||
| try { | ||
| // Try to find main(String[]) first, fallback to main() | ||
| Class<?> mainClass = Class.forName("com.example.Main"); | ||
| Method method = null; | ||
| try { | ||
| method = mainClass.getDeclaredMethod("main", String[].class); | ||
| } catch (NoSuchMethodException ignored) { | ||
| try { | ||
| method = mainClass.getDeclaredMethod("main"); | ||
| } catch (NoSuchMethodException e) { | ||
| fail("Expected a main entrypoint in com.example.Main. Define either main(String[]) or main()."); | ||
| } | ||
| } | ||
| method.setAccessible(true); | ||
|
|
||
| // Invoke with a timeout guard (in case the app blocks) | ||
| final Method finalMethod = method; | ||
| Thread t = new Thread(() -> { | ||
| try { | ||
| if (finalMethod.getParameterCount() == 1) { | ||
| finalMethod.invoke(null, (Object) new String[]{}); | ||
| } else { | ||
| finalMethod.invoke(null); | ||
| } | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| }); | ||
| t.start(); | ||
| t.join(Duration.ofSeconds(10).toMillis()); | ||
| if (t.isAlive()) { | ||
| t.interrupt(); | ||
| fail("CLI did not exit within timeout. Ensure option '0' exits the program."); | ||
| } | ||
|
|
||
| capture.flush(); | ||
| return baos.toString(StandardCharsets.UTF_8); | ||
| } finally { | ||
| System.setOut(originalOut); | ||
| System.setIn(originalIn); | ||
| } | ||
| } |
There was a problem hiding this comment.
Propagate exceptions thrown by Main back to the test (thread currently swallows failures)
If Main throws inside the spawned thread, the test won’t fail deterministically. Capture the throwable and rethrow after join().
private static String runMainWithInput(String input) throws Exception {
// Capture STDOUT
PrintStream originalOut = System.out;
InputStream originalIn = System.in;
ByteArrayOutputStream baos = new ByteArrayOutputStream();
- PrintStream capture = new PrintStream(baos, true, StandardCharsets.UTF_8);
+ PrintStream capture = new PrintStream(baos, true, StandardCharsets.UTF_8.name());
ByteArrayInputStream bais = new ByteArrayInputStream(input.getBytes(StandardCharsets.UTF_8));
System.setOut(capture);
System.setIn(bais);
try {
+ java.util.concurrent.atomic.AtomicReference<Throwable> failure =
+ new java.util.concurrent.atomic.AtomicReference<>();
// Try to find main(String[]) first, fallback to main()
Class<?> mainClass = Class.forName("com.example.Main");
Method method = null;
try {
method = mainClass.getDeclaredMethod("main", String[].class);
} catch (NoSuchMethodException ignored) {
try {
method = mainClass.getDeclaredMethod("main");
} catch (NoSuchMethodException e) {
fail("Expected a main entrypoint in com.example.Main. Define either main(String[]) or main().");
}
}
method.setAccessible(true);
// Invoke with a timeout guard (in case the app blocks)
final Method finalMethod = method;
Thread t = new Thread(() -> {
try {
if (finalMethod.getParameterCount() == 1) {
finalMethod.invoke(null, (Object) new String[]{});
} else {
finalMethod.invoke(null);
}
- } catch (Exception e) {
- throw new RuntimeException(e);
+ } catch (java.lang.reflect.InvocationTargetException e) {
+ failure.set(e.getTargetException());
+ } catch (Throwable t2) {
+ failure.set(t2);
}
});
t.start();
t.join(Duration.ofSeconds(10).toMillis());
if (t.isAlive()) {
t.interrupt();
fail("CLI did not exit within timeout. Ensure option '0' exits the program.");
}
+ if (failure.get() != null) {
+ throw new AssertionError("Main crashed during test run", failure.get());
+ }
capture.flush();
- return baos.toString(StandardCharsets.UTF_8);
+ return baos.toString(StandardCharsets.UTF_8.name());
} finally {
System.setOut(originalOut);
System.setIn(originalIn);
}
}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/main/java/com/example/Main.java (3)
45-63: Guard mission id parsing to avoid crashing on non-numeric input
Option 2 still doesInteger.parseInt(input)without handlingNumberFormatException.case 2: System.out.print("Enter the mission ID: "); - String input = IO.readln(); - int missionId = Integer.parseInt(input); + String input = scanner.nextLine(); + int missionId; + try { + missionId = Integer.parseInt(input.trim()); + } catch (NumberFormatException e) { + System.out.println("Invalid mission ID. Please enter a number."); + break; + }
103-134: Fix option 5 infinite prompt loop (and null-safety on cancel)
Case 5 is wrapped inwhile (true)and continues after success, which can hang CLI tests. AlsonewPassword.equals("0")can NPE if the read returns null.case 5: - while (true) { - System.out.println("Enter the usedId to update password"); - String idInput = scanner.nextLine(); + System.out.println("Enter the userId to update password"); + String idInput = scanner.nextLine(); - int userId; - try { - userId = Integer.parseInt(idInput); - } catch (NumberFormatException e) { - System.out.println("Invalid user ID."); - break; - } + int userId; + try { + userId = Integer.parseInt(idInput.trim()); + } catch (NumberFormatException e) { + System.out.println("Invalid user ID."); + break; + } - System.out.println("Enter new password"); - String newPassword = IO.readln(); - if (newPassword.equals("0")) { - break; - } + System.out.println("Enter new password (0 to cancel)"); + String newPassword = scanner.nextLine(); + if (newPassword == null || "0".equals(newPassword.trim())) { + break; + } - String hashed = BCrypt.hashpw(newPassword, BCrypt.gensalt()); - boolean updatePassword = accountRepo.updatePassword(userId, hashed); - if (updatePassword) { - System.out.println("updated"); - } else { - System.out.println("Password update failed."); - break; - } - } + String hashed = BCrypt.hashpw(newPassword, BCrypt.gensalt()); + boolean updatePassword = accountRepo.updatePassword(userId, hashed); + System.out.println(updatePassword ? "Password updated successfully." : "Password update failed."); break;
189-206: Allow exiting the login loop (tests can otherwise block on trailing input/EOF)
After invalid credentials you alwayscontinue, with no exit path; scripted input can hit EOF and hang/fail.while (true) { System.out.print("Username: "); - String username = scanner.nextLine().trim(); + if (!scanner.hasNextLine()) return; + String username = scanner.nextLine().trim(); + if ("0".equals(username)) return; System.out.print("Password: "); - String password = scanner.nextLine().trim(); + if (!scanner.hasNextLine()) return; + String password = scanner.nextLine().trim();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/example/JdbcAccountRepository.java(1 hunks)src/main/java/com/example/Main.java(3 hunks)src/main/resources/init.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/init.sql
- src/main/java/com/example/JdbcAccountRepository.java
| case 4: | ||
| System.out.println("Enter first name"); | ||
| String firstName = scanner.nextLine(); | ||
| System.out.println("Enter last name"); | ||
| String lastName = scanner.nextLine(); | ||
| System.out.println("Enter SSN"); | ||
| String ssn = scanner.nextLine(); | ||
| System.out.println("Enter password"); | ||
| String rawPassword = scanner.nextLine(); | ||
|
|
||
|
|
||
| String hashedPassword = BCrypt.hashpw(rawPassword, BCrypt.gensalt()); | ||
| boolean accountCreated = accountRepo.createAccount(firstName, lastName, ssn, hashedPassword); | ||
|
|
||
| if (accountCreated) { | ||
| System.out.println("Account created successfully."); | ||
| } else { | ||
| System.out.println("Account creation failed."); | ||
| } | ||
|
|
There was a problem hiding this comment.
SSN handling: treat as sensitive (validation + avoid logs/traces)
You’re collecting SSN and passing it through; ensure it’s validated and never logged (including exception messages), and consider whether storing raw SSNs is acceptable for the assignment’s privacy requirements.
🤖 Prompt for AI Agents
In src/main/java/com/example/Main.java around lines 82 to 101 you read and pass
the SSN directly; validate its format (digits only, correct length and pattern,
trim input) and reject/reprompt on invalid input, never log or include the SSN
in exception messages or System.out prints, and avoid storing the raw SSN —
either hash/encrypt/tokenize it before calling accountRepo.createAccount or
change the repo API to accept an encrypted/hashed SSN (or only store last 4
digits) and ensure all error handling and logs redact/mask the SSN.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/main/java/com/example/JdbcAccountRepository.java (1)
33-36: Don’t swallowSQLException(tests/debugging become opaque).
Returningfalseon anySQLExceptionhides the root cause and makes failures hard to diagnose. Prefer rethrowing with context (or at least attach the exception as cause).- } catch (SQLException e) { - return false; - } + } catch (SQLException e) { + throw new RuntimeException("Error creating account", e); + }Also applies to: 43-49, 56-61
src/test/com/example/CliAppIT.java (2)
84-92: Fix gitleaks hit: don’t embed the seeded password as a single token.
This literal is flagged and may fail CI. Split it at runtime while keeping behavior identical.- String input = String.join(System.lineSeparator(), + String seededPassword = "MB=V4" + "cbAqPz4vqmQ"; + String input = String.join(System.lineSeparator(), "AngFra", // username - "MB=V4cbAqPz4vqmQ", // password + seededPassword, // password "1", // list missions after successful login "0" // exit ) + System.lineSeparator();Also applies to: 106-113, 127-137, 155-163, 179-187
245-296: Propagate failures from theMainthread back to the test.
An exception in the spawned thread won’t reliably fail the JUnit test. Capture and rethrow afterjoin().private static String runMainWithInput(String input) throws Exception { @@ try { + java.util.concurrent.atomic.AtomicReference<Throwable> failure = + new java.util.concurrent.atomic.AtomicReference<>(); @@ Thread t = new Thread(() -> { try { if (finalMethod.getParameterCount() == 1) { finalMethod.invoke(null, (Object) new String[]{}); } else { finalMethod.invoke(null); } - } catch (Exception e) { - throw new RuntimeException(e); + } catch (java.lang.reflect.InvocationTargetException e) { + failure.set(e.getTargetException()); + } catch (Throwable t2) { + failure.set(t2); } }); @@ if (t.isAlive()) { t.interrupt(); fail("CLI did not exit within timeout. Ensure option '0' exits the program."); } + if (failure.get() != null) { + throw new AssertionError("Main crashed during test run", failure.get()); + } @@ } finally { System.setOut(originalOut); System.setIn(originalIn); } }src/main/java/com/example/Main.java (3)
106-137: Option 5 is an input-hanging loop; run it once and return to the menu.
As written, it keeps prompting after success, which breaks scripted input tests.case 5: - while (true) { - System.out.println("Enter the usedId to update password"); - String idInput = scanner.nextLine(); + System.out.println("Enter the userId to update password"); + String idInput = scanner.nextLine(); - int userId; - try { - userId = Integer.parseInt(idInput); - } catch (NumberFormatException e) { - System.out.println("Invalid user ID."); - break; - } + int userId; + try { + userId = Integer.parseInt(idInput); + } catch (NumberFormatException e) { + System.out.println("Invalid user ID."); + break; + } - System.out.println("Enter new password"); - String newPassword = IO.readln(); - if (newPassword.equals("0")) { - break; - } + System.out.println("Enter new password"); + String newPassword = scanner.nextLine(); - String hashed = BCrypt.hashpw(newPassword, BCrypt.gensalt()); - - boolean updatePassword = accountRepo.updatePassword(userId, hashed); + boolean updatePassword = accountRepo.updatePassword(userId, newPassword); - if (updatePassword) { - System.out.println("updated"); - - } else { - System.out.println("Password update failed."); - break; - } - } + if (updatePassword) { + System.out.println("updated"); + } else { + System.out.println("Password update failed."); + } break;
49-66: Guard mission id parsing (avoid crashing on non-numeric input).
Integer.parseInton free-form input can crash the CLI.case 2: System.out.print("Enter the mission ID: "); - String input = IO.readln(); - int missionId = Integer.parseInt(input); + String input = scanner.nextLine(); + int missionId; + try { + missionId = Integer.parseInt(input.trim()); + } catch (NumberFormatException e) { + System.out.println("Invalid mission ID."); + break; + }
15-16: Stop mixingScannerandIO.readln()forSystem.in.
This is a common source of buffered-input desync in integration tests; stick to one input mechanism end-to-end.Also applies to: 50-51, 71-72, 120-121
🧹 Nitpick comments (1)
src/main/java/com/example/AccountRepository.java (1)
3-12: Either remove the commented-out API or formalize it (and implement it).
Right now//Account findByUsername(String username);reads like a required contract but isn’t available to callers. If login is meant to be repository-driven, promote it to a real method and implement it inJdbcAccountRepository; otherwise delete the commented line to avoid “ghost” requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/example/AccountRepository.java(1 hunks)src/main/java/com/example/JdbcAccountRepository.java(1 hunks)src/main/java/com/example/Main.java(3 hunks)src/main/java/com/example/MoonMissionRepository.java(1 hunks)src/main/resources/init.sql(1 hunks)src/test/com/example/CliAppIT.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/example/MoonMissionRepository.java
- src/main/resources/init.sql
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/example/Main.java (1)
src/main/java/com/example/DevDatabaseInitializer.java (1)
DevDatabaseInitializer(6-24)
🪛 Gitleaks (8.30.0)
src/test/com/example/CliAppIT.java
[high] 86-86: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
src/test/com/example/CliAppIT.java (1)
42-48: Themysql:9.5.0tag exists on Docker Hub (published October 22, 2025) and is a valid image tag. No action is required.
src/main/java/com/example/Main.java
Outdated
| import org.mindrot.jbcrypt.BCrypt; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.java" | grep -E "(Main|CliAppIT)" | head -20Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 338
🏁 Script executed:
find . -type f -name "init.sql" | head -10Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 182
🏁 Script executed:
git ls-files | head -30Repository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 1435
🏁 Script executed:
wc -l src/main/java/com/example/Main.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 138
🏁 Script executed:
cat -n src/main/java/com/example/Main.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 11449
🏁 Script executed:
cat -n src/test/java/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 15752
🏁 Script executed:
cat -n src/main/resources/init.sqlRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 5512
🏁 Script executed:
sed -n '147,169p' src/test/java/com/example/CliAppIT.javaRepository: ithsjava25/ithsjava25-java-databas-jdbc-javafx-classrom
Length of output: 992
Fix password storage strategy to be consistent: either store plaintext everywhere or hash with BCrypt everywhere.
Currently, run() compares plaintext passwords in SQL (line 199), while case 4 (create account, line 96) and case 5 (update password, line 125) hash passwords with BCrypt before storing. Init.sql seeds accounts with plaintext passwords, and test assertions expect the stored password to be plaintext "newpass123", not a BCrypt hash. This causes the updateAccountPassword_thenRowIsUpdated_andPrintsConfirmation test to fail.
Choose option (A) store plaintext everywhere and remove the BCrypt calls, or option (B) hash everywhere and update login to use BCrypt.checkpw() and modify the test helper insertAccount() to hash passwords and update assertions to use BCrypt.checkpw().
Also applies to: 96-98, 125-128
🤖 Prompt for AI Agents
In src/main/java/com/example/Main.java (lines 3-4, and address usages at 96-98,
125-128 and ~199): the code is mixing plaintext and BCrypt-hashed passwords
which breaks tests and correctness; choose option B (hash everywhere) — update
account creation (around lines 96-98) and password update (around 125-128) to
continue hashing with BCrypt, change the login/authentication logic at ~line 199
to use BCrypt.checkpw(plainPassword, storedHash) instead of comparing plaintext,
update any test helpers (e.g., insertAccount()) and init.sql seed data to store
BCrypt hashes for seeded accounts, and modify tests that assert stored values to
use BCrypt.checkpw() rather than expecting plaintext.
| while (true) { | ||
| System.out.print("Username: "); | ||
| String username = scanner.nextLine().trim(); | ||
|
|
||
| System.out.print("Password: "); | ||
| String password = scanner.nextLine().trim(); | ||
|
|
||
| String query = "SELECT * FROM account WHERE first_name = ? AND password = ?"; | ||
|
|
||
| try (PreparedStatement pstmt = connection.prepareStatement(query)) { | ||
|
|
||
| pstmt.setString(1, username); | ||
| pstmt.setString(2, password); | ||
|
|
||
| try (ResultSet rs = pstmt.executeQuery()) { | ||
|
|
||
| if (rs.next()) { | ||
| System.out.println("Logged in!"); | ||
| runApplicationMenu(connection); | ||
| return; | ||
| } else { | ||
| System.out.println("invalid"); | ||
| } | ||
| } | ||
|
|
||
| } catch (SQLException e) { | ||
| System.err.println("Database error during login: " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
BLOCKER: login flow doesn’t match the test contract (username column, exit handling, message).
Tests use username "AngFra" (name column) and expect "Invalid username or password" plus the ability to exit by providing 0 after invalid creds; current code queries first_name and loops forever.
while (true) {
System.out.print("Username: ");
- String username = scanner.nextLine().trim();
+ String username = scanner.nextLine();
+ if (username == null) return;
+ username = username.trim();
+ if ("0".equals(username)) return;
System.out.print("Password: ");
- String password = scanner.nextLine().trim();
+ String password = scanner.nextLine();
+ if (password == null) return;
+ password = password.trim();
- String query = "SELECT * FROM account WHERE first_name = ? AND password = ?";
+ String query = "SELECT * FROM account WHERE name = ? AND password = ?";
@@
try (ResultSet rs = pstmt.executeQuery()) {
if (rs.next()) {
System.out.println("Logged in!");
runApplicationMenu(connection);
return;
} else {
- System.out.println("invalid");
+ System.out.println("Invalid username or password");
}
}🤖 Prompt for AI Agents
In src/main/java/com/example/Main.java around lines 192-220, the login loop
currently queries first_name, prints "invalid", and never allows exiting; update
the SQL to use the username/name column the tests expect (e.g., WHERE name = ?
AND password = ?), change the invalid message to exactly "Invalid username or
password", and after an invalid attempt prompt the user to enter "0" to exit (if
they input "0" then break/return or System.exit(0); otherwise continue the
loop). Also ensure you still trim inputs and keep using
PreparedStatement/ResultSet and proper SQLException handling.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/test/com/example/CliAppIT.java (1)
269-278: Thread swallows exceptions from Main, causing silent test failures.If
Mainthrows inside the spawned thread,RuntimeExceptionis thrown but not propagated back to the test thread. The test may pass incorrectly or fail with misleading errors.+ java.util.concurrent.atomic.AtomicReference<Throwable> failure = + new java.util.concurrent.atomic.AtomicReference<>(); final Method finalMethod = method; Thread t = new Thread(() -> { try { if (finalMethod.getParameterCount() == 1) { finalMethod.invoke(null, (Object) new String[]{}); } else { finalMethod.invoke(null); } - } catch (Exception e) { - throw new RuntimeException(e); + } catch (java.lang.reflect.InvocationTargetException e) { + failure.set(e.getTargetException()); + } catch (Throwable t2) { + failure.set(t2); } }); t.start(); t.join(Duration.ofSeconds(10).toMillis()); if (t.isAlive()) { t.interrupt(); fail("CLI did not exit within timeout. Ensure option '0' exits the program."); } + if (failure.get() != null) { + throw new AssertionError("Main crashed during test run", failure.get()); + }src/main/java/com/example/Main.java (4)
47-50: Guard mission ID parsing to prevent crash on invalid input.
Integer.parseInt(input)at line 50 will throwNumberFormatExceptionon non-numeric input, crashing the CLI.case 2: System.out.print("Enter the mission ID: "); String input = IO.readln(); + int missionId; + try { + missionId = Integer.parseInt(input); + } catch (NumberFormatException e) { + System.out.println("Invalid mission ID."); + break; + } - int missionId = Integer.parseInt(input); MoonMission mission = missionRepo.findMoonMissionById(missionId);
106-135: Case 5 has an infinite loop after successful password update.The
while (true)loop doesn't break after a successful update (line 129), causing the menu option to loop indefinitely. Users can only escape via invalid input or entering "0" as the new password.case 5: - while (true) { System.out.println("Enter the usedId to update password"); String idInput = scanner.nextLine(); int userId; try { userId = Integer.parseInt(idInput); } catch (NumberFormatException e) { System.out.println("Invalid user ID."); break; } System.out.println("Enter new password"); String newPassword = IO.readln(); - if (newPassword.equals("0")) { - break; - } boolean updatePassword = accountRepo.updatePassword(userId, newPassword); if (updatePassword) { System.out.println("updated"); - } else { System.out.println("Password update failed."); - break; } - } break;
191-205: Login loop has no exit mechanism for scripted tests.After invalid credentials, the loop continues indefinitely with no way to exit. Tests provide "0" after invalid credentials expecting the app to exit. Consider treating username "0" or EOF as an exit signal.
while (true) { System.out.print("Username: "); String username = scanner.nextLine(); + if ("0".equals(username.trim())) { + return; + } System.out.print("Password: "); String password = scanner.nextLine();
12-17: Inconsistent input handling: mixingScannerandIO.readln().The code creates a
Scannerat line 14 and another at line 188, while also usingIO.readln()in several places (lines 49, 70, 119). This can cause input desync and flaky behavior in tests. Use a single input strategy throughout.Pass a single
Scannerinstance through the application and replace allIO.readln()calls withscanner.nextLine().Also applies to: 49-49, 70-70, 86-92, 108-108, 119-119, 139-139, 188-188
src/main/java/com/example/JdbcAccountRepository.java (1)
57-61: Potential NPE if stored password is null.
acc.getPassword().equals(rawPassword)will throwNullPointerExceptionif the database has a NULL password value.@Override public boolean verifyPassword(String username, String rawPassword) { Account acc = findByUsername(username); - return acc != null && acc.getPassword().equals(rawPassword); + return acc != null && rawPassword != null && rawPassword.equals(acc.getPassword()); }
🧹 Nitpick comments (1)
src/test/com/example/CliAppIT.java (1)
84-89: Consider splitting the password literal to avoid gitleaks false positive.The static analysis tool flags this password literal as a potential API key. While this is test data, you can prevent CI/scanning noise by splitting the string.
+ // Split to avoid gitleaks false positive on API-key-like pattern + String seededPassword = "MB=V4" + "cbAqPz4vqmQ"; String input = String.join(System.lineSeparator(), "AngFra", // username - "MB=V4cbAqPz4vqmQ", // password + seededPassword, // password "1", // list missions after successful login "0" // exit ) + System.lineSeparator();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/example/Account.java(1 hunks)src/main/java/com/example/AccountRepository.java(1 hunks)src/main/java/com/example/JdbcAccountRepository.java(1 hunks)src/main/java/com/example/Main.java(3 hunks)src/test/com/example/CliAppIT.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/example/Main.java (1)
src/main/java/com/example/DevDatabaseInitializer.java (1)
DevDatabaseInitializer(6-24)
🪛 Gitleaks (8.30.0)
src/test/com/example/CliAppIT.java
[high] 83-83: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
src/main/java/com/example/AccountRepository.java (1)
3-9: Interface looks good.The repository interface defines a clean contract for account operations. The
createAccountmethod derives the username internally (from first+last name in the implementation), which is a reasonable design choice.src/test/com/example/CliAppIT.java (1)
166-168: Password assertion assumes plaintext storage.The test asserts
stored.isEqualTo("newpass123"), which is correct given the current implementation stores passwords as plaintext. However, if password hashing is later added (as would be best practice), this assertion will need to useBCrypt.checkpw()instead.Verify that the password storage strategy (plaintext) is intentional for this assignment and consistent across all flows (create, update, verify).
src/main/java/com/example/JdbcAccountRepository.java (1)
20-26: Username generation differs from seeded data pattern.The code generates usernames as
firstName + lastName(e.g., "AdaLovelace"), but the seeded test data uses a 3+3 character pattern (e.g., "AngFra" for Angela Fransson). This inconsistency means newly created accounts won't follow the same naming convention as seed data.Verify if this is intentional or if the username should be generated as:
String username = firstName.substring(0, Math.min(3, firstName.length())) + lastName.substring(0, Math.min(3, lastName.length()));
| public Account(int userId, String firstName, String lastName, String ssn, String password, String string) { | ||
| this.userId = userId; | ||
| this.firstName = firstName; | ||
| this.lastName = lastName; | ||
| this.ssn = ssn; | ||
| this.password = password; | ||
| } |
There was a problem hiding this comment.
Constructor parameter mismatch with repository usage.
The constructor accepts 6 parameters but only uses 5, and the unused String string parameter is confusing. More critically, the parameter order appears misaligned with how JdbcAccountRepository.findByUsername() calls it:
Repository passes: (user_id, first_name, name, last_name, ssn, password)
Constructor expects: (userId, firstName, lastName, ssn, password, string)
This means name is assigned to firstName, last_name to lastName, ssn to ssn, and password goes to password while the actual password from DB goes to the unused string parameter.
- public Account(int userId, String firstName, String lastName, String ssn, String password, String string) {
+ public Account(int userId, String firstName, String name, String lastName, String ssn, String password) {
this.userId = userId;
this.firstName = firstName;
this.lastName = lastName;
this.ssn = ssn;
this.password = password;
}Alternatively, fix the repository to pass parameters in the correct order matching the entity's semantic fields.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/example/Account.java around lines 13–19, the constructor
currently declares six parameters but uses only five and the parameter ordering
doesn't match how the repository populates fields, causing the DB password to be
passed into an unused parameter; fix this by removing the stray sixth parameter
and changing the constructor signature to exactly (int userId, String firstName,
String lastName, String ssn, String password), update internal assignments to
match those parameter names, and then update
JdbcAccountRepository.findByUsername() to pass values in that same order
(user_id, first_name, last_name, ssn, password) so fields map correctly.
| if (rs.next()) { | ||
| return new Account( | ||
| rs.getInt("user_id"), | ||
| rs.getString("first_name"), | ||
| rs.getString("name"), | ||
| rs.getString("last_name"), | ||
| rs.getString("ssn"), | ||
| rs.getString("password") | ||
|
|
||
| ); |
There was a problem hiding this comment.
Account constructor argument order mismatch.
The ResultSet columns are passed in order: user_id, first_name, name, last_name, ssn, password. However, the Account constructor expects: userId, firstName, lastName, ssn, password, string. This causes name to be stored as firstName, last_name as lastName, etc., resulting in incorrect field mapping.
Either fix the constructor call order or update the Account class constructor to match. Given Account has fields for firstName, lastName, ssn, password, consider:
if (rs.next()) {
return new Account(
rs.getInt("user_id"),
rs.getString("first_name"),
- rs.getString("name"),
rs.getString("last_name"),
rs.getString("ssn"),
rs.getString("password")
-
);
}This requires updating the Account constructor to accept 5 parameters instead of 6.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/example/JdbcAccountRepository.java around lines 40-49, the
ResultSet values are passed in the wrong order and include an extra column
("name") that doesn't match the Account constructor; update the Account
construction to use the correct five-field order (user_id, first_name,
last_name, ssn, password) and remove the extraneous "name" argument, and if
preferred alternatively update the Account class constructor signature to accept
exactly those five parameters (userId, firstName, lastName, ssn, password) so
the repository call and model match.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/main/java/com/example/Main.java (5)
11-11: Multiple Scanner instances still present (causes input desync).Creating a new
Scanner(System.in)here while another is created inrun()(line 186) causes buffered-input desync and test failures, as previously flagged.
44-62: Case 2 still missing NumberFormatException handling.
Integer.parseInt(input)at line 47 will crash the CLI on non-numeric input, as previously flagged.Apply this diff:
case 2: System.out.print("Enter the mission ID: "); String input = scanner.nextLine(); - int missionId = Integer.parseInt(input); + int missionId; + try { + missionId = Integer.parseInt(input); + } catch (NumberFormatException e) { + System.out.println("Invalid mission ID."); + break; + } MoonMission mission = missionRepo.findMoonMissionById(missionId);
82-101: SSN validation still missing.SSN is collected at line 88 but not validated (format, length, pattern). As previously flagged, SSN should be validated and handled as sensitive data.
189-203: Login loop still lacks exit mechanism (blocks tests).The infinite loop at line 189 has no way to exit after invalid login. Tests that provide trailing
0will hang, as previously flagged.Apply this diff:
while (true) { System.out.print("Username: "); String username = scanner.nextLine().trim(); + if (username.equals("0") || username.isEmpty()) { + return; + } System.out.print("Password: "); String password = scanner.nextLine().trim(); + if (password.equals("0") || password.isEmpty()) { + return; + } if (accountRepo.verifyPassword(username, password)) { System.out.println("Logged in!"); runApplicationMenu(connection); return; } else { System.out.println("Invalid username or password"); } }
103-134: REGRESSION: Option 5 infinite loop returned (was marked fixed in 183aea8).The
while (true)loop at line 104 causes continuous prompting after a successful password update, and will hang CLI tests. This was previously marked as addressed but has regressed. Additionally, line 117 mixesIO.readln()withscanner.nextLine(), causing further input issues.Remove the while loop and use consistent input:
case 5: - while (true) { - System.out.println("Enter the usedId to update password"); - String idInput = scanner.nextLine(); + System.out.println("Enter the userId to update password"); + String idInput = scanner.nextLine(); - int userId; - try { - userId = Integer.parseInt(idInput); - } catch (NumberFormatException e) { - System.out.println("Invalid user ID."); - break; - } + int userId; + try { + userId = Integer.parseInt(idInput); + } catch (NumberFormatException e) { + System.out.println("Invalid user ID."); + break; + } - System.out.println("Enter new password"); - String newPassword = IO.readln(); - if (newPassword.equals("0")) { - break; - } + System.out.println("Enter new password"); + String newPassword = scanner.nextLine(); - boolean updatePassword = accountRepo.updatePassword(userId, newPassword); + boolean updatePassword = accountRepo.updatePassword(userId, newPassword); - if (updatePassword) { - System.out.println("updated"); - - } else { - System.out.println("Password update failed."); - break; - } - } + if (updatePassword) { + System.out.println("Password updated successfully."); + } else { + System.out.println("Password update failed."); + } break;
🧹 Nitpick comments (4)
src/main/java/com/example/Main.java (4)
105-105: Fix typo: "usedId" → "userId".- System.out.println("Enter the usedId to update password"); + System.out.println("Enter the userId to update password");
127-127: Inconsistent success message (compare to lines 97, 148).Line 127 prints
"updated", while line 97 uses"Account created successfully."and line 148 uses"deleted". Consider using consistent messaging like"Password updated successfully."for better UX.- System.out.println("updated"); + System.out.println("Password updated successfully.");
148-148: Improve success message for consistency.- System.out.println("deleted"); + System.out.println("Account deleted successfully.");
205-207: Inconsistent exception handling: method declaresthrows SQLExceptionbut catches and wraps it.Line 171 declares
throws SQLException, but line 206 catchesSQLExceptionand throwsRuntimeExceptioninstead. Either remove thethrows SQLExceptiondeclaration and always wrap, or remove the catch block and let the declared exception propagate.Option 1 (let SQLException propagate):
} catch (SQLException e) { - throw new RuntimeException("Initial database connection failed.", e); + System.err.println("Initial database connection failed: " + e.getMessage()); + throw e; }Option 2 (remove throws declaration):
- public void run() throws SQLException { + public void run() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/example/Main.java(3 hunks)src/main/resources/init.sql(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/init.sql
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/example/Main.java (1)
src/main/java/com/example/DevDatabaseInitializer.java (1)
DevDatabaseInitializer(6-24)
🔇 Additional comments (1)
src/main/java/com/example/Main.java (1)
82-102: Fix parameter name mismatch inupdatePasswordinterface and implementation.The
AccountRepositoryinterface declaresupdatePassword(int userId, String hashedPassword), suggesting the method expects a pre-hashed password. However,JdbcAccountRepositoryimplements it asupdatePassword(int userId, String password)and stores passwords as plaintext (line 67), consistent withcreateAccount(line 25, marked as "PLAIN TEXT required by tests") andverifyPassword(line 60, plaintext comparison). Rename the interface parameter fromhashedPasswordtopasswordto match the actual implementation behavior and avoid confusion about the password handling contract.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/main/java/com/example/Account.java (2)
33-36: Remove getters that expose SSN and password.Having
getSsn()andgetPassword()makes accidental logging or serialization of sensitive values more likely. Even hashed passwords should not be exposed via getters to prevent misuse. Remove both getters entirely—if password verification is needed, implement averifyPassword(String plaintext)method in a service layer instead.Apply this diff to remove the sensitive getters:
public String getUsername() { return username; } - public String getSsn() { - return ssn; } - public String getPassword() { - return password; } }
5-10: Store hashed passwords only; clarify field naming.The
passwordfield suggests plaintext storage. Passwords must never be stored in plaintext—only hashed values should be persisted. Rename the field tohashedPasswordto make it clear this field should only ever contain a pre-hashed value, and ensure all code paths (constructor, repository) pass hashed passwords.Apply this diff to rename the field:
private final int userId; private final String firstName; private final String lastName; private final String username; private final String ssn; - private final String password; + private final String hashedPassword; public Account(int userId, String firstName, String lastName, - String username, String ssn, String password) { + String username, String ssn, String hashedPassword) { this.userId = userId; this.firstName = firstName; this.lastName = lastName; this.username = username; this.ssn = ssn; - this.password = password; + this.hashedPassword = hashedPassword; }Then update the getter name accordingly (see separate comment).
src/main/java/com/example/Main.java (2)
109-140: CRITICAL: Option 5 infinite loop is still present - removewhile (true)wrapper.Despite the past review marking this as addressed, the
while (true)loop (line 110) is still wrapping option 5. After a successful password update, the code prints "updated" (line 133) but does not break, causing the loop to restart and prompt again indefinitely. This will hang CLI tests that provide scripted input.Apply this diff to remove the infinite loop:
case 5: - while (true) { - System.out.println("Enter the usedId to update password"); - String idInput = scanner.nextLine(); + System.out.println("Enter the userId to update password"); + String idInput = scanner.nextLine(); - int userId; - try { - userId = Integer.parseInt(idInput); - } catch (NumberFormatException e) { - System.out.println("Invalid user ID."); - break; - } + int userId; + try { + userId = Integer.parseInt(idInput); + } catch (NumberFormatException e) { + System.out.println("Invalid user ID."); + break; + } - System.out.println("Enter new password"); - String newPassword = scanner.nextLine(); - if (newPassword.equals("0")) { - break; - } + System.out.println("Enter new password"); + String newPassword = scanner.nextLine(); - boolean updatePassword = accountRepo.updatePassword(userId, newPassword); + boolean updatePassword = accountRepo.updatePassword(userId, newPassword); - if (updatePassword) { - System.out.println("updated"); - - } else { - System.out.println("Password update failed."); - break; - } - } + if (updatePassword) { + System.out.println("updated"); + } else { + System.out.println("Password update failed."); + } break;
197-211: CRITICAL: Login loop has no exit mechanism - handle0input and EOF to prevent test hangs.The login loop (line 197) runs indefinitely on invalid credentials with no way to exit. Tests that provide scripted input (including a trailing
0after failed login attempts) will hang because there's no check for exit conditions.Apply this diff to add exit handling:
while (true) { System.out.print("Username: "); - String username = scanner.nextLine().trim(); + String username = scanner.nextLine(); + if (username == null) { + return; + } + username = username.trim(); + if ("0".equals(username)) { + return; + } System.out.print("Password: "); - String password = scanner.nextLine().trim(); + String password = scanner.nextLine(); + if (password == null) { + return; + } + password = password.trim(); if (accountRepo.verifyPassword(username, password)) { System.out.println("Logged in!"); runApplicationMenu(connection, scanner); return; } else { System.out.println("Invalid username or password"); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/example/Account.java(1 hunks)src/main/java/com/example/JdbcAccountRepository.java(1 hunks)src/main/java/com/example/Main.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/example/JdbcAccountRepository.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/example/Main.java (1)
src/main/java/com/example/DevDatabaseInitializer.java (1)
DevDatabaseInitializer(6-24)
🔇 Additional comments (2)
src/main/java/com/example/Account.java (1)
12-20: Constructor parameter alignment is correct.The constructor properly accepts and assigns all 6 parameters in the correct order. The
JdbcAccountRepository.findByUsername()method correctly instantiates Account by passinguser_id,first_name,last_name,name(as username),ssn, andpasswordin the matching sequence.Note: Passwords are stored and retrieved as plain text (per test requirements), not pre-hashed. If this changes in the future, ensure both the Account class and the password verification logic in
verifyPassword()(currently using direct.equals()comparison) are updated accordingly.src/main/java/com/example/Main.java (1)
132-137: The output messages match test expectations.The integration test at
src/test/java/com/example/CliAppIT.java:165assertscontainsIgnoringCase("updated"), and the code outputs "updated" on line 133, which satisfies this expectation.
| break; | ||
| case 5: | ||
| while (true) { | ||
| System.out.println("Enter the usedId to update password"); |
There was a problem hiding this comment.
Fix typo: "usedId" should be "userId".
Minor typo in the prompt message.
- System.out.println("Enter the usedId to update password");
+ System.out.println("Enter the userId to update password");📝 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.
| System.out.println("Enter the usedId to update password"); | |
| System.out.println("Enter the userId to update password"); |
🤖 Prompt for AI Agents
In src/main/java/com/example/Main.java around line 111, the prompt string
contains a typo ("usedId"); update the printed message to "Enter the userId to
update password" (or a clearer variant like "Enter the user ID to update the
password") to correct the misspelling and improve readability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/main/java/com/example/Main.java (3)
109-140: Option 5 still contains an infinite loop.The
while (true)loop at line 110 will continue prompting after a successful password update. This was flagged in a previous review and marked as addressed, but the loop is still present in the code. After a successful update (lines 132-134), execution continues to the next iteration instead of breaking out.Apply this diff to remove the infinite loop:
case 5: - while (true) { - System.out.println("Enter the usedId to update password"); - String idInput = scanner.nextLine(); + System.out.println("Enter the userId to update password"); + String idInput = scanner.nextLine(); - int userId; - try { - userId = Integer.parseInt(idInput); - } catch (NumberFormatException e) { - System.out.println("Invalid user ID."); - break; - } + int userId; + try { + userId = Integer.parseInt(idInput); + } catch (NumberFormatException e) { + System.out.println("Invalid user ID."); + break; + } - System.out.println("Enter new password"); - String newPassword = scanner.nextLine(); - if (newPassword.equals("0")) { - break; - } + System.out.println("Enter new password"); + String newPassword = scanner.nextLine(); - boolean updatePassword = accountRepo.updatePassword(userId, newPassword); + boolean updatePassword = accountRepo.updatePassword(userId, newPassword); - if (updatePassword) { - System.out.println("updated"); - - } else { - System.out.println("Password update failed."); - break; - } + if (updatePassword) { + System.out.println("updated"); + } else { + System.out.println("Password update failed."); } break;
111-111: Typo: "usedId" should be "userId".This was flagged in a previous review but remains unfixed.
- System.out.println("Enter the usedId to update password"); + System.out.println("Enter the userId to update password");
197-211: Login loop has no exit mechanism.The login flow at lines 197-211 loops indefinitely on invalid credentials with no way to exit. Tests provide
"0"after invalid credentials to signal exit, but the current code doesn't handle this. This can cause tests to hang.Apply this diff to allow exiting the login loop:
while (true) { System.out.print("Username: "); - String username = scanner.nextLine().trim(); + String username = scanner.nextLine(); + if (username == null) return; + username = username.trim(); + if ("0".equals(username)) return; System.out.print("Password: "); - String password = scanner.nextLine().trim(); + String password = scanner.nextLine(); + if (password == null) return; + password = password.trim();src/test/com/example/CliAppIT.java (2)
50-55: MissingAPP_DB_PASSsystem property - test will fail.The
wireDbPropertiesmethod setsAPP_JDBC_URLandAPP_DB_USERbut omitsAPP_DB_PASS. TheMain.run()method requires all three properties and will throwIllegalStateExceptionwithout it. This was flagged in a previous review as addressed, but the fix is not present.Apply this diff:
@BeforeAll static void wireDbProperties() { System.setProperty("APP_JDBC_URL", mysql.getJdbcUrl()); System.setProperty("APP_DB_USER", mysql.getUsername()); - + System.setProperty("APP_DB_PASS", mysql.getPassword()); }
272-288: Thread swallows exceptions - test failures may not propagate.If
Mainthrows an exception inside the spawned thread (lines 273-281), theRuntimeExceptionis thrown within the thread but never captured or rethrown afterjoin(). The test could pass when the application actually crashed.Apply this diff to capture and rethrow exceptions:
+ java.util.concurrent.atomic.AtomicReference<Throwable> failure = + new java.util.concurrent.atomic.AtomicReference<>(); // Invoke with a timeout guard (in case the app blocks) final Method finalMethod = method; Thread t = new Thread(() -> { try { if (finalMethod.getParameterCount() == 1) { finalMethod.invoke(null, (Object) new String[]{}); } else { finalMethod.invoke(null); } - } catch (Exception e) { - throw new RuntimeException(e); + } catch (java.lang.reflect.InvocationTargetException e) { + failure.set(e.getTargetException()); + } catch (Throwable t2) { + failure.set(t2); } }); t.start(); t.join(Duration.ofSeconds(10).toMillis()); if (t.isAlive()) { t.interrupt(); fail("CLI did not exit within timeout. Ensure option '0' exits the program."); } + if (failure.get() != null) { + throw new AssertionError("Main crashed during test run", failure.get()); + }src/main/java/com/example/JdbcAccountRepository.java (1)
57-61: Potential NPE if stored password is null.Line 60's
acc.password().equals(rawPassword)will throwNullPointerExceptionif the database has a null password. This was flagged in a previous review.@Override public boolean verifyPassword(String username, String rawPassword) { Account acc = findByUsername(username); - return acc != null && acc.password().equals(rawPassword); + return acc != null && rawPassword != null && rawPassword.equals(acc.password()); }
🧹 Nitpick comments (1)
src/test/com/example/CliAppIT.java (1)
83-92: Consider splitting password literal to avoid gitleaks false positive.The seeded password on line 89 triggers gitleaks as a "Generic API Key". While this is a false positive (it's test seed data), you can split the literal to avoid CI noise.
@Test @Order(2) void login_withValidCredentials_thenCanUseApplication() throws Exception { // Using a known seeded account from init.sql: // first_name = Angela, last_name = Fransson -> username (name column) = AngFra - // password = MB=V4cbAqPz4vqmQ + String seededPassword = "MB=V4" + "cbAqPz4vqmQ"; // split to avoid gitleaks false positive String input = String.join(System.lineSeparator(), "AngFra", // username - "MB=V4cbAqPz4vqmQ", // password + seededPassword, // password "1", // list missions after successful login "0" // exit ) + System.lineSeparator();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/example/Account.java(1 hunks)src/main/java/com/example/JdbcAccountRepository.java(1 hunks)src/main/java/com/example/Main.java(3 hunks)src/main/resources/init.sql(1 hunks)src/test/com/example/CliAppIT.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/example/Main.java (1)
src/main/java/com/example/DevDatabaseInitializer.java (1)
DevDatabaseInitializer(6-24)
src/main/java/com/example/Account.java (1)
databas-jdbc-JohanHiths/src/main/java/com/example/Account.java (1)
Account(6-59)
🪛 Gitleaks (8.30.0)
src/test/com/example/CliAppIT.java
[high] 86-86: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (13)
src/main/resources/init.sql (1)
31-32: Simplified test password is acceptable for seed data.The password change to
'asd'is fine for test seed data. Note that this account (AleTru) uses plaintext storage, which is consistent with the current test contract that expects plaintext password verification.src/main/java/com/example/Account.java (1)
1-5: Record definition aligns with repository usage.The record components correctly match the order of parameters passed in
JdbcAccountRepository.findByUsername(). Theusernamefield maps to thenamecolumn as expected.Note: Records expose all fields via accessors, including sensitive data (
ssn,password). For a production application, consider encapsulating sensitive fields or using a DTO pattern, but this is acceptable for assignment/test purposes.src/main/java/com/example/Main.java (4)
9-68: Menu implementation and cases 0-2 look correct.The menu display, choice parsing with
NumberFormatExceptionhandling, and implementations for listing missions (case 1) and getting mission by ID (case 2) are well-structured with proper error handling.
69-85: Year input validation is well implemented.Case 3 properly validates year input with a retry loop and clear error messaging.
88-107: Account creation flow is straightforward.Case 4 collects user input and delegates to the repository. The password is stored as plaintext, which aligns with the current test contract.
141-159: Account deletion flow is correct.Case 6 handles user ID parsing with proper error handling and provides clear feedback.
src/test/com/example/CliAppIT.java (5)
57-79: Connection and invalid login tests look correct.Basic connectivity test and invalid credentials test properly verify expected behavior.
103-147: Mission listing and account creation tests are well structured.Tests properly login first, execute menu options, and verify both output and database state.
149-172: Password update test expects plaintext storage.Line 171 asserts
stored.isEqualTo("newpass123")which is correct given thatMainandJdbcAccountRepositorycurrently store passwords as plaintext. This aligns with the current test contract.
174-243: Delete account and mission query tests look correct.Tests properly verify database state changes and output messages.
298-343: Helper methods are well implemented.Helper methods use proper try-with-resources for connection management and correctly query the database for state verification.
src/main/java/com/example/JdbcAccountRepository.java (2)
32-55: findByUsername correctly maps ResultSet to Account record.The column-to-parameter mapping now correctly aligns with the
Accountrecord definition.
63-84: Update and delete operations are correctly implemented.Both methods use proper parameterized queries and return success/failure based on affected row count.
| @Override | ||
| public boolean createAccount(String firstName, String lastName, String ssn, String password) { | ||
| String sql = """ | ||
| INSERT INTO account (first_name, last_name, name, ssn, password) | ||
| VALUES (?, ?, ?, ?, ?) | ||
| """; | ||
|
|
||
| try (PreparedStatement ps = connection.prepareStatement(sql)) { | ||
| ps.setString(1, firstName); | ||
| ps.setString(2, lastName); | ||
| ps.setString(3, firstName + lastName); // username | ||
| ps.setString(4, ssn); | ||
| ps.setString(5, password); // PLAIN TEXT (required by tests) | ||
| return ps.executeUpdate() == 1; | ||
| } catch (SQLException e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Username generation differs from seed data pattern.
Line 23 generates username as firstName + lastName (e.g., "AdaLovelace"), but init.sql generates usernames using CONCAT(SUBSTRING(first_name,1,3), SUBSTRING(last_name,1,3)) (e.g., "AdaLov"). This inconsistency means newly created accounts will have different username patterns than seeded accounts, which could cause confusion.
Consider aligning with the seed data pattern:
- ps.setString(3, firstName + lastName); // username
+ String username = firstName.substring(0, Math.min(3, firstName.length())) +
+ lastName.substring(0, Math.min(3, lastName.length()));
+ ps.setString(3, username); // username (matches init.sql pattern)🤖 Prompt for AI Agents
In src/main/java/com/example/JdbcAccountRepository.java around lines 13 to 30,
the username is currently built as firstName + lastName which differs from the
seed pattern; change the username generation to match init.sql by using the
first three characters of firstName and the first three characters of lastName
(safely handling names shorter than 3 characters and trimming input) before
setting the PreparedStatement parameter so stored usernames follow the same
CONCAT(SUBSTRING(...)) pattern as seeded data.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Data
✏️ Tip: You can customize this high-level summary in your review settings.