-
-
Notifications
You must be signed in to change notification settings - Fork 71
delete bytes from video #2168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
delete bytes from video #2168
Changes from all commits
78f97cb
97ded36
3281db9
bd4eab5
0b33043
9c5804d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -65,14 +65,6 @@ public String handleRequest( | |||||||||||||||||||||||||||||||||||||
| log.info("handleRequest"); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Video video = videoDao.read(id); | ||||||||||||||||||||||||||||||||||||||
| if (StringUtils.isBlank(video.getChecksumGitHub())) { | ||||||||||||||||||||||||||||||||||||||
| String checksumGitHub = GitHubLfsHelper.uploadVideoToLfs(video, video.getBytes()); | ||||||||||||||||||||||||||||||||||||||
| video.setChecksumGitHub(checksumGitHub); | ||||||||||||||||||||||||||||||||||||||
| video.setRevisionNumber(video.getRevisionNumber() + 1); | ||||||||||||||||||||||||||||||||||||||
| videoDao.update(video); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // TODO: https://github.com/elimu-ai/webapp/issues/1545 | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| model.addAttribute("video", video); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| model.addAttribute("contentLicenses", ContentLicense.values()); | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -105,8 +97,9 @@ public String handleSubmit( | |||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| byte[] bytes = null; | ||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||
| byte[] bytes = multipartFile.getBytes(); | ||||||||||||||||||||||||||||||||||||||
| bytes = multipartFile.getBytes(); | ||||||||||||||||||||||||||||||||||||||
| if (multipartFile.isEmpty() || (bytes == null) || (bytes.length == 0)) { | ||||||||||||||||||||||||||||||||||||||
| result.rejectValue("bytes", "NotNull"); | ||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -125,9 +118,8 @@ public String handleSubmit( | |||||||||||||||||||||||||||||||||||||
| log.info("contentType: " + contentType); | ||||||||||||||||||||||||||||||||||||||
| video.setContentType(contentType); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| video.setBytes(bytes); | ||||||||||||||||||||||||||||||||||||||
| video.setFileSize(bytes.length); | ||||||||||||||||||||||||||||||||||||||
| video.setChecksumMd5(ChecksumHelper.calculateMD5(bytes)); | ||||||||||||||||||||||||||||||||||||||
| // TODO: https://github.com/elimu-ai/webapp/issues/2137 | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // TODO: convert to a default video format? | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -150,6 +142,14 @@ public String handleSubmit( | |||||||||||||||||||||||||||||||||||||
| video.setTitle(video.getTitle().toLowerCase()); | ||||||||||||||||||||||||||||||||||||||
| video.setTimeLastUpdate(Calendar.getInstance()); | ||||||||||||||||||||||||||||||||||||||
| video.setRevisionNumber(video.getRevisionNumber() + 1); | ||||||||||||||||||||||||||||||||||||||
| 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); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+145
to
+152
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion
Even after the earlier null-protection, there is still a corner case: editing a video’s metadata without uploading a new file ( 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||
| videoDao.update(video); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // TODO: https://github.com/elimu-ai/webapp/issues/1545 | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # 2.5.109 | ||
|
|
||
| ALTER TABLE `Video` DROP COLUMN `bytes`; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package ai.elimu.rest.v2.content; | ||
|
|
||
| import ai.elimu.util.JsonLoader; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import selenium.util.DomainHelper; | ||
| import org.json.JSONArray; | ||
| import org.json.JSONObject; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
|
|
||
| @Slf4j | ||
| public class VideosRestControllerTest { | ||
|
|
||
| @Test | ||
| public void testHandleGetRequest() { | ||
| String jsonResponse = JsonLoader.loadJson(DomainHelper.getRestUrlV2() + "/content/videos"); | ||
| log.info("jsonResponse: " + jsonResponse); | ||
|
|
||
| JSONArray videosJSONArray = new JSONArray(jsonResponse); | ||
| log.info("videosJSONArray.length(): " + videosJSONArray.length()); | ||
| assertFalse(videosJSONArray.isEmpty()); | ||
|
|
||
| JSONObject videoJsonObject = videosJSONArray.getJSONObject(0); | ||
| assertNotNull(videoJsonObject.getString("title")); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package selenium.content.video; | ||
|
|
||
| import org.openqa.selenium.By; | ||
| import org.openqa.selenium.WebDriver; | ||
|
|
||
| public class VideoCreatePage { | ||
|
|
||
| private WebDriver driver; | ||
|
|
||
| public VideoCreatePage(WebDriver driver) { | ||
| this.driver = driver; | ||
|
|
||
| driver.findElement(By.id("videoCreatePage")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Guard against
bytes == nullbefore further processingMultipartFile#getBytes()can throwIOException, in which casebytesremainsnull.Because the catch-block only logs the exception, execution continues and
bytesis 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
nullpayload.📝 Committable suggestion