Skip to content

Commit

Permalink
GH-1270: Fix conflict between single and multi-value headers (#1347)
Browse files Browse the repository at this point in the history 
Fixes: #1270

`HeaderTemplate` was confusing iterable values with literal values due
to the presence of comma `,` characters in the result.  The result was
that, in certain cases like HTTP Dates, additional spaces were inserted
into the final expanded value.

The root cause of the issue is that `HeaderTemplate` combined all values
into a single `String` template, with each value separated by a comma.

This change refactors `HeaderTemplate` to treat all `values` as individual
`Templates`, removing the need to combine any provided values into a single
String.

* Remove unnecessary string splits when expanding Headers in RequestTemplate
  • Loading branch information
@kdavisk6
kdavisk6 committed Dec 29, 2020
1 parent 784301a commit f8ad867
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 122 deletions.
38 changes: 2 additions & 36 deletions core/src/main/java/feign/RequestTemplate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,12 +231,8 @@ public RequestTemplate resolve(Map<String, ?> variables) {
/* resolve the header */
String header = headerTemplate.expand(variables);
if (!header.isEmpty()) {
/* split off the header values and add it to the resolved template */
String headerValues = header.substring(header.indexOf(" ") + 1);
if (!headerValues.isEmpty()) {
/* append the header as a new literal as the value has already been expanded. */
resolved.header(headerTemplate.getName(), Literal.create(headerValues));
}
/* append the header as a new literal as the value has already been expanded. */
resolved.header(headerTemplate.getName(), header);
}
}
}
Expand Down Expand Up @@ -694,20 +690,6 @@ public RequestTemplate header(String name, String... values) {
return header(name, Arrays.asList(values));
}

/**
* Add a header using the supplied Chunks.
*
* @param name of the header.
* @param chunks to add.
* @return a RequestTemplate for chaining.
*/
private RequestTemplate header(String name, TemplateChunk... chunks) {
if (chunks == null) {
throw new IllegalArgumentException("chunks are required.");
}
return appendHeader(name, Arrays.asList(chunks));
}

/**
* Specify a Header, with the specified values. Values can be literals or template expressions.
*
Expand Down Expand Up @@ -771,22 +753,6 @@ private RequestTemplate appendHeader(String name, Iterable<String> values) {
return this;
}

private RequestTemplate appendHeader(String name, List<TemplateChunk> chunks) {
if (chunks.isEmpty()) {
this.headers.remove(name);
return this;
}

this.headers.compute(name, (headerName, headerTemplate) -> {
if (headerTemplate == null) {
return HeaderTemplate.from(name, chunks);
} else {
return HeaderTemplate.appendFrom(headerTemplate, chunks);
}
});
return this;
}

/**
* Headers for this Request.
*
Expand Down
132 changes: 55 additions & 77 deletions core/src/main/java/feign/template/HeaderTemplate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
package feign.template;

import feign.Util;
import feign.template.Template.EncodingOptions;
import feign.template.Template.ExpansionOptions;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
Expand All @@ -29,23 +31,10 @@
* Template for HTTP Headers. Variables that are unresolved are ignored and Literals are not
* encoded.
*/
public final class HeaderTemplate extends Template {
public final class HeaderTemplate {

/* cache a copy of the variables for lookup later */
private LinkedHashSet<String> values;
private String name;

public static HeaderTemplate from(String name, List<TemplateChunk> chunks) {
if (name == null || name.isEmpty()) {
throw new IllegalArgumentException("name is required.");
}

if (chunks == null) {
throw new IllegalArgumentException("chunks are required.");
}

return new HeaderTemplate(name, Util.UTF_8, chunks);
}
private final String name;
private final List<Template> values = new CopyOnWriteArrayList<>();

public static HeaderTemplate create(String name, Iterable<String> values) {
if (name == null || name.isEmpty()) {
Expand All @@ -56,20 +45,7 @@ public static HeaderTemplate create(String name, Iterable<String> values) {
throw new IllegalArgumentException("values are required");
}

/* construct a uri template from the name and values */
StringBuilder template = new StringBuilder();
template.append(name)
.append(" ");

/* create a comma separated template for the header values */
Iterator<String> iterator = values.iterator();
while (iterator.hasNext()) {
template.append(iterator.next());
if (iterator.hasNext()) {
template.append(",");
}
}
return new HeaderTemplate(template.toString(), name, values, Util.UTF_8);
return new HeaderTemplate(name, values, Util.UTF_8);
}

/**
Expand All @@ -88,67 +64,69 @@ public static HeaderTemplate append(HeaderTemplate headerTemplate, Iterable<Stri
}

/**
* Append {@link TemplateChunk} to a Header Template.
*
* @param headerTemplate to append to.
* @param chunks to append.
* @return a new HeaderTemplate with the values added.
*/
public static HeaderTemplate appendFrom(HeaderTemplate headerTemplate,
List<TemplateChunk> chunks) {
List<TemplateChunk> existing = new CopyOnWriteArrayList<>(headerTemplate.getTemplateChunks());
existing.addAll(chunks);
return from(headerTemplate.getName(), existing);
}

/**
* Creates a new Header Template.
* Create a new Header Template.
*
* @param template to parse.
* @param name of the Header.
* @param values for the Header.
* @param charset to use when encoding the values.
*/
private HeaderTemplate(String template, String name, Iterable<String> values, Charset charset) {
super(template, ExpansionOptions.REQUIRED, EncodingOptions.NOT_REQUIRED, false, charset);
this.values = StreamSupport.stream(values.spliterator(), false)
.filter(Util::isNotBlank)
.collect(Collectors.toCollection(LinkedHashSet::new));
private HeaderTemplate(String name, Iterable<String> values, Charset charset) {
this.name = name;
}

/**
* Creates a new Header Template from a set of TemplateChunks.
*
* @param name of the header.
* @param charset to encode the expanded values in.
* @param chunks for the template.
*/
private HeaderTemplate(String name, Charset charset, List<TemplateChunk> chunks) {
super(ExpansionOptions.REQUIRED, EncodingOptions.NOT_REQUIRED, false, charset,
chunks);
this.values = chunks.stream()
.map(TemplateChunk::getValue)
.collect(Collectors.toCollection(LinkedHashSet::new));
this.name = name;
for (String value : values) {
if (value == null || value.isEmpty()) {
/* skip */
continue;
}

this.values.add(
new Template(
value,
ExpansionOptions.REQUIRED,
EncodingOptions.NOT_REQUIRED,
false,
charset));
}
}

public Collection<String> getValues() {
return Collections.unmodifiableCollection(values);
return Collections.unmodifiableList(this.values.stream()
.map(Template::toString)
.collect(Collectors.toList()));
}

public List<String> getVariables() {
List<String> variables = new ArrayList<>();
for (Template template : this.values) {
variables.addAll(template.getVariables());
}
return Collections.unmodifiableList(variables);
}

public String getName() {
return name;
return this.name;
}

@Override
public String expand(Map<String, ?> variables) {
String result = super.expand(variables);
List<String> expanded = new ArrayList<>();
if (!this.values.isEmpty()) {
for (Template template : this.values) {
String result = template.expand(variables);

if (result == null) {
/* ignore unresolved values */
continue;
}

expanded.add(result);
}
}

/* remove any trailing commas */
while (result.endsWith(",")) {
result = result.replaceAll(",$", "");
StringBuilder result = new StringBuilder();
if (!expanded.isEmpty()) {
result.append(String.join(", ", expanded));
}

/* space all the commas now */
result = result.replaceAll(",", ", ");
return result;
return result.toString();
}
}
23 changes: 14 additions & 9 deletions core/src/test/java/feign/template/HeaderTemplateTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Map;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
Expand Down Expand Up @@ -46,12 +47,6 @@ public void it_should_throw_exception_when_value_is_null() {
exception.expectMessage("values are required");
}

@Test(expected = IllegalArgumentException.class)
public void it_should_throw_exception_when_value_is_null_for_chunks() {
HeaderTemplate.from("test", null);
exception.expectMessage("values are required");
}

@Test
public void it_should_return_name() {
HeaderTemplate headerTemplate =
Expand All @@ -63,16 +58,16 @@ public void it_should_return_name() {
public void it_should_return_expanded() {
HeaderTemplate headerTemplate =
HeaderTemplate.create("hello", Arrays.asList("emre", "savci", "{name}", "{missing}"));
assertEquals("hello emre, savci", headerTemplate.expand(Collections.emptyMap()));
assertEquals("hello emre, savci, firsts",
assertEquals("emre, savci", headerTemplate.expand(Collections.emptyMap()));
assertEquals("emre, savci, firsts",
headerTemplate.expand(Collections.singletonMap("name", "firsts")));
}

@Test
public void it_should_return_expanded_literals() {
HeaderTemplate headerTemplate =
HeaderTemplate.create("hello", Arrays.asList("emre", "savci", "{replace_me}"));
assertEquals("hello emre, savci, {}",
assertEquals("emre, savci, {}",
headerTemplate.expand(Collections.singletonMap("replace_me", "{}")));
}

Expand Down Expand Up @@ -111,4 +106,14 @@ public void append_should_preserve_order() {
assertThat(new ArrayList<>(headerTemplateWithSecondOrdering.getValues()),
equalTo(Arrays.asList("test 2", "test 1")));
}

@Test
public void it_should_support_http_date() {
HeaderTemplate headerTemplate =
HeaderTemplate.create("Expires", Collections.singletonList("{expires}"));
assertEquals(
headerTemplate.expand(
Collections.singletonMap("expires", "Wed, 4 Jul 2001 12:08:56 -0700")),
"Wed, 4 Jul 2001 12:08:56 -0700");
}
}
2 changes: 2 additions & 0 deletions ribbon/src/test/java/feign/ribbon/RibbonClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.junit.After;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TestName;
Expand All @@ -48,6 +49,7 @@
import feign.Retryer;
import feign.client.TrustingSSLSocketFactory;

@Ignore("inconsistent, deprecated toolset")
public class RibbonClientTest {

@Rule
Expand Down

0 comments on commit f8ad867

Please sign in to comment.