thomasrebele commented on code in PR #320:
URL: https://github.com/apache/calcite-avatica/pull/320#discussion_r3879174973


##########
server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java:
##########
@@ -202,6 +207,122 @@ public class JdbcMetaTest {
     // Our opened connection should get closed when this race condition happens
     Mockito.verify(conn2).close();
   }
+
+  private static Set<String> setOf(String... names) {
+    return new HashSet<>(java.util.Arrays.asList(names));
+  }
+
+  @Test public void testCheckClientPropertiesNoRulesConfigured() {
+    // With both lists null, every client name is passed through.
+    JdbcMeta.checkClientProperties(null, null,
+        Collections.singletonMap("anything", "v"));
+  }
+
+  @Test public void testCheckClientPropertiesDenylistRejectsListedName() {
+    final ForbiddenConnectionPropertyException e =
+        assertThrows(ForbiddenConnectionPropertyException.class, () ->
+            JdbcMeta.checkClientProperties(setOf("propA", "propB"), null,
+                Collections.singletonMap("propA", "v")));
+    assertThat(e.getPropertyName(), is("propA"));
+    assertThat(e.getRule(), 
is(ForbiddenConnectionPropertyException.Rule.DENYLIST));
+    assertThat(e.getMessage(), containsString("propA"));
+    assertThat(e.getMessage(), containsString("denylist"));
+    assertThat(e.getMessage(),
+        containsString(JdbcMeta.CLIENT_PROPERTIES_DENYLIST_KEY));
+  }
+
+  @Test public void testCheckClientPropertiesDenylistPermitsUnlistedName() {
+    // No exception when the name is not on the denylist.
+    JdbcMeta.checkClientProperties(setOf("propA"), null,
+        Collections.singletonMap("propOther", "v"));
+  }
+
+  @Test public void testCheckClientPropertiesAllowlistRejectsUnlistedName() {
+    final ForbiddenConnectionPropertyException e =
+        assertThrows(ForbiddenConnectionPropertyException.class, () ->
+            JdbcMeta.checkClientProperties(null, setOf("propA", "propB"),
+                Collections.singletonMap("propC", "v")));
+    assertThat(e.getPropertyName(), is("propC"));
+    assertThat(e.getRule(), 
is(ForbiddenConnectionPropertyException.Rule.ALLOWLIST));
+    assertThat(e.getMessage(), containsString("propC"));
+    assertThat(e.getMessage(), containsString("allowlist"));
+    assertThat(e.getMessage(),
+        containsString(JdbcMeta.CLIENT_PROPERTIES_ALLOWLIST_KEY));
+  }
+
+  @Test public void testCheckClientPropertiesAllowlistPermitsListedName() {
+    JdbcMeta.checkClientProperties(null, setOf("propA", "propB"),
+        Collections.singletonMap("propA", "v"));
+  }
+
+  @Test public void testCheckClientPropertiesDenylistAppliedBeforeAllowlist() {
+    // A name on both lists is rejected by the denylist, not the allowlist,
+    // the denylist rule always wins because it is checked first.
+    final ForbiddenConnectionPropertyException e =
+        assertThrows(ForbiddenConnectionPropertyException.class, () ->
+            JdbcMeta.checkClientProperties(setOf("propA"), setOf("propA", 
"propB"),
+                Collections.singletonMap("propA", "v")));
+    assertThat(e.getRule(), 
is(ForbiddenConnectionPropertyException.Rule.DENYLIST));
+  }
+
+  @Test public void testCheckClientPropertiesEmptyMapBypassesChecks() {
+    // An empty client map trivially satisfies both lists; the check must
+    // not fabricate a rejection when there is nothing to check.
+    JdbcMeta.checkClientProperties(null, setOf("propA"), 
Collections.emptyMap());
+    JdbcMeta.checkClientProperties(setOf("propA"), setOf("propA"),
+        Collections.emptyMap());
+  }
+
+  @Test public void testCheckClientPropertiesNullMapBypassesChecks() {
+    JdbcMeta.checkClientProperties(setOf("propA"), setOf("propA"), null);
+  }
+
+  @Test public void testParsePropertyNameListNullOrBlankIsUnconfigured() {
+    // A null or all-blank value means the rule was not configured; the
+    // parser must return null so the check treats it as absent, not as an
+    // empty allowlist that would reject everything.

Review Comment:
   Ok, we can keep it as it is. The "dummy" could be put on both, allowlist and 
denylist, so that no properties are accepted.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to