-
-
Notifications
You must be signed in to change notification settings - Fork 387
Spring Native Support added #1421
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
Spring Native Support added #1421
Conversation
| "java.util.ArrayList", | ||
|
|
||
| // Package: org.javers.core | ||
| "org.javers.core.Javers", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
hardcoding all class names seems fragile, what if we change something?
-
Could you please explain (or point to some docs) why do we need to register all these names to achieve Spring Native support
-
how this move affects classic jvm apps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi bartoszwalacik,
thx for taking your time
hardcoding class names is unfortunately exactly how native hints work ... there is basically no other alternative than this.
You need to tell the compiler upfront which classes need to be registered for reflection, so that they are prepared during compilation.
If classes are changed a Runtime Exception will be thrown as the Class.Forname fails then.
Normally classes are listed as Classes, and not as literals, which would automatically change upon Renaming the class.
Unfortunately Javers uses a lot of Package protected classes, that are not importable this way.
See 1. .. the compiler needs to be told upfront which classes use reflection and all of these classes do so.
https://www.baeldung.com/spring-native-intro
It should not affect jvm apps at all.
The JaversNativeCOnfiguration should just be picked up by any external project that uses spring aot.
As javers does not seem to use the aot plugin at all, apps and javers itself without aot processing will not be touched in any ways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goafabric thanks for your answer, still not sure I fully understand it, most Javers' classes are , well, just normal classes, only a few of them, or maybe only the one -- ReflectionUtil -- uses reflection API, so why registering them all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concerning 3. , from what I see, this RuntimeHintsRegistrar is executed unconditionally
so it will slow down starting Javers for normal JVM apps. Could you please come up with some condition, end execute it only when needed by a native image app? might is also be an alternative starter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RuntimeRegistrar is only executed during build time, never during run time.
You may validate that by setting a break point.
And then also only if the plugin "org.graalvm.buildtools.native" is inside the build.
Concerning Reflection, it is less about classes that use ReflectionUtil and more about which classes are accessed reflectively.
This will be e.g. every ClassForname(class) and every field, method, constructor etc that will be executed via reflection.
The list was initially generated the graalvm agent.
And after that you have to add manually further classes the agent did not catch.
--- example---
class A() {
doReflectionStuff() {
ReflectionUtil.doSomething(B.class)
ReflectionUtil.doSomething(C.class)
ReflectionUtil.doSomething(D.class)
}
}
=> needs reflection hints for B,C + D, not for A
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes good idea, have done so by removing the jpa/sql specifics and duplicating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, maybe we can move this large array of class names to the javers-spring package and share for two starters (no duplication then). and just a question, how can I refresh this array of class names when sth changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the duplication is currently intentional, as both classes differ a little
and more deviation could happen in the future
honestly i've no done a lot of changes, and every roundtrip is rather expensive .. so my suggestion would be to get the things merged and then later on improve in the future
concerning the fresh of the classnames, if classes are renamed or removed, the test will indicate
And you may have to manually adapt the array ... unless your IDE does (idea is party capable of that)
if classes need to be added, someone else (like me) might need to take care, as it will not become evident
but that is the same with existing frameworks like hibernate and flyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I took the extra time for the refactoring .. I hope we can now getting it merged ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @goafabric , well done, I will merge and release soon
|
builds are visible here master...spring-boot-native-configuration |
|
issue #1295 |
|
released in 7.7.0 |
This PR adds Spring Boot Native Support
We need to add the Native hints, with reflection during build time due to Classes being package protected.
And we need to update the JpaHibernateConnectionProvider,
because we will get a Nullpointer on EntityManager otherwise