From d213d506b3e85f6204bf3cfb4fa104d88d692450 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Thu, 12 Jun 2025 16:09:55 +0200 Subject: [PATCH 1/6] HV-1591 Add extra methods to ListAppender Make it easier to get and filter the logged messages for testing --- .../validator/testutils/ListAppender.java | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/engine/src/test/java/org/hibernate/validator/testutils/ListAppender.java b/engine/src/test/java/org/hibernate/validator/testutils/ListAppender.java index 940485ded..94a5b8200 100644 --- a/engine/src/test/java/org/hibernate/validator/testutils/ListAppender.java +++ b/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 getEvents() { - return Collections.unmodifiableList( this.events ); + return unmodifiableList( this.events ); + } + + /** + * Returns all logged messages + */ + public List getMessages() { + return getMessages( logEvent -> true ); + } + + /** + * Returns logged messages matching the specified {@link Level} + */ + public List getMessages(Level level) { + return getMessages( logEvent -> logEvent.getLevel().equals( level ) ); + } + + private List getMessages(Predicate filter) { + return events.stream().filter( filter ).map( LogEvent::getMessage ) + .map( Message::getFormattedMessage ).toList(); } } From 090196d8fabf759c7faaf659c3b3633e8db998d3 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Thu, 12 Jun 2025 16:17:29 +0200 Subject: [PATCH 2/6] HV-1591 Replace status with rootLogger.level in log4j.properties Only for testing, otherwise it doesn't seem to have any effect. --- engine/src/test/resources/log4j2.properties | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/engine/src/test/resources/log4j2.properties b/engine/src/test/resources/log4j2.properties index d4a2ce801..69039b50a 100644 --- a/engine/src/test/resources/log4j2.properties +++ b/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 . - -status = error +rootLogger.level = error +rootLogger.appenderRefs = console +rootLogger.appenderRef.console.ref = console # Direct log messages to stdout appender.console.type = Console From 9ff87ef66040be018a24e629a75db57e157b7497 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Mon, 30 Jun 2025 16:37:45 +0200 Subject: [PATCH 3/6] HV-1591 Create warning log message for improper use of Valid --- .../org/hibernate/validator/internal/util/logging/Log.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/engine/src/main/java/org/hibernate/validator/internal/util/logging/Log.java b/engine/src/main/java/org/hibernate/validator/internal/util/logging/Log.java index 590bd2af1..684e1f8a9 100644 --- a/engine/src/main/java/org/hibernate/validator/internal/util/logging/Log.java +++ b/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); } From e5634677d1d4d3f466f28e9f884f6bae6bbd9843 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Mon, 30 Jun 2025 16:37:57 +0200 Subject: [PATCH 4/6] HV-1591 Log a warning for improper use of Valid annotation --- .../internal/metadata/aggregated/CascadingMetaDataBuilder.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/CascadingMetaDataBuilder.java b/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/CascadingMetaDataBuilder.java index 90907484a..4fc34f63e 100644 --- a/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/CascadingMetaDataBuilder.java +++ b/engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/CascadingMetaDataBuilder.java @@ -224,6 +224,9 @@ public CascadingMetaData build(ValueExtractorManager valueExtractorManager, Obje Set 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 ), From 991b85757a0adce264830971ff64cafcb7fb5ce1 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Mon, 30 Jun 2025 16:37:38 +0200 Subject: [PATCH 5/6] HV-1591 Test for deprecated use of Value annotation A user must apply `@Value` to the argument(s) of a container type. If they don't, there should be message in the logs warning them. --- .../annotations/hv/ValidAnnotationTest.java | 207 ++++++++++++++++++ 1 file changed, 207 insertions(+) create mode 100644 engine/src/test/java/org/hibernate/validator/test/constraints/annotations/hv/ValidAnnotationTest.java diff --git a/engine/src/test/java/org/hibernate/validator/test/constraints/annotations/hv/ValidAnnotationTest.java b/engine/src/test/java/org/hibernate/validator/test/constraints/annotations/hv/ValidAnnotationTest.java new file mode 100644 index 000000000..0b3992e60 --- /dev/null +++ b/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 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 prop; + + public Foo(List 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 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 prop; + + public Foo(Set 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 prop; + + public Foo(Map 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 prop; + + public Foo(Map 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; + } + } +} From 67226509d8badf375d7d351467c5a70564753595 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Tue, 1 Jul 2025 10:43:56 +0200 Subject: [PATCH 6/6] HV-1591 chore: Remove IDE warnings (Optional) --- .../ap/testutil/CompilerTestHelper.java | 48 +++++++++---------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/annotation-processor/src/test/java/org/hibernate/validator/ap/testutil/CompilerTestHelper.java b/annotation-processor/src/test/java/org/hibernate/validator/ap/testutil/CompilerTestHelper.java index 2288a7d14..dc77175dc 100644 --- a/annotation-processor/src/test/java/org/hibernate/validator/ap/testutil/CompilerTestHelper.java +++ b/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 dependencies, File... sourceFiles) { StandardJavaFileManager fileManager = compiler.getStandardFileManager( null, null, null ); Iterable compilationUnits = fileManager.getJavaFileObjects( sourceFiles ); - List options = new ArrayList(); + 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 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 extractOptions(Kind diagnosticKind, Boolean verbose, Boolean allowMethodConstraints) { + final List 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 asExpectations(Collection getDependenciesAsFiles(EnumSet dependencies) { - Set files = new HashSet(); + Set files = new HashSet<>(); for ( Library oneDependency : dependencies ) { files.add( new File( testLibraryDir + File.separator + oneDependency.getName() ) );