Skip to content

Commit

Permalink
fix: allow dots and colons in project id (#36)
Browse files Browse the repository at this point in the history 
* fix: allow dots and colons in project id

The project ID part of a connection URL should allow the use of dots
and colons.

Fixes #33

* fix: escaping dot in square brackets is not needed
  • Loading branch information
@olavloite
olavloite committed Jan 22, 2020
1 parent 769fba6 commit 5957008
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 133 deletions.
2 changes: 1 addition & 1 deletion src/main/java/com/google/cloud/spanner/jdbc/ConnectionOptions.java
View file
Expand Up @@ -231,7 +231,7 @@ private Builder() {}

/** Spanner {@link ConnectionOptions} URI format. */
public static final String SPANNER_URI_FORMAT =
"(?:cloudspanner:)(?<HOSTGROUP>//[\\w.-]+(?:\\.[\\w\\.-]+)*[\\w\\-\\._~:/?#\\[\\]@!\\$&'\\(\\)\\*\\+,;=.]+)?/projects/(?<PROJECTGROUP>(([a-z]|[-]|[0-9])+|(DEFAULT_PROJECT_ID)))(/instances/(?<INSTANCEGROUP>([a-z]|[-]|[0-9])+)(/databases/(?<DATABASEGROUP>([a-z]|[-]|[_]|[0-9])+))?)?(?:[?|;].*)?";
"(?:cloudspanner:)(?<HOSTGROUP>//[\\w.-]+(?:\\.[\\w\\.-]+)*[\\w\\-\\._~:/?#\\[\\]@!\\$&'\\(\\)\\*\\+,;=.]+)?/projects/(?<PROJECTGROUP>(([a-z]|[-.:]|[0-9])+|(DEFAULT_PROJECT_ID)))(/instances/(?<INSTANCEGROUP>([a-z]|[-]|[0-9])+)(/databases/(?<DATABASEGROUP>([a-z]|[-]|[_]|[0-9])+))?)?(?:[?|;].*)?";

private static final String SPANNER_URI_REGEX = "(?is)^" + SPANNER_URI_FORMAT + "$";
private static final Pattern SPANNER_URI_PATTERN = Pattern.compile(SPANNER_URI_REGEX);
Expand Down
244 changes: 118 additions & 126 deletions src/test/java/com/google/cloud/spanner/jdbc/ConnectionOptionsTest.java
View file
Expand Up @@ -16,12 +16,9 @@

package com.google.cloud.spanner.jdbc;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.junit.Assert.assertThat;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.fail;

import com.google.auth.oauth2.GoogleCredentials;
import com.google.auth.oauth2.ServiceAccountCredentials;
import com.google.cloud.spanner.SpannerOptions;
import java.util.Arrays;
Expand All @@ -35,22 +32,38 @@ public class ConnectionOptionsTest {
ConnectionOptionsTest.class.getResource("test-key.json").getFile();
private static final String DEFAULT_HOST = "https://spanner.googleapis.com";

@Test
public void testBuildWithURIWithDots() {
ConnectionOptions.Builder builder = ConnectionOptions.newBuilder();
builder.setUri(
"cloudspanner:/projects/some-company.com:test-project-123/instances/test-instance-123/databases/test-database-123");
builder.setCredentialsUrl(FILE_TEST_PATH);
ConnectionOptions options = builder.build();
assertThat(options.getHost()).isEqualTo(DEFAULT_HOST);
assertThat(options.getProjectId()).isEqualTo("some-company.com:test-project-123");
assertThat(options.getInstanceId()).isEqualTo("test-instance-123");
assertThat(options.getDatabaseName()).isEqualTo("test-database-123");
assertThat(options.getCredentials())
.isEqualTo(new CredentialsService().createCredentials(FILE_TEST_PATH));
assertThat(options.isAutocommit()).isEqualTo(ConnectionOptions.DEFAULT_AUTOCOMMIT);
assertThat(options.isReadOnly()).isEqualTo(ConnectionOptions.DEFAULT_READONLY);
}

@Test
public void testBuildWithValidURIAndCredentialsFileURL() {
ConnectionOptions.Builder builder = ConnectionOptions.newBuilder();
builder.setUri(
"cloudspanner:/projects/test-project-123/instances/test-instance-123/databases/test-database-123");
builder.setCredentialsUrl(FILE_TEST_PATH);
ConnectionOptions options = builder.build();
assertThat(options.getHost(), is(equalTo(DEFAULT_HOST)));
assertThat(options.getProjectId(), is(equalTo("test-project-123")));
assertThat(options.getInstanceId(), is(equalTo("test-instance-123")));
assertThat(options.getDatabaseName(), is(equalTo("test-database-123")));
assertThat(
(GoogleCredentials) options.getCredentials(),
is(equalTo(new CredentialsService().createCredentials(FILE_TEST_PATH))));
assertThat(options.isAutocommit(), is(equalTo(ConnectionOptions.DEFAULT_AUTOCOMMIT)));
assertThat(options.isReadOnly(), is(equalTo(ConnectionOptions.DEFAULT_READONLY)));
assertThat(options.getHost()).isEqualTo(DEFAULT_HOST);
assertThat(options.getProjectId()).isEqualTo("test-project-123");
assertThat(options.getInstanceId()).isEqualTo("test-instance-123");
assertThat(options.getDatabaseName()).isEqualTo("test-database-123");
assertThat(options.getCredentials())
.isEqualTo(new CredentialsService().createCredentials(FILE_TEST_PATH));
assertThat(options.isAutocommit()).isEqualTo(ConnectionOptions.DEFAULT_AUTOCOMMIT);
assertThat(options.isReadOnly()).isEqualTo(ConnectionOptions.DEFAULT_READONLY);
}

@Test
Expand All @@ -60,15 +73,14 @@ public void testBuildWithValidURIAndProperties() {
"cloudspanner:/projects/test-project-123/instances/test-instance-123/databases/test-database-123?autocommit=false;readonly=true");
builder.setCredentialsUrl(FILE_TEST_PATH);
ConnectionOptions options = builder.build();
assertThat(options.getHost(), is(equalTo(DEFAULT_HOST)));
assertThat(options.getProjectId(), is(equalTo("test-project-123")));
assertThat(options.getInstanceId(), is(equalTo("test-instance-123")));
assertThat(options.getDatabaseName(), is(equalTo("test-database-123")));
assertThat(
(GoogleCredentials) options.getCredentials(),
is(equalTo(new CredentialsService().createCredentials(FILE_TEST_PATH))));
assertThat(options.isAutocommit(), is(equalTo(false)));
assertThat(options.isReadOnly(), is(equalTo(true)));
assertThat(options.getHost()).isEqualTo(DEFAULT_HOST);
assertThat(options.getProjectId()).isEqualTo("test-project-123");
assertThat(options.getInstanceId()).isEqualTo("test-instance-123");
assertThat(options.getDatabaseName()).isEqualTo("test-database-123");
assertThat(options.getCredentials())
.isEqualTo(new CredentialsService().createCredentials(FILE_TEST_PATH));
assertThat(options.isAutocommit()).isEqualTo(false);
assertThat(options.isReadOnly()).isEqualTo(true);
}

@Test
Expand All @@ -78,15 +90,14 @@ public void testBuildWithHostAndValidURI() {
"cloudspanner://test-spanner.googleapis.com/projects/test-project-123/instances/test-instance-123/databases/test-database-123");
builder.setCredentialsUrl(FILE_TEST_PATH);
ConnectionOptions options = builder.build();
assertThat(options.getHost(), is(equalTo("https://test-spanner.googleapis.com")));
assertThat(options.getProjectId(), is(equalTo("test-project-123")));
assertThat(options.getInstanceId(), is(equalTo("test-instance-123")));
assertThat(options.getDatabaseName(), is(equalTo("test-database-123")));
assertThat(
(GoogleCredentials) options.getCredentials(),
is(equalTo(new CredentialsService().createCredentials(FILE_TEST_PATH))));
assertThat(options.isAutocommit(), is(equalTo(ConnectionOptions.DEFAULT_AUTOCOMMIT)));
assertThat(options.isReadOnly(), is(equalTo(ConnectionOptions.DEFAULT_READONLY)));
assertThat(options.getHost()).isEqualTo("https://test-spanner.googleapis.com");
assertThat(options.getProjectId()).isEqualTo("test-project-123");
assertThat(options.getInstanceId()).isEqualTo("test-instance-123");
assertThat(options.getDatabaseName()).isEqualTo("test-database-123");
assertThat(options.getCredentials())
.isEqualTo(new CredentialsService().createCredentials(FILE_TEST_PATH));
assertThat(options.isAutocommit()).isEqualTo(ConnectionOptions.DEFAULT_AUTOCOMMIT);
assertThat(options.isReadOnly()).isEqualTo(ConnectionOptions.DEFAULT_READONLY);
}

@Test
Expand All @@ -96,15 +107,14 @@ public void testBuildWithLocalhostPortAndValidURI() {
"cloudspanner://localhost:8443/projects/test-project-123/instances/test-instance-123/databases/test-database-123");
builder.setCredentialsUrl(FILE_TEST_PATH);
ConnectionOptions options = builder.build();
assertThat(options.getHost(), is(equalTo("https://localhost:8443")));
assertThat(options.getProjectId(), is(equalTo("test-project-123")));
assertThat(options.getInstanceId(), is(equalTo("test-instance-123")));
assertThat(options.getDatabaseName(), is(equalTo("test-database-123")));
assertThat(
(GoogleCredentials) options.getCredentials(),
is(equalTo(new CredentialsService().createCredentials(FILE_TEST_PATH))));
assertThat(options.isAutocommit(), is(equalTo(ConnectionOptions.DEFAULT_AUTOCOMMIT)));
assertThat(options.isReadOnly(), is(equalTo(ConnectionOptions.DEFAULT_READONLY)));
assertThat(options.getHost()).isEqualTo("https://localhost:8443");
assertThat(options.getProjectId()).isEqualTo("test-project-123");
assertThat(options.getInstanceId()).isEqualTo("test-instance-123");
assertThat(options.getDatabaseName()).isEqualTo("test-database-123");
assertThat(options.getCredentials())
.isEqualTo(new CredentialsService().createCredentials(FILE_TEST_PATH));
assertThat(options.isAutocommit()).isEqualTo(ConnectionOptions.DEFAULT_AUTOCOMMIT);
assertThat(options.isReadOnly()).isEqualTo(ConnectionOptions.DEFAULT_READONLY);
}

@Test
Expand All @@ -114,21 +124,20 @@ public void testBuildWithDefaultProjectPlaceholder() {
"cloudspanner:/projects/default_project_id/instances/test-instance-123/databases/test-database-123");
builder.setCredentialsUrl(FILE_TEST_PATH);
ConnectionOptions options = builder.build();
assertThat(options.getHost(), is(equalTo(DEFAULT_HOST)));
assertThat(options.getHost()).isEqualTo(DEFAULT_HOST);
String projectId = SpannerOptions.getDefaultProjectId();
if (projectId == null) {
projectId =
((ServiceAccountCredentials) new CredentialsService().createCredentials(FILE_TEST_PATH))
.getProjectId();
}
assertThat(options.getProjectId(), is(equalTo(projectId)));
assertThat(options.getInstanceId(), is(equalTo("test-instance-123")));
assertThat(options.getDatabaseName(), is(equalTo("test-database-123")));
assertThat(
(GoogleCredentials) options.getCredentials(),
is(equalTo(new CredentialsService().createCredentials(FILE_TEST_PATH))));
assertThat(options.isAutocommit(), is(equalTo(ConnectionOptions.DEFAULT_AUTOCOMMIT)));
assertThat(options.isReadOnly(), is(equalTo(ConnectionOptions.DEFAULT_READONLY)));
assertThat(options.getProjectId()).isEqualTo(projectId);
assertThat(options.getInstanceId()).isEqualTo("test-instance-123");
assertThat(options.getDatabaseName()).isEqualTo("test-database-123");
assertThat(options.getCredentials())
.isEqualTo(new CredentialsService().createCredentials(FILE_TEST_PATH));
assertThat(options.isAutocommit()).isEqualTo(ConnectionOptions.DEFAULT_AUTOCOMMIT);
assertThat(options.isReadOnly()).isEqualTo(ConnectionOptions.DEFAULT_READONLY);
}

@Test
Expand Down Expand Up @@ -203,129 +212,112 @@ public void testBuilderSetUri() {
}

private void setInvalidUri(ConnectionOptions.Builder builder, String uri) {
boolean invalid = false;
try {
builder.setUri(uri);
fail(uri + " should be considered an invalid uri");
} catch (IllegalArgumentException e) {
invalid = true;
}
assertThat(uri + " should be considered an invalid uri", invalid, is(true));
}

private void setInvalidProperty(
ConnectionOptions.Builder builder, String uri, String expectedInvalidProperties) {
boolean invalid = false;
try {
builder.setUri(uri);
fail("missing expected exception");
} catch (IllegalArgumentException e) {
invalid = e.getMessage().contains(expectedInvalidProperties);
assertThat(e.getMessage()).contains(expectedInvalidProperties);
}
assertThat(uri + " should contain invalid properties", invalid, is(true));
}

@Test
public void testParseUriProperty() {
final String baseUri =
"cloudspanner:/projects/test-project-123/instances/test-instance/databases/test-database";

assertThat(ConnectionOptions.parseUriProperty(baseUri, "autocommit"), is(nullValue()));
assertThat(
ConnectionOptions.parseUriProperty(baseUri + "?autocommit=true", "autocommit"),
is(equalTo("true")));
assertThat(ConnectionOptions.parseUriProperty(baseUri, "autocommit")).isNull();
assertThat(ConnectionOptions.parseUriProperty(baseUri + "?autocommit=true", "autocommit"))
.isEqualTo("true");
assertThat(ConnectionOptions.parseUriProperty(baseUri + "?autocommit=false", "autocommit"))
.isEqualTo("false");
assertThat(ConnectionOptions.parseUriProperty(baseUri + "?autocommit=true;", "autocommit"))
.isEqualTo("true");
assertThat(ConnectionOptions.parseUriProperty(baseUri + "?autocommit=false;", "autocommit"))
.isEqualTo("false");
assertThat(
ConnectionOptions.parseUriProperty(baseUri + "?autocommit=false", "autocommit"),
is(equalTo("false")));
ConnectionOptions.parseUriProperty(
baseUri + "?autocommit=true;readOnly=false", "autocommit"))
.isEqualTo("true");
assertThat(
ConnectionOptions.parseUriProperty(baseUri + "?autocommit=true;", "autocommit"),
is(equalTo("true")));
ConnectionOptions.parseUriProperty(
baseUri + "?autocommit=false;readOnly=false", "autocommit"))
.isEqualTo("false");
assertThat(
ConnectionOptions.parseUriProperty(baseUri + "?autocommit=false;", "autocommit"),
is(equalTo("false")));
ConnectionOptions.parseUriProperty(
baseUri + "?readOnly=false;autocommit=true", "autocommit"))
.isEqualTo("true");
assertThat(
ConnectionOptions.parseUriProperty(
baseUri + "?autocommit=true;readOnly=false", "autocommit"),
is(equalTo("true")));
ConnectionOptions.parseUriProperty(
baseUri + "?readOnly=false;autocommit=false", "autocommit"))
.isEqualTo("false");
assertThat(
ConnectionOptions.parseUriProperty(
baseUri + "?autocommit=false;readOnly=false", "autocommit"),
is(equalTo("false")));
ConnectionOptions.parseUriProperty(
baseUri + "?readOnly=false;autocommit=true;foo=bar", "autocommit"))
.isEqualTo("true");
assertThat(
ConnectionOptions.parseUriProperty(
baseUri + "?readOnly=false;autocommit=true", "autocommit"),
is(equalTo("true")));
assertThat(
ConnectionOptions.parseUriProperty(
baseUri + "?readOnly=false;autocommit=false", "autocommit"),
is(equalTo("false")));
assertThat(
ConnectionOptions.parseUriProperty(
baseUri + "?readOnly=false;autocommit=true;foo=bar", "autocommit"),
is(equalTo("true")));
assertThat(
ConnectionOptions.parseUriProperty(
baseUri + "?readOnly=false;autocommit=false;foo=bar", "autocommit"),
is(equalTo("false")));
ConnectionOptions.parseUriProperty(
baseUri + "?readOnly=false;autocommit=false;foo=bar", "autocommit"))
.isEqualTo("false");

// case insensitive
assertThat(
ConnectionOptions.parseUriProperty(baseUri + "?AutoCommit=true", "autocommit"),
is(equalTo("true")));
assertThat(
ConnectionOptions.parseUriProperty(baseUri + "?AutoCommit=false", "autocommit"),
is(equalTo("false")));
assertThat(ConnectionOptions.parseUriProperty(baseUri + "?AutoCommit=true", "autocommit"))
.isEqualTo("true");
assertThat(ConnectionOptions.parseUriProperty(baseUri + "?AutoCommit=false", "autocommit"))
.isEqualTo("false");

// ; instead of ? before the properties is ok
assertThat(
ConnectionOptions.parseUriProperty(baseUri + ";autocommit=true", "autocommit"),
is(equalTo("true")));
assertThat(ConnectionOptions.parseUriProperty(baseUri + ";autocommit=true", "autocommit"))
.isEqualTo("true");

// forgot the ? or ; before the properties
assertThat(
ConnectionOptions.parseUriProperty(baseUri + "autocommit=true", "autocommit"),
is(nullValue()));
assertThat(ConnectionOptions.parseUriProperty(baseUri + "autocommit=true", "autocommit"))
.isNull();
// substring is not ok
assertThat(
ConnectionOptions.parseUriProperty(baseUri + "?isautocommit=true", "autocommit"),
is(nullValue()));
assertThat(ConnectionOptions.parseUriProperty(baseUri + "?isautocommit=true", "autocommit"))
.isNull();
}

@Test
public void testParseProperties() {
final String baseUri =
"cloudspanner:/projects/test-project-123/instances/test-instance/databases/test-database";
assertThat(
ConnectionOptions.parseProperties(baseUri + "?autocommit=true"),
is(equalTo(Arrays.asList("autocommit"))));
assertThat(
ConnectionOptions.parseProperties(baseUri + "?autocommit=true;readonly=false"),
is(equalTo(Arrays.asList("autocommit", "readonly"))));
assertThat(
ConnectionOptions.parseProperties(baseUri + "?autocommit=true;READONLY=false"),
is(equalTo(Arrays.asList("autocommit", "READONLY"))));
assertThat(
ConnectionOptions.parseProperties(baseUri + ";autocommit=true;readonly=false"),
is(equalTo(Arrays.asList("autocommit", "readonly"))));
assertThat(
ConnectionOptions.parseProperties(baseUri + ";autocommit=true;readonly=false;"),
is(equalTo(Arrays.asList("autocommit", "readonly"))));
assertThat(ConnectionOptions.parseProperties(baseUri + "?autocommit=true"))
.isEqualTo(Arrays.asList("autocommit"));
assertThat(ConnectionOptions.parseProperties(baseUri + "?autocommit=true;readonly=false"))
.isEqualTo(Arrays.asList("autocommit", "readonly"));
assertThat(ConnectionOptions.parseProperties(baseUri + "?autocommit=true;READONLY=false"))
.isEqualTo(Arrays.asList("autocommit", "READONLY"));
assertThat(ConnectionOptions.parseProperties(baseUri + ";autocommit=true;readonly=false"))
.isEqualTo(Arrays.asList("autocommit", "readonly"));
assertThat(ConnectionOptions.parseProperties(baseUri + ";autocommit=true;readonly=false;"))
.isEqualTo(Arrays.asList("autocommit", "readonly"));
}

@Test
public void testParsePropertiesSpecifiedMultipleTimes() {
final String baseUri =
"cloudspanner:/projects/test-project-123/instances/test-instance/databases/test-database";
assertThat(
ConnectionOptions.parseUriProperty(
baseUri + "?autocommit=true;autocommit=false", "autocommit"),
is(equalTo("true")));
ConnectionOptions.parseUriProperty(
baseUri + "?autocommit=true;autocommit=false", "autocommit"))
.isEqualTo("true");
assertThat(
ConnectionOptions.parseUriProperty(
baseUri + "?autocommit=false;autocommit=true", "autocommit"),
is(equalTo("false")));
ConnectionOptions.parseUriProperty(
baseUri + "?autocommit=false;autocommit=true", "autocommit"))
.isEqualTo("false");
assertThat(
ConnectionOptions.parseUriProperty(
baseUri + ";autocommit=false;readonly=false;autocommit=true", "autocommit"),
is(equalTo("false")));
ConnectionOptions.parseUriProperty(
baseUri + ";autocommit=false;readonly=false;autocommit=true", "autocommit"))
.isEqualTo("false");
ConnectionOptions.newBuilder()
.setUri(
"cloudspanner:/projects/test-project-123/instances/test-instance/databases/test-database"
Expand Down

0 comments on commit 5957008

Please sign in to comment.