Intersection API MongoDB Query Limits

[jacob6838]

PR Details

Description

Adding and handling Intersection API MongoDB query limits. Limits are enforced on all queries except for configuration parameters. All endpoints which enforce limits on queries may now return a 206 status code, indicating that the query response may have been limited.

How Has This Been Tested?

This was tested by dropping the CM_MAXIMUM_RESPONSE_SIZE to a low number, and executing queries which exceeded this limit, confirming that data counts were limited and that 206 response codes were returned

Types of changes

• Defect fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that cause existing functionality to change)

Checklist:

• My changes require new environment variables:
  • I have updated the docker-compose, K8s YAML, and all dependent deployment configuration files.
• My changes require updates to the documentation:
  • I have updated the documentation accordingly.
• My changes require updates and/or additions to the unit tests:
  • I have modified/added tests to cover my changes.
• All existing tests pass.

[mcook42]

A general suggestion as I'm going through the review process (it's looking good so far!)

Suggestion(non-blocking): It seems like what we're trying to do with these Repositories and Controllers is to support the following (non-exhaustive):

1. Filtering the data retrieved using user-defined filters (like "full count" or "start/end date")
2. Limiting the size of data returned by our API
3. Indicating to the API consumer that there is more data available

Seeing that this is a pretty common scenario for CRUD apps (like most of this code is) we may be able to leverage one of two Spring Boot starters to provide out-of-the-box Pagination:

• Spring Data Rest.
• Spring JPA Query (which we're already using, it turns out, just the mongo-specific implementation)

By leveraging either of these options, we don't have to roll our own Pagination solution (the usual way of implementing 2 and 3), and it's more transparent to the API consumer how much data is left to query and how to retrieve the rest of the data.

In Spring Data, a Pageable is an interface that represents a request to retrieve data in a paginated manner. When querying data from MongoDB collection, a Pageable object allows us to specify parameters such as page number, page size, and sorting criteria. This is particularly useful for displaying large datasets on applications where showing all items at once would be slow.
ref: https://www.baeldung.com/spring-data-mongorepository-limit-skip

This API is built with Spring Boot & the only consumer of this API (as far as I know) is the UI maintained in this repo. With that knowledge, it shouldn't be much of a lift to use the provided starter. There is a section in the docs talking about how to add Spring Data REST to an existing API, so that could help shortcut the efforts and reduce risk.

If Pagination isn't needed, leveraging the Spring Data framework more effectively with @Query annotations on the interface methods may be the better approach

Baeldung's GH repo contains a module that shows how to do this with the mongo spring data impl.

[mcook42]

suggestion(non-blocking): Since we are only ever using getMaximumResponseSize() from the ConflictApiMonitorProperties object, we could instead pass in @Value("whatever the property path is") boolean maxResponseSize to all of the constructors. This will decouple the repositories from the large ConflictApiMonitorProperties object, and make the code a little clearer.

[mcook42]

issue (blocking): It's my understanding that a 206 Partial Content HTTP Response code is intended for the following use case:

The HTTP 206 Partial Content successful response status code is sent in response to a range request. The response body contains the requested ranges of data as specified in the Range header of the request.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/206

Since the requests made to the API controllers are not range requests (there aren't headers indicating a Range), I don't believe that the 206 Partial Content response code is appropriate here.

also, see:

206 Partial Content
The server is delivering only part of the resource (byte serving) due to a range header sent by the client. The range header is used by HTTP clients to enable resuming of interrupted downloads, or split a download into multiple simultaneous streams.
https://en.wikipedia.org/wiki/List_of_HTTP_status_codes

[mcook42]

Overall, this is looking great. I have a few blockers, though.

chore(blocking): there are multiple instances of field injection where we should use constructor injection. Please make those updates. An example was included in services/intersection-api/api/src/main/java/us/dot/its/jpo/ode/api/accessors/bsm/OdeBsmJsonRepositoryImpl.java

issue(blocking): my earlier issue is still unresolved

[mcook42]

question(blocking): From what I can see in the code, we call this method in two places:

1. NotificationController.countConnectionOfTravelNotification
2. AssessmentController.countLaneDirectionOfTravelAssessment

Both of those calls have @RequestParam(name = "page", required = false, defaultValue = "0") Integer page , which should provide a default Integer of 0. With these two locations providing a non-null default Integer value, what value does this createNullablePage provide?

[jacob6838]

The createNullablePage runs off of the size being null, which was no defaultValue. I think this is a logical way to set up the defaults - page is 0, the first page, and if the size isn't set, for count queries, it defaults to not restricting the response size. For actual data requests, we default the size to 10k, to stop accidental massive queries

I pushed an update right after this applying the createNullablePage to the rest of the converted count queries.

I don't love sharing this method through an interface, as having the Controllers implement the interface seems a little silly - would a static utility class be reasonable here?

[mcook42]

Thanks for the clarification. I've pulled down the most recent code, so I'm working with the same info as you.

After digging in more, I wonder if we can remove Pageable from the count interfaces for the repositories instead. As far as I can tell, counts are counts of the resources found and don't need to be paged (because they only return a number).

For example, the ConnectionOfTravelAssessmentRepositoryImpl.count method would look like:

/**
     * Get a page representing the count of data for a given intersectionID,
     * startTime, and endTime
     *
     * @param intersectionID the intersection ID to query by, if null will not be
     *                       applied
     * @param startTime      the start time to query by, if null will not be applied
     * @param endTime        the end time to query by, if null will not be applied
     * @return the number of records found
     */
    public long count(
            Integer intersectionID,
            Long startTime,
            Long endTime) {
        Criteria criteria = new IntersectionCriteria()
                .whereOptional(INTERSECTION_ID_FIELD, intersectionID)
                .withinTimeWindow(DATE_FIELD, startTime, endTime);
        Query query = Query.query(criteria);
        return mongoTemplate.count(query, collectionName);
    }

[mcook42]

chore(blocking): use constructor injection

Suggested change

	public OdeBsmJsonRepositoryImpl(MongoTemplate mongoTemplate) {
	public OdeBsmJsonRepositoryImpl(MongoTemplate mongoTemplate, @Value("${maximumResponseSize}") int maximumResponseSize) {
	this.maximumResponseSize = maximumResponseSize;

[mcook42]

chore(blocking): together with the chore below. use constructor injection and keep this property private

Suggested change

	int maximumResponseSize;
	private final int maximumResponseSize;

[mcook42]

praise: nice usage of value injection to keep the constructor params focused and clear (not hidden by using a larger ConflictMonitorApiProperties param)

[mcook42]

question(blocking): is there a way to make this a single value? I'm not super familiar with mongo and aggregate queries, but it seems to me that we may be able to simplify this to Long count instead of a list of counts from which we only use the first element

jpo-cvmanager/services/intersection-api/api/src/main/java/us/dot/its/jpo/ode/api/accessors/PageableQuery.java

Lines 63 to 64 in 182649e

	List<T> data = result.getResults();
	long totalElements = result.getCounts().getFirst();