Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle the zero limit case explicitly for MongoDB aggregation #187

Merged
merged 1 commit into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2053,6 +2053,17 @@ public void testAggregateWithSingleKey(final String datastoreName) throws IOExce
}
}

@ParameterizedTest
@ArgumentsSource(AllProvider.class)
public void testAggregateWithZeroLimitAndOffset(final String datastoreName) throws IOException {
final Collection collection = getCollection(datastoreName);

final Iterator<Document> resultDocs =
collection.aggregate(
Query.builder().setPagination(Pagination.builder().limit(0).offset(0).build()).build());
assertDocsAndSizeEqual(datastoreName, resultDocs, "query/empty_response.json", 0);
}

@Nested
class AtomicUpdateTest {
@ParameterizedTest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,16 @@ public class MongoPaginationHelper {
private static final String LIMIT_CLAUSE = "$limit";

static BasicDBObject getSkipClause(final Query query) {
Optional<Pagination> paginationOptional = query.getPagination();
Optional<Pagination> paginationOptional =
query.getPagination().filter(pagination -> pagination.getOffset() > 0);
return paginationOptional
.map(pagination -> new BasicDBObject(SKIP_CLAUSE, pagination.getOffset()))
.orElse(new BasicDBObject());
}

static BasicDBObject getLimitClause(final Query query) {
Optional<Pagination> paginationOptional = query.getPagination();
Optional<Pagination> paginationOptional =
query.getPagination().filter(pagination -> pagination.getLimit() > 0);
return paginationOptional
.map(pagination -> new BasicDBObject(LIMIT_CLAUSE, pagination.getLimit()))
.orElse(new BasicDBObject());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,14 @@

import com.mongodb.BasicDBObject;
import com.mongodb.MongoCommandException;
import com.mongodb.ServerAddress;
import com.mongodb.ServerCursor;
import com.mongodb.client.AggregateIterable;
import com.mongodb.client.FindIterable;
import com.mongodb.client.MongoCursor;
import java.util.Collection;
import java.util.List;
import java.util.NoSuchElementException;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand All @@ -49,6 +52,47 @@
query -> singleton(getSkipClause(query)),
query -> singleton(getLimitClause(query)));

private static final Integer ZERO = Integer.valueOf(0);
private static final MongoCursor<BasicDBObject> EMPTY_CURSOR =
new MongoCursor<>() {
@Override
public void close() {
// Do nothing
}

@Override
public boolean hasNext() {
return false;
}

@Override
public BasicDBObject next() {
throw new NoSuchElementException();

Check warning on line 70 in document-store/src/main/java/org/hypertrace/core/documentstore/mongo/query/MongoQueryExecutor.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/mongo/query/MongoQueryExecutor.java#L70

Added line #L70 was not covered by tests
}

@Override
public int available() {
return 0;

Check warning on line 75 in document-store/src/main/java/org/hypertrace/core/documentstore/mongo/query/MongoQueryExecutor.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/mongo/query/MongoQueryExecutor.java#L75

Added line #L75 was not covered by tests
}

@Override
public BasicDBObject tryNext() {
throw new NoSuchElementException();

Check warning on line 80 in document-store/src/main/java/org/hypertrace/core/documentstore/mongo/query/MongoQueryExecutor.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/mongo/query/MongoQueryExecutor.java#L80

Added line #L80 was not covered by tests
}

@Override
public ServerCursor getServerCursor() {
// It is okay to throw exception since we are never invoking this method
throw new UnsupportedOperationException();

Check warning on line 86 in document-store/src/main/java/org/hypertrace/core/documentstore/mongo/query/MongoQueryExecutor.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/mongo/query/MongoQueryExecutor.java#L86

Added line #L86 was not covered by tests
}

@Override
public ServerAddress getServerAddress() {
// It is okay to throw exception since we are never invoking this method
throw new UnsupportedOperationException();

Check warning on line 92 in document-store/src/main/java/org/hypertrace/core/documentstore/mongo/query/MongoQueryExecutor.java

View check run for this annotation

Codecov / codecov/patch

document-store/src/main/java/org/hypertrace/core/documentstore/mongo/query/MongoQueryExecutor.java#L92

Added line #L92 was not covered by tests
}
};

private final com.mongodb.client.MongoCollection<BasicDBObject> collection;

public MongoCursor<BasicDBObject> find(final Query query) {
Expand All @@ -70,6 +114,11 @@
}

public MongoCursor<BasicDBObject> aggregate(final Query originalQuery) {
if (originalQuery.getPagination().map(Pagination::getLimit).map(ZERO::equals).orElse(false)) {
log.debug("Not executing query because of a 0 limit");
return EMPTY_CURSOR;
}

Query query = transformAndLog(originalQuery);

List<BasicDBObject> pipeline =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ public static class PaginationBuilder {
public Pagination build() {
Preconditions.checkArgument(limit != null, "limit is null");
Preconditions.checkArgument(offset != null, "offset is null");

Preconditions.checkArgument(limit >= 0, "limit must be non-negative");
Preconditions.checkArgument(offset >= 0, "offset must be non-negative");

return new Pagination(limit, offset);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@
}
}
},
{
"$skip": 0
},
{
"$limit": 10
}
Expand Down