http: Fix wrong behavior of {+var} (with var not a list) in UriTemplate.

https://codereview.appspot.com/66770043/

Author: wonderfly

google-http-client-test/src/main/java/com/google/api/client/test/util/store/AbstractDataStoreFactoryTest.java

@@ -22,6 +22,7 @@
 import junit.framework.TestCase;
 
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Set;
 
 /**
@@ -56,6 +57,11 @@ public void tearDown() throws Exception {
     assertTrue(boolTyped.values().isEmpty());
   }
 
+  private static void assertContentsAnyOrder(Collection<?> c, Object... elts) {
+    assertEquals(Sets.newHashSet(c),
+                 Sets.newHashSet(Arrays.asList(elts)));
+  }
+
   public void testId() throws Exception {
     subtestIdNoException("1");
     subtestIdNoException("123456789012345678901234567890");
@@ -127,7 +133,7 @@ public void testValues() throws Exception {
     stringTyped.set("k", "new");
     stringTyped.set("k2", "other");
     // check values
-    assertEquals(Arrays.asList("new", "other"), stringTyped.values());
+    assertContentsAnyOrder(stringTyped.values(), "new", "other");
     // delete one
     stringTyped.delete("k");
     assertNull(stringTyped.get("k"));

google-http-client/src/main/java/com/google/api/client/http/UriTemplate.java

@@ -189,6 +189,10 @@ String getEncodedValue(String value) {
       }
       return encodedValue;
     }
+
+    boolean getReservedExpansion() {
+      return reservedExpansion;
+    }
   }
 
   static CompositeOutput getCompositeOutput(String propertyName) {
@@ -329,7 +333,11 @@ public static String expand(String pathUri, Object parameters,
         value = getMapPropertyValue(varName, map, containsExplodeModifier, compositeOutput);
       } else {
         // For everything else...
-        value = CharEscapers.escapeUriPath(value.toString());
+        if (compositeOutput.getReservedExpansion()) {
+          value = CharEscapers.escapeUriPathWithoutReserved(value.toString());
+        } else {
+          value = CharEscapers.escapeUriPath(value.toString());
+        }
       }
       pathBuf.append(value);
     }

google-http-client/src/main/java/com/google/api/client/testing/http/MockHttpTransport.java

@@ -48,6 +48,14 @@ public class MockHttpTransport extends HttpTransport {
    * */
   private MockLowLevelHttpRequest lowLevelHttpRequest;
 
+  /**
+   * The {@link MockLowLevelHttpResponse} to be returned when this {@link MockHttpTransport}
+   * executes the associated request. Note that this field is ignored if the caller provided
+   * a non-{@code null} {@link MockLowLevelHttpRequest} with this {@link MockHttpTransport}
+   * was built.
+   */
+  private MockLowLevelHttpResponse lowLevelHttpResponse;
+
   public MockHttpTransport() {
   }
 
@@ -59,6 +67,7 @@ public MockHttpTransport() {
   protected MockHttpTransport(Builder builder) {
     supportedMethods = builder.supportedMethods;
     lowLevelHttpRequest = builder.lowLevelHttpRequest;
+    lowLevelHttpResponse = builder.lowLevelHttpResponse;
   }
 
   @Override
@@ -69,9 +78,14 @@ public boolean supportsMethod(String method) throws IOException {
   @Override
   public LowLevelHttpRequest buildRequest(String method, String url) throws IOException {
     Preconditions.checkArgument(supportsMethod(method), "HTTP method %s not supported", method);
-    return lowLevelHttpRequest == null
-        ? new MockLowLevelHttpRequest(url)
-        : lowLevelHttpRequest;
+    if (lowLevelHttpRequest != null) {
+      return lowLevelHttpRequest;
+    }
+    MockLowLevelHttpRequest request = new MockLowLevelHttpRequest(url);
+    if (lowLevelHttpResponse != null) {
+      request.setResponse(lowLevelHttpResponse);
+    }
+    return request;
   }
 
   /**
@@ -95,8 +109,13 @@ public final MockLowLevelHttpRequest getLowLevelHttpRequest() {
   /**
    * Returns an instance of a new builder.
    *
+   * <p>
+   * @deprecated (to be removed in the future) Use {@link Builder#Builder()} instead.
+   * </p>
+   *
    * @since 1.5
    */
+  @Deprecated
   public static Builder builder() {
     return new Builder();
   }
@@ -124,7 +143,20 @@ public static class Builder {
      * */
     MockLowLevelHttpRequest lowLevelHttpRequest;
 
-    protected Builder() {
+    /**
+     * The {@link MockLowLevelHttpResponse} that should be the result of the
+     * {@link MockLowLevelHttpRequest} to be returned by {@link #buildRequest}. Note
+     * that this field is ignored if the caller provides a {@link MockLowLevelHttpRequest}
+     * via {@link #setLowLevelHttpRequest}.
+     */
+    MockLowLevelHttpResponse lowLevelHttpResponse;
+
+    /**
+     * Constructs a new {@link Builder}. Note that this constructor was {@code protected}
+     * in version 1.17 and its predecessors, and was made {@code public} in version
+     * 1.18.
+     */
+    public Builder() {
     }
 
     /** Builds a new instance of {@link MockHttpTransport}. */
@@ -152,9 +184,14 @@ public final Builder setSupportedMethods(Set<String> supportedMethods) {
      * non-{@code null}. If {@code null}, {@link #buildRequest} will create a new
      * {@link MockLowLevelHttpRequest} arguments.
      *
+     * <p>Note that the user can set a low level HTTP Request only if a low level HTTP response
+     * has not been set on this instance.
+     *
      * @since 1.18
      */
     public final Builder setLowLevelHttpRequest(MockLowLevelHttpRequest lowLevelHttpRequest) {
+      Preconditions.checkState(lowLevelHttpResponse == null,
+          "Cannnot set a low level HTTP request when a low level HTTP response has been set.");
       this.lowLevelHttpRequest = lowLevelHttpRequest;
       return this;
     }
@@ -168,5 +205,35 @@ public final Builder setLowLevelHttpRequest(MockLowLevelHttpRequest lowLevelHttp
     public final MockLowLevelHttpRequest getLowLevelHttpRequest() {
       return lowLevelHttpRequest;
     }
+
+
+    /**
+     * Sets the {@link MockLowLevelHttpResponse} that will be the result when the
+     * {@link MockLowLevelHttpRequest} returned by {@link #buildRequest} is executed.
+     * Note that the response can be set only the caller has not provided a
+     * {@link MockLowLevelHttpRequest} via {@link #setLowLevelHttpRequest}.
+     *
+     * @throws IllegalStateException if the caller has already set a {@link LowLevelHttpRequest}
+     * in this instance
+     *
+     * @since 1.18
+     */
+    public final Builder setLowLevelHttpResponse(MockLowLevelHttpResponse lowLevelHttpResponse) {
+      Preconditions.checkState(lowLevelHttpRequest == null,
+          "Cannot set a low level HTTP response when a low level HTTP request has been set.");
+      this.lowLevelHttpResponse = lowLevelHttpResponse;
+      return this;
+    }
+
+
+    /**
+     * Returns the {@link MockLowLevelHttpResponse} that is associated with this {@link Builder},
+     * or {@code null} if no such instance exists.
+     *
+     * @since 1.18
+     */
+    MockLowLevelHttpResponse getLowLevelHttpResponse() {
+      return this.lowLevelHttpResponse;
+    }
   }
 }

google-http-client/src/main/java/com/google/api/client/util/escape/CharEscapers.java

@@ -31,6 +31,9 @@ public final class CharEscapers {
   private static final Escaper URI_PATH_ESCAPER =
       new PercentEscaper(PercentEscaper.SAFEPATHCHARS_URLENCODER, false);
 
+  private static final Escaper URI_RESERVED_ESCAPER =
+      new PercentEscaper(PercentEscaper.SAFE_PLUS_RESERVED_CHARS_URLENCODER, false);
+
   private static final Escaper URI_USERINFO_ESCAPER =
       new PercentEscaper(PercentEscaper.SAFEUSERINFOCHARS_URLENCODER, false);
 
@@ -124,6 +127,15 @@ public static String escapeUriPath(String value) {
     return URI_PATH_ESCAPER.escape(value);
   }
 
+  /**
+   * Escapes a URI path but retains all reserved characters, including all general delimiters.
+   * That is the same as {@link #escapeUriPath(String)} except that it keeps '?', '+', and '/'
+   * unescaped.
+   */
+  public static String escapeUriPathWithoutReserved(String value) {
+    return URI_RESERVED_ESCAPER.escape(value);
+  }
+
   /**
    * Escapes the string value so it can be safely included in URI user info part. For details on
    * escaping URIs, see <a href="http://tools.ietf.org/html/rfc3986#section-2.4">RFC 3986 - section

google-http-client/src/main/java/com/google/api/client/util/escape/PercentEscaper.java

@@ -70,6 +70,13 @@ public class PercentEscaper extends UnicodeEscaper {
    */
   public static final String SAFEPATHCHARS_URLENCODER = "-_.!~*'()@:$&,;=";
 
+  /**
+   * Contains the save characters plus all reserved characters. This happens to be the safe path
+   * characters plus those characters which are reserved for URI segments, namely '+', '/', and
+   * '?'.
+   */
+  public static final String SAFE_PLUS_RESERVED_CHARS_URLENCODER = SAFEPATHCHARS_URLENCODER + "+/?";
+
   /**
    * A string of characters that do not need to be encoded when used in URI user info part, as
    * specified in RFC 3986. Note that some of these characters do need to be escaped when used in

google-http-client/src/test/java/com/google/api/client/http/UriTemplateTest.java

@@ -265,4 +265,14 @@ public void testExpandTemplates_withBaseUrl() {
     assertEquals("http://test3/xyz/123/bar/", UriTemplate.expand("https://test/base/path/",
         "http://test3/{abc}/{def}/bar/", requestMap, true));
   }
+
+  public void testExpandNonReservedNonComposite() {
+    SortedMap<String, Object> requestMap = Maps.newTreeMap();
+    requestMap.put("abc", "xyz");
+    requestMap.put("def", "a/b?c");
+    assertEquals("foo/xyz/bar/a%2Fb%3Fc",
+        UriTemplate.expand("foo/{abc}/bar/{def}", requestMap, false));
+    assertEquals("foo/xyz/bar/a/b?c",
+        UriTemplate.expand("foo/{abc}/bar/{+def}", requestMap, false));
+  }
 }