Skip to content

Commit 0e9ea2c

Browse files
poutsmapoutsma
committed
Fix review remarks on Servlet.fn
This commit incoporates the remarks made during the Servlet.fn review. See gh-21490
1 parent 2f682e1 commit 0e9ea2c

9 files changed

+163
-138
lines changed

‎spring-webmvc/src/main/java/org/springframework/web/servlet/function/DefaultEntityResponseBuilder.java

+63-55
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package org.springframework.web.servlet.function;
1818

1919
import java.io.IOException;
20+
import java.lang.reflect.Type;
2021
import java.net.URI;
2122
import java.time.Instant;
2223
import java.time.ZonedDateTime;
@@ -39,12 +40,14 @@
3940
import org.reactivestreams.Subscriber;
4041
import org.reactivestreams.Subscription;
4142

43+
import org.springframework.core.ParameterizedTypeReference;
4244
import org.springframework.http.CacheControl;
4345
import org.springframework.http.HttpHeaders;
4446
import org.springframework.http.HttpMethod;
4547
import org.springframework.http.HttpStatus;
4648
import org.springframework.http.InvalidMediaTypeException;
4749
import org.springframework.http.MediaType;
50+
import org.springframework.http.converter.GenericHttpMessageConverter;
4851
import org.springframework.http.converter.HttpMessageConverter;
4952
import org.springframework.http.server.ServletServerHttpResponse;
5053
import org.springframework.lang.Nullable;
@@ -64,7 +67,7 @@ class DefaultEntityResponseBuilder<T> implements EntityResponse.Builder<T> {
6467

6568
private final T entity;
6669

67-
private final BuilderFunction<T> builderFunction;
70+
private final Type entityType;
6871

6972
private int status = HttpStatus.OK.value();
7073

@@ -73,9 +76,9 @@ class DefaultEntityResponseBuilder<T> implements EntityResponse.Builder<T> {
7376
private final MultiValueMap<String, Cookie> cookies = new LinkedMultiValueMap<>();
7477

7578

76-
private DefaultEntityResponseBuilder(T entity, BuilderFunction<T> builderFunction) {
79+
private DefaultEntityResponseBuilder(T entity, @Nullable Type entityType) {
7780
this.entity = entity;
78-
this.builderFunction = builderFunction;
81+
this.entityType = (entityType != null) ? entityType : entity.getClass();
7982
}
8083

8184
@Override
@@ -185,52 +188,40 @@ public EntityResponse.Builder<T> varyBy(String... requestHeaders) {
185188
return this;
186189
}
187190

191+
@SuppressWarnings({"rawtypes", "unchecked"})
188192
@Override
189193
public EntityResponse<T> build() {
190-
return this.builderFunction.build(this.status, this.headers, this.cookies, this.entity);
194+
if (this.entity instanceof CompletionStage) {
195+
CompletionStage completionStage = (CompletionStage) this.entity;
196+
return new CompletionStageEntityResponse(this.status, this.headers, this.cookies,
197+
completionStage, this.entityType);
198+
}
199+
else if (this.entity instanceof Publisher) {
200+
Publisher publisher = (Publisher) this.entity;
201+
return new PublisherEntityResponse(this.status, this.headers, this.cookies, publisher,
202+
this.entityType);
203+
}
204+
else {
205+
return new DefaultEntityResponse<>(this.status, this.headers, this.cookies, this.entity,
206+
this.entityType);
207+
}
191208
}
192209

193210

194211
/**
195212
* Return a new {@link EntityResponse.Builder} from the given object.
196213
*/
197214
public static <T> EntityResponse.Builder<T> fromObject(T t) {
198-
return new DefaultEntityResponseBuilder<>(t, DefaultEntityResponse::new);
199-
}
200-
201-
/**
202-
* Return a new {@link EntityResponse.Builder} from the given completion stage.
203-
*/
204-
public static <T> EntityResponse.Builder<CompletionStage<T>> fromCompletionStage(
205-
CompletionStage<T> completionStage) {
206-
return new DefaultEntityResponseBuilder<>(completionStage,
207-
CompletionStageEntityResponse::new);
215+
return new DefaultEntityResponseBuilder<>(t, null);
208216
}
209217

210218
/**
211-
* Return a new {@link EntityResponse.Builder} from the given Reactive Streams publisher.
219+
* Return a new {@link EntityResponse.Builder} from the given object and type reference.
212220
*/
213-
public static <T> EntityResponse.Builder<Publisher<T>> fromPublisher(Publisher<T> publisher) {
214-
return new DefaultEntityResponseBuilder<>(publisher, PublisherEntityResponse::new);
221+
public static <T> EntityResponse.Builder<T> fromObject(T t, ParameterizedTypeReference<?> bodyType) {
222+
return new DefaultEntityResponseBuilder<>(t, bodyType.getType());
215223
}
216224

217-
@SuppressWarnings("unchecked")
218-
private static <T> HttpMessageConverter<T> cast(HttpMessageConverter<?> messageConverter) {
219-
return (HttpMessageConverter<T>) messageConverter;
220-
}
221-
222-
223-
/**
224-
* Defines contract for building {@link EntityResponse} instances.
225-
*/
226-
private interface BuilderFunction<T> {
227-
228-
EntityResponse<T> build(int statusCode, HttpHeaders headers,
229-
MultiValueMap<String, Cookie> cookies, T entity);
230-
231-
}
232-
233-
234225
/**
235226
* Default {@link EntityResponse} implementation for synchronous bodies.
236227
*/
@@ -240,12 +231,15 @@ private static class DefaultEntityResponse<T>
240231

241232
private final T entity;
242233

234+
private final Type entityType;
235+
243236

244237
public DefaultEntityResponse(int statusCode, HttpHeaders headers,
245-
MultiValueMap<String, Cookie> cookies, T entity) {
238+
MultiValueMap<String, Cookie> cookies, T entity, Type entityType) {
246239

247240
super(statusCode, headers, cookies);
248241
this.entity = entity;
242+
this.entityType = entityType;
249243
}
250244

251245
@Override
@@ -258,11 +252,12 @@ protected ModelAndView writeToInternal(HttpServletRequest servletRequest,
258252
HttpServletResponse servletResponse, Context context)
259253
throws ServletException, IOException {
260254

261-
writeEntityWithMessageConverters(this.entity, servletRequest, servletResponse, context);
255+
writeEntityWithMessageConverters(this.entity, servletRequest,servletResponse, context);
262256

263257
return null;
264258
}
265259

260+
@SuppressWarnings("unchecked")
266261
protected void writeEntityWithMessageConverters(Object entity,
267262
HttpServletRequest request, HttpServletResponse response,
268263
ServerResponse.Context context)
@@ -271,30 +266,39 @@ protected void writeEntityWithMessageConverters(Object entity,
271266
ServletServerHttpResponse serverResponse = new ServletServerHttpResponse(response);
272267

273268
MediaType contentType = getContentType(response);
274-
Class<?> entityType = entity.getClass();
275-
276-
HttpMessageConverter<Object> messageConverter = context.messageConverters().stream()
277-
.filter(converter -> converter.canWrite(entityType, contentType))
278-
.findFirst()
279-
.map(DefaultEntityResponseBuilder::cast)
280-
.orElseThrow(() -> new HttpMediaTypeNotAcceptableException(
281-
producibleMediaTypes(context.messageConverters(), entityType)));
269+
Class<?> entityClass = entity.getClass();
270+
271+
for (HttpMessageConverter<?> messageConverter : context.messageConverters()) {
272+
if (messageConverter instanceof GenericHttpMessageConverter<?>) {
273+
GenericHttpMessageConverter<Object> genericMessageConverter =
274+
(GenericHttpMessageConverter<Object>) messageConverter;
275+
if (genericMessageConverter.canWrite(this.entityType, entityClass, contentType)) {
276+
genericMessageConverter.write(entity, this.entityType, contentType, serverResponse);
277+
return;
278+
}
279+
}
280+
if (messageConverter.canWrite(entityClass, contentType)) {
281+
((HttpMessageConverter<Object>)messageConverter).write(entity, contentType, serverResponse);
282+
return;
283+
}
284+
}
282285

283-
messageConverter.write(entity, contentType, serverResponse);
286+
List<MediaType> producibleMediaTypes = producibleMediaTypes(context.messageConverters(), entityClass);
287+
throw new HttpMediaTypeNotAcceptableException(producibleMediaTypes);
284288
}
285289

286290
@Nullable
287-
private MediaType getContentType(HttpServletResponse response) {
291+
private static MediaType getContentType(HttpServletResponse response) {
288292
try {
289-
return MediaType.parseMediaType(response.getContentType());
293+
return MediaType.parseMediaType(response.getContentType()).removeQualityValue();
290294
}
291295
catch (InvalidMediaTypeException ex) {
292296
return null;
293297
}
294298
}
295299

296-
protected final void tryWriteEntityWithMessageConverters(Object entity,
297-
HttpServletRequest request, HttpServletResponse response,
300+
protected void tryWriteEntityWithMessageConverters(Object entity,
301+
HttpServletRequest request, HttpServletResponse response,
298302
ServerResponse.Context context) {
299303
try {
300304
writeEntityWithMessageConverters(entity, request, response, context);
@@ -323,10 +327,10 @@ private static List<MediaType> producibleMediaTypes(
323327
private static class CompletionStageEntityResponse<T>
324328
extends DefaultEntityResponse<CompletionStage<T>> {
325329

326-
public CompletionStageEntityResponse(int statusCode,
327-
HttpHeaders headers,
328-
MultiValueMap<String, Cookie> cookies, CompletionStage<T> entity) {
329-
super(statusCode, headers, cookies, entity);
330+
public CompletionStageEntityResponse(int statusCode, HttpHeaders headers,
331+
MultiValueMap<String, Cookie> cookies, CompletionStage<T> entity,
332+
Type entityType) {
333+
super(statusCode, headers, cookies, entity, entityType);
330334
}
331335

332336
@Override
@@ -338,6 +342,7 @@ protected ModelAndView writeToInternal(HttpServletRequest servletRequest,
338342
entity().whenComplete((entity, throwable) -> {
339343
try {
340344
if (entity != null) {
345+
341346
tryWriteEntityWithMessageConverters(entity,
342347
(HttpServletRequest) asyncContext.getRequest(),
343348
(HttpServletResponse) asyncContext.getResponse(),
@@ -358,8 +363,9 @@ else if (throwable != null) {
358363
private static class PublisherEntityResponse<T> extends DefaultEntityResponse<Publisher<T>> {
359364

360365
public PublisherEntityResponse(int statusCode, HttpHeaders headers,
361-
MultiValueMap<String, Cookie> cookies, Publisher<T> entity) {
362-
super(statusCode, headers, cookies, entity);
366+
MultiValueMap<String, Cookie> cookies, Publisher<T> entity,
367+
Type entityType) {
368+
super(statusCode, headers, cookies, entity, entityType);
363369
}
364370

365371
@Override
@@ -425,6 +431,8 @@ public void onError(Throwable t) {
425431
(HttpServletRequest) this.asyncContext.getRequest(),
426432
(HttpServletResponse) this.asyncContext.getResponse(),
427433
this.context);
434+
435+
this.asyncContext.complete();
428436
}
429437

430438
@Override

‎spring-webmvc/src/main/java/org/springframework/web/servlet/function/DefaultServerRequest.java

+27-15
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package org.springframework.web.servlet.function;
1818

1919
import java.io.IOException;
20+
import java.lang.reflect.ParameterizedType;
2021
import java.lang.reflect.Type;
2122
import java.net.InetSocketAddress;
2223
import java.net.URI;
@@ -48,7 +49,6 @@
4849
import org.springframework.http.converter.GenericHttpMessageConverter;
4950
import org.springframework.http.converter.HttpMessageConverter;
5051
import org.springframework.http.server.ServletServerHttpRequest;
51-
import org.springframework.lang.Nullable;
5252
import org.springframework.util.CollectionUtils;
5353
import org.springframework.util.LinkedMultiValueMap;
5454
import org.springframework.util.MultiValueMap;
@@ -152,35 +152,42 @@ public <T> T body(Class<T> bodyType) throws IOException, ServletException {
152152
@Override
153153
public <T> T body(ParameterizedTypeReference<T> bodyType) throws IOException, ServletException {
154154
Type type = bodyType.getType();
155-
Class<?> contextClass = null;
155+
return bodyInternal(type, bodyClass(type));
156+
}
157+
158+
static Class<?> bodyClass(Type type) {
156159
if (type instanceof Class) {
157-
contextClass = (Class<?>) type;
160+
return (Class<?>) type;
158161
}
159-
return bodyInternal(type, contextClass);
162+
if (type instanceof ParameterizedType) {
163+
ParameterizedType parameterizedType = (ParameterizedType) type;
164+
if (parameterizedType.getRawType() instanceof Class) {
165+
return (Class<?>) parameterizedType.getRawType();
166+
}
167+
}
168+
return Object.class;
160169
}
161170

162171
@SuppressWarnings("unchecked")
163-
private <T> T bodyInternal(Type type, @Nullable Class<?> contextClass)
172+
private <T> T bodyInternal(Type bodyType, Class<?> bodyClass)
164173
throws ServletException, IOException {
165174

166175
MediaType contentType =
167176
this.headers.contentType().orElse(MediaType.APPLICATION_OCTET_STREAM);
168177

169178
for (HttpMessageConverter<?> messageConverter : this.messageConverters) {
170-
if (messageConverter instanceof GenericHttpMessageConverter<?>) {
179+
if (messageConverter instanceof GenericHttpMessageConverter) {
171180
GenericHttpMessageConverter<T> genericMessageConverter =
172181
(GenericHttpMessageConverter<T>) messageConverter;
173-
if (genericMessageConverter.canRead(type, contextClass, contentType)) {
174-
return genericMessageConverter.read(type, contextClass, this.serverHttpRequest);
182+
if (genericMessageConverter.canRead(bodyType, bodyClass, contentType)) {
183+
return genericMessageConverter.read(bodyType, bodyClass, this.serverHttpRequest);
175184
}
176185
}
177-
else {
178-
if (messageConverter.canRead(contextClass, contentType)) {
179-
HttpMessageConverter<T> theConverter =
180-
(HttpMessageConverter<T>) messageConverter;
181-
Class<? extends T> clazz = (Class<? extends T>) contextClass;
182-
return theConverter.read(clazz, this.serverHttpRequest);
183-
}
186+
if (messageConverter.canRead(bodyClass, contentType)) {
187+
HttpMessageConverter<T> theConverter =
188+
(HttpMessageConverter<T>) messageConverter;
189+
Class<? extends T> clazz = (Class<? extends T>) bodyClass;
190+
return theConverter.read(clazz, this.serverHttpRequest);
184191
}
185192
}
186193
throw new HttpMediaTypeNotSupportedException(contentType, this.allSupportedMediaTypes);
@@ -196,6 +203,11 @@ public Map<String, Object> attributes() {
196203
return this.attributes;
197204
}
198205

206+
@Override
207+
public Optional<String> param(String name) {
208+
return Optional.ofNullable(servletRequest().getParameter(name));
209+
}
210+
199211
@Override
200212
public MultiValueMap<String, String> params() {
201213
return this.params;

‎spring-webmvc/src/main/java/org/springframework/web/servlet/function/DefaultServerRequestBuilder.java

+10-17
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@
4646
import org.springframework.http.MediaType;
4747
import org.springframework.http.converter.GenericHttpMessageConverter;
4848
import org.springframework.http.converter.HttpMessageConverter;
49-
import org.springframework.lang.Nullable;
5049
import org.springframework.util.Assert;
5150
import org.springframework.util.LinkedMultiValueMap;
5251
import org.springframework.util.MultiValueMap;
@@ -239,35 +238,29 @@ public <T> T body(Class<T> bodyType) throws IOException, ServletException {
239238
@Override
240239
public <T> T body(ParameterizedTypeReference<T> bodyType) throws IOException, ServletException {
241240
Type type = bodyType.getType();
242-
Class<?> contextClass = null;
243-
if (type instanceof Class) {
244-
contextClass = (Class<?>) type;
245-
}
246-
return bodyInternal(type, contextClass);
241+
return bodyInternal(type, DefaultServerRequest.bodyClass(type));
247242
}
248243

249244
@SuppressWarnings("unchecked")
250-
private <T> T bodyInternal(Type type, @Nullable Class<?> contextClass)
245+
private <T> T bodyInternal(Type bodyType, Class<?> bodyClass)
251246
throws ServletException, IOException {
252247

253248
HttpInputMessage inputMessage = new BuiltInputMessage();
254249
MediaType contentType = headers().contentType().orElse(MediaType.APPLICATION_OCTET_STREAM);
255250

256251
for (HttpMessageConverter<?> messageConverter : this.messageConverters) {
257-
if (messageConverter instanceof GenericHttpMessageConverter<?>) {
252+
if (messageConverter instanceof GenericHttpMessageConverter) {
258253
GenericHttpMessageConverter<T> genericMessageConverter =
259254
(GenericHttpMessageConverter<T>) messageConverter;
260-
if (genericMessageConverter.canRead(type, contextClass, contentType)) {
261-
return genericMessageConverter.read(type, contextClass, inputMessage);
255+
if (genericMessageConverter.canRead(bodyType, bodyClass, contentType)) {
256+
return genericMessageConverter.read(bodyType, bodyClass, inputMessage);
262257
}
263258
}
264-
else {
265-
if (messageConverter.canRead(contextClass, contentType)) {
266-
HttpMessageConverter<T> theConverter =
267-
(HttpMessageConverter<T>) messageConverter;
268-
Class<? extends T> clazz = (Class<? extends T>) contextClass;
269-
return theConverter.read(clazz, inputMessage);
270-
}
259+
if (messageConverter.canRead(bodyClass, contentType)) {
260+
HttpMessageConverter<T> theConverter =
261+
(HttpMessageConverter<T>) messageConverter;
262+
Class<? extends T> clazz = (Class<? extends T>) bodyClass;
263+
return theConverter.read(clazz, inputMessage);
271264
}
272265
}
273266
throw new HttpMediaTypeNotSupportedException(contentType, Collections.emptyList());

0 commit comments

Comments
 (0)