Explicitly detect (and prevent) private @scheduled methods on CGLIB proxies

Issue: SPR-12308

Author: jhoeller

Diff for: spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java

@@ -17,6 +17,7 @@
 package org.springframework.scheduling.annotation;
 
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedHashSet;
@@ -77,9 +78,8 @@
  * @see org.springframework.scheduling.TaskScheduler
  * @see org.springframework.scheduling.config.ScheduledTaskRegistrar
  */
-public class ScheduledAnnotationBeanPostProcessor
-		implements BeanPostProcessor, Ordered, EmbeddedValueResolverAware, BeanFactoryAware,
-		SmartInitializingSingleton, DisposableBean {
+public class ScheduledAnnotationBeanPostProcessor implements BeanPostProcessor, Ordered,
+		EmbeddedValueResolverAware, BeanFactoryAware, SmartInitializingSingleton, DisposableBean {
 
 	protected final Log logger = LogFactory.getLog(getClass());
 
@@ -227,17 +227,27 @@ protected void processScheduled(Scheduled scheduled, Method method, Object bean)
 				}
 				catch (NoSuchMethodException ex) {
 					throw new IllegalStateException(String.format(
-							"@Scheduled method '%s' found on bean target class '%s', " +
-							"but not found in any interface(s) for bean JDK proxy. Either " +
-							"pull the method up to an interface or switch to subclass (CGLIB) " +
-							"proxies by setting proxy-target-class/proxyTargetClass " +
-							"attribute to 'true'", method.getName(), method.getDeclaringClass().getSimpleName()));
+							"@Scheduled method '%s' found on bean target class '%s' but not " +
+							"found in any interface(s) for a dynamic proxy. Either pull the " +
+							"method up to a declared interface or switch to subclass (CGLIB) " +
+							"proxies by setting proxy-target-class/proxyTargetClass to 'true'",
+							method.getName(), method.getDeclaringClass().getSimpleName()));
+				}
+			}
+			else if (AopUtils.isCglibProxy(bean)) {
+				// Common problem: private methods end up in the proxy instance, not getting delegated.
+				if (Modifier.isPrivate(method.getModifiers())) {
+					throw new IllegalStateException(String.format(
+							"@Scheduled method '%s' found on CGLIB proxy for target class '%s' but cannot " +
+							"be delegated to target bean. Switch its visibility to package or protected.",
+							method.getName(), method.getDeclaringClass().getSimpleName()));
 				}
 			}
 
 			Runnable runnable = new ScheduledMethodRunnable(bean, method);
 			boolean processedSchedule = false;
-			String errorMessage = "Exactly one of the 'cron', 'fixedDelay(String)', or 'fixedRate(String)' attributes is required";
+			String errorMessage =
+					"Exactly one of the 'cron', 'fixedDelay(String)', or 'fixedRate(String)' attributes is required";
 
 			// Determine initial delay
 			long initialDelay = scheduled.initialDelay();

Diff for: spring-context/src/test/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessorTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,6 +28,7 @@
 import java.util.Properties;
 import java.util.TimeZone;
 
+import org.junit.After;
 import org.junit.Test;
 
 import org.springframework.beans.DirectFieldAccessor;
@@ -46,6 +47,8 @@
 import org.springframework.scheduling.support.SimpleTriggerContext;
 import org.springframework.tests.Assume;
 import org.springframework.tests.TestGroup;
+import org.springframework.validation.annotation.Validated;
+import org.springframework.validation.beanvalidation.MethodValidationPostProcessor;
 
 import static org.junit.Assert.*;
 
@@ -60,6 +63,13 @@ public class ScheduledAnnotationBeanPostProcessorTests {
 
 	private final StaticApplicationContext context = new StaticApplicationContext();
 
+
+	@After
+	public void closeContextAfterTest() {
+		context.close();
+	}
+
+
 	@Test
 	public void fixedDelayTask() {
 		BeanDefinition processorDefinition = new RootBeanDefinition(ScheduledAnnotationBeanPostProcessor.class);
@@ -140,7 +150,8 @@ public void fixedRateTaskWithInitialDelay() {
 	public void severalFixedRatesWithRepeatedScheduledAnnotation() {
 		BeanDefinition processorDefinition = new
 		RootBeanDefinition(ScheduledAnnotationBeanPostProcessor.class);
-		BeanDefinition targetDefinition = new RootBeanDefinition(SeveralFixedRatesWithRepeatedScheduledAnnotationTestBean.class);
+		BeanDefinition targetDefinition = new RootBeanDefinition(
+				SeveralFixedRatesWithRepeatedScheduledAnnotationTestBean.class);
 		severalFixedRates(context, processorDefinition, targetDefinition);
 	}
 
@@ -155,6 +166,7 @@ public void severalFixedRatesWithSchedulesContainerAnnotation() {
 
 	private void severalFixedRates(StaticApplicationContext context,
 			BeanDefinition processorDefinition, BeanDefinition targetDefinition) {
+
 		context.registerBeanDefinition("postProcessor", processorDefinition);
 		context.registerBeanDefinition("target", targetDefinition);
 		context.refresh();
@@ -248,7 +260,8 @@ public void cronTaskWithZone() throws InterruptedException {
 		Date lastActualExecutionTime = cal.getTime();
 		cal.add(Calendar.MINUTE, 30); // 4:30
 		Date lastCompletionTime = cal.getTime();
-		TriggerContext triggerContext = new SimpleTriggerContext(lastScheduledExecutionTime, lastActualExecutionTime, lastCompletionTime);
+		TriggerContext triggerContext = new SimpleTriggerContext(
+				lastScheduledExecutionTime, lastActualExecutionTime, lastCompletionTime);
 		cal.add(Calendar.MINUTE, 30);
 		cal.add(Calendar.HOUR_OF_DAY, 1); // 6:00
 		Date nextExecutionTime = cronTrigger.nextExecutionTime(triggerContext);
@@ -269,6 +282,18 @@ public void cronTaskWithInvalidZone() throws InterruptedException {
 		Thread.sleep(10000);
 	}
 
+	@Test(expected = BeanCreationException.class)
+	public void cronTaskWithMethodValidation() throws InterruptedException {
+		BeanDefinition validationDefinition = new RootBeanDefinition(MethodValidationPostProcessor.class);
+		BeanDefinition processorDefinition = new RootBeanDefinition(ScheduledAnnotationBeanPostProcessor.class);
+		BeanDefinition targetDefinition = new RootBeanDefinition(
+				ScheduledAnnotationBeanPostProcessorTests.CronTestBean.class);
+		context.registerBeanDefinition("methodValidation", validationDefinition);
+		context.registerBeanDefinition("postProcessor", processorDefinition);
+		context.registerBeanDefinition("target", targetDefinition);
+		context.refresh();
+	}
+
 	@Test
 	public void metaAnnotationWithFixedRate() {
 		BeanDefinition processorDefinition = new RootBeanDefinition(ScheduledAnnotationBeanPostProcessor.class);
@@ -527,10 +552,11 @@ public void fixedRate() {
 	}
 
 
+	@Validated
 	static class CronTestBean {
 
 		@Scheduled(cron="*/7 * * * * ?")
-		public void cron() throws IOException {
+		private void cron() throws IOException {
 			throw new IOException("no no no");
 		}
 	}
@@ -539,7 +565,7 @@ public void cron() throws IOException {
 	static class CronWithTimezoneTestBean {
 
 		@Scheduled(cron="0 0 0-4,6-23 * * ?", zone = "GMT+10")
-		public void cron() throws IOException {
+		protected void cron() throws IOException {
 			throw new IOException("no no no");
 		}
 	}
@@ -642,7 +668,8 @@ public void fixedRate() {
 	@Scheduled(cron="${schedules.businessHours}")
 	@Target(ElementType.METHOD)
 	@Retention(RetentionPolicy.RUNTIME)
-	private static @interface BusinessHours {}
+	private static @interface BusinessHours {
+	}
 
 
 	static class PropertyPlaceholderMetaAnnotationTestBean {