Skip to content

Commit

Permalink
NO-SNOW: Revert one change that updates public API for internal use c…
Browse files Browse the repository at this point in the history
…ase (#662)

Revert 2b702db

This change updates the public API for an internal special use case only which is not ideal, looks like we keep updating public APIs in #661 as well. Given that we're still in a POC phase, I suggest we do everything in another branch instead, hence removing this from the next public release and could discuss what's the best way to support this internal use case once the POC is done.
  • Loading branch information
sfc-gh-tzhang authored Jan 17, 2024
1 parent e0f575e commit db1fd02
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 153 deletions.
82 changes: 0 additions & 82 deletions src/main/java/net/snowflake/ingest/connection/RequestBuilder.java
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

0 comments on commit db1fd02

Please sign in to comment.