Cache and bound the collection page link check #12672 - #12675
Open
ErykKul wants to merge 1 commit into
Open
Conversation
Collaborator
Author
|
@vera |
Test Results405 tests 390 ✅ 33m 41s ⏱️ Results for commit f54777b. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
Makes the collection page's Link button check cheap again.
DataversePage.showLinkingPopup()is bound torendered=in two places indataverse.xhtml, so it is evaluated repeatedly per request, and it computed its answer from the full list of collections the user may link — for a superuser,SELECT id, name, alias FROM DATAVERSEwith an entity per row. That is a sequential scan of the dataverse table several times per collection page load, for logged-in users only, which is what made qa.dataverse.org unusable (#12672).Two changes, no behavior change:
showLinkingPopup()memoizes into aBooleanfield, cleared bysetDataverse. Same patternDatasetPagealready uses for its ownshowLinkingPopupfield and forhasDataversesToChoose.PermissionServiceBean.findSomePermittedCollections(..., limit)appendsLIMIT nto the same base SQL, as the existinghasMultiplePermittedCollectionsdoes withLIMIT 2. The existence check asks for 1 row; the eligibility check asks for2 + ancestorCount. The current collection and its parent tree are the only collectionsremoveUnlinkableDataversescan drop here, so one candidate beyond that tree decides the question: fewer rows than the limit means we have the complete set and filter exactly; a full limit means at least one row falls outside the exclusion set.Which issue(s) this PR closes:
Special notes for your reviewer:
This is the alternative to reverting #12250 (#12674). If the revert lands first, this needs rebasing onto the re-land rather than onto develop.
Deliberately unchanged: the
query != nullbranch is dead code (DataversePage.queryis never assigned — the collection page's search query lives onSearchIncludeFragment.query), but rewiring it would change who sees the Link button, so it is left alone. Worth a follow-up issue.Also left alone:
updateLinkableDataverses()still fetches the full list, but it only runs when someone opens the Link dialog and it needs every row for the dropdown. It was unbounded before #12250 too (findAll()), so it is not part of this regression.Two things this does not fix, both worth their own issues: the superuser branch of
findPermittedCollectionsis still an unbounded scan for its four remaining callers (all API/action paths, none per-render), and it selects three columns while mapping toDataverse.class, which someone should check in a SQL log for per-row fetches.Suggestions on how to test this:
Log in as a superuser and load a collection page, including the root collection. It should be as fast as it is when logged out. The Link button should still appear when you have
LinkDataverseon some other collection and not when you do not, and the Link dialog should still list the same collections.pg_stat_statementsshould no longer showFROM DATAVERSE dvaccumulating calls per page render.Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No.
Is there a release notes update needed for this change?:
No — #12250 is unreleased, so this is a fix within the same cycle.
Additional documentation:
None.