From a5f96c263f2e09d7f7ac31f12f24a1c39cf05038 Mon Sep 17 00:00:00 2001 From: Nicole Deflaux Date: Thu, 12 Mar 2015 20:39:31 -0700 Subject: [PATCH] Provide a better error for client-side shard filtering. 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. --- .../cloud/genomics/utils/Paginator.java | 77 ++++++++++++++++++- .../cloud/genomics/utils/PaginatorTest.java | 34 +++++++- 2 files changed, 106 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/cloud/genomics/utils/Paginator.java b/src/main/java/com/google/cloud/genomics/utils/Paginator.java index 20b6d07..ebd6e00 100644 --- a/src/main/java/com/google/cloud/genomics/utils/Paginator.java +++ b/src/main/java/com/google/cloud/genomics/utils/Paginator.java @@ -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; @@ -319,6 +321,11 @@ public static class Reads extends Paginator< SearchReadsResponse, Read> { + private static ImmutableMap REQUIRED_STRICT_SHARD_FIELDS = + ImmutableMap.builder() + .put("alignment", ".*\\p{Punct}alignment\\p{Punct}.*") + .put("position", ".*\\p{Punct}position\\p{Punct}.*") + .build(); private final ShardBoundary shardBoundary; private Predicate shardPredicate = null; @@ -369,6 +376,26 @@ public boolean apply(Read read) { .or(request)); } + @Override + public GenomicsRequestInitializer> setFieldsInitializer(final String fields) { + return new GenomicsRequestInitializer>() { + @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(); } @@ -396,6 +423,11 @@ public static class Annotations extends Paginator< SearchAnnotationsResponse, Annotation> { + private static ImmutableMap REQUIRED_STRICT_SHARD_FIELDS = + ImmutableMap.builder() + .put("position", ".*\\p{Punct}position\\p{Punct}.*") + .put("start", ".*\\p{Punct}start\\p{Punct}.*") + .build(); private final ShardBoundary shardBoundary; private Predicate shardPredicate = null; @@ -445,6 +477,26 @@ public boolean apply(Annotation anno) { .or(request)); } + @Override + public GenomicsRequestInitializer> setFieldsInitializer(final String fields) { + return new GenomicsRequestInitializer>() { + @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(); } @@ -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 shardPredicate = null; @@ -834,6 +889,24 @@ public boolean apply(Variant variant) { .or(request)); } + @Override + public GenomicsRequestInitializer> setFieldsInitializer(final String fields) { + return new GenomicsRequestInitializer>() { + @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(); } @@ -905,7 +978,7 @@ private Variantsets(Genomics genomics) { @Override public void initialize(GenomicsRequest search) {} }; - public static GenomicsRequestInitializer> setFieldsInitializer( + public GenomicsRequestInitializer> setFieldsInitializer( final String fields) { return new GenomicsRequestInitializer>() { @Override public void initialize(GenomicsRequest search) { @@ -915,7 +988,7 @@ public static GenomicsRequestInitializer> setFieldsInitialize } }; } - + private final Genomics genomics; public Paginator(Genomics genomics) { diff --git a/src/test/java/com/google/cloud/genomics/utils/PaginatorTest.java b/src/test/java/com/google/cloud/genomics/utils/PaginatorTest.java index 51b799e..62e5327 100644 --- a/src/test/java/com/google/cloud/genomics/utils/PaginatorTest.java +++ b/src/test/java/com/google/cloud/genomics/utils/PaginatorTest.java @@ -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; @@ -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); @@ -157,6 +162,15 @@ 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 overlappingVariants = Lists.newArrayList(); for (Variant variant : overlappingPaginator.search(request)) { @@ -164,8 +178,13 @@ public void testVariantPagination() throws Exception { } 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 { @@ -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); @@ -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(); + } }