Skip to content

Fix performance when checking dataset version file count #12494

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
landreev merged 10 commits into from
Jul 1, 2026
+61 −31
Merged

Fix performance when checking dataset version file count #12494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
78c62b4
Fix performance when checking dataset version file count
stevenwinship Jun 29, 2026
1a19e93
fix guest user check
stevenwinship Jun 29, 2026
16765bb
cache hasFiles in perm wrapper
stevenwinship Jun 30, 2026
9c13f0a
cache hasFiles in perm wrapper
stevenwinship Jun 30, 2026
fd99707
remove extra calls to get dataset in SearchIncludeFragment
stevenwinship Jun 30, 2026
264c290
remove extra calls to get dataset in SearchIncludeFragment
stevenwinship Jun 30, 2026
d3d1414
null entity fix
stevenwinship Jun 30, 2026
0fe658d
null entity fix
stevenwinship Jun 30, 2026
594d637
refactor
stevenwinship Jun 30, 2026
9fac0bb
another refactor
stevenwinship Jul 1, 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
10 changes: 10 additions & 0 deletions src/main/java/edu/harvard/iq/dataverse/DatasetVersion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,9 @@ public enum VersionState {
//The Json version of the archivalCopyLocation string
@Transient
private JsonObject archivalCopyLocationJson;

@Transient
private Boolean hasFiles = null;

public Long getId() {
return this.id;
Expand Down Expand Up @@ -2192,5 +2195,12 @@ public void setVersionNote(String note) {

this.versionNote = note;
}

public Boolean hasFiles() {
return hasFiles;
}
public void setHasFiles(Boolean hasFiles) {
this.hasFiles = hasFiles;
}
}

9 changes: 9 additions & 0 deletions src/main/java/edu/harvard/iq/dataverse/DatasetVersionServiceBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1340,6 +1340,15 @@ public Long getDatasetVersionCount(Long datasetId, boolean canViewUnpublishedVer
return em.createQuery(cq).getSingleResult();
}

public boolean hasFiles(Long datasetVersionId) {
Query query = em.createNativeQuery("SELECT id FROM fileMetadata WHERE datasetversion_id="+datasetVersionId+" LIMIT 1");
try {
query.getSingleResult();
return true;
} catch (NoResultException e) {
return false;
}
}

/**
* Update the archival copy location for a specific version of a dataset.
Expand Down
51 changes: 31 additions & 20 deletions src/main/java/edu/harvard/iq/dataverse/PermissionsWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import edu.harvard.iq.dataverse.authorization.Permission;
import edu.harvard.iq.dataverse.authorization.groups.impl.builtin.AuthenticatedUsers;
import edu.harvard.iq.dataverse.authorization.users.GuestUser;
import edu.harvard.iq.dataverse.authorization.users.User;
import edu.harvard.iq.dataverse.engine.command.Command;
import edu.harvard.iq.dataverse.engine.command.DataverseRequest;
Expand All @@ -33,6 +34,9 @@ public class PermissionsWrapper implements java.io.Serializable {
@EJB
PermissionServiceBean permissionService;

@EJB
DatasetVersionServiceBean datasetVersionService;

@Inject
DataverseSession session;

Expand Down Expand Up @@ -257,35 +261,42 @@ public boolean canIssueDeleteDatasetCommand(DvObject dvo){
// PUBLISH DATASET
public boolean canIssuePublishDatasetCommand(DvObject dvo){

@qqmyers qqmyers Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These are in the permissionsWrapper class - shouldn't the results also be cached for the life of the bean?

@stevenwinship stevenwinship Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why would we cache the results? It would change when a file upload completes

@qqmyers qqmyers Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If caching isn't appropriate, should the method be in the PermissionServiceBean instead? The *Wrapper classes are viewscoped and should just have cached info (probably not enforced). That would make the call accessable via API as well.

@landreev landreev Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally, we want everything that is called from JSF rendered="#{...}" attributes cached (because these are evaluated 7 times). Probably a good idea even when there is no reason to expect the task to be expensive.

In this specific case, I am actually not sure off the top of my head about the "would change when a file upload completes"... PermissionsWrapper is @ViewScoped. Does that mean that it will be a new "View" when the page re-renders itself after an upload completes? Or the same one?

@landreev landreev Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be easy to test though.

@qqmyers qqmyers Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good point - I'm not sure w/o digging - @viewScoped would stay as long as we're just doing ajax updates? We also cache in the DatasetPage at times - that would be a place where the cache could be emptied when a file upload completes.

@landreev landreev Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looked into it some more

  1. From a quick test, caching that info within a View does NOT prevent the page from properly updating itself once one or more files has been uploaded (that re-rendering apparently counts as a new View).

  2. Yes, we DO want to cache this (on top of caching the result of permissionService...canIssue(command) that we are already doing there). JSF is calling these 2 methods over and over incessantly. Having that NativeQuery executed every time feels wrong.

  3. I mentioned in the perf. issue that "The query count goes from 9K+ down to 813 ... This is ~100 more than it has been consistently between the last few releases, but ..." That was from a test of the first iteration of this PR, that was not checking for || u instanceof GuestUser. The extra 100 queries were solely from the native query and whatever it instantiated in the datasetVersionService.hasFiles() performed on the 10 underlying datasets. Re-testing the PR in its current state brought that query count down to 715. (it was 714 in 6.10.1).
    It will be worse for the dataset page and a logged in browser user.

... So yes, we want to cache.
We'll wrap it up tomorrow.

@landreev landreev Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI, this was my quick experiment last night: #12495 (this is a pr into this branch).
I am not positive if this is the best way of handling this.
I'm hoping to be able to focus on other work today.

@landreev landreev Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One last comment:
What's truly annoying, is that the only serious performance degradation this was causing was on the dataverse.xhtml page; where, from what I understand, the check on the number of files may not even be relevant. (the "can publish" check there seems to be a proxy for "can see [some] curation info" - ?)
In dataset.xhtml, where we do need to know whether the dataset has files or not, the original ...Version.getFileMetadatas().size() was NOT even a problem, I don't think - because the filemetadatas are already instantiated, via a monster findDeep() query.

... we should avoid dumping any excessive amounts of work into this, in other words.

@landreev landreev Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(although permissionsWrapper.canIssuePublishDatasetCommand() is also called from FilePage.java; it'll definitely help to avoid unnecessarily instantiating all the files in the parent dataset in there as well)

User u = session.getUser();

@qqmyers qqmyers Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

doesn't getUser return a GuestUser? i.e. getAuthenticatedUser() is needed for the check?

@stevenwinship stevenwinship Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

if (u != null && u.isSuperuser()) {
if (dvo == null || u == null || u instanceof GuestUser || !(dvo instanceof Dataset)) {
return false; // guests can not publish
}
if (u.isSuperuser()) {
return true;
}
// Return false if dataset has 0 files and user want to 'publish' or 'submit for review' and 'publish dataset requires files' flag is set
if (dvo.isInstanceofDataset()) {
Dataverse dv =((Dataset)dvo).getOwner();
if (dv != null) {
List<FileMetadata> metadataList = ((Dataset) dvo).getLatestVersion().getFileMetadatas();
if (metadataList.size() == 0 && dv.getEffectiveRequiresFilesToPublishDataset()) {
return false;
}
}
Dataset ds = (Dataset)dvo;
Dataverse dv = ds.getOwner();
if (dv != null && !datasetVersionHasFiles(ds.getLatestVersion()) && dv.getEffectiveRequiresFilesToPublishDataset()) {
return false;
}
return canIssueCommand(dvo, PublishDatasetCommand.class);
return canIssueCommand(ds, PublishDatasetCommand.class);
}

// SUBMIT DATASET FOR REVIEW
public boolean canIssueSubmitDatasetForReviewCommand(DvObject dvo){
public boolean canIssueSubmitDatasetForReviewCommand(DvObject dvo) {
User u = session.getUser();
if (dvo == null || u == null || u instanceof GuestUser || !(dvo instanceof Dataset)) {
return false; // guests can not submit for review
}
// Return false if dataset has 0 files and user want to 'publish' or 'submit for review' and 'publish dataset requires files' flag is set
if (dvo.isInstanceofDataset()) {
Dataverse dv =((Dataset)dvo).getOwner();
if (dv != null) {
List<FileMetadata> metadataList = ((Dataset) dvo).getLatestVersion().getFileMetadatas();
if (metadataList.size() == 0 && dv.getEffectiveRequiresFilesToPublishDataset()) {
return false;
}
}
Dataset ds = (Dataset)dvo;
Dataverse dv = ds.getOwner();
if (dv != null && !datasetVersionHasFiles(ds.getLatestVersion()) && dv.getEffectiveRequiresFilesToPublishDataset()) {
return false;
}
return canIssueCommand(ds, SubmitDatasetForReviewCommand.class);
}

// cache the hasFiles in the ds version for performance reasons
private boolean datasetVersionHasFiles(DatasetVersion dsv) {
if (dsv.hasFiles() == null) {
dsv.setHasFiles(datasetVersionService.hasFiles(dsv.getId()));
}
return canIssueCommand(dvo, SubmitDatasetForReviewCommand.class);
return dsv.hasFiles();
}

// For the dataverse_header fragment (and therefore, most of the pages),
Expand Down
20 changes: 10 additions & 10 deletions src/main/java/edu/harvard/iq/dataverse/search/SearchIncludeFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1411,10 +1411,6 @@ public String dataFileSizeDisplay(DataFile datafile) {
return datafile.getFriendlySize();

}

public boolean canPublishDataset(Long datasetId){
return permissionsWrapper.canIssuePublishDatasetCommand(dvObjectService.findDvObject(datasetId));
}

public void setDisplayCardValues() {

Expand Down Expand Up @@ -1552,13 +1548,17 @@ public boolean isValid(SolrSearchResult result) {
return true;
});
}

public boolean canSeeCurationStatus(Long datasetId) {
boolean creatorsCanSeeStatus = JvmSettings.UI_SHOW_CURATION_STATUS_TO_ALL.lookupOptional(Boolean.class).orElse(false);
if (creatorsCanSeeStatus) {
return permissionsWrapper.canViewUnpublishedDataset(getDataverseRequest(),(Dataset) dvObjectService.findDvObject(datasetId));

public boolean canSeeCurationStatus(DvObject dvo) {
if (dvo != null && dvo instanceof Dataset) {
boolean creatorsCanSeeStatus = JvmSettings.UI_SHOW_CURATION_STATUS_TO_ALL.lookupOptional(Boolean.class).orElse(false);
if (creatorsCanSeeStatus) {
return permissionsWrapper.canViewUnpublishedDataset(getDataverseRequest(), (Dataset)dvo);
} else {
return permissionsWrapper.canIssuePublishDatasetCommand(dvo);
}
} else {
return canPublishDataset(datasetId);
return false;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/webapp/search-include-fragment.xhtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@
<h:outputText value="#{bundle['dataset.versionUI.deaccessioned']}" styleClass="label label-danger" rendered="#{result.deaccessionedState}"/>
<h:outputText value="#{bundle['embargoed']}" styleClass="label label-primary" rendered="#{SearchIncludeFragment.isActivelyEmbargoed(result)}"/>
<h:outputText value="#{bundle['retentionExpired']}" styleClass="label label-warning" rendered="#{SearchIncludeFragment.isRetentionExpired(result)}"/>
<h:outputText value="#{DatasetUtil:getLocaleCurationStatusLabelFromString(result.curationStatus)}" styleClass="label label-info curation-status" rendered="#{SearchIncludeFragment.canSeeCurationStatus(result.entityId)}"/>
<h:outputText value="#{DatasetUtil:getLocaleCurationStatusLabelFromString(result.curationStatus)}" styleClass="label label-info curation-status" rendered="#{!empty result.curationStatus and SearchIncludeFragment.canSeeCurationStatus(result.entity)}"/>
<h:outputText value="#{result.userRole}" styleClass="label label-primary" rendered="#{!empty result.userRole}"/>
<h:outputText value="#{bundle['incomplete']}" styleClass="label label-danger" rendered="#{!result.harvested and !SearchIncludeFragment.isValid(result)}"/>
<h:outputText value="#{bundle['dataset.versionUI.locallyFAIR']}" title="#{bundle['dataset.versionUI.locallyFAIR.tip']}" styleClass="label label-primary" rendered="#{result.locallyFAIR}"/>
Expand Down