feat: add support for db roles list #1916

rahul2393 · 2022-06-13T05:12:15Z

Added support of passing databaseRole in cloud-spanner sessions.

Old PR: #1807

generated-files-bot · 2022-06-13T05:12:23Z

Warning: This pull request is touching the following templated files:

google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/DatabaseAdminClient.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/DatabaseAdminSettings.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/gapic_metadata.json
google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/stub/DatabaseAdminStub.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/stub/DatabaseAdminStubSettings.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/stub/GrpcDatabaseAdminStub.java
google-cloud-spanner/src/test/java/com/google/cloud/spanner/admin/database/v1/DatabaseAdminClientTest.java
google-cloud-spanner/src/test/java/com/google/cloud/spanner/admin/database/v1/MockDatabaseAdminImpl.java
grpc-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/DatabaseAdminGrpc.java
proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/SpannerDatabaseAdminProto.java
proto-google-cloud-spanner-admin-database-v1/src/main/proto/google/spanner/admin/database/v1/spanner_database_admin.proto

olavloite · 2022-07-05T06:02:32Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseRole.java

+import com.google.common.base.Preconditions;
+import java.util.Objects;
+
+/** Represents a restore operation of a Cloud Spanner backup. */


nit: copy-paste

olavloite · 2022-07-05T06:08:22Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java

@@ -830,6 +834,12 @@ public Builder setSessionPoolOption(SessionPoolOptions sessionPoolOptions) {
      return this;
    }

+    /** Sets the database role that should be used by this instance. */


I think this needs a more elaborate Javadoc on what this means, as most users won't understand this automatically. I would suggest something like:

Suggested change

/** Sets the database role that should be used by this instance. */

/**

* Sets the database role that should be used for connections that are created by this instance.

* The database role that is used determines the access permissions that a connection has. This

* can for example be used to create connections that are only permitted to access certain tables.

*/

olavloite · 2022-07-05T06:10:40Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionOptions.java

@@ -213,8 +214,8 @@ public String[] getValidValues() {
  public static final String LENIENT_PROPERTY_NAME = "lenient";
  /** Name of the 'rpcPriority' connection property. */
  public static final String RPC_PRIORITY_NAME = "rpcPriority";
-  /** Dialect to use for a connection. */
-  private static final String DIALECT_PROPERTY_NAME = "dialect";


I agree that we should remove this, as it is no longer needed, BUT:

We should consider doing so in a separate PR. It is not related to this change.

We should do it in a non-breaking way. Anyone who currently has dialect=... in their connection string will get an error after this change. We should therefore silently ignore it if we encounter dialect=... in a connection string.

Reverted the change to remove Dialect, will be done in another PR

olavloite · 2022-07-05T06:13:09Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionOptions.java

@@ -959,6 +969,11 @@ public TransportChannelProvider getChannelProvider() {
    }
  }

+  /** The creator role to use for the connection. */


I don't think users will understand what 'creator role' means. This seems like an internal name (I (think I) understand that it means the role that is used to create a session, but that is not clear to a user.)

I would suggest: The database role that is used for this connection. Assigning a role to a connection can be used to for example restrict the access of a connection to a specific set of tables.

olavloite · 2022-07-05T06:16:20Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/testing/RemoteSpannerHelper.java

@@ -50,6 +50,8 @@ public class RemoteSpannerHelper {
  private final InstanceId instanceId;
  private static final AtomicInteger dbSeq = new AtomicInteger();
  private static final int dbPrefix = new Random().nextInt(Integer.MAX_VALUE);
+  private static int dbRoleSeq;


Although the probability is low that it goes wrong, it would be better to use an AtomicInteger for this, as our integration tests can run in parallel.

olavloite · 2022-07-05T06:18:04Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionClientTests.java

    when(spannerOptions.getSessionLabels()).thenReturn(labels);
+      when(spannerOptions.getDatabaseRole()).thenReturn(databaseRole);


nit: indentation seems off here

olavloite · 2022-07-05T06:19:03Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionClientTests.java

@@ -143,13 +145,16 @@ public void batchCreateAndCloseSessions() {
    final String sessionName = dbName + "/sessions/s%d";
    final Map<String, String> labels = new HashMap<>();
    labels.put("env", "dev");
+      String databaseRole = new String("role");


nit: indentation seems to be off here (and also elsewhere in this class). Maybe run code formatter again?

olavloite · 2022-07-05T06:19:55Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java

@@ -76,7 +76,7 @@ public void defaultBuilder() {
    // id.
    SpannerOptions options = SpannerOptions.newBuilder().setProjectId("test-project").build();
    if (Strings.isNullOrEmpty(System.getenv("SPANNER_EMULATOR_HOST"))) {
-      assertThat(options.getHost()).isEqualTo("https://spanner.googleapis.com");
+      assertThat(options.getHost()).isEqualTo("https://staging-wrenchworks.sandbox.googleapis.com/");


nit: Remove before merging

snippet-bot · 2022-07-19T12:04:20Z

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

olavloite

Looks generally good to me, except we need some Javadoc for the version argument, as it is not self-explanatory.

olavloite · 2022-07-20T05:08:36Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/Database.java

@@ -145,7 +145,7 @@ public Page<Operation> listDatabaseOperations() {

  /** Returns the IAM {@link Policy} for this database. */
  public Policy getIAMPolicy() {
-    return dbClient.getDatabaseIAMPolicy(instance(), database());
+    return dbClient.getDatabaseIAMPolicy(instance(), database(), 1);


What is the 1 here?

EDIT: Apparently this is a version number. Should we add the version as an argument to the getIAMPolicy method?

olavloite · 2022-07-20T05:09:48Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClient.java

@@ -492,7 +495,7 @@ OperationFuture<Void, UpdateDatabaseDdlMetadata> updateDatabaseDdl(
  Operation getOperation(String name);

  /** Returns the IAM policy for the given database. */
-  Policy getDatabaseIAMPolicy(String instanceId, String databaseId);
+  Policy getDatabaseIAMPolicy(String instanceId, String databaseId, int version);


The version argument needs some Javadoc. What does it do and when/why should I use it?

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

rahul2393 requested review from a team as code owners June 13, 2022 05:12

product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/java-spanner API. labels Jun 13, 2022

rahul2393 mentioned this pull request Jun 13, 2022

feat: Add support for db roles list #1807

Closed

ansh0l requested a review from olavloite July 4, 2022 18:02

olavloite reviewed Jul 5, 2022

View reviewed changes

rahul2393 force-pushed the fgac-change branch from ce34d5b to 52e0559 Compare July 7, 2022 06:36

rahul2393 force-pushed the fgac-change branch from c5ca7c2 to b49d58d Compare July 19, 2022 12:05

olavloite approved these changes Jul 20, 2022

View reviewed changes

feat: add support for db roles

7483a57

rahul2393 force-pushed the fgac-change branch from 62e124e to 7483a57 Compare August 11, 2022 17:52

fix sample indentations

71dbda6

rahul2393 force-pushed the fgac-change branch from 300fb3f to 71dbda6 Compare August 11, 2022 17:59

rahul2393 added 4 commits August 12, 2022 10:45

fix tests

6b39ff8

Fix tests

0c74190

Delete samples

4f0650a

skip unsupported emulator tests

c775cfa

rahul2393 added the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 18, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 18, 2022

🦉 Updates from OwlBot post-processor

7288aaa

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

rahul2393 merged commit 8034c67 into googleapis:main Aug 22, 2022

release-please bot mentioned this pull request Aug 29, 2022

chore(main): release 6.29.0 #1985

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for db roles list #1916

feat: add support for db roles list #1916

rahul2393 commented Jun 13, 2022

generated-files-bot bot commented Jun 13, 2022 •

edited

olavloite Jul 5, 2022

olavloite Jul 5, 2022

olavloite Jul 5, 2022

rahul2393 Jul 7, 2022

olavloite Jul 5, 2022

olavloite Jul 5, 2022

olavloite Jul 5, 2022

olavloite Jul 5, 2022

olavloite Jul 5, 2022

snippet-bot bot commented Jul 19, 2022 •

edited

olavloite left a comment

olavloite Jul 20, 2022

olavloite Jul 20, 2022

-    /** Sets the database role that should be used by this instance. */
+    /**
+    * Sets the database role that should be used for connections that are created by this instance.
+    * The database role that is used determines the access permissions that a connection has. This
+    * can for example be used to create connections that are only permitted to access certain tables.
+    */

		when(spannerOptions.getSessionLabels()).thenReturn(labels);
		when(spannerOptions.getDatabaseRole()).thenReturn(databaseRole);

feat: add support for db roles list #1916

feat: add support for db roles list #1916

Conversation

rahul2393 commented Jun 13, 2022

generated-files-bot bot commented Jun 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snippet-bot bot commented Jul 19, 2022 • edited

No region tags are edited in this PR.

olavloite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

generated-files-bot bot commented Jun 13, 2022 •

edited

snippet-bot bot commented Jul 19, 2022 •

edited