Skip to content
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

Fix bug on download images based on omero-insight#264 #78

Merged
merged 11 commits into from
Jul 19, 2023
Merged

Fix bug on download images based on omero-insight#264 #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d1f3b01
Fix bug on download images based on omero-insight#264
Rdornier Jul 7, 2023
bf37f70
remove unused imports
Rdornier Jul 10, 2023
e59a04a
Remove gradle files
Rdornier Jul 10, 2023
b64ade5
Delete gradlew
Rdornier Jul 10, 2023
0373ea1
Delete gradle/wrapper directory
Rdornier Jul 10, 2023
aa5f22b
Delete gradlew.bat
Rdornier Jul 10, 2023
ebffc73
Merge branch 'download-bug' of https://github.com/Rdornier/omero-gate…
Rdornier Jul 10, 2023
ef10da9
remove commented lines
Rdornier Jul 10, 2023
9a4720b
Delete hs_err_pid13244.log
Rdornier Jul 10, 2023
648d907
Remove log files from git
Rdornier Jul 10, 2023
32ef773
Merge branch 'download-bug' of https://github.com/Rdornier/omero-gate…
Rdornier Jul 10, 2023
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
4 changes: 4 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@

# generated files
/build
/gradle
/src/generated/*
gradlew
*.bat
*.log

# Local configuration file (sdk path, etc)
local.properties
Expand Down
141 changes: 67 additions & 74 deletions src/main/java/omero/gateway/facility/TransferFacilityHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,13 @@

import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ExecutionException;

import omero.RType;
import omero.ServerError;
import omero.api.IQueryPrx;
import omero.api.RawFileStorePrx;
import omero.gateway.Gateway;
Expand Down Expand Up @@ -90,7 +89,6 @@ public class TransferFacilityHelper {
List<File> downloadImage(SecurityContext context, String targetPath,
long imageId) throws DSAccessException, DSOutOfServiceException {
List<File> files = new ArrayList<File>();

ImageData image = browse.findObject(context, ImageData.class, imageId,
true);

Expand Down Expand Up @@ -125,84 +123,79 @@ List<File> downloadImage(SecurityContext context, String targetPath,
throw new DSAccessException("Cannot retrieve original file", e);
}

Map<Boolean, Object> result = new HashMap<Boolean, Object>();
Copy link
Member

Choose a reason for hiding this comment

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

Do you need that Map? In the end only the downloaded list is returned. This map and the notDownloaded list isn't actuall used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially thought returning downloaded and notDownloaded (i.e. the result Map) but this would change the method definition (returns now a Map and not a list anymore) and may not be compatible with previous versions.

The reason I kept them is beacuse I'm wondering if there is any possibility to return also the information contained in notDownloaded. Do you think it is possible ?

Copy link
Member

Choose a reason for hiding this comment

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

I see. An option would be to add a second method for now which returns the map and mark the old one as deprecated. That would only be needed in the main TransferFacility class, not really in the helper. Looking at the code, I actually can't see a reason any more why I split the code into a helper class in the first place, could as well go into the TransferFacility class itself. Your thoughts @jburel ? (Btw, this part of the Java gateway is not used by Insight, but should be!)

Copy link
Member

Choose a reason for hiding this comment

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

But that could be done in a follow up PR anyway. Lets get this one in first :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An option would be to add a second method for now which returns the map

Yes, exactly. It would then be possible to know which files had not been downloaded.

But that could be done in a follow up PR anyway. Lets get this one in first :-)

Also true

if (CollectionUtils.isEmpty(filesets))
return files;
Iterator<?> i;
List<OriginalFile> values = new ArrayList<OriginalFile>();
List<File> downloaded = new ArrayList<File>();
List<String> notDownloaded = new ArrayList<String>();
result.put(Boolean.valueOf(true), downloaded);
result.put(Boolean.valueOf(false), notDownloaded);

if (image.isFSImage()) {
i = filesets.iterator();
Fileset set;
List<FilesetEntry> entries;
Iterator<FilesetEntry> j;
while (i.hasNext()) {
set = (Fileset) i.next();
entries = set.copyUsedFiles();
j = entries.iterator();
while (j.hasNext()) {
FilesetEntry fs = j.next();
values.add(fs.getOriginalFile());
for (Object tmp : filesets) {
Fileset fs = (Fileset) tmp;

String repoPath = fs.getTemplatePrefix().getValue();
for (FilesetEntry fse: fs.copyUsedFiles()) {
OriginalFile of = fse.getOriginalFile();
String ofDir = of.getPath().getValue().replace(repoPath, "");
File outDir = new File(targetPath+File.separator+ofDir);
outDir.mkdirs();
File saved = saveOriginalFile(context, of, outDir);
if (saved != null)
downloaded.add(saved);
else
notDownloaded.add(of.getName().getValue());
}
}
} else
values.addAll((List<OriginalFile>) filesets);

RawFileStorePrx store = null;
OriginalFile of;
long size;
FileOutputStream stream = null;
long offset = 0;
i = values.iterator();
File f = null;

while (i.hasNext()) {
of = (OriginalFile) i.next();

try {
store = gateway.getRawFileService(context);
store.setFileId(of.getId().getValue());

f = new File(targetPath, of.getName().getValue());
files.add(f);

stream = new FileOutputStream(f);
size = of.getSize().getValue();
try {
try {
for (offset = 0; (offset + INC) < size;) {
stream.write(store.read(offset, INC));
offset += INC;
}
} finally {
stream.write(store.read(offset, (int) (size - offset)));
stream.close();
}
} catch (Exception e) {
if (stream != null)
stream.close();
if (f != null) {
f.delete();
files.remove(f);
}
}
} catch (IOException e) {
if (f != null) {
f.delete();
files.remove(f);
}
throw new DSAccessException("Cannot create file in folderPath",
e);
} catch (Throwable t) {
throw new DSAccessException("ServerError on retrieveArchived",
t);
} finally {
try {
store.close();
} catch (ServerError e) {
}
}
else { //Prior to FS
for (Object tmp : filesets) {
OriginalFile of = (OriginalFile) tmp;
File outDir = new File(targetPath);
File saved = saveOriginalFile(context, of, outDir);
if (saved != null)
downloaded.add(saved);
else
notDownloaded.add(of.getName().getValue());
}
}

return files;
return downloaded;
}

/**
* Save an OriginalFile of into directory dir
* @param ctx The SecurityContext
* @param of The OriginalFile
* @param dir The output directory
* @return The File if the operation was successfull, null if it wasn't.
*/
private File saveOriginalFile(SecurityContext ctx, OriginalFile of, File dir) {
File out = new File(dir, of.getName().getValue());
if (out.exists()) {
return null;
}

try {
RawFileStorePrx store = gateway.getRawFileService(ctx);
store.setFileId(of.getId().getValue());

long size = of.getSize().getValue();
long offset = 0;
try (FileOutputStream stream = new FileOutputStream(out))
{
for (offset = 0; (offset+INC) < size;) {
stream.write(store.read(offset, INC));
offset += INC;
}
stream.write(store.read(offset, (int) (size-offset)));
}
} catch (Exception e) {

return null;
}
return out;
}

/**
Expand Down