Skip to content

Commit

Permalink
feat: allow lenient mode for connection properties (#671)
Browse files Browse the repository at this point in the history 
* feat: allow lenient mode for connection properties

Some applications automatically add additional properties to connection strings
that are unknown to the Spanner Connection API (and thereby also the Spanner
JDBC driver). This causes the connection attempt to fail. This change allows a
user to specify 'lenient' mode where unknown properties only generate a warning
instead of an error.

Fixes dropwizard/dropwizard#3461
Fixes googleapis/google-cloud-java#6671
Fixes googleapis/java-spanner-jdbc#283

* fix: add credentials to prevent tests from trying to use env credentials
  • Loading branch information
@olavloite
olavloite committed Nov 30, 2020
1 parent 3f9f74a commit f6a8ba6
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 6 deletions.
42 changes: 36 additions & 6 deletions ...le-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionOptions.java
View file
Expand Up @@ -42,6 +42,7 @@
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import javax.annotation.Nullable;

/**
* Internal connection API for Google Cloud Spanner. This class may introduce breaking changes
Expand Down Expand Up @@ -152,6 +153,7 @@ public String[] getValidValues() {
private static final String DEFAULT_NUM_CHANNELS = null;
private static final String DEFAULT_USER_AGENT = null;
private static final String DEFAULT_OPTIMIZER_VERSION = "";
private static final boolean DEFAULT_LENIENT = false;

private static final String PLAIN_TEXT_PROTOCOL = "http:";
private static final String HOST_PROTOCOL = "https:";
Expand All @@ -176,6 +178,8 @@ public String[] getValidValues() {
private static final String USER_AGENT_PROPERTY_NAME = "userAgent";
/** Query optimizer version to use for a connection. */
private static final String OPTIMIZER_VERSION_PROPERTY_NAME = "optimizerVersion";
/** Name of the 'lenientMode' connection property. */
public static final String LENIENT_PROPERTY_NAME = "lenient";

/** All valid connection properties. */
public static final Set<ConnectionProperty> VALID_PROPERTIES =
Expand Down Expand Up @@ -212,7 +216,11 @@ public String[] getValidValues() {
"The custom user-agent property name to use when communicating with Cloud Spanner. This property is intended for internal library usage, and should not be set by applications."),
ConnectionProperty.createStringProperty(
OPTIMIZER_VERSION_PROPERTY_NAME,
"Sets the default query optimizer version to use for this connection."))));
"Sets the default query optimizer version to use for this connection."),
ConnectionProperty.createBooleanProperty(
LENIENT_PROPERTY_NAME,
"Silently ignore unknown properties in the connection string/properties (true/false)",
DEFAULT_LENIENT))));

private static final Set<ConnectionProperty> INTERNAL_PROPERTIES =
Collections.unmodifiableSet(
Expand Down Expand Up @@ -416,6 +424,7 @@ public static Builder newBuilder() {
}

private final String uri;
private final String warnings;
private final String credentialsUrl;
private final String oauthToken;
private final Credentials fixedCredentials;
Expand All @@ -441,7 +450,7 @@ private ConnectionOptions(Builder builder) {
Matcher matcher = Builder.SPANNER_URI_PATTERN.matcher(builder.uri);
Preconditions.checkArgument(
matcher.find(), String.format("Invalid connection URI specified: %s", builder.uri));
checkValidProperties(builder.uri);
this.warnings = checkValidProperties(builder.uri);

this.uri = builder.uri;
this.sessionPoolOptions = builder.sessionPoolOptions;
Expand Down Expand Up @@ -574,6 +583,12 @@ static String parseOptimizerVersion(String uri) {
return value != null ? value : DEFAULT_OPTIMIZER_VERSION;
}

@VisibleForTesting
static boolean parseLenient(String uri) {
String value = parseUriProperty(uri, LENIENT_PROPERTY_NAME);
return value != null ? Boolean.valueOf(value) : DEFAULT_LENIENT;
}

@VisibleForTesting
static String parseUriProperty(String uri, String property) {
Pattern pattern = Pattern.compile(String.format("(?is)(?:;|\\?)%s=(.*?)(?:;|$)", property));
Expand All @@ -586,9 +601,10 @@ static String parseUriProperty(String uri, String property) {

/** Check that only valid properties have been specified. */
@VisibleForTesting
static void checkValidProperties(String uri) {
static String checkValidProperties(String uri) {
String invalidProperties = "";
List<String> properties = parseProperties(uri);
boolean lenient = parseLenient(uri);
for (String property : properties) {
if (!INTERNAL_VALID_PROPERTIES.contains(ConnectionProperty.createEmptyProperty(property))) {
if (invalidProperties.length() > 0) {
Expand All @@ -597,9 +613,17 @@ static void checkValidProperties(String uri) {
invalidProperties = invalidProperties + property;
}
}
Preconditions.checkArgument(
invalidProperties.isEmpty(),
"Invalid properties found in connection URI: " + invalidProperties.toString());
if (lenient) {
return String.format(
"Invalid properties found in connection URI: %s", invalidProperties.toString());
} else {
Preconditions.checkArgument(
invalidProperties.isEmpty(),
String.format(
"Invalid properties found in connection URI. Add lenient=true to the connection string to ignore unknown properties. Invalid properties: %s",
invalidProperties.toString()));
return null;
}
}

@VisibleForTesting
Expand Down Expand Up @@ -706,6 +730,12 @@ public boolean isRetryAbortsInternally() {
return retryAbortsInternally;
}

/** Any warnings that were generated while creating the {@link ConnectionOptions} instance. */
@Nullable
public String getWarnings() {
return warnings;
}

/** Use http instead of https. Only valid for (local) test servers. */
boolean isUsePlainText() {
return usePlainText;
Expand Down
35 changes: 35 additions & 0 deletions ...loud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionOptionsTest.java
View file
Expand Up @@ -386,4 +386,39 @@ public void testSetOAuthTokenAndCredentials() {
assertThat(e.getMessage()).contains("Cannot specify both credentials and an OAuth token");
}
}

@Test
public void testLenient() {
ConnectionOptions options =
ConnectionOptions.newBuilder()
.setUri(
"cloudspanner:/projects/test-project-123/instances/test-instance/databases/test-database?lenient=true;foo=bar")
.setCredentialsUrl(FILE_TEST_PATH)
.build();
assertThat(options.getWarnings()).isNotNull();
assertThat(options.getWarnings()).contains("foo");
assertThat(options.getWarnings()).doesNotContain("lenient");

options =
ConnectionOptions.newBuilder()
.setUri(
"cloudspanner:/projects/test-project-123/instances/test-instance/databases/test-database?bar=foo;lenient=true")
.setCredentialsUrl(FILE_TEST_PATH)
.build();
assertThat(options.getWarnings()).isNotNull();
assertThat(options.getWarnings()).contains("bar");
assertThat(options.getWarnings()).doesNotContain("lenient");

try {
options =
ConnectionOptions.newBuilder()
.setUri(
"cloudspanner:/projects/test-project-123/instances/test-instance/databases/test-database?bar=foo")
.setCredentialsUrl(FILE_TEST_PATH)
.build();
fail("missing expected exception");
} catch (IllegalArgumentException e) {
assertThat(e.getMessage()).contains("bar");
}
}
}

0 comments on commit f6a8ba6

Please sign in to comment.