DonalEvans commented on a change in pull request #7088:
URL: https://github.com/apache/geode/pull/7088#discussion_r744021365
##########
File path:
geode-core/src/test/java/org/apache/geode/cache/client/internal/OpExecutorImplUnitTest.java
##########
@@ -94,9 +95,57 @@ public void
authenticateIfRequired_setId_singleUser_hasNoId() {
when(server.getUserId()).thenReturn(-1L);
when(pool.executeOn(any(Connection.class),
any(Op.class))).thenReturn(123L);
when(connection.getWrappedConnection()).thenReturn(connection);
- executor.authenticateIfRequired(connection, op);
- verify(pool).executeOn(any(Connection.class), any(Op.class));
- verify(server).setUserId(eq(123L));
+ executor.authenticateIfMultiUser(connection, op);
+ verify(pool, never()).executeOn(any(Connection.class), any(Op.class));
+ }
+
+ @Test
+ public void execute_calls_authenticateIfMultiUser() throws Exception {
+ when(connection.execute(any())).thenReturn(123L);
+ when(connectionManager.borrowConnection(5)).thenReturn(connection);
+ OpExecutorImpl spy = spy(executor);
+
+ spy.execute(op, 1);
+ verify(spy).authenticateIfMultiUser(any(), any());
}
+ @Test
+ public void authenticateIfMultiUser_calls_authenticateMultiUser() {
Review comment:
This test seems to be testing a lot of different behaviour. It might be
best to split it out into multiple tests that are each testing one thing, so
that if there's a failure, it's more obvious at a glance what the problem is.
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/security/MultiUserAPIDUnitTest.java
##########
@@ -226,12 +231,38 @@ public void jsonFormatterOnTheClientWithSingleUser()
throws Exception {
region.put("key", value);
// make sure the client only needs to authenticate once
-
assertThat(TrackableUserPasswordAuthInit.timeInitialized.get()).isEqualTo(1);
+ assertThat(CountableUserPasswordAuthInit.count.get()).isEqualTo(1);
}
+ @Rule
+ public ExecutorServiceRule executor = new ExecutorServiceRule();
+
+ @Test
+ public void multiUser_OneUserShouldOnlyAuthenticateOnceByDifferentThread()
throws Exception {
+ ClientCache cache = client.withServerConnection(server.getPort())
+ .withProperty(SECURITY_CLIENT_AUTH_INIT,
CountableUserPasswordAuthInit.class.getName())
+ .withMultiUser(true)
+ .createCache();
+ Properties properties = new Properties();
+ properties.setProperty(UserPasswordAuthInit.USER_NAME, "data");
+ properties.setProperty(UserPasswordAuthInit.PASSWORD, "data");
+ RegionService regionService = cache.createAuthenticatedView(properties);
+
+
cache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region");
+ Region region = regionService.getRegion(SEPARATOR + "region");
Review comment:
Warning here can be fixed by using `Region<Object, Object>`
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/security/MultiUserAPIDUnitTest.java
##########
@@ -252,4 +283,50 @@ public void jsonFormatterOnTheClientWithMultiUser() throws
Exception {
PdxInstance value = regionService.getJsonFormatter().toPdxInstance(json);
regionView.put("key", value);
}
+
+ @Test
+ public void transactionTest() throws Exception {
+ ClientCache cache = client.withServerConnection(server.getPort())
+ .withCredential("data", "data")
+ .createCache();
+ Region<Object, Object> region = client.createProxyRegion("region");
+ CacheTransactionManager txManager =
+ cache.getCacheTransactionManager();
+
+ try {
+ txManager.begin();
+ region.put("key1", "value1");
+ region.put("key2", "value2");
+ txManager.commit();
+ } catch (CommitConflictException conflict) {
+ // ... do necessary work for a transaction that failed on commit
+ } finally {
+ if (txManager.exists()) {
+ txManager.rollback();
+ }
+ }
Review comment:
This test isn't asserting anything and the name is quite vague. What is
the behaviour that's being tested here, and what's the expected outcome? Also,
if an exception is thrown, it's currently caught and ignored. Should it instead
be thrown so that the test fails? Or do we expect to see an exception here?
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/security/MultiUserAPIDUnitTest.java
##########
@@ -252,4 +283,50 @@ public void jsonFormatterOnTheClientWithMultiUser() throws
Exception {
PdxInstance value = regionService.getJsonFormatter().toPdxInstance(json);
regionView.put("key", value);
}
+
+ @Test
+ public void transactionTest() throws Exception {
+ ClientCache cache = client.withServerConnection(server.getPort())
+ .withCredential("data", "data")
+ .createCache();
+ Region<Object, Object> region = client.createProxyRegion("region");
+ CacheTransactionManager txManager =
+ cache.getCacheTransactionManager();
+
+ try {
+ txManager.begin();
+ region.put("key1", "value1");
+ region.put("key2", "value2");
+ txManager.commit();
+ } catch (CommitConflictException conflict) {
+ // ... do necessary work for a transaction that failed on commit
+ } finally {
+ if (txManager.exists()) {
+ txManager.rollback();
+ }
+ }
+ }
+
+ @Test
+ public void multiUserWithClientSubscription() throws Exception {
Review comment:
Could this test name be made more descriptive, so that it states what
the expected behaviour is and what the conditions are? In this case I suspect
it would be something like
"registerInterestAndCQCreationFailGivenMultiUserAuthenticationWithIncorrectPassword"
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/security/MultiUserAPIDUnitTest.java
##########
@@ -226,12 +231,38 @@ public void jsonFormatterOnTheClientWithSingleUser()
throws Exception {
region.put("key", value);
// make sure the client only needs to authenticate once
-
assertThat(TrackableUserPasswordAuthInit.timeInitialized.get()).isEqualTo(1);
+ assertThat(CountableUserPasswordAuthInit.count.get()).isEqualTo(1);
}
+ @Rule
+ public ExecutorServiceRule executor = new ExecutorServiceRule();
Review comment:
Can this rule be put at the top of the class with the other rules please?
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/security/MultiUserAPIDUnitTest.java
##########
@@ -252,4 +283,50 @@ public void jsonFormatterOnTheClientWithMultiUser() throws
Exception {
PdxInstance value = regionService.getJsonFormatter().toPdxInstance(json);
regionView.put("key", value);
}
+
+ @Test
+ public void transactionTest() throws Exception {
+ ClientCache cache = client.withServerConnection(server.getPort())
+ .withCredential("data", "data")
+ .createCache();
+ Region<Object, Object> region = client.createProxyRegion("region");
+ CacheTransactionManager txManager =
+ cache.getCacheTransactionManager();
+
+ try {
+ txManager.begin();
+ region.put("key1", "value1");
+ region.put("key2", "value2");
+ txManager.commit();
+ } catch (CommitConflictException conflict) {
+ // ... do necessary work for a transaction that failed on commit
+ } finally {
+ if (txManager.exists()) {
+ txManager.rollback();
+ }
+ }
+ }
+
+ @Test
+ public void multiUserWithClientSubscription() throws Exception {
+ ClientCache cache = client.withServerConnection(server.getPort())
+ .withPoolSubscription(true)
+ .withProperty(SECURITY_CLIENT_AUTH_INIT,
CountableUserPasswordAuthInit.class.getName())
+ .withMultiUser(true)
+ .createCache();
+ Properties properties = new Properties();
+ properties.setProperty(UserPasswordAuthInit.USER_NAME, "data");
+ properties.setProperty(UserPasswordAuthInit.PASSWORD, "wrongPassword");
+ RegionService regionService = cache.createAuthenticatedView(properties);
+
+
cache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region");
+ Region region = regionService.getRegion(SEPARATOR + "region");
Review comment:
Warning here can be fixed by using `Region<Object, Object>`
##########
File path:
geode-cq/src/distributedTest/java/org/apache/geode/security/MultiUserAPIDUnitTest.java
##########
@@ -252,4 +283,50 @@ public void jsonFormatterOnTheClientWithMultiUser() throws
Exception {
PdxInstance value = regionService.getJsonFormatter().toPdxInstance(json);
regionView.put("key", value);
}
+
+ @Test
+ public void transactionTest() throws Exception {
+ ClientCache cache = client.withServerConnection(server.getPort())
+ .withCredential("data", "data")
+ .createCache();
+ Region<Object, Object> region = client.createProxyRegion("region");
+ CacheTransactionManager txManager =
+ cache.getCacheTransactionManager();
+
+ try {
+ txManager.begin();
+ region.put("key1", "value1");
+ region.put("key2", "value2");
+ txManager.commit();
+ } catch (CommitConflictException conflict) {
+ // ... do necessary work for a transaction that failed on commit
+ } finally {
+ if (txManager.exists()) {
+ txManager.rollback();
+ }
+ }
+ }
+
+ @Test
+ public void multiUserWithClientSubscription() throws Exception {
+ ClientCache cache = client.withServerConnection(server.getPort())
+ .withPoolSubscription(true)
+ .withProperty(SECURITY_CLIENT_AUTH_INIT,
CountableUserPasswordAuthInit.class.getName())
+ .withMultiUser(true)
+ .createCache();
+ Properties properties = new Properties();
+ properties.setProperty(UserPasswordAuthInit.USER_NAME, "data");
+ properties.setProperty(UserPasswordAuthInit.PASSWORD, "wrongPassword");
+ RegionService regionService = cache.createAuthenticatedView(properties);
+
+
cache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region");
+ Region region = regionService.getRegion(SEPARATOR + "region");
+ assertThatThrownBy(() -> region.registerInterestForAllKeys())
+ .isInstanceOf(UnsupportedOperationException.class);
Review comment:
This assertion seems unrelated to the code being tested in this class,
since `registerInterest()` will always throw an `UnsupportedOperationException`
when called on a `ProxyRegion`, regardless of the authentication being used.
Can it just be removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
