Skip to content

Commit 4da6d5c

Browse files
velovelo
authored
NPE when resolving a template with binary body (OpenFeign#821)
* NPE when resolving a template with binary body * Initial change to introduce body object to Request's
1 parent 4b2a48e commit 4da6d5c

8 files changed

Lines changed: 149 additions & 54 deletions

File tree

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

Lines changed: 89 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,73 @@
1717
import static feign.Util.valuesOrEmpty;
1818
import java.net.HttpURLConnection;
1919
import java.nio.charset.Charset;
20-
import java.util.Collection;
21-
import java.util.Map;
20+
import java.util.*;
21+
import feign.template.BodyTemplate;
2222

2323
/**
2424
* An immutable request to an http server.
2525
*/
2626
public final class Request {
2727

28+
public static class Body {
29+
30+
private final byte[] data;
31+
private final Charset encoding;
32+
private final BodyTemplate bodyTemplate;
33+
34+
private Body(byte[] data, Charset encoding, BodyTemplate bodyTemplate) {
35+
super();
36+
this.data = data;
37+
this.encoding = encoding;
38+
this.bodyTemplate = bodyTemplate;
39+
}
40+
41+
public Request.Body expand(Map<String, ?> variables) {
42+
if (bodyTemplate == null)
43+
return this;
44+
45+
return encoded(bodyTemplate.expand(variables).getBytes(encoding), encoding);
46+
}
47+
48+
public List<String> getVariables() {
49+
if (bodyTemplate == null)
50+
return Collections.emptyList();
51+
return bodyTemplate.getVariables();
52+
}
53+
54+
public static Request.Body encoded(byte[] bodyData, Charset encoding) {
55+
return new Request.Body(bodyData, encoding, null);
56+
}
57+
58+
public int length() {
59+
/* calculate the content length based on the data provided */
60+
return data != null ? data.length : 0;
61+
}
62+
63+
public byte[] asBytes() {
64+
return data;
65+
}
66+
67+
public static Request.Body bodyTemplate(String bodyTemplate, Charset encoding) {
68+
return new Request.Body(null, encoding, BodyTemplate.create(bodyTemplate));
69+
}
70+
71+
public String bodyTemplate() {
72+
return (bodyTemplate != null) ? bodyTemplate.toString() : null;
73+
}
74+
75+
public String asString() {
76+
return encoding != null && data != null
77+
? new String(data, encoding)
78+
: "Binary data";
79+
}
80+
81+
public static Body empty() {
82+
return new Request.Body(null, null, null);
83+
}
84+
85+
}
86+
2887
public enum HttpMethod {
2988
GET, HEAD, POST, PUT, DELETE, CONNECT, OPTIONS, TRACE, PATCH
3089
}
@@ -60,23 +119,35 @@ public static Request create(HttpMethod httpMethod,
60119
Map<String, Collection<String>> headers,
61120
byte[] body,
62121
Charset charset) {
63-
return new Request(httpMethod, url, headers, body, charset);
122+
return create(httpMethod, url, headers, Body.encoded(body, charset));
123+
}
64124

125+
/**
126+
* Builds a Request. All parameters must be effectively immutable, via safe copies.
127+
*
128+
* @param httpMethod for the request.
129+
* @param url for the request.
130+
* @param headers to include.
131+
* @param body of the request, can be {@literal null}
132+
* @return a Request
133+
*/
134+
public static Request create(HttpMethod httpMethod,
135+
String url,
136+
Map<String, Collection<String>> headers,
137+
Body body) {
138+
return new Request(httpMethod, url, headers, body);
65139
}
66140

67141
private final HttpMethod httpMethod;
68142
private final String url;
69143
private final Map<String, Collection<String>> headers;
70-
private final byte[] body;
71-
private final Charset charset;
144+
private final Body body;
72145

73-
Request(HttpMethod method, String url, Map<String, Collection<String>> headers, byte[] body,
74-
Charset charset) {
146+
Request(HttpMethod method, String url, Map<String, Collection<String>> headers, Body body) {
75147
this.httpMethod = checkNotNull(method, "httpMethod of %s", method.name());
76148
this.url = checkNotNull(url, "url");
77149
this.headers = checkNotNull(headers, "headers of %s %s", method, url);
78-
this.body = body; // nullable
79-
this.charset = charset; // nullable
150+
this.body = body;
80151
}
81152

82153
/**
@@ -112,18 +183,25 @@ public Map<String, Collection<String>> headers() {
112183
* The character set with which the body is encoded, or null if unknown or not applicable. When
113184
* this is present, you can use {@code new String(req.body(), req.charset())} to access the body
114185
* as a String.
186+
*
187+
* @deprecated use {@link #requestBody()} instead
115188
*/
116189
public Charset charset() {
117-
return charset;
190+
return body.encoding;
118191
}
119192

120193
/**
121194
* If present, this is the replayable body to send to the server. In some cases, this may be
122195
* interpretable as text.
123196
*
124197
* @see #charset()
198+
* @deprecated use {@link #requestBody()} instead
125199
*/
126200
public byte[] body() {
201+
return body.data;
202+
}
203+
204+
public Body requestBody() {
127205
return body;
128206
}
129207

@@ -137,7 +215,7 @@ public String toString() {
137215
}
138216
}
139217
if (body != null) {
140-
builder.append('\n').append(charset != null ? new String(body, charset) : "Binary data");
218+
builder.append('\n').append(body.asString());
141219
}
142220
return builder.toString();
143221
}

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

Lines changed: 41 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,9 @@ public final class RequestTemplate implements Serializable {
5555
private String target;
5656
private boolean resolved = false;
5757
private UriTemplate uriTemplate;
58-
private BodyTemplate bodyTemplate;
5958
private HttpMethod method;
6059
private transient Charset charset = Util.UTF_8;
61-
private byte[] body;
60+
private Request.Body body = Request.Body.empty();
6261
private boolean decodeSlash = true;
6362
private CollectionFormat collectionFormat = CollectionFormat.EXPLODED;
6463

@@ -83,15 +82,13 @@ public RequestTemplate() {
8382
*/
8483
private RequestTemplate(String target,
8584
UriTemplate uriTemplate,
86-
BodyTemplate bodyTemplate,
8785
HttpMethod method,
8886
Charset charset,
89-
byte[] body,
87+
Request.Body body,
9088
boolean decodeSlash,
9189
CollectionFormat collectionFormat) {
9290
this.target = target;
9391
this.uriTemplate = uriTemplate;
94-
this.bodyTemplate = bodyTemplate;
9592
this.method = method;
9693
this.charset = charset;
9794
this.body = body;
@@ -109,7 +106,7 @@ private RequestTemplate(String target,
109106
public static RequestTemplate from(RequestTemplate requestTemplate) {
110107
RequestTemplate template =
111108
new RequestTemplate(requestTemplate.target, requestTemplate.uriTemplate,
112-
requestTemplate.bodyTemplate, requestTemplate.method, requestTemplate.charset,
109+
requestTemplate.method, requestTemplate.charset,
113110
requestTemplate.body, requestTemplate.decodeSlash, requestTemplate.collectionFormat);
114111

115112
if (!requestTemplate.queries().isEmpty()) {
@@ -137,7 +134,6 @@ public RequestTemplate(RequestTemplate toCopy) {
137134
this.headers.putAll(toCopy.headers);
138135
this.charset = toCopy.charset;
139136
this.body = toCopy.body;
140-
this.bodyTemplate = toCopy.bodyTemplate;
141137
this.decodeSlash = toCopy.decodeSlash;
142138
this.collectionFormat =
143139
(toCopy.collectionFormat != null) ? toCopy.collectionFormat : CollectionFormat.EXPLODED;
@@ -225,9 +221,7 @@ public RequestTemplate resolve(Map<String, ?> variables) {
225221
}
226222
}
227223

228-
if (this.bodyTemplate != null) {
229-
resolved.body(this.bodyTemplate.expand(variables));
230-
}
224+
resolved.body(this.body.expand(variables));
231225

232226
/* mark the new template resolved */
233227
resolved.resolved = true;
@@ -261,7 +255,7 @@ public Request request() {
261255
if (!this.resolved) {
262256
throw new IllegalStateException("template has not been resolved.");
263257
}
264-
return Request.create(this.method, this.url(), this.headers(), this.body(), this.charset);
258+
return Request.create(this.method, this.url(), this.headers(), this.requestBody());
265259
}
266260

267261
/**
@@ -541,9 +535,7 @@ public List<String> variables() {
541535
}
542536

543537
/* body */
544-
if (this.bodyTemplate != null) {
545-
variables.addAll(this.bodyTemplate.getVariables());
546-
}
538+
variables.addAll(this.body.getVariables());
547539

548540
return variables;
549541
}
@@ -717,20 +709,11 @@ public Map<String, Collection<String>> headers() {
717709
* @param bodyData to send, can be null.
718710
* @param charset of the encoded data.
719711
* @return a RequestTemplate for chaining.
712+
* @deprecated use {@link RequestTemplate#body(feign.Request.Body)} instead
720713
*/
714+
@Deprecated
721715
public RequestTemplate body(byte[] bodyData, Charset charset) {
722-
723-
/*
724-
* since the body is being set directly, we need to clear out any existing body template
725-
* information to prevent unintended side effects.
726-
*/
727-
this.bodyTemplate = null;
728-
this.charset = charset;
729-
this.body = bodyData;
730-
731-
/* calculate the content length based on the data provided */
732-
int bodyLength = bodyData != null ? bodyData.length : 0;
733-
header(CONTENT_LENGTH, String.valueOf(bodyLength));
716+
this.body(Request.Body.encoded(bodyData, charset));
734717

735718
return this;
736719
}
@@ -740,28 +723,49 @@ public RequestTemplate body(byte[] bodyData, Charset charset) {
740723
*
741724
* @param bodyText to send.
742725
* @return a RequestTemplate for chaining.
726+
* @deprecated use {@link RequestTemplate#body(feign.Request.Body)} instead
743727
*/
728+
@Deprecated
744729
public RequestTemplate body(String bodyText) {
745730
byte[] bodyData = bodyText != null ? bodyText.getBytes(UTF_8) : null;
746731
return body(bodyData, UTF_8);
747732
}
748733

734+
/**
735+
* Set the Body for this request.
736+
*
737+
* @param body to send.
738+
* @return a RequestTemplate for chaining.
739+
*/
740+
public RequestTemplate body(Request.Body body) {
741+
this.body = body;
742+
743+
header(CONTENT_LENGTH);
744+
if (body.length() > 0) {
745+
header(CONTENT_LENGTH, String.valueOf(body.length()));
746+
}
747+
748+
return this;
749+
}
750+
749751
/**
750752
* Charset of the Request Body, if known.
751753
*
752754
* @return the currently applied Charset.
753755
*/
754-
public Charset charset() {
756+
public Charset requestCharset() {
755757
return charset;
756758
}
757759

758760
/**
759761
* The Request Body.
760762
*
761763
* @return the request body.
764+
* @deprecated replaced by {@link RequestTemplate#requestBody()}
762765
*/
766+
@Deprecated
763767
public byte[] body() {
764-
return body;
768+
return body.asBytes();
765769
}
766770

767771

@@ -770,11 +774,11 @@ public byte[] body() {
770774
*
771775
* @param bodyTemplate to use.
772776
* @return a RequestTemplate for chaining.
777+
* @deprecated replaced by {@link RequestTemplate#body(feign.Request.Body)}
773778
*/
779+
@Deprecated
774780
public RequestTemplate bodyTemplate(String bodyTemplate) {
775-
this.bodyTemplate = BodyTemplate.create(bodyTemplate);
776-
this.charset = Util.UTF_8;
777-
this.body = null;
781+
this.body(Request.Body.bodyTemplate(bodyTemplate, Util.UTF_8));
778782
return this;
779783
}
780784

@@ -784,7 +788,7 @@ public RequestTemplate bodyTemplate(String bodyTemplate) {
784788
* @return the unresolved body template.
785789
*/
786790
public String bodyTemplate() {
787-
return (bodyTemplate != null) ? bodyTemplate.toString() : null;
791+
return body.bodyTemplate();
788792
}
789793

790794
@Override
@@ -884,6 +888,10 @@ private SimpleImmutableEntry<String, String> splitQueryParameter(String pair) {
884888
return new SimpleImmutableEntry<>(name, value);
885889
}
886890

891+
public Request.Body requestBody() {
892+
return this.body;
893+
}
894+
887895
/**
888896
* Factory for creating RequestTemplate.
889897
*/
@@ -894,4 +902,5 @@ interface Factory {
894902
*/
895903
RequestTemplate create(Object[] argv);
896904
}
905+
897906
}

‎core/src/test/java/feign/RequestTemplateTest.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,18 @@ public void resolveTemplateWithParameterizedPathSkipsEncodingSlash() {
9696
.hasUrl("/hostedzone/Z1PA6795UKMFR9");
9797
}
9898

99+
@Test
100+
public void resolveTemplateWithBinaryBody() {
101+
RequestTemplate template = new RequestTemplate().method(HttpMethod.GET)
102+
.uri("{zoneId}")
103+
.body(new byte[] {7, 3, -3, -7}, null);
104+
105+
template = template.resolve(mapOf("zoneId", "/hostedzone/Z1PA6795UKMFR9"));
106+
107+
assertThat(template)
108+
.hasUrl("/hostedzone/Z1PA6795UKMFR9");
109+
}
110+
99111
@Test
100112
public void canInsertAbsoluteHref() {
101113
RequestTemplate template = new RequestTemplate().method(HttpMethod.GET)

‎jaxb/src/test/java/feign/jaxb/examples/AWSSignatureVersion4.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,7 @@ private static String canonicalString(RequestTemplate input, String host) {
7575
canonicalRequest.append("host").append('\n');
7676

7777
// HexEncode(Hash(Payload))
78-
String bodyText =
79-
input.charset() != null && input.body() != null ? new String(input.body(), input.charset())
80-
: null;
78+
String bodyText = input.requestBody().asString();
8179
if (bodyText != null) {
8280
canonicalRequest.append(hex(sha256(bodyText)));
8381
} else {

‎mock/src/main/java/feign/mock/MockClient.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ private Response.Builder executeSequential(RequestKey requestKey) {
8383

8484
RequestResponse expectedRequestResponse = responseIterator.next();
8585
if (!expectedRequestResponse.requestKey.equalsExtended(requestKey)) {
86-
throw new VerificationAssertionError("Expected %s, but was %s",
86+
throw new VerificationAssertionError("Expected: \n%s,\nbut was: \n%s",
8787
expectedRequestResponse.requestKey,
8888
requestKey);
8989
}

‎mock/src/test/java/feign/mock/MockClientSequentialTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ public void sequentialRequestsInWrongOrder() throws Exception {
167167
githubSequential.contributors("7 7", "netflix", "feign");
168168
fail();
169169
} catch (VerificationAssertionError e) {
170-
assertThat(e.getMessage(), startsWith("Expected Request ["));
170+
assertThat(e.getMessage(), startsWith("Expected: \nRequest ["));
171171
}
172172
}
173173

0 commit comments

Comments
 (0)