feat: Add support for db roles list#1807
Conversation
vi3k6i5
added
the
do not merge
product-auto-label
Bot
added
the
api: spanner
|
Warning: This pull request is touching the following templated files:
|
olavloite
left a comment
olavloite
left a comment
There was a problem hiding this comment.
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.
| final String databaseName = getDatabaseName(instanceId, databaseId); | ||
| final Options listOptions = Options.fromListOptions(options); | ||
| Preconditions.checkArgument( | ||
| !listOptions.hasFilter(), "Filter option is not supported by listDatabasesRoles"); |
There was a problem hiding this comment.
I assume that this is a backend limitation (albeit one that I find a little strange...)
There was a problem hiding this comment.
Yes, As per the discussions with the team they wont be providing a filter option, I will remove it from the code.
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (o == null || getClass() != o.getClass()) { |
There was a problem hiding this comment.
nit: I would prefer equals over !=, even when we know that the default implementation of equals just uses ==
| if (o == null || getClass() != o.getClass()) { | |
| if (o == null || !getClass().equals(o.getClass())) { |
| "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."), |
There was a problem hiding this comment.
(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.
| 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.")))); |
There was a problem hiding this comment.
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.
| private final String name; | ||
|
|
||
| public Builder(String name) { | ||
| this.name = name; |
There was a problem hiding this comment.
We seem to assume that the name will always be non-null and not empty, so we should check for that here.
| this.name = name; | |
| this.name = Preconditions.checkNotNullOrEmpty(name); |
| try (ResultSet rs = | ||
| dbClient.singleUse().executeQuery(Statement.of("SELECT COUNT(*) as cnt FROM T"))) { | ||
| rs.next(); | ||
| fail("Missing expected permission denied to dbRoleParent Exception"); |
There was a problem hiding this comment.
use assertThrows(...) instead of fail(..).
| try (ResultSet rs = | ||
| dbClient.singleUse().executeQuery(Statement.of("SELECT COUNT(*) as cnt FROM T"))) { | ||
| rs.next(); | ||
| fail("Missing expected permission denied to dbRoleOrphan Exception"); |
There was a problem hiding this comment.
Here also: use assertThrows(SpannerException.class, rs::next)
| instanceId, databaseId, ImmutableList.of(createTableT, createRoleOrphan)) | ||
| .get(5, TimeUnit.MINUTES); | ||
|
|
||
| // Connect to db with dbRoleParent. |
There was a problem hiding this comment.
nit:
| // Connect to db with dbRoleParent. | |
| // Connect to db with dbRoleOrphan. |
| } | ||
|
|
||
| @Test | ||
| public void orphanRolePermissions() throws Exception { |
There was a problem hiding this comment.
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.
|
|
||
| // Required. The database whose roles should be listed. | ||
| // Values are of the form | ||
| // `projects/<project>/instances/<instance>/databases/<database>/databaseRoles`. |
There was a problem hiding this comment.
Should there really be a "/databaseRoles" suffix there?
|
Rebase to #1916 |
6 participants
feat: Add support for db roles list.