Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2168 +/- ##
============================================
- Coverage 15.33% 15.32% -0.02%
Complexity 391 391
============================================
Files 233 233
Lines 6148 6153 +5
Branches 710 711 +1
============================================
Hits 943 943
- Misses 5154 5159 +5
Partials 51 51 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jo-elimu
requested review from
AshishBagdane,
shiv810 and
vuriaval
and removed request for
a team
WalkthroughThis update removes the storage of raw video file bytes from the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VideoCreateController
participant VideoDao
participant GitHubLFS
User->>VideoCreateController: Uploads video file
VideoCreateController->>VideoDao: readByChecksumMd5(checksum)
alt Video with checksum exists
VideoDao-->>VideoCreateController: Existing Video (with GitHub checksum)
VideoCreateController->>VideoDao: Save new Video referencing existing GitHub checksum
else No existing video
VideoCreateController->>GitHubLFS: Upload video bytes
GitHubLFS-->>VideoCreateController: Returns GitHub checksum
VideoCreateController->>VideoDao: Save new Video with new GitHub checksum
end
Assessment against linked issues
Possibly related PRs
✨ Finishing Touches
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. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
src/test/java/selenium/content/video/VideoCreatePage.java (1)
1-15: Page object implementation is appropriateThe VideoCreatePage follows the Page Object pattern correctly, which helps maintain clean separation between test logic and page interactions. The validation in the constructor ensures the driver is on the correct page.
Consider enhancing this page object with methods for interacting with form elements and submitting data to make it more useful in tests.
package selenium.content.video; import org.openqa.selenium.By; import org.openqa.selenium.WebDriver; +import org.openqa.selenium.WebElement; +import java.io.File; public class VideoCreatePage { private WebDriver driver; public VideoCreatePage(WebDriver driver) { this.driver = driver; driver.findElement(By.id("videoCreatePage")); } + + public void setTitle(String title) { + WebElement titleField = driver.findElement(By.id("title")); + titleField.clear(); + titleField.sendKeys(title); + } + + public void selectVideoFile(File videoFile) { + WebElement fileInput = driver.findElement(By.id("videoFile")); + fileInput.sendKeys(videoFile.getAbsolutePath()); + } + + public void submit() { + driver.findElement(By.id("submitButton")).click(); + } }src/test/java/selenium/content/video/VideoEditPage.java (1)
1-15: Page object implementation is appropriateThe VideoEditPage follows the Page Object pattern correctly, which helps maintain clean separation between test logic and page interactions. The validation in the constructor ensures the driver is on the correct page.
Similar to the VideoCreatePage, consider enhancing this page object with methods for interacting with form elements and submitting changes to make it more useful in tests.
package selenium.content.video; import org.openqa.selenium.By; import org.openqa.selenium.WebDriver; +import org.openqa.selenium.WebElement; +import java.io.File; public class VideoEditPage { private WebDriver driver; public VideoEditPage(WebDriver driver) { this.driver = driver; driver.findElement(By.id("videoEditPage")); } + + public void setTitle(String title) { + WebElement titleField = driver.findElement(By.id("title")); + titleField.clear(); + titleField.sendKeys(title); + } + + public void updateVideoFile(File videoFile) { + WebElement fileInput = driver.findElement(By.id("videoFile")); + fileInput.sendKeys(videoFile.getAbsolutePath()); + } + + public void submit() { + driver.findElement(By.id("submitButton")).click(); + } }src/test/java/ai/elimu/rest/v2/content/VideosRestControllerTest.java (1)
16-27: Good test coverage for the videos REST endpoint.This test validates that the
/content/videosendpoint returns proper JSON data with videos that have titles. However, it could be enhanced to verify more fields, especially the newfileSizeproperty.Consider adding assertions for the fileSize field to ensure it's properly included in the REST response:
JSONObject videoJsonObject = videosJSONArray.getJSONObject(0); assertNotNull(videoJsonObject.getString("title")); +assertNotNull(videoJsonObject.getInt("fileSize"));src/main/java/ai/elimu/web/servlet/CustomDispatcherServlet.java (1)
277-277: Consider applying the same pattern for thumbnail as for video content.While the video content is no longer stored in the database, the thumbnail is still loaded as a byte array and stored directly. For consistency, consider handling the thumbnail the same way as video content in the future.
Since the thumbnail field is already marked as deprecated in the Video entity, you might want to consider storing thumbnail references externally as well and just keeping metadata in the database.
src/main/java/ai/elimu/web/content/multimedia/video/VideoEditController.java (1)
121-123: Consider stronger hashing algorithm for future-proof deduplicationMD5 is fast but collision-prone; for binary artefacts SHA-256 (or at least SHA-1) is the contemporary baseline.
If changing the column type is expensive now, at least add a TODO for migrating to a stronger checksum to avoid deliberate hash-collision attacks on the LFS store.
[security]src/test/java/selenium/content/video/VideoTest.java (3)
19-35: Add implicit wait & Chrome options for stability in CIHard navigation without waits is brittle; slow environments may fail sporadically.
In addition, enabling--no-sandbox/--disable-gpuis often required in headless CI.ChromeOptions chromeOptions = new ChromeOptions(); +chromeOptions.addArguments("--no-sandbox", "--disable-gpu"); ... driver = new ChromeDriver(chromeOptions); -driver.get(DomainHelper.getBaseUrl() + "/content"); +driver.manage().timeouts().implicitlyWait(Duration.ofSeconds(5)); +driver.get(DomainHelper.getBaseUrl() + "/content");This small tweak greatly improves reliability of the UI tests.
45-57: Assert page state to give the test a purposeCurrently the test only navigates; it never verifies that the target edit page actually loaded, so a broken page could pass unnoticed.
VideoEditPage videoEditPage = new VideoEditPage(driver); assertTrue(videoEditPage.isAt(), "Video Edit page did not load correctly");Expose an
isAt()helper in each page object to assert on a unique element (id="videoEditPage").
59-70: Same assertion missing for creation flowMirror the previous suggestion for the create page to ensure both navigation paths are validated.
VideoCreatePage videoCreatePage = new VideoCreatePage(driver); assertTrue(videoCreatePage.isAt(), "Video Create page did not load correctly");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/resources/ai/elimu/web/servlet/placeholder.mp4is excluded by!**/*.mp4,!**/*.mp4
📒 Files selected for processing (22)
pom-dependency-tree.txt(1 hunks)src/main/java/ai/elimu/dao/VideoDao.java(1 hunks)src/main/java/ai/elimu/dao/jpa/VideoDaoJpa.java(1 hunks)src/main/java/ai/elimu/entity/content/multimedia/Video.java(1 hunks)src/main/java/ai/elimu/rest/v2/JpaToGsonConverter.java(1 hunks)src/main/java/ai/elimu/web/content/multimedia/video/VideoCreateController.java(3 hunks)src/main/java/ai/elimu/web/content/multimedia/video/VideoCsvExportController.java(2 hunks)src/main/java/ai/elimu/web/content/multimedia/video/VideoEditController.java(3 hunks)src/main/java/ai/elimu/web/download/VideoController.java(0 hunks)src/main/java/ai/elimu/web/servlet/CustomDispatcherServlet.java(3 hunks)src/main/resources/META-INF/jpa-schema-export.sql(0 hunks)src/main/resources/db/migration/2005109.sql(1 hunks)src/main/webapp/WEB-INF/jsp/content/main.jsp(1 hunks)src/main/webapp/WEB-INF/jsp/content/multimedia/video/create.jsp(1 hunks)src/main/webapp/WEB-INF/jsp/content/multimedia/video/edit.jsp(2 hunks)src/main/webapp/WEB-INF/jsp/content/multimedia/video/list.jsp(1 hunks)src/test/java/ai/elimu/rest/v2/content/VideosRestControllerTest.java(1 hunks)src/test/java/selenium/content/MainContentPage.java(1 hunks)src/test/java/selenium/content/video/VideoCreatePage.java(1 hunks)src/test/java/selenium/content/video/VideoEditPage.java(1 hunks)src/test/java/selenium/content/video/VideoListPage.java(1 hunks)src/test/java/selenium/content/video/VideoTest.java(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/resources/META-INF/jpa-schema-export.sql
- src/main/java/ai/elimu/web/download/VideoController.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/ai/elimu/rest/v2/content/VideosRestControllerTest.java (1)
src/test/java/selenium/content/video/VideoTest.java (1)
Slf4j(14-71)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_ui
- GitHub Check: test_ui
- GitHub Check: test_ui
- GitHub Check: test_ui
- GitHub Check: test_ui
- GitHub Check: test_rest
🔇 Additional comments (19)
pom-dependency-tree.txt (1)
1-1: Artifact version bumped correctly
The project’s main artifact version has been updated from2.5.108-SNAPSHOTto2.5.109-SNAPSHOT, which matches the coordinated schema and code changes in this PR.src/main/webapp/WEB-INF/jsp/content/main.jsp (1)
120-120: LGTM! ID attribute added consistently with other content links.The addition of the "videoListLink" ID attribute follows the same pattern used for other content links in this file, enabling consistent UI test automation.
src/main/webapp/WEB-INF/jsp/content/multimedia/video/list.jsp (1)
74-74: LGTM! ID attribute added to support UI test automation.Adding the "createButton" ID to the video creation link enables reliable interaction in Selenium tests, improving test automation capabilities without affecting functionality.
src/test/java/selenium/content/MainContentPage.java (1)
57-60: LGTM! Method added consistently with existing patterns.The
pressVideoListLink()method follows the same implementation pattern as other navigation methods in this class, maintaining code consistency while enabling video list page testing.src/main/resources/db/migration/2005109.sql (1)
1-3: LGTM! Database migration correctly removes the bytes column.This migration script properly removes the
bytescolumn from theVideotable, aligning with the architectural change to stop storing raw video content in the database and use file size metadata instead.src/main/java/ai/elimu/dao/VideoDao.java (1)
13-13: Good addition of a method to fetch videos by checksum.Adding the
readByChecksumMd5method is a good approach for supporting deduplication of videos with identical content, which aligns with the PR's objective of optimizing video storage.src/main/webapp/WEB-INF/jsp/content/multimedia/video/edit.jsp (2)
92-92: Appropriate change to file upload field.Changing from Spring form tag to a plain HTML input correctly aligns with the removal of the
bytesfield from the Video entity. This approach decouples the UI from the old data model that directly stored binary content.
361-364: Good addition of file size display in the UI.Adding the file size display provides useful metadata to users and aligns with the new storage approach where file size is an explicit property rather than derived from the binary content.
src/main/java/ai/elimu/web/content/multimedia/video/VideoCsvExportController.java (1)
30-30: CSV export properly updated to include file size.The CSV export functionality has been properly updated to include the file size as a column, maintaining consistency with the entity model changes.
Also applies to: 41-41
src/main/webapp/WEB-INF/jsp/content/multimedia/video/create.jsp (1)
84-84: Appropriate change to file upload field in creation form.Similar to the edit page, changing from Spring form tag to a plain HTML input correctly aligns with the removal of the
bytesfield from the Video entity in the creation workflow.src/main/java/ai/elimu/rest/v2/JpaToGsonConverter.java (1)
357-357: Implementation looks goodThe change to use
video.getFileSize()directly aligns with the architectural change of not storing raw video bytes in the database, but rather storing file size metadata. This approach is more efficient for handling large media files.src/main/java/ai/elimu/dao/jpa/VideoDaoJpa.java (1)
27-39: Well-implemented DAO methodThe
readByChecksumMd5method is correctly implemented following the same pattern as other methods in the class. It properly handles the case when no result is found by catching theNoResultException.This method will help prevent duplicate uploads by allowing the system to check if a video with the same content (identified by MD5 checksum) already exists in the database.
src/main/java/ai/elimu/entity/content/multimedia/Video.java (3)
42-42: Note the deprecated thumbnail field.The
thumbnailfield is now marked as@Deprecated, suggesting that it will be removed in a future release. This is consistent with the overall approach of removing direct storage of binary data from the entity.Consider adding a deprecation message to explain why it's deprecated and what alternatives should be used instead:
- @Deprecated + @Deprecated(since = "2.0", forRemoval = true)
26-27: The fileSize field properly replaces bytes storage.The
fileSizefield with@NotNullconstraint effectively replaces the need for storing the actual video bytes while still maintaining information about the video's size.
52-57: URL construction logic is working appropriately.The
getUrl()method correctly constructs the URL to fetch the video from GitHub LFS using the MD5 checksum and video format. This approach is efficient and aligns with the removal of direct byte storage.src/main/java/ai/elimu/web/content/multimedia/video/VideoCreateController.java (3)
72-74: Improved byte array declaration for better scope management.Moving the byte array declaration outside the try block ensures it remains accessible throughout the method, which is a good practice for variable scoping.
93-95: Properly sets video metadata instead of storing raw bytes.The code now sets the
fileSizeproperty and MD5 checksum instead of storing the raw video bytes in the entity. This approach is more efficient for database storage.
126-133: Excellent optimization to avoid duplicate uploads.This new block of code checks if a video with the same MD5 checksum already exists in the database before uploading it to GitHub LFS. If found, it reuses the existing GitHub checksum, preventing redundant uploads and saving storage space and network bandwidth.
src/main/java/ai/elimu/web/servlet/CustomDispatcherServlet.java (1)
266-283: Properly added placeholder video initialization for development environment.The code correctly initializes a placeholder video in the development environment, setting all required fields including the file size and checksum without storing the raw bytes. This is consistent with the new approach of not storing video bytes directly in the database.
| Video existingVideoWithSameFileContent = videoDao.readByChecksumMd5(video.getChecksumMd5()); | ||
| if (existingVideoWithSameFileContent != null) { | ||
| // Re-use existing file | ||
| video.setChecksumGitHub(existingVideoWithSameFileContent.getChecksumGitHub()); | ||
| } else { | ||
| String checksumGitHub = GitHubLfsHelper.uploadVideoToLfs(video, bytes); | ||
| video.setChecksumGitHub(checksumGitHub); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
bytes may be null here, leading to NPE inside GitHubLfsHelper.uploadVideoToLfs
Even after the earlier null-protection, there is still a corner case: editing a video’s metadata without uploading a new file (multipartFile.isEmpty() == true).
In that flow bytes is never assigned and the deduplication branch falls through to uploadVideoToLfs, causing a crash.
Add an explicit guard:
if (existingVideoWithSameFileContent != null) {
// Re-use existing file
video.setChecksumGitHub(existingVideoWithSameFileContent.getChecksumGitHub());
-} else {
+} else if (bytes != null) {
String checksumGitHub = GitHubLfsHelper.uploadVideoToLfs(video, bytes);
video.setChecksumGitHub(checksumGitHub);
+} else {
+ // No new file supplied; keep the current GitHub checksum unchanged
}This allows metadata-only edits while preserving the existing file reference.
📝 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.
| Video existingVideoWithSameFileContent = videoDao.readByChecksumMd5(video.getChecksumMd5()); | |
| if (existingVideoWithSameFileContent != null) { | |
| // Re-use existing file | |
| video.setChecksumGitHub(existingVideoWithSameFileContent.getChecksumGitHub()); | |
| } else { | |
| String checksumGitHub = GitHubLfsHelper.uploadVideoToLfs(video, bytes); | |
| video.setChecksumGitHub(checksumGitHub); | |
| } | |
| Video existingVideoWithSameFileContent = videoDao.readByChecksumMd5(video.getChecksumMd5()); | |
| if (existingVideoWithSameFileContent != null) { | |
| // Re-use existing file | |
| video.setChecksumGitHub(existingVideoWithSameFileContent.getChecksumGitHub()); | |
| } else if (bytes != null) { | |
| String checksumGitHub = GitHubLfsHelper.uploadVideoToLfs(video, bytes); | |
| video.setChecksumGitHub(checksumGitHub); | |
| } else { | |
| // No new file supplied; keep the current GitHub checksum unchanged | |
| } |
| byte[] bytes = null; | ||
| try { | ||
| byte[] bytes = multipartFile.getBytes(); | ||
| bytes = multipartFile.getBytes(); | ||
| if (multipartFile.isEmpty() || (bytes == null) || (bytes.length == 0)) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against bytes == null before further processing
MultipartFile#getBytes() can throw IOException, in which case bytes remains null.
Because the catch-block only logs the exception, execution continues and bytes is later dereferenced (e.g. when calculating file size or uploading to GitHub LFS), which will trigger an NPE.
byte[] bytes = null;
try {
bytes = multipartFile.getBytes();
+} catch (IOException e) {
+ log.error("Unable to read uploaded bytes", e);
+ result.rejectValue("bytes", "NotReadable");
+}
+
+if (bytes == null) {
+ // Bail out early – the rest of the logic depends on non-null bytes
+ model.addAttribute("video", video);
+ ...
+ return "content/multimedia/video/edit";
}
-if (multipartFile.isEmpty() || (bytes == null) || (bytes.length == 0)) {
+if (multipartFile.isEmpty() || (bytes.length == 0)) {
result.rejectValue("bytes", "NotNull");This ensures we never reach the later upload step with a null payload.
📝 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.
| byte[] bytes = null; | |
| try { | |
| byte[] bytes = multipartFile.getBytes(); | |
| bytes = multipartFile.getBytes(); | |
| if (multipartFile.isEmpty() || (bytes == null) || (bytes.length == 0)) { | |
| byte[] bytes = null; | |
| try { | |
| bytes = multipartFile.getBytes(); | |
| } catch (IOException e) { | |
| log.error("Unable to read uploaded bytes", e); | |
| result.rejectValue("bytes", "NotReadable"); | |
| } | |
| if (bytes == null) { | |
| // Bail out early – the rest of the logic depends on non-null bytes | |
| model.addAttribute("video", video); | |
| // ... any other attributes you need to repopulate ... | |
| return "content/multimedia/video/edit"; | |
| } | |
| if (multipartFile.isEmpty() || (bytes.length == 0)) { | |
| result.rejectValue("bytes", "NotNull"); | |
| } |
| public void pressRandomVideo() { | ||
| List<WebElement> links = driver.findElements(By.className("editLink")); | ||
| int randomIndex = (int) (Math.random() * links.size()); | ||
| WebElement randomLink = links.get(randomIndex); | ||
| randomLink.click(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle empty video list gracefully
links.size() can be 0, in which case Math.random() * 0 yields NaN and get(int) throws IndexOutOfBoundsException, breaking the test suite when the database has no videos.
public void pressRandomVideo() {
List<WebElement> links = driver.findElements(By.className("editLink"));
- int randomIndex = (int) (Math.random() * links.size());
- WebElement randomLink = links.get(randomIndex);
- randomLink.click();
+ if (links.isEmpty()) {
+ throw new IllegalStateException("No videos present on the page");
+ }
+ int randomIndex = ThreadLocalRandom.current().nextInt(links.size());
+ links.get(randomIndex).click();
}Also switched to ThreadLocalRandom for better statistical properties.
📝 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 void pressRandomVideo() { | |
| List<WebElement> links = driver.findElements(By.className("editLink")); | |
| int randomIndex = (int) (Math.random() * links.size()); | |
| WebElement randomLink = links.get(randomIndex); | |
| randomLink.click(); | |
| } | |
| public void pressRandomVideo() { | |
| List<WebElement> links = driver.findElements(By.className("editLink")); | |
| + if (links.isEmpty()) { | |
| + throw new IllegalStateException("No videos present on the page"); | |
| + } | |
| + int randomIndex = ThreadLocalRandom.current().nextInt(links.size()); | |
| + links.get(randomIndex).click(); | |
| } |
1 participant
Issue Number
Purpose
Technical Details
Testing Instructions
Screenshots
Format Checks
Note
Files in PRs are automatically checked for format violations with
mvn spotless:check.If this PR contains files with format violations, run
mvn spotless:applyto fix them.