feat: Add support for db roles list #1807

vi3k6i5 · 2022-04-06T07:17:08Z

feat: Add support for db roles list.

generated-files-bot · 2022-04-06T07:17:18Z

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
google-cloud-spanner/src/test/java/com/google/cloud/spanner/v1/SpannerClientTest.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
proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/Session.java
proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/SessionOrBuilder.java
proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/SpannerProto.java
proto-google-cloud-spanner-v1/src/main/proto/google/spanner/v1/spanner.proto

olavloite

This looks generally good to me, with a couple of small nits. The biggest issue here might be the documentation / explanation of what this is and how customers should use it. We are calling it something like defaultCreatorRole, but I don't think that makes it clear to customers what it does and why they should use it.

olavloite · 2022-04-19T06:21:23Z

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

+    final String databaseName = getDatabaseName(instanceId, databaseId);
+    final Options listOptions = Options.fromListOptions(options);
+    Preconditions.checkArgument(
+        !listOptions.hasFilter(), "Filter option is not supported by listDatabasesRoles");


I assume that this is a backend limitation (albeit one that I find a little strange...)

Yes, As per the discussions with the team they wont be providing a filter option, I will remove it from the code.

olavloite · 2022-04-19T06:27:40Z

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

+    if (this == o) {
+      return true;
+    }
+    if (o == null || getClass() != o.getClass()) {


nit: I would prefer equals over !=, even when we know that the default implementation of equals just uses ==

Suggested change

if (o == null || getClass() != o.getClass()) {

if (o == null || !getClass().equals(o.getClass())) {

olavloite · 2022-04-19T06:30:03Z

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

@@ -271,7 +274,10 @@ public String[] getValidValues() {
                      RPC_PRIORITY_NAME,
                      "Sets the priority for all RPC invocations from this connection (HIGH/MEDIUM/LOW). The default is HIGH."),
                  ConnectionProperty.createStringProperty(
-                      DIALECT_PROPERTY_NAME, "Sets the dialect to use for this connection."))));
+                      DIALECT_PROPERTY_NAME, "Sets the dialect to use for this connection."),


(Not related to this PR, but more a general note): We should remove this property as we don't need it and don't use it.

olavloite · 2022-04-19T06:31:40Z

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

+                      DIALECT_PROPERTY_NAME, "Sets the dialect to use for this connection."),
+                  ConnectionProperty.createStringProperty(
+                      CREATOR_ROLE_PROPERTY_NAME,
+                      "Sets the default creator role to use for this connection."))));


I don't think anyone is going to understand what 'the default creator role` is. We need a better description of what this property is and why anyone might want to use this.

olavloite · 2022-04-19T06:34:11Z

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

+    private final String name;
+
+    public Builder(String name) {
+      this.name = name;


We seem to assume that the name will always be non-null and not empty, so we should check for that here.

Suggested change

this.name = name;

this.name = Preconditions.checkNotNullOrEmpty(name);

olavloite · 2022-04-19T06:59:17Z

...le-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITDatabaseRolePermissionTest.java

+    try (ResultSet rs =
+        dbClient.singleUse().executeQuery(Statement.of("SELECT COUNT(*) as cnt FROM T"))) {
+      rs.next();
+      fail("Missing expected permission denied to dbRoleParent Exception");


use assertThrows(...) instead of fail(..).

olavloite · 2022-04-19T07:00:58Z

...le-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITDatabaseRolePermissionTest.java

+    try (ResultSet rs =
+        dbClient.singleUse().executeQuery(Statement.of("SELECT COUNT(*) as cnt FROM T"))) {
+      rs.next();
+      fail("Missing expected permission denied to dbRoleOrphan Exception");


Here also: use assertThrows(SpannerException.class, rs::next)

olavloite · 2022-04-19T07:01:59Z

...le-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITDatabaseRolePermissionTest.java

+                instanceId, databaseId, ImmutableList.of(createTableT, createRoleOrphan))
+            .get(5, TimeUnit.MINUTES);
+
+    // Connect to db with dbRoleParent.


nit:

Suggested change

// Connect to db with dbRoleParent.

// Connect to db with dbRoleOrphan.

olavloite · 2022-04-19T07:03:11Z

...le-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITDatabaseRolePermissionTest.java

+  }
+
+  @Test
+  public void orphanRolePermissions() throws Exception {


What exactly is this test case testing? That a role that has not received any specific permissions has no permissions? I think it would be good to add a comment to explain what it is that we are verifying here.

devbww · 2022-06-11T21:11:19Z

...min-database-v1/src/main/proto/google/spanner/admin/database/v1/spanner_database_admin.proto

+
+  // Required. The database whose roles should be listed.
+  // Values are of the form
+  // `projects/<project>/instances/<instance>/databases/<database>/databaseRoles`.


Should there really be a "/databaseRoles" suffix there?

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

Rebase to #1916

vi3k6i5 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 6, 2022

vi3k6i5 requested review from a team as code owners April 6, 2022 07:17

product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Apr 6, 2022

vi3k6i5 added 4 commits April 6, 2022 12:50

feature fgac

d275d6a

feat: Add support for FGAC and getDatabaseRoles

5f7a3da

test: add fgac tests

d0400c7

refactor: lint files

bd0d0da

vi3k6i5 force-pushed the ft-fgac branch from 5513a86 to bd0d0da Compare April 6, 2022 07:39

vi3k6i5 marked this pull request as draft April 6, 2022 07:42

vi3k6i5 added 5 commits April 7, 2022 17:10

refactor: linting fixes

19546ea

feat: fgac related proto updates

d047d82

test: add integration test for fgac

60d1f3d

refactor: use db_name instead of instance name for list database roles

4495685

refactor: lint fixes

273fd5a

product-auto-label bot added the size: l Pull request size is large. label Apr 19, 2022

olavloite reviewed Apr 19, 2022

View reviewed changes

ansh0l assigned rajatbhatta and asthamohta Apr 25, 2022

devbww reviewed Jun 11, 2022

View reviewed changes

rahul2393 mentioned this pull request Jun 13, 2022

feat: add support for db roles list #1916

Merged

rahul2393 closed this Jun 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for db roles list #1807

feat: Add support for db roles list #1807

vi3k6i5 commented Apr 6, 2022

generated-files-bot bot commented Apr 6, 2022

olavloite left a comment

olavloite Apr 19, 2022

vi3k6i5 Apr 26, 2022

olavloite Apr 19, 2022

olavloite Apr 19, 2022

olavloite Apr 19, 2022

olavloite Apr 19, 2022

olavloite Apr 19, 2022

olavloite Apr 19, 2022

olavloite Apr 19, 2022

olavloite Apr 19, 2022

devbww Jun 11, 2022

rahul2393 commented Jun 13, 2022

	if (o == null \|\| getClass() != o.getClass()) {
	if (o == null \|\| !getClass().equals(o.getClass())) {

	this.name = name;
	this.name = Preconditions.checkNotNullOrEmpty(name);

	// Connect to db with dbRoleParent.
	// Connect to db with dbRoleOrphan.

feat: Add support for db roles list #1807

feat: Add support for db roles list #1807

Conversation

vi3k6i5 commented Apr 6, 2022

generated-files-bot bot commented Apr 6, 2022

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

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

rahul2393 commented Jun 13, 2022