thomasrebele commented on code in PR #320:
URL: https://github.com/apache/calcite-avatica/pull/320#discussion_r3878870297
##########
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() {
Review Comment:
The name of the test is a bit misleading: if there are several properties,
there may be an allowlist exception for one property, even if there is a
denylist exception for another property. This depends on the iteration order.
How about
`testCheckClientPropertiesDenylistAppliedBeforeAllowlistForCertainProperty`?
##########
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:
If someone wants to reject all properties, how would they do it? Or is there
a property that the client needs to always set?
--
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]
