Skip to content

Commit ffff9a0

Browse files
wonderflywonderfly
committed
http: Fix wrong behavior of {+var} (with var not a list) in UriTemplate.
https://codereview.appspot.com/66770043/
1 parent 0ceeaa2 commit ffff9a0

File tree

6 files changed

+116
-6
lines changed

6 files changed

+116
-6
lines changed

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import junit.framework.TestCase;
2323

2424
import java.util.Arrays;
25+
import java.util.Collection;
2526
import java.util.Set;
2627

2728
/**
@@ -56,6 +57,11 @@ public void tearDown() throws Exception {
5657
assertTrue(boolTyped.values().isEmpty());
5758
}
5859

60+
private static void assertContentsAnyOrder(Collection<?> c, Object... elts) {
61+
assertEquals(Sets.newHashSet(c),
62+
Sets.newHashSet(Arrays.asList(elts)));
63+
}
64+
5965
public void testId() throws Exception {
6066
subtestIdNoException("1");
6167
subtestIdNoException("123456789012345678901234567890");
@@ -127,7 +133,7 @@ public void testValues() throws Exception {
127133
stringTyped.set("k", "new");
128134
stringTyped.set("k2", "other");
129135
// check values
130-
assertEquals(Arrays.asList("new", "other"), stringTyped.values());
136+
assertContentsAnyOrder(stringTyped.values(), "new", "other");
131137
// delete one
132138
stringTyped.delete("k");
133139
assertNull(stringTyped.get("k"));

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,10 @@ String getEncodedValue(String value) {
189189
}
190190
return encodedValue;
191191
}
192+
193+
boolean getReservedExpansion() {
194+
return reservedExpansion;
195+
}
192196
}
193197

194198
static CompositeOutput getCompositeOutput(String propertyName) {
@@ -329,7 +333,11 @@ public static String expand(String pathUri, Object parameters,
329333
value = getMapPropertyValue(varName, map, containsExplodeModifier, compositeOutput);
330334
} else {
331335
// For everything else...
332-
value = CharEscapers.escapeUriPath(value.toString());
336+
if (compositeOutput.getReservedExpansion()) {
337+
value = CharEscapers.escapeUriPathWithoutReserved(value.toString());
338+
} else {
339+
value = CharEscapers.escapeUriPath(value.toString());
340+
}
333341
}
334342
pathBuf.append(value);
335343
}

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

Lines changed: 71 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,14 @@ public class MockHttpTransport extends HttpTransport {
4848
* */
4949
private MockLowLevelHttpRequest lowLevelHttpRequest;
5050

51+
/**
52+
* The {@link MockLowLevelHttpResponse} to be returned when this {@link MockHttpTransport}
53+
* executes the associated request. Note that this field is ignored if the caller provided
54+
* a non-{@code null} {@link MockLowLevelHttpRequest} with this {@link MockHttpTransport}
55+
* was built.
56+
*/
57+
private MockLowLevelHttpResponse lowLevelHttpResponse;
58+
5159
public MockHttpTransport() {
5260
}
5361

@@ -59,6 +67,7 @@ public MockHttpTransport() {
5967
protected MockHttpTransport(Builder builder) {
6068
supportedMethods = builder.supportedMethods;
6169
lowLevelHttpRequest = builder.lowLevelHttpRequest;
70+
lowLevelHttpResponse = builder.lowLevelHttpResponse;
6271
}
6372

6473
@Override
@@ -69,9 +78,14 @@ public boolean supportsMethod(String method) throws IOException {
6978
@Override
7079
public LowLevelHttpRequest buildRequest(String method, String url) throws IOException {
7180
Preconditions.checkArgument(supportsMethod(method), "HTTP method %s not supported", method);
72-
return lowLevelHttpRequest == null
73-
? new MockLowLevelHttpRequest(url)
74-
: lowLevelHttpRequest;
81+
if (lowLevelHttpRequest != null) {
82+
return lowLevelHttpRequest;
83+
}
84+
MockLowLevelHttpRequest request = new MockLowLevelHttpRequest(url);
85+
if (lowLevelHttpResponse != null) {
86+
request.setResponse(lowLevelHttpResponse);
87+
}
88+
return request;
7589
}
7690

7791
/**
@@ -95,8 +109,13 @@ public final MockLowLevelHttpRequest getLowLevelHttpRequest() {
95109
/**
96110
* Returns an instance of a new builder.
97111
*
112+
* <p>
113+
* @deprecated (to be removed in the future) Use {@link Builder#Builder()} instead.
114+
* </p>
115+
*
98116
* @since 1.5
99117
*/
118+
@Deprecated
100119
public static Builder builder() {
101120
return new Builder();
102121
}
@@ -124,7 +143,20 @@ public static class Builder {
124143
* */
125144
MockLowLevelHttpRequest lowLevelHttpRequest;
126145

127-
protected Builder() {
146+
/**
147+
* The {@link MockLowLevelHttpResponse} that should be the result of the
148+
* {@link MockLowLevelHttpRequest} to be returned by {@link #buildRequest}. Note
149+
* that this field is ignored if the caller provides a {@link MockLowLevelHttpRequest}
150+
* via {@link #setLowLevelHttpRequest}.
151+
*/
152+
MockLowLevelHttpResponse lowLevelHttpResponse;
153+
154+
/**
155+
* Constructs a new {@link Builder}. Note that this constructor was {@code protected}
156+
* in version 1.17 and its predecessors, and was made {@code public} in version
157+
* 1.18.
158+
*/
159+
public Builder() {
128160
}
129161

130162
/** Builds a new instance of {@link MockHttpTransport}. */
@@ -152,9 +184,14 @@ public final Builder setSupportedMethods(Set<String> supportedMethods) {
152184
* non-{@code null}. If {@code null}, {@link #buildRequest} will create a new
153185
* {@link MockLowLevelHttpRequest} arguments.
154186
*
187+
* <p>Note that the user can set a low level HTTP Request only if a low level HTTP response
188+
* has not been set on this instance.
189+
*
155190
* @since 1.18
156191
*/
157192
public final Builder setLowLevelHttpRequest(MockLowLevelHttpRequest lowLevelHttpRequest) {
193+
Preconditions.checkState(lowLevelHttpResponse == null,
194+
"Cannnot set a low level HTTP request when a low level HTTP response has been set.");
158195
this.lowLevelHttpRequest = lowLevelHttpRequest;
159196
return this;
160197
}
@@ -168,5 +205,35 @@ public final Builder setLowLevelHttpRequest(MockLowLevelHttpRequest lowLevelHttp
168205
public final MockLowLevelHttpRequest getLowLevelHttpRequest() {
169206
return lowLevelHttpRequest;
170207
}
208+
209+
210+
/**
211+
* Sets the {@link MockLowLevelHttpResponse} that will be the result when the
212+
* {@link MockLowLevelHttpRequest} returned by {@link #buildRequest} is executed.
213+
* Note that the response can be set only the caller has not provided a
214+
* {@link MockLowLevelHttpRequest} via {@link #setLowLevelHttpRequest}.
215+
*
216+
* @throws IllegalStateException if the caller has already set a {@link LowLevelHttpRequest}
217+
* in this instance
218+
*
219+
* @since 1.18
220+
*/
221+
public final Builder setLowLevelHttpResponse(MockLowLevelHttpResponse lowLevelHttpResponse) {
222+
Preconditions.checkState(lowLevelHttpRequest == null,
223+
"Cannot set a low level HTTP response when a low level HTTP request has been set.");
224+
this.lowLevelHttpResponse = lowLevelHttpResponse;
225+
return this;
226+
}
227+
228+
229+
/**
230+
* Returns the {@link MockLowLevelHttpResponse} that is associated with this {@link Builder},
231+
* or {@code null} if no such instance exists.
232+
*
233+
* @since 1.18
234+
*/
235+
MockLowLevelHttpResponse getLowLevelHttpResponse() {
236+
return this.lowLevelHttpResponse;
237+
}
171238
}
172239
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@ public final class CharEscapers {
3131
private static final Escaper URI_PATH_ESCAPER =
3232
new PercentEscaper(PercentEscaper.SAFEPATHCHARS_URLENCODER, false);
3333

34+
private static final Escaper URI_RESERVED_ESCAPER =
35+
new PercentEscaper(PercentEscaper.SAFE_PLUS_RESERVED_CHARS_URLENCODER, false);
36+
3437
private static final Escaper URI_USERINFO_ESCAPER =
3538
new PercentEscaper(PercentEscaper.SAFEUSERINFOCHARS_URLENCODER, false);
3639

@@ -124,6 +127,15 @@ public static String escapeUriPath(String value) {
124127
return URI_PATH_ESCAPER.escape(value);
125128
}
126129

130+
/**
131+
* Escapes a URI path but retains all reserved characters, including all general delimiters.
132+
* That is the same as {@link #escapeUriPath(String)} except that it keeps '?', '+', and '/'
133+
* unescaped.
134+
*/
135+
public static String escapeUriPathWithoutReserved(String value) {
136+
return URI_RESERVED_ESCAPER.escape(value);
137+
}
138+
127139
/**
128140
* Escapes the string value so it can be safely included in URI user info part. For details on
129141
* 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‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,13 @@ public class PercentEscaper extends UnicodeEscaper {
7070
*/
7171
public static final String SAFEPATHCHARS_URLENCODER = "-_.!~*'()@:$&,;=";
7272

73+
/**
74+
* Contains the save characters plus all reserved characters. This happens to be the safe path
75+
* characters plus those characters which are reserved for URI segments, namely '+', '/', and
76+
* '?'.
77+
*/
78+
public static final String SAFE_PLUS_RESERVED_CHARS_URLENCODER = SAFEPATHCHARS_URLENCODER + "+/?";
79+
7380
/**
7481
* A string of characters that do not need to be encoded when used in URI user info part, as
7582
* 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‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,4 +265,14 @@ public void testExpandTemplates_withBaseUrl() {
265265
assertEquals("http://test3/xyz/123/bar/", UriTemplate.expand("https://test/base/path/",
266266
"http://test3/{abc}/{def}/bar/", requestMap, true));
267267
}
268+
269+
public void testExpandNonReservedNonComposite() {
270+
SortedMap<String, Object> requestMap = Maps.newTreeMap();
271+
requestMap.put("abc", "xyz");
272+
requestMap.put("def", "a/b?c");
273+
assertEquals("foo/xyz/bar/a%2Fb%3Fc",
274+
UriTemplate.expand("foo/{abc}/bar/{def}", requestMap, false));
275+
assertEquals("foo/xyz/bar/a/b?c",
276+
UriTemplate.expand("foo/{abc}/bar/{+def}", requestMap, false));
277+
}
268278
}

0 commit comments

Comments
 (0)