chore: add getter for database role in DatabaseClient and BatchClient by rahul2393 · Pull Request #2029 · googleapis/java-spanner

rahul2393 · 2022-09-22T07:07:43Z

No description provided.

… for beam unit tests

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

rajatbhatta · 2022-09-22T07:24:17Z

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

+   * 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.
+   */
+  String getDatabaseRole();


Suggested change

String getDatabaseRole();

default String getDatabaseRole() {

throw new UnsupportedOperationException("method should be overwritten");

}

I think we should provide a default implementation for this method in the interface, so that it isn't a breaking change.

rajatbhatta · 2022-09-22T09:26:01Z

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


  private final SessionPoolOptions options;
  private final SettableFuture<Dialect> dialect = SettableFuture.create();
+


nit: remove extra newline.

rajatbhatta · 2022-09-22T09:26:51Z

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

      Clock clock) {
    return createPool(
        poolOptions,
+        "",


Should we pass in null here?

Suggested change

"",

null,

rajatbhatta · 2022-09-22T09:32:09Z

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

    }
  }

+  String getDatabaseRole() {


Suggested change

String getDatabaseRole() {

public String getDatabaseRole() {

rajatbhatta · 2022-09-22T09:32:26Z

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

      Clock clock, MetricRegistry metricRegistry, List<LabelValue> labelValues) {
    return SessionPool.createPool(
        options,
+        "",


Suggested change

"",

null,

rajatbhatta · 2022-09-22T09:34:11Z

...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"))) {
      assertTrue(rs.next());
+      assertThat(dbClient.getDatabaseRole()).isEqualTo(dbRoleParent);


Suggested change

assertThat(dbClient.getDatabaseRole()).isEqualTo(dbRoleParent);

assertEquals(dbClient.getDatabaseRole(), dbRoleParent);

nit: change to assertEquals.

rajatbhatta · 2022-09-22T11:39:14Z

@rahul2393 : Can we do the following as well?

Add a unit test to https://github.com/googleapis/java-spanner/blob/main/google-cloud-spanner/src/test/java/com/google/cloud/spanner/BatchClientImplTest.java to see if client.getDatabaseRole() returns the correct database role?
Can we add a similar unit test each to https://github.com/googleapis/java-spanner/blob/main/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java and https://github.com/googleapis/java-spanner/blob/main/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java as well?

rajatbhatta · 2022-09-22T12:56:43Z

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

-import static com.google.cloud.spanner.MetricRegistryConstants.NUM_WRITE_SESSIONS;
-import static com.google.cloud.spanner.MetricRegistryConstants.SPANNER_LABEL_KEYS;
-import static com.google.cloud.spanner.MetricRegistryConstants.SPANNER_LABEL_KEYS_WITH_TYPE;
+import static com.google.cloud.spanner.MetricRegistryConstants.*;


Suggested change

import static com.google.cloud.spanner.MetricRegistryConstants.*;

import static com.google.cloud.spanner.MetricRegistryConstants.NUM_IN_USE_SESSIONS;

import static com.google.cloud.spanner.MetricRegistryConstants.NUM_READ_SESSIONS;

import static com.google.cloud.spanner.MetricRegistryConstants.NUM_SESSIONS_BEING_PREPARED;

import static com.google.cloud.spanner.MetricRegistryConstants.NUM_WRITE_SESSIONS;

import static com.google.cloud.spanner.MetricRegistryConstants.SPANNER_LABEL_KEYS;

import static com.google.cloud.spanner.MetricRegistryConstants.SPANNER_LABEL_KEYS_WITH_TYPE;

Google style requires us to never use wildcard imports: https://google.github.io/styleguide/javaguide.html

rajatbhatta · 2022-09-22T12:58:04Z

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

  }

+  @Test
+  public void testDatabaseRole() throws Exception {


Suggested change

public void testDatabaseRole() throws Exception {

public void testGetDatabaseRole() throws Exception {

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

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

…tchClientImplTest.java

rajatbhatta

LGTM.
@olavloite: Can you please take a look at this PR once? Thanks!

olavloite

LGTM (with a tiny nit on the Javadoc)

olavloite · 2022-09-28T05:52:32Z

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

  BatchReadOnlyTransaction batchReadOnlyTransaction(BatchTransactionId batchTransactionId);
+
+  /**
+   * Returns the {@link DatabaseRole} used by the client connection. The database role that is used


nit: replace the {@link DatabaseRole} with just database role. DatabaseRole is not a valid class name, so it cannot be linked.

olavloite · 2022-09-28T05:52:40Z

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

  }

+  /**
+   * Returns the {@link DatabaseRole} used by the client connection. The database role that is used


Same as above

chore: add getter for database role in DatabaseClient and BatchClient…

8cb330c

… for beam unit tests

rahul2393 requested a review from a team September 22, 2022 07:07

product-auto-label bot added size: s Pull request size is small. api: spanner Issues related to the googleapis/java-spanner API. labels Sep 22, 2022

fix clirr

0c6ef43

rahul2393 requested a review from a team September 22, 2022 07:19

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

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

🦉 Updates from OwlBot post-processor

8e10121

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

rajatbhatta suggested changes Sep 22, 2022

View reviewed changes

rajatbhatta requested a review from olavloite September 22, 2022 09:38

incorporate requested changes

f3f5e63

product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Sep 22, 2022

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

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

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

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

incorporate requested changes

c3c9e81

rahul2393 force-pushed the add-database-role-getter branch from cc008d9 to c3c9e81 Compare September 22, 2022 12:49

rajatbhatta reviewed Sep 22, 2022

View reviewed changes

incorporate requested changes

ee9b14b

rahul2393 force-pushed the add-database-role-getter branch from 26e1438 to ee9b14b Compare September 22, 2022 17:11

rajatbhatta added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 22, 2022

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

🦉 Updates from OwlBot post-processor

3d61677

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

rajatbhatta reviewed Sep 22, 2022

View reviewed changes

google-cloud-spanner/src/test/java/com/google/cloud/spanner/BatchClientImplTest.java Outdated Show resolved Hide resolved

Update google-cloud-spanner/src/test/java/com/google/cloud/spanner/Ba…

05d16b8

…tchClientImplTest.java

Update BatchClientImplTest.java

62bc1f3

rajatbhatta added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 22, 2022

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

rajatbhatta self-requested a review September 22, 2022 20:53

rajatbhatta approved these changes Sep 22, 2022

View reviewed changes

rajatbhatta added 2 commits September 23, 2022 02:32

Add doc for getDatabaseRole in DatabaseClient

a4313d6

Update DatabaseClient.java

226451c

rajatbhatta added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 23, 2022

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

olavloite approved these changes Sep 28, 2022

View reviewed changes

ansh0l approved these changes Sep 29, 2022

View reviewed changes

ansh0l merged commit c2363e3 into googleapis:main Sep 29, 2022

-  String getDatabaseRole();
+  default String getDatabaseRole() {
+    throw new UnsupportedOperationException("method should be overwritten");
+  }


		private final SessionPoolOptions options;
		private final SettableFuture<Dialect> dialect = SettableFuture.create();

	assertThat(dbClient.getDatabaseRole()).isEqualTo(dbRoleParent);
	assertEquals(dbClient.getDatabaseRole(), dbRoleParent);

	public void testDatabaseRole() throws Exception {
	public void testGetDatabaseRole() throws Exception {

Conversation

rahul2393 commented Sep 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatbhatta commented Sep 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rajatbhatta left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olavloite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rajatbhatta left a comment •

edited

Loading