Skip to content
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.

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

NO-SNOW: Revert one change that updates public API for internal use case #662

Merged
merged 3 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,6 @@ public class RequestBuilder {
// whatever the actual host is
private final String host;

private final String accountName;

private final boolean addAccountNameInRequest;

private final String userAgentSuffix;

// Reference to the telemetry service
Expand Down Expand Up @@ -127,8 +123,6 @@ public class RequestBuilder {

public static final String HTTP_HEADER_CONTENT_TYPE_JSON = "application/json";

private static final String SF_HEADER_ACCOUNT_NAME = "Snowflake-Account";

/**
* RequestBuilder - general usage constructor
*
Expand Down Expand Up @@ -212,36 +206,6 @@ public RequestBuilder(
null);
}

/**
* RequestBuilder - constructor used by streaming ingest
*
* @param url - the Snowflake account to which we're connecting
* @param userName - the username of the entity loading files
* @param credential - the credential we'll use to authenticate
* @param httpClient - reference to the http client
* @param clientName - name of the client, used to uniquely identify a client if used
*/
public RequestBuilder(
SnowflakeURL url,
String userName,
Object credential,
CloseableHttpClient httpClient,
String clientName,
boolean addAccountNameInRequest) {
this(
url.getAccount(),
userName,
credential,
url.getScheme(),
url.getUrlWithoutPort(),
url.getPort(),
null,
null,
httpClient,
clientName,
addAccountNameInRequest);
}

/**
* RequestBuilder - constructor used by streaming ingest
*
Expand Down Expand Up @@ -295,47 +259,6 @@ public RequestBuilder(
SecurityManager securityManager,
CloseableHttpClient httpClient,
String clientName) {
this(
accountName,
userName,
credential,
schemeName,
hostName,
portNum,
userAgentSuffix,
securityManager,
httpClient,
clientName,
false);
}

/**
* RequestBuilder - this constructor is for testing purposes only
*
* @param accountName - the account name to which we're connecting
* @param userName - for whom are we connecting?
* @param credential - our auth credentials, either JWT key pair or OAuth credential
* @param schemeName - are we HTTP or HTTPS?
* @param hostName - the host for this snowflake instance
* @param portNum - the port number
* @param userAgentSuffix - The suffix part of HTTP Header User-Agent
* @param securityManager - The security manager for authentication
* @param httpClient - reference to the http client
* @param clientName - name of the client, used to uniquely identify a client if used
* @param addAccountNameInRequest if ture, add account name in request header
*/
public RequestBuilder(
String accountName,
String userName,
Object credential,
String schemeName,
String hostName,
int portNum,
String userAgentSuffix,
SecurityManager securityManager,
CloseableHttpClient httpClient,
String clientName,
boolean addAccountNameInRequest) {
// none of these arguments should be null
if (accountName == null || userName == null || credential == null) {
throw new IllegalArgumentException();
Expand All @@ -358,8 +281,6 @@ public RequestBuilder(
this.scheme = schemeName;
this.host = hostName;
this.userAgentSuffix = userAgentSuffix;
this.accountName = accountName;
this.addAccountNameInRequest = addAccountNameInRequest;

// create our security/token manager
if (securityManager == null) {
Expand Down Expand Up @@ -649,9 +570,6 @@ private static void addUserAgent(HttpUriRequest request, String userAgentSuffix)
public void addToken(HttpUriRequest request) {
request.setHeader(HttpHeaders.AUTHORIZATION, BEARER_PARAMETER + securityManager.getToken());
request.setHeader(SF_HEADER_AUTHORIZATION_TOKEN_TYPE, this.securityManager.getTokenType());
if (addAccountNameInRequest) {
request.setHeader(SF_HEADER_ACCOUNT_NAME, accountName);
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,8 @@ public static class Builder {
// Allows client to override some default parameter values
private Map<String, Object> parameterOverrides;

// preset SnowflakeURL instance
private SnowflakeURL snowflakeURL;

// flag to specify if we need to add account name in the request header
private boolean addAccountNameInRequest;
// Indicates whether it's under test mode
private boolean isTestMode;

private Builder(String name) {
this.name = name;
Expand All @@ -43,18 +40,13 @@ public Builder setProperties(Properties prop) {
return this;
}

public Builder setSnowflakeURL(SnowflakeURL snowflakeURL) {
this.snowflakeURL = snowflakeURL;
return this;
}

public Builder setAddAccountNameInRequest(boolean addAccountNameInRequest) {
this.addAccountNameInRequest = addAccountNameInRequest;
public Builder setParameterOverrides(Map<String, Object> parameterOverrides) {
this.parameterOverrides = parameterOverrides;
return this;
}

public Builder setParameterOverrides(Map<String, Object> parameterOverrides) {
this.parameterOverrides = parameterOverrides;
public Builder setIsTestMode(boolean isTestMode) {
this.isTestMode = isTestMode;
return this;
}

Expand All @@ -63,17 +55,10 @@ public SnowflakeStreamingIngestClient build() {
Utils.assertNotNull("connection properties", this.prop);

Properties prop = Utils.createProperties(this.prop);
SnowflakeURL accountURL = this.snowflakeURL;
if (accountURL == null) {
accountURL = new SnowflakeURL(prop.getProperty(Constants.ACCOUNT_URL));
}
SnowflakeURL accountURL = new SnowflakeURL(prop.getProperty(Constants.ACCOUNT_URL));

if (addAccountNameInRequest) {
return new SnowflakeStreamingIngestClientInternal<>(
this.name, accountURL, prop, this.parameterOverrides, addAccountNameInRequest);
}
return new SnowflakeStreamingIngestClientInternal<>(
this.name, accountURL, prop, this.parameterOverrides);
this.name, accountURL, prop, this.parameterOverrides, this.isTestMode);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -158,30 +158,6 @@ public class SnowflakeStreamingIngestClientInternal<T> implements SnowflakeStrea
boolean isTestMode,
RequestBuilder requestBuilder,
Map<String, Object> parameterOverrides) {
this(name, accountURL, prop, httpClient, isTestMode, requestBuilder, parameterOverrides, false);
}

/**
* Constructor
*
* @param name the name of the client
* @param accountURL Snowflake account url
* @param prop connection properties
* @param httpClient http client for sending request
* @param isTestMode whether we're under test mode
* @param requestBuilder http request builder
* @param parameterOverrides parameters we override in case we want to set different values
* @param addAccountNameInRequest if true, will add account name in request header
*/
SnowflakeStreamingIngestClientInternal(
String name,
SnowflakeURL accountURL,
Properties prop,
CloseableHttpClient httpClient,
boolean isTestMode,
RequestBuilder requestBuilder,
Map<String, Object> parameterOverrides,
boolean addAccountNameInRequest) {
this.parameterProvider = new ParameterProvider(parameterOverrides, prop);

this.name = name;
Expand Down Expand Up @@ -220,8 +196,7 @@ public class SnowflakeStreamingIngestClientInternal<T> implements SnowflakeStrea
prop.get(USER).toString(),
credential,
this.httpClient,
String.format("%s_%s", this.name, System.currentTimeMillis()),
addAccountNameInRequest);
String.format("%s_%s", this.name, System.currentTimeMillis()));

logger.logInfo("Using {} for authorization", this.requestBuilder.getAuthType());

Expand Down Expand Up @@ -252,35 +227,18 @@ public class SnowflakeStreamingIngestClientInternal<T> implements SnowflakeStrea
* @param accountURL Snowflake account url
* @param prop connection properties
* @param parameterOverrides map of parameters to override for this client
*/
public SnowflakeStreamingIngestClientInternal(
String name,
SnowflakeURL accountURL,
Properties prop,
Map<String, Object> parameterOverrides) {
this(name, accountURL, prop, null, false, null, parameterOverrides);
}

/**
* Default Constructor
*
* @param name the name of the client
* @param accountURL Snowflake account url
* @param prop connection properties
* @param parameterOverrides map of parameters to override for this client
* @param addAccountNameInRequest if true, add account name in request header
* @param isTestMode indicates whether it's under test mode
*/
public SnowflakeStreamingIngestClientInternal(
String name,
SnowflakeURL accountURL,
Properties prop,
Map<String, Object> parameterOverrides,
boolean addAccountNameInRequest) {
this(name, accountURL, prop, null, false, null, parameterOverrides, addAccountNameInRequest);
boolean isTestMode) {
this(name, accountURL, prop, null, isTestMode, null, parameterOverrides);
}

/**
* Constructor for TEST ONLY
/*** Constructor for TEST ONLY
*
* @param name the name of the client
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ public void testConstructorParameters() throws Exception {
SnowflakeStreamingIngestClientFactory.builder("client")
.setProperties(prop)
.setParameterOverrides(parameterMap)
.setIsTestMode(true)
.build();

Assert.assertEquals("client", client.getName());
Expand Down Expand Up @@ -263,7 +264,10 @@ public void testClientFactorySuccess() throws Exception {
prop.put(ROLE, TestUtils.getRole());

SnowflakeStreamingIngestClient client =
SnowflakeStreamingIngestClientFactory.builder("client").setProperties(prop).build();
SnowflakeStreamingIngestClientFactory.builder("client")
.setProperties(prop)
.setIsTestMode(true)
.build();

Assert.assertEquals("client", client.getName());
Assert.assertFalse(client.isClosed());
Expand Down
Loading