Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis PR introduces a data-access layer using the repository pattern with two new repository classes (AccountRepository, MoonMissionRepository) and a custom DataSource implementation. The Main class is refactored to instantiate these repositories and provide an interactive CLI for user login and database operations. A logging dependency is added to pom.xml. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as Main (CLI)
participant AcctRepo as AccountRepository
participant DataSource as SimpleDriverManagerDataSource
participant DB as Database
participant MissionRepo as MoonMissionRepository
User->>CLI: Start application
CLI->>CLI: Initialize SimpleDriverManagerDataSource
CLI->>User: Display login prompt
User->>CLI: Enter username & password
CLI->>AcctRepo: login(username, password)
AcctRepo->>DataSource: getConnection()
DataSource->>DB: DriverManager.getConnection()
DB-->>DataSource: Connection
DataSource-->>AcctRepo: Connection
AcctRepo->>DB: SELECT * FROM accounts (via PreparedStatement)
DB-->>AcctRepo: ResultSet or empty
AcctRepo-->>CLI: true/false
alt Login Successful
CLI->>User: Display main menu
User->>CLI: Select operation (e.g., list missions)
CLI->>MissionRepo: listMissions()
MissionRepo->>DataSource: getConnection()
DataSource->>DB: DriverManager.getConnection()
DB-->>DataSource: Connection
DataSource-->>MissionRepo: Connection
MissionRepo->>DB: SELECT spacecraft FROM moon_mission
DB-->>MissionRepo: ResultSet
MissionRepo->>User: Print missions to stdout
MissionRepo-->>CLI: Complete
CLI->>User: Redisplay menu
else Login Failed
CLI->>User: Display error message
CLI->>CLI: Exit or retry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/com/example/com/example/CliAppIT.java (1)
1-1: Fix test directory structure to follow Maven conventions.The file is located at
src/test/com/example/com/example/CliAppIT.javawith duplicatecom/examplesegments. Maven expects integration tests undersrc/test/java/, so this should be moved tosrc/test/java/com/example/CliAppIT.java. There is no customtestSourceDirectoryconfiguration in pom.xml to justify this non-standard structure.
🧹 Nitpick comments (8)
pom.xml (1)
54-58: Consider adding<scope>test</scope>to slf4j-nop.The
slf4j-nopdependency is typically used to silence SLF4J logging warnings from Testcontainers during tests. If it's not needed at runtime, scope it totestto avoid including it in production artifacts.<dependency> <groupId>org.slf4j</groupId> <artifactId>slf4j-nop</artifactId> <version>2.0.17</version> + <scope>test</scope> </dependency>Note: If
DevDatabaseInitializerruns at application startup in dev mode and triggers SLF4J warnings, then the current (compile) scope may be intentional.src/main/java/com/example/MoonMissionRepository.java (2)
13-22: Repository methods should return data, not print to console.The repository layer directly writes to
System.out, coupling data access to presentation. A cleaner design returns data (e.g.,List<String>) and lets the caller handle output. This improves testability and reusability.For a simple CLI assignment this may be acceptable, but consider:
- public void listMissions() throws SQLException { + public List<String> listMissions() throws SQLException { String sql = "SELECT spacecraft FROM moon_mission ORDER BY mission_id"; + List<String> missions = new ArrayList<>(); try (Connection conn = ds.getConnection(); PreparedStatement ps = conn.prepareStatement(sql); ResultSet rs = ps.executeQuery()) { while (rs.next()) { - System.out.println(rs.getString("spacecraft")); + missions.add(rs.getString("spacecraft")); } } + return missions; }
24-37: No feedback when mission is not found.
getMissionByIdsilently returns nothing if the ID doesn't exist, leaving the user with no indication that the query succeeded but found no results. Consider printing a "Mission not found" message or returning anOptional.try (ResultSet rs = ps.executeQuery()) { if (rs.next()) { System.out.println("Mission ID: " + rs.getLong("mission_id")); System.out.println("Spacecraft: " + rs.getString("spacecraft")); System.out.println("Launch Date: " + rs.getDate("launch_date")); + } else { + System.out.println("Mission not found for ID: " + id); } }src/main/java/com/example/Main.java (2)
156-156: Avoid closing Scanner wrapping System.in.Calling
scanner.close()also closes the underlyingSystem.instream, which cannot be reopened. If the application or tests attempt to read fromSystem.inafterward, it will fail. For a CLI that terminates after the loop, this is typically harmless, but consider removing this line for safety.- scanner.close(); + // Intentionally not closing scanner to avoid closing System.in
94-105: Plaintext password storage.Passwords are stored in plaintext via
accountRepo.createAccount(). For a learning exercise this is acceptable, but production systems should hash passwords (e.g., using BCrypt).src/main/java/com/example/SimpleDriverManagerDataSource.java (1)
10-19: Simple DataSource implementation is appropriate for learning purposes.The implementation is thread-safe with immutable fields. Consider adding null checks in the constructor for production use.
For production environments, consider:
- Using a connection pool library like HikariCP
- Adding validation for null/empty parameters
public SimpleDriverManagerDataSource(String url, String user, String pass) { + if (url == null || url.trim().isEmpty()) { + throw new IllegalArgumentException("URL cannot be null or empty"); + } this.url = url; this.user = user; this.pass = pass; }src/main/java/com/example/AccountRepository.java (2)
38-38: Remove redundant setAutoCommit call.Setting autoCommit to
trueis redundant since it's the default value for new connections.try (Connection conn = ds.getConnection()) { - conn.setAutoCommit(true); - String candidate = baseName;
53-57: Replace System.out with proper logging.Using
System.out.printlnfor application logging is not recommended. Consider using SLF4J (already added to dependencies).Add a logger field to the class:
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public class AccountRepository { + private static final Logger logger = LoggerFactory.getLogger(AccountRepository.class); private final DataSource ds;Replace the System.out calls:
try (ResultSet keys = ps.getGeneratedKeys()) { if (keys.next()) { - System.out.println("Account created with user_id=" + keys.getLong(1) - + " and username=" + candidate); + logger.info("Account created with user_id={} and username={}", + keys.getLong(1), candidate); } else { - System.out.println("Account created with username=" + candidate); + logger.info("Account created with username={}", candidate); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)pom.xml(2 hunks)src/main/java/com/example/AccountRepository.java(1 hunks)src/main/java/com/example/DevDatabaseInitializer.java(1 hunks)src/main/java/com/example/Main.java(2 hunks)src/main/java/com/example/MoonMissionRepository.java(1 hunks)src/main/java/com/example/SimpleDriverManagerDataSource.java(1 hunks)src/test/com/example/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)
🔇 Additional comments (12)
src/main/java/com/example/DevDatabaseInitializer.java (1)
24-24: LGTM!No functional changes – only EOF formatting adjustment.
src/test/com/example/com/example/CliAppIT.java (1)
341-341: LGTM!No functional changes – only EOF formatting adjustment.
README.md (1)
1-1: LGTM!GitHub Classroom assignment badge added appropriately.
pom.xml (1)
49-53: Verify intended scope for testcontainers mysql.The
testcontainers:mysqldependency has no explicit scope (defaults tocompile), meaning it will be bundled in production artifacts. This appears intentional for the dev-modeDevDatabaseInitializer, but verify this is the desired behavior.src/main/java/com/example/MoonMissionRepository.java (1)
6-11: LGTM!Constructor injection of
DataSourcefollows good dependency injection practices, making the repository testable and decoupled from connection configuration.src/main/java/com/example/Main.java (3)
45-51: Login failure exits without consuming remaining input.After an invalid login, the method returns immediately at line 48. The test provides
"0"after invalid credentials (line 69-70 of CliAppIT), which remains unconsumed. This works because the test doesn't require processing that input, but the behavior could be confusing. The comment acknowledges this, which is good.
29-31: Good use of repository pattern with DataSource injection.Clean initialization of the data-access layer with dependency injection, making the repositories testable and decoupled from configuration resolution.
76-153: Menu implementation handles errors appropriately.The switch-case structure with outer try-catch for
SQLExceptionandNumberFormatExceptionprovides resilient error handling that prevents crashes on invalid input.src/main/java/com/example/SimpleDriverManagerDataSource.java (2)
21-29: Connection methods correctly delegate to DriverManager.Both overloads properly implement the DataSource contract. Ensure callers close connections in try-with-resources blocks to prevent resource leaks.
31-64: Unsupported operations are appropriately stubbed.The implementation correctly signals unsupported operations. This is acceptable for a minimal DataSource wrapper.
src/main/java/com/example/AccountRepository.java (2)
76-85: Update implementation is correct; ensure password hashing.The method correctly uses prepared statements and returns success status. However, the
newPasswordparameter should be hashed before storage (related to the critical security issue flagged in theloginmethod).
88-96: No cascade concerns needed—tables are independent.The
deleteAccountmethod is correctly implemented. The database schema contains no foreign key constraints or relationships between theaccountandmoon_missiontables, so no cascade handling or explicit cleanup is required.
| public boolean login(String username, String password) throws SQLException { | ||
| String sql = "SELECT 1 FROM account WHERE name=? AND password=?"; | ||
| try (Connection conn = ds.getConnection(); | ||
| PreparedStatement ps = conn.prepareStatement(sql)) { | ||
| ps.setString(1, username); | ||
| ps.setString(2, password); | ||
| try (ResultSet rs = ps.executeQuery()) { | ||
| return rs.next(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Plaintext password storage and comparison.
The login method compares passwords in plaintext, which is a severe security vulnerability. Passwords must be hashed using a secure algorithm like BCrypt or Argon2.
This affects the entire account system:
- Line 15: Query retrieves plaintext passwords
- Line 19: Compares unhashed password parameter
- Related:
createAccount(line 45) andupdatePassword(line 80) also handle plaintext passwords
Recommended fix:
- Hash passwords before storing (in
createAccount) - Hash the login password and compare hashes
- Update
updatePasswordto hash new passwords
Example using BCrypt:
import org.mindrot.jbcrypt.BCrypt;
public boolean login(String username, String password) throws SQLException {
String sql = "SELECT password FROM account WHERE name=?";
try (Connection conn = ds.getConnection();
PreparedStatement ps = conn.prepareStatement(sql)) {
ps.setString(1, username);
try (ResultSet rs = ps.executeQuery()) {
if (rs.next()) {
String hashedPassword = rs.getString("password");
return BCrypt.checkpw(password, hashedPassword);
}
return false;
}
}
}Add BCrypt dependency to pom.xml:
<dependency>
<groupId>org.mindrot</groupId>
<artifactId>jbcrypt</artifactId>
<version>0.4</version>
</dependency>🤖 Prompt for AI Agents
In src/main/java/com/example/AccountRepository.java around lines 14 to 24, the
login method currently retrieves and compares plaintext passwords; change it to
retrieve the stored hashed password for the user and verify the provided
password with a secure password-hashing library (e.g., BCrypt.checkpw). Also
update createAccount (around line 45) to hash passwords before inserting into
the DB and update updatePassword (around line 80) to hash new passwords before
saving; add the BCrypt dependency to pom.xml and ensure any migration plan is in
place to re-hash existing plaintext passwords or force password resets. Ensure
prepared statements still use parameter binding and do not log raw passwords.
| while (true) { | ||
| try (PreparedStatement ps = conn.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS)) { | ||
| ps.setString(1, candidate); | ||
| ps.setString(2, password); // medvetet: lösenord trimmas inte | ||
| ps.setString(3, base); | ||
| ps.setString(4, sur); | ||
| ps.setString(5, ssnTrim); | ||
| ps.executeUpdate(); | ||
|
|
||
| try (ResultSet keys = ps.getGeneratedKeys()) { | ||
| if (keys.next()) { | ||
| System.out.println("Account created with user_id=" + keys.getLong(1) | ||
| + " and username=" + candidate); | ||
| } else { | ||
| System.out.println("Account created with username=" + candidate); | ||
| } | ||
| } | ||
| return candidate; | ||
| } catch (SQLException e) { | ||
| // SQLState 23000 = integrity constraint violation (includes unique constraint) | ||
| String sqlState = e.getSQLState(); | ||
| if ("23000".equals(sqlState) || e.getMessage().toLowerCase().contains("duplicate") || e.getMessage().toLowerCase().contains("unique")) { | ||
| suffix++; | ||
| candidate = baseName + suffix; | ||
| // prova igen med nytt kandidatnamn | ||
| continue; | ||
| } | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add safety limit to prevent infinite retry loop.
The retry logic could loop indefinitely if a non-duplicate constraint violation occurs repeatedly (e.g., invalid SSN format, NULL constraint on another field).
Add a maximum retry limit:
String candidate = baseName;
int suffix = 0;
+ int maxRetries = 100;
+ int attempts = 0;
- while (true) {
+ while (attempts < maxRetries) {
+ attempts++;
try (PreparedStatement ps = conn.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS)) {
ps.setString(1, candidate);
ps.setString(2, password);
ps.setString(3, base);
ps.setString(4, sur);
ps.setString(5, ssnTrim);
ps.executeUpdate();
try (ResultSet keys = ps.getGeneratedKeys()) {
if (keys.next()) {
System.out.println("Account created with user_id=" + keys.getLong(1)
+ " and username=" + candidate);
} else {
System.out.println("Account created with username=" + candidate);
}
}
return candidate;
} catch (SQLException e) {
String sqlState = e.getSQLState();
if ("23000".equals(sqlState) || e.getMessage().toLowerCase().contains("duplicate") || e.getMessage().toLowerCase().contains("unique")) {
suffix++;
candidate = baseName + suffix;
continue;
}
throw e;
}
}
+ throw new SQLException("Failed to create account after " + maxRetries + " attempts");Committable suggestion skipped: line range outside the PR's diff.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.