Skip to content

Table display performance & persistent selection #4

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

Merged
kelseym merged 3 commits into from
Jun 4, 2026
Merged

Table display performance & persistent selection #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
66bc064
Fix for https://github.com/NrgXnat/batch-transfer-plugin/issues/2
kelseym Jun 4, 2026
f5d1fb1
Fixes https://github.com/NrgXnat/batch-transfer-plugin/issues/1
kelseym Jun 4, 2026
4262bbf
Fix for #3: reimport no longer fails with "Pipe closed" on large mult…
kelseym Jun 4, 2026
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
1 change: 1 addition & 0 deletions docs/ROADMAP.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ The Batch Transfer Plugin enables bulk data operations across XNAT projects. Use
## Planned

### Operations & Workflow
- Implement UX cohort building filters for large (>10k+) datasets where the select/deselect item workflow would be tedious
- Launch Advanced Search from destination project Action Menu
- Include all XNAT experiment data types
- Computed size estimate in the operation-detail panel — replace the static duplication warning with a real byte count for the current selection (e.g. "≈ 4.2 GB will be duplicated to *DestProject*"). Requires either a new XAPI endpoint that walks the archive, or per-row `data-size` attributes emitted by `XDATScreen_batch_transfer.java`, plus rollup logic in `batchTransfer.js`.
Expand Down
118 changes: 55 additions & 63 deletions src/main/java/org/nrg/xnat/turbine/modules/screens/XDATScreen_batch_transfer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

package org.nrg.xnat.turbine.modules.screens;

import org.nrg.xnatx.plugins.transfer.model.TransferMode;
import org.nrg.xnatx.plugins.transfer.model.Fields;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.StringUtils;
Expand All @@ -30,8 +29,6 @@
@SuppressWarnings("unused")
@Slf4j
public class XDATScreen_batch_transfer extends SecureScreen {
private final List<String> nonSharingProjects = new ArrayList<>();
private final List<String> nonCopyingProjects = new ArrayList<>();
private static final String FIELD_ID = "BATCH_TRANSFER_ID";

private static final String READABLE_SUBJECTS = "SELECT field_value,read_element,field,element_name,grp.tag, xdat_user_id\n" +
Expand Down Expand Up @@ -70,7 +67,8 @@ protected void doBuildTemplate(RunData data, Context context) throws Exception {
// (2) default — DisplaySearch in the session from a bulk action on a search results table.
String sourceProject = data.getParameters().getString("sourceProject");
String column;
List<String> itemIds;
List<String> itemIds = null; // search-results entry point only
String sourceProjectId = null; // sourceProject entry point only
DisplaySearch search = null;

if (StringUtils.isNotBlank(sourceProject)) {
Expand All @@ -80,15 +78,10 @@ protected void doBuildTemplate(RunData data, Context context) throws Exception {
context.put("msg", "Project not found or access denied.");
return;
}
String validatedProjectId = proj.getId();
sourceProjectId = proj.getId();
context.put("searchType", "subject");
context.put("projectContext", validatedProjectId);
context.put("projectContext", sourceProjectId);
column = "subj.id";
itemIds = getSubjectIdsInProject(validatedProjectId, user);
if (itemIds.isEmpty()) {
context.put("sharingMsg", "None of the requested data is available for sharing or cloning.");
return;
}
} else {
// retrieve passed search object
search = TurbineUtils.getSearch(data);
Expand Down Expand Up @@ -118,6 +111,12 @@ protected void doBuildTemplate(RunData data, Context context) throws Exception {
itemIds = table.convertColumnToArrayList(FIELD_ID);
}

boolean byProject = (sourceProjectId != null);
String safeProj = byProject ? sourceProjectId.replaceAll("[^A-Za-z0-9_\\-]", "") : "";

@kathrynalpert kathrynalpert Jun 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I realize you inherited this, but It'd be nicer to do this as a parameterized query

String ownedScope = byProject ? "WHERE subj.project='" + safeProj + "'\n" : "";
String sharedScope = byProject ? "WHERE pp.project='" + safeProj + "'\n" : "";
String finalFilter = byProject ? "" : ("WHERE " + column + " IN ('" + buildWhereClause(itemIds) + "')");

// Build the query to retrieve information about the items we want listed.
String query = "SELECT DISTINCT ON (SUBJ.ID, SADS.ID, IAD.ID) secondary_id AS project_label,subj.id AS subject_id, subj.label AS subject_label, subj.project AS subject_project, SADS.id AS session_id, SADS.label AS session_label, SADS.project AS session_project, SADS.date AS session_expt_date, SADS.element_name as session_element, IAD.id AS assessor_id, IAD.label AS assessor_label, IAD.project AS assessor_project, IAD.date AS assess_expt_date, IAD.element_name AS assessor_element\n" +
"FROM (\n" +
Expand All @@ -126,13 +125,15 @@ protected void doBuildTemplate(RunData data, Context context) throws Exception {
"JOIN (\n" +
READABLE_SUBJECTS +
"WHERE xdat_user_id={USER_ID} AND read_element=1 AND element_name='xnat:subjectData' AND field='xnat:subjectData/project') OWNED ON subj.project=OWNED.tag\n" +
ownedScope +
"UNION\n" +
"SELECT subj.id, pp.label, subj.project\n" +
"FROM xnat_subjectData subj\n" +
" JOIN xnat_projectParticipant pp ON subj.id=pp.subject_id\n" +
"JOIN (\n" +
READABLE_SUBJECTS +
"WHERE xdat_user_id={USER_ID} AND read_element=1 AND element_name='xnat:subjectData' AND field='xnat:subjectData/sharing/share/project' ) SHARED ON pp.project=SHARED.tag\n" +
sharedScope +
") SUBJ\n" +
"LEFT JOIN (\n" +
" SELECT expt.id, expt.label, expt.project, sad.subject_id, expt.date, xme.element_name\n" +
Expand Down Expand Up @@ -165,40 +166,49 @@ protected void doBuildTemplate(RunData data, Context context) throws Exception {
"JOIN xnat_imageAssessorData sad ON shared.id=sad.id\n" +
") IAD ON SADS.id=IAD.imagesession_id\n" +
"LEFT JOIN xnat_projectData ON subj.project=xnat_projectData.id " +
"WHERE " +
column +
" IN ('" +
buildWhereClause(itemIds) +
"');";
finalFilter +
";";

query = query.replace("{USER_ID}", ((XDATUser) user).getXdatUserId().toString());

// Execute the query to get a list of experiments
XFTTable experiments = XFTTable.Execute(query, user.getDBName(), user.getUsername());

// Insert the items into the context.
Map<String, ItemContainer> sharingItems = getItems(experiments, search, TransferMode.SHARE);

Map<String, ItemContainer> copyingItems = getItems(experiments, search, TransferMode.CLONE);

if (sharingItems.isEmpty() && copyingItems.isEmpty()) {
context.put("sharingMsg", "None of the requested data is available for sharing or cloning.");
// Build ONE item set (all readable items), then compute per-project eligibility once.
// Share/Clone are feature-gated; Reimport is always allowed, so the set is unfiltered.
Map<String, ItemContainer> items = buildItems(experiments, search);
if (items.isEmpty()) {
context.put("sharingMsg", "None of the requested data is available.");
return;
}

if (sharingItems.isEmpty()) {
context.put("sharingMsg", "There is no data available to share. Please try another operation to proceed.");
} else if (!nonSharingProjects.isEmpty()) {
context.put("sharingMsg", "Some of the data requested is not available for sharing and has been excluded from the table below.");
UserI details = XDAT.getUserDetails();
Set<String> shareableProjects = new HashSet<>();
Set<String> cloneableProjects = new HashSet<>();
for (String proj : items.keySet()) {
if (Features.checkRestrictedFeature(details, proj, Features.PROJECT_SHARING_FEATURE)) {
shareableProjects.add(proj);
}
if (Features.checkRestrictedFeature(details, proj, Fields.PROJECT_COPYING_FEATURE)) {
cloneableProjects.add(proj);
}
}

if (copyingItems.isEmpty()) {
context.put("copyingMsg", "There is no data available to clone. Please try another operation to proceed.");
} else if (!nonCopyingProjects.isEmpty()) {
context.put("copyingMsg", "Some of the data requested is not available for cloning and has been excluded from the table below.");
// Per-operation availability warnings, derived from eligibility (Reimport always has data).
if (shareableProjects.isEmpty()) {
context.put("sharingMsg", "No data is available to share. Try another operation to proceed.");
} else if (shareableProjects.size() < items.size()) {
context.put("sharingMsg", "Some data is not available for sharing and is hidden while Share is selected.");
}
if (cloneableProjects.isEmpty()) {
context.put("copyingMsg", "No data is available to clone. Try another operation to proceed.");
} else if (cloneableProjects.size() < items.size()) {
context.put("copyingMsg", "Some data is not available for cloning and is hidden while Clone is selected.");
}
context.put("sharingItems", sharingItems);
context.put("copyingItems", copyingItems);

context.put("items", items);
context.put("shareableProjects", shareableProjects);
context.put("cloneableProjects", cloneableProjects);
context.put("turbineUtils", TurbineUtils.GetInstance());
}

Expand All @@ -221,10 +231,12 @@ private void addShareIdDisplayField(SchemaElement rootElement, DisplaySearch sea
* @param search - DisplaySearch
* @return Hashtable<String, ItemContainer>
*/
private Map<String, ItemContainer> getItems(XFTTable t, DisplaySearch search, TransferMode operation) {
private Map<String, ItemContainer> buildItems(XFTTable t, DisplaySearch search) {
Map<String, ItemContainer> items = new HashMap<>();

// For each experiment in the hashtable.
// For each experiment row, build the project→subject→session→assessor tree. The set is
// unfiltered — per-project Share/Clone eligibility is computed once in doBuildTemplate,
// and Reimport is always allowed.
for(Hashtable exp : t.rowHashs()){

String project = (String) exp.get("subject_project");
Expand All @@ -233,18 +245,6 @@ private Map<String, ItemContainer> getItems(XFTTable t, DisplaySearch search, Tr
continue;
}

if (operation == TransferMode.SHARE) {
if(!Features.checkRestrictedFeature(XDAT.getUserDetails(), project, Features.PROJECT_SHARING_FEATURE)){
nonSharingProjects.add(project);
continue;
}
} else {
if(!Features.checkRestrictedFeature(XDAT.getUserDetails(), project, Fields.PROJECT_COPYING_FEATURE)){
nonCopyingProjects.add(project);
continue;
}
}

ItemContainer projectContainer = items.get(project);
if (projectContainer == null) {
projectContainer = new ItemContainer(project, (String) exp.get("project_label"),
Expand Down Expand Up @@ -297,22 +297,6 @@ private String buildWhereClause(List<String> ids){
return StringUtils.join(ids, "','");
}

/**
* Returns all subject IDs belonging to the given project, including subjects shared into it.
* Permission filtering is applied downstream by the main hierarchy query's READABLE_* joins.
* @param projectId - validated project id (already checked via XnatProjectdata.getProjectByIDorAlias)
* @param user - current user
* @return list of subject ids
*/
private List<String> getSubjectIdsInProject(String projectId, UserI user) throws Exception {
String safeProjectId = projectId.replaceAll("[^A-Za-z0-9_\\-]", "");
String query = "SELECT DISTINCT subj.id AS id FROM xnat_subjectData subj " +
"LEFT JOIN xnat_projectParticipant pp ON subj.id=pp.subject_id " +
"WHERE subj.project = '" + safeProjectId + "' OR pp.project = '" + safeProjectId + "';";
XFTTable table = XFTTable.Execute(query, user.getDBName(), user.getUsername());
return table.convertColumnToArrayList("id");
}

private static Map<String, ItemContainer> sortItemContainersByLabel(Map<String, ItemContainer> items) {
items.forEach((key, value) -> value.sortChildren());
return items.entrySet().stream()
Expand Down Expand Up @@ -362,5 +346,13 @@ public String getProject() {
public String getXsiType() {
return this.xsiType;
}

/**
* Reimport is session-only: only image sessions (xsiType ending in "SessionData") are
* reimportable — subjects and (image) assessors are not. Drives data-reimportable in the UI.
*/
public boolean isReimportable() {
return this.xsiType != null && this.xsiType.toLowerCase().endsWith("sessiondata");
}
}
}
113 changes: 91 additions & 22 deletions src/main/java/org/nrg/xnatx/plugins/transfer/service/impl/BatchTransferServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,17 +249,35 @@ private void importExperiment(XnatExperimentdata sourceExperiment, XnatProjectda
*/
protected List<String> runImporter(final UserI user, final FileWriterWrapperI wrapper, final Map<String, Object> params) throws Exception {
final Thread heartbeat = startImporterHeartbeat(wrapper.getName());
List<String> result = null;
Exception importerError = null;
try (DicomZipImporter importer = new DicomZipImporter(null, user, wrapper, params)) {
importer.setIdentifier(this.identifier);
return importer.call();
result = importer.call();
} catch (Exception e) {
importerError = e;
} finally {
heartbeat.interrupt();
if (wrapper instanceof StreamingZipFileWriter) {
final StreamingZipFileWriter s = (StreamingZipFileWriter) wrapper;
s.shutdown();
s.awaitProducer(30_000L);
try {
s.awaitProducer(30_000L);
} catch (IOException producerError) {
// The importer's own failure is the user-facing cause; a producer-side error
// (genuine source truncation) must never mask it — attach it as suppressed.
if (importerError == null) {
importerError = producerError;
} else {
importerError.addSuppressed(producerError);
}
}
}
}
if (importerError != null) {
throw importerError;
}
return result;
}

private static Thread startImporterHeartbeat(final String name) {
Expand Down Expand Up @@ -341,35 +359,86 @@ public InputStream getInputStream() throws IOException {
}

private void produce() {
try (ZipOutputStream zos = new ZipOutputStream(new BufferedOutputStream(pipedOut, 65536));
Stream<Path> paths = Files.walk(sourceDir)) {
zos.setLevel(Deflater.BEST_SPEED);
final byte[] buffer = new byte[65536];
final ZipOutputStream zos = new ZipOutputStream(new BufferedOutputStream(pipedOut, 65536));
zos.setLevel(Deflater.BEST_SPEED);
final byte[] buffer = new byte[65536];
try (Stream<Path> paths = Files.walk(sourceDir)) {
// writeEntry returns false once the consumer closes the pipe; allMatch then stops
// streaming. Real source-read failures throw and are caught (and recorded) below.
paths.filter(Files::isRegularFile)
.filter(p -> StreamSupport.stream(p.spliterator(), false)
.anyMatch(part -> part.toString().equals("DICOM")))
.filter(p -> !p.getFileName().toString().toLowerCase().endsWith("catalog.xml"))
.forEach(p -> writeEntry(zos, p, buffer));
} catch (Throwable t) {
// Unwrap UncheckedIOException for cleaner error messages.
final Throwable recorded = (t instanceof UncheckedIOException) ? t.getCause() : t;
error.set(recorded);
log.warn("Streaming zip producer for {} failed", displayName, recorded);
.allMatch(p -> writeEntry(zos, p, buffer));
} catch (UncheckedIOException e) {
recordError(e.getCause()); // a source DICOM file could not be read
} catch (IOException e) {
recordError(e); // the source tree could not be walked
} finally {
finishZip(zos);
}
}

private void writeEntry(final ZipOutputStream zos, final Path filePath, final byte[] buffer) {
try {
zos.putNextEntry(new ZipEntry(sourceDir.relativize(filePath).toString()));
try (InputStream in = Files.newInputStream(filePath)) {
int len;
while ((len = in.read(buffer)) > 0) {
zos.write(buffer, 0, len);
private void recordError(final Throwable cause) {
error.set(cause);
log.warn("Streaming zip producer for {} failed", displayName, cause);
}

/**
* Copy one source file into the zip, streaming through the shared fixed-size buffer.
* Returns {@code false} — not an error — when a write to the pipe fails: that means the
* consumer (importer) closed the read end after taking the DICOM it needed, so there is
* nothing left to stream. A failure to <em>read</em> the source file is a genuine producer
* error, rethrown as {@link UncheckedIOException} so it surfaces via {@link #awaitProducer(long)}.
*
* <p>Streaming in fixed-size chunks (which block and drain through the 64&nbsp;KB pipe)
* keeps producer memory constant regardless of slice size — a large multi-frame instance
* is never buffered whole.
*/
private boolean writeEntry(final ZipOutputStream zos, final Path filePath, final byte[] buffer) {
try (InputStream in = Files.newInputStream(filePath)) {
try {
zos.putNextEntry(new ZipEntry(sourceDir.relativize(filePath).toString()));
} catch (IOException consumerClosedPipe) {
return false;
}
while (true) {
final int len;
try {
len = in.read(buffer); // read from source
} catch (IOException sourceReadError) {
throw new UncheckedIOException(sourceReadError);
}
if (len <= 0) {
break;
}
try {
zos.write(buffer, 0, len); // write to the pipe (sink)
} catch (IOException consumerClosedPipe) {
return false;
}
}
zos.closeEntry();
} catch (IOException e) {
throw new UncheckedIOException(e);
try {
zos.closeEntry();
} catch (IOException consumerClosedPipe) {
return false;
}
return true;
} catch (IOException sourceOpenError) {
throw new UncheckedIOException(sourceOpenError);
}
}

/**
* Flush and close the zip, writing its central directory. The consumer never reads past
* the last entry, so for a large session this final write can fail once the consumer has
* closed the pipe — that is the normal end of streaming, not an error.
*/
private void finishZip(final ZipOutputStream zos) {
try {
zos.close();
} catch (IOException consumerClosedPipe) {
log.debug("Streaming zip producer for {}: consumer closed before the zip finished (benign)", displayName);
}
}

Expand Down
Loading
Loading