From b97c8bbc03ea36f6ef81c22ed04708fbbb5d6929 Mon Sep 17 00:00:00 2001 From: Stepan Burlakov Date: Mon, 9 Sep 2024 13:19:18 +0300 Subject: [PATCH] refactor: FIR-35469 remove account resolve endpoint from jdbc (#452) --- .../integration/tests/SystemEngineTest.java | 5 +-- .../java/integration/tests/TimeoutTest.java | 2 +- .../account/FireboltAccountRetriever.java | 10 +++--- .../client/query/StatementClientImpl.java | 5 ++- .../FireboltConnectionServiceSecret.java | 14 +++----- .../gateway/FireboltGatewayUrlClientTest.java | 35 +++++-------------- 6 files changed, 24 insertions(+), 47 deletions(-) diff --git a/src/integrationTest/java/integration/tests/SystemEngineTest.java b/src/integrationTest/java/integration/tests/SystemEngineTest.java index 549d22db9..6e400ca16 100644 --- a/src/integrationTest/java/integration/tests/SystemEngineTest.java +++ b/src/integrationTest/java/integration/tests/SystemEngineTest.java @@ -136,7 +136,8 @@ void shouldFailToSelectFromCustomDbUsingSystemEngine() throws SQLException { } FireboltException e = assertThrows(FireboltException.class, () -> systemConnection.createStatement().executeQuery("select count(*) from dummy")); String actualErrorMessage = e.getErrorMessageFromServer().replaceAll("\r?\n", ""); - assertTrue(expectedErrorMessages.contains(actualErrorMessage), "Unexpected error message: " + actualErrorMessage); + // Check that at least one error message from expectedErrorMessages is contained in the actual error message + assertTrue(expectedErrorMessages.stream().anyMatch(actualErrorMessage::contains), "Unexpected error message: " + actualErrorMessage); } finally { try { customConnection.createStatement().executeUpdate("DROP TABLE dummy"); @@ -292,7 +293,7 @@ private String getClientSecret(Connection connection, String serviceAccountName, FireboltConnection fbConn = (FireboltConnection)connection; String accessToken = fbConn.getAccessToken().orElseThrow(() -> new IllegalStateException("access token is not found")); FireboltProperties fbProps = fbConn.getSessionProperties(); - URL url = new URL(format("%s/query?output_format=TabSeparatedWithNamesAndTypes&database=%s&account_id=%s", fbProps.getHttpConnectionUrl(), database, fbProps.getAccountId())); + URL url = new URL(format("%s/query?output_format=TabSeparatedWithNamesAndTypes&database=%s", fbProps.getHttpConnectionUrl(), database)); HttpURLConnection con = (HttpURLConnection)url.openConnection(); con.setRequestMethod("POST"); con.setRequestProperty("Content-Type", "application/x-www-form-urlencoded"); diff --git a/src/integrationTest/java/integration/tests/TimeoutTest.java b/src/integrationTest/java/integration/tests/TimeoutTest.java index 54fa6cd28..0a8b281b6 100644 --- a/src/integrationTest/java/integration/tests/TimeoutTest.java +++ b/src/integrationTest/java/integration/tests/TimeoutTest.java @@ -24,7 +24,7 @@ @CustomLog class TimeoutTest extends IntegrationTest { private static final int MIN_TIME_SECONDS = 350; - private static final Map SERIES_SIZE = Map.of(1, 80000000000L, 2, 180000000000L); + private static final Map SERIES_SIZE = Map.of(1, 80000000000L, 2, 600000000000L); private long startTime; @BeforeEach diff --git a/src/main/java/com/firebolt/jdbc/client/account/FireboltAccountRetriever.java b/src/main/java/com/firebolt/jdbc/client/account/FireboltAccountRetriever.java index 8b4876af4..3e578829f 100644 --- a/src/main/java/com/firebolt/jdbc/client/account/FireboltAccountRetriever.java +++ b/src/main/java/com/firebolt/jdbc/client/account/FireboltAccountRetriever.java @@ -15,22 +15,20 @@ import static java.net.HttpURLConnection.HTTP_NOT_FOUND; public class FireboltAccountRetriever extends FireboltClient implements CacheListener { - private static final String URL = "https://%s/web/v3/account/%s/%s"; + private static final String URL = "https://%s/web/v3/account/%s/engineUrl"; private final String host; - private final String path; private final Class type; private static final Map valueCache = new ConcurrentHashMap<>(); - public FireboltAccountRetriever(OkHttpClient httpClient, FireboltConnection connection, String customDrivers, String customClients, String host, String path, Class type) { + public FireboltAccountRetriever(OkHttpClient httpClient, FireboltConnection connection, String customDrivers, String customClients, String host, Class type) { super(httpClient, connection, customDrivers, customClients); this.host = host; - this.path = path; this.type = type; } public T retrieve(String accessToken, String accountName) throws SQLException { try { - String url = format(URL, host, accountName, path); + String url = format(URL, host, accountName); @SuppressWarnings("unchecked") T value = (T)valueCache.get(url); if (value == null) { @@ -39,7 +37,7 @@ public T retrieve(String accessToken, String accountName) throws SQLException { } return value; } catch (IOException e) { - throw new FireboltException(String.format("Failed to get %s url for account %s: %s", path, accountName, e.getMessage()), e); + throw new FireboltException(String.format("Failed to get engine url for account %s: %s", accountName, e.getMessage()), e); } } diff --git a/src/main/java/com/firebolt/jdbc/client/query/StatementClientImpl.java b/src/main/java/com/firebolt/jdbc/client/query/StatementClientImpl.java index 5fb4a3fa4..8e37b9b3c 100644 --- a/src/main/java/com/firebolt/jdbc/client/query/StatementClientImpl.java +++ b/src/main/java/com/firebolt/jdbc/client/query/StatementClientImpl.java @@ -314,9 +314,12 @@ private Map getAllParameters(FireboltProperties fireboltProperti } } else { if (connection.getInfraVersion() >= 2) { + String engine = fireboltProperties.getEngine(); if (accountId != null) { params.put(FireboltQueryParameterKey.ACCOUNT_ID.getKey(), accountId); - params.put(FireboltQueryParameterKey.ENGINE.getKey(), fireboltProperties.getEngine()); + } + if (engine != null) { + params.put(FireboltQueryParameterKey.ENGINE.getKey(), engine); } params.put(FireboltQueryParameterKey.QUERY_LABEL.getKey(), statementInfoWrapper.getLabel()); //QUERY_LABEL } diff --git a/src/main/java/com/firebolt/jdbc/connection/FireboltConnectionServiceSecret.java b/src/main/java/com/firebolt/jdbc/connection/FireboltConnectionServiceSecret.java index ab08eb342..72122f9a4 100644 --- a/src/main/java/com/firebolt/jdbc/connection/FireboltConnectionServiceSecret.java +++ b/src/main/java/com/firebolt/jdbc/connection/FireboltConnectionServiceSecret.java @@ -34,7 +34,6 @@ public class FireboltConnectionServiceSecret extends FireboltConnection { private static final String PROTOCOL_VERSION = "2.1"; private final FireboltGatewayUrlService fireboltGatewayUrlService; - private final FireboltAccountIdService fireboltAccountIdService; private FireboltEngineService fireboltEngineService; // depends on infra version and is discovered during authentication FireboltConnectionServiceSecret(@NonNull String url, @@ -46,7 +45,6 @@ public class FireboltConnectionServiceSecret extends FireboltConnection { FireboltAccountIdService fireboltAccountIdService) throws SQLException { super(url, connectionSettings, fireboltAuthenticationService, fireboltStatementService, PROTOCOL_VERSION); this.fireboltGatewayUrlService = fireboltGatewayUrlService; - this.fireboltAccountIdService = fireboltAccountIdService; this.fireboltEngineService = fireboltEngineService; connect(); } @@ -55,14 +53,13 @@ public class FireboltConnectionServiceSecret extends FireboltConnection { FireboltConnectionServiceSecret(@NonNull String url, Properties connectionSettings) throws SQLException { super(url, connectionSettings, PROTOCOL_VERSION); OkHttpClient httpClient = getHttpClient(loginProperties); - this.fireboltGatewayUrlService = new FireboltGatewayUrlService(createFireboltAccountRetriever(httpClient,"engineUrl", GatewayUrlResponse.class)); - this.fireboltAccountIdService = new FireboltAccountIdService(createFireboltAccountRetriever(httpClient,"resolve", FireboltAccount.class)); + this.fireboltGatewayUrlService = new FireboltGatewayUrlService(createFireboltAccountRetriever(httpClient, GatewayUrlResponse.class)); // initialization of fireboltEngineService depends on the infraVersion (the version of engine) connect(); } - private FireboltAccountRetriever createFireboltAccountRetriever(OkHttpClient httpClient, String path, Class type) { - return new FireboltAccountRetriever<>(httpClient, this, loginProperties.getUserDrivers(), loginProperties.getUserClients(), loginProperties.getHost(), path, type); + private FireboltAccountRetriever createFireboltAccountRetriever(OkHttpClient httpClient, Class type) { + return new FireboltAccountRetriever<>(httpClient, this, loginProperties.getUserDrivers(), loginProperties.getUserClients(), loginProperties.getHost(), type); } @Override @@ -102,11 +99,9 @@ protected void assertDatabaseExisting(String database) throws SQLException { private FireboltProperties getSessionPropertiesForSystemEngine(String accessToken, String accountName) throws SQLException { String systemEngineEndpoint = fireboltGatewayUrlService.getUrl(accessToken, accountName); - FireboltAccount account = fireboltAccountIdService.getValue(accessToken, accountName); - infraVersion = account.getInfraVersion(); + infraVersion = 2; URL systemEngienUrl = UrlUtil.createUrl(systemEngineEndpoint); Map systemEngineUrlUrlParams = UrlUtil.getQueryParameters(systemEngienUrl); - String accountId = systemEngineUrlUrlParams.getOrDefault(ACCOUNT_ID.getKey(), account.getId()); for (Entry e : systemEngineUrlUrlParams.entrySet()) { loginProperties.addProperty(e); } @@ -114,7 +109,6 @@ private FireboltProperties getSessionPropertiesForSystemEngine(String accessToke .toBuilder() .systemEngine(true) .compress(false) - .accountId(accountId) .host(systemEngienUrl.getHost()) .build(); } diff --git a/src/test/java/com/firebolt/jdbc/client/gateway/FireboltGatewayUrlClientTest.java b/src/test/java/com/firebolt/jdbc/client/gateway/FireboltGatewayUrlClientTest.java index 360eac32b..c0aa7d13d 100644 --- a/src/test/java/com/firebolt/jdbc/client/gateway/FireboltGatewayUrlClientTest.java +++ b/src/test/java/com/firebolt/jdbc/client/gateway/FireboltGatewayUrlClientTest.java @@ -65,13 +65,11 @@ class FireboltAccountRetrieverTest { private FireboltConnection fireboltConnection; private FireboltAccountRetriever fireboltGatewayUrlClient; - private FireboltAccountRetriever fireboltAccountIdResolver; @BeforeEach void setUp() { - fireboltGatewayUrlClient = new FireboltAccountRetriever<>(httpClient, fireboltConnection, null, null, "test-firebolt.io", "engineUrl", GatewayUrlResponse.class); - fireboltAccountIdResolver = new FireboltAccountRetriever<>(httpClient, fireboltConnection, null, null, "test-firebolt.io", "resolve", FireboltAccount.class); + fireboltGatewayUrlClient = new FireboltAccountRetriever<>(httpClient, fireboltConnection, null, null, "test-firebolt.io", GatewayUrlResponse.class); } @Test @@ -81,21 +79,6 @@ void shouldGetGatewayUrlWhenResponseIsOk() throws SQLException, IOException { assertEquals(engineUrl, fireboltGatewayUrlClient.retrieve("access_token", "account").getEngineUrl()); } - @Test - void shouldGetAccountId() throws SQLException, IOException { - fireboltAccountIdResolver.cleanup(); - FireboltAccount account = new FireboltAccount("12345", "central", 2); - injectMockedResponse(httpClient, HTTP_OK, "{\"id\": \"12345\", \"region\":\"central\", \"infraVersion\":2}"); - assertEquals(account, fireboltAccountIdResolver.retrieve("access_token", "account")); - Mockito.verify(httpClient, times(1)).newCall(any()); - // Do this again. The response is cached, so the invocation count will remain 1 - assertEquals(account, fireboltAccountIdResolver.retrieve("access_token", "account")); - // now clean the cache and call resolve() again. The invocation counter will be incremented. - fireboltAccountIdResolver.cleanup(); - assertEquals(account, fireboltAccountIdResolver.retrieve("access_token", "account")); - Mockito.verify(httpClient, times(2)).newCall(any()); - } - @Test void shouldRuntimeExceptionUponRuntimeException() { when(httpClient.newCall(any())).thenThrow(new IllegalArgumentException("ex")); @@ -107,20 +90,19 @@ void shouldThrowFireboltExceptionUponIOException() throws IOException { Call call = mock(Call.class); when(httpClient.newCall(any())).thenReturn(call); when(call.execute()).thenThrow(new IOException("io error")); - assertEquals("Failed to get engineUrl url for account acc: io error", assertThrows(FireboltException.class, () -> fireboltGatewayUrlClient.retrieve("token", "acc")).getMessage()); + assertEquals("Failed to get engine url for account acc: io error", assertThrows(FireboltException.class, () -> fireboltGatewayUrlClient.retrieve("token", "acc")).getMessage()); } @ParameterizedTest(name = "{0}:{1}") @CsvSource({ - "resolve, com.firebolt.jdbc.client.account.FireboltAccount, {}, JSONObject[\"id\"] not found.", - "engineUrl, com.firebolt.jdbc.client.gateway.GatewayUrlResponse, {}, JSONObject[\"engineUrl\"] not found." + "com.firebolt.jdbc.client.gateway.GatewayUrlResponse, {}, JSONObject[\"engineUrl\"] not found." }) - void shouldThrowFireboltExceptionUponWrongJsonFormat(String path, Class clazz, String json, String expectedErrorMessage) throws IOException { - FireboltAccountRetriever fireboltAccountIdResolver = mockAccountRetriever(path, clazz, json); - assertEquals(format("Failed to get %s url for account acc: %s", path, expectedErrorMessage), assertThrows(FireboltException.class, () -> fireboltAccountIdResolver.retrieve("token", "acc")).getMessage()); + void shouldThrowFireboltExceptionUponWrongJsonFormat(Class clazz, String json, String expectedErrorMessage) throws IOException { + FireboltAccountRetriever fireboltAccountIdResolver = mockAccountRetriever(clazz, json); + assertEquals(format("Failed to get engine url for account acc: %s", expectedErrorMessage), assertThrows(FireboltException.class, () -> fireboltAccountIdResolver.retrieve("token", "acc")).getMessage()); } - private FireboltAccountRetriever mockAccountRetriever(String path, Class clazz, String json) throws IOException { + private FireboltAccountRetriever mockAccountRetriever(Class clazz, String json) throws IOException { try (Response response = mock(Response.class)) { when(response.code()).thenReturn(200); ResponseBody responseBody = mock(ResponseBody.class); @@ -130,7 +112,7 @@ private FireboltAccountRetriever mockAccountRetriever(String path, Class< Call call = mock(); when(call.execute()).thenReturn(response); when(okHttpClient.newCall(any())).thenReturn(call); - return new FireboltAccountRetriever<>(okHttpClient, mock(), null, null, "test-firebolt.io", path, clazz); + return new FireboltAccountRetriever<>(okHttpClient, mock(), null, null, "test-firebolt.io", clazz); } } @@ -165,7 +147,6 @@ private FireboltAccountRetriever mockAccountRetriever(String path, Class< }) void testFailedAccountDataRetrieving(int statusCode, String errorMessageTemplate) throws IOException { injectMockedResponse(httpClient, statusCode, null); - assertErrorMessage(fireboltAccountIdResolver, "one", format(errorMessageTemplate, "one", "resolve")); assertErrorMessage(fireboltGatewayUrlClient, "two", format(errorMessageTemplate, "two", "engineUrl")); }