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

Upgrade to Spring 6.1 (and Java 17 minimum) #625

Merged
merged 6 commits into from
Nov 28, 2024
Merged

Upgrade to Spring 6.1 (and Java 17 minimum) #625

merged 6 commits into from
Nov 28, 2024

Conversation

ato
Copy link
Collaborator

@ato ato commented Nov 20, 2024

Spring 5 is no longer receiving open source security updates. Spring 6 requires Java 17 so we may as well set that as the new source version level.

The @Required annotation has been removed and they suggest using constructor injection instead. But changing to that would break existing Heritrix crawl XML files. We could just live without it but it's nice having the error message enforced at config load time rather than risking the job crashing with a more cryptic error later so I reimplemented an equivalent annotation, but marked deprecated to avoid in new beans.

ato added 3 commits November 20, 2024 15:31
Spring 6 removed @required and they suggest using constructor injection instead. If we switched our beans to that we'd break existing Heritrix crawl configs. So this change implements our own basic version so we still get errors when a @required property is null.
@ato
Copy link
Collaborator Author

ato commented Nov 24, 2024

The unit tests work but I ran into two issues so far while testing:

  1. When Heritrix is compiled with Java source version 17, Kryo fails with java.lang.IllegalAccessException: class org.objenesis.instantiator.basic.ObjectStreamClassInstantiator cannot access a member of class java.io.ObjectStreamClass due to the new stricter encapsulation. This error can be worked around with the JVM option --add-opens java.base/java.io=ALL-UNNAMED. I guess the proper fix is to stop using SerializingInstantiatorStrategy. I'm not sure yet how difficult that would be.
  2. The default xml config file has two defined beans that are subclasses of DecideRule: scope and acceptSurts. It seems AbstractFrontier and CandidateScoper have @Autowired setScope(DecideRule scope) methods and the new version of Spring apparently doesn't know which bean to inject. It seems you can break the tie by referencing the bean id explicitly with @Qualifier("scope").

ato added 3 commits November 24, 2024 18:38
This is needed for Spring 6.1 to be able to reflect on parameter names. We have some autowiring that relies on parameter names such as `AbstractFrontier.setScope(DecideRule scope)`. Without this Spring doesn't know whether to inject the `scope` or `acceptSurts` beans as they both subclass DecideRule. Apparently older versions of Spring used to discover method names by parsing bytecode but this was mechanism removed in 6.1.
…rStrategy

SerializingInstantiatorStrategy when compiled for Java 17 fails when running without `--add-opens java.base/java.io=ALL-UNNAMED`. StdInstantiatorStrategy seems to work OK though.
@ato
Copy link
Collaborator Author

ato commented Nov 24, 2024

  1. Using StdInstantiatorStrategy instead of SerializingInstantiatorStrategy seems to be another solution. It seems to be sufficiently equivalent for our purposes.
  2. The Spring 6.1 release notes explains this and recommends compiling with -parameters and that seems to resolve the problem without needing to use @Qualifier. I went with that since it would be hard to determine if there are any non-default beans relying autowiring using parameter names.
  3. Browse Beans broke again, due to Logger infinitely looping and a different exception with ConcurrentHashMap. I changed the code so it avoids recursing into Logger and treats Maps and Lists only as containers, not as beans.

@ato ato merged commit 8ec6142 into master Nov 28, 2024
4 checks passed
@ato ato deleted the spring-6.1 branch November 28, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant