diff --git a/src/main/java/org/opentripplanner/middleware/OtpMiddlewareMain.java b/src/main/java/org/opentripplanner/middleware/OtpMiddlewareMain.java index d072c82e3..7c19889e6 100644 --- a/src/main/java/org/opentripplanner/middleware/OtpMiddlewareMain.java +++ b/src/main/java/org/opentripplanner/middleware/OtpMiddlewareMain.java @@ -112,7 +112,7 @@ private static void initializeHttpEndpoints() throws IOException, InterruptedExc * _encoded_ URL e.g. http://localhost:3000/#/register which allows for greater flexibility. */ spark.get("/register", (request, response) -> { - String route = HttpUtils.getRequiredQueryParamFromRequest(request, "route", false); + String route = HttpUtils.getQueryParamFromRequest(request, "route", false); if (route == null) { logMessageAndHalt(request, HttpStatus.BAD_REQUEST_400, diff --git a/src/main/java/org/opentripplanner/middleware/controllers/api/ApiController.java b/src/main/java/org/opentripplanner/middleware/controllers/api/ApiController.java index 9b9ee685e..c99fbac06 100644 --- a/src/main/java/org/opentripplanner/middleware/controllers/api/ApiController.java +++ b/src/main/java/org/opentripplanner/middleware/controllers/api/ApiController.java @@ -2,13 +2,14 @@ import com.beerboy.ss.ApiEndpoint; import com.beerboy.ss.SparkSwagger; +import com.beerboy.ss.descriptor.ParameterDescriptor; import com.beerboy.ss.rest.Endpoint; import com.fasterxml.jackson.core.JsonProcessingException; import com.mongodb.client.model.Filters; -import org.bson.conversions.Bson; import org.eclipse.jetty.http.HttpStatus; import org.opentripplanner.middleware.auth.Auth0Connection; import org.opentripplanner.middleware.auth.Auth0UserProfile; +import org.opentripplanner.middleware.controllers.response.ResponseList; import org.opentripplanner.middleware.models.Model; import org.opentripplanner.middleware.models.OtpUser; import org.opentripplanner.middleware.persistence.TypedPersistence; @@ -21,9 +22,7 @@ import spark.Request; import spark.Response; -import java.lang.reflect.Array; import java.util.Date; -import java.util.List; import static com.beerboy.ss.descriptor.EndpointDescriptor.endpointPath; import static com.beerboy.ss.descriptor.MethodDescriptor.path; @@ -38,8 +37,8 @@ * methods. This will identify the MongoDB collection on which to operate based on the provided {@link Model} class. * * TODO: Add hooks so that validation can be performed on certain methods (e.g., validating fields on create/update, - * checking user permissions to perform certain actions, checking whether an entity can be deleted due to references - * that exist in other collection). + * checking user permissions to perform certain actions, checking whether an entity can be deleted due to references + * that exist in other collection). * * @param One of the {@link Model} classes (extracted from {@link TypedPersistence}) */ @@ -52,6 +51,19 @@ public abstract class ApiController implements Endpoint { private static final Logger LOG = LoggerFactory.getLogger(ApiController.class); final TypedPersistence persistence; private final Class clazz; + public static final String LIMIT_PARAM = "limit"; + public static final int DEFAULT_LIMIT = 10; + public static final int DEFAULT_OFFSET = 0; + public static final String OFFSET_PARAM = "offset"; + + public static final ParameterDescriptor LIMIT = ParameterDescriptor.newBuilder() + .withName(LIMIT_PARAM) + .withDefaultValue(String.valueOf(DEFAULT_LIMIT)) + .withDescription("If specified, the maximum number of items to return.").build(); + public static final ParameterDescriptor OFFSET = ParameterDescriptor.newBuilder() + .withName(OFFSET_PARAM) + .withDefaultValue(String.valueOf(DEFAULT_OFFSET)) + .withDescription("If specified, the number of records to skip/offset.").build(); /** * @param apiPrefix string prefix to use in determining the resource location @@ -112,19 +124,19 @@ protected void buildEndpoint(ApiEndpoint baseEndpoint) { baseEndpoint // Get multiple entities. - .get(path(ROOT_ROUTE) - .withDescription("Gets a list of all '" + className + "' entities.") + .get( + path(ROOT_ROUTE) + .withDescription("Gets a paginated list of all '" + className + "' entities.") + .withQueryParam(LIMIT) + .withQueryParam(OFFSET) .withProduces(JSON_ONLY) - // Set the return type as the array of clazz objects. - // Note: there exists a method withResponseAsCollection, but unlike what its name suggests, - // it does exactly the same as .withResponseType and does not generate a return type array. - // See issue https://github.com/manusant/spark-swagger/issues/12. - .withResponseType(Array.newInstance(clazz, 0).getClass()), + .withResponseType(ResponseList.class), this::getMany, JsonUtils::toJson ) // Get one entity. - .get(path(ROOT_ROUTE + ID_PATH) + .get( + path(ROOT_ROUTE + ID_PATH) .withDescription("Returns the '" + className + "' entity with the specified id, or 404 if not found.") .withPathParam().withName(ID_PARAM).withRequired(true).withDescription("The id of the entity to search.").and() // .withResponses(...) // FIXME: not implemented (requires source change). @@ -134,7 +146,8 @@ protected void buildEndpoint(ApiEndpoint baseEndpoint) { ) // Create entity request - .post(path("") + .post( + path("") .withDescription("Creates a '" + className + "' entity.") .withConsumes(JSON_ONLY) .withRequestType(clazz) @@ -144,7 +157,8 @@ protected void buildEndpoint(ApiEndpoint baseEndpoint) { ) // Update entity request - .put(path(ID_PATH) + .put( + path(ID_PATH) .withDescription("Updates and returns the '" + className + "' entity with the specified id, or 404 if not found.") .withPathParam().withName(ID_PARAM).withRequired(true).withDescription("The id of the entity to update.").and() .withConsumes(JSON_ONLY) @@ -158,7 +172,8 @@ protected void buildEndpoint(ApiEndpoint baseEndpoint) { ) // Delete entity request - .delete(path(ID_PATH) + .delete( + path(ID_PATH) .withDescription("Deletes the '" + className + "' entity with the specified id if it exists.") .withPathParam().withName(ID_PARAM).withRequired(true).withDescription("The id of the entity to delete.").and() .withProduces(JSON_ONLY) @@ -172,33 +187,26 @@ protected void buildEndpoint(ApiEndpoint baseEndpoint) { */ // FIXME Maybe better if the user check (and filtering) was done in a pre hook? // FIXME Will require further granularity for admin - private List getMany(Request req, Response res) { - + private ResponseList getMany(Request req, Response res) { + int limit = HttpUtils.getQueryParamFromRequest(req, LIMIT_PARAM, 0, DEFAULT_LIMIT, 100); + int offset = HttpUtils.getQueryParamFromRequest(req, OFFSET_PARAM, 0, DEFAULT_OFFSET); Auth0UserProfile requestingUser = getUserFromRequest(req); if (isUserAdmin(requestingUser)) { // If the user is admin, the context is presumed to be the admin dashboard, so we deliver all entities for // management or review without restriction. - return persistence.getAll(); + return persistence.getResponseList(offset, limit); } else if (persistence.clazz == OtpUser.class) { // If the required entity is of type 'OtpUser' the assumption is that a call is being made via the // OtpUserController. Therefore, the request should be limited to return just the entity matching the // requesting user. - return getObjectsFiltered("_id", requestingUser.otpUser.id); + return persistence.getResponseList(Filters.eq("_id", requestingUser.otpUser.id), offset, limit); } else { // For all other cases the assumption is that the request is being made by an Otp user and the requested // entities have a 'userId' parameter. Only entities that match the requesting user id are returned. - return getObjectsFiltered("userId", requestingUser.otpUser.id); + return persistence.getResponseList(Filters.eq("userId", requestingUser.otpUser.id), offset, limit); } } - /** - * Get a list of objects filtered by the provided field name and value. - */ - private List getObjectsFiltered(String fieldName, String value) { - Bson filter = Filters.eq(fieldName, value); - return persistence.getFiltered(filter); - } - /** * HTTP endpoint to get one entity specified by ID. This will return an object based on the checks carried out in * the overridden 'canBeManagedBy' method. It is the responsibility of this method to define access to it's own diff --git a/src/main/java/org/opentripplanner/middleware/controllers/api/BugsnagController.java b/src/main/java/org/opentripplanner/middleware/controllers/api/BugsnagController.java index 2026ffa4f..4c0a2f82c 100644 --- a/src/main/java/org/opentripplanner/middleware/controllers/api/BugsnagController.java +++ b/src/main/java/org/opentripplanner/middleware/controllers/api/BugsnagController.java @@ -2,9 +2,11 @@ import com.beerboy.ss.SparkSwagger; import com.beerboy.ss.rest.Endpoint; -import com.mongodb.client.model.Filters; -import org.bson.conversions.Bson; +import com.google.common.collect.Maps; +import com.mongodb.client.FindIterable; +import com.mongodb.client.model.Sorts; import org.opentripplanner.middleware.bugsnag.EventSummary; +import org.opentripplanner.middleware.controllers.response.ResponseList; import org.opentripplanner.middleware.models.BugsnagEvent; import org.opentripplanner.middleware.models.BugsnagProject; import org.opentripplanner.middleware.persistence.Persistence; @@ -16,9 +18,15 @@ import java.util.ArrayList; import java.util.List; +import java.util.Map; import static com.beerboy.ss.descriptor.EndpointDescriptor.endpointPath; import static com.beerboy.ss.descriptor.MethodDescriptor.path; +import static org.opentripplanner.middleware.controllers.api.ApiController.DEFAULT_LIMIT; +import static org.opentripplanner.middleware.controllers.api.ApiController.LIMIT; +import static org.opentripplanner.middleware.controllers.api.ApiController.LIMIT_PARAM; +import static org.opentripplanner.middleware.controllers.api.ApiController.OFFSET; +import static org.opentripplanner.middleware.controllers.api.ApiController.OFFSET_PARAM; import static org.opentripplanner.middleware.utils.HttpUtils.JSON_ONLY; /** @@ -44,29 +52,38 @@ public void bind(final SparkSwagger restApi) { restApi.endpoint( endpointPath(ROOT_ROUTE).withDescription("Interface for reporting and retrieving application errors using Bugsnag."), HttpUtils.NO_FILTER - ).get(path(ROOT_ROUTE) - .withDescription("Gets a list of all Bugsnag event summaries.") + ).get( + path(ROOT_ROUTE) + .withDescription("Gets a paginated list of the latest Bugsnag event summaries.") + .withQueryParam(LIMIT) + .withQueryParam(OFFSET) .withProduces(JSON_ONLY) // Note: unlike what the name suggests, withResponseAsCollection does not generate an array // as the return type for this method. (It does generate the type for that class nonetheless.) .withResponseAsCollection(BugsnagEvent.class), - BugsnagController::getEventSummary, JsonUtils::toJson); + BugsnagController::getEventSummaries, JsonUtils::toJson); } /** - * Get all Bugsnag events from Mongo and replace the project id with the project name and return + * Get the latest Bugsnag {@link EventSummary} from MongoDB (event summary is composed of {@link BugsnagEvent} and + * {@link BugsnagProject}. */ - private static List getEventSummary(Request request, Response response) { - List eventSummaries = new ArrayList<>(); - List events = bugsnagEvents.getAll(); - + private static ResponseList getEventSummaries(Request req, Response res) { + int limit = HttpUtils.getQueryParamFromRequest(req, LIMIT_PARAM, 0, DEFAULT_LIMIT, 100); + int offset = HttpUtils.getQueryParamFromRequest(req, OFFSET_PARAM, 0, 0); + // Get latest events from database. + FindIterable events = bugsnagEvents.getSortedIterableWithOffsetAndLimit( + Sorts.descending("receivedAt"), + offset, + limit + ); + // Get Bugsnag projects by id (avoid multiple queries to Mongo for the same project). + Map projectsById = Maps.uniqueIndex(bugsnagProjects.getAll(), p -> p.projectId); + // Construct event summaries from project map. // FIXME: Group by error/project type? - for (BugsnagEvent event : events) { - Bson filter = Filters.eq("projectId", event.projectId); - BugsnagProject project = bugsnagProjects.getOneFiltered(filter); - eventSummaries.add(new EventSummary(project, event)); - } - - return eventSummaries; + List eventSummaries = events + .map(event -> new EventSummary(projectsById.get(event.projectId), event)) + .into(new ArrayList<>()); + return new ResponseList<>(EventSummary.class, eventSummaries, offset, limit, bugsnagEvents.getCount()); } } diff --git a/src/main/java/org/opentripplanner/middleware/controllers/api/TripHistoryController.java b/src/main/java/org/opentripplanner/middleware/controllers/api/TripHistoryController.java index 7340251f5..43beaf555 100644 --- a/src/main/java/org/opentripplanner/middleware/controllers/api/TripHistoryController.java +++ b/src/main/java/org/opentripplanner/middleware/controllers/api/TripHistoryController.java @@ -2,13 +2,14 @@ import com.beerboy.ss.SparkSwagger; import com.beerboy.ss.rest.Endpoint; +import org.bson.conversions.Bson; import org.eclipse.jetty.http.HttpStatus; +import org.opentripplanner.middleware.controllers.response.ResponseList; import org.opentripplanner.middleware.models.TripRequest; +import org.opentripplanner.middleware.persistence.Persistence; import org.opentripplanner.middleware.utils.DateTimeUtils; import org.opentripplanner.middleware.utils.HttpUtils; import org.opentripplanner.middleware.utils.JsonUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import spark.Request; import spark.Response; @@ -16,11 +17,17 @@ import java.time.LocalTime; import java.time.format.DateTimeParseException; import java.util.Date; -import java.util.List; import static com.beerboy.ss.descriptor.EndpointDescriptor.endpointPath; import static com.beerboy.ss.descriptor.MethodDescriptor.path; import static org.opentripplanner.middleware.auth.Auth0Connection.isAuthorized; +import static org.opentripplanner.middleware.controllers.api.ApiController.DEFAULT_LIMIT; +import static org.opentripplanner.middleware.controllers.api.ApiController.DEFAULT_OFFSET; +import static org.opentripplanner.middleware.controllers.api.ApiController.LIMIT; +import static org.opentripplanner.middleware.controllers.api.ApiController.LIMIT_PARAM; +import static org.opentripplanner.middleware.controllers.api.ApiController.OFFSET; +import static org.opentripplanner.middleware.controllers.api.ApiController.OFFSET_PARAM; +import static org.opentripplanner.middleware.persistence.TypedPersistence.filterByUserAndDateRange; import static org.opentripplanner.middleware.utils.DateTimeUtils.YYYY_MM_DD; import static org.opentripplanner.middleware.utils.HttpUtils.JSON_ONLY; import static org.opentripplanner.middleware.utils.JsonUtils.logMessageAndHalt; @@ -30,14 +37,8 @@ * To provide a response to the calling MOD UI in JSON based on the passed in parameters. */ public class TripHistoryController implements Endpoint { - - private static final Logger LOG = LoggerFactory.getLogger(TripHistoryController.class); - - private static final String FROM_DATE_PARAM_NAME = "fromDate"; - private static final String TO_DATE_PARAM_NAME = "toDate"; - private static final String LIMIT_PARAM_NAME = "limit"; - private static final int DEFAULT_LIMIT = 10; - + private static final String FROM_DATE_PARAM = "fromDate"; + private static final String TO_DATE_PARAM = "toDate"; private final String ROOT_ROUTE; public TripHistoryController(String apiPrefix) { @@ -55,33 +56,31 @@ public void bind(final SparkSwagger restApi) { HttpUtils.NO_FILTER ).get( path(ROOT_ROUTE) - .withDescription("Gets a list of all trip requests for a user.") + .withDescription("Gets a paginated list of the most recent trip requests for a user.") .withQueryParam() .withName("userId") .withRequired(true) .withDescription("The OTP user for which to retrieve trip requests.").and() - .withQueryParam() - .withName(LIMIT_PARAM_NAME) - .withDefaultValue(String.valueOf(DEFAULT_LIMIT)) - .withDescription("If specified, the maximum number of trip requests to return, starting from the most recent.").and() - .withQueryParam() - .withName(FROM_DATE_PARAM_NAME) + .withQueryParam(LIMIT) + .withQueryParam(OFFSET) + .withQueryParam() + .withName(FROM_DATE_PARAM) .withPattern(YYYY_MM_DD) .withDefaultValue("The current date") .withDescription(String.format( "If specified, the earliest date (format %s) for which trip requests are retrieved.", YYYY_MM_DD )).and() - .withQueryParam() - .withName(TO_DATE_PARAM_NAME) + .withQueryParam() + .withName(TO_DATE_PARAM) .withPattern(YYYY_MM_DD) .withDefaultValue("The current date") .withDescription(String.format( - "If specified, the latest date (format %s) for which usage logs are retrieved.", YYYY_MM_DD + "If specified, the latest date (format %s) for which trip requests are retrieved.", YYYY_MM_DD )).and() - .withProduces(JSON_ONLY) - // Note: unlike the name suggests, withResponseAsCollection does not generate an array - // as the return type for this method. (It does generate the type for that class nonetheless.) - .withResponseAsCollection(TripRequest.class), + .withProduces(JSON_ONLY) + // Note: unlike the name suggests, withResponseAsCollection does not generate an array + // as the return type for this method. (It does generate the type for that class nonetheless.) + .withResponseAsCollection(TripRequest.class), TripHistoryController::getTripRequests, JsonUtils::toJson); } @@ -89,43 +88,29 @@ public void bind(final SparkSwagger restApi) { * Return a user's trip request history based on provided parameters. * An authorized user (Auth0) and user id are required. */ - private static List getTripRequests(Request request, Response response) { - final String userId = HttpUtils.getRequiredQueryParamFromRequest(request, "userId", false); + private static ResponseList getTripRequests(Request request, Response response) { + final String userId = HttpUtils.getQueryParamFromRequest(request, "userId", false); // Check that the user is authorized (otherwise a halt is thrown). isAuthorized(userId, request); - - int limit = DEFAULT_LIMIT; - - String paramLimit = null; - try { - paramLimit = HttpUtils.getRequiredQueryParamFromRequest(request, LIMIT_PARAM_NAME, true); - if (paramLimit != null) { - limit = Integer.parseInt(paramLimit); - if (limit <= 0) { - limit = DEFAULT_LIMIT; - } - } - } catch (NumberFormatException e) { - LOG.error("Unable to parse {} value of {}. Using default limit: {}", LIMIT_PARAM_NAME, - paramLimit, DEFAULT_LIMIT, e); - } - - String paramFromDate = HttpUtils.getRequiredQueryParamFromRequest(request, FROM_DATE_PARAM_NAME, true); - Date fromDate = getDate(request, FROM_DATE_PARAM_NAME, paramFromDate, LocalTime.MIDNIGHT); - - String paramToDate = HttpUtils.getRequiredQueryParamFromRequest(request, TO_DATE_PARAM_NAME, true); - Date toDate = getDate(request, TO_DATE_PARAM_NAME, paramToDate, LocalTime.MAX); - + // Get params from request (or use defaults). + int limit = HttpUtils.getQueryParamFromRequest(request, LIMIT_PARAM, 0, DEFAULT_LIMIT, 100); + int offset = HttpUtils.getQueryParamFromRequest(request, OFFSET_PARAM, 0, DEFAULT_OFFSET); + String paramFromDate = HttpUtils.getQueryParamFromRequest(request, FROM_DATE_PARAM, true); + Date fromDate = getDate(request, FROM_DATE_PARAM, paramFromDate, LocalTime.MIDNIGHT); + String paramToDate = HttpUtils.getQueryParamFromRequest(request, TO_DATE_PARAM, true); + Date toDate = getDate(request, TO_DATE_PARAM, paramToDate, LocalTime.MAX); + // Throw halt if the date params are bad. if (fromDate != null && toDate != null && toDate.before(fromDate)) { logMessageAndHalt(request, HttpStatus.BAD_REQUEST_400, - String.format("%s (%s) before %s (%s)", TO_DATE_PARAM_NAME, paramToDate, FROM_DATE_PARAM_NAME, + String.format("%s (%s) before %s (%s)", TO_DATE_PARAM, paramToDate, FROM_DATE_PARAM, paramFromDate)); } - return TripRequest.requestsForUser(userId, fromDate, toDate, limit); + Bson filter = filterByUserAndDateRange(userId, fromDate, toDate); + return Persistence.tripRequests.getResponseList(filter, offset, limit); } /** - * Get date from request parameter and convert to java.util.Date at a specific time of day. The date conversion + * Get date from request parameter and convert to {@link Date} at a specific time of day. The date conversion * is based on the system time zone. */ private static Date getDate(Request request, String paramName, String paramValue, LocalTime timeOfDay) { diff --git a/src/main/java/org/opentripplanner/middleware/controllers/response/ResponseList.java b/src/main/java/org/opentripplanner/middleware/controllers/response/ResponseList.java new file mode 100644 index 000000000..a9e09d751 --- /dev/null +++ b/src/main/java/org/opentripplanner/middleware/controllers/response/ResponseList.java @@ -0,0 +1,77 @@ +package org.opentripplanner.middleware.controllers.response; + +import com.mongodb.client.MongoCollection; +import org.bson.conversions.Bson; + +import java.util.ArrayList; +import java.util.Date; +import java.util.List; + +/** + * Generic class for wrapping a paginated list response for a 'get all' HTTP endpoint. + */ +public class ResponseList { + /** + * Simple class name representing the data type. + * TODO: remove in favor of better approach to swagger doc generation? + */ + public String clazz; + /** Data elements requested */ + public List data; + /** number of results by which to offset */ + public int offset; + /** number of results by which the response should be limited */ + public int limit; + /** total results found in query */ + public long total; + /** time that response was constructed */ + public Date timestamp; + + /** + * No-arg constructor for de/serialization. + */ + public ResponseList() { } + + /** + * Primary constructor for generating a paginated response list. + * @param collection - Mongo collection from which to construct the list + * @param filter - filter (query) to apply to find operation + * @param offset - number of results by which to offset + * @param limit - number of results by which the response should be limited + */ + public ResponseList(MongoCollection collection, Bson filter, int offset, int limit){ + this( + collection.getDocumentClass(), + collection.find(filter).skip(offset).limit(limit).into(new ArrayList<>()), + offset, + limit, + collection.countDocuments(filter) + ); + } + + /** + * Shorthand constructor for generating an unfiltered response list. + */ + public ResponseList(MongoCollection collection, int offset, int limit){ + this( + collection.getDocumentClass(), + collection.find().skip(offset).limit(limit).into(new ArrayList<>()), + offset, + limit, + collection.countDocuments() + ); + } + + /** + * Alternate constructor to generate paginated response when the data cannot be derived directly from a MongoDB + * collection (e.g., with {@link org.opentripplanner.middleware.bugsnag.EventSummary}). + */ + public ResponseList(Class clazz, List data, int offset, int limit, long total){ + this.clazz = clazz.getSimpleName(); + this.data = data; + this.offset = offset; + this.limit = limit; + this.total = total; + this.timestamp = new Date(); + } +} diff --git a/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java b/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java index 9b5b5f123..ce0fc0734 100644 --- a/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java +++ b/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java @@ -1,5 +1,6 @@ package org.opentripplanner.middleware.models; +import com.mongodb.client.FindIterable; import org.bson.conversions.Bson; import org.opentripplanner.middleware.auth.Auth0UserProfile; import org.opentripplanner.middleware.auth.Permission; @@ -15,8 +16,6 @@ import static com.mongodb.client.model.Filters.eq; import org.opentripplanner.middleware.persistence.TypedPersistence; -import java.util.List; - /** * A monitored trip represents a trip a user would like to receive notification on if affected by a delay and/or route * change. @@ -270,7 +269,7 @@ public void clearRealtimeInfo() { /** * Get monitored trips for the specified {@link OtpUser} user Id. */ - public static List tripsForUser(String userId) { + public static FindIterable tripsForUser(String userId) { return Persistence.monitoredTrips.getFiltered(TypedPersistence.filterByUserId(userId)); } diff --git a/src/main/java/org/opentripplanner/middleware/models/TripRequest.java b/src/main/java/org/opentripplanner/middleware/models/TripRequest.java index d634518b7..2b2f9ee84 100644 --- a/src/main/java/org/opentripplanner/middleware/models/TripRequest.java +++ b/src/main/java/org/opentripplanner/middleware/models/TripRequest.java @@ -1,14 +1,11 @@ package org.opentripplanner.middleware.models; +import com.mongodb.client.FindIterable; import org.opentripplanner.middleware.persistence.Persistence; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.Date; -import java.util.List; - import static com.mongodb.client.model.Filters.eq; -import static org.opentripplanner.middleware.persistence.TypedPersistence.filterByUserAndDateRange; import static org.opentripplanner.middleware.persistence.TypedPersistence.filterByUserId; /** @@ -73,14 +70,10 @@ public String toString() { '}'; } - public static List requestsForUser(String userId) { + public static FindIterable requestsForUser(String userId) { return Persistence.tripRequests.getFiltered(filterByUserId(userId)); } - public static List requestsForUser(String userId, Date fromDate, Date toDate, int limit) { - return Persistence.tripRequests.getFilteredWithLimit(filterByUserAndDateRange(userId, fromDate, toDate), limit); - } - @Override public boolean delete() { boolean summariesDeleted = Persistence.tripSummaries.removeFiltered(eq("tripRequestId", this.id)); diff --git a/src/main/java/org/opentripplanner/middleware/persistence/TypedPersistence.java b/src/main/java/org/opentripplanner/middleware/persistence/TypedPersistence.java index 0541d8882..41678e111 100644 --- a/src/main/java/org/opentripplanner/middleware/persistence/TypedPersistence.java +++ b/src/main/java/org/opentripplanner/middleware/persistence/TypedPersistence.java @@ -10,6 +10,7 @@ import org.bson.Document; import org.bson.conversions.Bson; import org.opentripplanner.middleware.bugsnag.BugsnagReporter; +import org.opentripplanner.middleware.controllers.response.ResponseList; import org.opentripplanner.middleware.models.Model; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -136,14 +137,9 @@ public T getById(String id) { } /** - * This is not memory efficient. TODO: Always use iterators / streams, always perform selection of subsets on the - * Mongo server side ("where clause"). + * Get all as {@link FindIterable} (avoid getting all as {@link List} to avoid memory issues with large datasets). */ - public List getAll() { - return mongoCollection.find().into(new ArrayList<>()); - } - - public FindIterable getAllAsFindIterable() { + public FindIterable getAll() { return mongoCollection.find(); } @@ -151,8 +147,29 @@ public FindIterable getAllAsFindIterable() { * Get objects satisfying the supplied Mongo filter, limited to the specified maximum. This ties our persistence * directly to Mongo for now but is expedient. We should really have a bit more abstraction here. */ - public List getFilteredWithLimit(Bson filter, int maximum) { - return mongoCollection.find(filter).limit(maximum).into(new ArrayList<>()); + public List getFilteredWithLimit(Bson filter, int limit) { + return mongoCollection.find(filter).limit(limit).into(new ArrayList<>()); + } + + /** + * Get an optionally sorted set of records from the collection. + * @param sort - optional sort to apply to query (null value is OK) + * @param offset - number of records to skip + * @param limit - max number of records to return + */ + public FindIterable getSortedIterableWithOffsetAndLimit(Bson sort, int offset, int limit) { + return mongoCollection.find() + .sort(sort) + .skip(offset) + .limit(limit); + } + + public ResponseList getResponseList(int offset, int limit) { + return new ResponseList(mongoCollection, offset, limit); + } + + public ResponseList getResponseList(Bson filter, int offset, int limit) { + return new ResponseList(mongoCollection, filter, offset, limit); } /** @@ -190,12 +207,19 @@ public long getCountFiltered(Bson filter) { return mongoCollection.countDocuments(filter); } + /** + * Return the number of items in the collection. + */ + public long getCount() { + return mongoCollection.countDocuments(); + } + /** * Get all objects satisfying the supplied Mongo filter. This ties our persistence directly to Mongo for now but is * expedient. We should really have a bit more abstraction here. */ - public List getFiltered(Bson filter) { - return mongoCollection.find(filter).into(new ArrayList<>()); + public FindIterable getFiltered(Bson filter) { + return mongoCollection.find(filter); } /** diff --git a/src/main/java/org/opentripplanner/middleware/tripMonitor/jobs/MonitorAllTripsJob.java b/src/main/java/org/opentripplanner/middleware/tripMonitor/jobs/MonitorAllTripsJob.java index 7c6e0db50..293d7dfb1 100644 --- a/src/main/java/org/opentripplanner/middleware/tripMonitor/jobs/MonitorAllTripsJob.java +++ b/src/main/java/org/opentripplanner/middleware/tripMonitor/jobs/MonitorAllTripsJob.java @@ -50,7 +50,7 @@ public void run() { try { // request all monitored trips from the mongo collection - for (MonitoredTrip monitoredTrip : Persistence.monitoredTrips.getAllAsFindIterable()) { + for (MonitoredTrip monitoredTrip : Persistence.monitoredTrips.getAll()) { // attempt to add trip to tripAnalysisQueue until a spot opens up in the queue. If the timeout is // exceeded, an InterruptedException is throw. tripAnalysisQueue.offer(monitoredTrip, BLOCKING_QUEUE_INSERT_TIMEOUT_SECONDS, TimeUnit.SECONDS); diff --git a/src/main/java/org/opentripplanner/middleware/utils/ApiGatewayUtils.java b/src/main/java/org/opentripplanner/middleware/utils/ApiGatewayUtils.java index 2813cc4b3..e0a7eb7f7 100644 --- a/src/main/java/org/opentripplanner/middleware/utils/ApiGatewayUtils.java +++ b/src/main/java/org/opentripplanner/middleware/utils/ApiGatewayUtils.java @@ -4,9 +4,11 @@ import com.amazonaws.services.apigateway.AmazonApiGateway; import com.amazonaws.services.apigateway.AmazonApiGatewayClient; import com.amazonaws.services.apigateway.AmazonApiGatewayClientBuilder; +import com.amazonaws.services.apigateway.model.ConflictException; import com.amazonaws.services.apigateway.model.CreateApiKeyRequest; import com.amazonaws.services.apigateway.model.CreateApiKeyResult; import com.amazonaws.services.apigateway.model.CreateUsagePlanKeyRequest; +import com.amazonaws.services.apigateway.model.CreateUsagePlanKeyResult; import com.amazonaws.services.apigateway.model.DeleteApiKeyRequest; import com.amazonaws.services.apigateway.model.GetApiKeyRequest; import com.amazonaws.services.apigateway.model.GetUsagePlanRequest; @@ -70,22 +72,20 @@ public static ApiKey createApiKey(ApiUser user, String usagePlanId) throws Creat GetUsagePlanRequest usagePlanRequest = new GetUsagePlanRequest(); usagePlanRequest.withUsagePlanId(usagePlanId); GetUsagePlanResult usagePlanResult = gateway.getUsagePlan(usagePlanRequest); - - // Create API key with descriptive fields (for tracing back to users). - CreateApiKeyRequest apiKeyRequest = new CreateApiKeyRequest(); - apiKeyRequest.setSdkRequestTimeout(SDK_REQUEST_TIMEOUT); // Construct key name in the form email-planname-shortId (e.g., user@email.com-Unlimited-2). Note: shortId is // not intended to be unique, just for a bit of differentiation in the AWS console. String shortId = UUID.randomUUID().toString().substring(0, 7); String keyName = String.join("-", user.email, usagePlanResult.getName(), shortId); - apiKeyRequest + // Create API key with descriptive fields (for tracing back to users). + CreateApiKeyRequest apiKeyRequest = new CreateApiKeyRequest() //FIXME This may need to include stage key(s). Not sure what impact that places on the calling // services though? .withName(keyName) // TODO: On deleting am ApiUser, it might be worth doing a query on the userId tag to make sure the keys // have been cleared. .withTags(Collections.singletonMap("userId", user.id)) - .withEnabled(true); + .withEnabled(true) + .withSdkRequestTimeout(SDK_REQUEST_TIMEOUT); CreateApiKeyResult apiKeyResult = gateway.createApiKey(apiKeyRequest); // add API key to usage plan diff --git a/src/main/java/org/opentripplanner/middleware/utils/HttpUtils.java b/src/main/java/org/opentripplanner/middleware/utils/HttpUtils.java index edf7ac333..7f2bdc5ce 100644 --- a/src/main/java/org/opentripplanner/middleware/utils/HttpUtils.java +++ b/src/main/java/org/opentripplanner/middleware/utils/HttpUtils.java @@ -2,6 +2,8 @@ import org.eclipse.jetty.http.HttpStatus; import org.opentripplanner.middleware.bugsnag.BugsnagReporter; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import spark.Filter; import spark.Request; @@ -12,7 +14,6 @@ import java.net.http.HttpRequest; import java.net.http.HttpResponse; import java.time.Duration; -import java.util.HashMap; import java.util.List; import java.util.Map; @@ -20,7 +21,7 @@ import static org.opentripplanner.middleware.utils.JsonUtils.logMessageAndHalt; public class HttpUtils { - + private static final Logger LOG = LoggerFactory.getLogger(HttpUtils.class); public enum REQUEST_METHOD {GET, POST, DELETE} /** @@ -104,11 +105,42 @@ public static HttpResponse httpRequestRawResponse(URI uri, int connectio } /** - * Get entity attribute value from request. If nulls are not allowed, halt with error message. + * Get optional query param value from request as int (defaults to defaultValue). If parsed value is outside of the + * range of accepted values, it will be pinned to the min or max value (depending on which end of the range it is + * located). + */ + public static int getQueryParamFromRequest(Request req, String name, int min, int defaultValue, int max) { + // Start with default value + int value = defaultValue; + String requestValue = null; + try { + // Attempt to get value from query param. + requestValue = HttpUtils.getQueryParamFromRequest(req, name, true); + if (requestValue != null) { + value = Integer.parseInt(requestValue); + // If requested value is out of range, pin to min/max. + if (value < min) value = min; + else if (value > max) value = max; + } + } catch (NumberFormatException e) { + LOG.warn("Unable to parse {} value of {}. Using default limit: {}", name, requestValue, defaultValue, e); + } + return value; + } + + /** + * Get query param value from request as int with no maximum value. If not optional, halt with error message. + */ + public static int getQueryParamFromRequest(Request req, String name, int min, int defaultValue) { + return getQueryParamFromRequest(req, name, min, defaultValue, Integer.MAX_VALUE); + } + + /** + * Get query param value from request as string. If not optional/nulls not allowed, halt with error message. */ - public static String getRequiredQueryParamFromRequest(Request req, String paramName, boolean allowNull) { + public static String getQueryParamFromRequest(Request req, String paramName, boolean optional) { String paramValue = req.queryParams(paramName); - if (paramValue == null && !allowNull) { + if (paramValue == null && !optional) { logMessageAndHalt(req, HttpStatus.BAD_REQUEST_400, String.format("The parameter name (%s) must be provided.", paramName)); diff --git a/src/main/resources/latest-spark-swagger-output.yaml b/src/main/resources/latest-spark-swagger-output.yaml index b69926e4a..f798da4c2 100644 --- a/src/main/resources/latest-spark-swagger-output.yaml +++ b/src/main/resources/latest-spark-swagger-output.yaml @@ -64,15 +64,27 @@ paths: get: tags: - "api/admin/user" - description: "Gets a list of all 'AdminUser' entities." + description: "Gets a paginated list of all 'AdminUser' entities." produces: - "application/json" - parameters: [] + parameters: + - name: "limit" + in: "query" + description: "If specified, the maximum number of items to return." + required: false + type: "string" + default: "10" + - name: "offset" + in: "query" + description: "If specified, the number of records to skip/offset." + required: false + type: "string" + default: "0" responses: "200": description: "successful operation" schema: - $ref: "#/definitions/AdminUser[]" + $ref: "#/definitions/ResponseList" post: tags: - "api/admin/user" @@ -229,15 +241,27 @@ paths: get: tags: - "api/secure/application" - description: "Gets a list of all 'ApiUser' entities." + description: "Gets a paginated list of all 'ApiUser' entities." produces: - "application/json" - parameters: [] + parameters: + - name: "limit" + in: "query" + description: "If specified, the maximum number of items to return." + required: false + type: "string" + default: "10" + - name: "offset" + in: "query" + description: "If specified, the number of records to skip/offset." + required: false + type: "string" + default: "0" responses: "200": description: "successful operation" schema: - $ref: "#/definitions/ApiUser[]" + $ref: "#/definitions/ResponseList" post: tags: - "api/secure/application" @@ -322,15 +346,27 @@ paths: get: tags: - "api/secure/monitoredtrip" - description: "Gets a list of all 'MonitoredTrip' entities." + description: "Gets a paginated list of all 'MonitoredTrip' entities." produces: - "application/json" - parameters: [] + parameters: + - name: "limit" + in: "query" + description: "If specified, the maximum number of items to return." + required: false + type: "string" + default: "10" + - name: "offset" + in: "query" + description: "If specified, the number of records to skip/offset." + required: false + type: "string" + default: "0" responses: "200": description: "successful operation" schema: - $ref: "#/definitions/MonitoredTrip[]" + $ref: "#/definitions/ResponseList" post: tags: - "api/secure/monitoredtrip" @@ -416,7 +452,7 @@ paths: get: tags: - "api/secure/triprequests" - description: "Gets a list of all trip requests for a user." + description: "Gets a paginated list of the most recent trip requests for a user." produces: - "application/json" parameters: @@ -427,11 +463,16 @@ paths: type: "string" - name: "limit" in: "query" - description: "If specified, the maximum number of trip requests to return,\ - \ starting from the most recent." + description: "If specified, the maximum number of items to return." required: false type: "string" default: "10" + - name: "offset" + in: "query" + description: "If specified, the number of records to skip/offset." + required: false + type: "string" + default: "0" - name: "fromDate" in: "query" description: "If specified, the earliest date (format yyyy-MM-dd) for which\ @@ -443,7 +484,7 @@ paths: - name: "toDate" in: "query" description: "If specified, the latest date (format yyyy-MM-dd) for which\ - \ usage logs are retrieved." + \ trip requests are retrieved." required: false type: "string" default: "The current date" @@ -519,15 +560,27 @@ paths: get: tags: - "api/secure/user" - description: "Gets a list of all 'OtpUser' entities." + description: "Gets a paginated list of all 'OtpUser' entities." produces: - "application/json" - parameters: [] + parameters: + - name: "limit" + in: "query" + description: "If specified, the maximum number of items to return." + required: false + type: "string" + default: "10" + - name: "offset" + in: "query" + description: "If specified, the number of records to skip/offset." + required: false + type: "string" + default: "0" responses: "200": description: "successful operation" schema: - $ref: "#/definitions/OtpUser[]" + $ref: "#/definitions/ResponseList" post: tags: - "api/secure/user" @@ -647,10 +700,22 @@ paths: get: tags: - "api/admin/bugsnag/eventsummary" - description: "Gets a list of all Bugsnag event summaries." + description: "Gets a paginated list of the latest Bugsnag event summaries." produces: - "application/json" - parameters: [] + parameters: + - name: "limit" + in: "query" + description: "If specified, the maximum number of items to return." + required: false + type: "string" + default: "10" + - name: "offset" + in: "query" + description: "If specified, the number of records to skip/offset." + required: false + type: "string" + default: "0" responses: "200": description: "successful operation" @@ -684,8 +749,27 @@ definitions: format: "date" id: type: "string" - AdminUser[]: + ResponseList: type: "object" + properties: + clazz: + type: "string" + data: + type: "array" + items: + type: "string" + offset: + type: "integer" + format: "int32" + limit: + type: "integer" + format: "int32" + total: + type: "integer" + format: "int64" + timestamp: + type: "string" + format: "date" ApiKey: type: "object" properties: @@ -714,10 +798,6 @@ definitions: type: "boolean" name: type: "string" - ApiUser[]: - type: "object" - MonitoredTrip[]: - type: "object" EncodedPolyline: type: "object" properties: @@ -1116,8 +1196,6 @@ definitions: type: "boolean" applicationId: type: "string" - OtpUser[]: - type: "object" GetUsageResult: type: "object" properties: diff --git a/src/test/java/org/opentripplanner/middleware/ApiUserFlowTest.java b/src/test/java/org/opentripplanner/middleware/ApiUserFlowTest.java index f858875fd..4773646d2 100644 --- a/src/test/java/org/opentripplanner/middleware/ApiUserFlowTest.java +++ b/src/test/java/org/opentripplanner/middleware/ApiUserFlowTest.java @@ -2,10 +2,14 @@ import com.auth0.exception.Auth0Exception; import com.auth0.json.mgmt.users.User; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; import org.eclipse.jetty.http.HttpStatus; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.opentripplanner.middleware.controllers.response.ResponseList; import org.opentripplanner.middleware.models.AdminUser; import org.opentripplanner.middleware.models.ApiUser; import org.opentripplanner.middleware.models.MonitoredTrip; @@ -21,7 +25,6 @@ import java.io.IOException; import java.net.http.HttpResponse; -import java.util.List; import java.util.UUID; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -123,7 +126,7 @@ public static void tearDown() { * records. This also includes Auth0 users if auth is enabled. */ @Test - public void canSimulateApiUserFlow() { + public void canSimulateApiUserFlow() throws JsonProcessingException { // create otp user as api user HttpResponse createUserResponse = mockAuthenticatedPost("api/secure/user", apiUser, @@ -157,7 +160,10 @@ public void canSimulateApiUserFlow() { HttpUtils.REQUEST_METHOD.GET ); assertEquals(HttpStatus.OK_200, tripRequestResponse.statusCode()); - List tripRequests = JsonUtils.getPOJOFromJSONAsList(tripRequestResponse.body(), TripRequest.class); + // Special handling of deserialization for ResponseList to avoid class cast issues encountered with JsonUtils + // methods. + ObjectMapper mapper = new ObjectMapper(); + ResponseList tripRequests = mapper.readValue(tripRequestResponse.body(), new TypeReference<>() {}); // Delete otp user. HttpResponse deleteUserResponse = mockAuthenticatedRequest( @@ -176,7 +182,7 @@ public void canSimulateApiUserFlow() { assertNull(deletedTrip); // Verify trip request no longer exists. - TripRequest tripRequest = Persistence.tripRequests.getById(tripRequests.get(0).id); + TripRequest tripRequest = Persistence.tripRequests.getById(tripRequests.data.get(0).id); assertNull(tripRequest); // Delete API user (this would happen through the OTP Admin portal). diff --git a/src/test/java/org/opentripplanner/middleware/persistence/TripHistoryPersistenceTest.java b/src/test/java/org/opentripplanner/middleware/persistence/TripHistoryPersistenceTest.java index 0e6eef197..d8dbf6c04 100644 --- a/src/test/java/org/opentripplanner/middleware/persistence/TripHistoryPersistenceTest.java +++ b/src/test/java/org/opentripplanner/middleware/persistence/TripHistoryPersistenceTest.java @@ -6,6 +6,7 @@ import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.opentripplanner.middleware.OtpMiddlewareTest; +import org.opentripplanner.middleware.controllers.response.ResponseList; import org.opentripplanner.middleware.models.OtpUser; import org.opentripplanner.middleware.models.TripRequest; import org.opentripplanner.middleware.models.TripSummary; @@ -21,6 +22,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; import static org.opentripplanner.middleware.persistence.PersistenceUtil.*; +import static org.opentripplanner.middleware.persistence.TypedPersistence.filterByUserAndDateRange; /** * Tests to verify that trip request and trip summary persistence in MongoDB collections are functioning properly. A @@ -90,20 +92,19 @@ public void canDeleteTripSummary() { @Test public void canGetFilteredTripRequestsWithFromAndToDate() { + OtpUser user = createUser(TEST_EMAIL); + List tripRequests = createTripRequests(LIMIT, user.id); LocalDateTime fromStartOfDay = DateTimeUtils.nowAsLocalDate().atTime(LocalTime.MIN); LocalDateTime toEndOfDay = DateTimeUtils.nowAsLocalDate().atTime(LocalTime.MAX); - Bson filter = Filters.and( - gte(TRIP_REQUEST_DATE_CREATED_FIELD_NAME, - Date.from(fromStartOfDay - .atZone(DateTimeUtils.getSystemZoneId()) - .toInstant())), - lte(TRIP_REQUEST_DATE_CREATED_FIELD_NAME, - Date.from(toEndOfDay - .atZone(DateTimeUtils.getSystemZoneId()) - .toInstant())), - eq(TRIP_REQUEST_USER_ID_FIELD_NAME, otpUser.id)); - List result = Persistence.tripRequests.getFilteredWithLimit(filter, LIMIT); - assertEquals(result.size(), tripRequests.size()); + Date fromDate = Date.from(fromStartOfDay + .atZone(DateTimeUtils.getSystemZoneId()) + .toInstant()); + Date toDate = Date.from(toEndOfDay + .atZone(DateTimeUtils.getSystemZoneId()) + .toInstant()); + Bson filter = filterByUserAndDateRange(user.id, fromDate, toDate); + ResponseList result = Persistence.tripRequests.getResponseList(filter, 0, LIMIT); + assertEquals(result.data.size(), tripRequests.size()); } @Test