Skip to content

SK-2859: Code smell and quality fixes for vault SDK #337

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Devesh-Skyflow wants to merge 1 commit into
base: main
Choose a base branch
from
Open

SK-2859: Code smell and quality fixes for vault SDK #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/main/java/com/skyflow/ConnectionClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ protected void setCommonCredentials(Credentials commonCredentials) throws Skyflo
prioritiseCredentials();
}

protected void updateConnectionConfig(ConnectionConfig connectionConfig) throws SkyflowException {
protected void updateConnectionConfig() throws SkyflowException {
prioritiseCredentials();
}

protected void setBearerToken() throws SkyflowException {
protected synchronized void setBearerToken() throws SkyflowException {
prioritiseCredentials();
Validations.validateCredentials(this.finalCredentials);
if (this.finalCredentials.getApiKey() != null) {
Expand Down Expand Up @@ -89,7 +89,7 @@ private void prioritiseCredentials() throws SkyflowException {
} catch (SkyflowException e) {
throw e;
} catch (Exception e) {
throw new RuntimeException(e);
throw new SkyflowException(ErrorCode.SERVER_ERROR.getCode(), ErrorMessage.EmptyCredentials.getMessage());
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/skyflow/Skyflow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ public SkyflowClientBuilder updateConnectionConfig(ConnectionConfig connectionCo
Validations.validateConnectionConfig(connectionConfig);
if (this.connectionsMap.containsKey(connectionConfig.getConnectionId())) {
ConnectionConfig updatedConfig = findAndUpdateConnectionConfig(connectionConfig);
this.connectionsMap.get(updatedConfig.getConnectionId()).updateConnectionConfig(connectionConfig);
this.connectionsMap.get(updatedConfig.getConnectionId()).updateConnectionConfig();
} else {
LogUtil.printErrorLog(Utils.parameterizedString(
ErrorLogs.CONNECTION_CONFIG_DOES_NOT_EXIST.getLog(), connectionConfig.getConnectionId()
Expand Down
340 changes: 127 additions & 213 deletions src/main/java/com/skyflow/VaultClient.java
View file
Open in desktop

Large diffs are not rendered by default.

16 changes: 15 additions & 1 deletion src/main/java/com/skyflow/errors/SkyflowException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import com.google.gson.JsonParser;
import com.google.gson.JsonSyntaxException;
import com.skyflow.utils.Constants;

import java.util.List;
Expand Down Expand Up @@ -95,7 +96,20 @@
}

private void setResponseBody(String responseBody, Map<String, List<String>> responseHeaders) {
this.responseBody = JsonParser.parseString(responseBody).getAsJsonObject();
JsonElement parsedBody;
try {
parsedBody = JsonParser.parseString(responseBody);
} catch (JsonSyntaxException e) {
// Malformed / unparseable JSON: use a generic message instead of echoing the raw body.

Check failure on line 103 in src/main/java/com/skyflow/errors/SkyflowException.java

View workflow job for this annotation

GitHub Actions / Run spellcheck 

Unknown word (unparseable)
this.message = ErrorMessage.ErrorOccurred.getMessage();
return;
}
if (!parsedBody.isJsonObject()) {
// Valid JSON but not the expected object shape: do not echo the raw body.
this.message = ErrorMessage.ErrorOccurred.getMessage();
return;
}
this.responseBody = parsedBody.getAsJsonObject();
if (this.responseBody.get("error") != null) {
setGrpcCode();
setHttpStatus();
Expand Down
12 changes: 11 additions & 1 deletion src/main/java/com/skyflow/utils/Constants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,19 @@ public final class Constants {
public static final String CURLY_PLACEHOLDER = "{%s}";
public static final String EMPTY_STRING = "";
public static final String QUOTE = "\"";
public static final int HTTP_MAX_IDLE_CONNECTIONS = 10;
public static final long HTTP_KEEP_ALIVE_DURATION_MINUTES = 1;

public static final class HttpUtilityExtra {
public static final String SDK_GENERATED_PREFIX = "SDK-Generated-";
public static final int HTTP_SUCCESS_STATUS_MAX = 299;
public static final String HEADER_ACCEPT = "Accept";
public static final String ACCEPT_ALL = "*/*";
public static final String HEADER_CONTENT_TYPE = "content-type";
public static final String HEADER_REQUEST_ID = "x-request-id";
public static final String CONTENT_TYPE_JSON = "application/json";
public static final String CONTENT_TYPE_FORM_URLENCODED = "application/x-www-form-urlencoded";
public static final String CONTENT_TYPE_MULTIPART = "multipart/form-data";
public static final String EMPTY_JSON_BODY = "{}";
private HttpUtilityExtra() {}
}

Expand Down
94 changes: 46 additions & 48 deletions src/main/java/com/skyflow/utils/HttpUtility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,102 +2,92 @@

import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import com.skyflow.errors.ErrorMessage;
import com.skyflow.errors.SkyflowException;

import java.io.*;
import java.io.BufferedReader;
import java.io.DataOutputStream;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.Reader;
import java.net.HttpURLConnection;
import java.net.URL;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.UUID;

public final class HttpUtility {

private static final String LINE_FEED = "\r\n";
private static String requestID;
// Per-thread so concurrent requests do not race on a single shared slot.
private static final ThreadLocal<String> REQUEST_ID = new ThreadLocal<>();

public static String getRequestID() {
return requestID;
return REQUEST_ID.get();
}

public static String sendRequest(String method, URL url, JsonObject params, Map<String, String> headers) throws IOException, SkyflowException {

HttpURLConnection connection = null;
BufferedReader in = null;
StringBuffer response = null;
StringBuilder response = null;
String boundary = String.valueOf(System.currentTimeMillis());

try {
connection = (HttpURLConnection) url.openConnection();
connection.setRequestMethod(method);
connection.setRequestProperty("Accept", "*/*");
boolean hasContentType = headers != null && headers.containsKey("content-type");
connection.setRequestProperty(Constants.HttpUtilityExtra.HEADER_ACCEPT, Constants.HttpUtilityExtra.ACCEPT_ALL);
boolean hasContentType = headers != null && headers.containsKey(Constants.HttpUtilityExtra.HEADER_CONTENT_TYPE);
if (!hasContentType && params != null && !params.isEmpty()) {
connection.setRequestProperty("content-type", "application/json");
connection.setRequestProperty(Constants.HttpUtilityExtra.HEADER_CONTENT_TYPE, Constants.HttpUtilityExtra.CONTENT_TYPE_JSON);
}

if (headers != null && !headers.isEmpty()) {
for (Map.Entry<String, String> entry : headers.entrySet())
for (Map.Entry<String, String> entry : headers.entrySet()) {
connection.setRequestProperty(entry.getKey(), entry.getValue());
}

// append dynamic boundary if content-type is multipart/form-data
if (headers.containsKey("content-type")) {
if (Objects.equals(headers.get("content-type"), "multipart/form-data")) {
connection.setRequestProperty("content-type", "multipart/form-data; boundary=" + boundary);
}
if (Constants.HttpUtilityExtra.CONTENT_TYPE_MULTIPART.equals(headers.get(Constants.HttpUtilityExtra.HEADER_CONTENT_TYPE))) {
connection.setRequestProperty(Constants.HttpUtilityExtra.HEADER_CONTENT_TYPE,
Constants.HttpUtilityExtra.CONTENT_TYPE_MULTIPART + "; boundary=" + boundary);
}
}
if (params != null && !params.isEmpty()) {
connection.setDoOutput(true);
try (DataOutputStream wr = new DataOutputStream(connection.getOutputStream())) {
byte[] input = null;
String requestContentType = connection.getRequestProperty("content-type");

if (requestContentType != null && requestContentType.contains("application/x-www-form-urlencoded")) {
input = formatJsonToFormEncodedString(params).getBytes(StandardCharsets.UTF_8);
} else if (requestContentType != null && requestContentType.contains("multipart/form-data")) {
input = formatJsonToMultiPartFormDataString(params, boundary).getBytes(StandardCharsets.UTF_8);
} else {
input = params.toString().getBytes(StandardCharsets.UTF_8);
}

byte[] input = encodeRequestBody(params, connection.getRequestProperty(Constants.HttpUtilityExtra.HEADER_CONTENT_TYPE), boundary);
wr.write(input, 0, input.length);
wr.flush();
}
}

int httpCode = connection.getResponseCode();
String requestID = connection.getHeaderField("x-request-id");
if (requestID != null) {
HttpUtility.requestID = requestID.split(",")[0];
} else {
HttpUtility.requestID = Constants.HttpUtilityExtra.SDK_GENERATED_PREFIX + UUID.randomUUID();
}
String responseRequestId = connection.getHeaderField(Constants.HttpUtilityExtra.HEADER_REQUEST_ID);
REQUEST_ID.set(responseRequestId != null ? responseRequestId.split(",")[0] : null);
Map<String, List<String>> responseHeaders = connection.getHeaderFields();
Reader streamReader;
if (httpCode > 299) {
if (connection.getErrorStream() != null)
if (httpCode > Constants.HttpUtilityExtra.HTTP_SUCCESS_STATUS_MAX) {
if (connection.getErrorStream() != null) {
streamReader = new InputStreamReader(connection.getErrorStream());
else {
String description = appendRequestId("replace with description", requestID);
throw new SkyflowException(description);
} else {
String description = appendRequestId(ErrorMessage.ErrorOccurred.getMessage(), REQUEST_ID.get());
throw new SkyflowException(httpCode, new Throwable(description), responseHeaders, Constants.HttpUtilityExtra.EMPTY_JSON_BODY);
}
} else {
streamReader = new InputStreamReader(connection.getInputStream());
}

response = new StringBuffer();
response = new StringBuilder();
in = new BufferedReader(streamReader);
String inputLine;
while ((inputLine = in.readLine()) != null) {
response.append(inputLine);
}

if (httpCode > 299) {
if (httpCode > Constants.HttpUtilityExtra.HTTP_SUCCESS_STATUS_MAX) {
throw new SkyflowException(httpCode, new Throwable(), responseHeaders, response.toString());
}
} finally {
Expand All @@ -112,12 +102,23 @@ public static String sendRequest(String method, URL url, JsonObject params, Map<

}

private static byte[] encodeRequestBody(JsonObject params, String requestContentType, String boundary) {
if (requestContentType != null && requestContentType.contains(Constants.HttpUtilityExtra.CONTENT_TYPE_FORM_URLENCODED)) {
return formatJsonToFormEncodedString(params).getBytes(StandardCharsets.UTF_8);
} else if (requestContentType != null && requestContentType.contains(Constants.HttpUtilityExtra.CONTENT_TYPE_MULTIPART)) {
return formatJsonToMultiPartFormDataString(params, boundary).getBytes(StandardCharsets.UTF_8);
} else {
return params.toString().getBytes(StandardCharsets.UTF_8);
}
}

public static String formatJsonToFormEncodedString(JsonObject requestBody) {
StringBuilder formEncodeString = new StringBuilder();
HashMap<String, String> jsonMap = convertJsonToMap(requestBody, "");

for (Map.Entry<String, String> currentEntry : jsonMap.entrySet())
for (Map.Entry<String, String> currentEntry : jsonMap.entrySet()) {
formEncodeString.append(makeFormEncodeKeyValuePair(currentEntry.getKey(), currentEntry.getValue()));
}

return formEncodeString.length() == 0 ? "" : formEncodeString.substring(0, formEncodeString.length() - 1);
}
Expand All @@ -126,8 +127,9 @@ public static String formatJsonToMultiPartFormDataString(JsonObject requestBody,
StringBuilder formEncodeString = new StringBuilder();
HashMap<String, String> jsonMap = convertJsonToMap(requestBody, "");

for (Map.Entry<String, String> currentEntry : jsonMap.entrySet())
for (Map.Entry<String, String> currentEntry : jsonMap.entrySet()) {
formEncodeString.append(makeFormDataKeyValuePair(currentEntry.getKey(), currentEntry.getValue(), boundary));
}

formEncodeString.append(LINE_FEED);
formEncodeString.append("--").append(boundary).append("--").append(LINE_FEED);
Expand Down Expand Up @@ -162,19 +164,15 @@ private static String makeFormDataKeyValuePair(String key, String value, String

public static String appendRequestId(String message, String requestId) {
if (requestId != null && !requestId.isEmpty()) {
message = message + " - requestId: " + requestId;
return message + " - requestId: " + requestId;
}
return message;
}

private static String makeFormEncodeKeyValuePair(String key, String value) {
try {
String encodedKey = URLEncoder.encode(key, StandardCharsets.UTF_8.toString());
String encodedValue = URLEncoder.encode(value, StandardCharsets.UTF_8.toString());
return encodedKey + "=" + encodedValue + "&";
} catch (Exception e) {
return key + "=" + value + "&";
}
String encodedKey = URLEncoder.encode(key, StandardCharsets.UTF_8);
String encodedValue = URLEncoder.encode(value, StandardCharsets.UTF_8);
return encodedKey + "=" + encodedValue + "&";
}

}
4 changes: 2 additions & 2 deletions src/test/java/com/skyflow/ConnectionClientDotenvTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public void testPrioritiseCredentials_dotenvReturnsCredentials_setsCredentials()

ConnectionClient client = buildClientWithNoCreds("dotenv-valid-1");
// updateConnectionConfig calls prioritiseCredentials which reads from .env
client.updateConnectionConfig(client.getConnectionConfig());
client.updateConnectionConfig();
}

@Test
Expand All @@ -72,7 +72,7 @@ public void testPrioritiseCredentials_dotenvReturnsNullKey_throwsSkyflowExceptio
ConnectionClient client = buildClientWithNoCreds("dotenv-null-1");
// Null sysCredentials → SkyflowException thrown directly
try {
client.updateConnectionConfig(client.getConnectionConfig());
client.updateConnectionConfig();
Assert.fail("Should have thrown SkyflowException");
} catch (SkyflowException e) {
Assert.assertTrue(e.getMessage().contains(ErrorMessage.EmptyCredentials.getMessage()));
Expand Down
10 changes: 5 additions & 5 deletions src/test/java/com/skyflow/ConnectionClientTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public void testSetBearerToken() {
Credentials credentials = new Credentials();
credentials.setToken(bearerToken);
connectionConfig.setCredentials(credentials);
connectionClient.updateConnectionConfig(connectionConfig);
connectionClient.updateConnectionConfig();

// regular scenario
connectionClient.setBearerToken();
Expand All @@ -69,7 +69,7 @@ public void testSetBearerTokenWithApiKey() {
Credentials credentials = new Credentials();
credentials.setApiKey(apiKey);
connectionConfig.setCredentials(null);
connectionClient.updateConnectionConfig(connectionConfig);
connectionClient.updateConnectionConfig();
connectionClient.setCommonCredentials(credentials);

// regular scenario
Expand All @@ -86,7 +86,7 @@ public void testSetBearerTokenWithApiKey() {
public void testSetBearerTokenWithEnvCredentials() {
try {
connectionConfig.setCredentials(null);
connectionClient.updateConnectionConfig(connectionConfig);
connectionClient.updateConnectionConfig();
connectionClient.setCommonCredentials(null);
Assert.assertNull(connectionClient.getConnectionConfig().getCredentials());
} catch (Exception e) {
Expand Down Expand Up @@ -152,15 +152,15 @@ public void testPrioritiseCredentials_credentialChange_resetsToken() {
config.setCredentials(credentialsA);
ConnectionClient client = new ConnectionClient(config, null);

client.updateConnectionConfig(config); // sets finalCredentials = credentialsA (original=null → no reset)
client.updateConnectionConfig(); // sets finalCredentials = credentialsA (original=null → no reset)
client.token = "cached-token-value"; // simulate previously obtained bearer token

// Change to different credentials object
Credentials credentialsB = new Credentials();
credentialsB.setToken("different-token");
config.setCredentials(credentialsB);

client.updateConnectionConfig(config); // original=A, new=B → !A.equals(B) → reset (lines 83-84)
client.updateConnectionConfig(); // original=A, new=B → !A.equals(B) → reset (lines 83-84)
Assert.assertNull(client.token);
Assert.assertNull(client.apiKey);
} catch (Exception e) {
Expand Down
5 changes: 2 additions & 3 deletions src/test/java/com/skyflow/VaultClientTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1043,7 +1043,7 @@ public void testGetDeIdentifyTextResponse_withEntityScores() {
}

@Test
public void testPrioritiseCredentials_credentialChange_resetsTokenAndApiKey() {
public void testPrioritiseCredentials_credentialChange_resetsToken() {
try {
Credentials credentialsA = new Credentials();
credentialsA.setToken("x.eyJleHAiOjk5OTk5OTk5OTl9.y");
Expand All @@ -1061,9 +1061,8 @@ public void testPrioritiseCredentials_credentialChange_resetsTokenAndApiKey() {
credentialsB.setToken("other-token");
config.setCredentials(credentialsB);

freshClient.updateVaultConfig(); // original=A, new=B → different → reset token/apiKey
freshClient.updateVaultConfig(); // original=A, new=B → different → reset token
Assert.assertNull(getPrivateField(freshClient, "token"));
Assert.assertNull(getPrivateField(freshClient, "apiKey"));
} catch (Exception e) {
Assert.fail("Should not have thrown: " + e.getMessage());
}
Expand Down
Loading
Loading