From 33e71c1ff5408c333a44425e2e11c94b7854588f Mon Sep 17 00:00:00 2001 From: James Sharkey Date: Wed, 12 Jul 2023 11:24:48 +0100 Subject: [PATCH 1/5] Use getContentById to load concept pages Unlike findSingleResult, the getContentById method uses the cache in GitContentManager. This should speed up loading commonly-accessed pages. --- .../ac/cam/cl/dtg/isaac/api/PagesFacade.java | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java index b308381305..6538ac52db 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java @@ -32,6 +32,7 @@ import uk.ac.cam.cl.dtg.isaac.dos.QuestionValidationResponse; import uk.ac.cam.cl.dtg.isaac.dos.content.Content; import uk.ac.cam.cl.dtg.isaac.dto.GameboardDTO; +import uk.ac.cam.cl.dtg.isaac.dto.IsaacConceptPageDTO; import uk.ac.cam.cl.dtg.isaac.dto.IsaacPageFragmentDTO; import uk.ac.cam.cl.dtg.isaac.dto.IsaacQuestionPageDTO; import uk.ac.cam.cl.dtg.isaac.dto.IsaacTopicSummaryPageDTO; @@ -69,6 +70,7 @@ import jakarta.ws.rs.core.Response.Status; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -235,40 +237,45 @@ public final Response getConcept(@Context final Request request, @Context final return new SegueErrorResponse(Status.BAD_REQUEST, "You must provide a valid concept id.").toResponse(); } - // Calculate the ETag on current live version of the content - // NOTE: Assumes that the latest version of the content is being used. - EntityTag etag = new EntityTag(this.contentManager.getCurrentContentSHA().hashCode() + "byId".hashCode() - + conceptId.hashCode() + ""); + // Calculate the ETag on current live version of the content: + EntityTag etag = new EntityTag(String.valueOf(this.contentManager.getCurrentContentSHA().hashCode() + conceptId.hashCode())); Response cachedResponse = generateCachedResponse(request, etag); if (cachedResponse != null) { return cachedResponse; } - Map> fieldsToMatch = Maps.newHashMap(); - fieldsToMatch.put(TYPE_FIELDNAME, Arrays.asList(CONCEPT_TYPE)); - - // options - fieldsToMatch.put(ID_FIELDNAME + "." + UNPROCESSED_SEARCH_FIELD_SUFFIX, Arrays.asList(conceptId)); - - Response result = this.findSingleResult(fieldsToMatch); try { - if (result.getEntity() instanceof SeguePageDTO) { + ContentDTO contentDTO = contentManager.getContentById(conceptId); + if (contentDTO instanceof IsaacConceptPageDTO) { + SeguePageDTO content = (SeguePageDTO) contentDTO; + // Do we want to use the user's actual question attempts here? We did not previously. + augmentContentWithRelatedContent(content, Collections.emptyMap()); + ImmutableMap logEntry = new ImmutableMap.Builder() - .put(CONCEPT_ID_LOG_FIELDNAME, conceptId).put(CONTENT_VERSION_FIELDNAME, this.contentManager.getCurrentContentSHA()) + .put(CONCEPT_ID_LOG_FIELDNAME, conceptId) + .put(CONTENT_VERSION_FIELDNAME, this.contentManager.getCurrentContentSHA()) .build(); // the request log getLogManager().logEvent(userManager.getCurrentUser(servletRequest), servletRequest, IsaacServerLogType.VIEW_CONCEPT, logEntry); - } - Response cachableResult = Response.status(result.getStatus()).entity(result.getEntity()) - .cacheControl(getCacheControl(NUMBER_SECONDS_IN_ONE_HOUR, true)).tag(etag).build(); - return cachableResult; + return Response.ok(content) + .cacheControl(getCacheControl(NUMBER_SECONDS_IN_ONE_HOUR, true)) + .tag(etag) + .build(); + } else { + String error = "Unable to locate a concept with the id specified: " + conceptId; + log.warn(error); + return SegueErrorResponse.getResourceNotFoundResponse(error); + } } catch (SegueDatabaseException e) { - SegueErrorResponse error = new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, - "Database error while looking up user information.", e); + SegueErrorResponse error = new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, "Database error while looking up user information.", e); + log.error(error.getErrorMessage(), e); + return error.toResponse(); + } catch (ContentManagerException e) { + SegueErrorResponse error = new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, "Error locating the content requested", e); log.error(error.getErrorMessage(), e); return error.toResponse(); } From 12299d83d7fb2f89f4af23e0344f16a2100ecdd0 Mon Sep 17 00:00:00 2001 From: James Sharkey Date: Wed, 12 Jul 2023 11:36:50 +0100 Subject: [PATCH 2/5] Use getContentById to load general pages Same as 33e71c1ff5408c333a44425e2e11c94b7854588f but for general pages. --- .../ac/cam/cl/dtg/isaac/api/PagesFacade.java | 46 ++++++++++--------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java index 6538ac52db..4fd5628145 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java @@ -628,44 +628,48 @@ public final Response getPage(@Context final Request request, @Context final Htt return new SegueErrorResponse(Status.BAD_REQUEST, "You must provide a valid page id.").toResponse(); } - // Calculate the ETag on current live version of the content - // NOTE: Assumes that the latest version of the content is being used. - EntityTag etag = new EntityTag(this.contentManager.getCurrentContentSHA().hashCode() + pageId.hashCode() + ""); + // Calculate the ETag on current live version of the content: + EntityTag etag = new EntityTag(String.valueOf(this.contentManager.getCurrentContentSHA().hashCode() + pageId.hashCode())); Response cachedResponse = generateCachedResponse(request, etag); if (cachedResponse != null) { return cachedResponse; } - Map> fieldsToMatch = Maps.newHashMap(); - fieldsToMatch.put(TYPE_FIELDNAME, Arrays.asList(PAGE_TYPE, QUESTIONS_PAGE_TYPE)); - - // options - fieldsToMatch.put(ID_FIELDNAME + "." + UNPROCESSED_SEARCH_FIELD_SUFFIX, Arrays.asList(pageId)); - try { - Response result = this.findSingleResult(fieldsToMatch); - - if (result.getEntity() instanceof SeguePageDTO) { - - - ImmutableMap logEntry = new ImmutableMap.Builder() - .put(PAGE_ID_LOG_FIELDNAME, pageId) - .put(CONTENT_VERSION_FIELDNAME, this.contentManager.getCurrentContentSHA()).build(); + ContentDTO contentDTO = contentManager.getContentById(pageId); + // We must not allow subclasses here, since general pages are the base class for all other page types! + if (SeguePageDTO.class.equals(contentDTO.getClass())) { + SeguePageDTO content = (SeguePageDTO) contentDTO; + // Unlikely we want to augment with a user's actual question attempts. Use an empty Map. + augmentContentWithRelatedContent(content, Collections.emptyMap()); // the request log + ImmutableMap logEntry = ImmutableMap.of( + PAGE_ID_LOG_FIELDNAME, pageId, + CONTENT_VERSION_FIELDNAME, this.contentManager.getCurrentContentSHA() + ); getLogManager().logEvent(userManager.getCurrentUser(httpServletRequest), httpServletRequest, IsaacServerLogType.VIEW_PAGE, logEntry); - } - Response cachableResult = Response.status(result.getStatus()).entity(result.getEntity()) - .cacheControl(getCacheControl(NUMBER_SECONDS_IN_ONE_HOUR, true)).tag(etag).build(); - return cachableResult; + return Response.ok(content) + .cacheControl(getCacheControl(NUMBER_SECONDS_IN_ONE_HOUR, true)) + .tag(etag) + .build(); + } else { + String error = "Unable to locate a page with the id specified: " + pageId; + log.warn(error); + return SegueErrorResponse.getResourceNotFoundResponse(error); + } } catch (SegueDatabaseException e) { SegueErrorResponse error = new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, "Database error while looking up user information.", e); log.error(error.getErrorMessage(), e); return error.toResponse(); + } catch (ContentManagerException e) { + SegueErrorResponse error = new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, "Error locating the content requested", e); + log.error(error.getErrorMessage(), e); + return error.toResponse(); } } From 4828c77ff27e707d65612f9063f52a3fd0e60bbd Mon Sep 17 00:00:00 2001 From: James Sharkey Date: Wed, 12 Jul 2023 11:57:40 +0100 Subject: [PATCH 3/5] Use getContentById to load page fragments Same as 33e71c1ff5408c333a44425e2e11c94b7854588f but for page fragments. --- .../ac/cam/cl/dtg/isaac/api/PagesFacade.java | 49 +++++++++++-------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java index 4fd5628145..6bfcb6073c 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java @@ -691,35 +691,44 @@ public final Response getPage(@Context final Request request, @Context final Htt @Operation(summary = "Get a content page fragment by ID.") public final Response getPageFragment(@Context final Request request, @Context final HttpServletRequest httpServletRequest, @PathParam("fragment_id") final String fragmentId) { - try { - // Calculate the ETag on current live version of the content - // NOTE: Assumes that the latest version of the content is being used. - EntityTag etag = new EntityTag(this.contentManager.getCurrentContentSHA().hashCode() + fragmentId.hashCode() + ""); - Response cachedResponse = generateCachedResponse(request, etag); - if (cachedResponse != null) { - return cachedResponse; - } - - Map> fieldsToMatch = Maps.newHashMap(); - fieldsToMatch.put(TYPE_FIELDNAME, Arrays.asList(PAGE_FRAGMENT_TYPE)); - fieldsToMatch.put(ID_FIELDNAME + "." + UNPROCESSED_SEARCH_FIELD_SUFFIX, Arrays.asList(fragmentId)); - Response result = this.findSingleResult(fieldsToMatch); + // Calculate the ETag on current live version of the content + EntityTag etag = new EntityTag(String.valueOf(this.contentManager.getCurrentContentSHA().hashCode() + fragmentId.hashCode())); + Response cachedResponse = generateCachedResponse(request, etag); + if (cachedResponse != null) { + return cachedResponse; + } + try { + ContentDTO contentDTO = contentManager.getContentById(fragmentId); + if (contentDTO instanceof IsaacPageFragmentDTO) { + // Unlikely we want to augment with related content here! - if (result.getEntity() instanceof IsaacPageFragmentDTO) { + // the request log + ImmutableMap logEntry = ImmutableMap.of( + FRAGMENT_ID_LOG_FIELDNAME, fragmentId, + CONTENT_VERSION_FIELDNAME, this.contentManager.getCurrentContentSHA() + ); getLogManager().logEvent(userManager.getCurrentUser(httpServletRequest), httpServletRequest, - IsaacServerLogType.VIEW_PAGE_FRAGMENT, ImmutableMap.of( - FRAGMENT_ID_LOG_FIELDNAME, fragmentId, - CONTENT_VERSION_FIELDNAME, this.contentManager.getCurrentContentSHA() - )); + IsaacServerLogType.VIEW_PAGE, logEntry); + + return Response.ok(contentDTO) + .cacheControl(getCacheControl(NUMBER_SECONDS_IN_ONE_HOUR, true)) + .tag(etag) + .build(); + } else { + String error = "Unable to locate a page fragment with the id specified: " + fragmentId; + log.warn(error); + return SegueErrorResponse.getResourceNotFoundResponse(error); } - return Response.status(result.getStatus()).entity(result.getEntity()) - .cacheControl(getCacheControl(NUMBER_SECONDS_IN_ONE_HOUR, true)).tag(etag).build(); } catch (SegueDatabaseException e) { SegueErrorResponse error = new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, "Database error while looking up user information.", e); log.error(error.getErrorMessage(), e); return error.toResponse(); + } catch (ContentManagerException e) { + SegueErrorResponse error = new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, "Error locating the content requested", e); + log.error(error.getErrorMessage(), e); + return error.toResponse(); } } From ec9024a0c7d5ba66a3e4024362b784b1e1a9285e Mon Sep 17 00:00:00 2001 From: James Sharkey Date: Wed, 12 Jul 2023 12:05:43 +0100 Subject: [PATCH 4/5] Remove unused findSingleResult method We now consistently use the cache-based getContentById. The only other difference (beyond this old method lacking caching) is that this errored out if more than one piece of content matched, but I think that's no big loss. --- .../ac/cam/cl/dtg/isaac/api/PagesFacade.java | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java index 6bfcb6073c..952c929847 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java @@ -919,40 +919,6 @@ private List extractContentSummaryFromList(final List list of queries to match - * @return A Response containing a single conceptPage or containing a SegueErrorResponse. - */ - private Response findSingleResult(final Map> fieldsToMatch) { - try { - ResultsWrapper resultList = api.findMatchingContent(this.contentIndex, - ContentService.generateDefaultFieldToMatch(fieldsToMatch), null, null); // includes - // type - // checking. - ContentDTO c = null; - if (resultList.getResults().size() > 1) { - return new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, "Multiple results (" - + resultList.getResults().size() + ") returned error. For search query: " + fieldsToMatch.values()) - .toResponse(); - } else if (resultList.getResults().isEmpty()) { - return new SegueErrorResponse(Status.NOT_FOUND, "No content found that matches the query with parameters: " - + fieldsToMatch.values()).toResponse(); - } else { - c = resultList.getResults().get(0); - } - - return Response.ok(this.augmentContentWithRelatedContent(c, Maps.newHashMap())).build(); - } catch (ContentManagerException e1) { - SegueErrorResponse error = new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, "Error locating the content requested", - e1); - log.error(error.getErrorMessage(), e1); - return error.toResponse(); - } - } - /** * Helper method to query segue for a list of content objects. * From fbd301284593ad2ac539420cdc4a1a4a016ab61a Mon Sep 17 00:00:00 2001 From: James Sharkey Date: Thu, 13 Jul 2023 15:44:27 +0100 Subject: [PATCH 5/5] Remove long-deprecated QuestionsPage constant We removed the type this corresponded to in 72cb9102ee184ca9e9cde72794f08cf09643a463 --- src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java index 98ecf7e1a3..b4a5affaa7 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java @@ -44,7 +44,6 @@ public final class Constants { public static final String PAGE_FRAGMENT_TYPE = "isaacPageFragment"; public static final String POD_FRAGMENT_TYPE = "isaacPod"; public static final String PAGE_TYPE = "page"; - public static final String QUESTIONS_PAGE_TYPE = "questionsPage"; public static final String TOPIC_SUMMARY_PAGE_TYPE = "isaacTopicSummaryPage"; public static final String EVENT_TYPE = "isaacEventPage"; public static final String QUIZ_TYPE = "isaacQuiz";