Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cascaded and container element constraints on a @ConfigurationProperties class do not work correctly in a native image #37101

Closed
KimSeongIl opened this issue Jun 20, 2023 · 13 comments
Labels
for: external-project For an external project and not something we can fix status: superseded An issue that has been superseded by another theme: aot An issue related to Ahead-of-time processing

Comments

@KimSeongIl
Copy link

KimSeongIl commented Jun 20, 2023

spring boot version: 3.1.0
graalvm plugin version: id("org.graalvm.buildtools.native") version "0.9.20"
graalvm version: 22.3.r17

DemoContext.kt

@ConfigurationProperties(prefix = "demo.context")
@Validated
class DemoContext {

    var exclude: List<Exclude> = mutableListOf()

    data class Exclude(
        var url: List<String>? = null,

        var httpMethod: List<HttpMethod>? = null,

        var httpStatus: List<@Pattern(regexp="^([1-5][x|X]{2}|[1-5][0-9]{2})\$") String>? = null,

        var elapsedTime: Long? = null
    )

}

application.yml

demo:
  context:
    exclude:
      - httpStatus: 2xx

nativeRun result

Binding to target [Bindable@517a5ddb type = java.util.List<DemoContext$Exclude>, value = 'provided', annotations = array<Annotation>[[empty]]] failed:

    Property: demo.context.exclude[0].httpstatus
    Value: "2xx"
    Origin: class path resource [application.yml] - 31:21
    Reason: The elements [demo.context.exclude[0].httpstatus] were left unbound.

Action:

Update your application's configuration

It worked fine before, but when I run it on the native image I get an error.
However, if I remove the Pattern annotation, it works fine.

Why is this problem occurring?
What should I do to resolve this?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 20, 2023
@sdeleuze sdeleuze self-assigned this Jun 21, 2023
@sdeleuze sdeleuze added the theme: aot An issue related to Ahead-of-time processing label Jun 21, 2023
@sdeleuze
Copy link
Contributor

sdeleuze commented Jun 21, 2023

I can reproduce, but:

  • Looks like a Spring Boot issue
  • May be Kotlin specific, can't reproduce with Java records
  • The binding is likely incorrect and should be something like:
demo:
  context:
    exclude:
      -
        httpStatus:
          - "2xx"
  • Even with the right yaml, the pattern validation does not seem to work (putting random values on statuses works)

@sdeleuze
Copy link
Contributor

See the related stacktrace with the refined application.yml here.

@snicoll snicoll transferred this issue from spring-projects/spring-framework Jun 21, 2023
@snicoll snicoll removed the theme: aot An issue related to Ahead-of-time processing label Jun 21, 2023
@KimSeongIl
Copy link
Author

KimSeongIl commented Jun 21, 2023

@sdeleuze

Thank you for your review.

But that code works fine with jvm not graalvm.

Result when "7xx" is value in jvm.

Binding to target org.springframework.boot.context.properties.bind.BindException: Failed to bind properties under 'demo.context' to DemoContext failed:

    Property: demo.context.exclude[0].httpStatus[0]
    Value: "7xx"
    Reason: "^([1-5][x|X]{2}|[1-5][0-9]{2})$"와 일치해야 합니다


Action:

Update your application's configuration

It doesn't work only in graalvm native image.

Why is this problem occurring?

@wilkinsona
Copy link
Member

The validation doesn't work for me on the JVM either:

> Task :bootRun

  .   ____          _            __ _ _
 /\\ / ___'_ __ _ _(_)_ __  __ _ \ \ \ \
( ( )\___ | '_ | '_| | '_ \/ _` | \ \ \ \
 \\/  ___)| |_)| | | | | || (_| |  ) ) ) )
  '  |____| .__|_| |_|_| |_\__, | / / / /
 =========|_|==============|___/=/_/_/_/
 :: Spring Boot ::                (v3.1.0)

2023-06-22T10:54:42.200+01:00  INFO 79694 --- [           main] c.example.gh36006.Gh36006ApplicationKt   : Starting Gh36006ApplicationKt using Java 17.0.5 with PID 79694 (/Users/awilkinson/Downloads/gh-36006/build/classes/kotlin/main started by awilkinson in /Users/awilkinson/Downloads/gh-36006)
2023-06-22T10:54:42.203+01:00  INFO 79694 --- [           main] c.example.gh36006.Gh36006ApplicationKt   : No active profile set, falling back to 1 default profile: "default"
2023-06-22T10:54:42.864+01:00  INFO 79694 --- [           main] c.example.gh36006.Gh36006ApplicationKt   : Started Gh36006ApplicationKt in 1.008 seconds (process running for 1.247)
[Exclude(url=null, httpMethod=null, httpStatus=[boom], elapsedTime=null)]

@KimSeongIl, so that we can be certain that we're looking at exactly the same thing, please provide a complete yet minimal sample that reproduces the behaviour you have described. Guessing dependencies and copy-pasting code from issue comments leaves to much room for unwanted differences.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jun 22, 2023
@KimSeongIl
Copy link
Author

KimSeongIl commented Jun 22, 2023

@wilkinsona @sdeleuze

https://github.com/KimSeongIl/demo

Created a demo project.

The problem is reproduced in this project.

Could you please check again?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 22, 2023
@KimSeongIl
Copy link
Author

@wilkinsona

https://github.com/KimSeongIl/demo

in the demo project

    tasks.withType<KotlinCompile> {
        kotlinOptions {
            freeCompilerArgs = listOf("-Xjsr305=strict", "-Xemit-jvm-type-annotations")
            jvmTarget = "11"
        }

-Xemit-jvm-type-annotations

option works fine in jvm environment.

Do I need to use other options in a graalvm environment?

@wilkinsona
Copy link
Member

Thanks, @KimSeongIl. I've been able to explore the problem now.

The problem occurs because Spring Boot's configuration property binding cannot identify the constructor to use to create an instance of Exclude. This only happens when running in a native image. It's this code that's behaving differently:

private static Constructor<?> deduceKotlinBindConstructor(Class<?> type) {
Constructor<?> primaryConstructor = BeanUtils.findPrimaryConstructor(type);
if (primaryConstructor != null && primaryConstructor.getParameterCount() > 0) {
return primaryConstructor;
}
return null;
}

On the JVM, BeanUtils.findPrimaryConstructor returns public com.demo.search.middle.demo.DemoContext$Exclude(java.util.List,java.util.List,java.util.List,java.lang.Long). In a native image it returns null. BeanUtils is part of Spring Framework but I'm not sure if this is a Spring Framework bug, a Kotlin bug, or even a Graal bug. We'll transfer this issue to the Framework team so that they can continue the investigation.

@wilkinsona
Copy link
Member

Digging a little more before transferring the issue shows that the primary constructor is null due to an UnsupportedOperationException that's thrown by Kotlin:

java.lang.UnsupportedOperationException: Type not found: class jakarta.validation.constraints.Pattern$Flag
        at kotlin.reflect.jvm.internal.impl.descriptors.runtime.structure.ReflectJavaClassifierType.getClassifierQualifiedName(ReflectJavaClassifierType.kt:41)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.types.JavaTypeResolver.createNotFoundClass(JavaTypeResolver.kt:161)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.types.JavaTypeResolver.computeTypeConstructor(JavaTypeResolver.kt:147)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.types.JavaTypeResolver.computeSimpleJavaClassifierType(JavaTypeResolver.kt:128)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.types.JavaTypeResolver.transformJavaClassifierType(JavaTypeResolver.kt:104)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.types.JavaTypeResolver.transformJavaType(JavaTypeResolver.kt:58)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.types.JavaTypeResolver.transformArrayType(JavaTypeResolver.kt:82)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.types.JavaTypeResolver.transformArrayType$default(JavaTypeResolver.kt:67)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.types.JavaTypeResolver.transformJavaType(JavaTypeResolver.kt:59)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.descriptors.LazyJavaClassMemberScope.createAnnotationConstructorParameters(LazyJavaClassMemberScope.kt:769)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.descriptors.LazyJavaClassMemberScope.createDefaultConstructor(LazyJavaClassMemberScope.kt:724)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.descriptors.LazyJavaClassMemberScope.access$createDefaultConstructor(LazyJavaClassMemberScope.kt:63)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.descriptors.LazyJavaClassMemberScope$constructors$1.invoke(LazyJavaClassMemberScope.kt:105)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.descriptors.LazyJavaClassMemberScope$constructors$1.invoke(LazyJavaClassMemberScope.kt:83)
        at kotlin.reflect.jvm.internal.impl.storage.LockBasedStorageManager$LockBasedLazyValue.invoke(LockBasedStorageManager.java:408)
        at kotlin.reflect.jvm.internal.impl.storage.LockBasedStorageManager$LockBasedNotNullLazyValue.invoke(LockBasedStorageManager.java:527)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.descriptors.LazyJavaClassDescriptor.getConstructors(LazyJavaClassDescriptor.kt:146)
        at kotlin.reflect.jvm.internal.impl.load.java.lazy.descriptors.LazyJavaClassDescriptor.getConstructors(LazyJavaClassDescriptor.kt:42)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.AnnotationDeserializer.deserializeAnnotation(AnnotationDeserializer.kt:45)
        at kotlin.reflect.jvm.internal.impl.load.kotlin.BinaryClassAnnotationAndConstantLoaderImpl.loadTypeAnnotation(BinaryClassAnnotationAndConstantLoaderImpl.kt:40)
        at kotlin.reflect.jvm.internal.impl.load.kotlin.BinaryClassAnnotationAndConstantLoaderImpl.loadTypeAnnotation(BinaryClassAnnotationAndConstantLoaderImpl.kt:27)
        at kotlin.reflect.jvm.internal.impl.load.kotlin.AbstractBinaryClassAnnotationLoader.loadTypeAnnotations(AbstractBinaryClassAnnotationLoader.kt:196)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.TypeDeserializer$simpleType$annotations$1.invoke(TypeDeserializer.kt:97)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.TypeDeserializer$simpleType$annotations$1.invoke(TypeDeserializer.kt:96)
        at kotlin.reflect.jvm.internal.impl.storage.LockBasedStorageManager$LockBasedLazyValue.invoke(LockBasedStorageManager.java:408)
        at kotlin.reflect.jvm.internal.impl.storage.LockBasedStorageManager$LockBasedNotNullLazyValue.invoke(LockBasedStorageManager.java:527)
        at kotlin.reflect.jvm.internal.impl.storage.StorageKt.getValue(storage.kt:42)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedAnnotations.getAnnotations(DeserializedAnnotations.kt:28)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedAnnotations.isEmpty(DeserializedAnnotations.kt:30)
        at kotlin.reflect.jvm.internal.impl.types.DefaultTypeAttributeTranslator.toAttributes(TypeAttributeTranslator.kt:28)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.TypeDeserializer.toAttributes(TypeDeserializer.kt:77)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.TypeDeserializer.simpleType(TypeDeserializer.kt:100)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.TypeDeserializer.type(TypeDeserializer.kt:68)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.TypeDeserializer.typeArgument(TypeDeserializer.kt:300)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.TypeDeserializer.simpleType(TypeDeserializer.kt:106)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.TypeDeserializer.type(TypeDeserializer.kt:68)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.MemberDeserializer.valueParameters(MemberDeserializer.kt:340)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.MemberDeserializer.loadConstructor(MemberDeserializer.kt:276)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedClassDescriptor.computePrimaryConstructor(DeserializedClassDescriptor.kt:137)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedClassDescriptor.access$computePrimaryConstructor(DeserializedClassDescriptor.kt:34)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedClassDescriptor$primaryConstructor$1.invoke(DeserializedClassDescriptor.kt:75)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedClassDescriptor$primaryConstructor$1.invoke(DeserializedClassDescriptor.kt:75)
        at kotlin.reflect.jvm.internal.impl.storage.LockBasedStorageManager$LockBasedLazyValue.invoke(LockBasedStorageManager.java:408)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedClassDescriptor.getUnsubstitutedPrimaryConstructor(DeserializedClassDescriptor.kt:141)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedClassDescriptor.computeConstructors(DeserializedClassDescriptor.kt:144)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedClassDescriptor.access$computeConstructors(DeserializedClassDescriptor.kt:34)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedClassDescriptor$constructors$1.invoke(DeserializedClassDescriptor.kt:76)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedClassDescriptor$constructors$1.invoke(DeserializedClassDescriptor.kt:76)
        at kotlin.reflect.jvm.internal.impl.storage.LockBasedStorageManager$LockBasedLazyValue.invoke(LockBasedStorageManager.java:408)
        at kotlin.reflect.jvm.internal.impl.storage.LockBasedStorageManager$LockBasedNotNullLazyValue.invoke(LockBasedStorageManager.java:527)
        at kotlin.reflect.jvm.internal.impl.serialization.deserialization.descriptors.DeserializedClassDescriptor.getConstructors(DeserializedClassDescriptor.kt:152)
        at kotlin.reflect.jvm.internal.KClassImpl.getConstructorDescriptors(KClassImpl.kt:203)
        at kotlin.reflect.jvm.internal.KClassImpl$Data$constructors$2.invoke(KClassImpl.kt:94)
        at kotlin.reflect.jvm.internal.KClassImpl$Data$constructors$2.invoke(KClassImpl.kt:93)
        at kotlin.reflect.jvm.internal.ReflectProperties$LazySoftVal.invoke(ReflectProperties.java:93)
        at kotlin.reflect.jvm.internal.ReflectProperties$Val.getValue(ReflectProperties.java:32)
        at kotlin.reflect.jvm.internal.KClassImpl$Data.getConstructors(KClassImpl.kt:93)
        at kotlin.reflect.jvm.internal.KClassImpl.getConstructors(KClassImpl.kt:238)
        at kotlin.reflect.full.KClasses.getPrimaryConstructor(KClasses.kt:36)
        at org.springframework.beans.BeanUtils$KotlinDelegate.findPrimaryConstructor(BeanUtils.java:853)

This is looking more like a GraalVM bug to me as it appears to have failed to include jakarta.validation.constraints.Pattern$Flag in the native image, despite the use of @Pattern and Pattern$Flag being the type of its flags() attribute.

@wilkinsona
Copy link
Member

wilkinsona commented Jun 27, 2023

it appears to have failed to include jakarta.validation.constraints.Pattern$Flag in the native image

That's not the problem. Changing the main function to reference Pattern$Flag does not help:

fun main(args: Array<String>) {
	println(jakarta.validation.constraints.Pattern.Flag::class)
	runApplication<DemooApplication>(*args)
}

This outputs class jakarta.validation.constraints.Pattern$Flag on startup and still fails with the same UnsupportedOperationException.

Perhaps BeanValidationBeanRegistrationAotProcessor needs to have generated some reflection hints for Bean Validation annotations so that KClasses.getPrimaryConstructor still works for constructors that use those annotations on their parameters?

We'll transfer this to Framework to get some of @sdeleuze's Kotlin expertise.

@bclozel bclozel transferred this issue from spring-projects/spring-boot Jun 27, 2023
@sdeleuze sdeleuze added theme: aot An issue related to Ahead-of-time processing theme: kotlin labels Jun 28, 2023
@sdeleuze sdeleuze self-assigned this Jun 28, 2023
@sdeleuze
Copy link
Contributor

After a deeper look, it looks like a GraalVM bug to me. I am not sure println(jakarta.validation.constraints.Pattern.Flag::class) is supposed to change something from a GraalVM reflection POV, but adding the hint fixes the issue. We could workaround in BeanValidationBeanRegistrationAotProcessor but that would be much more involved that current algorythm and would basically implement the logic that GraalVM should implement OOTB.

GraalVM is supposed to add the related annotation reflection config transitively based on com.demo.search.middle.demo.DemoContext hints, so @KimSeongIl please create an issue on https://github.com/oracle/graal/issues with:

  • A link to this issue
  • Your repro
  • A mention to me (@sdeleuze)

We will see the feedback from GraalVM team.

@sdeleuze sdeleuze closed this as not planned Won't fix, can't repro, duplicate, stale Jul 11, 2023
@sdeleuze sdeleuze added status: declined A suggestion or change that we don't feel we should currently apply for: external-project For an external project and not something we can fix and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Jul 11, 2023
@sdeleuze sdeleuze added status: waiting-for-triage An issue we've not yet triaged and removed status: declined A suggestion or change that we don't feel we should currently apply for: external-project For an external project and not something we can fix labels Aug 24, 2023
@sdeleuze
Copy link
Contributor

@snicoll @bclozel @mhalbritter @wilkinsona It looks like we have been going full circle on this one. GraalVM confirmed this need to be fixed on Spring side, and after a another look I agree.

I had a deeper look on Spring Boot side while debugging, and I think I now understand why BeanValidationBeanRegistrationAotProcessor detects no relevant hint. It does not because it has no knowledge of org.springframework.boot.context.properties.bind.AggregateBinder logic which is able to analyze DemoContext List<Exclude> property, to invoke Hibernate validator on individual Exclude element.

As a consequence, I think this issue should be bring back to Spring Boot side where advanced validation handling like AggregateBinder should have related AOT processor to anticipate what will be needed by Hibernate validator.

Notice that a logic similar to BeanValidationBeanRegistrationAotProcessor will probably be needed, the key point being to process for the repro use case Exclude, not DemoContext.

If some refinements are needed on Spring Framework side like extracting BeanValidationBeanRegistrationAotProcessor logic to be able to apply it on any Class<?> not only RegisteredBean, feel free to create dedicated issue and I will take care of it.

@sdeleuze sdeleuze reopened this Aug 24, 2023
@bclozel bclozel transferred this issue from spring-projects/spring-framework Aug 25, 2023
@mhalbritter mhalbritter added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 5, 2023
@mhalbritter mhalbritter added this to the 3.0.x milestone Sep 5, 2023
@mhalbritter mhalbritter changed the title @Validated in @ConfigurationProperties doesn't work when running in native image @Validated in @ConfigurationProperties for container types doesn't work in a native image Sep 5, 2023
@philwebb philwebb modified the milestones: 3.0.x, 3.1.x Nov 8, 2023
@wilkinsona wilkinsona modified the milestones: 3.1.x, 3.2.x May 20, 2024
@wilkinsona
Copy link
Member

It does not because it has no knowledge of org.springframework.boot.context.properties.bind.AggregateBinder logic which is able to analyze DemoContext List<Exclude> property, to invoke Hibernate validator on individual Exclude element

This validation of Exclude without @Valid is a mistake in the binder that we've corrected in 3.4.

From Spring Boot's perspective, this should work in a native image if @Valid is added to the declaration of the exclude field as this should be enough for AOT processing to cascade the search for constraints down into Exclude as Hibernate Validator itself does. However, it doesn't work as BeanValidationBeanRegistrationAotProcessor doesn't consider cascaded constraints or container element constraints. I've opened spring-projects/spring-framework#33842 so that this can be addressed in Framework.

Once a fix has been made in Framework, we should be able to fix this by documenting in this section that @Valid is also needed in a native image.

@wilkinsona wilkinsona changed the title @Validated in @ConfigurationProperties for container types doesn't work in a native image Document that validation of @ConfigurationProperties in a native image requires @Valid to cascade validation Nov 4, 2024
@wilkinsona wilkinsona added the status: blocked An issue that's blocked on an external project change label Nov 4, 2024
@wilkinsona
Copy link
Member

The Framework change has been made in 6.2 only due to its complexity. As such, there's nothing more that we can do here. Boot 3.4 already documents the need to use @Valid. In the interim, manual hints will be required to use bean validation with container element constraints and/or constraints that rely on cascading.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Nov 19, 2024
@wilkinsona wilkinsona removed this from the 3.2.x milestone Nov 19, 2024
@wilkinsona wilkinsona added for: external-project For an external project and not something we can fix status: superseded An issue that has been superseded by another and removed type: bug A general bug status: blocked An issue that's blocked on an external project change labels Nov 19, 2024
@wilkinsona wilkinsona changed the title Document that validation of @ConfigurationProperties in a native image requires @Valid to cascade validation Cascaded and container element constraints on a @ConfigurationProperties class do not work correctly in a native image Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project For an external project and not something we can fix status: superseded An issue that has been superseded by another theme: aot An issue related to Ahead-of-time processing
Projects
None yet
Development

No branches or pull requests

7 participants