From 213cf443e1a1e336db4caa0b060b5ad7bf82950d Mon Sep 17 00:00:00 2001 From: anhefti Date: Mon, 17 May 2021 21:19:01 +0200 Subject: [PATCH] simplified LMS API --- .../servicelayer/dao/impl/ExamDAOImpl.java | 18 +--- .../servicelayer/lms/LmsAPITemplate.java | 42 ++++----- .../lms/impl/AbstractCourseAccess.java | 60 +++++++++++-- .../lms/impl/edx/OpenEdxCourseAccess.java | 89 +++++++++---------- .../lms/impl/edx/OpenEdxLmsAPITemplate.java | 16 ++-- .../lms/impl/mockup/MockupLmsAPITemplate.java | 21 ++--- .../lms/impl/moodle/MoodleCourseAccess.java | 77 ++++++++-------- .../lms/impl/moodle/MoodleLmsAPITemplate.java | 4 +- 8 files changed, 170 insertions(+), 157 deletions(-) diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/ExamDAOImpl.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/ExamDAOImpl.java index e1525283..db5e6755 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/ExamDAOImpl.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/ExamDAOImpl.java @@ -806,11 +806,9 @@ public class ExamDAOImpl implements ExamDAO { // get and map quizzes final Map quizzes = this.lmsAPIService .getLmsAPITemplate(lmsSetupId) - .map(template -> getQuizzesFromLMS(template, recordMapping.keySet())) - .onError(error -> log.error("Failed to get quizzes for exams: ", error)) - .getOr(Collections.emptyList()) + .flatMap(template -> template.getQuizzes(recordMapping.keySet())) + .getOrElse(() -> Collections.emptyList()) .stream() - .flatMap(Result::skipOnError) .collect(Collectors.toMap(q -> q.id, Function.identity())); if (records.size() != quizzes.size()) { @@ -858,18 +856,6 @@ public class ExamDAOImpl implements ExamDAO { }); } - private Collection> getQuizzesFromLMS( - final LmsAPITemplate template, - final Set ids) { - - try { - return template.getQuizzes(ids); - } catch (final Exception e) { - log.error("Unexpected error while using LmsAPITemplate to get quizzes: ", e); - return Collections.emptyList(); - } - } - private QuizData getQuizData( final Map quizzes, final String externalId, diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/LmsAPITemplate.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/LmsAPITemplate.java index 80b397b7..8b990a67 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/LmsAPITemplate.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/LmsAPITemplate.java @@ -12,7 +12,7 @@ import java.util.Collection; import java.util.List; import java.util.Set; -import ch.ethz.seb.sebserver.gbl.async.MemoizingCircuitBreaker; +import ch.ethz.seb.sebserver.gbl.async.CircuitBreaker; import ch.ethz.seb.sebserver.gbl.model.exam.Chapters; import ch.ethz.seb.sebserver.gbl.model.exam.Exam; import ch.ethz.seb.sebserver.gbl.model.exam.QuizData; @@ -22,6 +22,7 @@ import ch.ethz.seb.sebserver.gbl.model.institution.LmsSetupTestResult; import ch.ethz.seb.sebserver.gbl.model.user.ExamineeAccountDetails; import ch.ethz.seb.sebserver.gbl.util.Result; import ch.ethz.seb.sebserver.webservice.servicelayer.dao.FilterMap; +import ch.ethz.seb.sebserver.webservice.servicelayer.lms.impl.AbstractCachedCourseAccess; import ch.ethz.seb.sebserver.webservice.servicelayer.lms.impl.AbstractCourseAccess; /** Defines an LMS API access template to build SEB Server LMS integration. @@ -41,28 +42,18 @@ import ch.ethz.seb.sebserver.webservice.servicelayer.lms.impl.AbstractCourseAcce * All course API requests of this template shall not block and return as fast as possible * with the best result it can provide for the time on that the request was made. *

+ * Each request to a remote LMS shall be executed within a protected call such that the + * request don't block the API call as well as do not attack the remote LMS with endless + * requests on failure.
+ * Therefore the abstract class {@link AbstractCourseAccess} defines protected calls + * for different API calls by using {@link CircuitBreaker}. documentation on the class for + * more information. + *

* Since the course API requests course data from potentially thousands of existing and - * active courses, the course API can implement some caches if needed.
- * A cache in the course API has manly two purposes; The first and prior purpose is to - * be able to provide course data as fast as possible even if the LMS is not available or - * busy at the time. The second purpose is to guarantee fast data access for the system - * if this is needed and data actuality has second priority.
- * Therefore usual get quiz data functions like {@link #getQuizzes(FilterMap filterMap) }, - * {@link #getQuizzes(Set ids) } and {@link #getQuiz(final String id) } - * shall always first try to connect to the LMS and request the specified data from the LMS. - * If this succeeds the cache shall be updated with the received quizzes data and return them. - * If this is not possible within a certain time, the implementation shall get as much of the - * requested data from the cache and return them to the caller to not block the call too long - * and allow the caller to return fast and present as much data as possible.
- * This can be done with a {@link MemoizingCircuitBreaker} or a simple {@link CircuitBreaker} - * with a separated cache, for example. The abstract implementation; {@link AbstractCourseAccess} - * provides already defined wrapped circuit breaker for each call. To use it, just extend the - * abstract class and implement the needed suppliers.
- * On the other hand, dedicated cache access functions like {@link #getQuizzesFromCache(Set ids) } - * shall always first look into the cache to geht the requested data and if not - * available, call the LMS and request the data from the LMS. If partial data is needed to get - * be requested from the LMS, this functions shall also update the catch with the requested - * and cache missed data afterwards. + * active courses, the course API can implement some short time caches if needed.
+ * The abstract class {@link AbstractCachedCourseAccess} defines such a short time + * cache for all implementing classes using EH-Cache. See documentation on the class for + * more information. *

* SEB restriction API
* For this API we need no caching since this is mostly about pushing data to the LMS for the LMS @@ -120,15 +111,14 @@ public interface LmsAPITemplate { Result> getQuizzes(FilterMap filterMap); /** Get all {@link QuizData } for the set of {@link QuizData } identifiers from LMS API in a collection - * of Result. If particular quiz cannot be loaded because of errors or deletion, - * the Result will have an error reference. + * of Result. If particular quizzes cannot be loaded because of errors or deletion, + * the the referencing QuizData will not be in the resulting list and an error is logged. * * @param ids the Set of Quiz identifiers to get the {@link QuizData } for * @return Collection of all {@link QuizData } from the given id set */ - Collection> getQuizzes(Set ids); + Result> getQuizzes(Set ids); /** Get the quiz data with specified identifier. - * * * @param id the quiz data identifier * @return Result refer to the quiz data or to an error when happened */ diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/AbstractCourseAccess.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/AbstractCourseAccess.java index 1f095800..2853abdd 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/AbstractCourseAccess.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/AbstractCourseAccess.java @@ -8,6 +8,7 @@ package ch.ethz.seb.sebserver.webservice.servicelayer.lms.impl; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Set; @@ -44,7 +45,11 @@ public abstract class AbstractCourseAccess { } /** CircuitBreaker for protected quiz and course data requests */ - protected final CircuitBreaker> quizzesRequest; + protected final CircuitBreaker> allQuizzesRequest; + /** CircuitBreaker for protected quiz and course data requests */ + protected final CircuitBreaker> quizzesRequest; + /** CircuitBreaker for protected quiz and course data requests */ + protected final CircuitBreaker quizRequest; /** CircuitBreaker for protected chapter data requests */ protected final CircuitBreaker chaptersRequest; /** CircuitBreaker for protected examinee account details requests */ @@ -54,6 +59,20 @@ public abstract class AbstractCourseAccess { final AsyncService asyncService, final Environment environment) { + this.allQuizzesRequest = asyncService.createCircuitBreaker( + environment.getProperty( + "sebserver.webservice.circuitbreaker.quizzesRequest.attempts", + Integer.class, + 3), + environment.getProperty( + "sebserver.webservice.circuitbreaker.quizzesRequest.blockingTime", + Long.class, + Constants.MINUTE_IN_MILLIS), + environment.getProperty( + "sebserver.webservice.circuitbreaker.quizzesRequest.timeToRecover", + Long.class, + Constants.MINUTE_IN_MILLIS)); + this.quizzesRequest = asyncService.createCircuitBreaker( environment.getProperty( "sebserver.webservice.circuitbreaker.quizzesRequest.attempts", @@ -68,6 +87,20 @@ public abstract class AbstractCourseAccess { Long.class, Constants.MINUTE_IN_MILLIS)); + this.quizRequest = asyncService.createCircuitBreaker( + environment.getProperty( + "sebserver.webservice.circuitbreaker.quizzesRequest.attempts", + Integer.class, + 3), + environment.getProperty( + "sebserver.webservice.circuitbreaker.quizzesRequest.blockingTime", + Long.class, + Constants.MINUTE_IN_MILLIS), + environment.getProperty( + "sebserver.webservice.circuitbreaker.quizzesRequest.timeToRecover", + Long.class, + Constants.MINUTE_IN_MILLIS)); + this.chaptersRequest = asyncService.createCircuitBreaker( environment.getProperty( "sebserver.webservice.circuitbreaker.chaptersRequest.attempts", @@ -97,8 +130,18 @@ public abstract class AbstractCourseAccess { Constants.SECOND_IN_MILLIS * 10)); } - public Result> getQuizzes(final FilterMap filterMap) { - return this.quizzesRequest.protectedRun(allQuizzesSupplier(filterMap)); + public Result> protectedQuizzesRequest(final FilterMap filterMap) { + return this.allQuizzesRequest.protectedRun(allQuizzesSupplier(filterMap)); + } + + public Collection protectedQuizzesRequest(final Set ids) { + return this.quizzesRequest.protectedRun(quizzesSupplier(ids)) + .onError(error -> log.error("Failed to get QuizData for ids: ", error)) + .getOrElse(() -> Collections.emptyList()); + } + + public Result protectedQuizRequest(final String id) { + return this.quizRequest.protectedRun(quizSupplier(id)); } public Result getExamineeAccountDetails(final String examineeSessionId) { @@ -116,7 +159,7 @@ public abstract class AbstractCourseAccess { .getOr(examineeSessionId); } - protected Result getCourseChapters(final String courseId) { + public Result getCourseChapters(final String courseId) { return this.chaptersRequest.protectedRun(getCourseChaptersSupplier(courseId)); } @@ -134,12 +177,15 @@ public abstract class AbstractCourseAccess { Collections.emptyMap()); } - /** Provides a supplier for the quiz data request to use within the circuit breaker */ - protected abstract Supplier> quizzesSupplier(final Set ids); - /** Provides a supplier to supply request to use within the circuit breaker */ protected abstract Supplier> allQuizzesSupplier(final FilterMap filterMap); + /** Provides a supplier for the quiz data request to use within the circuit breaker */ + protected abstract Supplier> quizzesSupplier(final Set ids); + + /** Provides a supplier for the quiz data request to use within the circuit breaker */ + protected abstract Supplier quizSupplier(final String id); + /** Provides a supplier for the course chapter data request to use within the circuit breaker */ protected abstract Supplier getCourseChaptersSupplier(final String courseId); diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/edx/OpenEdxCourseAccess.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/edx/OpenEdxCourseAccess.java index 40bb11cd..71f87985 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/edx/OpenEdxCourseAccess.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/edx/OpenEdxCourseAccess.java @@ -12,12 +12,10 @@ import java.net.URL; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.NoSuchElementException; import java.util.Set; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -198,57 +196,25 @@ final class OpenEdxCourseAccess extends AbstractCachedCourseAccess { .getOrThrow(); } - public Collection> getQuizzesFromCache(final Set ids) { - final HashSet leftIds = new HashSet<>(ids); - final Collection> result = new ArrayList<>(); - ids.stream() - .map(this::getQuizFromCache) - .forEach(q -> { - if (q != null) { - leftIds.remove(q.id); - result.add(Result.of(q)); - } - }); + @Override + protected Supplier quizSupplier(final String id) { + return () -> { + final LmsSetup lmsSetup = getApiTemplateDataSupplier().getLmsSetup(); + final String externalStartURI = getExternalLMSServerAddress(lmsSetup); + final QuizData quizData = quizDataOf( + lmsSetup, + this.getOneCourse(id, this.restTemplate, id), + externalStartURI); - if (!leftIds.isEmpty()) { - super.quizzesRequest.protectedRun(this.quizzesSupplier(leftIds)) - .onError(error -> log.error("Failed to get quizzes by ids: ", error)) - .getOrElse(() -> Collections.emptyList()) - .stream() - .forEach(q -> { - leftIds.remove(q.id); - result.add(Result.of(q)); - }); - } - - if (!leftIds.isEmpty()) { - leftIds.forEach(q -> result.add(Result.ofError(new NoSuchElementException()))); - } - - return result; - } - - public QuizData getQuizFromCache(final String id) { - return super.getFromCache(id); - } - - public QuizData getQuizFromLMS(final String id) { - final LmsSetup lmsSetup = getApiTemplateDataSupplier().getLmsSetup(); - final String externalStartURI = getExternalLMSServerAddress(lmsSetup); - final QuizData quizData = quizDataOf( - lmsSetup, - this.getOneCourse(id, this.restTemplate, id), - externalStartURI); - - if (quizData != null) { - super.putToCache(quizData); - } - return quizData; + if (quizData != null) { + super.putToCache(quizData); + } + return quizData; + }; } @Override - protected Supplier> quizzesSupplier(final Set ids) { - + protected Supplier> quizzesSupplier(final Set ids) { if (ids.size() == 1) { return () -> { @@ -295,6 +261,31 @@ final class OpenEdxCourseAccess extends AbstractCachedCourseAccess { }; } + public Result> getQuizzesFromCache(final Set ids) { + return Result.tryCatch(() -> { + final HashSet leftIds = new HashSet<>(ids); + final Collection result = new ArrayList<>(); + ids.stream() + .map(this::getQuizFromCache) + .forEach(q -> { + if (q != null) { + leftIds.remove(q.id); + result.add(q); + } + }); + + if (!leftIds.isEmpty()) { + result.addAll(super.protectedQuizzesRequest(leftIds)); + } + + return result; + }); + } + + public QuizData getQuizFromCache(final String id) { + return super.getFromCache(id); + } + private ArrayList collectQuizzes(final OAuth2RestTemplate restTemplate, final Set ids) { final LmsSetup lmsSetup = getApiTemplateDataSupplier().getLmsSetup(); final String externalStartURI = getExternalLMSServerAddress(lmsSetup); diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/edx/OpenEdxLmsAPITemplate.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/edx/OpenEdxLmsAPITemplate.java index 7713e792..8975f97a 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/edx/OpenEdxLmsAPITemplate.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/edx/OpenEdxLmsAPITemplate.java @@ -74,7 +74,7 @@ final class OpenEdxLmsAPITemplate implements LmsAPITemplate { @Override public Result> getQuizzes(final FilterMap filterMap) { return this.openEdxCourseAccess - .getQuizzes(filterMap) + .protectedQuizzesRequest(filterMap) .map(quizzes -> quizzes.stream() .filter(LmsAPIService.quizFilterPredicate(filterMap)) .collect(Collectors.toList())); @@ -82,18 +82,16 @@ final class OpenEdxLmsAPITemplate implements LmsAPITemplate { @Override public Result getQuiz(final String id) { - return Result.tryCatch(() -> { - final QuizData quizFromCache = this.openEdxCourseAccess.getQuizFromCache(id); - if (quizFromCache != null) { - return quizFromCache; - } + final QuizData quizFromCache = this.openEdxCourseAccess.getQuizFromCache(id); + if (quizFromCache != null) { + return Result.of(quizFromCache); + } - return this.openEdxCourseAccess.getQuizFromLMS(id); - }); + return this.openEdxCourseAccess.protectedQuizRequest(id); } @Override - public Collection> getQuizzes(final Set ids) { + public Result> getQuizzes(final Set ids) { return this.openEdxCourseAccess.getQuizzesFromCache(ids); } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/mockup/MockupLmsAPITemplate.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/mockup/MockupLmsAPITemplate.java index 4ca961e5..cad64113 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/mockup/MockupLmsAPITemplate.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/mockup/MockupLmsAPITemplate.java @@ -172,17 +172,18 @@ public class MockupLmsAPITemplate implements LmsAPITemplate { } @Override - public Collection> getQuizzes(final Set ids) { - if (!authenticate()) { - throw new IllegalArgumentException("Wrong clientId or secret"); - } + public Result> getQuizzes(final Set ids) { + return Result.tryCatch(() -> { + if (!authenticate()) { + throw new IllegalArgumentException("Wrong clientId or secret"); + } - return this.mockups - .stream() - .map(this::getExternalAddressAlias) - .filter(mock -> ids.contains(mock.id)) - .map(Result::of) - .collect(Collectors.toList()); + return this.mockups + .stream() + .map(this::getExternalAddressAlias) + .filter(mock -> ids.contains(mock.id)) + .collect(Collectors.toList()); + }); } @Override diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/moodle/MoodleCourseAccess.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/moodle/MoodleCourseAccess.java index 3b5e328c..a0fb86ff 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/moodle/MoodleCourseAccess.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/moodle/MoodleCourseAccess.java @@ -14,7 +14,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.NoSuchElementException; import java.util.Set; import java.util.function.Function; import java.util.function.Supplier; @@ -245,52 +244,54 @@ public class MoodleCourseAccess extends AbstractCourseAccess { } } - // get from LMS - final Set ids = Stream.of(id).collect(Collectors.toSet()); - return super.quizzesRequest - .protectedRun(quizzesSupplier(ids)) - .getOrThrow() - .get(0); + // get from LMS in protected request + return super.protectedQuizRequest(id).getOrThrow(); }); } - public Collection> getQuizzesFromCache(final Set ids) { - final List cached = getCached(); - final List available = (cached != null) - ? cached - : Collections.emptyList(); + public Result> getQuizzesFromCache(final Set ids) { + return Result.tryCatch(() -> { + final List cached = getCached(); + final List available = (cached != null) + ? cached + : Collections.emptyList(); - final Map quizMapping = available - .stream() - .collect(Collectors.toMap(q -> q.id, Function.identity())); - - if (!quizMapping.keySet().containsAll(ids)) { - - final Map collect = super.quizzesRequest - .protectedRun(quizzesSupplier(ids)) - .onError(error -> log.error("Failed to get quizzes by ids: ", error)) - .getOrElse(() -> Collections.emptyList()) + final Map quizMapping = available .stream() - .collect(Collectors.toMap(qd -> qd.id, Function.identity())); - if (collect != null) { - quizMapping.clear(); - quizMapping.putAll(collect); - } - } + .collect(Collectors.toMap(q -> q.id, Function.identity())); - return ids - .stream() - .map(id -> { - final QuizData q = quizMapping.get(id); - return (q == null) - ? Result. ofError(new NoSuchElementException("Quiz with id: " + id)) - : Result.of(q); - }) - .collect(Collectors.toList()); + if (!quizMapping.keySet().containsAll(ids)) { + + final Map collect = super.quizzesRequest + .protectedRun(quizzesSupplier(ids)) + .onError(error -> log.error("Failed to get quizzes by ids: ", error)) + .getOrElse(() -> Collections.emptyList()) + .stream() + .collect(Collectors.toMap(qd -> qd.id, Function.identity())); + if (collect != null) { + quizMapping.clear(); + quizMapping.putAll(collect); + } + } + + return quizMapping.values(); + + }); } @Override - protected Supplier> quizzesSupplier(final Set ids) { + protected Supplier quizSupplier(final String id) { + return () -> { + final Set ids = Stream.of(id).collect(Collectors.toSet()); + return getRestTemplate() + .map(template -> getQuizzesForIds(template, ids)) + .getOr(Collections.emptyList()) + .get(0); + }; + } + + @Override + protected Supplier> quizzesSupplier(final Set ids) { return () -> getRestTemplate() .map(template -> getQuizzesForIds(template, ids)) .getOr(Collections.emptyList()); diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/moodle/MoodleLmsAPITemplate.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/moodle/MoodleLmsAPITemplate.java index 72086e70..264d24ec 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/moodle/MoodleLmsAPITemplate.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/lms/impl/moodle/MoodleLmsAPITemplate.java @@ -86,7 +86,7 @@ public class MoodleLmsAPITemplate implements LmsAPITemplate { @Override public Result> getQuizzes(final FilterMap filterMap) { return this.moodleCourseAccess - .getQuizzes(filterMap) + .protectedQuizzesRequest(filterMap) .map(quizzes -> quizzes.stream() .filter(LmsAPIService.quizFilterPredicate(filterMap)) .collect(Collectors.toList())); @@ -98,7 +98,7 @@ public class MoodleLmsAPITemplate implements LmsAPITemplate { } @Override - public Collection> getQuizzes(final Set ids) { + public Result> getQuizzes(final Set ids) { return this.moodleCourseAccess.getQuizzesFromCache(ids); }