Fix: wrong YouTube playlist detection #630
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related issues
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
the
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 😀
❌ Linting errors found!@Tarunkumarkanakam Please fix the following errors: CLI/src/main/java/main/Drifty_CLI.java:309:29: '}' at column 29 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurly]
Core/src/main/java/utils/Utility.java:917:9: 'if' construct must use '{}'s. [NeedBraces]
CLI/src/main/java/main/Drifty_CLI.java:296:21: '}' at column 21 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurly]Generated from this workflow run (ID: 10766866379) |
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- CLI/src/main/java/main/Drifty_CLI.java (1 hunks)
- Core/src/main/java/utils/Utility.java (1 hunks)
Additional comments not posted (1)
Core/src/main/java/utils/Utility.java (1)
909-924: Approve the implementation ofcontains.The
containsmethod is implemented correctly and efficiently checks for the presence of a value in an array. It handles null arrays gracefully by returning false immediately, which is a good defensive programming practice.
| else if (isYoutube(link)) { | ||
| String videoId = null; | ||
| try { | ||
| URI uri = URI.create(link); | ||
| String domain = uri.getHost(); | ||
|
|
||
| // checking if the domain is youtu.be | ||
| if ("youtu.be".equals(domain)) { | ||
| String path = uri.getPath(); | ||
| if (path != null && path.length() > 1) { | ||
| videoId = path.substring(1); // removing the leading "/" | ||
| } | ||
| } | ||
| // checking if the domain is youtube.com | ||
| else if ("www.youtube.com".equals(domain) || "youtube.com".equals(domain)) { | ||
| Map<String, String> queryParams = extractQueryParams(link, "v"); | ||
| videoId = queryParams.get("v"); | ||
| } | ||
|
|
||
| if (videoId != null) { | ||
| // constructing link in youtube.com/watch?v={videoID} | ||
| uri = new URI("https", "www.youtube.com", "/watch", "v=" + videoId, null); | ||
| link = uri.toString(); | ||
| } else { | ||
| messageBroker.msgLinkError("YouTube video ID not found in the link."); | ||
| messageBroker.msgInputInfo(QUIT_OR_CONTINUE, true); | ||
| String choice = SC.next().toLowerCase().strip(); | ||
| if ("q".equals(choice)) { | ||
| LOGGER.log(MessageType.INFO, CLI_APPLICATION_TERMINATED); | ||
| break; | ||
| } | ||
| printBanner(); | ||
| continue; | ||
| } | ||
|
|
||
| } catch (IllegalArgumentException | URISyntaxException e) { | ||
| messageBroker.msgLinkError("Failed to process the YouTube link: " + e.getMessage()); | ||
| } |
There was a problem hiding this comment.
Refactor YouTube link handling for clarity and robustness.
The handling of YouTube links in the Drifty_CLI class is functional but could be improved for better clarity and error handling:
- Domain Checking: The method uses string comparisons for domain checking, which could be refactored into a more robust method using URL parsing.
- Error Handling: The method could provide more specific error messages related to URI creation and parsing issues.
Consider refactoring the code to improve readability and error handling:
- if ("youtu.be".equals(domain)) {
+ if (domain.endsWith("youtu.be")) { // More flexible domain checking
- } else if ("www.youtube.com".equals(domain) || "youtube.com".equals(domain)) {
+ } else if (domain.endsWith("youtube.com")) {
- uri = new URI("https", "www.youtube.com", "/watch", "v=" + videoId, null);
+ try {
+ uri = new URI("https", "www.youtube.com", "/watch", "v=" + videoId, null);
+ link = uri.toString();
+ } catch (URISyntaxException e) {
+ messageBroker.msgLinkError("Failed to construct YouTube watch URL: " + e.getMessage());
+ continue; // Skip further processing and allow user to correct input
+ }Committable suggestion was skipped due to low confidence.
SaptarshiSarkar12
changed the title
|
@Tarunkumarkanakam After PR #632 has been merged, the code has been refactored a lot to centralize the process of fetching the required data for downloading any type of file, via the Drifty/Core/src/main/java/support/DownloadConfiguration.java Lines 39 to 51 in aac9a81 Please pull the latest changes at first before starting the modification. If you face any issue or have any suggestion, please let me know. |
❌ Linting errors found!@Tarunkumarkanakam Please fix the following errors: Core/src/main/java/utils/Utility.java:872:9: '}' at column 9 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurly]
Core/src/main/java/utils/Utility.java:942:9: 'if' construct must use '{}'s. [NeedBraces]
CLI/src/main/java/main/Drifty_CLI.java:297:21: '}' at column 21 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurly]Generated from this workflow run (ID: 11100480958) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
Core/src/main/java/utils/Utility.java (1)
862-887: LGTM! Consider a minor readability improvement.The
formatYoutubeLinkmethod effectively handles different YouTube URL formats and standardizes them. It correctly extracts the video ID from bothyoutu.beandyoutube.comURLs.Consider extracting the domain check into a separate method for improved readability:
public static String formatYoutubeLink(String link) throws MalformedURLException, URISyntaxException { String videoId = null; URI uri = URI.create(link); - String domain = uri.getHost(); + videoId = extractVideoId(uri); - if ("youtu.be".equals(domain)) { - String path = uri.getPath(); - if (path != null && path.length() > 1) { - videoId = path.substring(1); // removing the leading "/" - } - } - else if ("www.youtube.com".equals(domain) || "youtube.com".equals(domain)) { - Map<String, String> queryParams = extractQueryParams(link, "v"); - videoId = queryParams.get("v"); - } if (videoId != null) { uri = new URI("https", "www.youtube.com", "/watch", "v=" + videoId, null); link = uri.toString(); } else { throw new MalformedURLException(link + " is not a valid youtube link!"); } return link; } +private static String extractVideoId(URI uri) { + String domain = uri.getHost(); + if ("youtu.be".equals(domain)) { + String path = uri.getPath(); + if (path != null && path.length() > 1) { + return path.substring(1); // removing the leading "/" + } + } + else if ("www.youtube.com".equals(domain) || "youtube.com".equals(domain)) { + Map<String, String> queryParams = extractQueryParams(uri.toString(), "v"); + return queryParams.get("v"); + } + return null; +}This change would make the main method more concise and easier to understand at a glance.
CLI/src/main/java/main/Drifty_CLI.java (2)
298-312: LGTM: Improved YouTube link processing with robust error handling.This new code block effectively addresses the PR objective of enhancing YouTube link handling. It includes:
- Proper formatting of YouTube links using the
formatYoutubeLinkmethod.- Comprehensive error handling for various exceptions.
- User-friendly interaction allowing continuation or quitting upon errors.
These changes align well with the requirements from issue #601 and improve the overall robustness of the application.
Consider enhancing the error message slightly for more clarity:
- messageBroker.msgLinkError("Failed to process the YouTube link: " + e.getMessage()); + messageBroker.msgLinkError("Failed to process the YouTube link. Error: " + e.getMessage());This minor change makes the error message more explicit about the nature of the failure.
Line range hint
679-684: LGTM: Robust URL normalization method added.The new
normalizeUrlmethod is a valuable addition that enhances URL handling in the application. Key benefits include:
- Normalizing the URL scheme to lowercase for consistency.
- Decoding the URL path, improving handling of encoded URLs.
- Removing trailing slashes (except for root "/"), adhering to common URL normalization practices.
- Comprehensive error handling for various exception types.
This method contributes significantly to the PR's objective of standardizing and improving URL processing.
Consider adding a null check at the beginning of the method for robustness:
private static String normalizeUrl(String urlString) { + if (urlString == null) { + return null; + } try { URI uri = new URI(urlString.trim()); // ... rest of the method } // ... exception handling }This addition would prevent potential
NullPointerExceptions if the method is called with a null argument.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CLI/src/main/java/main/Drifty_CLI.java (2 hunks)
- Core/src/main/java/utils/Utility.java (3 hunks)
🔇 Additional comments (2)
Core/src/main/java/utils/Utility.java (1)
862-949: Overall improvements in YouTube link handling and query parameter extraction.The changes introduced in this file significantly enhance the functionality for handling YouTube links and extracting query parameters. The new methods
formatYoutubeLink,extractQueryParams, andcontainswork together to provide a robust solution for standardizing YouTube URLs and parsing their parameters.Key improvements:
- Standardization of YouTube links to a consistent format.
- Flexible query parameter extraction with the ability to filter specific parameters.
- Better error handling for malformed URLs.
While the implementation is generally good, the suggested refactorings in the previous comments will further improve readability, efficiency, and robustness of the code.
These changes align well with the PR objectives of improving YouTube playlist detection and enhancing the overall URL processing capabilities of the application.
CLI/src/main/java/main/Drifty_CLI.java (1)
24-24: LGTM: New import for improved error handling.The addition of
java.net.MalformedURLExceptionimport is appropriate for handling potential URL-related errors, which aligns with the PR's objective of improving YouTube link processing.
| /** | ||
| * Helper method to check if an array contains a specific value. | ||
| * | ||
| * @param array The array to check. | ||
| * @param value The value to search for. | ||
| * @return True if the array contains the value, false otherwise. | ||
| */ | ||
| private static boolean contains(String[] array, String value) { | ||
| if (array == null) return false; | ||
| for (String item : array) { | ||
| if (item.equals(value)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using Java's built-in method for array contains check.
The contains method is correctly implemented, but we can improve its efficiency by using Java's built-in methods.
Consider replacing the method with this more concise and potentially more efficient version:
-private static boolean contains(String[] array, String value) {
- if (array == null) return false;
- for (String item : array) {
- if (item.equals(value)) {
- return true;
- }
- }
- return false;
-}
+private static boolean contains(String[] array, String value) {
+ return array != null && Arrays.asList(array).contains(value);
+}This change utilizes Java's Arrays.asList(array).contains(value) method, which may be more efficient, especially for larger arrays. It also maintains the null check for the input array.
📝 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.
| /** | |
| * Helper method to check if an array contains a specific value. | |
| * | |
| * @param array The array to check. | |
| * @param value The value to search for. | |
| * @return True if the array contains the value, false otherwise. | |
| */ | |
| private static boolean contains(String[] array, String value) { | |
| if (array == null) return false; | |
| for (String item : array) { | |
| if (item.equals(value)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| /** | |
| * Helper method to check if an array contains a specific value. | |
| * | |
| * @param array The array to check. | |
| * @param value The value to search for. | |
| * @return True if the array contains the value, false otherwise. | |
| */ | |
| private static boolean contains(String[] array, String value) { | |
| return array != null && Arrays.asList(array).contains(value); | |
| } |
| /** | ||
| * Extracts the specified query parameters from the given URL. If no parameter names are provided (null or empty), all parameters are returned. | ||
| * | ||
| * @param urlLink The URL string from which to extract parameters. | ||
| * @param paramNames The names of the query parameters to extract. If null or empty, all parameters will be returned. | ||
| * @return A map containing the query parameter names and their corresponding values. | ||
| */ | ||
| public static Map<String, String> extractQueryParams(String urlLink, String... paramNames) { | ||
| Map<String, String> paramMap = new HashMap<>(); | ||
|
|
||
| URL url = null; | ||
| try { | ||
| url = URI.create(urlLink).toURL(); | ||
| } catch (MalformedURLException e) { | ||
| msgBroker.msgLinkError("Connection to the link timed out! Please check your internet connection. " + e.getMessage()); | ||
| } | ||
| String query = url != null ? url.getQuery() : null; | ||
|
|
||
| // query is null or empty, return an empty map (no query parameters) | ||
| if (query == null || query.isEmpty()) { | ||
| return paramMap; | ||
| } | ||
|
|
||
| // splitting query string into individual parameters | ||
| String[] params = query.split("&"); | ||
|
|
||
| // check if specific parameters are requested or if all should be returned | ||
| boolean returnAllParams = (paramNames == null || paramNames.length == 0); | ||
|
|
||
| for (String param : params) { | ||
| String[] pair = param.split("="); | ||
| if (pair.length == 2) { | ||
| String paramName = pair[0]; | ||
| String paramValue = pair[1]; | ||
|
|
||
| // add parameter to the map if it's requested or if all parameters should be returned | ||
| if (returnAllParams || contains(paramNames, paramName)) { | ||
| paramMap.put(paramName, paramValue); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return paramMap; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and efficiency in extractQueryParams.
The method correctly extracts query parameters, but there are opportunities for improvement:
- Error handling can be enhanced by returning early if the URL is malformed.
- The method can be more efficient by avoiding unnecessary null checks after URL creation.
- Handling of parameters without values can be improved.
Consider applying these changes:
public static Map<String, String> extractQueryParams(String urlLink, String... paramNames) {
Map<String, String> paramMap = new HashMap<>();
- URL url = null;
try {
- url = URI.create(urlLink).toURL();
+ URL url = URI.create(urlLink).toURL();
+ String query = url.getQuery();
+
+ if (query == null || query.isEmpty()) {
+ return paramMap;
+ }
+
+ String[] params = query.split("&");
+ boolean returnAllParams = (paramNames == null || paramNames.length == 0);
+
+ for (String param : params) {
+ String[] pair = param.split("=", 2);
+ String paramName = pair[0];
+ String paramValue = pair.length > 1 ? pair[1] : "";
+
+ if (returnAllParams || contains(paramNames, paramName)) {
+ paramMap.put(paramName, paramValue);
+ }
+ }
} catch (MalformedURLException e) {
- msgBroker.msgLinkError("Connection to the link timed out! Please check your internet connection. " + e.getMessage());
+ msgBroker.msgLinkError("Malformed URL: " + e.getMessage());
}
- String query = url != null ? url.getQuery() : null;
-
- if (query == null || query.isEmpty()) {
- return paramMap;
- }
-
- String[] params = query.split("&");
- boolean returnAllParams = (paramNames == null || paramNames.length == 0);
-
- for (String param : params) {
- String[] pair = param.split("=");
- if (pair.length == 2) {
- String paramName = pair[0];
- String paramValue = pair[1];
-
- if (returnAllParams || contains(paramNames, paramName)) {
- paramMap.put(paramName, paramValue);
- }
- }
- }
return paramMap;
}These changes address the issues mentioned in the past review comment and improve the overall efficiency and robustness of the method.
📝 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.
| /** | |
| * Extracts the specified query parameters from the given URL. If no parameter names are provided (null or empty), all parameters are returned. | |
| * | |
| * @param urlLink The URL string from which to extract parameters. | |
| * @param paramNames The names of the query parameters to extract. If null or empty, all parameters will be returned. | |
| * @return A map containing the query parameter names and their corresponding values. | |
| */ | |
| public static Map<String, String> extractQueryParams(String urlLink, String... paramNames) { | |
| Map<String, String> paramMap = new HashMap<>(); | |
| URL url = null; | |
| try { | |
| url = URI.create(urlLink).toURL(); | |
| } catch (MalformedURLException e) { | |
| msgBroker.msgLinkError("Connection to the link timed out! Please check your internet connection. " + e.getMessage()); | |
| } | |
| String query = url != null ? url.getQuery() : null; | |
| // query is null or empty, return an empty map (no query parameters) | |
| if (query == null || query.isEmpty()) { | |
| return paramMap; | |
| } | |
| // splitting query string into individual parameters | |
| String[] params = query.split("&"); | |
| // check if specific parameters are requested or if all should be returned | |
| boolean returnAllParams = (paramNames == null || paramNames.length == 0); | |
| for (String param : params) { | |
| String[] pair = param.split("="); | |
| if (pair.length == 2) { | |
| String paramName = pair[0]; | |
| String paramValue = pair[1]; | |
| // add parameter to the map if it's requested or if all parameters should be returned | |
| if (returnAllParams || contains(paramNames, paramName)) { | |
| paramMap.put(paramName, paramValue); | |
| } | |
| } | |
| } | |
| return paramMap; | |
| } | |
| /** | |
| * Extracts the specified query parameters from the given URL. If no parameter names are provided (null or empty), all parameters are returned. | |
| * | |
| * @param urlLink The URL string from which to extract parameters. | |
| * @param paramNames The names of the query parameters to extract. If null or empty, all parameters will be returned. | |
| * @return A map containing the query parameter names and their corresponding values. | |
| */ | |
| public static Map<String, String> extractQueryParams(String urlLink, String... paramNames) { | |
| Map<String, String> paramMap = new HashMap<>(); | |
| try { | |
| URL url = URI.create(urlLink).toURL(); | |
| String query = url.getQuery(); | |
| if (query == null || query.isEmpty()) { | |
| return paramMap; | |
| } | |
| String[] params = query.split("&"); | |
| boolean returnAllParams = (paramNames == null || paramNames.length == 0); | |
| for (String param : params) { | |
| String[] pair = param.split("=", 2); | |
| String paramName = pair[0]; | |
| String paramValue = pair.length > 1 ? pair[1] : ""; | |
| if (returnAllParams || contains(paramNames, paramName)) { | |
| paramMap.put(paramName, paramValue); | |
| } | |
| } | |
| } catch (MalformedURLException e) { | |
| msgBroker.msgLinkError("Malformed URL: " + e.getMessage()); | |
| } | |
| return paramMap; | |
| } |
Fixes issue
Fixes #601
Changes proposed
youtu.beandyoutube.comdomains.youtu.beURLs to theyoutube.com/watch?v={videoID}format.Check List (Check all the applicable boxes)
Screenshots
Note to reviewers
This PR ensures that YouTube links with different domain structures (youtu.be and youtube.com) are correctly standardized and handled. It also includes enhanced error handling for cases where the video ID is missing (playlist URLs) or the URL is malformed.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes