Skip to content

Commit

Permalink
Ensure Iterable values are encoded before template expansion (#1138)
Browse files Browse the repository at this point in the history 
* Ensure Iterable values are encoded before template expansion

Fixes #1123, Fixes #1133, Fixes #1102, Fixes #1028

Ensures that all expressions are fully-encoded before being
manipulated during template expansion.  This allows parameters
to include reserved values and result in properly encoded
results.

Additionally, `Iterable` values are now handled in accordance
with RFC 6570 allowing for the specified `CollectionFormat` to
be applied and empty parameters to be expanded correctly as this
is the main use case that exhibited this issue.
  • Loading branch information
@kdavisk6
kdavisk6 committed Dec 27, 2019
1 parent 033de93 commit a7b7c01
Show file tree
Hide file tree
Showing 10 changed files with 231 additions and 326 deletions.
8 changes: 4 additions & 4 deletions core/src/main/java/feign/CollectionFormat.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,21 +74,21 @@ public CharSequence join(String field, Collection<String> values, Charset charse
if (separator == null) {
// exploded
builder.append(valueCount++ == 0 ? "" : "&");
builder.append(UriUtils.queryEncode(field, charset));
builder.append(UriUtils.encode(field, charset));
if (value != null) {
builder.append('=');
builder.append(UriUtils.queryEncode(value, charset));
builder.append(UriUtils.encode(value, charset));
}
} else {
// delimited with a separator character
if (builder.length() == 0) {
builder.append(UriUtils.queryEncode(field, charset));
builder.append(UriUtils.encode(field, charset));
}
if (value == null) {
continue;
}
builder.append(valueCount++ == 0 ? "=" : separator);
builder.append(UriUtils.queryEncode(value, charset));
builder.append(UriUtils.encode(value, charset));
}
}
return builder;
Expand Down
5 changes: 4 additions & 1 deletion core/src/main/java/feign/RequestTemplate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,10 @@ public RequestTemplate resolve(Map<String, ?> variables) {
this.uriTemplate = UriTemplate.create("", !this.decodeSlash, this.charset);
}

uri.append(this.uriTemplate.expand(variables));
String expanded = this.uriTemplate.expand(variables);
if (expanded != null) {
uri.append(expanded);
}

/*
* for simplicity, combine the queries into the uri and use the resulting uri to seed the
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/feign/template/Expression.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
*/
package feign.template;

import feign.CollectionFormat;
import java.util.Optional;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
Expand Down
55 changes: 39 additions & 16 deletions core/src/main/java/feign/template/Expressions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,13 @@
*/
package feign.template;

import feign.Util;
import feign.template.UriUtils.FragmentType;
import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import feign.Util;

public final class Expressions {
private static Map<Pattern, Class<? extends Expression>> expressions;
Expand All @@ -43,7 +40,7 @@ public final class Expressions {
SimpleExpression.class);
}

public static Expression create(final String value, final FragmentType type) {
public static Expression create(final String value) {

/* remove the start and end braces */
final String expression = stripBraces(value);
Expand Down Expand Up @@ -86,7 +83,7 @@ public static Expression create(final String value, final FragmentType type) {
}
}

return new SimpleExpression(variableName, variablePattern, type);
return new SimpleExpression(variableName, variablePattern);
}

private static String stripBraces(String expression) {
Expand All @@ -105,26 +102,19 @@ private static String stripBraces(String expression) {
*/
static class SimpleExpression extends Expression {

private final FragmentType type;

SimpleExpression(String expression, String pattern, FragmentType type) {
SimpleExpression(String expression, String pattern) {
super(expression, pattern);
this.type = type;
}

String encode(Object value) {
return UriUtils.encodeReserved(value.toString(), type, Util.UTF_8);
return UriUtils.encode(value.toString(), Util.UTF_8);
}

@Override
String expand(Object variable, boolean encode) {
StringBuilder expanded = new StringBuilder();
if (Iterable.class.isAssignableFrom(variable.getClass())) {
List<String> items = new ArrayList<>();
for (Object item : ((Iterable) variable)) {
items.add((encode) ? encode(item) : item.toString());
}
expanded.append(String.join(Template.COLLECTION_DELIMITER, items));
expanded.append(this.expandIterable((Iterable<?>) variable));
} else {
expanded.append((encode) ? encode(variable) : variable);
}
Expand All @@ -137,5 +127,38 @@ String expand(Object variable, boolean encode) {
}
return result;
}


private String expandIterable(Iterable<?> values) {
StringBuilder result = new StringBuilder();
for (Object value : values) {
if (value == null) {
/* skip */
continue;
}

/* expand the value */
String expanded = this.encode(value);
if (expanded.isEmpty()) {
/* always append the separator */
result.append(",");
} else {
if (result.length() != 0) {
if (!result.toString().equalsIgnoreCase(",")) {
result.append(",");
}
}
result.append(expanded);
}
}

if (result.length() == 0) {
/* completely unresolved */
return null;
}

/* return the expanded value */
return result.toString();
}
}
}
112 changes: 66 additions & 46 deletions core/src/main/java/feign/template/QueryTemplate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,29 @@
*/
package feign.template;

import feign.CollectionFormat;
import feign.Util;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Iterator;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
import feign.CollectionFormat;
import feign.Util;
import feign.template.Template.EncodingOptions;
import feign.template.Template.ExpansionOptions;

/**
* Template for a Query String parameter.
*/
public final class QueryTemplate extends Template {
public final class QueryTemplate {

private static final String UNDEF = "undef";
private List<String> values;
private List<Template> values;
private final Template name;
private final CollectionFormat collectionFormat;
private boolean pure = false;
Expand Down Expand Up @@ -75,16 +78,7 @@ public static QueryTemplate create(String name,
.filter(Util::isNotBlank)
.collect(Collectors.toList());

StringBuilder template = new StringBuilder();
Iterator<String> iterator = remaining.iterator();
while (iterator.hasNext()) {
template.append(iterator.next());
if (iterator.hasNext()) {
template.append(COLLECTION_DELIMITER);
}
}

return new QueryTemplate(template.toString(), name, remaining, charset, collectionFormat);
return new QueryTemplate(name, remaining, charset, collectionFormat);
}

/**
Expand All @@ -101,31 +95,43 @@ public static QueryTemplate append(QueryTemplate queryTemplate,
queryValues.addAll(StreamSupport.stream(values.spliterator(), false)
.filter(Util::isNotBlank)
.collect(Collectors.toList()));
return create(queryTemplate.getName(), queryValues, queryTemplate.getCharset(),
return create(queryTemplate.getName(), queryValues, StandardCharsets.UTF_8,
collectionFormat);
}

/**
* Create a new Query Template.
*
* @param template for the Query String.
* @param name of the query parameter.
* @param values for the parameter.
* @param collectionFormat to use.
*/
private QueryTemplate(
String template,
String name,
Iterable<String> values,
Charset charset,
CollectionFormat collectionFormat) {
super(template, ExpansionOptions.REQUIRED, EncodingOptions.REQUIRED, true, charset);
this.values = new CopyOnWriteArrayList<>();
this.name = new Template(name, ExpansionOptions.ALLOW_UNRESOLVED, EncodingOptions.REQUIRED,
false, charset);
this.collectionFormat = collectionFormat;
this.values = StreamSupport.stream(values.spliterator(), false)
.filter(Util::isNotBlank)
.collect(Collectors.toList());

/* parse each value into a template chunk for resolution later */
for (String value : values) {
if (value.isEmpty()) {
/* skip */
continue;
}

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

if (this.values.isEmpty()) {
/* in this case, we have a pure parameter */
this.pure = true;
Expand All @@ -134,7 +140,17 @@ private QueryTemplate(
}

public List<String> getValues() {
return 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() {
Expand All @@ -143,7 +159,7 @@ public String getName() {

@Override
public String toString() {
return this.queryString(this.name.toString(), super.toString());
return this.queryString(this.name.toString(), this.getValues());
}

/**
Expand All @@ -153,39 +169,43 @@ public String toString() {
* @param variables containing the values for expansion.
* @return the expanded template.
*/
@Override
public String expand(Map<String, ?> variables) {
String name = this.name.expand(variables);
return this.queryString(name, super.expand(variables));
}

@Override
protected String resolveExpression(Expression expression, Map<String, ?> variables) {
if (variables.containsKey(expression.getName())) {
if (variables.get(expression.getName()) == null) {
/* explicit undefined */
return UNDEF;
if (this.pure) {
return name;
}

List<String> expanded = new ArrayList<>();
for (Template template : this.values) {
String result = template.expand(variables);
if (result == null) {
continue;
}

/*
* check for an iterable result, and if one is there, we need to split it into individual
* values
*/
if (result.contains(",")) {
/* we need to split it */
expanded.addAll(Arrays.asList(result.split(",")));
} else {
expanded.add(result);
}
return super.resolveExpression(expression, variables);
}

/* mark the variable as undefined */
return UNDEF;
return this.queryString(name, Collections.unmodifiableList(expanded));
}

private String queryString(String name, String values) {

private String queryString(String name, List<String> values) {
if (this.pure) {
return name;
}

/* covert the comma separated values into a value query string */
List<String> resolved = Arrays.stream(values.split(COLLECTION_DELIMITER))
.filter(Objects::nonNull)
.filter(s -> !UNDEF.equalsIgnoreCase(s))
.collect(Collectors.toList());

if (!resolved.isEmpty()) {
return this.collectionFormat.join(name, resolved, this.getCharset()).toString();
if (!values.isEmpty()) {
return this.collectionFormat.join(name, values, StandardCharsets.UTF_8).toString();
}

/* nothing to return, all values are unresolved */
Expand Down

0 comments on commit a7b7c01

Please sign in to comment.