Made changes for external data handling #763
Conversation
Add CRUD methods for FILE and SESSION tables
Added FileState enum and methods to get Job objects from DB
and dded functionality to insert FILE records into the database in the GUI module
implementation of get, update, delete methods in DbConnection
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive migration from file-based to database-driven job and file management. A new Changes
Sequence DiagramsequenceDiagram
participant User
participant UIController
participant DbConnection
participant FileDownloader
User->>UIController: Add Job
UIController->>DbConnection: Add File Record to Queue
DbConnection-->>UIController: Record Created
UIController->>User: Job Added
User->>FileDownloader: Start Download
FileDownloader->>DbConnection: Update File State
DbConnection-->>FileDownloader: State Updated
FileDownloader->>User: Download Status
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
App 💻
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Yeah! You did it 🎉 Now, Relax 😉, Grab a drink ☕, and wait for the maintainers to check your contributions. Meanwhile, you can discuss on other issues and solve them 😀. Thank You 😃!
Meanwhile you can also discuss about the project in our Discord Server 😀
This comment was marked as resolved.
This comment was marked as resolved.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| File file; | ||
| for (int i = 0; i < numberOfThreads; i++) { | ||
| file = Files.createTempFile(fileName.hashCode() + String.valueOf(i), ".tmp").toFile(); | ||
| fileOut = new FileOutputStream(file); |
Check warning
Code scanning / CodeQL
Potential output resource leak Warning
There was a problem hiding this comment.
Added close statement on line 98. I guess it isn't enough because there is always a possibility that an exception occurs before the method gets invoked and the streams won't get closed. Open to suggestions on how to fix that.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
Core/src/main/java/preferences/Get.java (1)
39-50: Ensure consistent exception handling injobHistorymethodIn the
jobHistorymethod, when catchingSQLException, aRuntimeExceptionis thrown without a descriptive message. To provide better context for debugging and consistent error handling, consider adding a specific message to theRuntimeException, similar to thejobsmethod.Apply this diff to improve the exception handling:
} catch (SQLException e) { - throw new RuntimeException(e); + throw new RuntimeException("Error fetching completed jobs from the database: " + e.getMessage(), e); }CLI/src/main/java/backend/FileDownloader.java (1)
117-129: Consolidate exception handling to reduce code duplicationThe exception handling code blocks for
SecurityException,FileNotFoundException,IOException, andNullPointerException(lines 117-134, 149-171, 198-201) contain duplicated code:
- Setting the
endDownloadingTime.- Updating the file info in the database with
FileState.FAILED.- Logging an error message.
Consider creating a private method to handle these exceptions uniformly. This method can update the database and log the error message based on the exception type.
For example:
private void handleDownloadException(Exception e, FileState state, String message) throws SQLException { String endDownloadingTime = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")); DbConnection db = DbConnection.getInstance(); db.updateFileInfo(fileId, state, endDownloadingTime, 0); M.msgDownloadError(message + " " + e.getMessage()); }Then, in your catch blocks, you can call:
catch (IOException e) { handleDownloadException(e, FileState.FAILED, "Failed to download contents:"); }Also applies to: 131-134, 149-171, 198-201
GUI/src/main/java/backend/FileDownloader.java (2)
97-121: Refactor database update logic to reduce code duplicationThe logic for updating the database after the download process (lines 97-121) is duplicated for both success and failure cases. Both blocks create
endDownloadingTimeand calldb.updateFileState(...).Consider extracting this logic into a separate method that accepts parameters necessary for the update. This will reduce duplication and make the code cleaner.
For example:
private void updateDownloadState(FileState state, long downloadedSize) throws SQLException { String endDownloadingTime = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")); db.updateFileState( job.getSourceLink(), job.getDir(), job.getFilename(), state, startDownloadingTime, endDownloadingTime, (int) downloadedSize ); }Then, you can call this method in both cases:
if (exitCode == 0) { long downloadedSize = new File(Paths.get(dir, filename).toString()).length(); updateDownloadState(FileState.COMPLETED, downloadedSize); } else { updateDownloadState(FileState.FAILED, 0); }
121-123: HandleSQLExceptionappropriately instead of throwingRuntimeExceptionCatching
SQLExceptionand throwing aRuntimeException(lines 121-123) can obscure the original exception and complicate error handling.Consider handling the
SQLExceptionwhere it occurs, possibly by logging the error and informing the user, or declare thethrows SQLExceptionin the method signature if you intend to propagate it.For example:
catch (SQLException e) { M.msgDownloadError("Database error occurred: " + e.getMessage()); // Additional error handling }GUI/src/main/java/ui/UIController.java (2)
503-507: ReplaceSystem.out.printlnstatements with proper loggingUsing
System.out.printlnfor logging (lines 503-507) is not recommended for production code.Consider using a logging framework or the existing
MessageBroker(M) to log messages, which allows better control over logging levels and outputs.For example:
M.msgInfo("Job Updated: " + newJob.getFilename());
496-537: Refactor database operations to avoid code duplicationThe database interaction code in
addJobandremoveJobFromListmethods is similar. To improve maintainability, consider refactoring the database operations into a separate utility method or class.For example, create a
JobDatabaseHandlerclass to manage job-related database operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Core/pom.xmlis excluded by!**/*.xml
📒 Files selected for processing (15)
.gitignore(1 hunks)CLI/src/main/java/backend/FileDownloader.java(6 hunks)CLI/src/main/java/module-info.java(1 hunks)Core/src/main/java/init/Environment.java(5 hunks)Core/src/main/java/module-info.java(1 hunks)Core/src/main/java/preferences/Get.java(3 hunks)Core/src/main/java/preferences/Set.java(0 hunks)Core/src/main/java/properties/FileState.java(1 hunks)Core/src/main/java/support/DownloadConfiguration.java(2 hunks)Core/src/main/java/support/JobHistory.java(0 hunks)Core/src/main/java/support/Jobs.java(0 hunks)Core/src/main/java/utils/DbConnection.java(1 hunks)GUI/src/main/java/backend/FileDownloader.java(3 hunks)GUI/src/main/java/module-info.java(1 hunks)GUI/src/main/java/ui/UIController.java(3 hunks)
💤 Files with no reviewable changes (3)
- Core/src/main/java/support/Jobs.java
- Core/src/main/java/preferences/Set.java
- Core/src/main/java/support/JobHistory.java
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- Core/src/main/java/properties/FileState.java
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Build [macos-13, GUI]
- GitHub Check: Build [macos-13, CLI]
- GitHub Check: Build [ubuntu-latest, GUI]
- GitHub Check: Build [ubuntu-latest, CLI]
- GitHub Check: Build [macos-14, GUI]
- GitHub Check: Build [macos-14, CLI]
- GitHub Check: Build [windows-latest, GUI]
- GitHub Check: Build [windows-latest, CLI]
- GitHub Check: Build and Scan [drifty-gui, windows-latest]
- GitHub Check: Build and Scan [drifty-cli, windows-latest]
- GitHub Check: Build and Scan [drifty-gui, macos-14]
- GitHub Check: Build and Scan [drifty-cli, macos-14]
- GitHub Check: Build and Scan [drifty-gui, macos-13]
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Dependency Review
- GitHub Check: Build and Scan [drifty-cli, macos-13]
- GitHub Check: build (GUI, drifty-gui)
- GitHub Check: Build and Scan [drifty-gui, ubuntu-latest]
- GitHub Check: Build and Scan [drifty-cli, ubuntu-latest]
- GitHub Check: Lint Code Base
- GitHub Check: build (CLI, drifty-cli)
🔇 Additional comments (6)
Core/src/main/java/init/Environment.java (2)
76-85: Database initialization logic is well-implementedThe addition of database setup in the
initializeEnvironment()method is effectively implemented with appropriate error handling. Logging the error message and terminating the application on failure ensures that the application does not continue in an inconsistent state.
100-106: Session end date update handles exceptions appropriatelyUpdating the session end date in the
terminate()method is correctly handled. Logging anySQLExceptionwithout interrupting the termination process ensures that the application exits gracefully while providing necessary error information.CLI/src/main/java/module-info.java (1)
6-6: LGTM! SQL module requirement aligns with database integration.The addition of the
java.sqlmodule requirement is appropriate for the transition to database-driven storage.Core/src/main/java/module-info.java (1)
6-6: LGTM! Module requirements are properly configured.The addition of both
org.slf4jandjava.sqlmodules is appropriate. The SLF4J integration will help with proper logging of database operations.Also applies to: 10-10
GUI/src/main/java/module-info.java (1)
11-11: LGTM! SQL module requirement is consistent across modules.The addition of the
java.sqlmodule requirement maintains consistency with the database integration approach across all modules.Core/src/main/java/support/DownloadConfiguration.java (1)
11-11: LGTM! Required imports are properly added.The new imports for database operations and error handling are appropriate for the changes.
Also applies to: 15-15, 19-19
| public static DbConnection getInstance() throws SQLException { | ||
| if (dbConnection != null) { | ||
| return dbConnection; | ||
| } | ||
| dbConnection = new DbConnection(); | ||
| return dbConnection; | ||
| } |
There was a problem hiding this comment.
Make the Singleton implementation of DbConnection thread-safe
The current Singleton pattern is not thread-safe. If multiple threads invoke getInstance() simultaneously, it could lead to the creation of multiple instances, potentially causing synchronization issues with the database connection.
To ensure thread safety, consider synchronizing the getInstance() method:
public static DbConnection getInstance() throws SQLException {
+ synchronized (DbConnection.class) {
if (dbConnection != null) {
return dbConnection;
}
dbConnection = new DbConnection();
return dbConnection;
+ }
}Alternatively, you can use the Initialization-on-demand holder idiom for a thread-safe Singleton without synchronization overhead:
public final class DbConnection {
private static Connection connection;
private DbConnection() throws SQLException {
if (connection != null && !connection.isClosed()) {
return;
}
connection = getConnection();
}
private static class DbConnectionHolder {
private static final DbConnection INSTANCE;
static {
try {
INSTANCE = new DbConnection();
} catch (SQLException e) {
throw new ExceptionInInitializerError(e);
}
}
}
public static DbConnection getInstance() {
return DbConnectionHolder.INSTANCE;
}
// ... rest of the class ...
}| public void createTables() throws SQLException { | ||
| String createSessionTableQuery = "CREATE TABLE IF NOT EXISTS SESSION (Id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, StartDate TEXT NOT NULL, EndDate TEXT);"; | ||
| String createFileTableQuery = "CREATE TABLE IF NOT EXISTS FILE (Id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, FileName TEXT NOT NULL, Url TEXT NOT NULL, SaveTargetPath TEXT NOT NULL, Size INTEGER, DownloadStartTime TEXT, DownloadEndTime TEXT, State INTEGER NOT NULL, SessionId INTEGER NOT NULL, FOREIGN KEY(SessionId) REFERENCES Session(Id));"; | ||
| PreparedStatement createSessionTableStatement = connection.prepareStatement(createSessionTableQuery); | ||
| createSessionTableStatement.executeUpdate(); | ||
| PreparedStatement createFileTableStatement = connection.prepareStatement(createFileTableQuery); | ||
| createFileTableStatement.executeUpdate(); | ||
| } |
There was a problem hiding this comment.
Close PreparedStatement and ResultSet resources to prevent leaks
In multiple methods, such as createTables(), addSessionRecord(), getCompletedJobs(), and others, PreparedStatement and ResultSet objects are not closed after use. This can lead to resource leaks and may exhaust database resources over time.
Use try-with-resources statements to ensure that resources are properly closed:
// Example for the createTables() method
public void createTables() throws SQLException {
String createSessionTableQuery = "CREATE TABLE IF NOT EXISTS SESSION ( ... );";
String createFileTableQuery = "CREATE TABLE IF NOT EXISTS FILE ( ... );";
try (PreparedStatement createSessionTableStatement = connection.prepareStatement(createSessionTableQuery);
PreparedStatement createFileTableStatement = connection.prepareStatement(createFileTableQuery)) {
createSessionTableStatement.executeUpdate();
createFileTableStatement.executeUpdate();
}
}Apply similar changes to other methods:
- Wrap
PreparedStatementandResultSetin try-with-resources blocks. - Remove the explicit calls to
executeUpdate()orexecuteQuery()outside the try-with-resources block.
This ensures that all resources are properly closed even if an exception occurs.
Also applies to: 43-55, 57-63, 65-81, 83-101, 102-110, 112-123, 125-133, 135-150, 152-168, 170-182, 184-195
| List<Long> partSizes = new ArrayList<>(numberOfThreads); | ||
| List<File> tempFiles = new ArrayList<>(numberOfThreads); | ||
| List<DownloaderThread> downloaderThreads = new ArrayList<>(numberOfThreads); | ||
| long partSize = Math.floorDiv(totalSize, numberOfThreads); | ||
| long start; | ||
| long end; | ||
| FileOutputStream fileOut; | ||
| File file; | ||
| for (int i = 0; i < numberOfThreads; i++) { | ||
| file = Files.createTempFile(fileName.hashCode() + String.valueOf(i), ".tmp").toFile(); | ||
| fileOut = new FileOutputStream(file); | ||
| start = i == 0 ? 0 : ((i * partSize) + 1); // The start of the range of bytes to be downloaded by the thread | ||
| end = (numberOfThreads - 1) == i ? totalSize : ((i * partSize) + partSize); // The end of the range of bytes to be downloaded by the thread | ||
| DownloaderThread downloader = new DownloaderThread(url, fileOut, start, end); | ||
| downloader.start(); | ||
| fileOutputStreams.add(fileOut); | ||
| partSizes.add(end - start); | ||
| downloaderThreads.add(downloader); | ||
| tempFiles.add(file); | ||
| } | ||
| ProgressBarThread progressBarThread = new ProgressBarThread(fileOutputStreams, partSizes, fileName, dir, totalSize, downloadMetrics); | ||
| progressBarThread.start(); | ||
| M.msgDownloadInfo(String.format(DOWNLOADING_F, fileName)); | ||
| // check if all the files are downloaded | ||
| while (!mergeDownloadedFileParts(fileOutputStreams, partSizes, downloaderThreads, tempFiles)) { | ||
| sleep(500); | ||
| } | ||
| for (File tempFile : tempFiles) { | ||
| Files.deleteIfExists(tempFile.toPath()); | ||
| } | ||
| } else { | ||
| InputStream urlStream = url.openStream(); | ||
| readableByteChannel = Channels.newChannel(urlStream); | ||
| FileOutputStream fos = new FileOutputStream(directoryPath.resolve(fileName).toFile()); | ||
| ProgressBarThread progressBarThread = new ProgressBarThread(fos, totalSize, fileName, dir, downloadMetrics); | ||
| progressBarThread.start(); | ||
| M.msgDownloadInfo(String.format(DOWNLOADING_F, fileName)); | ||
| fos.getChannel().transferFrom(readableByteChannel, 0, Long.MAX_VALUE); | ||
| fos.close(); | ||
| urlStream.close(); | ||
| } | ||
| ProgressBarThread progressBarThread = new ProgressBarThread(fileOutputStreams, partSizes, fileName, dir, totalSize, downloadMetrics); | ||
| progressBarThread.start(); | ||
| M.msgDownloadInfo(String.format(DOWNLOADING_F, fileName)); | ||
| // check if all the files are downloaded | ||
| while (!mergeDownloadedFileParts(fileOutputStreams, partSizes, downloaderThreads, tempFiles)) { | ||
| sleep(500); | ||
| } | ||
| for (File tempFile : tempFiles) { | ||
| Files.deleteIfExists(tempFile.toPath()); | ||
| } | ||
| } else { | ||
| InputStream urlStream = url.openStream(); | ||
| readableByteChannel = Channels.newChannel(urlStream); | ||
| FileOutputStream fos = new FileOutputStream(directoryPath.resolve(fileName).toFile()); | ||
| ProgressBarThread progressBarThread = new ProgressBarThread(fos, totalSize, fileName, dir, downloadMetrics); | ||
| progressBarThread.start(); | ||
| M.msgDownloadInfo(String.format(DOWNLOADING_F, fileName)); | ||
| fos.getChannel().transferFrom(readableByteChannel, 0, Long.MAX_VALUE); | ||
| fos.close(); | ||
| urlStream.close(); | ||
| downloadMetrics.setActive(false); | ||
| // keep the main thread from closing the IO for a short amount of time so the UI thread can finish and give output | ||
| Utility.sleep(1800); | ||
|
|
||
| Path downloadedFilePath = directoryPath.resolve(fileName); | ||
| long downloadedSize = Files.size(downloadedFilePath); | ||
| endDownloadingTime = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")); | ||
| db.updateFileInfo(fileId, FileState.COMPLETED, endDownloadingTime, (int) downloadedSize); | ||
| } catch (SecurityException e) { | ||
| endDownloadingTime = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")); | ||
| db.updateFileInfo(fileId, FileState.FAILED, endDownloadingTime, 0); | ||
| M.msgDownloadError("Write access to the download directory is DENIED! " + e.getMessage()); | ||
| } catch (FileNotFoundException fileNotFoundException) { | ||
| endDownloadingTime = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")); | ||
| db.updateFileInfo(fileId, FileState.FAILED, endDownloadingTime, 0); | ||
| M.msgDownloadError(FILE_NOT_FOUND_ERROR); | ||
| } catch (IOException e) { | ||
| endDownloadingTime = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")); | ||
| db.updateFileInfo(fileId, FileState.FAILED, endDownloadingTime, 0); | ||
| M.msgDownloadError(FAILED_TO_DOWNLOAD_CONTENTS + e.getMessage()); | ||
| } | ||
| downloadMetrics.setActive(false); | ||
| // keep the main thread from closing the IO for a short amount of time so the UI thread can finish and give output | ||
| Utility.sleep(1800); | ||
| } catch (SecurityException e) { | ||
| M.msgDownloadError("Write access to the download directory is DENIED! " + e.getMessage()); | ||
| } catch (FileNotFoundException fileNotFoundException) { | ||
| M.msgDownloadError(FILE_NOT_FOUND_ERROR); | ||
| } catch (IOException e) { | ||
| M.msgDownloadError(FAILED_TO_DOWNLOAD_CONTENTS + e.getMessage()); | ||
| } catch (NullPointerException e) { | ||
| endDownloadingTime = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")); | ||
| db.updateFileInfo(fileId, FileState.FAILED, endDownloadingTime, 0); | ||
| M.msgDownloadError(FAILED_READING_STREAM); | ||
| } | ||
| } catch (NullPointerException e) { | ||
| M.msgDownloadError(FAILED_READING_STREAM); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor nested try-catch blocks to improve readability and maintainability
The downloadFile method contains deeply nested try-catch blocks, which can make the code hard to read and maintain. For example, starting from line 59, there are multiple levels of nesting due to sequential try-catch statements.
Consider refactoring the method to reduce the nesting level. You can achieve this by:
- Separating the download logic into smaller helper methods.
- Using a single
try-catchblock with multiplecatchclauses for different exceptions. - Handling exceptions at appropriate levels to simplify the flow.
| endDownloadingTime = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")); | ||
| db.updateFileInfo(fileId, FileState.FAILED, endDownloadingTime, 0); | ||
| } | ||
| } catch (MalformedURLException | URISyntaxException e) { | ||
| M.msgLinkError(INVALID_LINK); | ||
| } catch (IOException e) { | ||
| M.msgDownloadError(String.format(FAILED_CONNECTION_F, url)); | ||
| } catch (SQLException e) { |
There was a problem hiding this comment.
Log or rethrow caught exceptions to aid debugging
In the run method, the generic catch (Exception e) block at lines 286-289 catches all exceptions but does not log or handle them. Swallowing exceptions can make debugging difficult.
Consider logging the exception details or rethrowing the exception to ensure that unexpected errors are properly recorded.
For example:
catch (Exception e) {
endDownloadingTime = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss"));
db.updateFileInfo(fileId, FileState.FAILED, endDownloadingTime, 0);
M.msgDownloadError("An unexpected error occurred: " + e.getMessage());
throw e; // or log the stack trace
}| String[] messageArray = msg.split(","); | ||
| if (messageArray.length >= 1 && messageArray[0].toLowerCase().trim().replaceAll(" ", "").contains("cannotrunprogram")) { // If yt-dlp program is not marked as executable | ||
| M.msgDownloadError(DRIFTY_COMPONENT_NOT_EXECUTABLE_ERROR); | ||
| } else if (messageArray.length >= 1 && "permissiondenied".equals(messageArray[1].toLowerCase().trim().replaceAll(" ", ""))) { // If a private YouTube / Instagram video is asked to be downloaded | ||
| M.msgDownloadError(PERMISSION_DENIED_ERROR); | ||
| } else if ("videounavailable".equals(messageArray[0].toLowerCase().trim().replaceAll(" ", ""))) { // If YouTube / Instagram video is unavailable | ||
| M.msgDownloadError(VIDEO_UNAVAILABLE_ERROR); | ||
| } else { | ||
| M.msgDownloadError("An Unknown Error occurred! " + e.getMessage()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid parsing exception messages to determine error types
In the downloadYoutubeOrInstagram method, lines 162-171 parse exception messages to handle different error scenarios. This approach is fragile because exception messages can change and are not intended for programmatic use.
Consider catching specific exception types or using error codes to handle different error conditions more reliably.
For example, if possible:
- Catch specific exceptions like
ProcessNotFoundException,PermissionDeniedException, etc. - Utilize exit codes returned by the process to determine the error type.
| job.getFilename(), | ||
| FileState.COMPLETED, | ||
| startDownloadingTime, | ||
| endDownloadingTime, | ||
| (int) downloadedSize | ||
| ); | ||
| } else { | ||
| String endDownloadingTime = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss")); | ||
| db.updateFileState( | ||
| job.getSourceLink(), | ||
| job.getDir(), | ||
| job.getFilename(), | ||
| FileState.COMPLETED, | ||
| startDownloadingTime, | ||
| endDownloadingTime, | ||
| 0 | ||
| ); | ||
| } | ||
| } catch(SQLException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| switch (type) { | ||
| case YOUTUBE, INSTAGRAM -> downloadYoutubeOrInstagram(LinkType.getLinkType(job.getSourceLink()).equals(LinkType.SPOTIFY)); | ||
| case OTHER -> splitDecision(); |
There was a problem hiding this comment.
Handle potential NullPointerException when process is null
In the downloadYoutubeOrInstagram method, if an exception occurs during processBuilder.start(), the process variable may remain null. Later, at line 127, exitCode = Objects.requireNonNull(process).waitFor(); will throw a NullPointerException.
Add a null check for process before calling process.waitFor(), or ensure that process is not null before proceeding.
For example:
if (process != null) {
exitCode = process.waitFor();
} else {
exitCode = 1; // or handle accordingly
}| try { | ||
| DbConnection dbConnection = DbConnection.getInstance(); | ||
| dbConnection.updateFile( | ||
| newJob.getFilename(), | ||
| oldJob.getSourceLink(), | ||
| newJob.getDir() | ||
| ); | ||
| } catch(SQLException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| System.out.println("Job Updated: " + newJob.getFilename()); | ||
| } | ||
| else { | ||
| try { | ||
| DbConnection dbConnection = DbConnection.getInstance(); | ||
| dbConnection.addFileRecordToQueue( | ||
| newJob.getFilename(), | ||
| newJob.getSourceLink(), | ||
| newJob.getDir(), | ||
| currentSessionId | ||
| ); | ||
| } catch(SQLException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| System.out.println("Job Added: " + newJob.getFilename()); | ||
| } | ||
| getJobs().add(newJob); | ||
| System.out.println("Job Added: " + newJob.getFilename()); | ||
| commitJobListToListView(); | ||
| } | ||
|
|
||
| private void removeJobFromList(Job oldJob) { | ||
| getJobs().remove(oldJob); | ||
| try { | ||
| DbConnection dbConnection = DbConnection.getInstance(); | ||
| dbConnection.deleteQueuedFile( | ||
| oldJob.getSourceLink(), | ||
| oldJob.getDir(), | ||
| oldJob.getFilename() | ||
| ); | ||
| } catch(SQLException e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle SQLException properly and avoid throwing RuntimeException
In the methods addJob (lines 496-520), removeJobFromList (lines 528-537), and clearJobHistory (lines 632-637), SQLException is caught and rethrown as a RuntimeException. This practice can hide the original exception and make it harder to debug.
Consider handling SQLException by:
- Logging the error with details.
- Informing the user if necessary.
- Deciding whether to continue execution or terminate gracefully.
For example:
catch (SQLException e) {
M.msgError("Database error occurred while adding job: " + e.getMessage());
// Handle the error appropriately
}Also applies to: 632-637
| try { | ||
| DbConnection dbConnection = DbConnection.getInstance(); | ||
| dbConnection.addFileRecordToQueue( | ||
| filename, | ||
| link, | ||
| directory, | ||
| currentSessionId | ||
| ); | ||
| } catch(SQLException e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and database operation management.
The current implementation has several areas for improvement:
- Wrapping SQLException in RuntimeException loses the error context
- Database connection management could be improved
- Transaction boundaries are not defined
Consider this improved implementation:
try {
DbConnection dbConnection = DbConnection.getInstance();
+ dbConnection.beginTransaction();
dbConnection.addFileRecordToQueue(
filename,
link,
directory,
currentSessionId
);
+ dbConnection.commitTransaction();
} catch(SQLException e) {
- throw new RuntimeException(e);
+ msgBroker.msgLogError("Failed to add file record to queue: " + e.getMessage());
+ try {
+ DbConnection.getInstance().rollbackTransaction();
+ } catch (SQLException rollbackEx) {
+ msgBroker.msgLogError("Failed to rollback transaction: " + rollbackEx.getMessage());
+ }
+ // Consider if we should continue processing other files or abort
}Committable suggestion skipped: line range outside the PR's diff.
💡 Codebase verification
Confirmed: Job list and database state can become inconsistent on DB failures
The verification reveals that if the database operation fails after adding the job to distinctJobList, the job still gets committed to AppSettings.jobs() through setList(), creating an inconsistency between the application state and database. Consider:
- Moving the distinctJobList.put() after successful DB operation
- Adding rollback logic to remove the job from distinctJobList if DB operation fails
🔗 Analysis chain
Verify database operations impact on job list consistency.
The database operation is performed after adding the job to the distinctJobList. If the database operation fails, the job list might be inconsistent with the database state.
Let's verify the potential impact:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other places where job list and database operations are coupled
rg -l "AppSettings.*jobs.*setList|addFileRecordToQueue" --type java
# Check for any existing error handling patterns
ast-grep --pattern 'catch \(SQLException $var\) {
$$$
}'
Length of output: 266
Script:
#!/bin/bash
# Check distinctJobList usage
rg "distinctJobList" --type java -B 2 -A 2
# Check addFileRecordToQueue implementation
rg "addFileRecordToQueue" --type java -B 2 -A 2
# Look for transaction management
rg "beginTransaction|commit|rollback" --type java
Length of output: 4531
|
@michalryn @Rafalrrr159 Hi Michał and Rafał 👋! |
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Michał Ryniewicz <[email protected]>
There was a problem hiding this comment.
@michalryn Awesome work by your team 👏!
I would request you to make certain changes as listed:
- After Drifty CLI downloaded a spotify song (here's the link -
https://open.spotify.com/track/36TUJPlbeuIAwq5X5W5ZaR), it successfully converts the webm to mp3 file but prints some irrelevant error message thereafter.
- When I download the same song via Drifty GUI, it shows that it is an invalid link. On further investigation, I found that the way GUI handles the link addition to DB and its further usage, is an issue. Check the below two images.
- Why only the file size of youtube displayed? It stores
0as the file size for Spotify songs.
- Also, I think storing the
downloadLinkandFileURL(you can use these names for column name) separately will help in preventing ambiguities in links especially for Spotify. What do you think?
| @@ -4,6 +4,7 @@ config/*.o | |||
| target/ | |||
| Website/.next | |||
| Website/node_modules | |||
| *.db | |||
There was a problem hiding this comment.
@michalryn Drifty already creates a .drifty folder (for linux and mac) and Drifty folder in Applications folder (probably) in case of windows. I would recommend putting the db file there. Also, it is better to separately name the db as drifty_cli.db and drifty_gui.db, to prevent data loss.
You can use the Program class which gives you the path to the drifty folder; the code would look like this: Program.get(Program.DRIFTY_PATH).
| @@ -0,0 +1,5 @@ | |||
| package properties; | |||
|
|
|||
| public enum FileState { | |||
There was a problem hiding this comment.
@michalryn While I was browsing the db created by Drifty just now, I saw it stores State as numbers. Just curious, what do each numbers mean? Are they the enum values?
| ); | ||
| } | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Is it possible to handle this exception in a better way?
| DbConnection dbConnection = DbConnection.getInstance(); | ||
| dbConnection.deleteFilesHistory(); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
We can show a popup mesage using ConfirmationDialog class instead of throwing RuntimeException. What do you think?
|
@michalryn Did you face any issue? Please let me know. |
Fixes issue
Fixes #411
Changes proposed
DbConnectionclass for creating and interacting with the databaseFileStateenum for representing different file statusesCheck List (Check all the applicable boxes)
Summary by CodeRabbit
Release Notes
New Features
Database Integration
System Changes
Technical Improvements