api: Delete io.grpc.Uri#getQuery and Builder#setQuery

Author: jdcormie

api/src/main/java/io/grpc/Uri.java

@@ -546,24 +546,17 @@ public String getRawPath() {
     return path;
   }
 
-  /**
-   * Returns the percent-decoded "query" component of this URI, or null if not present.
-   *
-   * <p>NB: This method assumes the query was encoded as UTF-8, although RFC 3986 doesn't specify an
-   * encoding.
-   *
-   * <p>Decoding errors are indicated by a {@code '\u005CuFFFD'} unicode replacement character in
-   * the output. Callers who want to detect and handle errors in some other way should call {@link
-   * #getRawQuery()}, {@link #percentDecode(CharSequence)}, then decode the bytes for themselves.
-   */
-  @Nullable
-  public String getQuery() {
-    return percentDecodeAssumedUtf8(query);
-  }
-
   /**
    * Returns the query component of this URI in its originally parsed, possibly percent-encoded
-   * form, without any leading '?' character.
+   * form, without any leading '?' character, or null if not present.
+   *
+   * <p>The query component can only be read in its raw form. That’s because virtually everyone uses
+   * it as a container with some additional layer of structure unknown to RFC-3986. Like <code>
+   * ?k1=v1&k2=v+2</code> "query parameters". These containers always have some set of characters
+   * that take on a special delimiter meaning when not percent-encoded and a literal meaning when
+   * they are (like '&', '=' and '+' above). Since it matters whether a character was percent
+   * encoded or not, offering a '#getQuery()' method that percent-decodes everything like we do for
+   * other components be error-prone.
    */
   @Nullable
   public String getRawQuery() {
@@ -776,25 +769,30 @@ public Builder setRawPath(String path) {
     }
 
     /**
-     * Specifies the query component of the new URI (not including the leading '?').
+     * Specifies the query component of the new URI, possibly percent-encoded, exactly as it will
+     * appear in the string form of the built URI.
      *
-     * <p>Query can contain any string of codepoints. Codepoints that can't be encoded literally
-     * will be percent-encoded for you as UTF-8.
+     * <p>'query' must only contain codepoints from RFC 3986's "query" character class. Any other
+     * characters must be percent-encoded using UTF-8. Do not include the leading '?' delimiter.
+     *
+     * <p>The query component can only be provided in raw form. That’s because virtually everyone
+     * uses it as a container with some additional layer of structure unknown to RFC-3986. Like
+     * <code>?k1=v1&k2=v+2</code> "query parameters". These containers always have some set of
+     * characters that take on a special delimiter meaning when not percent-encoded and a literal
+     * meaning when they are (like '&', '=' and '+' above). Since 'query' must have already been
+     * carefully percent-encoded externally, a '#setQuery(String)' method that percent-encodes an
+     * assumed-cooked string would be error-prone.
      *
      * <p>This field is optional.
      *
      * @param query the new query component, or null to clear this field
      * @return this, for fluent building
      */
     @CanIgnoreReturnValue
-    public Builder setQuery(@Nullable String query) {
-      this.query = percentEncode(query, queryChars);
-      return this;
-    }
-
-    @CanIgnoreReturnValue
-    Builder setRawQuery(String query) {
-      checkPercentEncodedArg(query, "query", queryChars);
+    public Builder setRawQuery(@Nullable String query) {
+      if (query != null) {
+        checkPercentEncodedArg(query, "query", queryChars);
+      }
       this.query = query;
       return this;
     }

api/src/test/java/io/grpc/UriTest.java

@@ -42,7 +42,7 @@ public void parse_allComponents() throws URISyntaxException {
     assertThat(uri.getPort()).isEqualTo(443);
     assertThat(uri.getRawPort()).isEqualTo("0443");
     assertThat(uri.getPath()).isEqualTo("/path");
-    assertThat(uri.getQuery()).isEqualTo("query");
+    assertThat(uri.getRawQuery()).isEqualTo("query");
     assertThat(uri.getFragment()).isEqualTo("fragment");
     assertThat(uri.toString()).isEqualTo("scheme://user@host:0443/path?query#fragment");
     assertThat(uri.isAbsolute()).isFalse(); // Has a fragment.
@@ -56,7 +56,7 @@ public void parse_noAuthority() throws URISyntaxException {
     assertThat(uri.getScheme()).isEqualTo("scheme");
     assertThat(uri.getAuthority()).isNull();
     assertThat(uri.getPath()).isEqualTo("/path");
-    assertThat(uri.getQuery()).isEqualTo("query");
+    assertThat(uri.getRawQuery()).isEqualTo("query");
     assertThat(uri.getFragment()).isEqualTo("fragment");
     assertThat(uri.toString()).isEqualTo("scheme:/path?query#fragment");
     assertThat(uri.isAbsolute()).isFalse(); // Has a fragment.
@@ -102,7 +102,7 @@ public void parse_noQuery() throws URISyntaxException {
     assertThat(uri.getScheme()).isEqualTo("scheme");
     assertThat(uri.getAuthority()).isEqualTo("authority");
     assertThat(uri.getPath()).isEqualTo("/path");
-    assertThat(uri.getQuery()).isNull();
+    assertThat(uri.getRawQuery()).isNull();
     assertThat(uri.getFragment()).isEqualTo("fragment");
     assertThat(uri.toString()).isEqualTo("scheme://authority/path#fragment");
   }
@@ -113,7 +113,7 @@ public void parse_noFragment() throws URISyntaxException {
     assertThat(uri.getScheme()).isEqualTo("scheme");
     assertThat(uri.getAuthority()).isEqualTo("authority");
     assertThat(uri.getPath()).isEqualTo("/path");
-    assertThat(uri.getQuery()).isEqualTo("query");
+    assertThat(uri.getRawQuery()).isEqualTo("query");
     assertThat(uri.getFragment()).isNull();
     assertThat(uri.toString()).isEqualTo("scheme://authority/path?query");
     assertThat(uri.isAbsolute()).isTrue();
@@ -125,7 +125,7 @@ public void parse_emptyPathWithAuthority() throws URISyntaxException {
     assertThat(uri.getScheme()).isEqualTo("scheme");
     assertThat(uri.getAuthority()).isEqualTo("authority");
     assertThat(uri.getPath()).isEmpty();
-    assertThat(uri.getQuery()).isNull();
+    assertThat(uri.getRawQuery()).isNull();
     assertThat(uri.getFragment()).isNull();
     assertThat(uri.toString()).isEqualTo("scheme://authority");
     assertThat(uri.isAbsolute()).isTrue();
@@ -139,7 +139,7 @@ public void parse_rootless() throws URISyntaxException {
     assertThat(uri.getScheme()).isEqualTo("mailto");
     assertThat(uri.getAuthority()).isNull();
     assertThat(uri.getPath()).isEqualTo("[email protected]");
-    assertThat(uri.getQuery()).isEqualTo("subject=raise");
+    assertThat(uri.getRawQuery()).isEqualTo("subject=raise");
     assertThat(uri.getFragment()).isNull();
     assertThat(uri.toString()).isEqualTo("mailto:[email protected]?subject=raise");
     assertThat(uri.isAbsolute()).isTrue();
@@ -153,7 +153,7 @@ public void parse_emptyPath() throws URISyntaxException {
     assertThat(uri.getScheme()).isEqualTo("scheme");
     assertThat(uri.getAuthority()).isNull();
     assertThat(uri.getPath()).isEmpty();
-    assertThat(uri.getQuery()).isNull();
+    assertThat(uri.getRawQuery()).isNull();
     assertThat(uri.getFragment()).isNull();
     assertThat(uri.toString()).isEqualTo("scheme:");
     assertThat(uri.isAbsolute()).isTrue();
@@ -165,7 +165,7 @@ public void parse_emptyPath() throws URISyntaxException {
   public void parse_emptyQuery() {
     Uri uri = Uri.create("scheme:?");
     assertThat(uri.getScheme()).isEqualTo("scheme");
-    assertThat(uri.getQuery()).isEmpty();
+    assertThat(uri.getRawQuery()).isEmpty();
   }
 
   @Test
@@ -322,7 +322,6 @@ public void parse_decoding() throws URISyntaxException {
     assertThat(uri.getPort()).isEqualTo(1234);
     assertThat(uri.getPath()).isEqualTo("/p ath");
     assertThat(uri.getRawPath()).isEqualTo("/p%20ath");
-    assertThat(uri.getQuery()).isEqualTo("q uery");
     assertThat(uri.getRawQuery()).isEqualTo("q%20uery");
     assertThat(uri.getFragment()).isEqualTo("f ragment");
     assertThat(uri.getRawFragment()).isEqualTo("f%20ragment");
@@ -336,9 +335,8 @@ public void parse_decodingNonAscii() throws URISyntaxException {
 
   @Test
   public void parse_decodingPercent() throws URISyntaxException {
-    Uri uri = Uri.parse("s://a/p%2520ath?q%25uery#f%25ragment");
+    Uri uri = Uri.parse("s://a/p%2520ath#f%25ragment");
     assertThat(uri.getPath()).isEqualTo("/p%20ath");
-    assertThat(uri.getQuery()).isEqualTo("q%uery");
     assertThat(uri.getFragment()).isEqualTo("f%ragment");
   }
 
@@ -420,7 +418,7 @@ public void toString_percentEncoding() throws URISyntaxException {
             .setScheme("s")
             .setHost("a b")
             .setPath("/p ath")
-            .setQuery("q uery")
+            .setRawQuery("q%20uery")
             .setFragment("f ragment")
             .build();
     assertThat(uri.toString()).isEqualTo("s://a%20b/p%20ath?q%20uery#f%20ragment");
@@ -440,7 +438,6 @@ public void parse_transparentRoundTrip_ipLiteral() {
     assertThat(uri.getRawPath()).isEqualTo("/%4a%4B%2f%2F");
     assertThat(uri.getPathSegments()).containsExactly("JK//");
     assertThat(uri.getRawQuery()).isEqualTo("%4c%4D");
-    assertThat(uri.getQuery()).isEqualTo("LM");
     assertThat(uri.getRawFragment()).isEqualTo("%4e%4F");
     assertThat(uri.getFragment()).isEqualTo("NO");
   }
@@ -459,7 +456,6 @@ public void parse_transparentRoundTrip_regName() {
     assertThat(uri.getRawPath()).isEqualTo("/%4a%4B%2f%2F");
     assertThat(uri.getPathSegments()).containsExactly("JK//");
     assertThat(uri.getRawQuery()).isEqualTo("%4c%4D");
-    assertThat(uri.getQuery()).isEqualTo("LM");
     assertThat(uri.getRawFragment()).isEqualTo("%4e%4F");
     assertThat(uri.getFragment()).isEqualTo("NO");
   }
@@ -529,7 +525,7 @@ public void builder_encodingWithAllowedReservedChars() throws URISyntaxException
             .setUserInfo("u@")
             .setHost("a[]")
             .setPath("/p:/@")
-            .setQuery("q/?")
+            .setRawQuery("q/?")
             .setFragment("f/?")
             .build();
     assertThat(uri.toString()).isEqualTo("s://u%40@a%5B%5D/p:/@?q/?#f/?");
@@ -600,7 +596,7 @@ public void builder_normalizesCaseWhereAppropriate() {
             .setScheme("hTtP") // #section-3.1 says producers (Builder) should normalize to lower.
             .setHost("aBc") // #section-3.2.2 says producers (Builder) should normalize to lower.
             .setPath("/CdE") // #section-6.2.2.1 says the rest are assumed to be case-sensitive
-            .setQuery("fGh")
+            .setRawQuery("fGh")
             .setFragment("IjK")
             .build();
     assertThat(uri.toString()).isEqualTo("http://abc/CdE?fGh#IjK");
@@ -621,12 +617,32 @@ public void builder_canClearAllOptionalFields() {
             .setPath("")
             .setUserInfo(null)
             .setPort(-1)
-            .setQuery(null)
+            .setRawQuery(null)
             .setFragment(null)
             .build();
     assertThat(uri.toString()).isEqualTo("http:");
   }
 
+  @Test
+  public void builder_setRawQuery() {
+    Uri uri = Uri.newBuilder().setScheme("http").setHost("host").setRawQuery("%61=b&c=%64").build();
+    assertThat(uri.getRawQuery()).isEqualTo("%61=b&c=%64");
+    assertThat(uri.toString()).isEqualTo("http://host?%61=b&c=%64");
+  }
+
+  @Test
+  public void builder_setRawQuery_null() {
+    Uri uri =
+        Uri.newBuilder()
+            .setScheme("http")
+            .setHost("host")
+            .setRawQuery("a=b")
+            .setRawQuery(null)
+            .build();
+    assertThat(uri.getRawQuery()).isNull();
+    assertThat(uri.toString()).isEqualTo("http://host");
+  }
+
   @Test
   public void builder_canClearAuthorityComponents() {
     Uri uri = Uri.create("s://user@host:80/path").toBuilder().setRawAuthority(null).build();
@@ -692,7 +708,7 @@ public void toString_percentEncodingLiteralPercent() throws URISyntaxException {
             .setScheme("s")
             .setHost("a")
             .setPath("/p%20ath")
-            .setQuery("q%uery")
+            .setRawQuery("q%25uery")
             .setFragment("f%ragment")
             .build();
     assertThat(uri.toString()).isEqualTo("s://a/p%2520ath?q%25uery#f%25ragment");