diff --git a/src/main/java/com/knowledgepixels/nanodash/QueryResult.java b/src/main/java/com/knowledgepixels/nanodash/QueryResult.java index 17e2a5e0..ba0e1630 100644 --- a/src/main/java/com/knowledgepixels/nanodash/QueryResult.java +++ b/src/main/java/com/knowledgepixels/nanodash/QueryResult.java @@ -95,6 +95,17 @@ public QueryRef getQueryRef() { return queryRef; } + /** + * The version of the view definition this view display is showing, as the id to hand to + * {@link View#refreshLatestVersion(String)} when checking for a newer one. + * + * @return the shown view's id, or null if this result has no view behind it + */ + public String getShownViewId() { + View view = (viewDisplay == null ? null : viewDisplay.getView()); + return view == null ? null : view.getId(); + } + public void setRefreshing(boolean refreshing) { refreshIndicator.setVisible(refreshing); } diff --git a/src/main/java/com/knowledgepixels/nanodash/View.java b/src/main/java/com/knowledgepixels/nanodash/View.java index 612dae42..cad98bd5 100644 --- a/src/main/java/com/knowledgepixels/nanodash/View.java +++ b/src/main/java/com/knowledgepixels/nanodash/View.java @@ -23,6 +23,7 @@ import java.io.Serializable; import java.util.*; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Supplier; import java.util.concurrent.TimeUnit; /** @@ -209,6 +210,14 @@ public static View get(String id, boolean resolveLatest) { // fall through to the memoized latest path, which resolves a governed // version space-based (never supersedes-based) for this pin } + // Inside a fresh-resolution scope (a page-level "refresh now", see + // withFreshResolution) the memo is not to be trusted at all: go back to the API + // once per id, then let the re-memoized answer serve the rest of the build. + Set freshScope = freshlyResolved.get(); + if (freshScope != null && freshScope.add(id)) { + View refreshed = refreshLatestVersion(id); + if (refreshed != null) return refreshed; + } Pair memo = latestResolvedViews.getIfPresent(id); if (memo != null) { if (System.currentTimeMillis() - memo.getLeft() > REFRESH_RESOLUTION_AFTER_MS) { @@ -223,6 +232,37 @@ public static View get(String id, boolean resolveLatest) { return resolved; } + /** + * The ids already re-resolved in the current fresh-resolution scope, or null outside + * one. Thread-confined: a scope covers one build on one thread (see + * {@link #withFreshResolution}). + */ + private static final ThreadLocal> freshlyResolved = new ThreadLocal<>(); + + /** + * Runs the given build with every latest-version resolution it makes going back to the + * query API instead of answering from the memo — what a page-level "refresh now" asks + * for (issue #654). Which id a view is looked up by is the caller's business (a display + * resolves the version its nanopub references, a built-in view the id hard-coded for + * it), so the scope covers the whole build rather than a list of ids guessed in advance; + * each id is re-resolved once, and what that leaves memoized serves the rest of it. + *

+ * The lookups block, so this belongs on a background thread, never on a request thread. + * + * @param build the build to run + * @param what it returns + * @return what the build returns + */ + public static T withFreshResolution(Supplier build) { + if (freshlyResolved.get() != null) return build.get(); + freshlyResolved.set(new HashSet<>()); + try { + return build.get(); + } finally { + freshlyResolved.remove(); + } + } + /** * Re-resolves the latest version of a view, going back to the query API instead of * trusting what is memoized. This is what lets a view display's "refresh now" bring the diff --git a/src/main/java/com/knowledgepixels/nanodash/WicketApplication.java b/src/main/java/com/knowledgepixels/nanodash/WicketApplication.java index ad32712a..46d182c2 100644 --- a/src/main/java/com/knowledgepixels/nanodash/WicketApplication.java +++ b/src/main/java/com/knowledgepixels/nanodash/WicketApplication.java @@ -353,6 +353,12 @@ private void registerListeners() { MaintainedResourceRepository.get().forceRootRefresh(waitMs); } else if (AbstractResourceWithProfile.isResourceWithProfile(target)) { AbstractResourceWithProfile resource = AbstractResourceWithProfile.get(target); + // What was just published can be a new version of a view this resource + // shows, whose resolution the rebuilt structure would otherwise take from + // the memo and so keep showing the previous definition (issue #654). The + // views' results are left alone: only the view that was acted on is + // refreshed (issue #622). + resource.requestViewDefinitionRefresh(); resource.forceRefresh(waitMs); if (resource instanceof Space) { SpaceRepository.get().forceRootRefresh(waitMs); diff --git a/src/main/java/com/knowledgepixels/nanodash/component/PageTitleMenu.java b/src/main/java/com/knowledgepixels/nanodash/component/PageTitleMenu.java index 873fd906..835b1519 100644 --- a/src/main/java/com/knowledgepixels/nanodash/component/PageTitleMenu.java +++ b/src/main/java/com/knowledgepixels/nanodash/component/PageTitleMenu.java @@ -138,14 +138,24 @@ public void onClick(Optional target) { boolean hasViewList = Boolean.TRUE.equals(getPage().visitChildren(ViewList.class, (IVisitor) (list, visit) -> visit.stop(Boolean.TRUE))); if (r != null) { - r.forceRefresh(0); + // Asked for before the structure refresh is set going, so that the + // update it triggers is one that already knows to re-resolve the view + // definitions along the way. if (hasViewList) r.requestViewRefresh(); + r.forceRefresh(0); } getPage().visitChildren(QueryResult.class, (IVisitor) (view, visit) -> { + if (view.findParent(ViewList.class) != null) return; QueryRef queryRef = view.getQueryRef(); - if (queryRef != null && view.findParent(ViewList.class) == null) { - ApiCache.clearCache(queryRef, 0); - } + if (queryRef != null) ApiCache.clearCache(queryRef, 0); + // A view built outside the page's structure has no refreshed view list to + // pick a newer definition up from, so its version is re-checked here, the + // way a view display's own "refresh now" does it (issue #654). The + // resolution is dropped rather than replaced in place: the panel looks the + // view up by the id hard-coded for it, and finding nothing memoized under + // that id is what makes the re-render resolve it afresh. + String shownViewId = view.getShownViewId(); + if (shownViewId != null) View.refreshLatestVersion(shownViewId); }); setResponsePage(getPage().getClass(), getPage().getPageParameters()); } diff --git a/src/main/java/com/knowledgepixels/nanodash/domain/AbstractResourceWithProfile.java b/src/main/java/com/knowledgepixels/nanodash/domain/AbstractResourceWithProfile.java index f1401a55..0c335964 100644 --- a/src/main/java/com/knowledgepixels/nanodash/domain/AbstractResourceWithProfile.java +++ b/src/main/java/com/knowledgepixels/nanodash/domain/AbstractResourceWithProfile.java @@ -5,6 +5,7 @@ import com.knowledgepixels.nanodash.ApiCache; import com.knowledgepixels.nanodash.NanodashThreadPool; import com.knowledgepixels.nanodash.QueryApiAccess; +import com.knowledgepixels.nanodash.View; import com.knowledgepixels.nanodash.ViewDisplay; import com.knowledgepixels.nanodash.repository.SpaceRepository; import com.knowledgepixels.nanodash.vocabulary.KPXL_TERMS; @@ -56,6 +57,13 @@ public abstract class AbstractResourceWithProfile implements Serializable, Resou // on is refreshed (issue #622). Taken away by the first view list built once the // refreshed structure has landed. See isViewRefreshDue. private volatile boolean viewRefreshRequested = false; + // The same page-level "refresh now" also asks for the view definitions the refreshed + // structure references to be re-checked, not only their results (issue #654): a + // memoized resolution is otherwise only re-checked once a minute in the background, so + // a view published a moment ago would keep rendering as its previous version. Read and + // taken by the structure update itself, where going back to the API costs nobody's + // request thread. See buildViewDisplays. + private volatile boolean viewDefinitionRefreshRequested = false; // Whether the view lists built during the current request are to refresh their views, // answered once per resource and remembered for the rest of the render so that several @@ -160,6 +168,11 @@ public synchronized Future triggerDataUpdate() { runUpdateAfter = null; logger.info("Data needs update for resource {}, starting update thread", id); dataNeedsUpdate = false; + // Taken here rather than in the task, so that a request arriving while the + // fetch runs is left standing for the next round instead of being answered by + // a build that started before it. + final boolean refreshViewDefinitions = viewDefinitionRefreshRequested; + viewDefinitionRefreshRequested = false; return NanodashThreadPool.submit(() -> { try { ResourceWithProfile newData = new ResourceWithProfile(); @@ -171,7 +184,7 @@ public synchronized Future triggerDataUpdate() { // aggregation in getViewDisplays() resolves overrides between presets and // standalone displays correctly, in either direction. seedFromCacheIfPossible(); - newData.viewDisplays.addAll(buildViewDisplays(viewDisplaysQueryRef())); + newData.viewDisplays.addAll(buildViewDisplays(viewDisplaysQueryRef(), refreshViewDefinitions)); newData.profilePicture = fetchProfilePicture(); data = newData; dataInitialized = true; @@ -183,6 +196,8 @@ public synchronized Future triggerDataUpdate() { logger.error("Error while trying to update data for resource {}", id, ex); runUpdateAfter = System.currentTimeMillis() + FAILED_UPDATE_BACKOFF_MS; dataNeedsUpdate = true; + // Nothing was rebuilt, so the request the retry is to honour is put back. + if (refreshViewDefinitions) viewDefinitionRefreshRequested = true; } }); } @@ -279,9 +294,27 @@ public boolean isStructureRefreshPending() { * honoured by the first view list built once the refreshed structure has landed (see * {@link #isViewRefreshDue(boolean)}), so it is the refreshed list that gets refreshed, not * the one that happened to be on screen when the user clicked. + *

+ * "Up to date" covers each view's definition as well as its results: the structure + * update re-resolves the referenced views instead of trusting the memoized resolution, + * so a view definition superseded a moment ago is picked up by this refresh rather than + * by whichever one happens to follow it (issue #654). */ public void requestViewRefresh() { viewRefreshRequested = true; + requestViewDefinitionRefresh(); + } + + /** + * Asks for the view definitions this resource's structure references to be re-resolved + * when it is next rebuilt, without asking for the views' results to be refreshed along + * with them. What a publication needs: the nanopub just published may be a new version + * of a view shown here, and the resolution that would otherwise be reused is memoized + * (issue #654). Refreshing every view's results on top of that is what issue #622 took + * away, so it stays away — only the view that was acted on is refreshed there. + */ + public void requestViewDefinitionRefresh() { + viewDefinitionRefreshRequested = true; } /** @@ -339,8 +372,12 @@ public boolean isViewRefreshDue(boolean waitsForStructure) { public String getStructureSignature() { StringBuilder sb = new StringBuilder(); for (ViewDisplay vd : data.viewDisplays) { + // Both the referenced view and the version it resolved to: a new version of a + // view the displays already reference leaves the reference as it was, and the + // page would go on showing the previous definition if that were all we compared. sb.append(vd.getNanopubId()).append('\t') .append(vd.getViewIri()).append('\t') + .append(vd.getView() == null ? "" : vd.getView().getId()).append('\t') .append(vd.getStructuralPosition()).append('\n'); } return sb.toString(); @@ -496,12 +533,33 @@ public List fetchViewDisplaysSync(String partId, Set partClass * displays with a bound {@code ?display}, and preset-supplied views with an unbound one). */ private List buildViewDisplays(QueryRef ref) { + return buildViewDisplays(ref, false); + } + + /** + * @param refreshViewDefinitions whether to go back to the API for each referenced view's + * latest version instead of trusting what is memoized — + * what a page-level "refresh now" asks for (issue #654). + * Only ever true on the update thread, as the lookups block. + */ + private List buildViewDisplays(QueryRef ref, boolean refreshViewDefinitions) { // Null on a cold cache or a failed (flaky federated) fetch — yields nothing for now; // the cache refreshes asynchronously and the page's auto-refresh repopulates it. - return buildViewDisplays(ApiCache.retrieveResponseSync(ref, true), ref); + return buildViewDisplays(ApiCache.retrieveResponseSync(ref, true), ref, refreshViewDefinitions); } private List buildViewDisplays(ApiResponse response, QueryRef ref) { + return buildViewDisplays(response, ref, false); + } + + private List buildViewDisplays(ApiResponse response, QueryRef ref, boolean refreshViewDefinitions) { + if (refreshViewDefinitions) { + // Every view this build resolves goes back to the API rather than to the memo. + // Done around the whole build rather than per row: which id a display's view is + // resolved by is up to the display nanopub (the referenced version) and the + // query variant (the server-resolved one), and the scope catches either. + return View.withFreshResolution(() -> buildViewDisplays(response, ref, false)); + } List list = new ArrayList<>(); if (response == null) return list; // The unresolved query variant returns ?view as the referenced version, leaving @@ -511,11 +569,11 @@ private List buildViewDisplays(ApiResponse response, QueryRef ref) boolean viewsPreResolved = !QueryApiAccess.GET_VIEW_DISPLAYS_UNRESOLVED.equals(ref.getQueryId()); for (ApiResponseEntry r : response.getData()) { try { + String view = r.get("view"); String display = r.get("display"); if (display != null && !display.isEmpty()) { - list.add(ViewDisplay.get(display, viewsPreResolved ? r.get("view") : null)); + list.add(ViewDisplay.get(display, viewsPreResolved ? view : null)); } else { - String view = r.get("view"); if (view == null || view.isEmpty()) continue; boolean topLevel = KPXL_TERMS.TOP_LEVEL_VIEW_DISPLAY.stringValue().equals(r.get("displayType")); boolean deactivated = KPXL_TERMS.DEACTIVATED_PRESET_ASSIGNMENT.stringValue().equals(r.get("displayMode"));