blocked deposit page, hide host field when only one collection (e.g. root) #11301
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ofahimIQSS
moved this from Ready for Triage
to Ready for Review ⏩
in IQSS Dataverse Project
Member
|
Can you reproduce this on demo (which is v6.5)? The code changes look OK but I haven't yet been able to trigger the problem on demo.dataverse.org. |
Member
|
I believe I was able to reproduce the bug on ab8110f (the latest in develop). The page is greyed out like this:
Stacktrace: |
Member
|
I tried again and I was able to reproduce it again. One difference for me, perhaps, is that after a hard-reset the fields I entered are still there, with the exception of the Subject field. So I don't lose all of my work, but still, it's a very annoying user experience! |
Member
|
@jo-pol I switched to this branch/PR and the stacktrace looks different but I have the same behavior in the UI. The screen goes gray-ish and the dataset isn't saved.
|
Member
|
FWIW: Looks like it only occurs when there are no templates for some reason. |
Contributor
Author
|
@pdurbin |
Contributor
Author
|
Tried the IQSS develop branch on a DANS v6.5 VM and got an internal DB error. |
Member
|
@jo-pol sorry, I was trying to use the same terminology you were. I said "hard reset" when I meant "hard refresh". What was the internal DB error? 🤔 |
Contributor
Author
|
The DB error was |
Contributor
Author
|
Tried returning null in stead of a string also worked on a DANS v6.5 VM. Examining the stack trace and corresponding jakarta/faces code that would cause a null pointer exception on IQSS development. So now I used zero as default. Don't know about the effects on templates and/or when the initial dataverse was another one. |
Contributor
Author
|
Not sure but I might have stayed on the page https://dev.archaeology.datastations.nl/dataset.xhtml?ownerId=1 when redeploying the war when I saw the DB error. |
Member
Right, sorry, that was our bad. Fixed in this PR: |
cmbz
added
FY25 Sprint 19
Member
|
@jo-pol we just released Dataverse 6.6. Can you please merge the latest from develop into this branch? Also, as I said above, I can definitely reproduce some strangeness but this pull request doesn't seem to help (the error in server.log was different). Am I missing something? Thanks! |
Contributor
Author
|
@pdurbin |
pdurbin
changed the title
jo-pol
added a commit
to DANS-KNAW/dataverse
that referenced
this pull request
Jun 5, 2025
qqmyers
approved these changes
Jun 5, 2025
Member
qqmyers
left a comment
qqmyers
left a comment
There was a problem hiding this comment.
OK - I think this looks good now (haven't tested the latest changes) except that the release note needs to be updated again (the default is the original dataverse not root now). I'll go ahead and approve so this can get into QA.
github-project-automation
bot
moved this from In Review 🔎
to Ready for QA ⏩
in IQSS Dataverse Project
Contributor
|
Fix looks good! Merging. |
github-project-automation
bot
moved this from QA ✅
to Merged 🚀
in IQSS Dataverse Project
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical bug where users could lose their dataset metadata when editing the "Host Dataverse" field. The solution involves hiding the host dataverse field when there's only one collection available and providing better error handling when the field is cleared accidentally.
- Hides the host dataverse field in CREATE mode when there's only one dataverse to choose from
- Adds robust error handling in DataverseConverter to prevent crashes when invalid values are submitted
- Includes release notes documenting the user-facing improvements
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/main/webapp/dataset.xhtml | Conditionally renders host dataverse field only when multiple dataverses exist |
| src/main/java/edu/harvard/iq/dataverse/DataverseConverter.java | Adds validation and fallback logic for invalid dataverse submissions |
| src/main/java/edu/harvard/iq/dataverse/DatasetPage.java | Implements method to check if multiple dataverses are available for selection |
| doc/release-notes/11301-blocked-deposit-page.md | Documents the bug fix and user-facing improvements |
| } | ||
|
|
||
| public boolean isHasDataversesToChoose() { | ||
| this.hasDataversesToChoose = dataverseService.findAll().size() > 1; |
There was a problem hiding this comment.
The method isHasDataversesToChoose() calls dataverseService.findAll().size() every time it's invoked, which could be expensive if there are many dataverses. Consider caching this value or using a more efficient query to count dataverses.
Suggested change
| this.hasDataversesToChoose = dataverseService.findAll().size() > 1; | |
| this.hasDataversesToChoose = dataverseService.countDataverses() > 1; |
Member
There was a problem hiding this comment.
DataverseServiceBean.getDataverseCount() exists and could be used here. - countDataverses() doesn't exist.
Contributor
Author
There was a problem hiding this comment.
@qqmyers mised these comment so far. Got another suggestion form copilot, perhaps more elaborate to implement but might be more effiecient
Query q = em.createQuery("SELECT 1 FROM Dataverse d");
q.setMaxResults(1);
boolean exists = !q.getResultList().isEmpty();
Member
There was a problem hiding this comment.
Hmm - with maxResults set to one, you'd never know if there was more than one (thanks Copilot!), but I guess it could be limited to 2 instead.
That said, I think they key thing is to avoid getting a list of objects - that's a big expense.
From a quick test, getting two rows is cheaper than getting the count, but both are orders of magnitude less than getting the objects.
I guess since limiting to 2 rows will scale better than getting the count, I guess this approach makes more sense if you're willing to implement it.
Contributor
Author
There was a problem hiding this comment.
Just logged the duration on a VM with almost 100K dataverses
findall: PT15.632S
getDataverseCount: PT-0.026S
Looks good enough with less effort.
| */ | ||
| @FacesConverter("dataverseConverter") | ||
| public class DataverseConverter implements Converter { | ||
| private static final Logger logger = Logger.getLogger(DatasetPage.class.getCanonicalName()); |
There was a problem hiding this comment.
The logger is incorrectly configured to use DatasetPage.class.getCanonicalName() instead of DataverseConverter.class.getCanonicalName(). This will cause log messages to appear as if they're coming from DatasetPage rather than DataverseConverter.
Suggested change
| private static final Logger logger = Logger.getLogger(DatasetPage.class.getCanonicalName()); | |
| private static final Logger logger = Logger.getLogger(DataverseConverter.class.getCanonicalName()); |
| return CDI.current().select(DatasetPage.class).get().getSelectedHostDataverse(); | ||
| } | ||
| else { | ||
| return dataverseService.find(Long.valueOf(submittedValue)); |
There was a problem hiding this comment.
Using Long.valueOf() can throw NumberFormatException even though the regex check should prevent this. However, the regex [0-9]+ doesn't account for potential overflow of Long values. Consider using Long.parseLong() with proper exception handling or add bounds checking.
Suggested change
| return dataverseService.find(Long.valueOf(submittedValue)); | |
| try { | |
| return dataverseService.find(Long.parseLong(submittedValue)); | |
| } catch (NumberFormatException e) { | |
| logger.severe("Submitted value is out of range for a Long: " + submittedValue); | |
| return null; | |
| } |
pdurbin
added a commit
that referenced
this pull request
Jul 30, 2025
pdurbin
added a commit
that referenced
this pull request
Jul 30, 2025
With the wrong line, the "host dataverse" field wasn't working at all!
ofahimIQSS
added a commit
that referenced
this pull request
Jul 31, 2025
Revert #11301 and bump version to 6.7.1
Member
|
@jo-pol - I don't know if you saw, but this PR was reverted in #11700 after it was discovered that the query you added caused a massive slowdown on the create dataset page at Harvard (where there are thousands of collections) - see #11698 for details. If you think all or part of this should get into v6.8, you should submit a new PR. (I haven't tested but I think the getDataverseCount() query noted in the comments above, and caching it's value after the first call as suggested in https://guides.dataverse.org/en/latest/developers/tips.html#avoiding-inefficiencies-in-jsf-render-logic would probably solve the performance issue.) Others were more involved in investigating this - feel free to reach out if you have questions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
A dataset is not saved and there is no option to correct the mistake if you edit the “Host Dataverse” field and immediately correct and leave it. When clicking Save the page freezes and after a hard refresh all your metadata is gone and you have to start over. Reproduced in 6.3 and 6.5 (DANS branches).
A more robust solution is not to show the edit field at al when there is nothing to choose.
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
There are 2 cases:
Fine logging of DataverseConverter will show the name of the submitted dataverse - for a successful test this would be null/not a valid dataverse id number.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Perhaps: the host dataverse field does not appear when there are none to choose from.
Additional documentation: