Merge pull request #578 from ajkannan/fix-cursor

aozarov · aozarov · commit c5bae84363c7 · 2016-01-25T14:00:46.000-08:00
Remove check for valid utf-8 cursor
diff --git a/gcloud-java-datastore/src/main/java/com/google/gcloud/datastore/Cursor.java b/gcloud-java-datastore/src/main/java/com/google/gcloud/datastore/Cursor.java
@@ -17,21 +17,14 @@
 package com.google.gcloud.datastore;
 
 import static com.google.common.base.Preconditions.checkNotNull;
-import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.api.services.datastore.DatastoreV1;
 import com.google.api.services.datastore.DatastoreV1.Value;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.MoreObjects.ToStringHelper;
-import com.google.common.base.Preconditions;
+import com.google.common.io.BaseEncoding;
 import com.google.protobuf.ByteString;
 import com.google.protobuf.InvalidProtocolBufferException;
-import com.google.protobuf.TextFormat;
-import com.google.protobuf.TextFormat.ParseException;
-
-import java.io.UnsupportedEncodingException;
-import java.net.URLDecoder;
-import java.net.URLEncoder;
 
 /**
  * A Google Cloud Datastore cursor.
@@ -44,7 +37,6 @@ public final class Cursor extends Serializable<DatastoreV1.Value> {
   private final transient ByteString byteString;
 
   Cursor(ByteString byteString) {
-    Preconditions.checkArgument(byteString.isValidUtf8(), "content is not a valid UTF-8");
     this.byteString = byteString;
   }
 
@@ -76,23 +68,16 @@ ByteString byteString() {
    * Returns the cursor in an encoded form that can be used as part of a URL.
    */
   public String toUrlSafe() {
-    try {
-      return URLEncoder.encode(TextFormat.printToString(toPb()), UTF_8.name());
-    } catch (UnsupportedEncodingException e) {
-      throw new IllegalStateException("Unexpected encoding exception", e);
-    }
+    return BaseEncoding.base64Url().encode(byteString.toByteArray());
   }
 
   /**
    * Create a {@code Cursor} given its URL safe encoded form.
    */
   public static Cursor fromUrlSafe(String urlSafe) {
     try {
-      String utf8Str = URLDecoder.decode(urlSafe, UTF_8.name());
-      DatastoreV1.Value.Builder builder = DatastoreV1.Value.newBuilder();
-      TextFormat.merge(utf8Str, builder);
-      return fromPb(builder.build());
-    } catch (UnsupportedEncodingException | ParseException e) {
+      return Cursor.copyFrom(BaseEncoding.base64Url().decode(urlSafe));
+    } catch (IllegalArgumentException e) {
       throw new IllegalStateException("Unexpected decoding exception", e);
     }
   }
diff --git a/gcloud-java-datastore/src/main/java/com/google/gcloud/datastore/StructuredQuery.java b/gcloud-java-datastore/src/main/java/com/google/gcloud/datastore/StructuredQuery.java
@@ -782,7 +782,7 @@ public StructuredQuery<V> build() {
     }
   }
 
-  static final class Builder<V> extends BaseBuilder<V, Builder<V>> {
+  public static final class Builder<V> extends BaseBuilder<V, Builder<V>> {
 
     Builder(ResultType<V> resultType) {
       super(resultType);
diff --git a/gcloud-java-datastore/src/test/java/com/google/gcloud/datastore/DatastoreTest.java b/gcloud-java-datastore/src/test/java/com/google/gcloud/datastore/DatastoreTest.java
@@ -39,6 +39,7 @@
 import com.google.gcloud.datastore.testing.LocalGcdHelper;
 import com.google.gcloud.spi.DatastoreRpc;
 import com.google.gcloud.spi.DatastoreRpcFactory;
+import com.google.protobuf.ByteString;
 
 import org.easymock.EasyMock;
 import org.junit.AfterClass;
@@ -496,7 +497,7 @@ public void testQueryPaginationWithLimit() throws DatastoreException {
       }
       query = query.toBuilder().startCursor(results.cursorAfter()).build();
     }
-    assertEquals(totalCount, 5);
+    assertEquals(5, totalCount);
     EasyMock.verify(rpcFactoryMock, rpcMock);
   }
 
@@ -524,7 +525,8 @@ private List<RunQueryResponse> buildResponsesForQueryPaginationWithLimit() {
         .setMoreResults(QueryResultBatch.MoreResultsType.MORE_RESULTS_AFTER_LIMIT)
         .clearEntityResult()
         .addAllEntityResult(queryResultBatchPb.getEntityResultList().subList(1, 2))
-        .setEndCursor(queryResultBatchPb.getEntityResultList().get(1).getCursor())
+        .setEndCursor(
+            ByteString.copyFrom(new byte[] {(byte) 0x80})) // test invalid UTF-8 string
         .build();
     responses.add(RunQueryResponse.newBuilder().setBatch(queryResultBatchPb2).build());
     QueryResultBatch queryResultBatchPb3 = QueryResultBatch.newBuilder()
@@ -546,6 +548,17 @@ private List<RunQueryResponse> buildResponsesForQueryPaginationWithLimit() {
     return responses;
   }
 
+  @Test
+  public void testToUrlSafe() {
+    byte[][] invalidUtf8 =
+        new byte[][] {{(byte) 0xfe}, {(byte) 0xc1, (byte) 0xbf}, {(byte) 0xc0}, {(byte) 0x80}};
+    for (byte[] bytes : invalidUtf8) {
+      assertFalse(ByteString.copyFrom(bytes).isValidUtf8());
+      Cursor cursor = new Cursor(ByteString.copyFrom(bytes));
+      assertEquals(cursor, Cursor.fromUrlSafe(cursor.toUrlSafe()));
+    }
+  }
+
   @Test
   public void testAllocateId() {
     KeyFactory keyFactory = datastore.newKeyFactory().kind(KIND1);

Original file line number	Diff line number	Diff line change
`@@ -782,7 +782,7 @@ public StructuredQuery<V> build() {`
`782`	`782`	`}`
`783`	`783`	`}`
`784`	`784`
`785`		`- static final class Builder<V> extends BaseBuilder<V, Builder<V>> {`
	`785`	`+ public static final class Builder<V> extends BaseBuilder<V, Builder<V>> {`
`786`	`786`
`787`	`787`	`Builder(ResultType<V> resultType) {`
`788`	`788`	`super(resultType);`