Skip to content

Add ability to reset password in JedisFactory #1606

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 1 commit into from
Closed

Add ability to reset password in JedisFactory #1606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 11 additions & 7 deletions src/main/java/redis/clients/jedis/JedisFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,19 @@
/**
* PoolableObjectFactory custom impl.
*/
class JedisFactory implements PooledObjectFactory<Jedis> {
public class JedisFactory implements PooledObjectFactory<Jedis> {
private final AtomicReference<HostAndPort> hostAndPort = new AtomicReference<HostAndPort>();
private final AtomicReference<String> password = new AtomicReference<String>();
private final int connectionTimeout;
private final int soTimeout;
private final String password;
private final int database;
private final String clientName;
private final boolean ssl;
private final SSLSocketFactory sslSocketFactory;
private final SSLParameters sslParameters;
private final HostnameVerifier hostnameVerifier;

JedisFactory(final String host, final int port, final int connectionTimeout,
public JedisFactory(final String host, final int port, final int connectionTimeout,
final int soTimeout, final String password, final int database, final String clientName) {
this(host, port, connectionTimeout, soTimeout, password, database, clientName,
false, null, null, null);
Expand All @@ -43,7 +43,7 @@ class JedisFactory implements PooledObjectFactory<Jedis> {
this.hostAndPort.set(new HostAndPort(host, port));
this.connectionTimeout = connectionTimeout;
this.soTimeout = soTimeout;
this.password = password;
this.password.set(password);
this.database = database;
this.clientName = clientName;
this.ssl = ssl;
Expand All @@ -68,7 +68,7 @@ class JedisFactory implements PooledObjectFactory<Jedis> {
this.hostAndPort.set(new HostAndPort(uri.getHost(), uri.getPort()));
this.connectionTimeout = connectionTimeout;
this.soTimeout = soTimeout;
this.password = JedisURIHelper.getPassword(uri);
this.password.set(JedisURIHelper.getPassword(uri));
this.database = JedisURIHelper.getDBIndex(uri);
this.clientName = clientName;
this.ssl = JedisURIHelper.isRedisSSLScheme(uri);
Expand All @@ -81,6 +81,10 @@ public void setHostAndPort(final HostAndPort hostAndPort) {
this.hostAndPort.set(hostAndPort);
}

public void setPassword(final String password) {
this.password.set(password);
}

@Override
public void activateObject(PooledObject<Jedis> pooledJedis) throws Exception {
final BinaryJedis jedis = pooledJedis.getObject();
Expand Down Expand Up @@ -115,8 +119,8 @@ public PooledObject<Jedis> makeObject() throws Exception {

try {
jedis.connect();
if (password != null) {
jedis.auth(password);
if (password.get() != null) {
jedis.auth(password.get());
}
if (database != 0) {
jedis.select(database);
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/redis/clients/jedis/JedisPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import javax.net.ssl.SSLParameters;
import javax.net.ssl.SSLSocketFactory;

import org.apache.commons.pool2.PooledObjectFactory;
import org.apache.commons.pool2.impl.GenericObjectPool;
import org.apache.commons.pool2.impl.GenericObjectPoolConfig;

Expand Down Expand Up @@ -228,6 +229,10 @@ public JedisPool(final GenericObjectPoolConfig poolConfig, final URI uri,
super(poolConfig, new JedisFactory(uri, connectionTimeout, soTimeout, null, sslSocketFactory,
sslParameters, hostnameVerifier));
}

public JedisPool(GenericObjectPoolConfig poolConfig, PooledObjectFactory<Jedis> factory) {
super(poolConfig, factory);
}

@Override
public Jedis getResource() {
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/redis/clients/jedis/JedisSentinelPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@ public JedisSentinelPool(String masterName, Set<String> sentinels,
initPool(master);
}

public JedisSentinelPool(String masterName, Set<String> sentinels, final GenericObjectPoolConfig poolConfig, final JedisFactory factory) {
this.poolConfig = poolConfig;
this.factory = factory;

HostAndPort master = initSentinels(sentinels, masterName);
initPool(poolConfig, factory);
initPool(master);
}

@Override
public void destroy() {
for (MasterListener m : masterListeners) {
Expand Down
39 changes: 38 additions & 1 deletion src/test/java/redis/clients/jedis/tests/JedisPoolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,12 @@

import redis.clients.jedis.HostAndPort;
import redis.clients.jedis.Jedis;
import redis.clients.jedis.JedisFactory;
import redis.clients.jedis.JedisPool;
import redis.clients.jedis.JedisPoolConfig;
import redis.clients.jedis.Transaction;
import redis.clients.jedis.exceptions.InvalidURIException;
import redis.clients.jedis.exceptions.JedisException;
import redis.clients.jedis.exceptions.JedisConnectionException;
import redis.clients.jedis.exceptions.JedisExhaustedPoolException;

public class JedisPoolTest {
Expand Down Expand Up @@ -381,4 +382,40 @@ private int getClientCount(final String clientList) {
return clientList.split("\n").length;
}

@Test
public void testResetInvalidPassword() {
JedisFactory factory = new JedisFactory(hnp.getHost(), hnp.getPort(), 2000, 2000, "foobared", 0, "my_shiny_client_name");


try(JedisPool pool = new JedisPool(new JedisPoolConfig(), factory);
Jedis obj1 = pool.getResource();){
obj1.set("foo", "bar");
assertEquals("bar", obj1.get("foo"));
assertEquals(1, pool.getNumActive());

factory.setPassword("wrong password");
try (Jedis obj2 = pool.getResource()) {
fail("Should not get resource from pool");
} catch (JedisConnectionException e) {}
}
}

@Test
public void testResetValidPassword() {
JedisFactory factory = new JedisFactory(hnp.getHost(), hnp.getPort(), 2000, 2000, "bad password", 0, "my_shiny_client_name");
JedisPool pool = new JedisPool(new JedisPoolConfig(), factory);
Jedis obj = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

If operation fails in try, obj will be null in finally and will get NPE. Declare directly in catch.

Copy link
Author

Choose a reason for hiding this comment

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

done

try {
pool.getResource();
Copy link
Contributor

Choose a reason for hiding this comment

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

add fail() below pool.getResource()

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Get this resource and close it properly, just to avoid any possible connection leak.

fail("Could not get resource from pool");
} catch (JedisConnectionException e) {
factory.setPassword("foobared");
obj = pool.getResource();
Copy link
Contributor

Choose a reason for hiding this comment

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

Declare obj here.

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Close obj within this catch block. Optionally, you can declare obj (aka Jedis obj = pool.getResource();) here with/without try-with-resources.

obj.set("foo", "bar");
assertEquals("bar", obj.get("foo"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Close obj after this.

Copy link
Author

Choose a reason for hiding this comment

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

done

} finally {
obj.close();
pool.close();
}
}
}
39 changes: 39 additions & 0 deletions src/test/java/redis/clients/jedis/tests/JedisSentinelPoolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of my comments in JedisPoolTest.java are also applicable in this class. So use those suggestions :)

Copy link
Author

Choose a reason for hiding this comment

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

done

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import java.util.HashSet;
import java.util.Set;
Expand All @@ -12,6 +13,8 @@

import redis.clients.jedis.HostAndPort;
import redis.clients.jedis.Jedis;
import redis.clients.jedis.JedisFactory;
import redis.clients.jedis.JedisPoolConfig;
import redis.clients.jedis.JedisSentinelPool;
import redis.clients.jedis.Transaction;
import redis.clients.jedis.exceptions.JedisConnectionException;
Expand Down Expand Up @@ -161,6 +164,42 @@ public void customClientName() {

assertTrue(pool.isClosed());
}

@Test
public void testResetInvalidPassword() {
JedisFactory factory = new JedisFactory(null, 0, 2000, 2000, "foobared", 0, "my_shiny_client_name");

try(JedisSentinelPool pool = new JedisSentinelPool(MASTER_NAME, sentinels, new JedisPoolConfig(), factory);
Jedis obj1 = pool.getResource(); ) {
obj1.set("foo", "bar");
assertEquals("bar", obj1.get("foo"));
assertEquals(1, pool.getNumActive());

factory.setPassword("wrong password");
try (Jedis obj2 = pool.getResource();) {
fail("Should not get resource from pool");
}catch (JedisConnectionException e) {}
}
}

@Test
public void testResetValidPassword() {
JedisFactory factory = new JedisFactory(null, 0, 2000, 2000, "wrong password", 0, "my_shiny_client_name");
JedisSentinelPool pool = new JedisSentinelPool(MASTER_NAME, sentinels, new JedisPoolConfig(), factory);
Jedis obj = null;
try {
pool.getResource();
Copy link
Contributor

Choose a reason for hiding this comment

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

add fail() below pool.getResource()

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Get this resource and close it properly, just to avoid any possible connection leak. try-with-resources can be used for convenience.

fail("Could not get resource from pool");
} catch (JedisConnectionException e) {
factory.setPassword("foobared");
obj = pool.getResource();
Copy link
Contributor

Choose a reason for hiding this comment

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

Close obj within this catch block. Optionally, you can declare obj (aka Jedis obj = pool.getResource();) here with/without try-with-resources.

obj.set("foo", "bar");
assertEquals("bar", obj.get("foo"));
} finally {
obj.close();
pool.close();
}
}

private void forceFailover(JedisSentinelPool pool) throws InterruptedException {
HostAndPort oldMaster = pool.getCurrentHostMaster();
Expand Down