Skip to content

UFAL/Fetching preview item data only when necessary #1007

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.

Sign up for GitHub

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

Open
jr-rk wants to merge 1 commit into
base: dtq-dev
Choose a base branch
from
Open

UFAL/Fetching preview item data only when necessary #1007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dspace-api/src/main/java/org/dspace/content/PreviewContentServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ public PreviewContent createPreviewContent(Context context, Bitstream bitstream,
@Override
public FileInfo createFileInfo(PreviewContent pc) {
Hashtable<String, FileInfo> sub = createSubMap(pc.sub, this::createFileInfo);
return new FileInfo(pc.name, pc.content, pc.size, pc.isDirectory, sub);
return new FileInfo(pc.name, pc.content, pc.size, pc.isDirectory);
}
Comment on lines 202 to 205
Copy link

@coderabbitai coderabbitai bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Missing assignment of sub map to FileInfo object.

The sub map is created on line 203 but never assigned to the FileInfo object. This will break the hierarchical structure of file previews as the sub-directory/file relationships will be lost.

 public FileInfo createFileInfo(PreviewContent pc) {
     Hashtable<String, FileInfo> sub = createSubMap(pc.sub, this::createFileInfo);
-    return new FileInfo(pc.name, pc.content, pc.size, pc.isDirectory);
+    FileInfo fileInfo = new FileInfo(pc.name, pc.content, pc.size, pc.isDirectory);
+    fileInfo.setSub(sub);
+    return fileInfo;
 }
📝 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.

Suggested change
public FileInfo createFileInfo(PreviewContent pc) {
Hashtable<String, FileInfo> sub = createSubMap(pc.sub, this::createFileInfo);
return new FileInfo(pc.name, pc.content, pc.size, pc.isDirectory, sub);
return new FileInfo(pc.name, pc.content, pc.size, pc.isDirectory);
}
public FileInfo createFileInfo(PreviewContent pc) {
Hashtable<String, FileInfo> sub = createSubMap(pc.sub, this::createFileInfo);
FileInfo fileInfo = new FileInfo(pc.name, pc.content, pc.size, pc.isDirectory);
fileInfo.setSub(sub);
return fileInfo;
}
🤖 Prompt for AI Agents 
In dspace-api/src/main/java/org/dspace/content/PreviewContentServiceImpl.java
around lines 202 to 205, the sub map created by createSubMap is not assigned to
the FileInfo object, causing loss of hierarchical structure. Modify the code to
assign the sub map to the FileInfo instance, either by passing it to the
constructor if supported or by setting it via a setter method after creating the
FileInfo object.


@Override
Expand Down
7 changes: 5 additions & 2 deletions dspace-api/src/main/java/org/dspace/util/FileInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,11 @@ public class FileInfo {

public Hashtable<String, FileInfo> sub = null;

public FileInfo(String name, String content, String size, boolean isDirectory, Hashtable<String, FileInfo> sub) {
public FileInfo(String name, String content, String size, boolean isDirectory) {
this.name = name;
this.content = content;
this.size = size;
this.isDirectory = isDirectory;
this.sub = sub;
}

public FileInfo(String name) {
Expand All @@ -45,4 +44,8 @@ public FileInfo(String name, String size) {
this.size = size;
isDirectory = false;
}

public void setSub(Hashtable<String, FileInfo> sub) {
this.sub = sub;
}
Comment on lines +48 to +50
Copy link

@coderabbitai coderabbitai bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation to the setter method.

The setter method lacks validation and could potentially cause issues if called with invalid data.

 public void setSub(Hashtable<String, FileInfo> sub) {
+    if (sub != null && !isDirectory) {
+        throw new IllegalStateException("Cannot set sub for non-directory FileInfo");
+    }
     this.sub = sub;
 }
📝 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.

Suggested change
public void setSub(Hashtable<String, FileInfo> sub) {
this.sub = sub;
}
public void setSub(Hashtable<String, FileInfo> sub) {
if (sub != null && !isDirectory) {
throw new IllegalStateException("Cannot set sub for non-directory FileInfo");
}
this.sub = sub;
}
🤖 Prompt for AI Agents 
In dspace-api/src/main/java/org/dspace/util/FileInfo.java around lines 48 to 50,
the setSub setter method currently assigns the input parameter directly without
validation. Add validation to check if the provided Hashtable<String, FileInfo>
sub is not null before assigning it to this.sub. If the input is invalid (e.g.,
null), handle it appropriately by either throwing an IllegalArgumentException or
ignoring the assignment to prevent potential issues.

}