Skip to content

Commit

Permalink
Fixes vnd header regression by changing Headers encoding
Browse files Browse the repository at this point in the history
I mistakenly advised `@Headers` to follow the encoding rules of `@Body`.
This was a a mistake as in both cases, url encoding is a bad choice, if
the only goal is to prevent accidental variable expansion. For example,
url encoding interferes a lot with content, including messing with '+'
characters, such as exist in "Accept: application/vnd.github.v3+json"

This changes `@Headers` to only address the problem, which where a '{'
literal is desired in a header value. The solution offered here is to
simply repeat "{{" when you desire a '{' literal. For example, if your
header value needs to be literally "{{variable}}", you'd encode it as
"{{{{variable}}".

The impact of this change is limited to those who have already started
using v8.15, and a fast release will occur after merge to limit that.

See #326
Fixes #345
Closes #346
  • Loading branch information
Adrian Cole committed Mar 9, 2016
1 parent b29f9d8 commit 57444d1
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 17 deletions.
33 changes: 20 additions & 13 deletions core/src/main/java/feign/RequestTemplate.java
Expand Up @@ -60,7 +60,6 @@ public final class RequestTemplate implements Serializable {
private boolean decodeSlash = true;

public RequestTemplate() {

}

/* Copy constructor. Use this when making templates. */
Expand Down Expand Up @@ -128,9 +127,19 @@ public static String expand(String template, Map<String, ?> variables) {
for (char c : template.toCharArray()) {
switch (c) {
case '{':
if (inVar) {
// '{{' is an escape: write the brace and don't interpret as a variable
builder.append("{");
inVar = false;
break;
}
inVar = true;
break;
case '}':
if (!inVar) { // then write the brace literally
builder.append('}');
break;
}
inVar = false;
String key = var.toString();
Object value = variables.get(var.toString());
Expand Down Expand Up @@ -209,13 +218,11 @@ public RequestTemplate resolve(Map<String, ?> unencoded) {
}
url = new StringBuilder(resolvedUrl);

Map<String, Collection<String>>
resolvedHeaders =
new LinkedHashMap<String, Collection<String>>();
Map<String, Collection<String>> resolvedHeaders = new LinkedHashMap<String, Collection<String>>();
for (String field : headers.keySet()) {
Collection<String> resolvedValues = new ArrayList<String>();
for (String value : valuesOrEmpty(headers, field)) {
String resolved = urlDecode(expand(value, encoded));
String resolved = expand(value, unencoded);
resolvedValues.add(resolved);
}
resolvedHeaders.put(field, resolvedValues);
Expand Down Expand Up @@ -285,37 +292,37 @@ public String url() {
}

/**
* Replaces queries with the specified {@code configKey} with url decoded {@code values} supplied.
* Replaces queries with the specified {@code name} with url decoded {@code values} supplied.
* <br> When the {@code value} is {@code null}, all queries with the {@code configKey} are
* removed. <br> <br><br><b>relationship to JAXRS 2.0</b><br> <br> Like {@code WebTarget.query},
* except the values can be templatized. <br> ex. <br>
* <pre>
* template.query(&quot;Signature&quot;, &quot;{signature}&quot;);
* </pre>
*
* @param configKey the configKey of the query
* @param name the name of the query
* @param values can be a single null to imply removing all values. Else no values are expected
* to be null.
* @see #queries()
*/
public RequestTemplate query(String configKey, String... values) {
queries.remove(checkNotNull(configKey, "configKey"));
public RequestTemplate query(String name, String... values) {
queries.remove(checkNotNull(name, "name"));
if (values != null && values.length > 0 && values[0] != null) {
ArrayList<String> encoded = new ArrayList<String>();
for (String value : values) {
encoded.add(encodeIfNotVariable(value));
}
this.queries.put(encodeIfNotVariable(configKey), encoded);
this.queries.put(encodeIfNotVariable(name), encoded);
}
return this;
}

/* @see #query(String, String...) */
public RequestTemplate query(String configKey, Iterable<String> values) {
public RequestTemplate query(String name, Iterable<String> values) {
if (values != null) {
return query(configKey, toArray(values, String.class));
return query(name, toArray(values, String.class));
}
return query(configKey, (String[]) null);
return query(name, (String[]) null);
}

private String encodeIfNotVariable(String in) {
Expand Down
20 changes: 16 additions & 4 deletions core/src/test/java/feign/RequestTemplateTest.java
Expand Up @@ -154,14 +154,26 @@ public void resolveTemplateWithHeaderSubstitutionsNotAtStart() {
}

@Test
public void resolveTemplateWithHeaderWithURLEncodedElements() {
public void resolveTemplateWithHeaderWithEscapedCurlyBrace() {
RequestTemplate template = new RequestTemplate().method("GET")
.header("Encoded", "%7Bvar%7D");
.header("Encoded", "{{{{dont_expand_me}}");

template.resolve(mapOf("var", "1234"));
template.resolve(mapOf("dont_expand_me", "1234"));

assertThat(template)
.hasHeaders(entry("Encoded", asList("{var}")));
.hasHeaders(entry("Encoded", asList("{{dont_expand_me}}")));
}

/** This ensures we don't mess up vnd types */
@Test
public void resolveTemplateWithHeaderIncludingSpecialCharacters() {
RequestTemplate template = new RequestTemplate().method("GET")
.header("Accept", "application/vnd.github.v3+{type}");

template.resolve(mapOf("type", "json"));

assertThat(template)
.hasHeaders(entry("Accept", asList("application/vnd.github.v3+json")));
}

@Test
Expand Down

0 comments on commit 57444d1

Please sign in to comment.