Skip to content

Rework resolving the default for Context.INITIAL_CONTEXT_FACTORY #579

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Rework resolving the default for Context.INITIAL_CONTEXT_FACTORY #579

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e18e7b
Rework resolving the default for Context.INITIAL_CONTEXT_FACTORY so t…
Apr 16, 2021
aefc836
Polishing
Apr 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions core/src/main/java/org/springframework/ldap/core/support/AbstractContextSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,16 @@ public abstract class AbstractContextSource implements BaseLdapPathContextSource

public static final String SUN_LDAP_POOLING_FLAG = "com.sun.jndi.ldap.connect.pool";

private static final String JDK_142 = "1.4.2";

private DirContextAuthenticationStrategy authenticationStrategy = new SimpleDirContextAuthenticationStrategy();

public AbstractContextSource() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
        }
    }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try {
this.contextFactory = Class.forName(DEFAULT_CONTEXT_FACTORY);
} catch (ClassNotFoundException e) {
LOG.trace("The default for contextFactory cannot be resolved", e);
}
}

public DirContext getContext(String principal, String credentials) {
// This method is typically called for authentication purposes, which means that we
// should explicitly disable pooling in case passwords are changed (LDAP-183).
Expand Down Expand Up @@ -407,7 +413,9 @@ public void afterPropertiesSet() {
if (ObjectUtils.isEmpty(urls)) {
throw new IllegalArgumentException("At least one server url must be set");
}

if (contextFactory == null) {
throw new IllegalArgumentException("contextFactory must be set");
}
if (authenticationSource == null) {
LOG.debug("AuthenticationSource not set - " + "using default implementation");
if (!StringUtils.hasText(userDn)) {
Expand Down Expand Up @@ -438,7 +446,7 @@ private Hashtable<String, Object> setupAnonymousEnv() {

Hashtable<String, Object> env = new Hashtable<String, Object>(baseEnv);

env.put(Context.INITIAL_CONTEXT_FACTORY, contextFactory != null ? contextFactory.getName() : DEFAULT_CONTEXT_FACTORY);
env.put(Context.INITIAL_CONTEXT_FACTORY, contextFactory.getName());
env.put(Context.PROVIDER_URL, assembleProviderUrlString(urls));

if (dirObjectFactory != null) {
Expand Down