Build modules using Java 8 #10817
Build modules using Java 8 #10817
Conversation
marcusdacoregio
added
status: duplicate
marcusdacoregio
changed the title
| @@ -130,3 +129,33 @@ tasks.withType(KotlinCompile).configureEach { | |||
| } | |||
|
|
|||
| build.dependsOn rncToXsd | |||
|
|
|||
| compileTestJava { | |||
There was a problem hiding this comment.
Is it true that we don't want to run these tests against JDK 8? It seems to me that we want to run the tests twice: Once for JDK 8 and once for JDK 11.
There was a problem hiding this comment.
We want to run they using JDK 11 only because they are using OpenSAML4 classes
| } | ||
| else { | ||
| return new OpenSamlAuthenticationRequestFactory(); | ||
| if (ClassUtils.isPresent("org.opensaml.core.xml.persist.impl.PassthroughSourceStrategy", |
There was a problem hiding this comment.
I wonder if it would still be appropriate to have a version() method for readability. It would simply determine the version via ClassUtils.isPresent.
| else { | ||
| return new OpenSamlAuthenticationRequestFactory(); | ||
| if (ClassUtils.isPresent("org.opensaml.core.xml.persist.impl.PassthroughSourceStrategy", | ||
| Version.class.getClassLoader())) { |
There was a problem hiding this comment.
It feels a little odd to be calling for Version's classloader given that we aren't working with the Version class. I wonder if passing null here would be better.
| } | ||
| return this.logoutResponseResolver; | ||
| if (ClassUtils.isPresent("org.opensaml.core.xml.persist.impl.PassthroughSourceStrategy", |
There was a problem hiding this comment.
It might be more readable to have one place for all the decision logic. Right now there is some here and some in OpenSaml4LogoutSupportFactory
| @@ -519,4 +512,38 @@ public void logout(HttpServletRequest request, HttpServletResponse response, Aut | |||
|
|
|||
| } | |||
|
|
|||
| private static class OpenSaml4LogoutSupportFactory { | |||
There was a problem hiding this comment.
Is this class still necessary? I think it was necessary when it was acting as a classpath guard. However, since you are using reflection now, I don't think this class offers this same benefit.
There was a problem hiding this comment.
Those classes make the code cleaner by moving the try-catch from the reflective operations to another method
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
| import sun.security.x509.X500Name; |
There was a problem hiding this comment.
Can you clarify how these changes pertain to the PR?
There was a problem hiding this comment.
Those changes came from the revert that I did to the old commit, where we weren't using toolchain to build the modules
marcusdacoregio
force-pushed
the
gradle-toolchain
branch
2 times, most recently
from
efbda48 to
5f22dfd
Compare
marcusdacoregio
force-pushed
the
gradle-toolchain
branch
from
5f22dfd to
45769fe
Compare
This reverts commit 60ed360.
This causes compile errors when trying to build using JDK 8 Issue spring-projectsgh-10695
Because the OpenSAML4 classes are compiled using Java 11, we have to rely on reflection to instante those classes since the config module should be compatible with Java 8 Issue spring-projectsgh-10816
marcusdacoregio
force-pushed
the
gradle-toolchain
branch
from
45769fe to
4fbf9dd
Compare
This PR does the following:
OAuth2ResourceServerConfigurermember variable using Java 9+ feature #10695 by creating astaticmethod to initialize the handlers;Saml2Login/LogoutConfigurerTestsare using some OpenSAML4 classes, these tests have to be compiled using Java 11. For this, those tests were removed from thecompileTestJavatask and a new Gradle taskcompileSaml2TestJavais being used to compile them;saml2Testsis being used to run the tests compiled bycompileSaml2TestJava.ModuleDescriptorto retrieve the OpenSAML version, now we are checking if the classorg.opensaml.core.xml.persist.impl.PassthroughSourceStrategyexists in the classpath. This class was introduced in OpenSAML4.