Rework resolving the default for Context.INITIAL_CONTEXT_FACTORY

[iherasymenko]

A follow up for #568, #570, #543.

With this approach in place, one will be able to specify a different implementation using the setContextFactory method in case com.sun.jndi.ldap.LdapCtxFactory gets removed one day (since this class is not exported by the java.naming modules in JDK 9+ and is an implementation detail).

Having the contextFactory field initialized as follows would lead to the ExceptionInInitializerError which users would not be able to overcome.
private static final Class<?> DEFAULT_CONTEXT_FACTORY = org.springframework.util.ClassUtils.resolveClassName("com.sun.jndi.ldap.LdapCtxFactory", null)

[Yberion]

Hello,

Will this PR fix Java 16 error ? (I'm using gradle 7)

SbtemplateApplicationTests > contextLoads() FAILED
    java.lang.IllegalStateException at DefaultCacheAwareContextLoaderDelegate.java:132
        Caused by: org.springframework.beans.factory.BeanCreationException at ConstructorResolver.java:658
            Caused by: org.springframework.beans.BeanInstantiationException at SimpleInstantiationStrategy.java:185
                Caused by: java.lang.IllegalAccessError at AbstractContextSource.java:77

I tried few versions and this error appear on every of them :

• 2.3.9.BUILD-SNAPSHOT
• 2.4.5-SNAPSHOT
• 2.4.6-SNAPSHOT
• 2.5.0-SNAPSHOT

Thank you.

[iherasymenko]

@Yberion yes, it will.

You can try out a snapshot build from jitpack.

https://jitpack.io/#iherasymenko/spring-ldap/java-9-and-above-compatibility-2.3.2.RELEASE-g87d79f1-49

[bryant1410]

?

[iherasymenko]

@bryant1410 sorry, I tagged you accidentally.

[Yberion]

Thank you for the answer.

repositories {
	...
	maven {
	    url 'https://jitpack.io'
    }
}

dependencies {
        ...
	implementation 'com.github.iherasymenko:spring-ldap:java-9-and-above-compatibility-SNAPSHOT'
}

Fixed it, waiting for the official release ! :)

[jzheaux]

Thanks for the PR, @iherasymenko! Please see my comments inline.

[jzheaux]

This isn't necessarily an error since the application may call setContextFactory later. Would this work be better in the afterPropertiesSet method? Then, you would be able to state definitively that the class is correctly or incorrectly configured and throw an exception accordingly.

[iherasymenko]

I changed the logging level to TRACE and added the validation to the afterPropertiesSet method. I did not completely move the resolution to the afterPropertiesSet method since having it there would result in the same behavior change described here.

[jzheaux]

Could you elaborate on how the same behavior would result? It would be an error to use this class before calling afterPropertiesSet.

[iherasymenko]

elaborated below.

[jzheaux]

Thanks for the quick turn around, @iherasymenko! Please see an additional comment in my review.

[jzheaux]

I think this should be moved into afterPropertiesSet, too, so that it's all in one place.

For example, in afterPropertiesSet you would do:

if (this.contextFactory == null) {
    try {
        this.contextFactory = Class.forName(DEFAULT_CONTEXT_FACTORY);
    } catch (ClassNotFoundException e) {
        throw new IllegalArgumentException("Could not find " + DEFAULT_CONTEXT_FACTORY + " and no context factory specified");
    }
}

One thing that's nice about this is that the ClassNotFoundException is not generated unless no context factory is specified. Since exceptions are expensive to generate, it's nice when they can be avoided.

[iherasymenko]

That was my original idea, yes. However, there will be a slight behavioral change compared to 2.3.3.

LdapContextSource ctx = new LdapContextSource();
// afterPropertiesSet is not required.
Class<?> contextFactory = ctx.getContextFactory(); // Version 2.3.3 would return com.sun.jndi.ldap.LdapCtxFactory

whereas with the change applied

LdapContextSource ctx = new LdapContextSource();
ctx.afterPropertiesSet(); // required otherwise the below will return a null value
Class<?> contextFactory = ctx.getContextFactory(); 

Please let me know if that is acceptable and I will update the PR.

[jzheaux]

Would it alleviate your concern if the PR also changed to run this logic in a private getter?

For example, you could add:

private Class<?> ensureContextFactoryName() {
    if (this.contextFactory != null) {
        return this.contextFactory.getName();
    }
    try {
        this.contextFactory = Class.forName(DEFAULT_CONTEXT_FACTORY);
    } catch (ClassNotFoundException e) {
        throw new IllegalArgumentException("Could not find " + DEFAULT_CONTEXT_FACTORY + " and no context factory specified");
    }
    return this.contextFactory.getName();
}

Then, in afterPropertiesSet, you'd do:

ensureContextFactoryName();

And in setupAnonymousEnv:

env.put(Context.INITIAL_CONTEXT_FACTORY, ensureContextFactoryName());

[rosti-il]

Why not just initialize the DEFAULT_CONTEXT_FACTORY in the static code block and do not make any other changes?

    private static final Class<?> DEFAULT_CONTEXT_FACTORY;
    static {
        try {
            DEFAULT_CONTEXT_FACTORY = Class.forName("com.sun.jndi.ldap.LdapCtxFactory");
        } catch (ClassNotFoundException e) {
            throw new RuntimeException(e);
        }
    }

[jzheaux]

That's problematic since it errors on something that may not be a problem. The class not being present is only a problem if the application does not set a context factory.

[rosti-il]

@jzheaux The com.sun.jndi.ldap.LdapCtxFactory class is always present but not always could be accessed. The ClassNotFoundException will never be thrown and catching it is just because it's a checked exception. In other words the DEFAULT_CONTEXT_FACTORY will always be present and so will be set the contextFactory field by default as well.
Following code example works on Java 16 properly:

public class MainClass {
    private static final Class<?> DEFAULT_CONTEXT_FACTORY;
    static {
        try {
            DEFAULT_CONTEXT_FACTORY = Class.forName("com.sun.jndi.ldap.LdapCtxFactory");
        } catch (ClassNotFoundException e) {
            throw new RuntimeException(e);
        }
    }

    public static void main(String[] args) {
        System.out.println(DEFAULT_CONTEXT_FACTORY.getName());
    }
}

Value of the DEFAULT_CONTEXT_FACTORY is used in the AbstractContextSource only for two things:

1. return value of getContextFactory()
2. env.put(Context.INITIAL_CONTEXT_FACTORY, contextFactory.getName());

With my version of change all of the above will continue to work properly and no behavior change will be introduced.

[jzheaux]

I see. It's interesting that Class.forName works when the direct reference does not. Given that, the PR's constructor won't throw the exception in normal JDK 16 circumstances.

[jzheaux]

Thanks, @iherasymenko for the PR! Based on this comment, it appears that the extra work to delay the thrown exception is needless.

I squashed your two commits adjusted the copyright header to be 2021, resulting in 0f30b05