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

Provide a better error for client-side shard filtering. #23

Merged
merged 3 commits into from
Mar 21, 2015
Merged

Provide a better error for client-side shard filtering. #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c4acecd
Increase default number of bases per shard.
deflaux Mar 13, 2015
a5f96c2
Provide a better error for client-side shard filtering.
deflaux Mar 13, 2015
00c633e
Refactored field enforcement.
deflaux Mar 21, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/google/cloud/genomics/utils/Contig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public class Contig implements Serializable {

private static final long serialVersionUID = -1730387112193404207L;

public static final long DEFAULT_NUMBER_OF_BASES_PER_SHARD = 100000;
public static final long DEFAULT_NUMBER_OF_BASES_PER_SHARD = 1000000;

public enum SexChromosomeFilter { INCLUDE_XY, EXCLUDE_XY }

Expand Down
106 changes: 92 additions & 14 deletions src/main/java/com/google/cloud/genomics/utils/Paginator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@
*/
package com.google.cloud.genomics.utils;

import java.io.IOException;
import java.io.Serializable;
import java.util.Collections;
import java.util.Iterator;
import java.util.Map;

import com.google.api.services.genomics.Genomics;
import com.google.api.services.genomics.GenomicsRequest;
import com.google.api.services.genomics.model.Annotation;
Expand Down Expand Up @@ -51,17 +57,16 @@
import com.google.api.services.genomics.model.Variant;
import com.google.api.services.genomics.model.VariantSet;
import com.google.common.base.Function;
import com.google.common.base.Joiner;
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;

import java.io.IOException;
import java.io.Serializable;
import java.util.Collections;
import java.util.Iterator;
import com.google.common.collect.Maps;

/**
* An abstraction that understands the {@code pageToken} / {@code nextPageToken} protocol for paging
Expand Down Expand Up @@ -319,6 +324,13 @@ public static class Reads extends Paginator<
SearchReadsResponse,
Read> {

// TODO: When this is supported server-side, these additional fields are no longer required
private static ImmutableMap<String,String> REQUIRED_STRICT_SHARD_FIELDS =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we assert nextPageToken somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added that too.

ImmutableMap.<String, String>builder()
.putAll(REQUIRED_FIELDS)
.put("alignment", ".*\\p{Punct}alignment\\p{Punct}.*")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alignments, right?

Also, is there some guarantee of leading/trailing punctuation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind about 'alignments', but the second comment stands.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind the whole thread. These will always be wrapped in the repeated response field, i.e. alignments, annotations, variants

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just requires time - it took me some time to wrap my brain around it too :)

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

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

@Override
public GenomicsRequestInitializer<GenomicsRequest<?>> setFieldsInitializer(final String fields) {
if(shardBoundary == ShardBoundary.STRICT) {
return new GenomicsSearchFieldRequestInitializer(fields, REQUIRED_STRICT_SHARD_FIELDS);
}
return new GenomicsSearchFieldRequestInitializer(fields, REQUIRED_FIELDS);
}

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

// TODO: When this is supported server-side, these additional fields are no longer required
private static ImmutableMap<String,String> REQUIRED_STRICT_SHARD_FIELDS =
ImmutableMap.<String, String>builder()
.putAll(REQUIRED_FIELDS)
.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 +472,14 @@ public boolean apply(Annotation anno) {
.or(request));
}

@Override
public GenomicsRequestInitializer<GenomicsRequest<?>> setFieldsInitializer(final String fields) {
if(shardBoundary == ShardBoundary.STRICT) {
return new GenomicsSearchFieldRequestInitializer(fields, REQUIRED_STRICT_SHARD_FIELDS);
}
return new GenomicsSearchFieldRequestInitializer(fields, REQUIRED_FIELDS);
}

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

// TODO: When this is supported server-side, these additional fields are no longer required
private static ImmutableMap<String,String> REQUIRED_STRICT_SHARD_FIELDS =
ImmutableMap.<String, String>builder()
.putAll(REQUIRED_FIELDS)
.put("start", ".*\\p{Punct}start\\p{Punct}.*")
.build();
private final ShardBoundary shardBoundary;
private Predicate<Variant> shardPredicate = null;

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

@Override
public GenomicsRequestInitializer<GenomicsRequest<?>> setFieldsInitializer(final String fields) {
if(shardBoundary == ShardBoundary.STRICT) {
return new GenomicsSearchFieldRequestInitializer(fields, REQUIRED_STRICT_SHARD_FIELDS);
}
return new GenomicsSearchFieldRequestInitializer(fields, REQUIRED_FIELDS);
}

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

public static GenomicsRequestInitializer<GenomicsRequest<?>> setFieldsInitializer(
final String fields) {
return new GenomicsRequestInitializer<GenomicsRequest<?>>() {
@Override public void initialize(GenomicsRequest<?> search) {
if (null != fields) {
search.setFields(fields);
}
}
};
protected static final ImmutableMap<String, String> REQUIRED_FIELDS = ImmutableMap
.<String, String>builder()
.put("nextPageToken", "(.+\\p{Punct})?nextPageToken(\\p{Punct}.+)?").
build();

public GenomicsRequestInitializer<GenomicsRequest<?>> setFieldsInitializer(final String fields) {
return new GenomicsSearchFieldRequestInitializer(fields, REQUIRED_FIELDS);
}

public static class GenomicsSearchFieldRequestInitializer implements
GenomicsRequestInitializer<GenomicsRequest<?>> {
private final String fields;
private final ImmutableMap<String, String> requiredFields;

public GenomicsSearchFieldRequestInitializer(String fields,
ImmutableMap<String, String> requiredFields) {
this.fields = fields;
this.requiredFields = requiredFields;
}

@Override
public void initialize(GenomicsRequest<?> search) {
if (null != fields) {
Map<String, String> missingFields =
Maps.filterValues(requiredFields, new Predicate<String>() {
@Override
public boolean apply(String input) {
return !fields.matches(input);
}
});
Preconditions
.checkArgument(
missingFields.isEmpty(),
"Required field(s) missing: '%s' Add this field to the list of fields to be returned in the partial response.",
Joiner.on(",").join(missingFields.keySet()));
search.setFields(fields);
}
}
}

private final Genomics genomics;

public Paginator(Genomics genomics) {
Expand Down
74 changes: 69 additions & 5 deletions src/test/java/com/google/cloud/genomics/utils/PaginatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@

import org.hamcrest.CoreMatchers;
import org.junit.Before;
import org.junit.Ignore;
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 +58,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 @@ -120,16 +126,26 @@ public void testFields() throws Exception {
Paginator.ReadGroupSets paginator = Paginator.ReadGroupSets.create(genomics);
List<String> ids = Lists.newArrayList();
for (ReadGroupSet set : paginator.search(
new SearchReadGroupSetsRequest().setName("HG"), "readGroupSets(id,name)")) {
new SearchReadGroupSetsRequest().setName("HG"), "nextPageToken,readGroupSets(id,name)")) {
ids.add(set.getId());
}

assertEquals(Lists.newArrayList("r1"), ids);

// Make sure the fields parameter actually gets passed along
Mockito.verify(readGroupSetSearch, Mockito.atLeastOnce()).setFields("readGroupSets(id,name)");
Mockito.verify(readGroupSetSearch, Mockito.atLeastOnce()).setFields("nextPageToken,readGroupSets(id,name)");
}

@Test
public void testFieldsMissingNextPageToken() throws Exception {
Mockito.when(readGroupSets.search(new SearchReadGroupSetsRequest().setName("HG")))
.thenReturn(readGroupSetSearch);

Paginator.ReadGroupSets paginator = Paginator.ReadGroupSets.create(genomics);
thrown.expect(IllegalArgumentException.class);
paginator.search(new SearchReadGroupSetsRequest().setName("HG"), "readGroupSets(id,name)").iterator().next();
}

@Test
public void testVariantPagination() throws Exception {

Expand Down Expand Up @@ -157,15 +173,33 @@ 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());
assertNotNull(filteredPaginator.search(request, "id,nextPageToken,variants(start,id,calls(genotype,callSetName))").iterator().next());
assertNotNull(filteredPaginator.search(request, "variants(start,id,calls(genotype,callSetName)),nextPageToken").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));

// Ensure searches with fields verify the preconditions 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());
assertNotNull(overlappingPaginator.search(request, "id,nextPageToken,variants(start,id,calls(genotype,callSetName))").iterator().next());
assertNotNull(overlappingPaginator.search(request, "variants(id,calls(genotype,callSetName)),nextPageToken").iterator().next());
}

@Test
public void testVariantPaginationEmptyShard() throws Exception {

Expand All @@ -178,8 +212,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 +255,34 @@ 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();
}

@Test
public void testStrictReadPaginationNextPageTokenPrecondition() 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, "reads(id,alignment(cigar,position))").iterator().next();
}

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

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

}