diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/GameboardsFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/GameboardsFacade.java index 12a6754c0c..e49342961e 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/GameboardsFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/GameboardsFacade.java @@ -437,6 +437,13 @@ public final Response renameAndSaveGameboard(@Context final HttpServletRequest r @Produces(MediaType.APPLICATION_JSON) @GZIP @Operation(summary = "List all gameboards linked to the current user.") + private String sanitiseForLog(final String input) { + if (input == null) { + return "null"; + } + return input.replace('\r', '_').replace('\n', '_'); + } + public final Response getGameboardsByCurrentUser(@Context final HttpServletRequest request, @QueryParam("start_index") final String startIndex, @QueryParam("limit") final String limit, @@ -529,6 +536,21 @@ public final Response getGameboardsByCurrentUser(@Context final HttpServletReque e1); log.error(error.getErrorMessage(), e1); return error.toResponse(); + } catch (RuntimeException e) { + // Catch-all so an unexpected error (e.g. malformed content/data for a single board) is logged with full + // context here rather than only surfacing as an opaque "unhandled error" id from the generic mapper. + String safeStartIndex = sanitiseForLog(startIndex); + String safeLimit = sanitiseForLog(limit); + String safeSortInstructions = sanitiseForLog(sortInstructions); + String safeShowCriteria = sanitiseForLog(showCriteria); + + String message = String.format( + "GAMEBOARD: Unexpected error building my-boards for user %s " + + "(start_index=%s, limit=%s, sort=%s, show_only=%s)", + currentUser.getId(), safeStartIndex, safeLimit, safeSortInstructions, safeShowCriteria); + log.error(message, e); + return new SegueErrorResponse(Status.INTERNAL_SERVER_ERROR, + "Error whilst trying to access your gameboards.").toResponse(); } getLogManager().logEvent( diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/GameManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/GameManager.java index a963733e36..eb4d0e34c2 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/GameManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/GameManager.java @@ -61,6 +61,7 @@ import java.util.Set; import java.util.UUID; import java.util.stream.Collectors; +import java.util.stream.IntStream; import org.apache.commons.collections4.comparators.ComparatorChain; import org.apache.commons.lang3.Validate; import org.apache.commons.lang3.tuple.ImmutablePair; @@ -633,12 +634,8 @@ public final GameboardListDTO getUsersGameboards(final RegisteredUserDTO user, @ } ComparatorChain comparatorForSorting = new ComparatorChain(); - Comparator defaultComparator = new Comparator() { - @Override - public int compare(final GameboardDTO o1, final GameboardDTO o2) { - return o1.getLastVisited().compareTo(o2.getLastVisited()); - } - }; + Comparator defaultComparator = + Comparator.comparing(GameboardDTO::getLastVisited, Comparator.nullsLast(Comparator.naturalOrder())); // assume we want reverse date order for visited date for now. if (null == sortInstructions || sortInstructions.isEmpty()) { @@ -653,9 +650,11 @@ public int compare(final GameboardDTO o1, final GameboardDTO o2) { } if (sortInstruction.getKey().equals(CREATED_DATE_FIELDNAME)) { - comparatorForSorting.addComparator(Comparator.comparing(GameboardDTO::getCreationDate), reverseOrder); + comparatorForSorting.addComparator(Comparator.comparing(GameboardDTO::getCreationDate, + Comparator.nullsLast(Comparator.naturalOrder())), reverseOrder); } else if (sortInstruction.getKey().equals(VISITED_DATE_FIELDNAME)) { - comparatorForSorting.addComparator(Comparator.comparing(GameboardDTO::getLastVisited), reverseOrder); + comparatorForSorting.addComparator(Comparator.comparing(GameboardDTO::getLastVisited, + Comparator.nullsLast(Comparator.naturalOrder())), reverseOrder); } else if (sortInstruction.getKey().equals(TITLE_FIELDNAME)) { comparatorForSorting.addComparator((o1, o2) -> { if (o1.getTitle() == null && o2.getTitle() == null) { @@ -928,10 +927,9 @@ private GameboardDTO augmentGameboardWithQuestionAttemptInformation( try { this.augmentGameItemWithAttemptInformation(gameItem, questionAttemptsFromUser); } catch (ResourceNotFoundException e) { - log.info(String.format( - "The gameboard '%s' references an unavailable question '%s'" - + " - treating it as if it never existed for marking!", - gameboardDTO.getId(), gameItem.getId())); + log.warn("GAMEBOARD: gameboard '{}' references an unavailable question '{}' " + + "- treating it as if it never existed for marking! Reason: {}", + gameboardDTO.getId(), gameItem.getId(), e.getMessage()); continue; } @@ -1198,7 +1196,17 @@ private GameboardItem augmentGameItemWithAttemptInformation( int questionPartsNotAttempted = 0; String questionPageId = gameItem.getId(); - IsaacQuestionPageDTO questionPage = (IsaacQuestionPageDTO) this.contentManager.getContentById(questionPageId); + // NOTE: getContentById returns the base ContentDTO; the id stored on a gameboard is expected to resolve to an + // IsaacQuestionPageDTO. If a content reindex retypes or replaces that id (or the id collides with non-question + // content) the cast below would throw an uncaught ClassCastException and return 500. + ContentDTO contentForQuestionPage = this.contentManager.getContentById(questionPageId); + if (!(contentForQuestionPage instanceof IsaacQuestionPageDTO questionPage)) { + String actualType = contentForQuestionPage == null ? "null" : contentForQuestionPage.getClass().getSimpleName(); + log.warn("GAMEBOARD: question page id '{}' did not resolve to an IsaacQuestionPageDTO (was '{}'). " + + "Treating it as an unavailable question.", questionPageId, actualType); + throw new ResourceNotFoundException(String.format( + "Content id '%s' is not an IsaacQuestionPageDTO (was %s)", questionPageId, actualType)); + } // get all question parts in the question page: depends on each question // having an id that starts with the question page id. Collection listOfQuestionParts = getAllMarkableQuestionPartsDFSOrder(questionPage); @@ -1211,14 +1219,10 @@ private GameboardItem augmentGameItemWithAttemptInformation( if (questionPartAttempts != null) { // Go through the attempts in reverse chronological order for this question part to determine if // there is a correct answer somewhere. - boolean foundCorrectForThisQuestion = false; - for (int i = questionPartAttempts.size() - 1; i >= 0; i--) { - if (questionPartAttempts.get(i).isCorrect() != null - && questionPartAttempts.get(i).isCorrect()) { - foundCorrectForThisQuestion = true; - break; - } - } + boolean foundCorrectForThisQuestion = + IntStream.iterate(questionPartAttempts.size() - 1, i -> i >= 0, i -> i - 1) + .anyMatch(i -> questionPartAttempts.get(i).isCorrect() != null + && questionPartAttempts.get(i).isCorrect()); if (foundCorrectForThisQuestion) { questionPartStates.add(QuestionPartState.CORRECT); questionPartsCorrect++; @@ -1237,11 +1241,7 @@ private GameboardItem augmentGameItemWithAttemptInformation( .map(questionPart -> QuestionPartState.NOT_ATTEMPTED).collect(Collectors.toList()); } - // Get the pass mark for the question page - if (questionPage == null) { - throw new ResourceNotFoundException(String.format("Unable to locate the question: %s for augmenting", - questionPageId)); - } + // Get the pass mark for the question page (already checked for null above). float passMark = questionPage.getPassMark() != null ? questionPage.getPassMark() : DEFAULT_QUESTION_PASS_MARK; gameItem.setPassMark(passMark); gameItem.setQuestionPartsCorrect(questionPartsCorrect); diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ContentIndexer.java b/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ContentIndexer.java index a5bd64d559..10763fb980 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ContentIndexer.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ContentIndexer.java @@ -141,41 +141,44 @@ void loadAndIndexContent(final String version) throws Exception { long endTime; totalStartTime = System.nanoTime(); - buildGitContentIndex(version, true, contentCache, tagsList, allUnits, publishedUnits, - indexProblemCache); - endTime = System.nanoTime(); + try { + buildGitContentIndex(version, true, contentCache, tagsList, allUnits, publishedUnits, + indexProblemCache); + endTime = System.nanoTime(); - log.info(CONTENT_LOG_PREFIX + "Finished populating Git content cache, took: {}ms", - (endTime - totalStartTime) / NANOSECONDS_IN_A_MILLISECOND); - log.info(CONTENT_LOG_PREFIX + "Beginning to record content errors"); + log.info(CONTENT_LOG_PREFIX + "Finished populating Git content cache, took: {}ms", + (endTime - totalStartTime) / NANOSECONDS_IN_A_MILLISECOND); + log.info(CONTENT_LOG_PREFIX + "Beginning to record content errors"); - startTime = System.nanoTime(); - recordContentErrors(version, contentCache, indexProblemCache); - endTime = System.nanoTime(); + startTime = System.nanoTime(); + recordContentErrors(version, contentCache, indexProblemCache); + endTime = System.nanoTime(); - log.info(CONTENT_LOG_PREFIX + "Finished recording content errors, took: {}ms", - (endTime - startTime) / NANOSECONDS_IN_A_MILLISECOND); + log.info(CONTENT_LOG_PREFIX + "Finished recording content errors, took: {}ms", + (endTime - startTime) / NANOSECONDS_IN_A_MILLISECOND); - startTime = System.nanoTime(); - buildElasticSearchIndex(version, contentCache, tagsList, allUnits, publishedUnits, - indexProblemCache); - endTime = System.nanoTime(); - long buildTime = (endTime - startTime) / NANOSECONDS_IN_A_MILLISECOND; - log.info(CONTENT_LOG_PREFIX + "Finished building ElasticSearch index, took: {}ms", buildTime); + startTime = System.nanoTime(); + buildElasticSearchIndex(version, contentCache, tagsList, allUnits, publishedUnits, + indexProblemCache); + endTime = System.nanoTime(); + long buildTime = (endTime - startTime) / NANOSECONDS_IN_A_MILLISECOND; + log.info(CONTENT_LOG_PREFIX + "Finished building ElasticSearch index, took: {}ms", buildTime); + + // Verify the version requested is now available + if (!allContentTypesAreIndexedForVersion(version)) { + expungeAnyContentTypeIndicesRelatedToVersion(version); + throw new Exception(String.format("Failed to index version %s. Don't know why.", version)); + } - // Verify the version requested is now available - if (!allContentTypesAreIndexedForVersion(version)) { - expungeAnyContentTypeIndicesRelatedToVersion(version); - throw new Exception(String.format("Failed to index version %s. Don't know why.", version)); + long totalTime = (endTime - totalStartTime) / NANOSECONDS_IN_A_MILLISECOND; + log.info(CONTENT_LOG_PREFIX + "Finished indexing version {}, total time: {}ms", + sanitiseInternalLogValue(version), totalTime); + } finally { + // Generate and log the indexing report whether indexing succeeded or failed, so that any + // validation problems collected before a failure are still reported. + generateIndexingReport(version, contentCache, indexProblemCache); } - long totalTime = (endTime - totalStartTime) / NANOSECONDS_IN_A_MILLISECOND; - log.info(CONTENT_LOG_PREFIX + "Finished indexing version {}, total time: {}ms", - sanitiseInternalLogValue(version), totalTime); - - // Generate and log indexing failure report - generateIndexingReport(version, contentCache, indexProblemCache); - } finally { VERSION_LOCKS.remove(version); } @@ -231,7 +234,7 @@ private synchronized void buildGitContentIndex(final String sha, throw new ContentManagerException("Failed to buildGitIndex - Unable to get tree walk for SHA: " + sha); } - log.info("Populating git content cache based on sha {} ...", sanitiseInternalLogValue(sha)); + log.info(CONTENT_LOG_PREFIX + "Populating git content cache based on sha {} ...", sanitiseInternalLogValue(sha)); // Traverse the git repository looking for the .json files IndexingContext context = new IndexingContext(contentCache, tagsList, allUnits, publishedUnits, @@ -241,11 +244,11 @@ private synchronized void buildGitContentIndex(final String sha, } repository.close(); - log.info("Tags available {}", tagsList); - log.info("All units: {}", allUnits); + log.info(CONTENT_LOG_PREFIX + "Tags available {}", tagsList); + log.info(CONTENT_LOG_PREFIX + "All units: {}", allUnits); } catch (IOException e) { - log.error("IOException while trying to access git repository. ", e); + log.error(CONTENT_LOG_PREFIX + "IOException while trying to access git repository. ", e); throw new ContentManagerException("Unable to index content, due to an IOException."); } } @@ -276,7 +279,7 @@ private void parseAndIndexJsonContent(final ObjectMapper objectMapper, final Str Content content = (Content) objectMapper.readValue(jsonContent, ContentBase.class); if (context.shouldSkipUnpublished(content)) { - log.info("Skipping unpublished content: {}", content.getId()); + log.info(CONTENT_LOG_PREFIX + "Skipping unpublished content: {}", content.getId()); return; } @@ -296,7 +299,7 @@ private void parseAndIndexJsonContent(final ObjectMapper objectMapper, final Str this.registerContentProblem(dummyContent, "Index failure - Unable to parse json file found - " + filePath + ERROR_OCCURRED_SUFFIX + e.getMessage(), context.indexProblemCache); } catch (IOException e) { - log.error("IOException while trying to parse {}", filePath, e); + log.error(CONTENT_LOG_PREFIX + "IOException while trying to parse {}", filePath, e); Content dummyContent = new Content(); dummyContent.setCanonicalSourceFile(filePath); this.registerContentProblem(dummyContent, @@ -333,7 +336,8 @@ private void validateAndCacheContent(final Content flattenedContent, final Conte if (flattenedContent instanceof IsaacQuiz) { List children = flattenedContent.getChildren(); if (children != null && children.stream().anyMatch(c -> !(c instanceof IsaacQuizSection))) { - log.info("IsaacQuiz ({}) contains top-level non-quiz sections. Skipping.", flattenedContent.getId()); + log.info(CONTENT_LOG_PREFIX + "IsaacQuiz ({}) contains top-level non-quiz sections. Skipping.", + flattenedContent.getId()); this.registerContentProblem(flattenedContent, "Index failure - Invalid " + "content type among quiz sections. Quizzes can only contain quiz sections " + "in the top-level children array.", context.indexProblemCache); @@ -342,14 +346,15 @@ private void validateAndCacheContent(final Content flattenedContent, final Conte } if (flattenedContent.getId().length() > MAXIMUM_CONTENT_ID_LENGTH) { - log.info("Content ID too long: {}", flattenedContent.getId()); + log.info(CONTENT_LOG_PREFIX + "Content ID too long: {}", flattenedContent.getId()); this.registerContentProblem(flattenedContent, "Content ID too long: " + flattenedContent.getId(), context.indexProblemCache); return; } if (flattenedContent.getId().contains(".")) { - log.info("Resource with invalid ID ({}) detected in cache. Skipping {}", parentContent.getId(), treeWalkPath); + log.info(CONTENT_LOG_PREFIX + "Resource with invalid ID ({}) detected in cache. Skipping {}", + parentContent.getId(), treeWalkPath); this.registerContentProblem(flattenedContent, "Index failure - Invalid ID " + flattenedContent.getId() + " found in file " + treeWalkPath + ". Must not contain restricted characters.", context.indexProblemCache); @@ -369,11 +374,13 @@ private void validateAndCacheContent(final Content flattenedContent, final Conte } if (context.contentCache.get(flattenedContent.getId()).equals(flattenedContent)) { - log.info("Resource ({}) already seen in cache. Skipping {}", parentContent.getId(), treeWalkPath); + log.info(CONTENT_LOG_PREFIX + "Resource ({}) already seen in cache. Skipping {}", + parentContent.getId(), treeWalkPath); return; } - log.info("Resource with duplicate ID ({}) detected in cache. Skipping {}", parentContent.getId(), treeWalkPath); + log.info(CONTENT_LOG_PREFIX + "Resource with duplicate ID ({}) detected in cache. Skipping {}", + parentContent.getId(), treeWalkPath); this.registerContentProblem(flattenedContent, String.format( "Index failure - Duplicate ID (%s) found in files (%s) and (%s): only one will be available.", parentContent.getId(), @@ -415,7 +422,7 @@ private Content augmentChildContent(final Content content, final String canonica // If this object is of type question then we need to give it a random // id if it doesn't have one. if (content instanceof Question && content.getId() == null) { - log.info("Found question without id {} {}", content.getTitle(), canonicalSourceFile); + log.info(CONTENT_LOG_PREFIX + "Found question without id {} {}", content.getTitle(), canonicalSourceFile); } String newParentId = computeParentId(parentId, content.getId()); @@ -498,7 +505,7 @@ private void augmentMediaFieldsViaReflection(final Content content, final String } } catch (SecurityException | IllegalAccessException | IllegalArgumentException | InvocationTargetException e) { - log.error("Unable to access method using reflection: attempting to fix Media Src", e); + log.error(CONTENT_LOG_PREFIX + "Unable to access method using reflection: attempting to fix Media Src", e); } }); } @@ -777,7 +784,7 @@ private void recordContentErrors(final String sha, final Map gi recordMissingContentProblems(refMap.expectedIds(), contentById, refMap.incomingReferences(), indexProblemCache); recordPublishedToUnpublishedReferenceProblems(refMap.incomingReferences(), contentById, indexProblemCache); - log.info("Validation processing ({}) complete. There are {} files with content problems", + log.info(CONTENT_LOG_PREFIX + "Validation processing ({}) complete. There are {} files with content problems", sanitiseInternalLogValue(sha), indexProblemCache.size()); if (indexProblemCache.isEmpty()) { @@ -798,7 +805,7 @@ private void recordContentErrors(final String sha, final Map gi * @param version the commit sha of the content that we are interested in. */ private void expungeAnyContentTypeIndicesRelatedToVersion(final String version) { - log.info("Deleting existing indexes for version {}", sanitiseInternalLogValue(version)); + log.info(CONTENT_LOG_PREFIX + "Deleting existing indexes for version {}", sanitiseInternalLogValue(version)); Arrays.stream(ContentIndextype.values()) .forEach(contentIndexType -> es.expungeIndexFromSearchCache(version, contentIndexType.toString())); } @@ -1184,7 +1191,7 @@ private void registerContentProblemValueWithChildren( + "Content objects are only allowed to have one or the other.", indexProblemCache); log.error( - "Invalid content item detected: The object with ID ({}) has both children and a value.", + CONTENT_LOG_PREFIX + "Invalid content item detected: The object with ID ({}) has both children and a value.", content.getCanonicalSourceFile() ); } @@ -1216,7 +1223,7 @@ private ContentReferenceMap buildReferenceMap(final String sha, final Set serializeUnits(final Map units, final Objec try { return objectMapper.writeValueAsString(Map.of("cleanKey", entry.getKey(), "unit", entry.getValue())); } catch (JsonProcessingException jsonProcessingException) { - log.error("Unable to serialise unit entry for unit: {}", entry.getValue()); + log.error(CONTENT_LOG_PREFIX + "Unable to serialise unit entry for unit: {}", entry.getValue()); return null; } }).filter(Objects::nonNull).toList(); @@ -1281,7 +1288,8 @@ private List serializeContentErrors(final Map> ind "published", e.getKey().getPublished() == null ? "" : e.getKey().getPublished(), "errors", e.getValue().toArray())); } catch (JsonProcessingException jsonProcessingException) { - log.error("Unable to serialise content error entry from file: {}", e.getKey().getCanonicalSourceFile()); + log.error(CONTENT_LOG_PREFIX + "Unable to serialise content error entry from file: {}", + e.getKey().getCanonicalSourceFile()); return null; } }).filter(Objects::nonNull).toList(); @@ -1298,7 +1306,7 @@ private List serializeContentErrors(final Map> ind private void generateIndexingReport(final String version, final Map contentCache, final Map> indexProblemCache) { if (indexProblemCache.isEmpty()) { - log.info(CONTENT_LOG_PREFIX + "✓ Indexing completed successfully with NO validation errors or warnings"); + log.info(CONTENT_LOG_PREFIX + "Indexing completed successfully with NO validation errors or warnings"); return; } @@ -1308,45 +1316,34 @@ private void generateIndexingReport(final String version, final Map>>> problemsByType = groupProblems(realProblems); - // Report each problem with details + // Report each problem with details, one log event per line int problemIndex = 1; for (Map.Entry>>> typeGroup : problemsByType.entrySet()) { - reportBuilder.append(String.format("%n[%s]%n", typeGroup.getKey())); + log.warn(CONTENT_LOG_PREFIX + "[{}]", typeGroup.getKey()); for (Map.Entry> problem : typeGroup.getValue()) { Content content = problem.getKey(); - List errors = problem.getValue(); - reportBuilder.append(String.format("%n %d. %s%n", problemIndex, content.getCanonicalSourceFile())); - if (content.getId() != null) { - reportBuilder.append(String.format(" ID: %s%n", content.getId())); - } - if (content.getTitle() != null) { - reportBuilder.append(String.format(" Title: %s%n", content.getTitle())); - } - reportBuilder.append(String.format(" Type: %s%n", content.getType())); - reportBuilder.append(String.format(" Published: %s%n", content.getPublished())); - reportBuilder.append(" Issues:\n"); + log.warn(CONTENT_LOG_PREFIX + "{}. {} | id={} | title={} | type={} | published={}", + problemIndex, content.getCanonicalSourceFile(), content.getId(), content.getTitle(), + content.getType(), content.getPublished()); - for (String error : errors) { - reportBuilder.append(String.format(" • %s%n", error)); + for (String error : problem.getValue()) { + log.warn(CONTENT_LOG_PREFIX + " - {}", error); } problemIndex++; @@ -1354,22 +1351,16 @@ private void generateIndexingReport(final String version, final Map>>> typeGroup : problemsByType.entrySet()) { int totalIssues = typeGroup.getValue().stream() .mapToInt(e -> e.getValue().size()) .sum(); - reportBuilder.append(String.format(" %-30s: %3d files, %3d total issues%n", - typeGroup.getKey(), typeGroup.getValue().size(), totalIssues)); + log.warn(CONTENT_LOG_PREFIX + " {}: {} files, {} total issues", + typeGroup.getKey(), typeGroup.getValue().size(), totalIssues); } - reportBuilder.append("-".repeat(100)).append("\n\n"); - - // Log the report - log.warn(reportBuilder.toString()); + log.warn(CONTENT_LOG_PREFIX + "===== INDEXING FAILURE REPORT END ====="); } /** diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ETLManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ETLManager.java index e358b00492..5a1fd73af4 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ETLManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ETLManager.java @@ -78,9 +78,9 @@ void indexContent() { try { this.setNamedVersion(entry.getKey(), entry.getValue()); } catch (VersionLockedException e) { - log.warn("Could not index new version, lock is already held by another thread."); + log.warn(CONTENT_LOG_PREFIX + "Could not index new version, lock is already held by another thread."); } catch (Exception e) { - log.error("Indexing version {} failed.", entry.getKey(), e); + log.error(CONTENT_LOG_PREFIX + "Indexing version {} failed.", entry.getKey(), e); } } diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/GameManagerTest.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/GameManagerTest.java index e161fd93ae..d66da0a8df 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/GameManagerTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/GameManagerTest.java @@ -21,24 +21,32 @@ import static org.easymock.EasyMock.anyObject; import static org.easymock.EasyMock.capture; import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.eq; import static org.easymock.EasyMock.expect; import static org.easymock.EasyMock.replay; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import java.time.Instant; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Objects; -import java.util.stream.Collectors; import org.easymock.Capture; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import uk.ac.cam.cl.dtg.isaac.dao.GameboardPersistenceManager; +import uk.ac.cam.cl.dtg.isaac.dos.LightweightQuestionValidationResponse; import uk.ac.cam.cl.dtg.isaac.dto.GameFilter; +import uk.ac.cam.cl.dtg.isaac.dto.GameboardDTO; +import uk.ac.cam.cl.dtg.isaac.dto.GameboardItem; +import uk.ac.cam.cl.dtg.isaac.dto.GameboardListDTO; import uk.ac.cam.cl.dtg.isaac.dto.IsaacQuestionPageDTO; import uk.ac.cam.cl.dtg.isaac.dto.ResultsWrapper; import uk.ac.cam.cl.dtg.isaac.dto.content.ContentDTO; +import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; import uk.ac.cam.cl.dtg.isaac.mappers.ContentMapper; import uk.ac.cam.cl.dtg.segue.api.Constants; import uk.ac.cam.cl.dtg.segue.api.managers.QuestionManager; @@ -46,10 +54,10 @@ import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager.BooleanSearchClause; +@SuppressWarnings("java:S8692") class GameManagerTest { private GitContentManager dummyContentManager; private GameboardPersistenceManager dummyGameboardPersistenceManager; - private ContentMapper dummyMapper; private QuestionManager dummyQuestionManager; private GameManager gameManager; @@ -57,12 +65,12 @@ class GameManagerTest { public void setUp() { this.dummyContentManager = createMock(GitContentManager.class); this.dummyGameboardPersistenceManager = createMock(GameboardPersistenceManager.class); - this.dummyMapper = createMock(ContentMapper.class); + ContentMapper dummyMapper = createMock(ContentMapper.class); this.dummyQuestionManager = createMock(QuestionManager.class); this.gameManager = new GameManager( this.dummyContentManager, this.dummyGameboardPersistenceManager, - this.dummyMapper, + dummyMapper, this.dummyQuestionManager ); } @@ -88,7 +96,7 @@ void getNextQuestionsForFilter_appliesExclusionFilterForDeprecatedQuestions() th // check that one of the filters sent to GitContentManager was the deprecated question exclusion filter List filters = capturedFilters.getValues().get(0); BooleanSearchClause deprecatedFilter = filters.stream() - .filter(f -> Objects.equals(f.getField(), "deprecated")).collect(Collectors.toList()).get(0); + .filter(f -> Objects.equals(f.getField(), "deprecated")).toList().get(0); assertNotNull(deprecatedFilter); assertEquals(Constants.BooleanOperator.NOT, deprecatedFilter.getOperator()); @@ -149,7 +157,7 @@ void generateRandomQuestions_appliesExclusionFilterForDeprecatedQuestions() thro // check that one of the filters sent to GitContentManager was the deprecated question exclusion filter List filters = capturedFilters.getValues().get(0); BooleanSearchClause deprecatedFilter = filters.stream() - .filter(f -> Objects.equals(f.getField(), "deprecated")).collect(Collectors.toList()).get(0); + .filter(f -> Objects.equals(f.getField(), "deprecated")).toList().get(0); assertNotNull(deprecatedFilter); assertEquals(Constants.BooleanOperator.NOT, deprecatedFilter.getOperator()); @@ -182,4 +190,40 @@ void generateRandomQuestions_appliesExclusionFilterForRegressionTest() throws assertEquals(Constants.BooleanOperator.NOT, tagsFilter.getOperator()); assertEquals(Collections.singletonList("regression_test"), tagsFilter.getValues()); } + + @Test + void getUsersGameboards_doesNotThrowWhenQuestionIdResolvesToNonQuestionContent() throws Exception { + RegisteredUserDTO user = new RegisteredUserDTO(); + user.setId(123L); + + GameboardItem brokenItem = new GameboardItem(); + brokenItem.setId("q1"); + + GameboardDTO board = new GameboardDTO(); + board.setId("board-1"); + board.setContents(new ArrayList<>(List.of(brokenItem))); + board.setCreationDate(Instant.now()); + board.setLastVisited(Instant.now()); + + List boards = new ArrayList<>(List.of(board)); + + expect(dummyGameboardPersistenceManager.getGameboardsByUserId(user)).andReturn(boards); + + Map>> noAttempts = new HashMap<>(); + expect(dummyQuestionManager.getMatchingQuestionAttempts(eq(user), anyObject())).andReturn(noAttempts); + + ContentDTO notAQuestionPage = new ContentDTO(); + notAQuestionPage.setId("q1"); + expect(dummyContentManager.getContentById("q1")).andStubReturn(notAQuestionPage); + + expect(dummyGameboardPersistenceManager.augmentGameboardItems(anyObject())).andStubReturn(boards); + + replay(dummyContentManager, dummyGameboardPersistenceManager, dummyQuestionManager); + + GameboardListDTO result = + gameManager.getUsersGameboards(user, 0, null, null, null); + + assertNotNull(result); + assertEquals(1, result.getResults().size()); + } }