Skip to content
This repository has been archived by the owner on Sep 15, 2021. It is now read-only.

Commit

Permalink
Provide a better error for client-side shard filtering.
Browse files Browse the repository at this point in the history 
Now that we are doing client-side filtering for strict shard boundaries, we need to check that we are requesting the field that the filter will require and return a useful error if not.
  • Loading branch information
@deflaux
deflaux committed Mar 13, 2015
1 parent c4acecd commit a5f96c2
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 5 deletions.
77 changes: 75 additions & 2 deletions src/main/java/com/google/cloud/genomics/utils/Paginator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,11 @@
import com.google.api.services.genomics.model.VariantSet;
import com.google.common.base.Function;
import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import com.google.common.base.Predicate;
import com.google.common.collect.AbstractSequentialIterator;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;

Expand Down Expand Up @@ -319,6 +321,11 @@ public static class Reads extends Paginator<
SearchReadsResponse,
Read> {

private static ImmutableMap<String,String> REQUIRED_STRICT_SHARD_FIELDS =
ImmutableMap.<String, String>builder()
.put("alignment", ".*\\p{Punct}alignment\\p{Punct}.*")
.put("position", ".*\\p{Punct}position\\p{Punct}.*")
.build();
private final ShardBoundary shardBoundary;
private Predicate<Read> shardPredicate = null;

Expand Down Expand Up @@ -369,6 +376,26 @@ public boolean apply(Read read) {
.or(request));
}

@Override
public GenomicsRequestInitializer<GenomicsRequest<?>> setFieldsInitializer(final String fields) {
return new GenomicsRequestInitializer<GenomicsRequest<?>>() {
@Override
public void initialize(GenomicsRequest<?> search) {
if (null != fields) {
for (String requiredField : REQUIRED_STRICT_SHARD_FIELDS.keySet()) {
Preconditions
.checkArgument(
shardBoundary != ShardBoundary.STRICT
|| fields.matches(REQUIRED_STRICT_SHARD_FIELDS.get(requiredField)),
"Required field missing: '%s' Add this field to the list of Read fields to be returned in the partial response.",
requiredField);
}
search.setFields(fields);
}
}
};
}

@Override Genomics.Reads getApi(Genomics genomics) {
return genomics.reads();
}
Expand Down Expand Up @@ -396,6 +423,11 @@ public static class Annotations extends Paginator<
SearchAnnotationsResponse,
Annotation> {

private static ImmutableMap<String,String> REQUIRED_STRICT_SHARD_FIELDS =
ImmutableMap.<String, String>builder()
.put("position", ".*\\p{Punct}position\\p{Punct}.*")
.put("start", ".*\\p{Punct}start\\p{Punct}.*")
.build();
private final ShardBoundary shardBoundary;
private Predicate<Annotation> shardPredicate = null;

Expand Down Expand Up @@ -445,6 +477,26 @@ public boolean apply(Annotation anno) {
.or(request));
}

@Override
public GenomicsRequestInitializer<GenomicsRequest<?>> setFieldsInitializer(final String fields) {
return new GenomicsRequestInitializer<GenomicsRequest<?>>() {
@Override
public void initialize(GenomicsRequest<?> search) {
if (null != fields) {
for(String requiredField : REQUIRED_STRICT_SHARD_FIELDS.keySet()) {
Preconditions
.checkArgument(
shardBoundary != ShardBoundary.STRICT
|| fields.matches(REQUIRED_STRICT_SHARD_FIELDS.get(requiredField)),
"Required field missing: '%s' Add this field to the list of Annotation fields to be returned in the partial response.",
requiredField);
}
search.setFields(fields);
}
}
};
}

@Override Genomics.Annotations getApi(Genomics genomics) {
return genomics.annotations();
}
Expand Down Expand Up @@ -784,6 +836,9 @@ public static class Variants extends Paginator<
SearchVariantsResponse,
Variant> {

private static final String REQUIRED_STRICT_SHARD_FIELD = "start";
private static final String REQUIRED_STRICT_SHARD_PATTERN =
".*\\p{Punct}" + REQUIRED_STRICT_SHARD_FIELD +"\\p{Punct}.*";
private final ShardBoundary shardBoundary;
private Predicate<Variant> shardPredicate = null;

Expand Down Expand Up @@ -834,6 +889,24 @@ public boolean apply(Variant variant) {
.or(request));
}

@Override
public GenomicsRequestInitializer<GenomicsRequest<?>> setFieldsInitializer(final String fields) {
return new GenomicsRequestInitializer<GenomicsRequest<?>>() {
@Override
public void initialize(GenomicsRequest<?> search) {
if (null != fields) {
Preconditions
.checkArgument(
shardBoundary != ShardBoundary.STRICT
|| fields.matches(REQUIRED_STRICT_SHARD_PATTERN),
"Required field missing: '%s' Add this field to the list of Variant fields to be returned in the partial response.",
REQUIRED_STRICT_SHARD_FIELD);
search.setFields(fields);
}
}
};
}

@Override Genomics.Variants getApi(Genomics genomics) {
return genomics.variants();
}
Expand Down Expand Up @@ -905,7 +978,7 @@ private Variantsets(Genomics genomics) {
@Override public void initialize(GenomicsRequest<?> search) {}
};

public static GenomicsRequestInitializer<GenomicsRequest<?>> setFieldsInitializer(
public GenomicsRequestInitializer<GenomicsRequest<?>> setFieldsInitializer(
final String fields) {
return new GenomicsRequestInitializer<GenomicsRequest<?>>() {
@Override public void initialize(GenomicsRequest<?> search) {
Expand All @@ -915,7 +988,7 @@ public static GenomicsRequestInitializer<GenomicsRequest<?>> setFieldsInitialize
}
};
}

private final Genomics genomics;

public Paginator(Genomics genomics) {
Expand Down
34 changes: 31 additions & 3 deletions src/test/java/com/google/cloud/genomics/utils/PaginatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@

import org.hamcrest.CoreMatchers;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
import org.mockito.Mock;
Expand All @@ -55,6 +57,9 @@ public class PaginatorTest {
@Mock Genomics.Reads reads;
@Mock Genomics.Reads.Search readsSearch;

@Rule
public ExpectedException thrown = ExpectedException.none();

@Before
public void initMocks() {
MockitoAnnotations.initMocks(this);
Expand Down Expand Up @@ -157,15 +162,29 @@ public void testVariantPagination() throws Exception {
assertThat(filteredVariants, CoreMatchers.hasItems(atStartWithinExtent,
atStartOverlapExtent, beyondStartWithinExtent, beyondOverlapExtent));

// Ensure searches with fields verify the preconditions for strict shards.
final String nullFields = null;
assertNotNull(filteredPaginator.search(request, nullFields).iterator().next());
assertNotNull(filteredPaginator.search(request, "nextPageToken,variants(start,id,calls(genotype,callSetName))").iterator().next());
try {
filteredPaginator.search(request, "nextPageToken,variants(id,calls(genotype,callSetName))").iterator().next();
fail("should have thrown an IllegalArgumentxception");
} catch (IllegalArgumentException e) {}

Paginator.Variants overlappingPaginator = Paginator.Variants.create(genomics, ShardBoundary.OVERLAPS);
List<Variant> overlappingVariants = Lists.newArrayList();
for (Variant variant : overlappingPaginator.search(request)) {
overlappingVariants.add(variant);
}
assertEquals(6, overlappingVariants.size());
assertThat(overlappingVariants, CoreMatchers.hasItems(input));

// There are no preconditions on fields for overlapping shards.
assertNotNull(overlappingPaginator.search(request, nullFields).iterator().next());
assertNotNull(overlappingPaginator.search(request, "nextPageToken,variants(start,id,calls(genotype,callSetName))").iterator().next());
assertNotNull(overlappingPaginator.search(request, "nextPageToken,variants(id,calls(genotype,callSetName))").iterator().next());
}

@Test
public void testVariantPaginationEmptyShard() throws Exception {

Expand All @@ -178,8 +197,8 @@ public void testVariantPaginationEmptyShard() throws Exception {
Paginator.Variants filteredPaginator = Paginator.Variants.create(genomics, ShardBoundary.STRICT);
assertNotNull(filteredPaginator.search(request));
}

Read readHelper(int start, int end) {
static Read readHelper(int start, int end) {
Position position = new Position().setPosition((long) start);
LinearAlignment alignment = new LinearAlignment().setPosition(position);
return new Read().setAlignment(alignment).setFragmentLength(end-start);
Expand Down Expand Up @@ -221,4 +240,13 @@ public void testReadPagination() throws Exception {
assertThat(overlappingReads, CoreMatchers.hasItems(input));
}

@Test
public void testReadPaginationStrictShardPrecondition() throws Exception {
SearchReadsRequest request = new SearchReadsRequest().setStart(1000L).setEnd(2000L);
Mockito.when(reads.search(request)).thenReturn(readsSearch);

Paginator.Reads filteredPaginator = Paginator.Reads.create(genomics, ShardBoundary.STRICT);
thrown.expect(IllegalArgumentException.class);
filteredPaginator.search(request, "nextPageToken,reads(id,alignment(cigar))").iterator().next();
}
}

0 comments on commit a5f96c2

Please sign in to comment.