HV-1591 Log warning when @Valid is used improperly

annotation-processor/src/test/java/org/hibernate/validator/ap/testutil/CompilerTestHelper.java

@@ -10,8 +10,8 @@
 import java.io.File;
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.EnumSet;
 import java.util.HashSet;
 import java.util.List;
@@ -57,7 +57,7 @@ public enum Library {
 
 		private final String name;
 
-		private Library(String name) {
+		Library(String name) {
 			this.name = name;
 		}
 
@@ -82,7 +82,7 @@ public String getName() {
 		PROCESSOR_OUT_DIR = new File( TARGET_DIR, "processor-generated-test-classes" );
 		if ( !PROCESSOR_OUT_DIR.exists() ) {
 			if ( !PROCESSOR_OUT_DIR.mkdirs() ) {
-				fail( "Unable to create test output directory " + PROCESSOR_OUT_DIR.toString() );
+				fail( "Unable to create test output directory " + PROCESSOR_OUT_DIR );
 			}
 		}
 	}
@@ -163,7 +163,23 @@ public boolean compile(Processor annotationProcessor, DiagnosticCollector<JavaFi
 			EnumSet<Library> dependencies, File... sourceFiles) {
 		StandardJavaFileManager fileManager = compiler.getStandardFileManager( null, null, null );
 		Iterable<? extends JavaFileObject> compilationUnits = fileManager.getJavaFileObjects( sourceFiles );
-		List<String> options = new ArrayList<String>();
+		try {
+			fileManager.setLocation( StandardLocation.CLASS_PATH, getDependenciesAsFiles( dependencies ) );
+			fileManager.setLocation( StandardLocation.CLASS_OUTPUT, Collections.singletonList( PROCESSOR_OUT_DIR ) );
+		}
+		catch (IOException e) {
+			throw new RuntimeException( e );
+		}
+
+		final List<String> options = extractOptions( diagnosticKind, verbose, allowMethodConstraints );
+		CompilationTask task = compiler.getTask( null, fileManager, diagnostics, options, null, compilationUnits );
+		task.setProcessors( Collections.singletonList( annotationProcessor ) );
+
+		return task.call();
+	}
+
+	private static List<String> extractOptions(Kind diagnosticKind, Boolean verbose, Boolean allowMethodConstraints) {
+		final List<String> options = new ArrayList<>();
 
 		if ( diagnosticKind != null ) {
 			options.add( StringHelper.format( "-A%s=%s", Configuration.DIAGNOSTIC_KIND_PROCESSOR_OPTION, diagnosticKind ) );
@@ -174,27 +190,9 @@ public boolean compile(Processor annotationProcessor, DiagnosticCollector<JavaFi
 		}
 
 		if ( allowMethodConstraints != null ) {
-			options.add(
-					StringHelper.format(
-							"-A%s=%b",
-							Configuration.METHOD_CONSTRAINTS_SUPPORTED_PROCESSOR_OPTION,
-							allowMethodConstraints
-					)
-			);
+			options.add( StringHelper.format( "-A%s=%b", Configuration.METHOD_CONSTRAINTS_SUPPORTED_PROCESSOR_OPTION, allowMethodConstraints ) );
 		}
-
-		try {
-			fileManager.setLocation( StandardLocation.CLASS_PATH, getDependenciesAsFiles( dependencies ) );
-			fileManager.setLocation( StandardLocation.CLASS_OUTPUT, Arrays.asList( PROCESSOR_OUT_DIR ) );
-		}
-		catch (IOException e) {
-			throw new RuntimeException( e );
-		}
-
-		CompilationTask task = compiler.getTask( null, fileManager, diagnostics, options, null, compilationUnits );
-		task.setProcessors( Arrays.asList( annotationProcessor ) );
-
-		return task.call();
+		return options;
 	}
 
 	/**
@@ -226,7 +224,7 @@ private static Set<DiagnosticExpectation> asExpectations(Collection<Diagnostic<?
 	}
 
 	private Set<File> getDependenciesAsFiles(EnumSet<Library> dependencies) {
-		Set<File> files = new HashSet<File>();
+		Set<File> files = new HashSet<>();
 
 		for ( Library oneDependency : dependencies ) {
 			files.add( new File( testLibraryDir + File.separator + oneDependency.getName() ) );

engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/CascadingMetaDataBuilder.java

@@ -224,6 +224,9 @@ public CascadingMetaData build(ValueExtractorManager valueExtractorManager, Obje
 		Set<ValueExtractorDescriptor> containerDetectionValueExtractorCandidates = valueExtractorManager.getResolver()
 				.getValueExtractorCandidatesForContainerDetectionOfGlobalCascadedValidation( enclosingType );
 		if ( !containerDetectionValueExtractorCandidates.isEmpty() ) {
+			// Using @Valid on a container is deprecated at the moment. You are supposed to apply the annotation on the type argument(s).
+			LOG.deprecatedUseOfValidOnContainer( ReflectionHelper.getClassFromType( enclosingType ), context );
+
 			if ( containerDetectionValueExtractorCandidates.size() > 1 ) {
 				throw LOG.getUnableToGetMostSpecificValueExtractorDueToSeveralMaximallySpecificValueExtractorsDeclaredException(
 						ReflectionHelper.getClassFromType( enclosingType ),

engine/src/main/java/org/hibernate/validator/internal/util/logging/Log.java

@@ -946,4 +946,8 @@ ConstraintDefinitionException getConstraintValidatorDefinitionConstraintMismatch
 	@LogMessage(level = DEBUG)
 	@Message(id = 269, value = "Unable to enable secure XML feature processing when loading %1$s: %2$s")
 	void unableToEnableSecureFeatureProcessingSchemaXml(String fileName, String message);
+
+	@LogMessage(level = WARN)
+	@Message(id = 270, value = "Using `@Valid` on a container (%1$s) is deprecated. You should apply the annotation on the type argument(s). Field name: %2$s")
+	void deprecatedUseOfValidOnContainer(@FormatWith(ClassObjectFormatter.class) Class<?> valueType, Object context);
 }

engine/src/test/java/org/hibernate/validator/test/constraints/annotations/hv/ValidAnnotationTest.java

@@ -0,0 +1,207 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ * Copyright Red Hat Inc. and Hibernate Authors
+ */
+package org.hibernate.validator.test.constraints.annotations.hv;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import jakarta.validation.Valid;
+
+import org.hibernate.validator.internal.metadata.aggregated.CascadingMetaDataBuilder;
+import org.hibernate.validator.test.constraints.annotations.AbstractConstrainedTest;
+import org.hibernate.validator.test.internal.engine.serialization.Email;
+import org.hibernate.validator.testutils.ListAppender;
+
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.core.Logger;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.config.Configurator;
+import org.testng.annotations.AfterTest;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.Test;
+
+public class ValidAnnotationTest extends AbstractConstrainedTest {
+
+	private ListAppender logAppender;
+	private Level originalLogLevel;
+	private Logger targetLogger;
+
+	/**
+	 * @return true if the string matches the expected log message for deprecated use of @Value.
+	 */
+	private static boolean deprecatedUsedOfValueCode(String s) {
+		return s.startsWith( "HV000270" );
+	}
+
+	@BeforeTest
+	public void setUpLogger() {
+		logAppender = new ListAppender( ValidAnnotationTest.class.getSimpleName() );
+		logAppender.start();
+
+		LoggerContext context = LoggerContext.getContext( false );
+		targetLogger = context.getLogger( CascadingMetaDataBuilder.class.getName() );
+		targetLogger.addAppender( logAppender );
+
+		// Set level of log messages to WARN (if they are not already enabled)
+		if ( targetLogger.getLevel().isMoreSpecificThan( Level.WARN ) ) {
+			// Store the original log level to restore it later
+			originalLogLevel = targetLogger.getLevel();
+
+			// Override the log level for this test class only
+			// Default tests will only print the error messages, we need to override it
+			// so that we can capture the deprecated warning message
+			Configurator.setLevel( CascadingMetaDataBuilder.class.getName(), Level.WARN );
+			context.updateLoggers();
+		}
+	}
+
+	@BeforeMethod
+	public void cleanLogger() {
+		logAppender.clear();
+	}
+
+	@AfterTest
+	public void tearDownLogger() {
+		targetLogger.removeAppender( logAppender );
+		logAppender.stop();
+
+		// Restore the original log level
+		if ( originalLogLevel != null ) {
+			Configurator.setLevel( CascadingMetaDataBuilder.class.getName(), originalLogLevel );
+			// Update the logger context to apply changes
+			LoggerContext context = LoggerContext.getContext( false );
+			context.updateLoggers();
+		}
+	}
+
+	@Test
+	public void twiceWithList() {
+		class Foo {
+
+			@Valid
+			private List<@Valid String> prop;
+
+			public Foo(List<String> prop) {
+				this.prop = prop;
+			}
+		}
+
+		Foo foo = new Foo( List.of( "K1" ) );
+		validator.validate( foo );
+
+		assertThat( logAppender.getMessages() ).hasSize( 1 ).allMatch( ValidAnnotationTest::deprecatedUsedOfValueCode );
+	}
+
+	@Test
+	public void onTheContainerWithList() {
+		class Foo {
+
+			@Valid
+			private List<MyBean> prop;
+
+			public Foo(List<MyBean> prop) {
+				this.prop = prop;
+			}
+		}
+
+		Foo foo = new Foo( List.of( new MyBean( "entry.list@email.com" ) ) );
+		validator.validate( foo );
+
+		assertThat( logAppender.getMessages() ).hasSize( 1 ).allMatch( ValidAnnotationTest::deprecatedUsedOfValueCode );
+	}
+
+	@Test
+	public void twiceWithSet() {
+		class Foo {
+
+			@Valid
+			private Set<@Valid String> prop;
+
+			public Foo(Set<String> prop) {
+				this.prop = prop;
+			}
+		}
+
+		Foo foo = new Foo( Set.of( "K1" ) );
+		validator.validate( foo );
+
+		assertThat( logAppender.getMessages() ).hasSize( 1 ).allMatch( ValidAnnotationTest::deprecatedUsedOfValueCode );
+	}
+
+	@Test
+	public void onTheContainerWithSet() {
+		class Foo {
+
+			@Valid
+			private Set<MyBean> prop;
+
+			public Foo(Set<MyBean> prop) {
+				this.prop = prop;
+			}
+		}
+
+		Foo foo = new Foo( Set.of( new MyBean( "entry.set@email.email" ) ) );
+		validator.validate( foo );
+
+		assertThat( logAppender.getMessages() ).hasSize( 1 ).allMatch( ValidAnnotationTest::deprecatedUsedOfValueCode );
+	}
+
+	@Test
+	public void twiceWithMap() {
+		class Foo {
+
+			@Valid
+			private Map<String, @Valid String> prop;
+
+			public Foo(Map<String, String> prop) {
+				this.prop = prop;
+			}
+		}
+
+		Foo foo = new Foo( Map.of( "K1", "V1" ) );
+		validator.validate( foo );
+
+		assertThat( logAppender.getMessages() ).hasSize( 1 ).allMatch( ValidAnnotationTest::deprecatedUsedOfValueCode );
+	}
+
+	@Test
+	public void onContainerWithMap() {
+		class Foo {
+
+			@Valid
+			private Map<String, MyBean> prop;
+
+			public Foo(Map<String, MyBean> prop) {
+				this.prop = prop;
+			}
+		}
+
+		Foo foo = new Foo( Map.of( "K1", new MyBean( "value1@email.email" ) ) );
+		validator.validate( foo );
+
+		assertThat( logAppender.getMessages() ).hasSize( 1 ).allMatch( ValidAnnotationTest::deprecatedUsedOfValueCode );
+	}
+
+	private static class MyBean {
+		@Email
+		private String email;
+
+		public MyBean(String email) {
+			this.email = email;
+		}
+
+		public String getEmail() {
+			return email;
+		}
+
+		public void setEmail(String email) {
+			this.email = email;
+		}
+	}
+}

engine/src/test/java/org/hibernate/validator/testutils/ListAppender.java

@@ -4,14 +4,19 @@
  */
 package org.hibernate.validator.testutils;
 
+import static java.util.Collections.unmodifiableList;
+
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.function.Predicate;
 
+import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.core.LogEvent;
 import org.apache.logging.log4j.core.appender.AbstractAppender;
 import org.apache.logging.log4j.core.config.Property;
 import org.apache.logging.log4j.core.impl.MutableLogEvent;
+import org.apache.logging.log4j.message.Message;
 
 /**
  * A simple log appender for tests.
@@ -39,6 +44,25 @@ public void clear() {
 	}
 
 	public List<LogEvent> getEvents() {
-		return Collections.unmodifiableList( this.events );
+		return unmodifiableList( this.events );
+	}
+
+	/**
+	 * Returns all logged messages
+	 */
+	public List<String> getMessages() {
+		return getMessages( logEvent -> true );
+	}
+
+	/**
+	 * Returns logged messages matching the specified {@link Level}
+	 */
+	public List<String> getMessages(Level level) {
+		return getMessages( logEvent -> logEvent.getLevel().equals( level ) );
+	}
+
+	private List<String> getMessages(Predicate<LogEvent> filter) {
+		return events.stream().filter( filter ).map( LogEvent::getMessage )
+				.map( Message::getFormattedMessage ).toList();
 	}
 }

engine/src/test/resources/log4j2.properties

@@ -1,7 +1,8 @@
 # License: Apache License, Version 2.0
 # See the LICENSE file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>.
-
-status = error
+rootLogger.level = error
+rootLogger.appenderRefs = console
+rootLogger.appenderRef.console.ref = console
 
 # Direct log messages to stdout
 appender.console.type = Console