Skip to content

Commit 0389684

Browse files
sbrannensbrannen
committed
Revise contribution
See gh-35352
1 parent 8edc7cd commit 0389684

File tree

5 files changed

+137
-165
lines changed

5 files changed

+137
-165
lines changed

‎spring-web/src/testFixtures/java/org/springframework/web/testfixture/method/VisibilityTestHandler.java‎ renamed to ‎spring-web/src/testFixtures/java/org/springframework/web/testfixture/method/PackagePrivateMethodController.java‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,11 @@
1919
import org.springframework.stereotype.Controller;
2020
import org.springframework.web.bind.annotation.RequestMapping;
2121

22-
public class VisibilityTestHandler {
22+
@Controller
23+
public class PackagePrivateMethodController {
2324

24-
@Controller
25-
public static class PackagePrivateController {
26-
@RequestMapping("/package-private")
27-
void packagePrivateMethod() {
28-
}
25+
@RequestMapping("/package-private")
26+
void packagePrivateMethod() {
2927
}
3028

31-
@Controller
32-
public static class ProtectedController {
33-
@RequestMapping("/protected")
34-
protected void protectedMethod() {
35-
}
36-
}
3729
}

‎spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java‎

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -161,11 +161,13 @@ protected boolean isHandler(Class<?> beanType) {
161161
* Uses type-level and method-level {@link RequestMapping @RequestMapping}
162162
* and {@link HttpExchange @HttpExchange} annotations to create the
163163
* {@link RequestMappingInfo}.
164-
* <p>For CGLIB proxy classes, additional validation is performed based on method visibility:
164+
* <p>For CGLIB proxy classes, additional validation is performed based on
165+
* method visibility:
165166
* <ul>
166-
* <li>Private methods cannot be overridden and therefore cannot be used as handler methods.</li>
167-
* <li>Package-private methods from different packages are inaccessible and must be
168-
* changed to public or protected.</li>
167+
* <li>Private methods cannot be overridden and therefore cannot be used as
168+
* handler methods.</li>
169+
* <li>Package-private methods from different packages are inaccessible and
170+
* must be changed to public or protected.</li>
169171
* </ul>
170172
* @return the created {@code RequestMappingInfo}, or {@code null} if the method
171173
* does not have a {@code @RequestMapping} or {@code @HttpExchange} annotation
@@ -200,37 +202,38 @@ protected boolean isHandler(Class<?> beanType) {
200202
return info;
201203
}
202204

203-
private void validateCglibProxyMethodVisibility(Method method, Class<?> handlerType) {
204-
if (isCglibProxy(handlerType)) {
205+
/**
206+
* Validate the method visibility requirements specified in {@link #getMappingForMethod(Method, Class)}.
207+
* @since 7.0
208+
*/
209+
private static void validateCglibProxyMethodVisibility(Method method, Class<?> handlerType) {
210+
if (handlerType.getName().contains(ClassUtils.CGLIB_CLASS_SEPARATOR)) {
205211
int modifiers = method.getModifiers();
206212

207213
if (Modifier.isPrivate(modifiers)) {
208-
throw new IllegalStateException(
209-
"Private method [" + method.getName() + "] on CGLIB proxy class [" + handlerType.getName() +
210-
"] cannot be used as a request handler method because private methods cannot be overridden. " +
211-
"Change the method to non-private visibility or use interface-based JDK proxying instead.");
214+
throw new IllegalStateException("""
215+
Private method [%s] on CGLIB proxy class [%s] cannot be used as a request \
216+
handler method, because private methods cannot be overridden. \
217+
Change the method to non-private visibility, or use interface-based JDK \
218+
proxying instead.""".formatted(method.getName(), handlerType.getName()));
212219
}
213220

214221
if (!Modifier.isPublic(modifiers) && !Modifier.isProtected(modifiers)) {
215222
Class<?> declaringClass = method.getDeclaringClass();
216-
Package methodPackage = declaringClass.getPackage();
217-
Package handlerPackage = handlerType.getPackage();
218-
219-
if (!Objects.equals(methodPackage, handlerPackage)) {
220-
throw new IllegalStateException(
221-
"Package-private method [" + method.getName() + "] on CGLIB proxy class [" + declaringClass.getName() +
222-
"] from package [" + methodPackage.getName() + "] cannot be advised when used by handler class [" +
223-
handlerType.getName() + "] from package [" + handlerPackage.getName() + "] because it is effectively private. " +
224-
"Either make the method public/protected or use interface-based JDK proxying instead.");
223+
String declaringPackage = declaringClass.getPackage().getName();
224+
String handlerPackage = handlerType.getPackage().getName();
225+
226+
if (!Objects.equals(declaringPackage, handlerPackage)) {
227+
throw new IllegalStateException("""
228+
Package-private method [%s] declared in class [%s] cannot be advised by \
229+
CGLIB-proxied handler class [%s], because it is effectively private. Either \
230+
make the method public or protected, or use interface-based JDK proxying instead."""
231+
.formatted(method.getName(), declaringClass.getName(), handlerType.getName()));
225232
}
226233
}
227234
}
228235
}
229236

230-
private boolean isCglibProxy(Class<?> beanType) {
231-
return beanType.getName().contains(ClassUtils.CGLIB_CLASS_SEPARATOR);
232-
}
233-
234237
private @Nullable RequestMappingInfo createRequestMappingInfo(AnnotatedElement element) {
235238

236239
List<AnnotationDescriptor> descriptors =

‎spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java‎

Lines changed: 40 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@
5353
import org.springframework.web.service.annotation.PostExchange;
5454
import org.springframework.web.service.annotation.PutExchange;
5555
import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest;
56+
import org.springframework.web.testfixture.method.PackagePrivateMethodController;
5657
import org.springframework.web.testfixture.server.MockServerWebExchange;
5758
import org.springframework.web.util.pattern.PathPattern;
5859
import org.springframework.web.util.pattern.PathPatternParser;
@@ -62,8 +63,6 @@
6263
import static org.mockito.Mockito.mock;
6364
import static org.springframework.web.bind.annotation.RequestMethod.POST;
6465
import static org.springframework.web.reactive.result.method.RequestMappingInfo.paths;
65-
import static org.springframework.web.testfixture.method.VisibilityTestHandler.PackagePrivateController;
66-
import static org.springframework.web.testfixture.method.VisibilityTestHandler.ProtectedController;
6766

6867
/**
6968
* Tests for {@link RequestMappingHandlerMapping}.
@@ -414,25 +413,26 @@ void requestBodyAnnotationFromImplementationOverridesInterface() {
414413
assertThat(matchingInfo).isEqualTo(paths(path).methods(POST).consumes(mediaType.toString()).build());
415414
}
416415

417-
@Test
416+
@Test // gh-35352
418417
void privateMethodOnCglibProxyShouldThrowException() throws Exception {
419418
RequestMappingHandlerMapping mapping = new RequestMappingHandlerMapping();
420419

421420
Class<?> handlerType = PrivateMethodController.class;
422-
Method method = handlerType.getDeclaredMethod("privateMethod");
423-
421+
String methodName = "privateMethod";
422+
Method method = handlerType.getDeclaredMethod(methodName);
424423
Class<?> proxyClass = createProxyClass(handlerType);
425424

426425
assertThatIllegalStateException()
427426
.isThrownBy(() -> mapping.getMappingForMethod(method, proxyClass))
428-
.withMessageContainingAll(
429-
"Private method [privateMethod]",
430-
"cannot be used as a request handler method"
431-
);
427+
.withMessage("""
428+
Private method [%s] on CGLIB proxy class [%s] cannot be used as a request \
429+
handler method, because private methods cannot be overridden. \
430+
Change the method to non-private visibility, or use interface-based JDK \
431+
proxying instead.""", methodName, proxyClass.getName());
432432
}
433433

434-
@Test
435-
void protectedMethodShouldNotThrowException() throws Exception {
434+
@Test // gh-35352
435+
void protectedMethodOnCglibProxyShouldNotThrowException() throws Exception {
436436
RequestMappingHandlerMapping mapping = new RequestMappingHandlerMapping();
437437

438438
Class<?> handlerType = ProtectedMethodController.class;
@@ -444,63 +444,33 @@ void protectedMethodShouldNotThrowException() throws Exception {
444444
assertThat(info.getPatternsCondition().getDirectPaths()).containsOnly("/protected");
445445
}
446446

447-
@Test
448-
void differentPackagePackagePrivateMethodShouldThrowException() throws Exception {
447+
@Test // gh-35352
448+
void differentPackagePackagePrivateMethodOnCglibProxyShouldThrowException() throws Exception {
449449
RequestMappingHandlerMapping mapping = new RequestMappingHandlerMapping();
450450

451-
Class<?> handlerType = ControllerWithPackagePrivateClass.class;
452-
Method method = PackagePrivateController.class.getDeclaredMethod("packagePrivateMethod");
453-
451+
Class<?> handlerType = LocalPackagePrivateMethodControllerSubclass.class;
452+
Class<?> declaringClass = PackagePrivateMethodController.class;
453+
String methodName = "packagePrivateMethod";
454+
Method method = declaringClass.getDeclaredMethod(methodName);
454455
Class<?> proxyClass = createProxyClass(handlerType);
455456

456457
assertThatIllegalStateException()
457458
.isThrownBy(() -> mapping.getMappingForMethod(method, proxyClass))
458-
.withMessageContainingAll(
459-
"Package-private method [packagePrivateMethod]",
460-
"cannot be advised when used by handler class"
461-
);
462-
}
463-
464-
@Test
465-
void differentPackageProtectedMethodShouldNotThrowException() throws Exception {
466-
RequestMappingHandlerMapping mapping = new RequestMappingHandlerMapping();
467-
468-
Class<?> handlerType = ControllerWithProtectedClass.class;
469-
Method method = ProtectedController.class.getDeclaredMethod("protectedMethod");
470-
471-
Class<?> proxyClass = createProxyClass(handlerType);
472-
473-
RequestMappingInfo info = mapping.getMappingForMethod(method, proxyClass);
474-
assertThat(info.getPatternsCondition().getDirectPaths()).containsOnly("/protected");
459+
.withMessage("""
460+
Package-private method [%s] declared in class [%s] cannot be advised by \
461+
CGLIB-proxied handler class [%s], because it is effectively private. Either \
462+
make the method public or protected, or use interface-based JDK proxying instead.""",
463+
methodName, declaringClass.getName(), proxyClass.getName());
475464
}
476465

477466

478-
private Class<?> createProxyClass(Class<?> targetClass) {
467+
private static Class<?> createProxyClass(Class<?> targetClass) {
479468
Enhancer enhancer = new Enhancer();
480469
enhancer.setSuperclass(targetClass);
481470
enhancer.setCallbackTypes(new Class[]{NoOp.class});
482-
483471
return enhancer.createClass();
484472
}
485473

486-
@Controller
487-
static class PrivateMethodController {
488-
@RequestMapping("/private")
489-
private void privateMethod() {}
490-
}
491-
492-
@Controller
493-
static class ProtectedMethodController {
494-
@RequestMapping("/protected")
495-
protected void protectedMethod() {}
496-
}
497-
498-
@Controller
499-
static class ControllerWithPackagePrivateClass extends PackagePrivateController { }
500-
501-
@Controller
502-
static class ControllerWithProtectedClass extends ProtectedController { }
503-
504474
private RequestMappingInfo assertComposedAnnotationMapping(RequestMethod requestMethod) {
505475
String methodName = requestMethod.name().toLowerCase();
506476
String path = "/" + methodName;
@@ -705,4 +675,21 @@ public void post(@RequestBody(required = true) Foo foo) {}
705675
@interface ExtraHttpExchange {
706676
}
707677

678+
@Controller
679+
static class PrivateMethodController {
680+
681+
@RequestMapping("/private")
682+
private void privateMethod() {}
683+
}
684+
685+
@Controller
686+
static class ProtectedMethodController {
687+
688+
@RequestMapping("/protected")
689+
protected void protectedMethod() {}
690+
}
691+
692+
static class LocalPackagePrivateMethodControllerSubclass extends PackagePrivateMethodController {
693+
}
694+
708695
}

‎spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java‎

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -187,11 +187,13 @@ protected boolean isHandler(Class<?> beanType) {
187187
* Uses type-level and method-level {@link RequestMapping @RequestMapping}
188188
* and {@link HttpExchange @HttpExchange} annotations to create the
189189
* {@link RequestMappingInfo}.
190-
* <p>For CGLIB proxy classes, additional validation is performed based on method visibility:
190+
* <p>For CGLIB proxy classes, additional validation is performed based on
191+
* method visibility:
191192
* <ul>
192-
* <li>Private methods cannot be overridden and therefore cannot be used as handler methods.</li>
193-
* <li>Package-private methods from different packages are inaccessible and must be
194-
* changed to public or protected.</li>
193+
* <li>Private methods cannot be overridden and therefore cannot be used as
194+
* handler methods.</li>
195+
* <li>Package-private methods from different packages are inaccessible and
196+
* must be changed to public or protected.</li>
195197
* </ul>
196198
* @return the created {@code RequestMappingInfo}, or {@code null} if the method
197199
* does not have a {@code @RequestMapping} or {@code @HttpExchange} annotation
@@ -219,38 +221,38 @@ protected boolean isHandler(Class<?> beanType) {
219221
return info;
220222
}
221223

222-
private void validateCglibProxyMethodVisibility(Method method, Class<?> handlerType) {
223-
if (isCglibProxy(handlerType)) {
224+
/**
225+
* Validate the method visibility requirements specified in {@link #getMappingForMethod(Method, Class)}.
226+
* @since 7.0
227+
*/
228+
private static void validateCglibProxyMethodVisibility(Method method, Class<?> handlerType) {
229+
if (handlerType.getName().contains(ClassUtils.CGLIB_CLASS_SEPARATOR)) {
224230
int modifiers = method.getModifiers();
225231

226232
if (Modifier.isPrivate(modifiers)) {
227-
throw new IllegalStateException(
228-
"Private method [" + method.getName() + "] on CGLIB proxy class [" + handlerType.getName() +
229-
"] cannot be used as a request handler method because private methods cannot be overridden. " +
230-
"Change the method to non-private visibility or use interface-based JDK proxying instead.");
233+
throw new IllegalStateException("""
234+
Private method [%s] on CGLIB proxy class [%s] cannot be used as a request \
235+
handler method, because private methods cannot be overridden. \
236+
Change the method to non-private visibility, or use interface-based JDK \
237+
proxying instead.""".formatted(method.getName(), handlerType.getName()));
231238
}
232239

233240
if (!Modifier.isPublic(modifiers) && !Modifier.isProtected(modifiers)) {
234241
Class<?> declaringClass = method.getDeclaringClass();
235-
Package methodPackage = declaringClass.getPackage();
236-
Package handlerPackage = handlerType.getPackage();
237-
238-
if (!Objects.equals(methodPackage, handlerPackage)) {
239-
throw new IllegalStateException(
240-
"Package-private method [" + method.getName() + "] on CGLIB proxy class [" + declaringClass.getName() +
241-
"] from package [" + methodPackage.getName() + "] cannot be advised when used by handler class [" +
242-
handlerType.getName() + "] from package [" + handlerPackage.getName() + "] because it is effectively private. " +
243-
"Either make the method public/protected or use interface-based JDK proxying instead.");
242+
String declaringPackage = declaringClass.getPackage().getName();
243+
String handlerPackage = handlerType.getPackage().getName();
244+
245+
if (!Objects.equals(declaringPackage, handlerPackage)) {
246+
throw new IllegalStateException("""
247+
Package-private method [%s] declared in class [%s] cannot be advised by \
248+
CGLIB-proxied handler class [%s], because it is effectively private. Either \
249+
make the method public or protected, or use interface-based JDK proxying instead."""
250+
.formatted(method.getName(), declaringClass.getName(), handlerType.getName()));
244251
}
245252
}
246253
}
247254
}
248255

249-
private boolean isCglibProxy(Class<?> beanType) {
250-
return beanType.getName().contains(ClassUtils.CGLIB_CLASS_SEPARATOR);
251-
}
252-
253-
254256
@Nullable String getPathPrefix(Class<?> handlerType) {
255257
for (Map.Entry<String, Predicate<Class<?>>> entry : this.pathPrefixes.entrySet()) {
256258
if (entry.getValue().test(handlerType)) {

0 commit comments

Comments
 (0)