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

GeneratedFiles#addSource does not provide proper context if the specified class name is invalid #31612

Conversation

PiotrFLEURY
Copy link
Contributor

Context

I tried to build native Spring boot artifact and spent hours to struggle with build failed obscure error. I was not able to spot the build failed root cause because of the too generic error message 'className' must be a valid identifier

This error message prevents me to fix the problem

I did not found any related issue to this specific point.

Observation

GeneratedFiles.getClassNamePath(String className) error message is too generic

Exception in thread "main" java.lang.IllegalArgumentException: 'className' must be a valid identifier
	at org.springframework.util.Assert.isTrue(Assert.java:122)
	at org.springframework.aot.generate.GeneratedFiles.getClassNamePath(GeneratedFiles.java:164)

Proposal

Put className value in the error message

Exception in thread "main" java.lang.IllegalArgumentException: 'className' com/example/HelloWorld.java must be a valid identifier
	at org.springframework.util.Assert.isTrue(Assert.java:122)
	at org.springframework.aot.generate.GeneratedFiles.getClassNamePath(GeneratedFiles.java:164)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 15, 2023
@PiotrFLEURY PiotrFLEURY force-pushed the className-valid-identifier-message branch from caf6486 to 67c7e27 Compare November 15, 2023 18:23
@pivotal-cla
Copy link

@PiotrFLEURY Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@PiotrFLEURY PiotrFLEURY force-pushed the className-valid-identifier-message branch from 67c7e27 to 8115c43 Compare November 15, 2023 18:24
@pivotal-cla
Copy link

@PiotrFLEURY Thank you for signing the Contributor License Agreement!

@snicoll
Copy link
Member

snicoll commented Nov 15, 2023

@PiotrFLEURY thanks for the PR. Can you share how you ended up in that situation?

@snicoll snicoll added type: bug A general bug theme: aot An issue related to Ahead-of-time processing and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 15, 2023
@snicoll snicoll self-assigned this Nov 15, 2023
@snicoll snicoll added this to the 6.0.14 milestone Nov 15, 2023
@snicoll snicoll changed the title Add className value to error message GeneratedFiles#addSource does not provide proper context if the specified class name is invalid Nov 15, 2023
snicoll pushed a commit that referenced this pull request Nov 15, 2023
@snicoll snicoll closed this in 51cdff5 Nov 15, 2023
@snicoll
Copy link
Member

snicoll commented Nov 15, 2023

Thank you for making your first contribution to Spring Framework, @PiotrFLEURY. Interested to understand more what caused this as there might be something else that needs fixing to prevent that from happening.

@PiotrFLEURY
Copy link
Contributor Author

Thank you for making your first contribution to Spring Framework, @PiotrFLEURY. Interested to understand more what caused this as there might be something else that needs fixing to prevent that from happening.

Thanks to you @snicoll for accepting my contribution.

The pull request was closed but I see that you merged polished modification on main branch. Thanks for the polishing.
Never saw this kind of flow before. Just for my curiosity, can you explain how did you that ?

I will give more details as soon as I fully understand what was the root cause of the build problem.

@PiotrFLEURY
Copy link
Contributor Author

@PiotrFLEURY thanks for the PR. Can you share how you ended up in that situation?

I finally spotted the root cause thanks to this new detailed message 🚀

It can easily reproduced.

Steps to reproduce

start.spring.io

Generates a new Spring boot project using Maven + Kotlin + Graalvm native support.

Required versions

spring-boot-starter-parent:3.1.5
java:17
kotlin:1.8.22

Code

Create a data class named MyConfig and ommit package directive

import org.springframework.boot.context.properties.ConfigurationProperties
import org.springframework.context.annotation.Configuration

@Configuration
@ConfigurationProperties(prefix = "config")
data class MyConfig(
    var apps: Map<String, String>
)

Now add @EnableConfigurationProperties annotation to the SpringBoot application class and import a config class

package com.example.demokotlinnative

import MyConfig
import org.springframework.boot.autoconfigure.SpringBootApplication
import org.springframework.boot.context.properties.EnableConfigurationProperties
import org.springframework.boot.runApplication

@SpringBootApplication
@EnableConfigurationProperties(MyConfig::class)
class DemoKotlinNativeApplication

fun main(args: Array<String>) {
	runApplication<DemoKotlinNativeApplication>(*args)
}

Try build native image

Run mvn -Pnative -DskipTests native:compile

Issue

Spring aot processor fails with the following message

Exception in thread "main" java.lang.IllegalArgumentException: 'className' must be a valid identifier, got '.MyConfig__BeanDefinitions'
	at org.springframework.util.Assert.isTrue(Assert.java:122)
	at org.springframework.aot.generate.GeneratedFiles.getClassNamePath(GeneratedFiles.java:164)

Expected behavior

As a missing package directive is a warning should the build pass with warning ?

Or should it fail with a dedicated message asking to fix the missing package directive ?

Everything works fine with the package directive inside the MyConfig class

Hope this will help @snicoll

Feel free to ask for any further info

@snicoll
Copy link
Member

snicoll commented Nov 20, 2023

Thanks @PiotrFLEURY. This is largely a Kotlin-specific problem as Java does not allow to import classes from the default package. I've reproduced based on your snippet above, please watch #31628 for updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: aot An issue related to Ahead-of-time processing type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants