From bd1cda289e4a06dc24918845292aaac61a4edbeb Mon Sep 17 00:00:00 2001 From: Piotr Rzysko Date: Tue, 14 Jan 2025 11:37:13 +0100 Subject: [PATCH] Improve test coverage for ThriftHttpMetastoreClient --- .../thrift/TestThriftHttpMetastoreClient.java | 57 ++++++++++++------- .../TestingThriftHttpMetastoreServer.java | 31 ++++++---- 2 files changed, 58 insertions(+), 30 deletions(-) diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHttpMetastoreClient.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHttpMetastoreClient.java index 1cbe6d01e5b4..b9a590e9c65d 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHttpMetastoreClient.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHttpMetastoreClient.java @@ -17,7 +17,9 @@ import io.opentelemetry.api.OpenTelemetry; import io.trino.hive.thrift.metastore.Database; import io.trino.hive.thrift.metastore.NoSuchObjectException; +import io.trino.hive.thrift.metastore.TableMeta; import io.trino.plugin.base.util.AutoCloseableCloser; +import io.trino.plugin.hive.metastore.thrift.TestingThriftHttpMetastoreServer.TestingThriftRequestsHandler; import io.trino.testing.TestingNodeManager; import jakarta.servlet.http.HttpServletRequest; import org.apache.http.HttpHeaders; @@ -34,14 +36,12 @@ import java.util.Optional; import java.util.function.Consumer; -import static io.trino.plugin.hive.TestingThriftHiveMetastoreBuilder.testingThriftHiveMetastoreBuilder; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; public class TestThriftHttpMetastoreClient { private static final AutoCloseableCloser closer = AutoCloseableCloser.create(); - private static ThriftMetastore delegate; @BeforeAll public static void setup() @@ -49,7 +49,6 @@ public static void setup() { File tempDir = Files.createTempDirectory(null).toFile(); tempDir.deleteOnExit(); - delegate = testingThriftHiveMetastoreBuilder().metastoreClient(createFakeMetastoreClient()).build(closer::register); } @AfterAll @@ -59,43 +58,61 @@ public static void tearDown() closer.close(); } - private static ThriftMetastoreClient createFakeMetastoreClient() + @Test + public void testHttpThriftConnection() + throws Exception { - return new MockThriftMetastoreClient() + ThriftHttpMetastoreConfig config = new ThriftHttpMetastoreConfig(); + config.setAuthenticationMode(ThriftHttpMetastoreConfig.AuthenticationMode.BEARER); + config.setAdditionalHeaders("key1:value1, key2:value2"); + + TestingThriftRequestsHandler handler = new TestingThriftRequestsHandler() { @Override - public Database getDatabase(String databaseName) + public List getAllDatabases() + { + return ImmutableList.of("testDbName"); + } + + @Override + public Database getDatabase(String name) throws NoSuchObjectException { - if (databaseName.equals("testDbName")) { - return new Database(databaseName, "testOwner", "testLocation", Map.of("key", "value")); + if (name.equals("testDbName")) { + return new Database(name, "testOwner", "testLocation", Map.of("key", "value")); } throw new NoSuchObjectException("Database does not exist"); } @Override - public List getAllDatabases() + public List getTables(String databaseName, String pattern) { - return ImmutableList.of("testDbName"); + if (databaseName.equals("testDbName")) { + return ImmutableList.of("testTable1", "testTable2"); + } + return ImmutableList.of(); } - }; - } - @Test - public void testHttpThriftConnection() - throws Exception - { - ThriftHttpMetastoreConfig config = new ThriftHttpMetastoreConfig(); - config.setAuthenticationMode(ThriftHttpMetastoreConfig.AuthenticationMode.BEARER); - config.setAdditionalHeaders("key1:value1, key2:value2"); + @Override + public List getTablesByType(String databaseName, String pattern, String tableType) + { + if (databaseName.equals("testDbName")) { + return ImmutableList.of("testTable3", "testTable4"); + } + return ImmutableList.of(); + } + }; - try (TestingThriftHttpMetastoreServer metastoreServer = new TestingThriftHttpMetastoreServer(delegate, new TestRequestHeaderInterceptor())) { + try (TestingThriftHttpMetastoreServer metastoreServer = new TestingThriftHttpMetastoreServer(handler, new TestRequestHeaderInterceptor())) { ThriftMetastoreClientFactory factory = new HttpThriftMetastoreClientFactory(config, new TestingNodeManager(), OpenTelemetry.noop()); URI metastoreUri = URI.create("http://localhost:" + metastoreServer.getPort()); ThriftMetastoreClient client = factory.create( metastoreUri, Optional.empty()); assertThat(client.getAllDatabases()).containsExactly("testDbName"); assertThat(client.getDatabase("testDbName")).isEqualTo(new Database("testDbName", "testOwner", "testLocation", Map.of("key", "value"))); + assertThat(client.getTableMeta("testDbName")) + .extracting(TableMeta::getTableName) + .containsExactlyInAnyOrder("testTable1", "testTable2", "testTable3", "testTable4"); // negative case assertThatThrownBy(() -> client.getDatabase("does-not-exist")).isInstanceOf(NoSuchObjectException.class); } diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestingThriftHttpMetastoreServer.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestingThriftHttpMetastoreServer.java index 4688e5285bca..79b531c61f52 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestingThriftHttpMetastoreServer.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestingThriftHttpMetastoreServer.java @@ -34,7 +34,7 @@ import java.io.Closeable; import java.io.IOException; import java.net.URI; -import java.util.Optional; +import java.util.List; import java.util.function.Consumer; import static com.google.common.reflect.Reflection.newProxy; @@ -46,9 +46,9 @@ public class TestingThriftHttpMetastoreServer private final LifeCycleManager lifeCycleManager; private final URI baseUri; - public TestingThriftHttpMetastoreServer(ThriftMetastore delegate, Consumer requestInterceptor) + public TestingThriftHttpMetastoreServer(TestingThriftRequestsHandler handler, Consumer requestInterceptor) { - ThriftHiveMetastore.Iface mockThriftHandler = proxyHandler(delegate, ThriftHiveMetastore.Iface.class); + ThriftHiveMetastore.Iface mockThriftHandler = proxyHandler(handler); TProcessor processor = new ThriftHiveMetastore.Processor<>(mockThriftHandler); thriftHttpServlet = new TestingThriftHttpServlet(processor, new TBinaryProtocol.Factory(), requestInterceptor); Bootstrap app = new Bootstrap( @@ -67,14 +67,13 @@ public TestingThriftHttpMetastoreServer(ThriftMetastore delegate, Consumer T proxyHandler(ThriftMetastore delegate, Class iface) + private static ThriftHiveMetastore.Iface proxyHandler(TestingThriftRequestsHandler handler) { - return newProxy(iface, (proxy, method, args) -> switch (method.getName()) { - case "getAllDatabases" -> delegate.getAllDatabases(); - case "getDatabase" -> { - Optional optionalDatabase = delegate.getDatabase(args[0].toString()); - yield optionalDatabase.orElseThrow(() -> new NoSuchObjectException("")); - } + return newProxy(ThriftHiveMetastore.Iface.class, (_, method, args) -> switch (method.getName()) { + case "getAllDatabases" -> handler.getAllDatabases(); + case "getDatabase" -> handler.getDatabase(args[0].toString()); + case "getTables" -> handler.getTables(args[0].toString(), args[1].toString()); + case "getTablesByType" -> handler.getTablesByType(args[0].toString(), args[1].toString(), args[2].toString()); default -> throw new UnsupportedOperationException(); }); } @@ -114,4 +113,16 @@ protected void doPost(HttpServletRequest request, super.doPost(request, response); } } + + public interface TestingThriftRequestsHandler + { + List getAllDatabases(); + + Database getDatabase(String name) + throws NoSuchObjectException; + + List getTables(String databaseName, String pattern); + + List getTablesByType(String databaseName, String pattern, String tableType); + } }