Skip to content

Commit 4bddbd3

Browse files
rstoyanchevrstoyanchev
committed
Polishing contribution
See gh-28386
1 parent c2a008f commit 4bddbd3

File tree

7 files changed

+290
-679
lines changed

7 files changed

+290
-679
lines changed

‎spring-web/src/main/java/org/springframework/web/service/invoker/HttpMethodArgumentResolver.java

+11-8
Original file line numberDiff line numberDiff line change
@@ -23,29 +23,32 @@
2323
import org.springframework.http.HttpMethod;
2424
import org.springframework.lang.Nullable;
2525

26+
2627
/**
27-
* An implementation of {@link HttpServiceMethodArgumentResolver} that resolves
28-
* request HTTP method based on argument type. Arguments of type
29-
* {@link HttpMethod} will be used to determine the method.
28+
* {@link HttpServiceMethodArgumentResolver} that resolves the target
29+
* request's HTTP method from an {@link HttpMethod} argument.
3030
*
3131
* @author Olga Maciaszek-Sharma
3232
* @since 6.0
3333
*/
3434
public class HttpMethodArgumentResolver implements HttpServiceMethodArgumentResolver {
3535

36-
private static final Log LOG = LogFactory.getLog(HttpMethodArgumentResolver.class);
36+
private static final Log logger = LogFactory.getLog(HttpMethodArgumentResolver.class);
37+
3738

3839
@Override
39-
public void resolve(@Nullable Object argument, MethodParameter parameter,
40-
HttpRequestDefinition requestDefinition) {
40+
public void resolve(
41+
@Nullable Object argument, MethodParameter parameter, HttpRequestDefinition requestDefinition) {
42+
4143
if (argument == null) {
4244
return;
4345
}
4446
if (argument instanceof HttpMethod httpMethod) {
45-
if (LOG.isTraceEnabled()) {
46-
LOG.trace("Resolved HTTP method to: " + httpMethod.name());
47+
if (logger.isTraceEnabled()) {
48+
logger.trace("Resolved HTTP method to: " + httpMethod.name());
4749
}
4850
requestDefinition.setHttpMethod(httpMethod);
4951
}
5052
}
53+
5154
}

‎spring-web/src/main/java/org/springframework/web/service/invoker/PathVariableArgumentResolver.java

+37-85
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,13 @@
2323
import org.apache.commons.logging.LogFactory;
2424

2525
import org.springframework.core.MethodParameter;
26-
import org.springframework.core.ReactiveAdapter;
27-
import org.springframework.core.ReactiveAdapterRegistry;
2826
import org.springframework.core.convert.ConversionService;
29-
import org.springframework.core.convert.TypeDescriptor;
3027
import org.springframework.lang.Nullable;
28+
import org.springframework.util.Assert;
3129
import org.springframework.util.StringUtils;
3230
import org.springframework.web.bind.annotation.PathVariable;
3331

32+
3433
/**
3534
* An implementation of {@link HttpServiceMethodArgumentResolver} that resolves
3635
* request path variables based on method arguments annotated
@@ -42,111 +41,64 @@
4241
*/
4342
public class PathVariableArgumentResolver implements HttpServiceMethodArgumentResolver {
4443

45-
private static final Log LOG = LogFactory.getLog(PathVariableArgumentResolver.class);
46-
private static final TypeDescriptor STRING_TYPE_DESCRIPTOR = TypeDescriptor.valueOf(String.class);
47-
@Nullable
44+
private static final Log logger = LogFactory.getLog(PathVariableArgumentResolver.class);
45+
46+
4847
private final ConversionService conversionService;
4948

50-
public PathVariableArgumentResolver(@Nullable ConversionService conversionService) {
49+
50+
public PathVariableArgumentResolver(ConversionService conversionService) {
51+
Assert.notNull(conversionService, "ConversionService is required");
5152
this.conversionService = conversionService;
5253
}
5354

55+
56+
@SuppressWarnings("unchecked")
5457
@Override
55-
public void resolve(@Nullable Object argument, MethodParameter parameter,
56-
HttpRequestDefinition requestDefinition) {
58+
public void resolve(
59+
@Nullable Object argument, MethodParameter parameter, HttpRequestDefinition requestDefinition) {
60+
5761
PathVariable annotation = parameter.getParameterAnnotation(PathVariable.class);
5862
if (annotation == null) {
5963
return;
6064
}
61-
String resolvedAnnotationName = StringUtils.hasText(annotation.value())
62-
? annotation.value() : annotation.name();
63-
boolean required = annotation.required();
64-
Object resolvedArgument = resolveFromOptional(argument);
65-
if (resolvedArgument instanceof Map<?, ?> valueMap) {
66-
if (StringUtils.hasText(resolvedAnnotationName)) {
67-
Object value = valueMap.get(resolvedAnnotationName);
68-
Object resolvedValue = resolveFromOptional(value);
69-
addUriParameter(requestDefinition, resolvedAnnotationName, resolvedValue, required);
70-
return;
71-
}
72-
valueMap.entrySet()
73-
.forEach(entry -> addUriParameter(requestDefinition, entry, required));
74-
return;
75-
}
76-
String name = StringUtils.hasText(resolvedAnnotationName)
77-
? resolvedAnnotationName : parameter.getParameterName();
78-
addUriParameter(requestDefinition, name, resolvedArgument, required);
79-
}
8065

81-
private void addUriParameter(HttpRequestDefinition requestDefinition, @Nullable String name,
82-
@Nullable Object value, boolean required) {
83-
if (name == null) {
84-
throw new IllegalStateException("Path variable name cannot be null");
66+
if (Map.class.isAssignableFrom(parameter.getParameterType())) {
67+
if (argument != null) {
68+
Assert.isInstanceOf(Map.class, argument);
69+
((Map<String, ?>) argument).forEach((key, value) ->
70+
addUriParameter(key, value, annotation.required(), requestDefinition));
71+
}
8572
}
86-
String stringValue = getStringValue(value, required);
87-
if (LOG.isTraceEnabled()) {
88-
LOG.trace("Path variable " + name + " resolved to " + stringValue);
73+
else {
74+
String name = StringUtils.hasText(annotation.value()) ? annotation.value() : annotation.name();
75+
name = StringUtils.hasText(name) ? name : parameter.getParameterName();
76+
Assert.notNull(name, "Failed to determine path variable name for parameter: " + parameter);
77+
addUriParameter(name, argument, annotation.required(), requestDefinition);
8978
}
90-
requestDefinition.getUriVariables().put(name, stringValue);
91-
}
92-
93-
@Nullable
94-
private String getStringValue(@Nullable Object value, boolean required) {
95-
validateForNull(value, required);
96-
validateForReactiveWrapper(value);
97-
return value != null
98-
? convertToString(TypeDescriptor.valueOf(value.getClass()), value) : null;
9979
}
10080

101-
private void addUriParameter(HttpRequestDefinition requestDefinition,
102-
Map.Entry<?, ?> entry, boolean required) {
103-
Object resolvedName = resolveFromOptional(entry.getKey());
104-
String stringName = getStringValue(resolvedName, true);
105-
Object resolvedValue = resolveFromOptional(entry.getValue());
106-
addUriParameter(requestDefinition, stringName, resolvedValue, required);
107-
}
81+
private void addUriParameter(
82+
String name, @Nullable Object value, boolean required, HttpRequestDefinition requestDefinition) {
10883

109-
private void validateForNull(@Nullable Object argument, boolean required) {
110-
if (argument == null) {
111-
if (required) {
112-
throw new IllegalStateException("Required variable cannot be null");
113-
}
84+
if (value instanceof Optional) {
85+
value = ((Optional<?>) value).orElse(null);
11486
}
115-
}
11687

117-
private void validateForReactiveWrapper(@Nullable Object object) {
118-
if (object != null) {
119-
Class<?> type = object.getClass();
120-
ReactiveAdapterRegistry adapterRegistry = ReactiveAdapterRegistry.getSharedInstance();
121-
ReactiveAdapter adapter = adapterRegistry.getAdapter(type);
122-
if (adapter != null) {
123-
throw new IllegalStateException(getClass().getSimpleName() +
124-
" does not support reactive type wrapper: " + type);
125-
}
88+
if (value == null) {
89+
Assert.isTrue(!required, "Missing required path variable '" + name + "'");
90+
return;
12691
}
12792

128-
}
129-
130-
@Nullable
131-
private Object resolveFromOptional(@Nullable Object argument) {
132-
if (argument instanceof Optional) {
133-
return ((Optional<?>) argument).orElse(null);
93+
if (!(value instanceof String)) {
94+
value = this.conversionService.convert(value, String.class);
13495
}
135-
return argument;
136-
}
13796

138-
@Nullable
139-
private String convertToString(TypeDescriptor typeDescriptor, @Nullable Object value) {
140-
if (value == null) {
141-
return null;
97+
if (logger.isTraceEnabled()) {
98+
logger.trace("Resolved path variable '" + name + "' to " + value);
14299
}
143-
if (value instanceof String) {
144-
return (String) value;
145-
}
146-
if (this.conversionService != null) {
147-
return (String) this.conversionService.convert(value, typeDescriptor, STRING_TYPE_DESCRIPTOR);
148-
}
149-
return String.valueOf(value);
100+
101+
requestDefinition.getUriVariables().put(name, (String) value);
150102
}
151103

152104
}

‎spring-web/src/test/java/org/springframework/web/service/invoker/HttpMethodArgumentResolverTests.java

+25-29
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,7 @@
1616

1717
package org.springframework.web.service.invoker;
1818

19-
import java.util.Collections;
20-
2119
import org.junit.jupiter.api.Test;
22-
import reactor.core.publisher.Mono;
23-
import reactor.test.StepVerifier;
2420

2521
import org.springframework.http.HttpMethod;
2622
import org.springframework.lang.Nullable;
@@ -29,65 +25,65 @@
2925

3026
import static org.assertj.core.api.Assertions.assertThat;
3127

28+
3229
/**
3330
* Tests for {@link HttpMethodArgumentResolver}.
3431
*
3532
* @author Olga Maciaszek-Sharma
3633
*/
37-
class HttpMethodArgumentResolverTests extends HttpServiceMethodTestSupport {
34+
public class HttpMethodArgumentResolverTests {
3835

39-
private final Service service = createService(Service.class,
40-
Collections.singletonList(new HttpMethodArgumentResolver()));
36+
private final TestHttpClientAdapter clientAdapter = new TestHttpClientAdapter();
4137

38+
private final Service service = this.clientAdapter.createService(Service.class, new HttpMethodArgumentResolver());
39+
40+
4241
@Test
4342
void shouldResolveRequestMethodFromArgument() {
44-
Mono<Void> execution = this.service.execute(HttpMethod.GET);
45-
46-
StepVerifier.create(execution).verifyComplete();
47-
assertThat(getRequestDefinition().getHttpMethod()).isEqualTo(HttpMethod.GET);
43+
this.service.execute(HttpMethod.GET);
44+
assertThat(getActualMethod()).isEqualTo(HttpMethod.GET);
4845
}
49-
46+
5047
@Test
5148
void shouldIgnoreArgumentsNotMatchingType() {
52-
Mono<Void> execution = this.service.execute("test");
53-
54-
StepVerifier.create(execution).verifyComplete();
55-
assertThat(getRequestDefinition().getHttpMethod()).isNull();
49+
this.service.execute("test");
50+
assertThat(getActualMethod()).isNull();
5651
}
5752

5853
@Test
59-
void shouldOverrideMethodAnnotationWithMethodArgument() {
60-
Mono<Void> execution = this.service.executeGet(HttpMethod.POST);
61-
62-
StepVerifier.create(execution).verifyComplete();
63-
assertThat(getRequestDefinition().getHttpMethod()).isEqualTo(HttpMethod.POST);
54+
void shouldOverrideMethodAnnotation() {
55+
this.service.executeGet(HttpMethod.POST);
56+
assertThat(getActualMethod()).isEqualTo(HttpMethod.POST);
6457
}
6558

6659
@Test
6760
void shouldIgnoreNullValue() {
68-
Mono<Void> execution = this.service.executeForNull(null);
61+
this.service.executeForNull(null);
62+
assertThat(getActualMethod()).isNull();
63+
}
6964

70-
StepVerifier.create(execution).verifyComplete();
71-
assertThat(getRequestDefinition().getHttpMethod()).isNull();
65+
@Nullable
66+
private HttpMethod getActualMethod() {
67+
return this.clientAdapter.getRequestDefinition().getHttpMethod();
7268
}
7369

7470

7571
private interface Service {
7672

7773
@HttpRequest
78-
Mono<Void> execute(HttpMethod method);
74+
void execute(HttpMethod method);
7975

8076
@GetRequest
81-
Mono<Void> executeGet(HttpMethod method);
77+
void executeGet(HttpMethod method);
8278

8379
@HttpRequest
84-
Mono<Void> execute(String test);
80+
void execute(String test);
8581

8682
@HttpRequest
87-
Mono<Void> execute(HttpMethod firstMethod, HttpMethod secondMethod);
83+
void execute(HttpMethod firstMethod, HttpMethod secondMethod);
8884

8985
@HttpRequest
90-
Mono<Void> executeForNull(@Nullable HttpMethod method);
86+
void executeForNull(@Nullable HttpMethod method);
9187
}
9288

9389
}

0 commit comments

Comments
 (0)