Skip to content

Commit a7b7c01

Browse files
kdavisk6kdavisk6
authored
Ensure Iterable values are encoded before template expansion (OpenFeign#1138)
* Ensure Iterable values are encoded before template expansion Fixes OpenFeign#1123, Fixes OpenFeign#1133, Fixes OpenFeign#1102, Fixes OpenFeign#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.
1 parent 033de93 commit a7b7c01

10 files changed

Lines changed: 231 additions & 326 deletions

File tree

‎core/src/main/java/feign/CollectionFormat.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,21 +74,21 @@ public CharSequence join(String field, Collection<String> values, Charset charse
7474
if (separator == null) {
7575
// exploded
7676
builder.append(valueCount++ == 0 ? "" : "&");
77-
builder.append(UriUtils.queryEncode(field, charset));
77+
builder.append(UriUtils.encode(field, charset));
7878
if (value != null) {
7979
builder.append('=');
80-
builder.append(UriUtils.queryEncode(value, charset));
80+
builder.append(UriUtils.encode(value, charset));
8181
}
8282
} else {
8383
// delimited with a separator character
8484
if (builder.length() == 0) {
85-
builder.append(UriUtils.queryEncode(field, charset));
85+
builder.append(UriUtils.encode(field, charset));
8686
}
8787
if (value == null) {
8888
continue;
8989
}
9090
builder.append(valueCount++ == 0 ? "=" : separator);
91-
builder.append(UriUtils.queryEncode(value, charset));
91+
builder.append(UriUtils.encode(value, charset));
9292
}
9393
}
9494
return builder;

‎core/src/main/java/feign/RequestTemplate.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,10 @@ public RequestTemplate resolve(Map<String, ?> variables) {
164164
this.uriTemplate = UriTemplate.create("", !this.decodeSlash, this.charset);
165165
}
166166

167-
uri.append(this.uriTemplate.expand(variables));
167+
String expanded = this.uriTemplate.expand(variables);
168+
if (expanded != null) {
169+
uri.append(expanded);
170+
}
168171

169172
/*
170173
* for simplicity, combine the queries into the uri and use the resulting uri to seed the

‎core/src/main/java/feign/template/Expression.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@
1313
*/
1414
package feign.template;
1515

16+
import feign.CollectionFormat;
1617
import java.util.Optional;
17-
import java.util.regex.Matcher;
1818
import java.util.regex.Pattern;
1919

2020
/**

‎core/src/main/java/feign/template/Expressions.java‎

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,13 @@
1313
*/
1414
package feign.template;
1515

16-
import feign.Util;
17-
import feign.template.UriUtils.FragmentType;
18-
import java.util.ArrayList;
1916
import java.util.LinkedHashMap;
20-
import java.util.List;
2117
import java.util.Map;
2218
import java.util.Map.Entry;
2319
import java.util.Optional;
2420
import java.util.regex.Matcher;
2521
import java.util.regex.Pattern;
22+
import feign.Util;
2623

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

46-
public static Expression create(final String value, final FragmentType type) {
43+
public static Expression create(final String value) {
4744

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

89-
return new SimpleExpression(variableName, variablePattern, type);
86+
return new SimpleExpression(variableName, variablePattern);
9087
}
9188

9289
private static String stripBraces(String expression) {
@@ -105,26 +102,19 @@ private static String stripBraces(String expression) {
105102
*/
106103
static class SimpleExpression extends Expression {
107104

108-
private final FragmentType type;
109-
110-
SimpleExpression(String expression, String pattern, FragmentType type) {
105+
SimpleExpression(String expression, String pattern) {
111106
super(expression, pattern);
112-
this.type = type;
113107
}
114108

115109
String encode(Object value) {
116-
return UriUtils.encodeReserved(value.toString(), type, Util.UTF_8);
110+
return UriUtils.encode(value.toString(), Util.UTF_8);
117111
}
118112

119113
@Override
120114
String expand(Object variable, boolean encode) {
121115
StringBuilder expanded = new StringBuilder();
122116
if (Iterable.class.isAssignableFrom(variable.getClass())) {
123-
List<String> items = new ArrayList<>();
124-
for (Object item : ((Iterable) variable)) {
125-
items.add((encode) ? encode(item) : item.toString());
126-
}
127-
expanded.append(String.join(Template.COLLECTION_DELIMITER, items));
117+
expanded.append(this.expandIterable((Iterable<?>) variable));
128118
} else {
129119
expanded.append((encode) ? encode(variable) : variable);
130120
}
@@ -137,5 +127,38 @@ String expand(Object variable, boolean encode) {
137127
}
138128
return result;
139129
}
130+
131+
132+
private String expandIterable(Iterable<?> values) {
133+
StringBuilder result = new StringBuilder();
134+
for (Object value : values) {
135+
if (value == null) {
136+
/* skip */
137+
continue;
138+
}
139+
140+
/* expand the value */
141+
String expanded = this.encode(value);
142+
if (expanded.isEmpty()) {
143+
/* always append the separator */
144+
result.append(",");
145+
} else {
146+
if (result.length() != 0) {
147+
if (!result.toString().equalsIgnoreCase(",")) {
148+
result.append(",");
149+
}
150+
}
151+
result.append(expanded);
152+
}
153+
}
154+
155+
if (result.length() == 0) {
156+
/* completely unresolved */
157+
return null;
158+
}
159+
160+
/* return the expanded value */
161+
return result.toString();
162+
}
140163
}
141164
}

‎core/src/main/java/feign/template/QueryTemplate.java‎

Lines changed: 66 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -13,26 +13,29 @@
1313
*/
1414
package feign.template;
1515

16-
import feign.CollectionFormat;
17-
import feign.Util;
1816
import java.nio.charset.Charset;
17+
import java.nio.charset.StandardCharsets;
1918
import java.util.ArrayList;
2019
import java.util.Arrays;
2120
import java.util.Collection;
22-
import java.util.Iterator;
21+
import java.util.Collections;
2322
import java.util.List;
2423
import java.util.Map;
25-
import java.util.Objects;
24+
import java.util.concurrent.CopyOnWriteArrayList;
2625
import java.util.stream.Collectors;
2726
import java.util.stream.StreamSupport;
27+
import feign.CollectionFormat;
28+
import feign.Util;
29+
import feign.template.Template.EncodingOptions;
30+
import feign.template.Template.ExpansionOptions;
2831

2932
/**
3033
* Template for a Query String parameter.
3134
*/
32-
public final class QueryTemplate extends Template {
35+
public final class QueryTemplate {
3336

3437
private static final String UNDEF = "undef";
35-
private List<String> values;
38+
private List<Template> values;
3639
private final Template name;
3740
private final CollectionFormat collectionFormat;
3841
private boolean pure = false;
@@ -75,16 +78,7 @@ public static QueryTemplate create(String name,
7578
.filter(Util::isNotBlank)
7679
.collect(Collectors.toList());
7780

78-
StringBuilder template = new StringBuilder();
79-
Iterator<String> iterator = remaining.iterator();
80-
while (iterator.hasNext()) {
81-
template.append(iterator.next());
82-
if (iterator.hasNext()) {
83-
template.append(COLLECTION_DELIMITER);
84-
}
85-
}
86-
87-
return new QueryTemplate(template.toString(), name, remaining, charset, collectionFormat);
81+
return new QueryTemplate(name, remaining, charset, collectionFormat);
8882
}
8983

9084
/**
@@ -101,31 +95,43 @@ public static QueryTemplate append(QueryTemplate queryTemplate,
10195
queryValues.addAll(StreamSupport.stream(values.spliterator(), false)
10296
.filter(Util::isNotBlank)
10397
.collect(Collectors.toList()));
104-
return create(queryTemplate.getName(), queryValues, queryTemplate.getCharset(),
98+
return create(queryTemplate.getName(), queryValues, StandardCharsets.UTF_8,
10599
collectionFormat);
106100
}
107101

108102
/**
109103
* Create a new Query Template.
110104
*
111-
* @param template for the Query String.
112105
* @param name of the query parameter.
113106
* @param values for the parameter.
114107
* @param collectionFormat to use.
115108
*/
116109
private QueryTemplate(
117-
String template,
118110
String name,
119111
Iterable<String> values,
120112
Charset charset,
121113
CollectionFormat collectionFormat) {
122-
super(template, ExpansionOptions.REQUIRED, EncodingOptions.REQUIRED, true, charset);
114+
this.values = new CopyOnWriteArrayList<>();
123115
this.name = new Template(name, ExpansionOptions.ALLOW_UNRESOLVED, EncodingOptions.REQUIRED,
124116
false, charset);
125117
this.collectionFormat = collectionFormat;
126-
this.values = StreamSupport.stream(values.spliterator(), false)
127-
.filter(Util::isNotBlank)
128-
.collect(Collectors.toList());
118+
119+
/* parse each value into a template chunk for resolution later */
120+
for (String value : values) {
121+
if (value.isEmpty()) {
122+
/* skip */
123+
continue;
124+
}
125+
126+
this.values.add(
127+
new Template(
128+
value,
129+
ExpansionOptions.REQUIRED,
130+
EncodingOptions.REQUIRED,
131+
false,
132+
charset));
133+
}
134+
129135
if (this.values.isEmpty()) {
130136
/* in this case, we have a pure parameter */
131137
this.pure = true;
@@ -134,7 +140,17 @@ private QueryTemplate(
134140
}
135141

136142
public List<String> getValues() {
137-
return values;
143+
return Collections.unmodifiableList(this.values.stream()
144+
.map(Template::toString)
145+
.collect(Collectors.toList()));
146+
}
147+
148+
public List<String> getVariables() {
149+
List<String> variables = new ArrayList<>();
150+
for (Template template : this.values) {
151+
variables.addAll(template.getVariables());
152+
}
153+
return Collections.unmodifiableList(variables);
138154
}
139155

140156
public String getName() {
@@ -143,7 +159,7 @@ public String getName() {
143159

144160
@Override
145161
public String toString() {
146-
return this.queryString(this.name.toString(), super.toString());
162+
return this.queryString(this.name.toString(), this.getValues());
147163
}
148164

149165
/**
@@ -153,39 +169,43 @@ public String toString() {
153169
* @param variables containing the values for expansion.
154170
* @return the expanded template.
155171
*/
156-
@Override
157172
public String expand(Map<String, ?> variables) {
158173
String name = this.name.expand(variables);
159-
return this.queryString(name, super.expand(variables));
160-
}
161174

162-
@Override
163-
protected String resolveExpression(Expression expression, Map<String, ?> variables) {
164-
if (variables.containsKey(expression.getName())) {
165-
if (variables.get(expression.getName()) == null) {
166-
/* explicit undefined */
167-
return UNDEF;
175+
if (this.pure) {
176+
return name;
177+
}
178+
179+
List<String> expanded = new ArrayList<>();
180+
for (Template template : this.values) {
181+
String result = template.expand(variables);
182+
if (result == null) {
183+
continue;
184+
}
185+
186+
/*
187+
* check for an iterable result, and if one is there, we need to split it into individual
188+
* values
189+
*/
190+
if (result.contains(",")) {
191+
/* we need to split it */
192+
expanded.addAll(Arrays.asList(result.split(",")));
193+
} else {
194+
expanded.add(result);
168195
}
169-
return super.resolveExpression(expression, variables);
170196
}
171197

172-
/* mark the variable as undefined */
173-
return UNDEF;
198+
return this.queryString(name, Collections.unmodifiableList(expanded));
174199
}
175200

176-
private String queryString(String name, String values) {
201+
202+
private String queryString(String name, List<String> values) {
177203
if (this.pure) {
178204
return name;
179205
}
180206

181-
/* covert the comma separated values into a value query string */
182-
List<String> resolved = Arrays.stream(values.split(COLLECTION_DELIMITER))
183-
.filter(Objects::nonNull)
184-
.filter(s -> !UNDEF.equalsIgnoreCase(s))
185-
.collect(Collectors.toList());
186-
187-
if (!resolved.isEmpty()) {
188-
return this.collectionFormat.join(name, resolved, this.getCharset()).toString();
207+
if (!values.isEmpty()) {
208+
return this.collectionFormat.join(name, values, StandardCharsets.UTF_8).toString();
189209
}
190210

191211
/* nothing to return, all values are unresolved */

0 commit comments

Comments
 (0)