diff --git a/config-templates/linux-local-dev-segue-config.properties b/config-templates/linux-local-dev-segue-config.properties index 9d8bca0d93..b4993f1037 100755 --- a/config-templates/linux-local-dev-segue-config.properties +++ b/config-templates/linux-local-dev-segue-config.properties @@ -19,7 +19,7 @@ EMAIL_SIGNATURE=Isaac Physics Project EVENT_ADMIN_EMAIL=events@isaacphysics.org EVENT_ICAL_UID_DOMAIN=isaacphysics.org -SCHOOL_CSV_LIST_PATH=/local/data/schools_list_2025_december.csv +SCHOOL_CSV_LIST_PATH=/local/data/schools_list_2026_spring.csv # Segue 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 c6a12b8c29..a963733e36 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 @@ -357,7 +357,7 @@ private static boolean validateFilterQuery(final GameFilter gameFilter) { * @param gameFilter Object representing the group of filters to use * @param boardOwner The user that should be marked as the creator of the gameBoard. * @return a gameboard if possible that satisfies the conditions provided by the parameters. Will return null if no - * questions can be provided. + * questions can be provided. * @throws NoWildcardException when we are unable to provide you with a wildcard object. * @throws SegueDatabaseException if there is an error contacting the database. * @throws ContentManagerException if there is an error retrieving the content requested. @@ -910,7 +910,7 @@ private GameboardDTO augmentGameboardWithQuestionAttemptInformationAndUserInform */ private GameboardDTO augmentGameboardWithQuestionAttemptInformation( final GameboardDTO gameboardDTO, final Map>> questionAttemptsFromUser) + ? extends List>> questionAttemptsFromUser) throws ContentManagerException { if (null == gameboardDTO) { return null; @@ -1155,6 +1155,13 @@ public List generateRandomQuestions(final GameFilter gameF List generatedQuestions = results.getResults(); List questionsToReturn = generatedQuestions.stream() + .filter(dto -> { + if (!(dto instanceof IsaacQuestionPageDTO)) { + log.warn("Skipping non-question DTO in random questions: {}", dto.getClass().getSimpleName()); + return false; + } + return true; + }) .map(IsaacQuestionPageDTO.class::cast) .filter(qp -> qp.getSupersededBy() == null || qp.getSupersededBy().isEmpty()) .collect(Collectors.toList()); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java index d4f6edb0d7..86441fffca 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java @@ -47,6 +47,7 @@ import uk.ac.cam.cl.dtg.segue.api.AuthenticationFacade; import uk.ac.cam.cl.dtg.segue.api.AuthorisationFacade; import uk.ac.cam.cl.dtg.segue.api.ContactFacade; +import uk.ac.cam.cl.dtg.segue.api.CorsFilter; import uk.ac.cam.cl.dtg.segue.api.EmailFacade; import uk.ac.cam.cl.dtg.segue.api.ExceptionSanitiser; import uk.ac.cam.cl.dtg.segue.api.GlossaryFacade; @@ -55,6 +56,7 @@ import uk.ac.cam.cl.dtg.segue.api.LogEventFacade; import uk.ac.cam.cl.dtg.segue.api.NotificationFacade; import uk.ac.cam.cl.dtg.segue.api.QuestionFacade; +import uk.ac.cam.cl.dtg.segue.api.SameSiteCookieFilter; import uk.ac.cam.cl.dtg.segue.api.SchoolLookupServiceFacade; import uk.ac.cam.cl.dtg.segue.api.SegueContentFacade; import uk.ac.cam.cl.dtg.segue.api.SegueDefaultFacade; @@ -127,9 +129,11 @@ public final Set getSingletons() { this.singletons.add(injector.getInstance(QuizFacade.class)); // initialise filters + this.singletons.add(injector.getInstance(CorsFilter.class)); this.singletons.add(injector.getInstance(PerformanceMonitor.class)); this.singletons.add(injector.getInstance(SessionValidator.class)); this.singletons.add(injector.getInstance(ExceptionSanitiser.class)); + this.singletons.add(injector.getInstance(SameSiteCookieFilter.class)); // initialise observers this.singletons.add(injector.getInstance(IGroupObserver.class)); diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/AuthenticationFacade.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/AuthenticationFacade.java index 0c3fbf3e4d..65efa7dcc0 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/api/AuthenticationFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/AuthenticationFacade.java @@ -189,6 +189,7 @@ private Response getUserAuthenticationSettings(final Long userId, final Register @Produces(MediaType.APPLICATION_JSON) @Operation(summary = "Get the SSO login redirect URL for an authentication provider.") public final Response authenticate(@Context final HttpServletRequest request, + @Context final HttpServletResponse response, @PathParam("provider") final String signinProvider) { if (userManager.isRegisteredUserLoggedIn(request)) { @@ -200,7 +201,7 @@ public final Response authenticate(@Context final HttpServletRequest request, try { Map redirectResponse = new ImmutableMap.Builder() - .put(REDIRECT_URL, userManager.authenticate(request, signinProvider)).build(); + .put(REDIRECT_URL, userManager.authenticate(request, response, signinProvider)).build(); return Response.ok(redirectResponse).build(); } catch (IOException e) { @@ -232,6 +233,7 @@ public final Response authenticate(@Context final HttpServletRequest request, description = "Very similar to the login case, but records this is a link request not an account creation" + " request.") public final Response linkExistingUserToProvider(@Context final HttpServletRequest request, + @Context final HttpServletResponse response, @PathParam("provider") final String authProviderAsString) { if (!this.userManager.isRegisteredUserLoggedIn(request)) { return SegueErrorResponse.getNotLoggedInResponse(); @@ -239,7 +241,7 @@ public final Response linkExistingUserToProvider(@Context final HttpServletReque try { Map redirectResponse = new ImmutableMap.Builder() - .put(REDIRECT_URL, this.userManager.initiateLinkAccountToUserFlow(request, authProviderAsString)) + .put(REDIRECT_URL, this.userManager.initiateLinkAccountToUserFlow(request, response, authProviderAsString)) .build(); return Response.ok(redirectResponse).build(); diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/Constants.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/Constants.java index 3bb40e0806..50b4198b8f 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/api/Constants.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/Constants.java @@ -615,6 +615,8 @@ public static SchoolInfoStatus get(final boolean schoolIdProvided, final boolean public static final String SEGUE_AUTH_COOKIE = "SEGUE_AUTH_COOKIE"; public static final String JSESSION_COOOKIE = "JSESSIONID"; + public static final String OAUTH_STATE_COOKIE = "OAUTH_STATE"; + public static final int OAUTH_STATE_COOKIE_TTL_SECONDS = 600; public static final String DEFAULT_DATE_FORMAT = "EEE MMM dd HH:mm:ss Z yyyy"; diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/CorsFilter.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/CorsFilter.java new file mode 100644 index 0000000000..050af25627 --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/CorsFilter.java @@ -0,0 +1,85 @@ +package uk.ac.cam.cl.dtg.segue.api; + +import com.google.inject.Inject; +import jakarta.ws.rs.container.ContainerRequestContext; +import jakarta.ws.rs.container.ContainerRequestFilter; +import jakarta.ws.rs.container.ContainerResponseContext; +import jakarta.ws.rs.container.ContainerResponseFilter; +import jakarta.ws.rs.container.PreMatching; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.ext.Provider; +import java.io.IOException; + +/** + * CORS Filter for ALB migration. + * Emits CORS headers from the application instead of relying on nginx ingress annotations. + * This allows the API to work with AWS ALB which doesn't have a built-in CORS module. + */ +@Provider +@PreMatching +public class CorsFilter implements ContainerRequestFilter, ContainerResponseFilter { + + private static final String DEFAULT_ALLOWED_ORIGINS = "https://*.isaaccomputerscience.org"; + + private final String allowedOrigins; + + @Inject + public CorsFilter() { + this.allowedOrigins = DEFAULT_ALLOWED_ORIGINS; + } + + @Override + public void filter(final ContainerRequestContext requestContext) throws IOException { + if (requestContext.getMethod().equalsIgnoreCase("OPTIONS")) { + requestContext.abortWith( + Response.ok() + .header("Access-Control-Allow-Origin", getAllowedOrigin(requestContext)) + .header("Access-Control-Allow-Methods", + "GET, POST, PUT, DELETE, OPTIONS, PATCH") + .header("Access-Control-Allow-Headers", + "Origin, X-Requested-With, Content-Type, Accept, Authorization, X-Api-Token") + .header("Access-Control-Allow-Credentials", "true") + .header("Access-Control-Max-Age", "3600") + .build()); + } + } + + @Override + public void filter(final ContainerRequestContext requestContext, + final ContainerResponseContext responseContext) throws IOException { + if (requestContext.getMethod().equalsIgnoreCase("OPTIONS")) { + // Request filter already set all headers via abortWith(). Running here too would duplicate them, + // which causes Safari/mobile to fail the CORS preflight check. + return; + } + String allowedOrigin = getAllowedOrigin(requestContext); + responseContext.getHeaders().add("Access-Control-Allow-Origin", allowedOrigin); + responseContext.getHeaders().add("Access-Control-Allow-Methods", + "GET, POST, PUT, DELETE, OPTIONS, PATCH"); + responseContext.getHeaders().add("Access-Control-Allow-Headers", + "Origin, X-Requested-With, Content-Type, Accept, Authorization, X-Api-Token"); + responseContext.getHeaders().add("Access-Control-Allow-Credentials", "true"); + } + + /** + * Validate and return the allowed origin from the request. + * Currently, allows all requests from Isaac domains (*.isaaccomputerscience.org). + * + * @param requestContext the request context + * @return the allowed origin, or * if validation fails + */ + private String getAllowedOrigin(final ContainerRequestContext requestContext) { + String origin = requestContext.getHeaderString("Origin"); + + if (origin == null) { + return allowedOrigins; + } + + // The allowedOrigins property can be configured as a regex or comma-separated list if needed. + if (origin.contains("isaaccomputerscience.org") || origin.contains("localhost")) { + return origin; + } + + return allowedOrigins; + } +} \ No newline at end of file diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/SameSiteCookieFilter.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/SameSiteCookieFilter.java new file mode 100644 index 0000000000..d44aafebbe --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/SameSiteCookieFilter.java @@ -0,0 +1,43 @@ +package uk.ac.cam.cl.dtg.segue.api; + +import jakarta.ws.rs.container.ContainerRequestContext; +import jakarta.ws.rs.container.ContainerResponseContext; +import jakarta.ws.rs.container.ContainerResponseFilter; +import jakarta.ws.rs.ext.Provider; +import java.util.ArrayList; +import java.util.List; + +@Provider +public class SameSiteCookieFilter implements ContainerResponseFilter { + + private static final String SAME_SITE_NONE = "__SAME_SITE_NONE__"; + private static final String SAME_SITE_LAX = "__SAME_SITE_LAX__"; + + @Override + public void filter(final ContainerRequestContext requestContext, + final ContainerResponseContext responseContext) { + + @SuppressWarnings("unchecked") + List setCookieHeaders = (List) (List) responseContext.getHeaders().get("Set-Cookie"); + if (setCookieHeaders == null || setCookieHeaders.isEmpty()) { + return; + } + + List updatedHeaders = new ArrayList<>(); + for (String cookieHeader : setCookieHeaders) { + String updatedHeader = addSameSiteAttribute(cookieHeader); + updatedHeaders.add(updatedHeader); + } + + responseContext.getHeaders().put("Set-Cookie", (List) (List) updatedHeaders); + } + + private String addSameSiteAttribute(final String cookieHeader) { + if (cookieHeader.contains(SAME_SITE_NONE)) { + return cookieHeader.replace(SAME_SITE_NONE, "").trim() + "; SameSite=None"; + } else if (cookieHeader.contains(SAME_SITE_LAX)) { + return cookieHeader.replace(SAME_SITE_LAX, "").trim() + "; SameSite=Lax"; + } + return cookieHeader; + } +} diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java index 7ac095e882..77af2fb26e 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java @@ -19,7 +19,12 @@ import com.mailjet.client.errors.MailjetClientCommunicationException; import com.mailjet.client.errors.MailjetException; import com.mailjet.client.errors.MailjetRateLimitException; +import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.Optional; +import org.json.JSONException; import org.json.JSONObject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -28,10 +33,16 @@ import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; import uk.ac.cam.cl.dtg.segue.dao.users.IExternalAccountDataManager; import uk.ac.cam.cl.dtg.util.email.MailJetApiClientWrapper; +import uk.ac.cam.cl.dtg.util.email.MailJetApiClientWrapper.JobStatus; import uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction; public class ExternalAccountManager implements IExternalAccountManager { private static final Logger log = LoggerFactory.getLogger(ExternalAccountManager.class); + private static final String MAILJET = "MAILJET - "; + private static final int BULK_BATCH_SIZE = 100; + private static final int RATE_LIMIT_RETRY_SLEEP_MS = 10000; // 10 seconds + private static final int JOB_POLL_INTERVAL_MS = 5000; // 5 seconds between polls + private static final int JOB_POLL_MAX_ATTEMPTS = 60; // max 5 minutes (60 × 5s) private final IExternalAccountDataManager database; private final MailJetApiClientWrapper mailjetApi; @@ -57,194 +68,372 @@ public ExternalAccountManager(final MailJetApiClientWrapper mailjetApi, final IE * @throws ExternalAccountSynchronisationException on unrecoverable errors with external providers. */ @Override - public synchronized void synchroniseChangedUsers() throws ExternalAccountSynchronisationException { - log.info("Starting Mailjet synchronization process"); + public synchronized SyncResult synchroniseChangedUsers() throws ExternalAccountSynchronisationException { + log.info("{}Starting Mailjet synchronization process", MAILJET); - List userRecordsToUpdate; + List userRecordsToUpdate = getRecentlyChangedUsersOrThrow(); + if (userRecordsToUpdate.isEmpty()) { + log.info("{}No users to synchronize", MAILJET); + return new SyncResult(List.of(), 0, 0); + } + + SyncMetrics metrics = new SyncMetrics(); + List successfullyProcessedUserIds = new ArrayList<>(); + List failedUsers = new ArrayList<>(); + + List usersToDelete = new ArrayList<>(); + List usersToSync = new ArrayList<>(); + partitionUsersByType(userRecordsToUpdate, usersToDelete, usersToSync); + + processDeletions(usersToDelete, metrics, successfullyProcessedUserIds); + processBulkSyncsWithJobTracking(usersToSync, metrics, successfullyProcessedUserIds, failedUsers); + markSuccessfullyProcessedAsSynced(successfullyProcessedUserIds, metrics); + logFailedUsers(failedUsers); + + logSyncSummary(metrics, userRecordsToUpdate.size()); + + // Build sync result with list of failed users + List failedUserDetails = failedUsers.stream() + .map(u -> "ID: " + u.getUserId() + ", email: " + u.getAccountEmail()) + .toList(); + return new SyncResult(failedUserDetails, metrics.getSuccessCount(), userRecordsToUpdate.size()); + } + + /** + * Retrieve recently changed users from database. + */ + private List getRecentlyChangedUsersOrThrow() + throws ExternalAccountSynchronisationException { try { - userRecordsToUpdate = database.getRecentlyChangedRecords(); - log.info("Found {} users to synchronize with Mailjet", userRecordsToUpdate.size()); + List users = database.getRecentlyChangedRecords(); + log.info("{}Found {} users to synchronize with Mailjet", MAILJET, users.size()); + return users; } catch (SegueDatabaseException e) { throw new ExternalAccountSynchronisationException("Failed to retrieve users for synchronization" + e.getMessage()); } + } - if (userRecordsToUpdate.isEmpty()) { - log.info("No users to synchronize"); - return; + /** + * Partition users into deletions and syncs. + */ + private void partitionUsersByType(final List userRecords, + final List usersToDelete, + final List usersToSync) { + for (UserExternalAccountChanges userRecord : userRecords) { + if (Boolean.TRUE.equals(userRecord.isDeleted()) && userRecord.getProviderUserId() != null) { + usersToDelete.add(userRecord); + } else { + usersToSync.add(userRecord); + } } + } - SyncMetrics metrics = new SyncMetrics(); - - for (UserExternalAccountChanges userRecord : userRecordsToUpdate) { - Long userId = userRecord.getUserId(); - + /** + * Process user deletions with error handling and backoff. + */ + private void processDeletions(final List usersToDelete, final SyncMetrics metrics, + final List successfullyProcessedUserIds) + throws ExternalAccountSynchronisationException { + for (UserExternalAccountChanges userRecord : usersToDelete) { try { - processUserSync(userRecord, metrics); - metrics.incrementSuccess(); - + deleteUserFromMailJetWithBackoff(userRecord.getProviderUserId(), userRecord); + database.updateProviderLastUpdated(userRecord.getUserId()); + metrics.incrementDeleted(); + successfullyProcessedUserIds.add(userRecord.getUserId()); } catch (SegueDatabaseException e) { metrics.incrementDatabaseError(); - log.error("Database error storing Mailjet update for user ID: {}", userId, e); + log.error("{}Database error during deletion for user ID: {}", MAILJET, userRecord.getUserId(), e); } catch (MailjetClientCommunicationException e) { metrics.incrementCommunicationError(); throw new ExternalAccountSynchronisationException("Failed to connect to Mailjet: " + e.getMessage()); - } catch (MailjetRateLimitException e) { - metrics.incrementRateLimitError(); - log.warn("Mailjet rate limit exceeded while processing user ID: {}. Processed {} users before limit", - userId, metrics.getSuccessCount()); - throw new ExternalAccountSynchronisationException( - "Mailjet API rate limits exceeded after processing " + metrics.getSuccessCount() + " users" + e); - } catch (MailjetException e) { metrics.incrementMailjetError(); - log.error("Mailjet API error while processing user ID: {}. Continuing with next user", userId, e); - } catch (Exception e) { - metrics.incrementUnexpectedError(); - log.error("Unexpected error processing user ID: {}", userId, e); + log.error("{}Mailjet API error during deletion for user ID: {}. Continuing.", + MAILJET, userRecord.getUserId(), e); } } - - logSyncSummary(metrics, userRecordsToUpdate.size()); } /** - * Process synchronization for a single user. + * Process bulk syncs with job tracking, polling, and per-user recovery. + * Groups users by subscription state, submits batches, polls for completion, + * and handles errors by verifying users individually at Mailjet. */ - private void processUserSync(UserExternalAccountChanges userRecord, SyncMetrics metrics) - throws SegueDatabaseException, MailjetException { - - Long userId = userRecord.getUserId(); - String accountEmail = userRecord.getAccountEmail(); + private void processBulkSyncsWithJobTracking(final List usersToSync, + final SyncMetrics metrics, + final List successfullyProcessedUserIds, + final List failedUsers) + throws ExternalAccountSynchronisationException { + Map> groupedUsers = + groupUsersBySubscriptionState(usersToSync); - if (accountEmail == null || accountEmail.trim().isEmpty()) { - log.warn("User ID {} has null or empty email address. Skipping", userId); - metrics.incrementSkipped(); - return; + List submittedJobs = new ArrayList<>(); + for (Map.Entry> entry : groupedUsers.entrySet()) { + List groupJobs = submitBatchesForGroup(entry.getKey(), entry.getValue()); + submittedJobs.addAll(groupJobs); } - boolean accountEmailDeliveryFailed = - EmailVerificationStatus.DELIVERY_FAILED.equals(userRecord.getEmailVerificationStatus()); - String mailjetId = userRecord.getProviderUserId(); - - if (mailjetId != null && !mailjetId.trim().isEmpty()) { - handleExistingMailjetUser(mailjetId, userRecord, accountEmail, accountEmailDeliveryFailed, metrics); - } else { - handleNewMailjetUser(userRecord, accountEmail, accountEmailDeliveryFailed, metrics); + try { + processAllBatches(submittedJobs, successfullyProcessedUserIds, failedUsers, metrics); + } catch (ExternalAccountSynchronisationException e) { + throw e; + } catch (Exception e) { + metrics.incrementUnexpectedError(); + log.error("{}Unexpected error during bulk sync", MAILJET, e); } + } - database.updateProviderLastUpdated(userId); + /** + * Poll and process completed batch jobs, tracking successes and failures. + */ + private void processAllBatches(final List submittedJobs, + final List successfullyProcessedUserIds, + final List failedUsers, + final SyncMetrics metrics) + throws ExternalAccountSynchronisationException { + for (BatchJob batch : submittedJobs) { + Optional status = pollJobToCompletion(batch.jobId(), batch.users().size()); + if (status.isEmpty()) { + log.warn("{}Job {} timed out. Treating all {} users as failed.", MAILJET, batch.jobId(), batch.users().size()); + failedUsers.addAll(batch.users()); + metrics.incrementUnexpectedError(batch.users().size()); + continue; + } + processCompletedJob(batch, status.get(), successfullyProcessedUserIds, failedUsers, metrics); + } } /** - * Handle synchronization for users that already exist in Mailjet. + * Submit batches for a subscription group and return list of submitted jobs. */ - private void handleExistingMailjetUser(String mailjetId, UserExternalAccountChanges userRecord, - String accountEmail, boolean accountEmailDeliveryFailed, SyncMetrics metrics) - throws SegueDatabaseException, MailjetException { + private List submitBatchesForGroup(final SubscriptionGroup group, + final List groupUsers) + throws ExternalAccountSynchronisationException { + List submittedJobs = new ArrayList<>(); - Long userId = userRecord.getUserId(); - JSONObject mailjetDetails = mailjetApi.getAccountByIdOrEmail(mailjetId); + for (int i = 0; i < groupUsers.size(); i += BULK_BATCH_SIZE) { + int endIndex = Math.min(i + BULK_BATCH_SIZE, groupUsers.size()); + List batch = groupUsers.subList(i, endIndex); - if (mailjetDetails == null) { - log.warn("User ID {} has Mailjet ID {} but account not found. Treating as new user", userId, mailjetId); - database.updateExternalAccount(userId, null); - handleNewMailjetUser(userRecord, accountEmail, accountEmailDeliveryFailed, metrics); - return; + try { + String jobId = mailjetApi.bulkSyncUsers(batch, group.newsAction, group.eventsAction); + if (jobId != null && !jobId.trim().isEmpty()) { + log.debug("{}Batch submitted: news={}, events={} for {} users (job ID: {})", + MAILJET, group.newsAction, group.eventsAction, batch.size(), jobId); + submittedJobs.add(new BatchJob(jobId, group, new ArrayList<>(batch))); + } else { + log.warn("{}Bulk sync returned null/empty job ID for {} users. Continuing.", MAILJET, batch.size()); + } + } catch (MailjetRateLimitException e) { + log.warn("{}Mailjet rate limit exceeded during batch submission.", MAILJET); + throw new ExternalAccountSynchronisationException("Mailjet API rate limits exceeded: " + e.getMessage()); + } catch (MailjetClientCommunicationException e) { + log.error("{}Communication error during batch submission for {} users.", MAILJET, batch.size(), e); + throw new ExternalAccountSynchronisationException("Failed to connect to Mailjet: " + e.getMessage()); + } catch (MailjetException e) { + log.error("{}Mailjet API error during batch submission of {} users. Continuing with next batch.", + MAILJET, batch.size(), e); + } } - if (userRecord.isDeleted()) { - deleteUserFromMailJet(mailjetId, userRecord); - metrics.incrementDeleted(); - - } else if (accountEmailDeliveryFailed) { - log.info("User ID {} has delivery failed status. Unsubscribing from all lists", userId); - mailjetApi.updateUserSubscriptions(mailjetId, - MailJetSubscriptionAction.REMOVE, - MailJetSubscriptionAction.REMOVE); - metrics.incrementUnsubscribed(); - - } else if (!accountEmail.equalsIgnoreCase(mailjetDetails.getString("Email"))) { - log.info("User ID {} changed email. Recreating Mailjet account", userId); - mailjetApi.permanentlyDeleteAccountById(mailjetId); - String newMailjetId = mailjetApi.addNewUserOrGetUserIfExists(accountEmail); - - if (newMailjetId == null) { - throw new MailjetException("Failed to create new Mailjet account after email change for user: " + userId); + return submittedJobs; + } + + /** + * Poll a job until it completes or times out. + * Returns Optional.empty() if job times out; otherwise returns the final JobStatus. + * Fails fast on repeated rate limiting (2+ consecutive rate limits). + */ + private Optional pollJobToCompletion(final String jobId, final int expectedCount) + throws ExternalAccountSynchronisationException { + int consecutiveRateLimits = 0; + + for (int attempt = 0; attempt < JOB_POLL_MAX_ATTEMPTS; attempt++) { + try { + JobStatus status = mailjetApi.getBulkJobStatus(jobId); + consecutiveRateLimits = 0; // Reset on successful poll + + if (status.isComplete() || status.hasFailed()) { + log.info("{}Job {} polling complete - " + + "Status: {}, " + + "Processed: {}/{}, " + + "Inserted: {}, " + + "Updated: {}, " + + "Unchanged: {}, " + + "Errors: {}", + MAILJET, jobId, status.status(), + status.processed(), + expectedCount, + status.inserted(), + status.updated(), + status.unchanged(), + status.errors()); + return Optional.of(status); + } + log.info("{}Job {} polling (attempt {}/{}). Status: {}, Processed: {}/{}, Errors: {}", + MAILJET, jobId, attempt + 1, + JOB_POLL_MAX_ATTEMPTS, + status.status(), + status.processed(), + expectedCount, + status.errors()); + + Thread.sleep(JOB_POLL_INTERVAL_MS); + } catch (MailjetRateLimitException e) { + consecutiveRateLimits++; + log.warn("{}Rate limit during job {} polling (attempt {}). Consecutive limits: {}", + MAILJET, jobId, attempt + 1, consecutiveRateLimits); + + if (consecutiveRateLimits >= 2) { + log.error("{}Job {} hit rate limit {} times. Failing fast to respect API limits.", + MAILJET, jobId, consecutiveRateLimits); + throw new ExternalAccountSynchronisationException( + "Mailjet API rate limit exceeded during job polling for job " + jobId); + } + } catch (MailjetException e) { + consecutiveRateLimits = 0; // Reset on non-rate-limit errors + log.warn("{}Error polling job {}: {}. Continuing with next attempt.", MAILJET, jobId, e.getMessage()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + log.warn("{}Polling interrupted for job {}.", MAILJET, jobId); + return Optional.empty(); } + } - updateUserOnMailJet(newMailjetId, userRecord); - metrics.incrementEmailChanged(); + log.warn("{}Job {} timed out after {} attempts.", MAILJET, jobId, JOB_POLL_MAX_ATTEMPTS); + return Optional.empty(); + } + /** + * Process a completed job: happy path (0 errors) or error path (recover per-user). + */ + private void processCompletedJob(final BatchJob batch, final JobStatus status, + final List successfullyProcessedUserIds, + final List failedUsers, + final SyncMetrics metrics) { + if (status.errors() == 0) { + log.info("{}Job {} completed successfully with {} users (inserted: {}, updated: {}, unchanged: {})", + MAILJET, batch.jobId(), batch.users().size(), status.inserted(), status.updated(), status.unchanged()); + for (UserExternalAccountChanges user : batch.users()) { + successfullyProcessedUserIds.add(user.getUserId()); + metrics.incrementSuccess(); + } + metrics.addCreatedCount(status.inserted()); + metrics.addUpdatedCount(status.updated()); } else { - updateUserOnMailJet(mailjetId, userRecord); - metrics.incrementUpdated(); + log.warn("{}Job {} completed with {} errors. Recovering per-user.", + MAILJET, batch.jobId(), status.errors()); + recoverUsersFromFailedJob(batch, successfullyProcessedUserIds, failedUsers, metrics); } } /** - * Handle synchronization for users that don't exist in Mailjet yet. + * Recover users from a failed job by querying each user individually at Mailjet. + * Verifies that user data (name, role, verification status, stage) was correctly synced. */ - private void handleNewMailjetUser(UserExternalAccountChanges userRecord, - String accountEmail, boolean accountEmailDeliveryFailed, SyncMetrics metrics) - throws SegueDatabaseException, MailjetException { - - Long userId = userRecord.getUserId(); + private void recoverUsersFromFailedJob(final BatchJob batch, + final List successfullyProcessedUserIds, + final List failedUsers, + final SyncMetrics metrics) { + for (UserExternalAccountChanges user : batch.users()) { + try { + JSONObject mailjetContact = mailjetApi.getAccountByIdOrEmail(user.getAccountEmail()); + if (mailjetContact != null && isUserDataCorrectInMailjet(mailjetContact, user)) { + log.debug("{}User ID {} verified in Mailjet after job error.", MAILJET, user.getUserId()); + successfullyProcessedUserIds.add(user.getUserId()); + metrics.incrementSuccess(); + } else { + log.warn("{}User ID {} ({}) data mismatch or not found in Mailjet after job error.", + MAILJET, user.getUserId(), user.getAccountEmail()); + failedUsers.add(user); + metrics.incrementMailjetError(); + } + } catch (MailjetException e) { + log.warn("{}Failed to verify user ID {} ({}) at Mailjet: {}", + MAILJET, user.getUserId(), user.getAccountEmail(), e.getMessage()); + failedUsers.add(user); + metrics.incrementMailjetError(); + } + } + } - if (!accountEmailDeliveryFailed && !userRecord.isDeleted()) { - log.info("Creating new Mailjet account for user ID {}", userId); + /** + * Verify that user data in Mailjet matches what we expect. + * Checks: givenName, role, emailVerificationStatus, stage. + */ + private boolean isUserDataCorrectInMailjet(final JSONObject mailjetContact, + final UserExternalAccountChanges user) { + try { + String mailjetName = mailjetContact.optString("Name", ""); + String expectedName = user.getGivenName() != null ? user.getGivenName() : ""; - String mailjetId = mailjetApi.addNewUserOrGetUserIfExists(accountEmail); + JSONObject properties = mailjetContact.optJSONObject("Properties"); + if (properties == null) { + log.debug("{}No properties found in Mailjet contact for user ID {}.", MAILJET, user.getUserId()); + return false; + } - if (mailjetId == null) { - throw new MailjetException("Mailjet returned null ID when creating account for user: " + userId); + String mailjetRole = properties.optString("role", ""); + String expectedRole = user.getRole().toString(); + + String mailjetVerification = properties.optString("verification_status", ""); + String expectedVerification = user.getEmailVerificationStatus().toString(); + + String mailjetStage = properties.optString("stage", ""); + String expectedStage = user.getStage() != null ? user.getStage() : "unknown"; + + boolean nameMatches = mailjetName.equals(expectedName); + boolean roleMatches = mailjetRole.equals(expectedRole); + boolean verificationMatches = mailjetVerification.equals(expectedVerification); + boolean stageMatches = mailjetStage.equals(expectedStage); + + if (!nameMatches || !roleMatches || !verificationMatches || !stageMatches) { + if (log.isDebugEnabled()) { + log.debug("{}User ID {} data mismatch: name ({}/{}) role ({}/{}) verification ({}/{}) stage ({}/{})", + MAILJET, user.getUserId(), + mailjetName, expectedName, + mailjetRole, expectedRole, + mailjetVerification, expectedVerification, + mailjetStage, expectedStage); + } + return false; } - updateUserOnMailJet(mailjetId, userRecord); - metrics.incrementCreated(); + return true; - } else { - log.debug("User ID {} not eligible for Mailjet (deleted={}, deliveryFailed={}). Skipping", - userId, userRecord.isDeleted(), accountEmailDeliveryFailed); - database.updateExternalAccount(userId, null); - metrics.incrementSkipped(); + } catch (JSONException e) { + log.warn("{}JSON error checking user data for user ID {}: {}", MAILJET, user.getUserId(), e.getMessage()); + return false; } } /** - * Update user details and subscriptions in Mailjet. + * Log failed users that could not be synced to Mailjet. */ - private void updateUserOnMailJet(final String mailjetId, final UserExternalAccountChanges userRecord) - throws SegueDatabaseException, MailjetException { - - if (mailjetId == null || mailjetId.trim().isEmpty()) { - throw new IllegalArgumentException("Mailjet ID cannot be null or empty"); + private void logFailedUsers(final List failedUsers) { + if (failedUsers.isEmpty()) { + return; } + log.warn("{}=== {} users failed Mailjet sync ===", MAILJET, failedUsers.size()); + for (UserExternalAccountChanges user : failedUsers) { + log.warn("{}Failed user - ID: {}, email: {}", MAILJET, user.getUserId(), user.getAccountEmail()); + } + } - Long userId = userRecord.getUserId(); - String stage = userRecord.getStage() != null ? userRecord.getStage() : "unknown"; - - mailjetApi.updateUserProperties( - mailjetId, - userRecord.getGivenName(), - userRecord.getRole().toString(), - userRecord.getEmailVerificationStatus().toString(), - stage - ); - - MailJetSubscriptionAction newsStatus = Boolean.TRUE.equals(userRecord.allowsNewsEmails()) - ? MailJetSubscriptionAction.FORCE_SUBSCRIBE - : MailJetSubscriptionAction.UNSUBSCRIBE; - - MailJetSubscriptionAction eventsStatus = Boolean.TRUE.equals(userRecord.allowsEventsEmails()) - ? MailJetSubscriptionAction.FORCE_SUBSCRIBE - : MailJetSubscriptionAction.UNSUBSCRIBE; - - mailjetApi.updateUserSubscriptions(mailjetId, newsStatus, eventsStatus); - database.updateExternalAccount(userId, mailjetId); - - log.debug("Updated Mailjet account {} for user ID {} (news={}, events={})", - mailjetId, userId, newsStatus, eventsStatus); + /** + * Mark successfully processed users as synced in database. + */ + private void markSuccessfullyProcessedAsSynced(final List successfullyProcessedUserIds, + final SyncMetrics metrics) { + try { + if (!successfullyProcessedUserIds.isEmpty()) { + database.batchMarkAsSynced(successfullyProcessedUserIds); + } + } catch (SegueDatabaseException e) { + metrics.incrementDatabaseError(); + log.error("{}Database error marking {} users as synced", MAILJET, successfullyProcessedUserIds.size(), e); + } } /** @@ -254,7 +443,7 @@ private void deleteUserFromMailJet(final String mailjetId, final UserExternalAcc throws SegueDatabaseException, MailjetException { if (mailjetId == null || mailjetId.trim().isEmpty()) { - log.warn("Attempted to delete user with null/empty Mailjet ID. User ID: {}", userRecord.getUserId()); + log.warn("{}Attempted to delete user with null/empty Mailjet ID. User ID: {}", MAILJET, userRecord.getUserId()); return; } @@ -262,29 +451,94 @@ private void deleteUserFromMailJet(final String mailjetId, final UserExternalAcc mailjetApi.permanentlyDeleteAccountById(mailjetId); database.updateExternalAccount(userId, null); - log.info("Deleted Mailjet account {} for user ID {} (GDPR deletion)", mailjetId, userId); + log.info("{}Deleted Mailjet account {} for user ID {} (GDPR deletion)", MAILJET, mailjetId, userId); + } + + /** + * Delete user from Mailjet with exponential backoff on rate limit. + */ + private void deleteUserFromMailJetWithBackoff(final String mailjetId, + final UserExternalAccountChanges userRecord) + throws SegueDatabaseException, MailjetException { + try { + deleteUserFromMailJet(mailjetId, userRecord); + } catch (MailjetRateLimitException e) { + log.warn("{}Rate limit on deletion, retrying after backoff...", MAILJET); + try { + Thread.sleep(RATE_LIMIT_RETRY_SLEEP_MS); + deleteUserFromMailJet(mailjetId, userRecord); + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + throw new MailjetException("Interrupted during deletion backoff", ie); + } + } + } + + /** + * Group users by subscription state combination. + */ + private Map> + groupUsersBySubscriptionState(final List users) { + Map> groups = new HashMap<>(); + + for (UserExternalAccountChanges user : users) { + if (user.getAccountEmail() == null || user.getAccountEmail().trim().isEmpty()) { + log.warn("{}User ID {} has null/empty email. Skipping.", MAILJET, user.getUserId()); + continue; + } + + boolean deliveryFailed = + EmailVerificationStatus.DELIVERY_FAILED.equals(user.getEmailVerificationStatus()); + + MailJetSubscriptionAction newsAction; + MailJetSubscriptionAction eventsAction; + + if (deliveryFailed) { + newsAction = MailJetSubscriptionAction.REMOVE; + eventsAction = MailJetSubscriptionAction.REMOVE; + } else { + newsAction = Boolean.TRUE.equals(user.allowsNewsEmails()) + ? MailJetSubscriptionAction.FORCE_SUBSCRIBE + : MailJetSubscriptionAction.UNSUBSCRIBE; + eventsAction = Boolean.TRUE.equals(user.allowsEventsEmails()) + ? MailJetSubscriptionAction.FORCE_SUBSCRIBE + : MailJetSubscriptionAction.UNSUBSCRIBE; + } + + SubscriptionGroup group = new SubscriptionGroup(newsAction, eventsAction); + groups.computeIfAbsent(group, k -> new ArrayList<>()).add(user); + } + + return groups; } /** * Log summary of synchronization results. */ private void logSyncSummary(SyncMetrics metrics, int totalUsers) { - log.info("=== Mailjet Synchronization Complete ==="); - log.info("Total users to process: {}", totalUsers); - log.info("Successfully processed: {}", metrics.getSuccessCount()); - log.info(" - Created: {}", metrics.getCreatedCount()); - log.info(" - Updated: {}", metrics.getUpdatedCount()); - log.info(" - Deleted: {}", metrics.getDeletedCount()); - log.info(" - Email changed: {}", metrics.getEmailChangedCount()); - log.info(" - Unsubscribed: {}", metrics.getUnsubscribedCount()); - log.info(" - Skipped: {}", metrics.getSkippedCount()); - log.info("Errors:"); - log.info(" - Database errors: {}", metrics.getDatabaseErrorCount()); - log.info(" - Communication errors: {}", metrics.getCommunicationErrorCount()); - log.info(" - Rate limit errors: {}", metrics.getRateLimitErrorCount()); - log.info(" - Mailjet API errors: {}", metrics.getMailjetErrorCount()); - log.info(" - Unexpected errors: {}", metrics.getUnexpectedErrorCount()); - log.info("========================================"); + log.info("{}=== Mailjet Synchronization Complete ===", MAILJET); + log.info("{}Total users to process: {}", MAILJET, totalUsers); + log.info("{}Successfully processed: {}", MAILJET, metrics.getSuccessCount()); + log.info("{} - Created: {}", MAILJET, metrics.getCreatedCount()); + log.info("{} - Updated: {}", MAILJET, metrics.getUpdatedCount()); + log.info("{} - Deleted: {}", MAILJET, metrics.getDeletedCount()); + log.info("{} - Email changed: {}", MAILJET, metrics.getEmailChangedCount()); + log.info("{} - Unsubscribed: {}", MAILJET, metrics.getUnsubscribedCount()); + log.info("{} - Skipped: {}", MAILJET, metrics.getSkippedCount()); + + int totalErrors = metrics.getDatabaseErrorCount() + metrics.getCommunicationErrorCount() + + metrics.getRateLimitErrorCount() + metrics.getMailjetErrorCount() + + metrics.getUnexpectedErrorCount(); + + if (totalErrors > 0) { + log.info("{}Errors:", MAILJET); + log.info("{} - Database errors: {}", MAILJET, metrics.getDatabaseErrorCount()); + log.info("{} - Communication errors: {}", MAILJET, metrics.getCommunicationErrorCount()); + log.info("{} - Rate limit errors: {}", MAILJET, metrics.getRateLimitErrorCount()); + log.info("{} - Mailjet API errors: {}", MAILJET, metrics.getMailjetErrorCount()); + log.info("{} - Unexpected errors: {}", MAILJET, metrics.getUnexpectedErrorCount()); + } + log.info("{}========================================", MAILJET); } /** @@ -312,10 +566,18 @@ void incrementCreated() { createdCount++; } + void addCreatedCount(int count) { + createdCount += count; + } + void incrementUpdated() { updatedCount++; } + void addUpdatedCount(int count) { + updatedCount += count; + } + void incrementDeleted() { deletedCount++; } @@ -352,6 +614,10 @@ void incrementUnexpectedError() { unexpectedErrorCount++; } + void incrementUnexpectedError(int count) { + unexpectedErrorCount += count; + } + int getSuccessCount() { return successCount; } @@ -400,4 +666,44 @@ int getUnexpectedErrorCount() { return unexpectedErrorCount; } } + + /** + * Key for grouping users by their subscription preferences. + */ + private static class SubscriptionGroup { + final MailJetSubscriptionAction newsAction; + final MailJetSubscriptionAction eventsAction; + + SubscriptionGroup(final MailJetSubscriptionAction newsAction, + final MailJetSubscriptionAction eventsAction) { + this.newsAction = newsAction; + this.eventsAction = eventsAction; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + SubscriptionGroup that = (SubscriptionGroup) o; + return newsAction == that.newsAction && eventsAction == that.eventsAction; + } + + @Override + public int hashCode() { + return java.util.Objects.hash(newsAction, eventsAction); + } + } + + /** + * Record representing a submitted batch job with its users. + */ + private record BatchJob( + String jobId, + SubscriptionGroup group, + List users + ) {} } \ No newline at end of file diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/IExternalAccountManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/IExternalAccountManager.java index c7603a3736..4cf2f1959f 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/IExternalAccountManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/IExternalAccountManager.java @@ -2,5 +2,5 @@ public interface IExternalAccountManager { - void synchroniseChangedUsers() throws ExternalAccountSynchronisationException; + SyncResult synchroniseChangedUsers() throws ExternalAccountSynchronisationException; } diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/StubExternalAccountManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/StubExternalAccountManager.java index f75188b8ca..e5f49b25f7 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/StubExternalAccountManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/StubExternalAccountManager.java @@ -22,7 +22,7 @@ public class StubExternalAccountManager implements IExternalAccountManager { @Override - public void synchroniseChangedUsers() throws ExternalAccountSynchronisationException { + public SyncResult synchroniseChangedUsers() throws ExternalAccountSynchronisationException { // Do not fail silently: throw new ExternalAccountSynchronisationException("No external account details configured!"); } diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/SyncResult.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/SyncResult.java new file mode 100644 index 0000000000..199b446eae --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/SyncResult.java @@ -0,0 +1,13 @@ +package uk.ac.cam.cl.dtg.segue.api.managers; + +import java.util.List; + +public record SyncResult( + List failedUserDetails, + int successCount, + int totalCount +) { + public boolean hasFailures() { + return !failedUserDetails.isEmpty(); + } +} diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAccountManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAccountManager.java index 676b8dd578..40df90e909 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAccountManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAccountManager.java @@ -232,9 +232,9 @@ public UserAccountManager(final IUserDataManager database, * @throws IOException if there is an error when contacting the OAuth server * @throws AuthenticationProviderMappingException as per exception description. */ - public URI authenticate(final HttpServletRequest request, final String provider) + public URI authenticate(final HttpServletRequest request, final HttpServletResponse response, final String provider) throws IOException, AuthenticationProviderMappingException { - return this.userAuthenticationManager.getThirdPartyAuthURI(request, provider); + return this.userAuthenticationManager.getThirdPartyAuthURI(request, response, provider); } /** @@ -251,12 +251,12 @@ public URI authenticate(final HttpServletRequest request, final String provider) * @throws IOException if there is an error when contacting the OAuth server * @throws AuthenticationProviderMappingException as per exception description. */ - public URI initiateLinkAccountToUserFlow(final HttpServletRequest request, final String provider) + public URI initiateLinkAccountToUserFlow(final HttpServletRequest request, final HttpServletResponse response, final String provider) throws IOException, AuthenticationProviderMappingException { // record our intention to link an account. request.getSession().setAttribute(LINK_ACCOUNT_PARAM_NAME, Boolean.TRUE); - return this.userAuthenticationManager.getThirdPartyAuthURI(request, provider); + return this.userAuthenticationManager.getThirdPartyAuthURI(request, response, provider); } /** diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAuthenticationManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAuthenticationManager.java index ad534a05ed..fcb7321ad0 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAuthenticationManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAuthenticationManager.java @@ -18,7 +18,6 @@ import static java.time.ZoneOffset.UTC; import static java.util.Objects.requireNonNull; - import static uk.ac.cam.cl.dtg.segue.api.Constants.DATE_EXPIRES; import static uk.ac.cam.cl.dtg.segue.api.Constants.DEFAULT_DATE_FORMAT; import static uk.ac.cam.cl.dtg.segue.api.Constants.EnvironmentType; @@ -28,6 +27,8 @@ import static uk.ac.cam.cl.dtg.segue.api.Constants.JSESSION_COOOKIE; import static uk.ac.cam.cl.dtg.segue.api.Constants.LOGOUT_SESSION_ALREADY_INVALIDATED_MESSAGE; import static uk.ac.cam.cl.dtg.segue.api.Constants.NO_SESSION_TOKEN_RESERVED_VALUE; +import static uk.ac.cam.cl.dtg.segue.api.Constants.OAUTH_STATE_COOKIE; +import static uk.ac.cam.cl.dtg.segue.api.Constants.OAUTH_STATE_COOKIE_TTL_SECONDS; import static uk.ac.cam.cl.dtg.segue.api.Constants.OAUTH_TOKEN_PARAM_NAME; import static uk.ac.cam.cl.dtg.segue.api.Constants.PARTIAL_LOGIN_FLAG; import static uk.ac.cam.cl.dtg.segue.api.Constants.PARTIAL_LOGIN_SESSION_EXPIRY_SECONDS; @@ -57,6 +58,7 @@ import java.security.GeneralSecurityException; import java.security.NoSuchAlgorithmException; import java.security.spec.InvalidKeySpecException; +import java.text.MessageFormat; import java.time.Instant; import java.time.format.DateTimeFormatter; import java.time.format.DateTimeParseException; @@ -64,6 +66,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.IntStream; import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; import org.apache.commons.codec.binary.Base64; @@ -113,6 +116,7 @@ public class UserAuthenticationManager { private static final Logger log = LoggerFactory.getLogger(UserAuthenticationManager.class); private static final String HMAC_SHA_ALGORITHM = "HmacSHA256"; private static final String SAME_SITE_LAX_COMMENT = "__SAME_SITE_LAX__"; + private static final String SAME_SITE_NONE_COMMENT = "__SAME_SITE_NONE__"; private final PropertiesLoader properties; private final IUserDataManager database; @@ -193,7 +197,8 @@ public String getUserIdentifierCsv(final HttpServletRequest request) { * @throws IOException if there is an error when contacting the OAuth server * @throws AuthenticationProviderMappingException as per exception description. */ - public URI getThirdPartyAuthURI(final HttpServletRequest request, final String provider) + public URI getThirdPartyAuthURI(final HttpServletRequest request, final HttpServletResponse response, + final String provider) throws IOException, AuthenticationProviderMappingException { IAuthenticator federatedAuthenticator = mapToProvider(provider); @@ -207,6 +212,11 @@ public URI getThirdPartyAuthURI(final HttpServletRequest request, final String p // Store antiForgeryToken in the users session. request.getSession().setAttribute(STATE_PARAM_NAME, antiForgeryTokenFromProvider); + // Also send as SameSite=None cookie so it survives cross-site redirect to OAuth provider + Cookie oauthStateCookie = createOAuthStateCookie(antiForgeryTokenFromProvider, request); + oauthStateCookie.setSecure(true); + response.addCookie(oauthStateCookie); + redirectLink = URI.create(oauth2Provider.getAuthorizationUrl(antiForgeryTokenFromProvider)); } else if (federatedAuthenticator instanceof IOAuth1Authenticator) { IOAuth1Authenticator oauth1Provider = (IOAuth1Authenticator) federatedAuthenticator; @@ -251,8 +261,8 @@ public UserFromAuthProvider getThirdPartyUserInformation(final HttpServletReques String providerSpecificUserLookupReference; // if we are an OAuth2Provider complete next steps of oauth - if (authenticator instanceof IOAuthAuthenticator) { - oauthProvider = (IOAuthAuthenticator) authenticator; + if (authenticator instanceof IOAuthAuthenticator ioAuthAuthenticator) { + oauthProvider = ioAuthAuthenticator; providerSpecificUserLookupReference = this.getOauthInternalRefCode(oauthProvider, request); } else { @@ -260,8 +270,7 @@ public UserFromAuthProvider getThirdPartyUserInformation(final HttpServletReques + provider + " is unknown"); } - UserFromAuthProvider userFromProvider = oauthProvider.getUserInfo(providerSpecificUserLookupReference); - return userFromProvider; + return oauthProvider.getUserInfo(providerSpecificUserLookupReference); } /** @@ -335,8 +344,8 @@ public boolean hasLocalCredentials(final RegisteredUser user) throws SegueDataba * * @param request containing session information * @param allowIncompleteLoginsToReturnUser boolean if true will allow users that haven't completed MFA to be - * returned, false will be stricter and return null if user hasn't - * completed MFA. + * returned, false will be stricter and return null if user hasn't + * completed MFA. * @return either a user or null if we couldn't find the user for whatever reason. */ public RegisteredUser getUserFromSession(final HttpServletRequest request, @@ -438,8 +447,8 @@ public Instant getSessionExpiry(final HttpServletRequest request) { * * @param currentSessionInformation the session information map extracted from the cookie. * @param allowIncompleteLoginsToReturnUser boolean if true will allow users that haven't completed MFA to be - * returned, false will be stricter and return null if user hasn't - * completed MFA. + * returned, false will be stricter and return null if user hasn't + * completed MFA. * @return either the valid user from the cookie, or null if no valid user * @see #getUserFromSession(HttpServletRequest, boolean) there are two types of "request" and they have identical * methods @@ -447,15 +456,13 @@ public Instant getSessionExpiry(final HttpServletRequest request) { */ private RegisteredUser getUserFromSessionInformationMap(final Map currentSessionInformation, final boolean allowIncompleteLoginsToReturnUser) { - if (!allowIncompleteLoginsToReturnUser) { - // check if the session has a caveat about incomplete MFA Login - if (!Strings.isNullOrEmpty(currentSessionInformation.get(PARTIAL_LOGIN_FLAG)) - && Boolean.parseBoolean(currentSessionInformation.get(PARTIAL_LOGIN_FLAG))) { - // login is incomplete we cannot proceed. - log.debug("Incomplete MFA flow - no user object to be provided"); - return null; - } + if (!allowIncompleteLoginsToReturnUser && !Strings.isNullOrEmpty(currentSessionInformation.get(PARTIAL_LOGIN_FLAG)) + && Boolean.parseBoolean(currentSessionInformation.get(PARTIAL_LOGIN_FLAG))) { + // login is incomplete we cannot proceed. + log.debug("Incomplete MFA flow - no user object to be provided"); + return null; } + // Retrieve the user from database. try { // Check that the user's session is indeed valid: @@ -569,8 +576,7 @@ public IAuthenticator mapToProvider(final String provider) throws Authentication + " has not been registered / implemented yet: " + provider); } - log.debug("Mapping provider: " + sanitiseExternalLogValue(provider) + " to " + enumProvider); - + log.debug("Mapping provider: {} to {}", sanitiseExternalLogValue(provider), enumProvider); return this.registeredAuthProviders.get(enumProvider); } @@ -738,14 +744,14 @@ private void sendFederatedAuthenticatorResetMessage(final RegisteredUser user, f providersString = providerNames.get(0); } else { StringBuilder providersBuilder = new StringBuilder(); - for (int i = 0; i < providerNames.size(); i++) { + IntStream.range(0, providerNames.size()).forEach(i -> { if (i == providerNames.size() - 1) { providersBuilder.append(" and "); } else if (i > 1) { providersBuilder.append(", "); } providersBuilder.append(providerNames.get(i)); - } + }); providersString = providersBuilder.toString(); } @@ -796,12 +802,10 @@ private String getOauthInternalRefCode(final IOAuthAuthenticator oauthProvider, // extract auth code from string buffer String authCode = oauthProvider.extractAuthCode(fullUrlBuf.toString()); - if (authCode != null) { - String internalReference = oauthProvider.exchangeCode(authCode); - return internalReference; - } else { + if (authCode == null) { throw new AuthenticationCodeException("User denied access to our app."); } + return oauthProvider.exchangeCode(authCode); } @@ -827,18 +831,22 @@ private boolean ensureNoCSRF(final HttpServletRequest request, final IOAuthAuthe } // to deal with cross site request forgery - String csrfTokenFromUser = (String) request.getSession().getAttribute(key); - String csrfTokenFromProvider = request.getParameter(key); - - if (null == csrfTokenFromUser || !csrfTokenFromUser.equals(csrfTokenFromProvider)) { - log.error("Invalid state parameter - Provider said: " + request.getParameter(STATE_PARAM_NAME) - + " Session said: " + request.getSession().getAttribute(STATE_PARAM_NAME)); - return false; + String csrfTokenFromUser; + if (oauthProvider instanceof IOAuth2Authenticator) { + // Prefer the dedicated short-lived cookie (SameSite=None; Secure). + // Fall back to the session in case the cookie was blocked by the browser + // (the state is stored in both places during the authenticate step). + csrfTokenFromUser = getOAuthStateCookieFromRequest(request); + if (csrfTokenFromUser == null) { + log.warn("CSRF: oauth state cookie missing, falling back to session (sessionId={})", + request.getSession().getId()); + csrfTokenFromUser = (String) request.getSession().getAttribute(key); + } } else { - log.debug("State parameter matches - Provider said: " + request.getParameter(STATE_PARAM_NAME) - + " Session said: " + request.getSession().getAttribute(STATE_PARAM_NAME)); - return true; + // Session-based approach + csrfTokenFromUser = (String) request.getSession().getAttribute(key); } + return csrfTokenFromUser != null && csrfTokenFromUser.equals(request.getParameter(key)); } /** @@ -847,7 +855,7 @@ private boolean ensureNoCSRF(final HttpServletRequest request, final IOAuthAuthe * @param response to store the session in our own segue cookie. * @param user account to associate the session with. * @param partialLoginFlag Boolean to indicate whether or not this cookie represents a partial login (true) or full - * (false) + * (false) */ private void createSession( final HttpServletResponse response, @@ -875,7 +883,7 @@ private void createSession( * @param user account to associate the session with. * @param sessionExpiryTimeInSeconds max age of the cookie. * @param partialLoginFlagString either null if this is a full login cookie or a string value of true if this is - * a partial login cookie + * a partial login cookie */ private void createSession(final HttpServletResponse response, final RegisteredUser user, final int sessionExpiryTimeInSeconds, @@ -908,12 +916,12 @@ private void createSession(final HttpServletResponse response, final RegisteredU * Verifies the HMAC for userId, expiry date, session token and partial login status; but DOES NOT enforce * partial login as invalid! I.e. this method will return true for partial logins. * - * @param sessionInformation map containing session information retrieved from the cookie + * @param sessionInformation map containing session information retrieved from the cookie * @param sessionTokenFromDatabase the real session token to validate this cookie against * @return true if it is still valid, false if not. */ public boolean isValidUsersSession(final Map sessionInformation, - final Integer sessionTokenFromDatabase) { + final Integer sessionTokenFromDatabase) { requireNonNull(sessionInformation); requireNonNull(sessionTokenFromDatabase); @@ -970,8 +978,8 @@ public boolean isValidUsersSession(final Map sessionInformation, * @return HMAC signature. */ public String calculateSessionHMAC(final String key, final String userId, final String currentDate, - final String sessionToken, - @Nullable final String partialLoginFlag) { + final String sessionToken, + @Nullable final String partialLoginFlag) { StringBuilder sb = new StringBuilder(); sb.append(userId); sb.append("|").append(currentDate); @@ -1219,4 +1227,34 @@ public NewCookie createAuthLogoutNewCookie() { .build(); return logoutCookie; } + + public Cookie createOAuthStateCookie(final String state, final HttpServletRequest request) { + Cookie stateCookie = new Cookie(OAUTH_STATE_COOKIE, state); + stateCookie.setMaxAge(OAUTH_STATE_COOKIE_TTL_SECONDS); + stateCookie.setPath("/"); + stateCookie.setHttpOnly(true); + stateCookie.setSecure(isSecure(request)); + stateCookie.setComment(SAME_SITE_NONE_COMMENT); + return stateCookie; + } + + private boolean isSecure(final HttpServletRequest request) { + String forwarded = request.getHeader("X-Forwarded-Proto"); + if (forwarded != null) { + return "https".equalsIgnoreCase(forwarded); + } + return request.isSecure(); + } + + private String getOAuthStateCookieFromRequest(final HttpServletRequest request) { + if (request.getCookies() == null) { + return null; + } + for (Cookie c : request.getCookies()) { + if (OAUTH_STATE_COOKIE.equals(c.getName())) { + return c.getValue(); + } + } + return null; + } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/IExternalAccountDataManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/IExternalAccountDataManager.java index d84d98a43b..63db38df84 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/IExternalAccountDataManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/IExternalAccountDataManager.java @@ -27,4 +27,6 @@ public interface IExternalAccountDataManager { void updateProviderLastUpdated(Long userId) throws SegueDatabaseException; void updateExternalAccount(Long userId, String providerUserIdentifier) throws SegueDatabaseException; + + void batchMarkAsSynced(List userIds) throws SegueDatabaseException; } diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java index cb4bbeae57..c78a5480cb 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java @@ -184,6 +184,41 @@ public void updateExternalAccount(final Long userId, final String providerUserId } } + @Override + public void batchMarkAsSynced(final List userIds) throws SegueDatabaseException { + if (userIds == null || userIds.isEmpty()) { + log.debug("{} Batch mark as synced called with empty list", MAILJET); + return; + } + + StringBuilder queryBuilder = new StringBuilder( + "INSERT INTO external_accounts (user_id, provider_name, provider_last_updated) VALUES "); + + for (int i = 0; i < userIds.size(); i++) { + if (i > 0) { + queryBuilder.append(", "); + } + queryBuilder.append("(?, 'MailJet', NOW())"); + } + + queryBuilder.append(" ON CONFLICT (user_id, provider_name) DO UPDATE SET provider_last_updated = NOW()"); + + try (Connection conn = database.getDatabaseConnection(); + PreparedStatement pst = conn.prepareStatement(queryBuilder.toString()) + ) { + for (int i = 0; i < userIds.size(); i++) { + pst.setLong(i + 1, userIds.get(i)); + } + + int rowsAffected = pst.executeUpdate(); + log.debug("{} Batch marked {} users as synced ({} rows affected)", MAILJET, userIds.size(), rowsAffected); + + } catch (SQLException e) { + String errorMsg = String.format("Database error marking %d users as synced", userIds.size()); + throw new SegueDatabaseException(errorMsg + ": " + e.getMessage(), e); + } + } + /** * Build UserExternalAccountChanges object from database result set. * Extracts stage information from registered_contexts JSONB[] field. 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 cc2eef4482..7f4502beee 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 @@ -12,7 +12,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.common.collect.Sets; import com.google.inject.Inject; import jakarta.annotation.Nullable; import java.io.ByteArrayOutputStream; @@ -41,7 +40,6 @@ import org.elasticsearch.action.ActionRequestValidationException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import uk.ac.cam.cl.dtg.isaac.dos.IsaacCard; import uk.ac.cam.cl.dtg.isaac.dos.IsaacCardDeck; import uk.ac.cam.cl.dtg.isaac.dos.IsaacClozeQuestion; import uk.ac.cam.cl.dtg.isaac.dos.IsaacEventPage; @@ -56,6 +54,7 @@ import uk.ac.cam.cl.dtg.isaac.dos.content.ContentBase; import uk.ac.cam.cl.dtg.isaac.dos.content.EmailTemplate; import uk.ac.cam.cl.dtg.isaac.dos.content.Formula; +import uk.ac.cam.cl.dtg.isaac.dos.content.Item; import uk.ac.cam.cl.dtg.isaac.dos.content.ItemChoice; import uk.ac.cam.cl.dtg.isaac.dos.content.Media; import uk.ac.cam.cl.dtg.isaac.dos.content.Quantity; @@ -71,6 +70,9 @@ public class ContentIndexer { private static final Logger log = LoggerFactory.getLogger(ContentIndexer.class); private static final ConcurrentHashMap VERSION_LOCKS = new ConcurrentHashMap<>(); + private static final String QUESTION = "Question: "; + private static final String NUMERIC_QUESTION = "Numeric Question: "; + private static final String SYMBOLIC_QUESTION = "Symbolic Question: "; private final ElasticSearchIndexer es; private final GitDb database; @@ -79,31 +81,20 @@ public class ContentIndexer { private static final int MEDIA_FILE_SIZE_LIMIT = 300 * 1024; // Bytes private static final int NANOSECONDS_IN_A_MILLISECOND = 1000000; private static final String ERROR_OCCURRED_SUFFIX = ". The following error occurred: "; + private static final String CONTENT_LOG_PREFIX = "CONTENT - "; - private static class IndexingContext { - final Map contentCache; - final Set tagsList; - final Map allUnits; - final Map publishedUnits; - final Map> indexProblemCache; - final boolean includeUnpublished; - - IndexingContext(final Map contentCache, final Set tagsList, - final Map allUnits, final Map publishedUnits, - final Map> indexProblemCache, final boolean includeUnpublished) { - this.contentCache = contentCache; - this.tagsList = tagsList; - this.allUnits = allUnits; - this.publishedUnits = publishedUnits; - this.indexProblemCache = indexProblemCache; - this.includeUnpublished = includeUnpublished; - } + private record IndexingContext(Map contentCache, Set tagsList, Map allUnits, + Map publishedUnits, Map> indexProblemCache, + boolean includeUnpublished) { boolean shouldSkipUnpublished(final Content content) { return !includeUnpublished && !content.getPublished(); } } + private record ContentReferenceMap(Set expectedIds, Map> incomingReferences) { + } + @Inject public ContentIndexer(final GitDb database, final ElasticSearchIndexer es, final ContentMapperUtils mapperUtils) { this.database = database; @@ -112,7 +103,7 @@ public ContentIndexer(final GitDb database, final ElasticSearchIndexer es, final } - void loadAndIndexContent(final String version) throws Exception, VersionLockedException { + void loadAndIndexContent(final String version) throws Exception { // Take version lock or fail Boolean alreadyLocked = VERSION_LOCKS.putIfAbsent(version, true); @@ -121,23 +112,22 @@ void loadAndIndexContent(final String version) throws Exception, VersionLockedEx throw new VersionLockedException(version); } - log.debug("Acquired lock for version " + sanitiseInternalLogValue(version) + ". Indexing."); + log.info(CONTENT_LOG_PREFIX + "Acquired lock for version {}. Starting indexing.", + sanitiseInternalLogValue(version)); try { - database.fetchLatestFromRemote(); // Now we have acquired the lock check in case someone else has already indexed this version. - // The case where only some of the content types have been successfully indexed for this version, should - // never happen but is covered by an expunge at the start of #buildElasticSearchIndex(...). + // The case where only some of the content types have been successfully indexed for this version, + // should never happen but is covered by an expunge at the start of #buildElasticSearchIndex(...). if (allContentTypesAreIndexedForVersion(version)) { - log.debug("Content already indexed: " + sanitiseInternalLogValue(version)); + log.info(CONTENT_LOG_PREFIX + "Content already indexed: {}", sanitiseInternalLogValue(version)); return; } - log.info(String.format( - "Rebuilding content index as sha (%s) does not exist in search provider.", - sanitiseInternalLogValue(version))); + log.info(CONTENT_LOG_PREFIX + "Rebuilding content index for sha: {}", + sanitiseInternalLogValue(version)); Map contentCache = new HashMap<>(); Set tagsList = new HashSet<>(); @@ -150,25 +140,27 @@ void loadAndIndexContent(final String version) throws Exception, VersionLockedEx long endTime; totalStartTime = System.nanoTime(); - buildGitContentIndex(version, true, contentCache, tagsList, allUnits, publishedUnits, indexProblemCache); + buildGitContentIndex(version, true, contentCache, tagsList, allUnits, publishedUnits, + indexProblemCache); endTime = System.nanoTime(); - log.info( - "Finished populating Git content cache, took: {}ms", - (endTime - totalStartTime) / NANOSECONDS_IN_A_MILLISECOND - ); - log.info("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(); - log.info("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); + buildElasticSearchIndex(version, contentCache, tagsList, allUnits, publishedUnits, + indexProblemCache); endTime = System.nanoTime(); - log.info("Finished indexing git content cache, took: {}ms", (endTime - startTime) / NANOSECONDS_IN_A_MILLISECOND); + 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)) { @@ -176,8 +168,9 @@ void loadAndIndexContent(final String version) throws Exception, VersionLockedEx throw new Exception(String.format("Failed to index version %s. Don't know why.", version)); } - log.info("Finished indexing version " + sanitiseInternalLogValue(version) + ", took: " - + ((endTime - totalStartTime) / NANOSECONDS_IN_A_MILLISECOND) + "ms"); + long totalTime = (endTime - totalStartTime) / NANOSECONDS_IN_A_MILLISECOND; + log.info(CONTENT_LOG_PREFIX + "Finished indexing version {}, total time: {}ms", + sanitiseInternalLogValue(version), totalTime); } finally { VERSION_LOCKS.remove(version); @@ -203,7 +196,7 @@ void setNamedVersion(final String alias, final String version) { * @param allUnits a map of units used in numeric questions * @param publishedUnits a map of units used in published numeric questions * @param indexProblemCache a map of problems found in the indexed content - * @throws ContentManagerException if the SHA is null or the associated resource cannot be accessed + * @throws ContentManagerException if the SHA is null or the associated resource cannot be accessed */ private synchronized void buildGitContentIndex(final String sha, final boolean includeUnpublished, @@ -234,7 +227,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("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, @@ -244,9 +237,8 @@ private synchronized void buildGitContentIndex(final String sha, } repository.close(); - log.debug("Tags available {}", tagsList); - log.debug("All units: {}", allUnits); - log.info("Git content cache population for " + sanitiseInternalLogValue(sha) + " completed!"); + log.info("Tags available {}", tagsList); + log.info("All units: {}", allUnits); } catch (IOException e) { log.error("IOException while trying to access git repository. ", e); @@ -255,7 +247,7 @@ private synchronized void buildGitContentIndex(final String sha, } private void processJsonFile(final TreeWalk treeWalk, final Repository repository, - final IndexingContext context) { + final IndexingContext context) { try { ByteArrayOutputStream out = new ByteArrayOutputStream(); ObjectLoader loader = repository.open(treeWalk.getObjectId(0)); @@ -267,19 +259,21 @@ private void processJsonFile(final TreeWalk treeWalk, final Repository repositor Content content = (Content) objectMapper.readValue(out.toString(), ContentBase.class); if (context.shouldSkipUnpublished(content)) { - log.debug("Skipping unpublished content: {}", content.getId()); + log.info("Skipping unpublished content: {}", content.getId()); return; } content = this.augmentChildContent(content, treeWalk.getPathString(), null, content.getPublished()); if (null != content) { + log.info(CONTENT_LOG_PREFIX + "Processing file: {} (type: {}, id: {})", treeWalk.getPathString(), + content.getType(), content.getId()); indexContentObject(context.contentCache, context.tagsList, context.allUnits, context.publishedUnits, context.indexProblemCache, treeWalk.getPathString(), content); } } catch (JsonMappingException e) { - log.debug(String.format("Unable to parse the json file found %s as a content object. " - + "Skipping file due to error: \n %s", treeWalk.getPathString(), e.getMessage())); + log.warn(CONTENT_LOG_PREFIX + "Unable to parse the json file found {} as a content object. " + + "Skipping file due to error: \n {}", treeWalk.getPathString(), e.getMessage()); Content dummyContent = new Content(); dummyContent.setCanonicalSourceFile(treeWalk.getPathString()); this.registerContentProblem(dummyContent, "Index failure - Unable to parse json file found - " @@ -293,7 +287,7 @@ private void processJsonFile(final TreeWalk treeWalk, final Repository repositor + ERROR_OCCURRED_SUFFIX + e.getMessage(), context.indexProblemCache); } } catch (Exception e) { - log.error("Unexpected error while processing file {}: {}", treeWalk.getPathString(), e.getMessage(), e); + log.error(CONTENT_LOG_PREFIX + "Unexpected error while processing file {}: {}", treeWalk.getPathString(), e.getMessage(), e); Content dummyContent = new Content(); dummyContent.setCanonicalSourceFile(treeWalk.getPathString()); this.registerContentProblem(dummyContent, @@ -322,7 +316,7 @@ private void indexContentObject( } private void validateAndCacheContent(final Content flattenedContent, final Content parentContent, - final String treeWalkPath, final IndexingContext context) { + final String treeWalkPath, final IndexingContext context) { if (flattenedContent.getId() == null) { return; } @@ -330,7 +324,7 @@ private void validateAndCacheContent(final Content flattenedContent, final Conte if (flattenedContent instanceof IsaacQuiz) { List children = flattenedContent.getChildren(); if (children.stream().anyMatch(c -> !(c instanceof IsaacQuizSection))) { - log.debug("IsaacQuiz ({}) contains top-level non-quiz sections. Skipping.", flattenedContent.getId()); + log.info("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); @@ -339,14 +333,14 @@ private void validateAndCacheContent(final Content flattenedContent, final Conte } if (flattenedContent.getId().length() > MAXIMUM_CONTENT_ID_LENGTH) { - log.debug("Content ID too long: {}", flattenedContent.getId()); + log.info("Content ID too long: {}", flattenedContent.getId()); this.registerContentProblem(flattenedContent, "Content ID too long: " + flattenedContent.getId(), context.indexProblemCache); return; } if (flattenedContent.getId().contains(".")) { - log.debug("Resource with invalid ID ({}) detected in cache. Skipping {}", parentContent.getId(), treeWalkPath); + log.info("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); @@ -354,23 +348,23 @@ private void validateAndCacheContent(final Content flattenedContent, final Conte } if (!context.contentCache.containsKey(flattenedContent.getId())) { - log.debug("Loading into cache: {} ({}) from {}", flattenedContent.getId(), flattenedContent.getType(), - treeWalkPath); context.contentCache.put(flattenedContent.getId(), flattenedContent); + log.info(CONTENT_LOG_PREFIX + "Cached content: {} (type: {})", flattenedContent.getId(), + flattenedContent.getType()); registerTags(flattenedContent.getTags(), context.tagsList); - if (flattenedContent instanceof IsaacNumericQuestion) { - registerUnits((IsaacNumericQuestion) flattenedContent, context.allUnits, context.publishedUnits); + if (flattenedContent instanceof IsaacNumericQuestion isaacNumericQuestion) { + registerUnits(isaacNumericQuestion, context.allUnits, context.publishedUnits); } return; } if (context.contentCache.get(flattenedContent.getId()).equals(flattenedContent)) { - log.debug("Resource ({}) already seen in cache. Skipping {}", parentContent.getId(), treeWalkPath); + log.info("Resource ({}) already seen in cache. Skipping {}", parentContent.getId(), treeWalkPath); return; } - log.debug("Resource with duplicate ID ({}) detected in cache. Skipping {}", parentContent.getId(), treeWalkPath); + log.info("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(), @@ -379,6 +373,16 @@ private void validateAndCacheContent(final Content flattenedContent, final Conte context.indexProblemCache); } + private String computeParentId(final String parentId, final String contentId) { + if (parentId == null && contentId != null) { + return contentId; + } + if (contentId != null) { + return parentId + Constants.ID_SEPARATOR + contentId; + } + return parentId; + } + /** * Augments all child objects recursively to include additional information. *
@@ -402,85 +406,54 @@ 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.debug("Found question without id {} {}", content.getTitle(), canonicalSourceFile); - } - - // Try to figure out the parent ids. - String newParentId; - if (null == parentId && content.getId() != null) { - newParentId = content.getId(); - } else { - if (content.getId() != null) { - newParentId = parentId + Constants.ID_SEPARATOR + content.getId(); - } else { - newParentId = parentId; - } + log.info("Found question without id {} {}", content.getTitle(), canonicalSourceFile); } + String newParentId = computeParentId(parentId, content.getId()); content.setCanonicalSourceFile(canonicalSourceFile); - if (!content.getChildren().isEmpty()) { - for (ContentBase cb : content.getChildren()) { - if (cb instanceof Content) { - Content c = (Content) cb; - - this.augmentChildContent(c, canonicalSourceFile, newParentId, parentPublished); - } - } + if (content.getChildren() != null && !content.getChildren().isEmpty()) { + content.getChildren().stream().filter(Content.class::isInstance).map(cb -> (Content) cb) + .forEach(c -> this.augmentChildContent(c, canonicalSourceFile, newParentId, parentPublished)); } - if (content instanceof Choice) { - Choice choice = (Choice) content; + if (content instanceof Choice choice) { this.augmentChildContent((Content) choice.getExplanation(), canonicalSourceFile, newParentId, parentPublished); } - // hack to get cards to count as children: - if (content instanceof IsaacCardDeck) { - for (IsaacCard card : ((IsaacCardDeck) content).getCards()) { - this.augmentChildContent(card, canonicalSourceFile, newParentId, parentPublished); - } + if (content instanceof IsaacCardDeck isaacCardDeck && isaacCardDeck.getCards() != null) { + isaacCardDeck.getCards().forEach(card -> this.augmentChildContent(card, canonicalSourceFile, + newParentId, parentPublished)); } - if (content instanceof Question) { - augmentQuestionContent((Question) content, canonicalSourceFile, newParentId, parentPublished); + if (content instanceof Question question) { + augmentQuestionContent(question, canonicalSourceFile, newParentId, parentPublished); } - // try to determine if we have media as fields to deal with in this class - Method[] methods = content.getClass().getDeclaredMethods(); - for (Method method : methods) { - if (Media.class.isAssignableFrom(method.getReturnType())) { - try { - Media media = (Media) method.invoke(content); - if (media != null) { - media.setSrc(fixMediaSrc(canonicalSourceFile, media.getSrc())); - } - } catch (SecurityException | IllegalAccessException | IllegalArgumentException - | InvocationTargetException e) { - log.error("Unable to access method using reflection: attempting to fix Media Src", e); - } - } - } + augmentMediaFieldsViaReflection(content, canonicalSourceFile); + augmentMediaContent(content, canonicalSourceFile, parentId); + updateContentIdentifier(content, newParentId, parentPublished); - if (content instanceof Media) { - Media media = (Media) content; - media.setSrc(fixMediaSrc(canonicalSourceFile, media.getSrc())); + return content; + } - // for tracking purposes we want to generate an id for all image content objects. - if (media.getId() == null && media.getSrc() != null && parentId != null) { - media.setId(parentId + Constants.ID_SEPARATOR - + Base64.encodeBase64String(media.getSrc().getBytes())); - } + private void augmentMediaContent(final Content content, final String canonicalSourceFile, final String parentId) { + if (!(content instanceof Media media)) { + return; + } + media.setSrc(fixMediaSrc(canonicalSourceFile, media.getSrc())); + if (media.getId() == null && media.getSrc() != null && parentId != null) { + media.setId(parentId + Constants.ID_SEPARATOR + + Base64.encodeBase64String(media.getSrc().getBytes())); } + } - // Concatenate the parentId with our id to get a fully qualified - // identifier. + private void updateContentIdentifier(final Content content, final String parentId, final boolean parentPublished) { if (content.getId() != null && parentId != null) { - content.setId(parentId + Constants.ID_SEPARATOR + content.getId()); + content.setId(parentId); content.setPublished(parentPublished); } - - return content; } private void collateSearchableContent(final Content content, final StringBuilder searchableContentBuilder) { @@ -494,10 +467,9 @@ private void collateSearchableContent(final Content content, final StringBuilder } // Repeat the process for each child - if (!content.getChildren().isEmpty()) { + if (content.getChildren() != null && !content.getChildren().isEmpty()) { for (ContentBase childContentBase : content.getChildren()) { - if (childContentBase instanceof Content) { - Content child = (Content) childContentBase; + if (childContentBase instanceof Content child) { this.collateSearchableContent(child, searchableContentBuilder); } } @@ -505,14 +477,33 @@ private void collateSearchableContent(final Content content, final StringBuilder } } + private void augmentMediaFieldsViaReflection(final Content content, final String canonicalSourceFile) { + Method[] methods = content.getClass().getDeclaredMethods(); + Arrays.stream(methods) + .filter(method -> Media.class.isAssignableFrom(method.getReturnType())) + .forEach(method -> { + try { + Media media = (Media) method.invoke(content); + if (media != null) { + media.setSrc(fixMediaSrc(canonicalSourceFile, media.getSrc())); + } + } catch (SecurityException | IllegalAccessException | IllegalArgumentException + | InvocationTargetException e) { + log.error("Unable to access method using reflection: attempting to fix Media Src", e); + } + }); + } + /** - * Concatenate the source of a media content object with its parent source file. + * Augments question content by recursively processing hints, answers, feedback, and choices. * - * @param canonicalSourceFile the canonical path to use for concat operations - * @return src with relative paths fixed. + * @param question the question to augment + * @param canonicalSourceFile the canonical path for child content + * @param newParentId the parent ID for nested content IDs + * @param parentPublished the published state for nested content */ private void augmentQuestionContent(final Question question, final String canonicalSourceFile, - final String newParentId, final boolean parentPublished) { + final String newParentId, final boolean parentPublished) { augmentHints(question, canonicalSourceFile, newParentId, parentPublished); augmentAnswerContent(question, canonicalSourceFile, newParentId, parentPublished); augmentFeedbackContent(question, canonicalSourceFile, newParentId, parentPublished); @@ -520,24 +511,29 @@ private void augmentQuestionContent(final Question question, final String canoni } private void augmentHints(final Question question, final String canonicalSourceFile, final String newParentId, - final boolean parentPublished) { - if (question.getHints() != null) { - for (ContentBase cb : question.getHints()) { - Content c = (Content) cb; - this.augmentChildContent(c, canonicalSourceFile, newParentId, parentPublished); - } + final boolean parentPublished) { + if (question.getHints() == null) { + return; } + question.getHints().stream().map(cb -> (Content) cb) + .forEach(c -> this.augmentChildContent(c, canonicalSourceFile, newParentId, parentPublished)); } private void augmentAnswerContent(final Question question, final String canonicalSourceFile, final String newParentId, - final boolean parentPublished) { + final boolean parentPublished) { if (question.getAnswer() != null) { Content answer = (Content) question.getAnswer(); if (answer.getChildren() != null) { - for (ContentBase cb : answer.getChildren()) { - Content c = (Content) cb; - this.augmentChildContent(c, canonicalSourceFile, newParentId, parentPublished); - } + answer.getChildren().stream().map(cb -> (Content) cb) + .forEach(c -> this.augmentChildContent(c, canonicalSourceFile, newParentId, parentPublished)); + } + } + + if (question.getDefaultFeedback() != null) { + Content defaultFeedback = question.getDefaultFeedback(); + if (defaultFeedback.getChildren() != null) { + defaultFeedback.getChildren().stream().map(cb -> (Content) cb) + .forEach(c -> this.augmentChildContent(c, canonicalSourceFile, newParentId, parentPublished)); } } } @@ -545,28 +541,21 @@ private void augmentAnswerContent(final Question question, final String canonica private void augmentFeedbackContent(final Question question, final String canonicalSourceFile, final String newParentId, - final boolean parentPublished) { + final boolean parentPublished) { if (question.getDefaultFeedback() != null) { Content defaultFeedback = question.getDefaultFeedback(); if (defaultFeedback.getChildren() != null) { - for (ContentBase cb : defaultFeedback.getChildren()) { - Content c = (Content) cb; - this.augmentChildContent(c, canonicalSourceFile, newParentId, parentPublished); - } + defaultFeedback.getChildren().stream().map(cb -> (Content) cb) + .forEach(c -> this.augmentChildContent(c, canonicalSourceFile, newParentId, parentPublished)); } } } private void augmentChoiceQuestionContent(final Question question, final String canonicalSourceFile, - final String newParentId, final boolean parentPublished) { - if (question instanceof ChoiceQuestion) { - ChoiceQuestion choiceQuestion = (ChoiceQuestion) question; - if (choiceQuestion.getChoices() != null) { - for (ContentBase cb : choiceQuestion.getChoices()) { - Content c = (Content) cb; - this.augmentChildContent(c, canonicalSourceFile, newParentId, parentPublished); - } - } + final String newParentId, final boolean parentPublished) { + if (question instanceof ChoiceQuestion choiceQuestion && choiceQuestion.getChoices() != null) { + choiceQuestion.getChoices().stream().map(cb -> (Content) cb) + .forEach(c -> this.augmentChildContent(c, canonicalSourceFile, newParentId, parentPublished)); } } @@ -586,45 +575,41 @@ private String fixMediaSrc(final String canonicalSourceFile, final String origin */ public Set flattenContentObjects(final Content content) { Set setOfContentObjects = new HashSet<>(); - if (!content.getChildren().isEmpty()) { - - List children = content.getChildren(); - - for (ContentBase child : children) { + if (content.getChildren() != null && !content.getChildren().isEmpty()) { + content.getChildren().forEach(child -> { setOfContentObjects.add((Content) child); setOfContentObjects.addAll(flattenContentObjects((Content) child)); - } + }); + } + if (content instanceof IsaacCardDeck isaacCardDeck && isaacCardDeck.getCards() != null) { + isaacCardDeck.getCards().forEach(card -> { + setOfContentObjects.add(card); + setOfContentObjects.addAll(flattenContentObjects(card)); + }); } - setOfContentObjects.add(content); - return setOfContentObjects; } - /** - * Helper method to register problems with content objects. - * - to which the problem relates + * Register a validation problem found during content indexing. * - * @param c Partial content object to represent the object that has problems. - * @param message Error message to associate with the problem file / content. - * @param indexProblemCache a map of problems found in the indexed content + * @param content the content object with problems (must not be null) + * @param message error message describing the validation problem + * @param indexProblemCache map accumulating problems indexed by content object */ - private synchronized void registerContentProblem(final Content c, final String message, + private synchronized void registerContentProblem(final Content content, final String message, final Map> indexProblemCache) { - requireNonNull(c); - - // try and make sure each dummy content object has a title - if (c.getTitle() == null) { - c.setTitle(Paths.get(c.getCanonicalSourceFile()).getFileName().toString()); - } + requireNonNull(content, "content must not be null"); + ensureTitleExists(content); + indexProblemCache.computeIfAbsent(content, c -> new ArrayList<>()).add(message); + log.warn(CONTENT_LOG_PREFIX + "{}", message); + } - if (!indexProblemCache.containsKey(c)) { - indexProblemCache.put(c, new ArrayList<>()); + private void ensureTitleExists(final Content content) { + if (content.getTitle() == null) { + content.setTitle(Paths.get(content.getCanonicalSourceFile()).getFileName().toString()); } - - log.debug(message); - indexProblemCache.get(c).add(message); //.replace("_", "\\_")); } /** @@ -639,15 +624,8 @@ private synchronized void registerTags(final Set tags, final Set // don't do anything. return; } - - Set newTagSet = Sets.newHashSet(); - // sanity check that tags are trimmed. - for (String tag : tags) { - newTagSet.add(tag.trim()); - } - - tagsList.addAll(newTagSet); + tagsList.addAll(tags.stream().map(String::trim).collect(Collectors.toSet())); } /** @@ -663,17 +641,13 @@ private synchronized void registerUnits(final IsaacNumericQuestion q, final Map< HashMap newUnits = Maps.newHashMap(); for (Choice c : q.getChoices()) { - if (c instanceof Quantity) { - Quantity quantity = (Quantity) c; - - if (quantity.getUnits() != null && !quantity.getUnits().isEmpty()) { - String units = quantity.getUnits(); - String cleanKey = units.replace("\t", "").replace("\n", "").replace(" ", ""); + if (c instanceof Quantity quantity && quantity.getUnits() != null && !quantity.getUnits().isEmpty()) { + String units = quantity.getUnits(); + String cleanKey = units.replace("\t", "").replace("\n", "").replace(" ", ""); - // May overwrite previous entry, doesn't matter as there is - // no mechanism by which to choose a winner - newUnits.put(cleanKey, units); - } + // May overwrite previous entry, doesn't matter as there is + // no mechanism by which to choose a winner + newUnits.put(cleanKey, units); } } @@ -683,7 +657,7 @@ private synchronized void registerUnits(final IsaacNumericQuestion q, final Map< } allUnits.putAll(newUnits); - if (q.getPublished()) { + if (Boolean.TRUE.equals(q.getPublished())) { publishedUnits.putAll(newUnits); } } @@ -699,30 +673,33 @@ private synchronized void registerUnits(final IsaacNumericQuestion q, final Map< * @param indexProblemCache a map of problems found in the indexed content */ public synchronized void buildElasticSearchIndex(final String sha, - final Map gitCache, - final Set tagsList, - final Map allUnits, - final Map publishedUnits, - final Map> indexProblemCache) { + final Map gitCache, + final Set tagsList, + final Map allUnits, + final Map publishedUnits, + final Map> indexProblemCache) { if (anyContentTypesAreIndexedForVersion(sha)) { expungeAnyContentTypeIndicesRelatedToVersion(sha); } - log.info("Building search indexes for: " + sanitiseInternalLogValue(sha)); + log.info(CONTENT_LOG_PREFIX + "Building search indexes for version: {}", sanitiseInternalLogValue(sha)); + log.info(CONTENT_LOG_PREFIX + "Content cache size: {} items", gitCache.size()); + log.info(CONTENT_LOG_PREFIX + "Units to index: {} total, {} published", allUnits.size(), publishedUnits.size()); // setup object mapper to use pre-configured deserializer module. // Required to deal with type polymorphism List> contentToIndex = Lists.newArrayList(); ObjectMapper objectMapper = mapperUtils.getSharedContentObjectMapper(); - for (Content content : gitCache.values()) { + gitCache.values().forEach(content -> { try { contentToIndex.add(immutableEntry(content.getId(), objectMapper.writeValueAsString(content))); } catch (JsonProcessingException e) { - log.error("Unable to serialize content object: {} for indexing with the search provider.", content.getId(), e); + log.error(CONTENT_LOG_PREFIX + "Unable to serialize content object: {} for indexing.", + content.getId(), e); this.registerContentProblem(content, "Search Index Error: " + content.getId() - + content.getCanonicalSourceFile() + " Exception: " + e.toString(), indexProblemCache); + + content.getCanonicalSourceFile() + " Exception: " + e, indexProblemCache); } - } + }); long startTime; long endTime; @@ -732,48 +709,23 @@ public synchronized void buildElasticSearchIndex(final String sha, objectMapper.writeValueAsString(Map.of("version", sha, "created", Instant.now().toString())), "general"); es.indexObject(sha, ContentIndextype.METADATA.toString(), objectMapper.writeValueAsString(Map.of("tags", tagsList)), "tags"); + log.info(CONTENT_LOG_PREFIX + "Indexed metadata with {} tags", tagsList.size()); startTime = System.nanoTime(); - es.bulkIndex(sha, ContentIndextype.UNIT.toString(), allUnits.entrySet().stream().map(entry -> { - 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()); - return null; - } - }).filter(Objects::nonNull).collect(Collectors.toList())); - es.bulkIndex(sha, ContentIndextype.PUBLISHED_UNIT.toString(), publishedUnits.entrySet().stream().map(entry -> { - try { - return objectMapper.writeValueAsString(Map.of("cleanKey", entry.getKey(), "unit", entry.getValue())); - } catch (JsonProcessingException jsonProcessingException) { - log.error("Unable to serialise published unit entry for unit: {}", entry.getValue()); - return null; - } - }).filter(Objects::nonNull).collect(Collectors.toList())); + es.bulkIndex(sha, ContentIndextype.UNIT.toString(), serializeUnits(allUnits, objectMapper)); + es.bulkIndex(sha, ContentIndextype.PUBLISHED_UNIT.toString(), serializeUnits(publishedUnits, objectMapper)); endTime = System.nanoTime(); - log.info("Bulk unit indexing took: {}ms", (endTime - startTime) / NANOSECONDS_IN_A_MILLISECOND); + log.info(CONTENT_LOG_PREFIX + "Bulk unit indexing took: {}ms", (endTime - startTime) / NANOSECONDS_IN_A_MILLISECOND); startTime = System.nanoTime(); - es.bulkIndex(sha, ContentIndextype.CONTENT_ERROR.toString(), indexProblemCache.entrySet().stream().map(e -> { - try { - return objectMapper.writeValueAsString(Map.of( - "canonicalSourceFile", e.getKey().getCanonicalSourceFile(), - "id", e.getKey().getId() == null ? "" : e.getKey().getId(), - "title", e.getKey().getTitle() == null ? "" : e.getKey().getTitle(), - // "tags", c.getTags(), // TODO: Add tags - "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()); - return null; - } - }).filter(Objects::nonNull).collect(Collectors.toList())); + es.bulkIndex(sha, ContentIndextype.CONTENT_ERROR.toString(), + serializeContentErrors(indexProblemCache, objectMapper)); endTime = System.nanoTime(); - log.info("Bulk content error indexing took: {}ms", (endTime - startTime) / NANOSECONDS_IN_A_MILLISECOND); + log.info(CONTENT_LOG_PREFIX + "Bulk content error indexing took: {}ms", (endTime - startTime) / NANOSECONDS_IN_A_MILLISECOND); } catch (JsonProcessingException e) { - log.error("Unable to serialise sha or tags"); + log.error(CONTENT_LOG_PREFIX + "Unable to serialise sha or tags"); } catch (SegueSearchException e) { - log.error("Unable to index sha, tags, units or content errors."); + log.error(CONTENT_LOG_PREFIX + "Unable to index sha, tags, units or content errors."); } @@ -781,12 +733,13 @@ public synchronized void buildElasticSearchIndex(final String sha, startTime = System.nanoTime(); es.bulkIndexWithIds(sha, ContentIndextype.CONTENT.toString(), contentToIndex); endTime = System.nanoTime(); - log.info("Bulk indexing content took: {}ms", (endTime - startTime) / NANOSECONDS_IN_A_MILLISECOND); - log.info("Search index request sent for: " + sanitiseInternalLogValue(sha)); + log.info(CONTENT_LOG_PREFIX + "Bulk content indexing completed: {} items in {}ms", + contentToIndex.size(), + (endTime - startTime) / NANOSECONDS_IN_A_MILLISECOND); } catch (SegueSearchException e) { - log.error("Error whilst trying to perform bulk index operation.", e); + log.error(CONTENT_LOG_PREFIX + "Error during bulk index operation.", e); } catch (ActionRequestValidationException e) { - log.error("Error validating content during index", e); + log.error(CONTENT_LOG_PREFIX + "Error validating content during index", e); } } @@ -803,73 +756,18 @@ private void recordContentErrors(final String sha, final Map gi final Map> indexProblemCache) { Set allObjectsSeen = new HashSet<>(); - Set expectedIds = new HashSet<>(); - Map contentById = new HashMap<>(); - Map> incomingReferences = new HashMap<>(); // my id -> set of who references me - - // Build up a set of all content (and content fragments for validation) for (Content c : gitCache.values()) { allObjectsSeen.addAll(this.flattenContentObjects(c)); } - // Start looking for issues in the flattened content data - for (Content c : allObjectsSeen) { - // add the id to the list of defined ids - if (c.getId() != null) { - contentById.put(c.getId(), c); - } + Map contentById = buildContentIndex(allObjectsSeen); + ContentReferenceMap refMap = buildReferenceMap(sha, allObjectsSeen, indexProblemCache); - // add the ids to the list of expected ids - if (c.getRelatedContent() != null) { - expectedIds.addAll(c.getRelatedContent()); - // record which content object was referencing which ID - for (String id : c.getRelatedContent()) { - if (!incomingReferences.containsKey(id)) { - incomingReferences.put(id, new HashSet<>()); - } - incomingReferences.get(id).add(c); - } - } - - // content type specific checks - try { - this.recordContentTypeSpecificError(sha, c, indexProblemCache); - } catch (NullPointerException e) { - log.warn("Failed processing content errors in file: {}", c.getCanonicalSourceFile()); - } - } - - // Find all references to missing content. - Set missingContent = new HashSet<>(expectedIds); - missingContent.removeAll(contentById.keySet()); - - for (String id : missingContent) { - for (Content src : incomingReferences.get(id)) { - this.registerContentProblem(src, "The id '" + id + "' was referenced by " - + src.getCanonicalSourceFile() + " but the content with that " - + "ID cannot be found.", indexProblemCache); - } - } - if (!missingContent.isEmpty()) { - log.debug("Referential integrity broken for ({}) related Content items. " - + "The following ids are referenced but do not exist: {}", missingContent.size(), expectedIds); - } - - // Find all references from published content to unpublished content. - for (String refTargetId : incomingReferences.keySet()) { - Content refTarget = contentById.get(refTargetId); - if (refTarget != null) { - for (Content refSrc : incomingReferences.get(refTargetId)) { - if (refSrc.getPublished() && !refTarget.getPublished()) { - this.registerContentProblem(refSrc, "Content is published, " - + "but references unpublished content '" + refTargetId + "'.", indexProblemCache); - } - } - } - } + recordMissingContentProblems(refMap.expectedIds(), contentById, refMap.incomingReferences(), indexProblemCache); + recordPublishedToUnpublishedReferenceProblems(refMap.incomingReferences(), contentById, indexProblemCache); - log.info(String.format("Validation processing (%s) complete. There are %s files with content problems", - sanitiseInternalLogValue(sha), indexProblemCache.size())); + log.info("Validation processing ({}) complete. There are {} files with content problems", + sanitiseInternalLogValue(sha), indexProblemCache.size()); if (indexProblemCache.isEmpty()) { // Register a no-op style error to simplify application logic by ensuring there is always a content errors index @@ -889,10 +787,9 @@ 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)); - for (ContentIndextype contentIndexType : ContentIndextype.values()) { - es.expungeIndexFromSearchCache(version, contentIndexType.toString()); - } + log.info("Deleting existing indexes for version {}", sanitiseInternalLogValue(version)); + Arrays.stream(ContentIndextype.values()) + .forEach(contentIndexType -> es.expungeIndexFromSearchCache(version, contentIndexType.toString())); } /** @@ -920,12 +817,10 @@ private boolean anyContentTypesAreIndexedForVersion(final String version) { private String collateExpandableChildren(final Content content) { StringBuilder ret = new StringBuilder(); - for (Content child : flattenContentObjects(content)) { - if (child != content && null != child.getExpandable() && child.getExpandable()) { - ret.append(null != child.getType() ? child.getType() : "undefined").append(","); - } - } - if (ret.length() > 0) { + flattenContentObjects(content).stream().filter( + child -> child != content && null != child.getExpandable() && Boolean.TRUE.equals(child.getExpandable())) + .forEach(child -> ret.append(null != child.getType() ? child.getType() : "undefined").append(",")); + if (!ret.isEmpty()) { ret.deleteCharAt(ret.length() - 1); } return ret.toString(); @@ -963,79 +858,103 @@ private void recordContentTypeSpecificError(final String sha, final Content cont registerContentProblemsNumericQuestionInvalidChoicesOrUnits(content, indexProblemCache); // Find Symbolic Questions with broken properties. - if (content instanceof IsaacSymbolicQuestion) { - if (content.getClass().equals(IsaacSymbolicQuestion.class)) { - registerContentProblemsSymbolicQuestionInvalidProperties(content, indexProblemCache); - } + if (content instanceof IsaacSymbolicQuestion && content.getClass().equals(IsaacSymbolicQuestion.class)) { + registerContentProblemsSymbolicQuestionInvalidProperties(content, indexProblemCache); } - registerContentProblemsClozeQuestionChoicesHaveWrongNumberOFItems(content, indexProblemCache); + registerContentProblemsClozeQuestionChoicesHaveWrongNumberOfItems(content, indexProblemCache); } - private void registerContentProblemsClozeQuestionChoicesHaveWrongNumberOFItems( + private void registerContentProblemsClozeQuestionChoicesHaveWrongNumberOfItems( final Content content, final Map> indexProblemCache) { - if (content instanceof IsaacClozeQuestion) { - IsaacClozeQuestion q = (IsaacClozeQuestion) content; - Integer numberItems = null; - for (Choice choice : q.getChoices()) { - if (choice instanceof ItemChoice) { - ItemChoice c = (ItemChoice) choice; - if (null == c.getItems() || c.getItems().isEmpty()) { - this.registerContentProblem(content, "Cloze Question: " + q.getId() + " has choice with missing items!", - indexProblemCache); - continue; - } - int items = c.getItems().size(); - if (numberItems != null && items != numberItems) { - this.registerContentProblem(content, - "Cloze Question: " + q.getId() + " has choice with incorrect number of items!" - + " (Expected " + numberItems + ", got " + items + "!)", indexProblemCache); - continue; - } - numberItems = items; - } + if (content instanceof IsaacClozeQuestion q) { + validateClozeQuestionChoiceItems(q, content, indexProblemCache); + } + } + + private void validateClozeQuestionChoiceItems(final IsaacClozeQuestion q, final Content content, + final Map> indexProblemCache) { + if (q.getChoices() == null) { + return; + } + + Integer expectedItemCount = null; + for (Choice choice : q.getChoices()) { + if (!(choice instanceof ItemChoice c)) { + continue; + } + + List items = c.getItems(); + if (items == null || items.isEmpty()) { + this.registerContentProblem(content, buildClozeQuestionMissingItemsMessage(q), indexProblemCache); + continue; + } + + int itemCount = items.size(); + if (expectedItemCount == null) { + expectedItemCount = itemCount; + } else if (itemCount != expectedItemCount) { + this.registerContentProblem(content, + buildClozeQuestionIncorrectItemCountMessage(q, expectedItemCount, itemCount), indexProblemCache); } } } + private String buildClozeQuestionMissingItemsMessage(final IsaacClozeQuestion q) { + return "Cloze Question: " + q.getId() + " has choice with missing items!"; + } + + private String buildClozeQuestionIncorrectItemCountMessage(final IsaacClozeQuestion q, final int expected, + final int actual) { + return "Cloze Question: " + q.getId() + " has choice with incorrect number of items!" + + " (Expected " + expected + ", got " + actual + "!)"; + } + private void registerContentProblemsSymbolicQuestionInvalidProperties( final Content content, final Map> indexProblemCache) { IsaacSymbolicQuestion question = (IsaacSymbolicQuestion) content; - for (String sym : question.getAvailableSymbols()) { - registerContentProblemQuestionSymbolContainsBackslash(content, indexProblemCache, question, sym); + if (question.getAvailableSymbols() != null) { + question.getAvailableSymbols().forEach( + sym -> registerContentProblemQuestionSymbolContainsBackslash(content, indexProblemCache, question, sym)); } - for (Choice choice : question.getChoices()) { - if (choice instanceof Formula) { - Formula f = (Formula) choice; - if (f.getPythonExpression().contains("\\")) { - registerContentProblemQuestionFormulaContainsBackslash(content, indexProblemCache, question, choice); - } else if (f.getPythonExpression() == null || f.getPythonExpression().isEmpty()) { - registerContentProblemQuestionFormulaIsEmpty(content, indexProblemCache, question, choice); - } - } else { - registerContentProblemSymbolicQuestionChoiceIsNotFormula(content, indexProblemCache, question, choice); + if (question.getChoices() != null) { + question.getChoices() + .forEach(choice -> validateSymbolicQuestionFormula(content, question, choice, indexProblemCache)); + } + } + + private void validateSymbolicQuestionFormula(final Content content, final IsaacSymbolicQuestion question, + final Choice choice, + final Map> indexProblemCache) { + if (choice instanceof Formula f) { + if (f.getPythonExpression().contains("\\")) { + registerContentProblemQuestionFormulaContainsBackslash(content, indexProblemCache, question, choice); + } else if (f.getPythonExpression() == null || f.getPythonExpression().isEmpty()) { + registerContentProblemQuestionFormulaIsEmpty(content, indexProblemCache, question, choice); } + } else { + registerContentProblemSymbolicQuestionChoiceIsNotFormula(content, indexProblemCache, question, choice); } } private void registerContentProblemSymbolicQuestionChoiceIsNotFormula( final Content content, final Map> indexProblemCache, final IsaacSymbolicQuestion question, final Choice choice) { - this.registerContentProblem(content, "Symbolic Question: " + question.getId() + " has non-Formula Choice (" + this.registerContentProblem(content, SYMBOLIC_QUESTION + question.getId() + " has non-Formula Choice (" + choice.getValue() + "). It must be deleted and a new Formula Choice created.", indexProblemCache); } private void registerContentProblemQuestionFormulaIsEmpty( final Content content, final Map> indexProblemCache, final IsaacSymbolicQuestion question, final Choice choice) { - this.registerContentProblem(content, "Symbolic Question: " + question.getId() + " has Formula (" + this.registerContentProblem(content, SYMBOLIC_QUESTION + question.getId() + " has Formula (" + choice.getValue() + ") with empty pythonExpression!", indexProblemCache); } private void registerContentProblemQuestionFormulaContainsBackslash( final Content content, final Map> indexProblemCache, final IsaacSymbolicQuestion question, final Choice choice) { - this.registerContentProblem(content, "Symbolic Question: " + question.getId() + " has Formula (" + this.registerContentProblem(content, SYMBOLIC_QUESTION + question.getId() + " has Formula (" + choice.getValue() + ") with pythonExpression which contains a '\\' character.", indexProblemCache); } @@ -1043,27 +962,23 @@ private void registerContentProblemQuestionSymbolContainsBackslash( final Content content, final Map> indexProblemCache, final IsaacSymbolicQuestion question, final String sym) { if (sym.contains("\\")) { - this.registerContentProblem(content, "Symbolic Question: " + question.getId() + " has availableSymbol (" + this.registerContentProblem(content, SYMBOLIC_QUESTION + question.getId() + " has availableSymbol (" + sym + ") which contains a '\\' character.", indexProblemCache); } } private void registerContentProblemsNumericQuestionInvalidChoicesOrUnits( final Content content, final Map> indexProblemCache) { - if (content instanceof IsaacNumericQuestion) { - IsaacNumericQuestion question = (IsaacNumericQuestion) content; - for (Choice choice : question.getChoices()) { - if (choice instanceof Quantity) { - Quantity quantity = (Quantity) choice; - - registerContentProblemCannotParseQuantityChoiceAsNumber(content, indexProblemCache, question, quantity); - - registerContentProblemUnnecessaryQuantityChoiceUnits(content, indexProblemCache, question, quantity); - - - } else { - registerContentProblemNumericQuestionChoiceIsNotQuantity(content, indexProblemCache, question, choice); - } + if (content instanceof IsaacNumericQuestion question) { + if (question.getChoices() != null) { + question.getChoices().forEach(choice -> { + if (choice instanceof Quantity quantity) { + registerContentProblemCannotParseQuantityChoiceAsNumber(content, indexProblemCache, question, quantity); + registerContentProblemUnnecessaryQuantityChoiceUnits(content, indexProblemCache, question, quantity); + } else { + registerContentProblemNumericQuestionChoiceIsNotQuantity(content, indexProblemCache, question, choice); + } + }); } registerContentProblemConflictingUnitSettings(content, indexProblemCache, question); } @@ -1073,7 +988,7 @@ private void registerContentProblemConflictingUnitSettings( final Content content, final Map> indexProblemCache, final IsaacNumericQuestion question) { if (question.getRequireUnits() && null != question.getDisplayUnit() && !question.getDisplayUnit().isEmpty()) { this.registerContentProblem(content, - "Numeric Question: " + question.getId() + " has a displayUnit set but also requiresUnits!" + NUMERIC_QUESTION + question.getId() + " has a displayUnit set but also requiresUnits!" + " Units will be ignored for this question!", indexProblemCache); } } @@ -1081,7 +996,7 @@ private void registerContentProblemConflictingUnitSettings( private void registerContentProblemNumericQuestionChoiceIsNotQuantity( final Content content, final Map> indexProblemCache, final IsaacNumericQuestion question, final Choice choice) { - this.registerContentProblem(content, "Numeric Question: " + question.getId() + " has non-Quantity Choice (" + this.registerContentProblem(content, NUMERIC_QUESTION + question.getId() + " has non-Quantity Choice (" + choice.getValue() + "). It must be deleted and a new Quantity Choice created.", indexProblemCache); } @@ -1089,7 +1004,7 @@ private void registerContentProblemUnnecessaryQuantityChoiceUnits( final Content content, final Map> indexProblemCache, final IsaacNumericQuestion question, final Quantity quantity) { if (!question.getRequireUnits() && null != quantity.getUnits() && !quantity.getUnits().isEmpty()) { - this.registerContentProblem(content, "Numeric Question: " + question.getId() + this.registerContentProblem(content, NUMERIC_QUESTION + question.getId() + " has a Quantity with units but does not require units!", indexProblemCache); } } @@ -1102,7 +1017,7 @@ private void registerContentProblemCannotParseQuantityChoiceAsNumber( new BigDecimal(quantity.getValue()); } catch (NumberFormatException e) { this.registerContentProblem(content, - "Numeric Question: " + question.getId() + " has Quantity (" + quantity.getValue() + NUMERIC_QUESTION + question.getId() + " has Quantity (" + quantity.getValue() + ") with value that cannot be interpreted as a number. " + "Users will never be able to match this answer.", indexProblemCache); } @@ -1146,11 +1061,12 @@ private void registerContentProblemChoiceQuestionMissingAnswer( for (Choice choice : question.getChoices()) { if (choice.isCorrect()) { correctOptionFound = true; + break; } } if (!correctOptionFound) { this.registerContentProblem(question, - "Question: " + question.getId() + " found without a correct answer. " + QUESTION + question.getId() + " found without a correct answer. " + "This question will always be automatically marked as incorrect", indexProblemCache); } } @@ -1158,22 +1074,21 @@ private void registerContentProblemChoiceQuestionMissingAnswer( private void registerContentProblemChoiceQuestionMissingChoices( final Map> indexProblemCache, final ChoiceQuestion question) { this.registerContentProblem(question, - "Question: " + question.getId() + " found without any choice metadata. " + QUESTION + question.getId() + " found without any choice metadata. " + "This question will always be automatically " + "marked as incorrect", indexProblemCache); } private void registerContentProblemQuestionMissingId( final Content content, final Map> indexProblemCache) { if (content instanceof Question && content.getId() == null) { - this.registerContentProblem(content, "Question: " + content.getTitle() + " in " + content.getCanonicalSourceFile() + this.registerContentProblem(content, QUESTION + content.getTitle() + " in " + content.getCanonicalSourceFile() + " found without a unique id. " + "This question cannot be logged correctly.", indexProblemCache); } } private void registerContentProblemsMediaInvalidProperties( final String sha, final Content content, final Map> indexProblemCache) { - if (content instanceof Media) { - Media media = (Media) content; + if (content instanceof Media media) { registerContentProblemMediaNotFoundOrTooLarge(sha, content, indexProblemCache, media); @@ -1184,14 +1099,12 @@ private void registerContentProblemsMediaInvalidProperties( private void registerContentProblemMediaMissingAltText( final Content content, final Map> indexProblemCache, final Media media) { - if (media.getAltText() == null || media.getAltText().isEmpty()) { - if (!(media instanceof Video) && !media.getId().equals("eventThumbnail")) { - // Videos probably don't need alt text unless there is a good reason. It's not important that event - // thumbnails have alt text, so we don't record errors for those either. - this.registerContentProblem(content, "No altText attribute set for media element: " + media.getSrc() - + " in Git source file " + content.getCanonicalSourceFile(), indexProblemCache); - } + if ((media.getAltText() == null || media.getAltText().isEmpty()) && !(media instanceof Video) + && !media.getId().equals("eventThumbnail")) { + this.registerContentProblem(content, "No altText attribute set for media element: " + media.getSrc() + + " in Git source file " + content.getCanonicalSourceFile(), indexProblemCache); } + } private void registerContentProblemMediaNotFoundOrTooLarge( @@ -1263,4 +1176,103 @@ private void registerContentProblemValueWithChildren( ); } } + + private Map buildContentIndex(final Set allObjectsSeen) { + Map contentById = new HashMap<>(); + for (Content c : allObjectsSeen) { + if (c.getId() != null) { + contentById.put(c.getId(), c); + } + } + return contentById; + } + + private ContentReferenceMap buildReferenceMap(final String sha, final Set allObjectsSeen, + final Map> indexProblemCache) { + Set expectedIds = new HashSet<>(); + Map> incomingReferences = new HashMap<>(); + + for (Content c : allObjectsSeen) { + if (c.getRelatedContent() != null) { + expectedIds.addAll(c.getRelatedContent()); + for (String id : c.getRelatedContent()) { + if (!incomingReferences.containsKey(id)) { + incomingReferences.put(id, new HashSet<>()); + } + incomingReferences.get(id).add(c); + } + } + + try { + this.recordContentTypeSpecificError(sha, c, indexProblemCache); + } catch (NullPointerException e) { + log.warn("Failed processing content errors in file: {}", c.getCanonicalSourceFile()); + } + } + + return new ContentReferenceMap(expectedIds, incomingReferences); + } + + private void recordMissingContentProblems(final Set expectedIds, final Map contentById, + final Map> incomingReferences, + final Map> indexProblemCache) { + Set missingContent = new HashSet<>(expectedIds); + missingContent.removeAll(contentById.keySet()); + + for (String id : missingContent) { + for (Content src : incomingReferences.get(id)) { + this.registerContentProblem(src, "The id '" + id + "' was referenced by " + + src.getCanonicalSourceFile() + " but the content with that " + + "ID cannot be found.", indexProblemCache); + } + } + if (!missingContent.isEmpty()) { + log.warn(CONTENT_LOG_PREFIX + "Referential integrity broken for ({}) related Content items. " + + "The following ids are referenced but do not exist: {}", missingContent.size(), missingContent); + } + } + + private void recordPublishedToUnpublishedReferenceProblems(final Map> incomingReferences, + final Map contentById, + final Map> indexProblemCache) { + for (String refTargetId : incomingReferences.keySet()) { + Content refTarget = contentById.get(refTargetId); + if (refTarget != null) { + for (Content refSrc : incomingReferences.get(refTargetId)) { + if (refSrc.getPublished() && !refTarget.getPublished()) { + this.registerContentProblem(refSrc, "Content is published, " + + "but references unpublished content '" + refTargetId + "'.", indexProblemCache); + } + } + } + } + } + + private List serializeUnits(final Map units, final ObjectMapper objectMapper) { + return units.entrySet().stream().map(entry -> { + 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()); + return null; + } + }).filter(Objects::nonNull).toList(); + } + + private List serializeContentErrors(final Map> indexProblemCache, + final ObjectMapper objectMapper) { + return indexProblemCache.entrySet().stream().map(e -> { + try { + return objectMapper.writeValueAsString(Map.of( + "canonicalSourceFile", e.getKey().getCanonicalSourceFile(), + "id", e.getKey().getId() == null ? "" : e.getKey().getId(), + "title", e.getKey().getTitle() == null ? "" : e.getKey().getTitle(), + "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()); + return null; + } + }).filter(Objects::nonNull).toList(); + } } 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 382e913d07..e358b00492 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 @@ -20,6 +20,7 @@ class ETLManager { private static final String LATEST_INDEX_ALIAS = "latest"; private static final String TASK_PERIOD_SECONDS = "TASK_PERIOD_SECONDS"; private static final long TASK_PERIOD_SECONDS_FALLBACK = 300; + private static final String CONTENT_LOG_PREFIX = "CONTENT - "; private final ContentIndexer indexer; private final SchoolIndexer schoolIndexer; @@ -55,10 +56,10 @@ class ETLManager { } void setNamedVersion(final String alias, final String version) throws Exception { - log.debug("Requested aliased version: {} - {}", alias, version); + log.info(CONTENT_LOG_PREFIX + "Requested aliased version: {} - {}", alias, version); indexer.loadAndIndexContent(version); indexer.setNamedVersion(alias, version); - log.debug("Version {} with alias '{}' is successfully indexed.", version, alias); + log.info(CONTENT_LOG_PREFIX + "Version {} with alias '{}' is successfully indexed.", version, alias); } // Indexes all content in idempotent fashion. If the content is already indexed no action is taken. @@ -95,13 +96,13 @@ private class ContentIndexerTask implements Runnable { @Override public void run() { - log.debug("Starting content indexer thread."); + log.info(CONTENT_LOG_PREFIX + "Starting content indexer thread."); try { indexContent(); } catch (Exception e) { - log.error("ContentIndexerTask failed.", e); + log.error(CONTENT_LOG_PREFIX + "ContentIndexerTask failed.", e); } - log.debug("Content indexer thread complete, waiting for next scheduled run."); + log.info(CONTENT_LOG_PREFIX + "Content indexer thread complete, waiting for next scheduled run."); } } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/jobs/SyncMailjetUsersJob.java b/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/jobs/SyncMailjetUsersJob.java index 12fd2d101c..56b7ac19aa 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/jobs/SyncMailjetUsersJob.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/jobs/SyncMailjetUsersJob.java @@ -27,6 +27,7 @@ import uk.ac.cam.cl.dtg.segue.api.Constants; import uk.ac.cam.cl.dtg.segue.api.managers.ExternalAccountSynchronisationException; import uk.ac.cam.cl.dtg.segue.api.managers.IExternalAccountManager; +import uk.ac.cam.cl.dtg.segue.api.managers.SyncResult; import uk.ac.cam.cl.dtg.segue.comm.EmailCommunicationMessage; import uk.ac.cam.cl.dtg.segue.comm.EmailManager; import uk.ac.cam.cl.dtg.segue.comm.EmailType; @@ -35,6 +36,7 @@ public class SyncMailjetUsersJob implements Job { private static final Logger log = LoggerFactory.getLogger(SyncMailjetUsersJob.class); + private static final String MAILJET = "MAILJET - "; private final IExternalAccountManager externalAccountManager; private final EmailManager emailManager; @@ -54,20 +56,32 @@ public SyncMailjetUsersJob() { @Override public void execute(final JobExecutionContext context) throws JobExecutionException { try { - externalAccountManager.synchroniseChangedUsers(); - log.info("Success: synchronised users"); + SyncResult result = externalAccountManager.synchroniseChangedUsers(); + log.info("{}Complete: {} synced, {} failed", MAILJET, + result.successCount(), result.failedUserDetails().size()); + + if (result.hasFailures()) { + String body = result.failedUserDetails().size() + " user(s) failed to sync:\n" + + String.join("\n", result.failedUserDetails()); + sendAdminEmail("SyncMailjetUsersJob completed with failures", body); + } } catch (ExternalAccountSynchronisationException e) { - final String subject = "Failed to execute SyncMailjetUsersJob"; StringWriter stringWriter = new StringWriter(); PrintWriter printWriter = new PrintWriter(stringWriter); e.printStackTrace(printWriter); String exception = stringWriter.toString(); - EmailCommunicationMessage email = - new EmailCommunicationMessage(properties.getProperty(Constants.SERVER_ADMIN_ADDRESS), - subject, exception, exception, EmailType.ADMIN); - emailManager.addSystemEmailToQueue(email); - log.error("Failed to synchronise users"); + sendAdminEmail("Failed to execute SyncMailjetUsersJob", exception); + log.error("{}Failed to synchronise users", MAILJET, e); } + } + /** + * Send email to server admin with given subject and body. + */ + private void sendAdminEmail(final String subject, final String body) { + EmailCommunicationMessage email = + new EmailCommunicationMessage(properties.getProperty(Constants.SERVER_ADMIN_ADDRESS), + subject, body, body, EmailType.ADMIN); + emailManager.addSystemEmailToQueue(email); } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java b/src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java index 50daa7cf06..f716a4bebd 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java +++ b/src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java @@ -26,6 +26,7 @@ import com.mailjet.client.errors.MailjetException; import com.mailjet.client.resource.Contact; import com.mailjet.client.resource.ContactManagecontactslists; +import com.mailjet.client.resource.ContactManagemanycontacts; import com.mailjet.client.resource.Contactdata; import com.mailjet.client.resource.Contacts; import com.mailjet.client.resource.ContactslistImportList; @@ -34,12 +35,17 @@ import org.json.JSONObject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import uk.ac.cam.cl.dtg.isaac.dos.users.UserExternalAccountChanges; public class MailJetApiClientWrapper { private static final Logger log = LoggerFactory.getLogger(MailJetApiClientWrapper.class); + private static final String MAILJET = "MAILJET - "; private static final String PROPERTY_VALUE_KEY = "value"; + private static final int BULK_BATCH_SIZE = 100; + private static final String ACTION = "Action"; + private static final String LIST_ID = "ListID"; private final MailjetClient mailjetClient; private final String newsListId; @@ -81,7 +87,7 @@ public MailJetApiClientWrapper(final String mailjetApiKey, final String mailjetA */ public JSONObject getAccountByIdOrEmail(final String mailjetIdOrEmail) throws MailjetException { if (mailjetIdOrEmail == null || mailjetIdOrEmail.trim().isEmpty()) { - log.debug("Attempted to get account with null/empty identifier"); + log.debug("{}Attempted to get account with null/empty identifier", MAILJET); return null; } @@ -94,7 +100,7 @@ public JSONObject getAccountByIdOrEmail(final String mailjetIdOrEmail) throws Ma } if (response.getStatus() != 200) { - log.warn("Unexpected Mailjet response status {} when fetching account", response.getStatus()); + log.warn("{}Unexpected Mailjet response status {} when fetching account", MAILJET, response.getStatus()); throw new MailjetException("Unexpected response status: " + response.getStatus()); } @@ -135,9 +141,9 @@ public void permanentlyDeleteAccountById(final String mailjetId) throws MailjetE MailjetResponse response = mailjetClient.delete(request); if (response.getStatus() == 204 || response.getStatus() == 200) { - log.info("Successfully deleted Mailjet account: {}", mailjetId); + log.info("{}Successfully deleted Mailjet account: {}", MAILJET, mailjetId); } else if (response.getStatus() == 404) { - log.debug("Attempted to delete non-existent Mailjet account: {}", mailjetId); + log.debug("{}Attempted to delete non-existent Mailjet account: {}", MAILJET, mailjetId); } else { throw new MailjetException("Failed to delete account. Status: " + response.getStatus()); } @@ -168,7 +174,7 @@ public void permanentlyDeleteAccountById(final String mailjetId) throws MailjetE */ public String addNewUserOrGetUserIfExists(final String email) throws MailjetException { if (email == null || email.trim().isEmpty()) { - log.warn("Attempted to create Mailjet account with null/empty email"); + log.warn("{}Attempted to create Mailjet account with null/empty email", MAILJET); return null; } @@ -201,7 +207,7 @@ private String createNewMailjetAccount(String normalizedEmail) throws MailjetExc if (response.getStatus() == 201 || response.getStatus() == 200) { JSONObject responseData = response.getData().getJSONObject(0); String mailjetId = String.valueOf(responseData.get("ID")); - log.info("Successfully created Mailjet account: {}", mailjetId); + log.info("{}Successfully created Mailjet account: {}", MAILJET, mailjetId); return mailjetId; } @@ -220,13 +226,13 @@ private String createNewMailjetAccount(String normalizedEmail) throws MailjetExc private String handleUserAlreadyExists(MailjetClientRequestException e, String normalizedEmail) throws MailjetException { if (e.getMessage() != null && e.getMessage().toLowerCase().contains("already exists")) { - log.debug("User already exists in Mailjet, fetching existing account"); + log.debug("{}User already exists in Mailjet, fetching existing account", MAILJET); try { JSONObject existingAccount = getAccountByIdOrEmail(normalizedEmail); if (existingAccount != null) { String mailjetId = String.valueOf(existingAccount.get("ID")); - log.info("Retrieved existing Mailjet account: {}", mailjetId); + log.info("{}Retrieved existing Mailjet account: {}", MAILJET, mailjetId); return mailjetId; } else { String errorMsg = String.format("User reported as existing but couldn't fetch account: %s", normalizedEmail); @@ -284,7 +290,7 @@ public void updateUserProperties(final String mailjetId, final String firstName, MailjetResponse response = mailjetClient.put(request); if (response.getStatus() == 200 && response.getTotal() == 1) { - log.debug("Successfully updated properties for Mailjet account: {}", mailjetId); + log.debug("{}Successfully updated properties for Mailjet account: {}", MAILJET, mailjetId); } else { throw new MailjetException( String.format("Failed to update user properties. Status: %d, Total: %d", response.getStatus(), @@ -343,7 +349,7 @@ ContactManagecontactslists.CONTACTSLISTS, new JSONArray().put( MailjetResponse response = mailjetClient.post(request); if (response.getStatus() == 201 && response.getTotal() == 1) { - log.debug("Successfully updated subscriptions for Mailjet account: {}", mailjetId); + log.debug("{}Successfully updated subscriptions for Mailjet account: {}", MAILJET, mailjetId); } else { throw new MailjetException( String.format("Failed to update user subscriptions. Status: %d, Total: %d", response.getStatus(), @@ -393,4 +399,190 @@ private boolean isCommunicationException(MailjetException e) { String msg = e.getMessage().toLowerCase(); return msg.contains("timeout") || msg.contains("connection"); } + + /** + * Bulk create or update contacts with properties and list subscriptions. + *
+ * Uses the asynchronous ContactManagemanycontacts endpoint to handle up to BULK_BATCH_SIZE + * contacts in a single API call. All contacts in the batch share the same list subscription + * actions. The caller must partition users by subscription state before calling this method. + * + * @param users - list of users to sync (caller must ensure size <= BULK_BATCH_SIZE) + * @param newsAction - subscription action for NEWS_AND_UPDATES list + * @param eventsAction - subscription action for EVENTS list + * @return Mailjet job ID for async tracking (or null if request fails) + * @throws MailjetException - if underlying MailjetClient throws an exception + */ + public String bulkSyncUsers(final java.util.List users, + final MailJetSubscriptionAction newsAction, + final MailJetSubscriptionAction eventsAction) throws MailjetException { + if (users == null || users.isEmpty()) { + log.warn("{}Attempted to bulk sync empty user list", MAILJET); + return null; + } + + if (users.size() > BULK_BATCH_SIZE) { + throw new IllegalArgumentException("{}Bulk sync batch size ({}) exceeds limit ({})" + .replace("{}", MAILJET).replace("{}", String.valueOf(users.size())) + .replace("{}", String.valueOf(BULK_BATCH_SIZE))); + } + + try { + JSONArray contactsArray = buildContactsArray(users); + JSONArray listsArray = buildListsArray(newsAction, eventsAction); + + MailjetRequest request = new MailjetRequest(ContactManagemanycontacts.resource) + .property(ContactManagemanycontacts.CONTACTS, contactsArray) + .property(ContactManagemanycontacts.CONTACTSLISTS, listsArray); + + return submitBulkSyncRequest(request, users, newsAction, eventsAction); + + } catch (JSONException e) { + String errorMsg = "{}JSON parsing error during bulk sync of {} users".replace("{}", MAILJET) + .replace("{}", String.valueOf(users.size())); + throw new MailjetException(errorMsg, e); + + } catch (MailjetException e) { + if (isCommunicationException(e)) { + String errorMsg = "{}Communication error during bulk sync of {} users" + .replace("{}", MAILJET).replace("{}", String.valueOf(users.size())); + throw new MailjetClientCommunicationException(errorMsg, e); + } + throw e; + } + } + + /** + * Build contacts array for bulk sync request. + * Extracted to reduce cognitive complexity. + */ + private JSONArray buildContactsArray(final java.util.List users) { + JSONArray contactsArray = new JSONArray(); + for (UserExternalAccountChanges user : users) { + JSONObject contactObj = new JSONObject() + .put("Email", user.getAccountEmail()) + .put("Name", user.getGivenName() != null ? user.getGivenName() : "") + .put("Properties", new JSONObject() + .put("firstname", user.getGivenName() != null ? user.getGivenName() : "") + .put("role", user.getRole().toString()) + .put("verification_status", user.getEmailVerificationStatus().toString()) + .put("stage", user.getStage() != null ? user.getStage() : "unknown")); + contactsArray.put(contactObj); + } + return contactsArray; + } + + /** + * Build lists array for bulk sync request. + * Extracted to reduce cognitive complexity. + */ + private JSONArray buildListsArray(final MailJetSubscriptionAction newsAction, + final MailJetSubscriptionAction eventsAction) { + return new JSONArray() + .put(new JSONObject() + .put(LIST_ID, legalListId) + .put(ACTION, MailJetSubscriptionAction.FORCE_SUBSCRIBE.getValue())) + .put(new JSONObject() + .put(LIST_ID, newsListId) + .put(ACTION, newsAction.getValue())) + .put(new JSONObject() + .put(LIST_ID, eventsListId) + .put(ACTION, eventsAction.getValue())); + } + + /** + * Submit the bulk sync request and handle the response. + * Extracted to reduce cognitive complexity. + */ + private String submitBulkSyncRequest(final MailjetRequest request, + final java.util.List users, + final MailJetSubscriptionAction newsAction, + final MailJetSubscriptionAction eventsAction) + throws MailjetException { + MailjetResponse response = mailjetClient.post(request); + int status = response.getStatus(); + + if (status == 200 || status == 201) { + JSONObject responseData = response.getData().getJSONObject(0); + String jobId = responseData.optString("JobID", null); + log.info("{}Bulk sync submitted: news={}, events={} for {} users (job ID: {})", + MAILJET, newsAction, eventsAction, users.size(), jobId); + return jobId; + } + + throw new MailjetException( + "{}Failed to submit bulk sync. Status: {}".replace("{}", MAILJET) + .replace("{}", String.valueOf(status))); + } + + /** + * Get the status of an async bulk sync job. + * + * @param jobId - the job ID returned from bulkSyncUsers + * @return a JobStatus record with job status details + * @throws MailjetException - if the API call fails + */ + public JobStatus getBulkJobStatus(final String jobId) throws MailjetException { + if (jobId == null || jobId.trim().isEmpty()) { + throw new IllegalArgumentException("Job ID cannot be null or empty"); + } + + try { + MailjetRequest request = new MailjetRequest(ContactManagemanycontacts.resource, Long.parseLong(jobId)); + MailjetResponse response = mailjetClient.get(request); + + if (response.getStatus() != 200) { + throw new MailjetException("Failed to fetch job status. Status: " + response.getStatus()); + } + + JSONObject jobData = response.getData().getJSONObject(0); + log.info("{}Raw Mailjet job response for job {}: {}", MAILJET, jobId, jobData.toString()); + + String status = jobData.optString("Status", "Unknown"); + int processed = jobData.optInt("Processed", 0); + int inserted = jobData.optInt("Inserted", 0); + int updated = jobData.optInt("Updated", 0); + int unchanged = jobData.optInt("Unchanged", 0); + int errors = jobData.optInt("Error", 0); + + log.info("{}Parsed job status - Status: {}, Processed: {}, Inserted: {}, Updated: {}, Unchanged: {}, Errors: {}", + MAILJET, status, processed, inserted, updated, unchanged, errors); + + return new JobStatus(status, processed, inserted, updated, unchanged, errors); + + } catch (NumberFormatException e) { + throw new MailjetException("Invalid job ID format: " + jobId, e); + + } catch (JSONException e) { + throw new MailjetException("JSON parsing error when fetching job status for job ID: " + jobId, e); + + } catch (MailjetException e) { + if (isCommunicationException(e)) { + String errorMsg = "Communication error fetching job status for job ID: " + jobId; + throw new MailjetClientCommunicationException(errorMsg, e); + } + throw e; + } + } + + /** + * Record representing the status of a bulk sync job. + */ + public record JobStatus( + String status, + int processed, + int inserted, + int updated, + int unchanged, + int errors + ) { + public boolean isComplete() { + return "Completed".equalsIgnoreCase(status); + } + + public boolean hasFailed() { + return "Error".equalsIgnoreCase(status); + } + } + } diff --git a/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java b/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java index d23ca4a64c..62feaf2534 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java @@ -1,6 +1,8 @@ package uk.ac.cam.cl.dtg.segue.api.managers; +import static org.easymock.EasyMock.anyObject; import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.eq; import static org.easymock.EasyMock.expect; import static org.easymock.EasyMock.expectLastCall; import static org.easymock.EasyMock.replay; @@ -11,21 +13,18 @@ import com.mailjet.client.errors.MailjetClientCommunicationException; import com.mailjet.client.errors.MailjetException; import com.mailjet.client.errors.MailjetRateLimitException; -import java.util.ArrayList; import java.util.List; import org.json.JSONObject; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.NullAndEmptySource; -import org.junit.jupiter.params.provider.ValueSource; import uk.ac.cam.cl.dtg.isaac.dos.users.EmailVerificationStatus; import uk.ac.cam.cl.dtg.isaac.dos.users.Role; import uk.ac.cam.cl.dtg.isaac.dos.users.UserExternalAccountChanges; import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; import uk.ac.cam.cl.dtg.segue.dao.users.IExternalAccountDataManager; import uk.ac.cam.cl.dtg.util.email.MailJetApiClientWrapper; +import uk.ac.cam.cl.dtg.util.email.MailJetApiClientWrapper.JobStatus; import uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction; class ExternalAccountManagerTest { @@ -45,267 +44,257 @@ public void setUp() { class SynchroniseChangedUsersTests { @Test - void synchroniseChangedUsers_WithNewUser_ShouldCreateAccount() + void synchroniseChangedUsers_WithBulkUsers_ShouldSubmitAndPoll() throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { - // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, null, "test@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" + // Arrange - users with same subscription preferences should be batched + List changedUsers = List.of( + new UserExternalAccountChanges( + 1L, null, "test1@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, true, "GCSE" + ), + new UserExternalAccountChanges( + 2L, null, "test2@example.com", Role.STUDENT, "Jane", false, + EmailVerificationStatus.VERIFIED, true, true, "GCSE" + ) ); - List changedUsers = List.of(userChanges); expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mockMailjetApi.addNewUserOrGetUserIfExists("test@example.com")).andReturn("mailjetId123"); - mockMailjetApi.updateUserProperties("mailjetId123", "John", "STUDENT", "VERIFIED", "GCSE"); - expectLastCall(); - mockMailjetApi.updateUserSubscriptions("mailjetId123", - MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); - expectLastCall(); - mockDatabase.updateExternalAccount(1L, "mailjetId123"); - expectLastCall(); - mockDatabase.updateProviderLastUpdated(1L); + expect(mockMailjetApi.bulkSyncUsers(anyObject(), eq(MailJetSubscriptionAction.FORCE_SUBSCRIBE), + eq(MailJetSubscriptionAction.FORCE_SUBSCRIBE))).andReturn("job123"); + // Job completes successfully with no errors + expect(mockMailjetApi.getBulkJobStatus("job123")) + .andReturn(new JobStatus("Completed", 2, 2, 0, 0, 0)); + mockDatabase.batchMarkAsSynced(anyObject(List.class)); expectLastCall(); replay(mockDatabase, mockMailjetApi); // Act - externalAccountManager.synchroniseChangedUsers(); + SyncResult result = externalAccountManager.synchroniseChangedUsers(); // Assert + assertTrue(result.successCount() > 0); + assertTrue(!result.hasFailures()); verify(mockDatabase, mockMailjetApi); } @Test - void synchroniseChangedUsers_WithExistingUser_ShouldUpdateAccount() + void synchroniseChangedUsers_WithMixedSubscriptionPreferences_ShouldGroupByPreferences() throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { - // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" + // Arrange - users with different subscription preferences should be in different groups + List changedUsers = List.of( + new UserExternalAccountChanges( + 1L, null, "test1@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, true, "GCSE" + ), + new UserExternalAccountChanges( + 2L, null, "test2@example.com", Role.STUDENT, "Jane", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ) ); - List changedUsers = List.of(userChanges); - - JSONObject mailjetDetails = new JSONObject(); - mailjetDetails.put("Email", "test@example.com"); expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")).andReturn(mailjetDetails); - mockMailjetApi.updateUserProperties("existingMailjetId", "John", "STUDENT", "VERIFIED", "GCSE"); - expectLastCall(); - mockMailjetApi.updateUserSubscriptions("existingMailjetId", - MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); - expectLastCall(); - mockDatabase.updateExternalAccount(1L, "existingMailjetId"); - expectLastCall(); - mockDatabase.updateProviderLastUpdated(1L); + // First bulk call for group (FORCE_SUBSCRIBE, FORCE_SUBSCRIBE) + expect(mockMailjetApi.bulkSyncUsers(anyObject(), eq(MailJetSubscriptionAction.FORCE_SUBSCRIBE), + eq(MailJetSubscriptionAction.FORCE_SUBSCRIBE))).andReturn("job123"); + expect(mockMailjetApi.getBulkJobStatus("job123")) + .andReturn(new JobStatus("Completed", 1, 1, 0, 0, 0)); + + // Second bulk call for group (FORCE_SUBSCRIBE, UNSUBSCRIBE) + expect(mockMailjetApi.bulkSyncUsers(anyObject(), eq(MailJetSubscriptionAction.FORCE_SUBSCRIBE), + eq(MailJetSubscriptionAction.UNSUBSCRIBE))).andReturn("job124"); + expect(mockMailjetApi.getBulkJobStatus("job124")) + .andReturn(new JobStatus("Completed", 1, 0, 1, 0, 0)); + + mockDatabase.batchMarkAsSynced(anyObject(List.class)); expectLastCall(); replay(mockDatabase, mockMailjetApi); // Act - externalAccountManager.synchroniseChangedUsers(); + SyncResult result = externalAccountManager.synchroniseChangedUsers(); // Assert + assertTrue(!result.hasFailures()); verify(mockDatabase, mockMailjetApi); } @Test - void synchroniseChangedUsers_WithDeletedUser_ShouldDeleteAccount() + void synchroniseChangedUsers_WithDeletedUser_ShouldDeleteIndividually() throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", true, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" + List changedUsers = List.of( + new UserExternalAccountChanges( + 1L, "mailjetId123", "test@example.com", Role.STUDENT, "John", true, + EmailVerificationStatus.VERIFIED, true, true, "GCSE" + ) ); - List changedUsers = List.of(userChanges); - - JSONObject mailjetDetails = new JSONObject(); - mailjetDetails.put("Email", "test@example.com"); expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")).andReturn(mailjetDetails); - mockMailjetApi.permanentlyDeleteAccountById("existingMailjetId"); + mockMailjetApi.permanentlyDeleteAccountById("mailjetId123"); expectLastCall(); mockDatabase.updateExternalAccount(1L, null); expectLastCall(); mockDatabase.updateProviderLastUpdated(1L); expectLastCall(); + mockDatabase.batchMarkAsSynced(anyObject(List.class)); + expectLastCall(); replay(mockDatabase, mockMailjetApi); // Act - externalAccountManager.synchroniseChangedUsers(); + SyncResult result = externalAccountManager.synchroniseChangedUsers(); // Assert + assertTrue(!result.hasFailures()); verify(mockDatabase, mockMailjetApi); } @Test - void synchroniseChangedUsers_WithDeliveryFailed_ShouldUnsubscribeFromAll() + void synchroniseChangedUsers_WithDeliveryFailedUser_ShouldGroupAsRemove() throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.DELIVERY_FAILED, true, false, "GCSE" + List changedUsers = List.of( + new UserExternalAccountChanges( + 1L, null, "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.DELIVERY_FAILED, true, true, "GCSE" + ) ); - List changedUsers = List.of(userChanges); - - JSONObject mailjetDetails = new JSONObject(); - mailjetDetails.put("Email", "test@example.com"); expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")).andReturn(mailjetDetails); - mockMailjetApi.updateUserSubscriptions("existingMailjetId", - MailJetSubscriptionAction.REMOVE, MailJetSubscriptionAction.REMOVE); - expectLastCall(); - mockDatabase.updateProviderLastUpdated(1L); + // Delivery failed users should call with REMOVE for both news and events + expect(mockMailjetApi.bulkSyncUsers(anyObject(), eq(MailJetSubscriptionAction.REMOVE), + eq(MailJetSubscriptionAction.REMOVE))).andReturn("job123"); + expect(mockMailjetApi.getBulkJobStatus("job123")) + .andReturn(new JobStatus("Completed", 1, 0, 0, 1, 0)); + mockDatabase.batchMarkAsSynced(anyObject(List.class)); expectLastCall(); replay(mockDatabase, mockMailjetApi); // Act - externalAccountManager.synchroniseChangedUsers(); + SyncResult result = externalAccountManager.synchroniseChangedUsers(); // Assert + assertTrue(!result.hasFailures()); verify(mockDatabase, mockMailjetApi); } @Test - void synchroniseChangedUsers_WithEmailChange_ShouldRecreateAccount() - throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + void synchroniseChangedUsers_WithEmptyUserList_ShouldReturnWithoutError() + throws SegueDatabaseException, ExternalAccountSynchronisationException { // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, "existingMailjetId", "newemail@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" - ); - List changedUsers = List.of(userChanges); - - JSONObject oldMailjetDetails = new JSONObject(); - oldMailjetDetails.put("Email", "oldemail@example.com"); - - expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")).andReturn(oldMailjetDetails); - mockMailjetApi.permanentlyDeleteAccountById("existingMailjetId"); - expectLastCall(); - expect(mockMailjetApi.addNewUserOrGetUserIfExists("newemail@example.com")).andReturn("newMailjetId"); - mockMailjetApi.updateUserProperties("newMailjetId", "John", "STUDENT", "VERIFIED", "GCSE"); - expectLastCall(); - mockMailjetApi.updateUserSubscriptions("newMailjetId", - MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); - expectLastCall(); - mockDatabase.updateExternalAccount(1L, "newMailjetId"); - expectLastCall(); - mockDatabase.updateProviderLastUpdated(1L); - expectLastCall(); + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(List.of()); replay(mockDatabase, mockMailjetApi); // Act - externalAccountManager.synchroniseChangedUsers(); + SyncResult result = externalAccountManager.synchroniseChangedUsers(); // Assert + assertTrue(!result.hasFailures()); verify(mockDatabase, mockMailjetApi); } @Test - void synchroniseChangedUsers_WithMailjetIdButAccountNotFound_ShouldTreatAsNew() - throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + void synchroniseChangedUsers_WithDatabaseException_ShouldThrow() throws SegueDatabaseException { // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, "nonExistentMailjetId", "test@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" - ); - List changedUsers = List.of(userChanges); - - expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mockMailjetApi.getAccountByIdOrEmail("nonExistentMailjetId")).andReturn(null); - mockDatabase.updateExternalAccount(1L, null); - expectLastCall(); - expect(mockMailjetApi.addNewUserOrGetUserIfExists("test@example.com")).andReturn("newMailjetId"); - mockMailjetApi.updateUserProperties("newMailjetId", "John", "STUDENT", "VERIFIED", "GCSE"); - expectLastCall(); - mockMailjetApi.updateUserSubscriptions("newMailjetId", - MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); - expectLastCall(); - mockDatabase.updateExternalAccount(1L, "newMailjetId"); - expectLastCall(); - mockDatabase.updateProviderLastUpdated(1L); - expectLastCall(); + expect(mockDatabase.getRecentlyChangedRecords()) + .andThrow(new SegueDatabaseException("Database error")); - replay(mockDatabase, mockMailjetApi); + replay(mockDatabase); - // Act - externalAccountManager.synchroniseChangedUsers(); + // Act & Assert + assertThrows(ExternalAccountSynchronisationException.class, + () -> externalAccountManager.synchroniseChangedUsers()); - // Assert - verify(mockDatabase, mockMailjetApi); + verify(mockDatabase); } - @ParameterizedTest - @NullAndEmptySource - @ValueSource(strings = {" "}) - void synchroniseChangedUsers_WithNullOrEmptyEmail_ShouldSkip(String email) - throws SegueDatabaseException, ExternalAccountSynchronisationException { - // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, null, email, Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" + @Test + void synchroniseChangedUsers_WithJobErrorsButUserDataCorrect_ShouldMarkAsSynced() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange - job completes but has errors, but user data is correct at Mailjet + List changedUsers = List.of( + new UserExternalAccountChanges( + 1L, null, "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ) ); - List changedUsers = List.of(userChanges); expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + expect(mockMailjetApi.bulkSyncUsers(anyObject(), anyObject(), anyObject())) + .andReturn("job123"); + // Job has 1 error + expect(mockMailjetApi.getBulkJobStatus("job123")) + .andReturn(new JobStatus("Completed", 1, 0, 0, 0, 1)); + // Recovery: user data is correct in Mailjet despite job error + JSONObject mailjetContact = new JSONObject() + .put("Name", "John") + .put("Properties", new JSONObject() + .put("role", "STUDENT") + .put("verification_status", "VERIFIED") + .put("stage", "GCSE")); + expect(mockMailjetApi.getAccountByIdOrEmail("test@example.com")) + .andReturn(mailjetContact); + mockDatabase.batchMarkAsSynced(anyObject(List.class)); + expectLastCall(); replay(mockDatabase, mockMailjetApi); // Act - externalAccountManager.synchroniseChangedUsers(); + SyncResult result = externalAccountManager.synchroniseChangedUsers(); - // Assert - No mailjet calls should be made + // Assert + assertTrue(!result.hasFailures()); verify(mockDatabase, mockMailjetApi); } @Test - void synchroniseChangedUsers_WithEmptyList_ShouldReturnEarly() - throws SegueDatabaseException, ExternalAccountSynchronisationException { - // Arrange - expect(mockDatabase.getRecentlyChangedRecords()).andReturn(new ArrayList<>()); + void synchroniseChangedUsers_WithJobErrorsAndUserNotFound_ShouldNotSyncUser() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange - job completes with errors and user not found in Mailjet + List changedUsers = List.of( + new UserExternalAccountChanges( + 1L, null, "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ) + ); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + expect(mockMailjetApi.bulkSyncUsers(anyObject(), anyObject(), anyObject())) + .andReturn("job123"); + // Job has 1 error + expect(mockMailjetApi.getBulkJobStatus("job123")) + .andReturn(new JobStatus("Completed", 1, 0, 0, 0, 1)); + // Recovery: user not found in Mailjet after error - treated as failed + expect(mockMailjetApi.getAccountByIdOrEmail("test@example.com")) + .andReturn(null); + // batchMarkAsSynced not called since user failed recovery (empty list) replay(mockDatabase, mockMailjetApi); // Act - externalAccountManager.synchroniseChangedUsers(); + SyncResult result = externalAccountManager.synchroniseChangedUsers(); - // Assert + // Assert — user not found in Mailjet after job error is treated as a sync failure + assertTrue(result.hasFailures()); verify(mockDatabase, mockMailjetApi); } - @Test - void synchroniseChangedUsers_WithDatabaseException_ShouldThrow() throws SegueDatabaseException { - // Arrange - expect(mockDatabase.getRecentlyChangedRecords()) - .andThrow(new SegueDatabaseException("Database error")); - - replay(mockDatabase); - - // Act & Assert - assertThrows(ExternalAccountSynchronisationException.class, - () -> externalAccountManager.synchroniseChangedUsers()); - - verify(mockDatabase); - } - @Test void synchroniseChangedUsers_WithMailjetException_ShouldLogAndContinue() throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" + List changedUsers = List.of( + new UserExternalAccountChanges( + 1L, null, "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ) ); - List changedUsers = List.of(userChanges); expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")) + expect(mockMailjetApi.bulkSyncUsers(anyObject(), anyObject(), anyObject())) .andThrow(new MailjetException("Mailjet error")); replay(mockDatabase, mockMailjetApi); @@ -321,14 +310,15 @@ void synchroniseChangedUsers_WithMailjetException_ShouldLogAndContinue() void synchroniseChangedUsers_WithCommunicationException_ShouldThrow() throws SegueDatabaseException, MailjetException { // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" + List changedUsers = List.of( + new UserExternalAccountChanges( + 1L, null, "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ) ); - List changedUsers = List.of(userChanges); expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")) + expect(mockMailjetApi.bulkSyncUsers(anyObject(), anyObject(), anyObject())) .andThrow(new MailjetClientCommunicationException("Communication error")); replay(mockDatabase, mockMailjetApi); @@ -344,14 +334,15 @@ void synchroniseChangedUsers_WithCommunicationException_ShouldThrow() void synchroniseChangedUsers_WithRateLimitException_ShouldThrow() throws SegueDatabaseException, MailjetException { // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" + List changedUsers = List.of( + new UserExternalAccountChanges( + 1L, null, "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ) ); - List changedUsers = List.of(userChanges); expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")) + expect(mockMailjetApi.bulkSyncUsers(anyObject(), anyObject(), anyObject())) .andThrow(new MailjetRateLimitException("Rate limit exceeded")); replay(mockDatabase, mockMailjetApi); @@ -367,152 +358,66 @@ void synchroniseChangedUsers_WithRateLimitException_ShouldThrow() } @Test - void synchroniseChangedUsers_WithDatabaseErrorDuringUpdate_ShouldLogAndContinue() - throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { - // Arrange - UserExternalAccountChanges user1 = new UserExternalAccountChanges( - 1L, "mailjetId1", "test1@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" - ); - UserExternalAccountChanges user2 = new UserExternalAccountChanges( - 2L, "mailjetId2", "test2@example.com", Role.TEACHER, "Jane", false, - EmailVerificationStatus.VERIFIED, false, true, "A Level" - ); - List changedUsers = List.of(user1, user2); - - JSONObject mailjet1 = new JSONObject(); - mailjet1.put("Email", "test1@example.com"); - JSONObject mailjet2 = new JSONObject(); - mailjet2.put("Email", "test2@example.com"); - - expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - - // First user - database error during update - expect(mockMailjetApi.getAccountByIdOrEmail("mailjetId1")).andReturn(mailjet1); - mockMailjetApi.updateUserProperties("mailjetId1", "John", "STUDENT", "VERIFIED", "GCSE"); - expectLastCall(); - mockMailjetApi.updateUserSubscriptions("mailjetId1", - MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); - expectLastCall(); - mockDatabase.updateExternalAccount(1L, "mailjetId1"); - expectLastCall().andThrow(new SegueDatabaseException("DB error")); - - // Second user - should still process - expect(mockMailjetApi.getAccountByIdOrEmail("mailjetId2")).andReturn(mailjet2); - mockMailjetApi.updateUserProperties("mailjetId2", "Jane", "TEACHER", "VERIFIED", "A Level"); - expectLastCall(); - mockMailjetApi.updateUserSubscriptions("mailjetId2", - MailJetSubscriptionAction.UNSUBSCRIBE, MailJetSubscriptionAction.FORCE_SUBSCRIBE); - expectLastCall(); - mockDatabase.updateExternalAccount(2L, "mailjetId2"); - expectLastCall(); - mockDatabase.updateProviderLastUpdated(2L); - expectLastCall(); - - replay(mockDatabase, mockMailjetApi); - - // Act - Should not throw, continues processing - externalAccountManager.synchroniseChangedUsers(); - - // Assert - verify(mockDatabase, mockMailjetApi); - } - - @Test - void synchroniseChangedUsers_WithMultipleUsers_ShouldProcessAll() + void synchroniseChangedUsers_WithSingleRateLimitDuringPolling_ShouldContinuePolling() throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { - // Arrange - UserExternalAccountChanges user1 = new UserExternalAccountChanges( - 1L, null, "new@example.com", Role.STUDENT, "Alice", false, - EmailVerificationStatus.VERIFIED, true, true, "GCSE" - ); - UserExternalAccountChanges user2 = new UserExternalAccountChanges( - 2L, "existingId", "existing@example.com", Role.TEACHER, "Bob", false, - EmailVerificationStatus.VERIFIED, false, false, "A Level" + // Arrange - single rate limit should be tolerated + List changedUsers = List.of( + new UserExternalAccountChanges( + 1L, null, "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ) ); - List changedUsers = List.of(user1, user2); - - JSONObject mailjet2 = new JSONObject(); - mailjet2.put("Email", "existing@example.com"); expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - - // User 1 - new user - expect(mockMailjetApi.addNewUserOrGetUserIfExists("new@example.com")).andReturn("newId"); - mockMailjetApi.updateUserProperties("newId", "Alice", "STUDENT", "VERIFIED", "GCSE"); - expectLastCall(); - mockMailjetApi.updateUserSubscriptions("newId", - MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.FORCE_SUBSCRIBE); - expectLastCall(); - mockDatabase.updateExternalAccount(1L, "newId"); - expectLastCall(); - mockDatabase.updateProviderLastUpdated(1L); - expectLastCall(); - - // User 2 - existing user - expect(mockMailjetApi.getAccountByIdOrEmail("existingId")).andReturn(mailjet2); - mockMailjetApi.updateUserProperties("existingId", "Bob", "TEACHER", "VERIFIED", "A Level"); - expectLastCall(); - mockMailjetApi.updateUserSubscriptions("existingId", - MailJetSubscriptionAction.UNSUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); - expectLastCall(); - mockDatabase.updateExternalAccount(2L, "existingId"); - expectLastCall(); - mockDatabase.updateProviderLastUpdated(2L); + expect(mockMailjetApi.bulkSyncUsers(anyObject(), anyObject(), anyObject())) + .andReturn("job123"); + // First poll hits rate limit, second succeeds + expect(mockMailjetApi.getBulkJobStatus("job123")) + .andThrow(new MailjetRateLimitException("Rate limit")); + expect(mockMailjetApi.getBulkJobStatus("job123")) + .andReturn(new JobStatus("Completed", 1, 0, 1, 0, 0)); + mockDatabase.batchMarkAsSynced(anyObject(List.class)); expectLastCall(); replay(mockDatabase, mockMailjetApi); // Act - externalAccountManager.synchroniseChangedUsers(); + SyncResult result = externalAccountManager.synchroniseChangedUsers(); // Assert + assertTrue(!result.hasFailures()); verify(mockDatabase, mockMailjetApi); } @Test - void synchroniseChangedUsers_WithUnexpectedError_ShouldLogAndContinue() - throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { - // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, "mailjetId", "test@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" + void synchroniseChangedUsers_WithRepeatedRateLimitDuringPolling_ShouldFailFast() + throws SegueDatabaseException, MailjetException { + // Arrange - 2+ consecutive rate limits should fail fast + List changedUsers = List.of( + new UserExternalAccountChanges( + 1L, null, "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ) ); - List changedUsers = List.of(userChanges); expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mockMailjetApi.getAccountByIdOrEmail("mailjetId")) - .andThrow(new RuntimeException("Unexpected error")); + expect(mockMailjetApi.bulkSyncUsers(anyObject(), anyObject(), anyObject())) + .andReturn("job123"); + // Both polls hit rate limit + expect(mockMailjetApi.getBulkJobStatus("job123")) + .andThrow(new MailjetRateLimitException("Rate limit")) + .times(2); replay(mockDatabase, mockMailjetApi); - // Act - Should not throw, logs error and continues - externalAccountManager.synchroniseChangedUsers(); - - // Assert - verify(mockDatabase, mockMailjetApi); - } - - @Test - void synchroniseChangedUsers_WithNewUserAndNullMailjetId() - throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { - // Arrange - UserExternalAccountChanges userChanges = new UserExternalAccountChanges( - 1L, null, "test@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "GCSE" - ); - List changedUsers = List.of(userChanges); - - expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mockMailjetApi.addNewUserOrGetUserIfExists("test@example.com")).andReturn(null); - - replay(mockDatabase, mockMailjetApi); + // Act & Assert - should throw on 2nd consecutive rate limit + ExternalAccountSynchronisationException exception = assertThrows( + ExternalAccountSynchronisationException.class, + () -> externalAccountManager.synchroniseChangedUsers()); - // Act & Assert - externalAccountManager.synchroniseChangedUsers(); + assertTrue(exception.getMessage().contains("rate limit")); verify(mockDatabase, mockMailjetApi); } - } } diff --git a/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/UserManagerTest.java b/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/UserManagerTest.java index 961aa4aa6c..7d345f5125 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/UserManagerTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/UserManagerTest.java @@ -237,21 +237,23 @@ void authenticate_badProviderGiven_authenticationProviderException() throws IOEx UserAccountManager userManager = buildTestUserManager(); HttpServletRequest request = createMock(HttpServletRequest.class); + HttpServletResponse response = createMock(HttpServletResponse.class); String someInvalidProvider = "BAD_PROVIDER!!"; replay(request); + replay(response); replay(dummyQuestionDatabase); // Act try { - userManager.authenticate(request, someInvalidProvider); + userManager.authenticate(request, response, someInvalidProvider); fail("Exception expected"); } catch (AuthenticationProviderMappingException e) { // pass } - verify(dummyQuestionDatabase, request); + verify(dummyQuestionDatabase, request, response); } /** @@ -269,6 +271,7 @@ void authenticate_selectedValidOAuthProvider_providesRedirectResponseForAuthoriz HttpSession dummySession = createMock(HttpSession.class); HttpServletRequest request = createMock(HttpServletRequest.class); + HttpServletResponse response = createMock(HttpServletResponse.class); String exampleRedirectUrl = "https://accounts.google.com/o/oauth2/auth?" + "client_id=267566420063-jalcbiffcpmteh42cib5hmgb16upspc0.apps.googleusercontent.com&" + "redirect_uri=http://localhost:8080/rutherford-server/segue/api/auth/google/callback&" @@ -278,11 +281,16 @@ void authenticate_selectedValidOAuthProvider_providesRedirectResponseForAuthoriz // for CSRF state information expect(request.getSession()).andReturn(dummySession).atLeastOnce(); + expect(request.getHeader("X-Forwarded-Proto")).andReturn(null).atLeastOnce(); + expect(request.isSecure()).andReturn(true).atLeastOnce(); dummySession.setAttribute(EasyMock.anyObject(), EasyMock.anyObject()); expectLastCall().atLeastOnce(); + response.addCookie(anyObject()); + expectLastCall().once(); replay(dummySession); replay(request); + replay(response); replay(dummyQuestionDatabase); String someAntiForgeryToken = "someAntiForgeryToken"; @@ -291,10 +299,10 @@ void authenticate_selectedValidOAuthProvider_providesRedirectResponseForAuthoriz replay(dummyAuth); // Act - URI redirectURI = userManager.authenticate(request, someValidProviderString); + URI redirectURI = userManager.authenticate(request, response, someValidProviderString); // Assert - verify(dummyQuestionDatabase, request); + verify(dummyQuestionDatabase, request, response); assertEquals(exampleRedirectUrl, redirectURI.toString()); } @@ -344,15 +352,17 @@ void authenticateCallback_checkNewUserIsAuthenticated_createInternalUserAccount( expect(request.getSession()).andReturn(dummySession).atLeastOnce(); - Cookie[] cookieWithoutSessionInfo = {}; // empty as not logged in. - expect(request.getCookies()).andReturn(cookieWithoutSessionInfo).times(2); + // getCookies() is called twice per method (null-check + for-loop): + // 2 calls from getOAuthStateCookieFromRequest (CSRF check) + // 2 calls from getSegueSessionFromRequest (session check before login) + Cookie oauthStateCookie = new Cookie(Constants.OAUTH_STATE_COOKIE, CSRF_TEST_VALUE); + expect(request.getCookies()).andReturn(new Cookie[]{oauthStateCookie}).times(4); expect(dummySession.getAttribute(Constants.ANONYMOUS_USER)).andReturn(someSegueAnonymousUserId) .atLeastOnce(); // session // id - // Mock CSRF checks - expect(dummySession.getAttribute(Constants.STATE_PARAM_NAME)).andReturn(CSRF_TEST_VALUE).atLeastOnce(); + // Mock CSRF checks — OAuth2 reads from cookie, not session expect(request.getParameter(Constants.STATE_PARAM_NAME)).andReturn(CSRF_TEST_VALUE).atLeastOnce(); // Mock URL params extract stuff @@ -455,17 +465,17 @@ void authenticateCallback_checkInvalidCSRF_throwsCSRFException() { String someInvalidCSRFValue = "FRAUDHASHAPPENED"; String validOAuthProvider = "test"; - expect(request.getSession()).andReturn(dummySession).atLeastOnce(); + expect(request.getSession()).andReturn(dummySession).anyTimes(); expect(dummySession.getAttribute(Constants.SESSION_USER_ID)).andReturn(null).anyTimes(); + expect(dummySession.getId()).andReturn("test-session-id").anyTimes(); // Mock URL params extract stuff // Return any non-null string String queryString = Constants.STATE_PARAM_NAME + "=" + someInvalidCSRFValue; expect(request.getQueryString()).andReturn(queryString).once(); - // Mock CSRF checks - expect(dummySession.getAttribute(Constants.STATE_PARAM_NAME)).andReturn(CSRF_TEST_VALUE).atLeastOnce(); - + // Mock CSRF checks — OAuth2 reads state from cookie; cookie has valid token but provider sends wrong one + expect(request.getCookies()).andReturn(new Cookie[]{new Cookie(Constants.OAUTH_STATE_COOKIE, CSRF_TEST_VALUE)}).anyTimes(); expect(request.getParameter(Constants.STATE_PARAM_NAME)).andReturn(someInvalidCSRFValue).atLeastOnce(); replay(dummySession, request, dummyQuestionDatabase); @@ -498,15 +508,18 @@ void authenticateCallback_checkWhenNoCSRFProvided_throwsCSRFException() { String validOAuthProvider = "test"; - expect(request.getSession()).andReturn(dummySession).atLeastOnce(); + expect(request.getSession()).andReturn(dummySession).anyTimes(); expect(dummySession.getAttribute(Constants.SESSION_USER_ID)).andReturn(null).anyTimes(); + expect(dummySession.getAttribute(Constants.STATE_PARAM_NAME)).andReturn(null).anyTimes(); + expect(dummySession.getId()).andReturn("test-session-id").anyTimes(); // Mock URL params extract stuff expect(request.getQueryString()).andReturn("").atLeastOnce(); - // Mock CSRF checks - expect(dummySession.getAttribute(Constants.STATE_PARAM_NAME)).andReturn(null).atLeastOnce(); - expect(request.getParameter(Constants.STATE_PARAM_NAME)).andReturn(CSRF_TEST_VALUE).atLeastOnce(); + // Mock CSRF checks — OAuth2 reads state from cookie; no cookie present → null → CSRF fails + // When both cookie and session are null, getParameter is not called due to short-circuit AND + expect(request.getCookies()).andReturn(new Cookie[]{}).anyTimes(); + expect(request.getParameter(Constants.STATE_PARAM_NAME)).andReturn(CSRF_TEST_VALUE).anyTimes(); replay(dummySession); replay(request); diff --git a/src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java b/src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java index 16cd0726be..74d8cf215e 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java @@ -15,6 +15,7 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; +import java.util.ArrayList; import java.util.List; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -126,6 +127,115 @@ void getRecentlyChangedRecords_WithDatabaseError_ShouldThrowException() throws E } } + @Nested + class BatchMarkAsSyncedTests { + + @Test + void batchMarkAsSynced_WithValidUserIds_ShouldUpdateDatabase() throws Exception { + // Arrange + List userIds = List.of(1L, 2L, 3L); + + expect(mockDatabase.getDatabaseConnection()).andReturn(mockConnection); + expect(mockConnection.prepareStatement(anyString())).andReturn(mockPreparedStatement); + + mockPreparedStatement.setLong(1, 1L); + expectLastCall(); + mockPreparedStatement.setLong(2, 2L); + expectLastCall(); + mockPreparedStatement.setLong(3, 3L); + expectLastCall(); + + expect(mockPreparedStatement.executeUpdate()).andReturn(3); + + mockPreparedStatement.close(); + expectLastCall(); + mockConnection.close(); + expectLastCall(); + + replay(mockDatabase, mockConnection, mockPreparedStatement); + + // Act + persistenceManager.batchMarkAsSynced(userIds); + + // Assert + verify(mockDatabase, mockConnection, mockPreparedStatement); + } + + @Test + void batchMarkAsSynced_WithEmptyList_ShouldDoNothing() throws Exception { + // Arrange - no expectations set, method should return early + + replay(mockDatabase, mockConnection, mockPreparedStatement); + + // Act + persistenceManager.batchMarkAsSynced(new ArrayList<>()); + + // Assert + verify(mockDatabase, mockConnection, mockPreparedStatement); + } + + @Test + void batchMarkAsSynced_WithLargeUserList_ShouldBatchInsert() throws Exception { + // Arrange + List userIds = new ArrayList<>(); + for (long i = 1; i <= 100; i++) { + userIds.add(i); + } + + expect(mockDatabase.getDatabaseConnection()).andReturn(mockConnection); + expect(mockConnection.prepareStatement(anyString())).andReturn(mockPreparedStatement); + + for (long i = 1; i <= 100; i++) { + mockPreparedStatement.setLong((int) i, i); + expectLastCall(); + } + + expect(mockPreparedStatement.executeUpdate()).andReturn(100); + + mockPreparedStatement.close(); + expectLastCall(); + mockConnection.close(); + expectLastCall(); + + replay(mockDatabase, mockConnection, mockPreparedStatement); + + // Act + persistenceManager.batchMarkAsSynced(userIds); + + // Assert + verify(mockDatabase, mockConnection, mockPreparedStatement); + } + + @Test + void batchMarkAsSynced_WithDatabaseError_ShouldThrowException() throws Exception { + // Arrange + List userIds = List.of(1L, 2L); + + expect(mockDatabase.getDatabaseConnection()).andReturn(mockConnection); + expect(mockConnection.prepareStatement(anyString())).andReturn(mockPreparedStatement); + + mockPreparedStatement.setLong(1, 1L); + expectLastCall(); + mockPreparedStatement.setLong(2, 2L); + expectLastCall(); + + expect(mockPreparedStatement.executeUpdate()).andThrow(new SQLException("Database error")); + + mockPreparedStatement.close(); + expectLastCall(); + mockConnection.close(); + expectLastCall(); + + replay(mockDatabase, mockConnection, mockPreparedStatement); + + // Act & Assert + assertThrows(SegueDatabaseException.class, + () -> persistenceManager.batchMarkAsSynced(userIds)); + + verify(mockDatabase, mockConnection, mockPreparedStatement); + } + } + // Helper method to setup mock ResultSet with all expected calls private void setupMockResultSetForUser(Long userId, String mailjetId, String email, String role, String givenName, boolean deleted, String verificationStatus, diff --git a/src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java b/src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java index 54870aa6af..1a6754321f 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java @@ -6,6 +6,9 @@ import com.mailjet.client.errors.MailjetClientRequestException; import com.mailjet.client.errors.MailjetException; import com.mailjet.client.errors.MailjetClientCommunicationException; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.IntStream; import org.json.JSONArray; import org.json.JSONObject; import org.junit.jupiter.api.BeforeEach; @@ -14,6 +17,9 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.NullAndEmptySource; import org.junit.jupiter.params.provider.ValueSource; +import uk.ac.cam.cl.dtg.isaac.dos.users.EmailVerificationStatus; +import uk.ac.cam.cl.dtg.isaac.dos.users.Role; +import uk.ac.cam.cl.dtg.isaac.dos.users.UserExternalAccountChanges; import uk.ac.cam.cl.dtg.util.email.MailJetApiClientWrapper; import uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction; @@ -671,6 +677,120 @@ void updateUserSubscriptions_WithCommunicationError_ShouldThrowCommunicationExce } } + @Nested + class BulkSyncUsersTests { + + @Test + void bulkSyncUsers_WithValidUsers_ShouldReturnJobId() throws MailjetException { + // Arrange + List users = List.of( + new UserExternalAccountChanges( + 1L, null, "test1@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, true, "GCSE" + ), + new UserExternalAccountChanges( + 2L, null, "test2@example.com", Role.TEACHER, "Jane", false, + EmailVerificationStatus.VERIFIED, false, true, "A_LEVEL" + ) + ); + + MailjetResponse mockResponse = createMock(MailjetResponse.class); + JSONArray mockData = new JSONArray(); + JSONObject mockResult = new JSONObject(); + mockResult.put("JobID", "job123"); + mockData.put(mockResult); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(200); + expect(mockResponse.getData()).andReturn(mockData); + + replay(mockMailjetClient, mockResponse); + + // Act + String jobId = mailJetApiClientWrapper.bulkSyncUsers(users, + MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.FORCE_SUBSCRIBE); + + // Assert + verify(mockMailjetClient, mockResponse); + assertEquals("job123", jobId); + } + + @Test + void bulkSyncUsers_WithEmptyList_ShouldReturnNull() throws MailjetException { + // Act + String result = mailJetApiClientWrapper.bulkSyncUsers(new ArrayList<>(), + MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.FORCE_SUBSCRIBE); + + // Assert + assertNull(result); + } + + @Test + void bulkSyncUsers_WithOversizedBatch_ShouldThrowException() { + // Arrange + List users = IntStream.range(0, 1001).mapToObj(i -> new UserExternalAccountChanges( + (long) i, null, "test" + i + "@example.com", Role.STUDENT, "User" + i, false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + )).toList(); + + // Act & Assert + assertThrows(IllegalArgumentException.class, () -> + mailJetApiClientWrapper.bulkSyncUsers(users, + MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.FORCE_SUBSCRIBE) + ); + } + + @Test + void bulkSyncUsers_WithApiError_ShouldThrowException() throws MailjetException { + // Arrange + List users = List.of( + new UserExternalAccountChanges( + 1L, null, "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, true, "GCSE" + ) + ); + + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(500); + + replay(mockMailjetClient, mockResponse); + + // Act & Assert + assertThrows(MailjetException.class, () -> + mailJetApiClientWrapper.bulkSyncUsers(users, + MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.FORCE_SUBSCRIBE) + ); + } + + @Test + void bulkSyncUsers_WithCommunicationError_ShouldThrowCommunicationException() + throws MailjetException { + // Arrange + List users = List.of( + new UserExternalAccountChanges( + 1L, null, "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, true, "GCSE" + ) + ); + + MailjetClientCommunicationException commException = + new MailjetClientCommunicationException("Connection timeout"); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))) + .andThrow(commException); + + replay(mockMailjetClient); + + // Act & Assert + assertThrows(MailjetClientCommunicationException.class, () -> + mailJetApiClientWrapper.bulkSyncUsers(users, + MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.FORCE_SUBSCRIBE) + ); + } + } + private void injectMockMailjetClient(MailJetApiClientWrapper wrapper, MailjetClient client) { try { var field = MailJetApiClientWrapper.class.getDeclaredField("mailjetClient"); diff --git a/web-api-live.xml b/web-api-live.xml index 711c28d20e..8c0001d93f 100644 --- a/web-api-live.xml +++ b/web-api-live.xml @@ -67,6 +67,7 @@ true + None diff --git a/web-api-local.xml b/web-api-local.xml index 35195322f2..bd1987dd5f 100644 --- a/web-api-local.xml +++ b/web-api-local.xml @@ -67,6 +67,7 @@ true + None