Code review: Matthew Swift

Jean-Noel Rouvignac

20.32.2014 41d93d231b0c81eff8a0c9b301f9ea9818440727

Code review: Matthew Swift

Connections.java, ErrorResultException.java, LDAPConnectionFactory.java, LDAPListener.java, LDAPServer.java, TestCaseUtils.java:
Fixed API/ABI compatibility issues with versions 2.6.0 up to 2.6.5.
However, current change is breaking API/ABI backward with 2.6.6. However, the changes introduced in 2.6.6 should not have been introduced in the first place because they were not backward compatible.

 opendj-ldap-sdk/src/main/java/org/forgerock/opendj/ldap/Connections.java

@@ -34,6 +34,7 @@

import org.forgerock.opendj.ldap.requests.BindRequest;
import org.forgerock.opendj.ldap.requests.SearchRequest;
import org.forgerock.opendj.ldap.requests.UnbindRequest;

import com.forgerock.opendj.ldap.InternalConnection;
import com.forgerock.opendj.util.Validator;
@@ -43,6 +44,9 @@
 * factories and connections.
 */
public final class Connections {

    private static final int DEFAULT_TIMEOUT_IN_SECONDS = 3;

    /**
     * Creates a new authenticated connection factory which will obtain
     * connections using the provided connection factory and immediately perform
@@ -241,6 +245,8 @@
        return new CachedConnectionPool(factory, poolSize, poolSize, 0L, null, null);
    }

    // FIXME We should remove the newHeartBeatConnectionFactory() methods and use a builder instead.

    /**
     * Creates a new heart-beat connection factory which will create connections
     * using the provided connection factory and periodically ping any created
@@ -255,7 +261,30 @@
     *             If {@code factory} was {@code null}.
     */
    public static ConnectionFactory newHeartBeatConnectionFactory(final ConnectionFactory factory) {
        return new HeartBeatConnectionFactory(factory, 10, 3, TimeUnit.SECONDS, null, null);
        return new HeartBeatConnectionFactory(factory, 10, DEFAULT_TIMEOUT_IN_SECONDS, TimeUnit.SECONDS, null, null);
    }

    /**
     * Creates a new heart-beat connection factory which will create connections
     * using the provided connection factory and periodically ping any created
     * connections in order to detect that they are still alive using the
     * specified frequency and the default scheduler.
     *
     * @param factory
     *            The connection factory to use for creating connections.
     * @param interval
     *            The interval between keepalive pings.
     * @param unit
     *            The time unit for the interval between keepalive pings.
     * @return The new heart-beat connection factory.
     * @throws IllegalArgumentException
     *             If {@code interval} was negative.
     * @throws NullPointerException
     *             If {@code factory} or {@code unit} was {@code null}.
     */
    public static ConnectionFactory newHeartBeatConnectionFactory(final ConnectionFactory factory,
            final long interval, final TimeUnit unit) {
        return new HeartBeatConnectionFactory(factory, interval, DEFAULT_TIMEOUT_IN_SECONDS, unit, null, null);
    }

    /**
@@ -294,6 +323,32 @@
     *            The connection factory to use for creating connections.
     * @param interval
     *            The interval between keepalive pings.
     * @param unit
     *            The time unit for the interval between keepalive pings.
     * @param heartBeat
     *            The search request to use for keepalive pings.
     * @return The new heart-beat connection factory.
     * @throws IllegalArgumentException
     *             If {@code interval} was negative.
     * @throws NullPointerException
     *             If {@code factory}, {@code unit}, or {@code heartBeat} was
     *             {@code null}.
     */
    public static ConnectionFactory newHeartBeatConnectionFactory(final ConnectionFactory factory,
            final long interval, final TimeUnit unit, final SearchRequest heartBeat) {
        return new HeartBeatConnectionFactory(factory, interval, DEFAULT_TIMEOUT_IN_SECONDS, unit, heartBeat, null);
    }

    /**
     * Creates a new heart-beat connection factory which will create connections
     * using the provided connection factory and periodically ping any created
     * connections using the specified search request in order to detect that
     * they are still alive.
     *
     * @param factory
     *            The connection factory to use for creating connections.
     * @param interval
     *            The interval between keepalive pings.
     * @param timeout
     *            The heart-beat timeout after which a connection will be marked
     *            as failed.
@@ -324,6 +379,37 @@
     *            The connection factory to use for creating connections.
     * @param interval
     *            The interval between keepalive pings.
     * @param unit
     *            The time unit for the interval between keepalive pings.
     * @param heartBeat
     *            The search request to use for keepalive pings.
     * @param scheduler
     *            The scheduler which should for periodically sending keepalive
     *            pings.
     * @return The new heart-beat connection factory.
     * @throws IllegalArgumentException
     *             If {@code interval} was negative.
     * @throws NullPointerException
     *             If {@code factory}, {@code unit}, or {@code heartBeat} was
     *             {@code null}.
     */
    public static ConnectionFactory newHeartBeatConnectionFactory(final ConnectionFactory factory,
            final long interval, final TimeUnit unit,
              final SearchRequest heartBeat, final ScheduledExecutorService scheduler) {
        return new HeartBeatConnectionFactory(factory, interval, DEFAULT_TIMEOUT_IN_SECONDS, unit, heartBeat,
                scheduler);
    }

    /**
     * Creates a new heart-beat connection factory which will create connections
     * using the provided connection factory and periodically ping any created
     * connections using the specified search request in order to detect that
     * they are still alive.
     *
     * @param factory
     *            The connection factory to use for creating connections.
     * @param interval
     *            The interval between keepalive pings.
     * @param timeout
     *            The heart-beat timeout after which a connection will be marked
     *            as failed.
@@ -628,8 +714,7 @@
    /**
     * Returns an uncloseable view of the provided connection. Attempts to call
     * {@link Connection#close()} or
     * {@link Connection#close(org.forgerock.opendj.ldap.requests.UnbindRequest, String)}
     * will be ignored.
     * {@link Connection#close(UnbindRequest, String)} will be ignored.
     *
     * @param connection
     *            The connection whose {@code close} methods are to be disabled.
@@ -642,10 +727,10 @@
                // Do nothing.
            }

            public void close(org.forgerock.opendj.ldap.requests.UnbindRequest request,
                    String reason) {
            @Override
            public void close(UnbindRequest request, String reason) {
                // Do nothing.
            };
            }
        };
    }


 opendj-ldap-sdk/src/main/java/org/forgerock/opendj/ldap/ErrorResultException.java

@@ -70,6 +70,28 @@
     *             If the provided result code does not represent a failure.
     * @throws NullPointerException
     *             If {@code resultCode} was {@code null}.
     * @deprecated use {@link #newErrorResult(ResultCode, CharSequence)} instead
     */
    @Deprecated
    public static ErrorResultException newErrorResult(ResultCode resultCode,
            String diagnosticMessage) {
        return newErrorResult(resultCode, (CharSequence) diagnosticMessage);
    }

    /**
     * Creates a new error result exception with the provided result code and
     * diagnostic message.
     *
     * @param resultCode
     *            The result code.
     * @param diagnosticMessage
     *            The diagnostic message, which may be empty or {@code null}
     *            indicating that none was provided.
     * @return The new error result exception.
     * @throws IllegalArgumentException
     *             If the provided result code does not represent a failure.
     * @throws NullPointerException
     *             If {@code resultCode} was {@code null}.
     */
    public static ErrorResultException newErrorResult(ResultCode resultCode,
            CharSequence diagnosticMessage) {
@@ -112,6 +134,31 @@
     *             If the provided result code does not represent a failure.
     * @throws NullPointerException
     *             If {@code resultCode} was {@code null}.
     * @deprecated use {@link #newErrorResult(ResultCode, CharSequence, Throwable)} instead
     */
    @Deprecated
    public static ErrorResultException newErrorResult(ResultCode resultCode,
            String diagnosticMessage, Throwable cause) {
        return newErrorResult(resultCode, (CharSequence) diagnosticMessage, cause);
    }

    /**
     * Creates a new error result exception with the provided result code,
     * diagnostic message, and cause.
     *
     * @param resultCode
     *            The result code.
     * @param diagnosticMessage
     *            The diagnostic message, which may be empty or {@code null}
     *            indicating that none was provided.
     * @param cause
     *            The throwable cause, which may be {@code null} indicating that
     *            none was provided.
     * @return The new error result exception.
     * @throws IllegalArgumentException
     *             If the provided result code does not represent a failure.
     * @throws NullPointerException
     *             If {@code resultCode} was {@code null}.
     */
    public static ErrorResultException newErrorResult(ResultCode resultCode,
            CharSequence diagnosticMessage, Throwable cause) {

 opendj-ldap-sdk/src/main/java/org/forgerock/opendj/ldap/LDAPConnectionFactory.java

@@ -29,6 +29,7 @@

import java.net.InetAddress;
import java.net.InetSocketAddress;
import java.net.SocketAddress;

import com.forgerock.opendj.ldap.LDAPConnectionFactoryImpl;
import com.forgerock.opendj.util.StaticUtils;
@@ -53,6 +54,38 @@
     *            The address of the Directory Server.
     * @throws NullPointerException
     *             If {@code address} was {@code null}.
     * @deprecated use {@link #LDAPConnectionFactory(InetSocketAddress)} instead
     */
    @Deprecated
    public LDAPConnectionFactory(final SocketAddress address) {
        this((InetSocketAddress) address, new LDAPOptions());
    }

    /**
     * Creates a new LDAP connection factory which can be used to create LDAP
     * connections to the Directory Server at the provided address.
     *
     * @param address
     *            The address of the Directory Server.
     * @param options
     *            The LDAP options to use when creating connections.
     * @throws NullPointerException
     *             If {@code address} or {@code options} was {@code null}.
     * @deprecated use {@link #LDAPConnectionFactory(InetSocketAddress, LDAPOptions)} instead
     */
    @Deprecated
    public LDAPConnectionFactory(final SocketAddress address, final LDAPOptions options) {
        this((InetSocketAddress) address, options);
    }

    /**
     * Creates a new LDAP connection factory which can be used to create LDAP
     * connections to the Directory Server at the provided address.
     *
     * @param address
     *            The address of the Directory Server.
     * @throws NullPointerException
     *             If {@code address} was {@code null}.
     */
    public LDAPConnectionFactory(final InetSocketAddress address) {
        this(address, new LDAPOptions());
@@ -117,7 +150,7 @@
     *         or {@code null} if it is unknown.
     */
    public InetAddress getAddress() {
        return getSocketAddress().getAddress();
        return getInetSocketAddress().getAddress();
    }

    @Override
@@ -143,9 +176,23 @@
     * will not perform a reverse DNS lookup.
     *
     * @return The host name that this LDAP listener is listening on.
     * @deprecated use {@link #getHostName()} instead
     */
    @Deprecated
    public String getHostname() {
        return getHostName();
    }

    /**
     * Returns the host name that this LDAP listener is listening on. The
     * returned host name is the same host name that was provided during
     * construction and may be an IP address. More specifically, this method
     * will not perform a reverse DNS lookup.
     *
     * @return The host name that this LDAP listener is listening on.
     */
    public String getHostName() {
        return StaticUtils.getHostName(getSocketAddress());
        return StaticUtils.getHostName(getInetSocketAddress());
    }

    /**
@@ -154,7 +201,7 @@
     * @return The port that this LDAP listener is listening on.
     */
    public int getPort() {
        return getSocketAddress().getPort();
        return getInetSocketAddress().getPort();
    }

    /**
@@ -162,7 +209,11 @@
     *
     * @return The address that this LDAP listener is listening on.
     */
    public InetSocketAddress getSocketAddress() {
    public SocketAddress getSocketAddress() {
        return getInetSocketAddress();
    }

    private InetSocketAddress getInetSocketAddress() {
        return impl.getSocketAddress();
    }


 opendj-ldap-sdk/src/main/java/org/forgerock/opendj/ldap/LDAPListener.java

@@ -31,6 +31,7 @@
import java.io.IOException;
import java.net.InetAddress;
import java.net.InetSocketAddress;
import java.net.SocketAddress;

import com.forgerock.opendj.ldap.LDAPListenerImpl;
import com.forgerock.opendj.util.StaticUtils;
@@ -155,6 +156,28 @@
     *             address.
     * @throws NullPointerException
     *             If {@code address} or {code factory} was {@code null}.
     * @deprecated use {@link #LDAPListener(InetSocketAddress, ServerConnectionFactory)} instead
     */
    @Deprecated
    public LDAPListener(final SocketAddress address,
            final ServerConnectionFactory<LDAPClientContext, Integer> factory) throws IOException {
        this((InetSocketAddress) address, factory, new LDAPListenerOptions());
    }

    /**
     * Creates a new LDAP listener implementation which will listen for LDAP
     * client connections at the provided address.
     *
     * @param address
     *            The address to listen on.
     * @param factory
     *            The server connection factory which will be used to create
     *            server connections.
     * @throws IOException
     *             If an error occurred while trying to listen on the provided
     *             address.
     * @throws NullPointerException
     *             If {@code address} or {code factory} was {@code null}.
     */
    public LDAPListener(final InetSocketAddress address,
            final ServerConnectionFactory<LDAPClientContext, Integer> factory) throws IOException {
@@ -178,6 +201,32 @@
     * @throws NullPointerException
     *             If {@code address}, {code factory}, or {@code options} was
     *             {@code null}.
     * @deprecated use {@link #LDAPListener(InetSocketAddress, ServerConnectionFactory, LDAPListenerOptions)} instead
     */
    @Deprecated
    public LDAPListener(final SocketAddress address,
            final ServerConnectionFactory<LDAPClientContext, Integer> factory,
            final LDAPListenerOptions options) throws IOException {
        this((InetSocketAddress) address, factory, options);
    }

    /**
     * Creates a new LDAP listener implementation which will listen for LDAP
     * client connections at the provided address.
     *
     * @param address
     *            The address to listen on.
     * @param factory
     *            The server connection factory which will be used to create
     *            server connections.
     * @param options
     *            The LDAP listener options.
     * @throws IOException
     *             If an error occurred while trying to listen on the provided
     *             address.
     * @throws NullPointerException
     *             If {@code address}, {code factory}, or {@code options} was
     *             {@code null}.
     */
    public LDAPListener(final InetSocketAddress address,
            final ServerConnectionFactory<LDAPClientContext, Integer> factory,
@@ -250,7 +299,21 @@
     * @return The {@code InetAddress} that this LDAP listener is listening on.
     */
    public InetAddress getAddress() {
        return getSocketAddress().getAddress();
        return getInetSocketAddress().getAddress();
    }

    /**
     * Returns the host name that this LDAP listener is listening on. The
     * returned host name is the same host name that was provided during
     * construction and may be an IP address. More specifically, this method
     * will not perform a reverse DNS lookup.
     *
     * @return The host name that this LDAP listener is listening on.
     * @deprecated use {@link #getHostName()} instead
     */
    @Deprecated
    public String getHostname() {
        return getHostName();
    }

    /**
@@ -262,7 +325,7 @@
     * @return The host name that this LDAP listener is listening on.
     */
    public String getHostName() {
        return StaticUtils.getHostName(getSocketAddress());
        return StaticUtils.getHostName(getInetSocketAddress());
    }

    /**
@@ -271,7 +334,7 @@
     * @return The port that this LDAP listener is listening on.
     */
    public int getPort() {
        return getSocketAddress().getPort();
        return getInetSocketAddress().getPort();
    }

    /**
@@ -279,7 +342,11 @@
     *
     * @return The address that this LDAP listener is listening on.
     */
    public InetSocketAddress getSocketAddress() {
    public SocketAddress getSocketAddress() {
        return getInetSocketAddress();
    }

    private InetSocketAddress getInetSocketAddress() {
        return impl.getSocketAddress();
    }


 opendj-ldap-sdk/src/test/java/org/forgerock/opendj/ldap/LDAPServer.java

@@ -30,7 +30,7 @@
import static com.forgerock.opendj.ldap.LDAPConstants.TYPE_AUTHENTICATION_SASL;

import java.io.IOException;
import java.net.InetSocketAddress;
import java.net.SocketAddress;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
@@ -563,7 +563,7 @@
     *
     * @return The socket address of the server.
     */
    public synchronized InetSocketAddress getSocketAddress() {
    public synchronized SocketAddress getSocketAddress() {
        if (!isRunning) {
            throw new IllegalStateException("Server is not running");
        }

 opendj-ldap-sdk/src/test/java/org/forgerock/opendj/ldap/TestCaseUtils.java

@@ -129,7 +129,7 @@
     *
     * @return The socket address of the server.
     */
    public static InetSocketAddress getServerSocketAddress() {
    public static SocketAddress getServerSocketAddress() {
        return LDAPServer.getInstance().getSocketAddress();
    }