Skip to content

Commit

Permalink
Merge branch 'master' into moshap/clean_old_settings
Browse files Browse the repository at this point in the history
  • Loading branch information
moshap-firebolt authored Oct 18, 2024
2 parents 9e48c87 + dc4463f commit 5613c63
Show file tree
Hide file tree
Showing 18 changed files with 104 additions and 91 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,12 @@ jobs:
run: |
echo "PROJECT_VERSION=$(./gradlew printVersion |grep 'PROJECT_VERSION=' | cut -d= -f2)" >> $GITHUB_OUTPUT
- name: Upload uber-jar
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: firebolt-jdbc-${{ steps.project-version.outputs.PROJECT_VERSION }}.jar
path: build/libs/firebolt-jdbc-${{ steps.project-version.outputs.PROJECT_VERSION }}.jar
- name: Upload sources-jar
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: firebolt-jdbc-${{ steps.project-version.outputs.PROJECT_VERSION }}-sources.jar
path: build/libs/firebolt-jdbc-${{ steps.project-version.outputs.PROJECT_VERSION }}-sources.jar
6 changes: 3 additions & 3 deletions .github/workflows/performance-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ jobs:
mkdir dependencies
- name: Download uber-jar
id: download-jar
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4.1.7
with:
name: ${{ needs.build.outputs.uber-jar }}
path: dependencies
Expand Down Expand Up @@ -63,12 +63,12 @@ jobs:
outputReportsFolder: reports/
args: -Jdatabase=${{ steps.find-database-name.outputs.database_name }} -Jpassword=${{ secrets.SERVICE_ACCOUNT_SECRET_STAGING }} -Jusername=${{ secrets.SERVICE_ACCOUNT_ID_STAGING }} -Jdriver=${{ needs.build.outputs.uber-jar }} -Jenvironment=staging -Jthreads=${{ inputs.threads }} -Jloops=${{ inputs.loops }}
- name: Upload JMeter report
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: performance_test_report
path: reports
- name: Upload JMeter logs
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: jmeter_log.log
path: jmeter_log.log
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ jobs:
with:
ref: ${{ github.event.release.tag_name || github.event.inputs.tag_name}}
- name: Download uber-jar
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4.1.7
with:
name: ${{ needs.build.outputs.uber-jar }}
- name: Download sources-jar
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4.1.7
with:
name: ${{ needs.build.outputs.sources-jar }}
- uses: xresloader/upload-to-github-release@v1
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
The official JDBC driver for Firebolt.

## Documentation
To download the JDBC driver and learn how to use it, please see the official [Firebolt documentation](https://docs.firebolt.io/developing-with-firebolt/connecting-with-jdbc.html).
To download the JDBC driver and learn how to use it, please see the official [Firebolt documentation](https://docs.firebolt.io/Guides/developing-with-firebolt/connecting-with-jdbc.html).

## Development
The code of this repository is formatted using the Eclipse formatter, which was chosen because it is supported by multiple modern IDEs (such as Intellij).
Expand Down
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
version=3.1.1
version=3.2.0
jdbcVersion=4.3
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
@CustomLog
class TimeoutTest extends IntegrationTest {
private static final int MIN_TIME_SECONDS = 350;
private static final Map<Integer, Long> SERIES_SIZE = Map.of(1, 80000000000L, 2, 180000000000L);
private static final Map<Integer, Long> SERIES_SIZE = Map.of(1, 80000000000L, 2, 600000000000L);
private long startTime;

@BeforeEach
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,20 @@
import static java.net.HttpURLConnection.HTTP_NOT_FOUND;

public class FireboltAccountRetriever<T> 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<T> type;
private static final Map<String, Object> valueCache = new ConcurrentHashMap<>();

public FireboltAccountRetriever(OkHttpClient httpClient, FireboltConnection connection, String customDrivers, String customClients, String host, String path, Class<T> type) {
public FireboltAccountRetriever(OkHttpClient httpClient, FireboltConnection connection, String customDrivers, String customClients, String host, Class<T> 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) {
Expand All @@ -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);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,9 +314,12 @@ private Map<String, String> 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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -417,17 +417,21 @@ public boolean isValid(int timeout) throws SQLException {
}
try {
if (!loginProperties.isSystemEngine()) {
validateConnection(getSessionProperties(), true);
validateConnection(getSessionProperties(), true, true);
}
return true;
} catch (Exception e) {
return false;
}
}

private void validateConnection(FireboltProperties fireboltProperties, boolean ignoreToManyRequestsError)
private void validateConnection(FireboltProperties fireboltProperties, boolean ignoreToManyRequestsError, boolean isInternalRequest)
throws SQLException {
try (Statement s = createStatement(fireboltProperties)) {
FireboltProperties propertiesCopy = FireboltProperties.copy(fireboltProperties);
if (isInternalRequest) {
propertiesCopy.addProperty("auto_start_stop_control", "ignore");
}
try (Statement s = createStatement(propertiesCopy)) {
s.execute("SELECT 1");
} catch (Exception e) {
// A connection is not invalid when too many requests are being sent.
Expand Down Expand Up @@ -470,7 +474,7 @@ private synchronized void changeProperty(Consumer<FireboltProperties> properties
try {
FireboltProperties tmpProperties = FireboltProperties.copy(sessionProperties);
propertiesEditor.accept(tmpProperties);
validateConnection(tmpProperties, false);
validateConnection(tmpProperties, false, false);
propertiesEditor.accept(sessionProperties);
} catch (FireboltException e) {
throw e;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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();
}
Expand All @@ -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 <T> FireboltAccountRetriever<T> createFireboltAccountRetriever(OkHttpClient httpClient, String path, Class<T> type) {
return new FireboltAccountRetriever<>(httpClient, this, loginProperties.getUserDrivers(), loginProperties.getUserClients(), loginProperties.getHost(), path, type);
private <T> FireboltAccountRetriever<T> createFireboltAccountRetriever(OkHttpClient httpClient, Class<T> type) {
return new FireboltAccountRetriever<>(httpClient, this, loginProperties.getUserDrivers(), loginProperties.getUserClients(), loginProperties.getHost(), type);
}

@Override
Expand Down Expand Up @@ -102,19 +99,16 @@ 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<String, String> systemEngineUrlUrlParams = UrlUtil.getQueryParameters(systemEngienUrl);
String accountId = systemEngineUrlUrlParams.getOrDefault(ACCOUNT_ID.getKey(), account.getId());
for (Entry<String, String> e : systemEngineUrlUrlParams.entrySet()) {
loginProperties.addProperty(e);
}
return loginProperties
.toBuilder()
.systemEngine(true)
.compress(false)
.accountId(accountId)
.host(systemEngienUrl.getHost())
.build();
}
Expand Down
12 changes: 9 additions & 3 deletions src/main/java/com/firebolt/jdbc/statement/FireboltStatement.java
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,8 @@ private Optional<ResultSet> execute(StatementInfoWrapper statementInfoWrapper) t
}
InputStream inputStream = null;
try {

log.info("Executing the statement with label {} : {}", statementInfoWrapper.getLabel(),
statementInfoWrapper.getSql());
log.debug("Executing the statement with label {} : {}", statementInfoWrapper.getLabel(),
sanitizeSql(statementInfoWrapper.getSql()));
if (statementInfoWrapper.getType() == StatementType.PARAM_SETTING) {
connection.addProperty(statementInfoWrapper.getParam());
log.debug("The property from the query {} was stored", runningStatementLabel);
Expand Down Expand Up @@ -519,4 +518,11 @@ public void setPoolable(boolean poolable) throws SQLException {
public boolean hasMoreResults() {
return currentStatementResult.getNext() != null;
}

private String sanitizeSql(String sql) {
// Replace any occurrence of secrets with ***
return sql.replaceAll("AWS_KEY_ID\\s*=\\s*[\\S]*", "AWS_KEY_ID=***")
.replaceAll("AWS_SECRET_KEY\\s*=\\s*[\\S]*",
"AWS_SECRET_KEY=***");
}
}
2 changes: 1 addition & 1 deletion src/test/java/com/firebolt/FireboltDriverTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ void jdbcCompliant() {
void version() {
FireboltDriver fireboltDriver = new FireboltDriver();
assertEquals(3, fireboltDriver.getMajorVersion());
assertEquals(1, fireboltDriver.getMinorVersion());
assertEquals(2, fireboltDriver.getMinorVersion());
}

@ParameterizedTest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,11 @@ class FireboltAccountRetrieverTest {
private FireboltConnection fireboltConnection;

private FireboltAccountRetriever<GatewayUrlResponse> fireboltGatewayUrlClient;
private FireboltAccountRetriever<FireboltAccount> 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
Expand All @@ -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"));
Expand All @@ -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."
})
<T> void shouldThrowFireboltExceptionUponWrongJsonFormat(String path, Class<T> clazz, String json, String expectedErrorMessage) throws IOException {
FireboltAccountRetriever<T> 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());
<T> void shouldThrowFireboltExceptionUponWrongJsonFormat(Class<T> clazz, String json, String expectedErrorMessage) throws IOException {
FireboltAccountRetriever<T> 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 <T> FireboltAccountRetriever<T> mockAccountRetriever(String path, Class<T> clazz, String json) throws IOException {
private <T> FireboltAccountRetriever<T> mockAccountRetriever(Class<T> clazz, String json) throws IOException {
try (Response response = mock(Response.class)) {
when(response.code()).thenReturn(200);
ResponseBody responseBody = mock(ResponseBody.class);
Expand All @@ -130,7 +112,7 @@ private <T> FireboltAccountRetriever<T> 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);
}
}

Expand Down Expand Up @@ -165,7 +147,6 @@ private <T> FireboltAccountRetriever<T> 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"));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ void shouldValidateConnectionWhenCallingIsValid() throws SQLException {
verify(fireboltStatementService).execute(queryInfoWrapperArgumentCaptor.capture(),
propertiesArgumentCaptor.capture(), any());
assertEquals(List.of("SELECT 1"), queryInfoWrapperArgumentCaptor.getAllValues().stream().map(StatementInfoWrapper::getSql).collect(toList()));
assertEquals(Map.of("auto_start_stop_control", "ignore"), propertiesArgumentCaptor.getValue().getAdditionalProperties());
}
}

Expand Down
Loading

0 comments on commit 5613c63

Please sign in to comment.