Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(bigquery): support IAM conditions in datasets in Java client. #3602

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat(bigquery): support IAM conditions in datasets in Java client. #3602

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
826654f
feat(bigquery): support IAM conditions in datasets in Java client.
whuffman36 Dec 9, 2024
a79baf3
Fix formatting
whuffman36 Dec 9, 2024
7610a8f
Account for possible null condition field in Acl.
whuffman36 Dec 10, 2024
001ed1c
Add toString() method to Acl.Expr object.
whuffman36 Dec 13, 2024
15fc548
Change Acl.User to be default google credentials in IT test
whuffman36 Dec 13, 2024
7babc4b
fix formatting
whuffman36 Dec 13, 2024
a17b6d9
Add Acl.Expr builder. Fix review nits.
whuffman36 Dec 18, 2024
fc6f203
fix formatting
whuffman36 Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 88 additions & 3 deletions google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/Acl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.api.core.ApiFunction;
import com.google.api.services.bigquery.model.Dataset.Access;
import com.google.api.services.bigquery.model.DatasetAccessEntry;
import com.google.api.services.bigquery.model.Expr;
import com.google.cloud.StringEnumType;
import com.google.cloud.StringEnumValue;
import java.io.Serializable;
Expand All @@ -41,6 +42,7 @@ public final class Acl implements Serializable {

private final Entity entity;
private final Role role;
private final Expr condition;

/**
* Dataset roles supported by BigQuery.
Expand Down Expand Up @@ -568,9 +570,77 @@ Access toPb() {
}
}

/** Expr represents the conditional information related to dataset access policies. */
public static final class Expr implements Serializable {
PhongChuong marked this conversation as resolved.
Show resolved Hide resolved
// Textual representation of an expression in Common Expression Language syntax.
private final String expression;
/**
* Optional. Title for the expression, i.e. a short string describing its purpose. This can be
* used e.g. in UIs which allow to enter the expression.
*/
private final String title;
/**
* Optional. Description of the expression. This is a longer text which describes the
* expression, e.g. when hovered over it in a UI.
*/
private final String description;
/**
* Optional. String indicating the location of the expression for error reporting, e.g. a file
* name and a position in the file.
*/
private final String location;

public Expr(String expression, String title, String description, String location) {
PhongChuong marked this conversation as resolved.
Show resolved Hide resolved
this.expression = expression;
this.title = title;
this.description = description;
this.location = location;
}

com.google.api.services.bigquery.model.Expr toPb() {
com.google.api.services.bigquery.model.Expr bqExpr =
new com.google.api.services.bigquery.model.Expr();
bqExpr.setExpression(this.expression);
bqExpr.setTitle(this.title);
bqExpr.setDescription(this.description);
bqExpr.setLocation(this.location);
return bqExpr;
}

static Expr fromPb(com.google.api.services.bigquery.model.Expr bqExpr) {
return new Expr(
bqExpr.getExpression(), bqExpr.getTitle(), bqExpr.getDescription(), bqExpr.getLocation());
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include toString() as well as the normal methods like equals and hashcode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, updated to have toString() method.

public int hashCode() {
return Objects.hash(expression, title, description, location);
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (obj == null || getClass() != obj.getClass()) {
return false;
}
final Expr other = (Expr) obj;
return Objects.equals(this.expression, other.expression)
&& Objects.equals(this.title, other.title)
&& Objects.equals(this.description, other.description)
&& Objects.equals(this.location, other.location);
}
}

private Acl(Entity entity, Role role) {
this(entity, role, null);
}

private Acl(Entity entity, Role role, Expr condition) {
this.entity = checkNotNull(entity);
this.role = role;
this.condition = condition;
}

/** @return Returns the entity for this ACL. */
Expand All @@ -582,6 +652,10 @@ public Entity getEntity() {
public Role getRole() {
return role;
}
/** @return Returns the condition specified by this ACL. */
public Expr getCondition() {
return condition;
}

/**
* @return Returns an Acl object.
Expand All @@ -592,6 +666,10 @@ public static Acl of(Entity entity, Role role) {
return new Acl(entity, role);
}

public static Acl of(Entity entity, Role role, Expr condition) {
return new Acl(entity, role, condition);
}

/**
* @param datasetAclEntity
* @return Returns an Acl object for a datasetAclEntity.
Expand All @@ -618,7 +696,7 @@ public static Acl of(Routine routine) {

@Override
public int hashCode() {
return Objects.hash(entity, role);
return Objects.hash(entity, role, condition);
}

@Override
Expand All @@ -635,19 +713,26 @@ public boolean equals(Object obj) {
return false;
}
final Acl other = (Acl) obj;
return Objects.equals(this.entity, other.entity) && Objects.equals(this.role, other.role);
return Objects.equals(this.entity, other.entity)
&& Objects.equals(this.role, other.role)
&& Objects.equals(this.condition, other.condition);
}

Access toPb() {
Access accessPb = entity.toPb();
if (role != null) {
accessPb.setRole(role.name());
}
if (condition != null) {
accessPb.setCondition(condition.toPb());
}
return accessPb;
}

static Acl fromPb(Access access) {
return Acl.of(
Entity.fromPb(access), access.getRole() != null ? Role.valueOf(access.getRole()) : null);
Entity.fromPb(access),
access.getRole() != null ? Role.valueOf(access.getRole()) : null,
access.getCondition() != null ? Expr.fromPb(access.getCondition()) : null);
}
}
4 changes: 4 additions & 0 deletions google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/BigQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,10 @@ public static DatasetOption fields(DatasetField... fields) {
return new DatasetOption(
BigQueryRpc.Option.FIELDS, Helper.selector(DatasetField.REQUIRED_FIELDS, fields));
}

public static DatasetOption accessPolicyVersion(Integer accessPolicyVersion) {
PhongChuong marked this conversation as resolved.
Show resolved Hide resolved
return new DatasetOption(BigQueryRpc.Option.ACCESS_POLICY_VERSION, accessPolicyVersion);
}
}

/** Class for specifying dataset delete options. */
Expand Down
3 changes: 2 additions & 1 deletion google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/spi/v2/BigQueryRpc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ enum Option {
REQUESTED_POLICY_VERSION("requestedPolicyVersion"),
TABLE_METADATA_VIEW("view"),
RETRY_OPTIONS("retryOptions"),
BIGQUERY_RETRY_CONFIG("bigQueryRetryConfig");
BIGQUERY_RETRY_CONFIG("bigQueryRetryConfig"),
ACCESS_POLICY_VERSION("accessPolicyVersion");

private final String value;

Expand Down
45 changes: 45 additions & 0 deletions google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/spi/v2/HttpBigQueryRpc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,21 @@ private void validateRPC() throws BigQueryException, IOException {
public Dataset getDataset(String projectId, String datasetId, Map<Option, ?> options) {
try {
validateRPC();
Integer accessPolicyVersion = null;
PhongChuong marked this conversation as resolved.
Show resolved Hide resolved
for (Map.Entry<Option, ?> entry : options.entrySet()) {
if (entry.getKey() == Option.ACCESS_POLICY_VERSION && entry.getValue() != null) {
accessPolicyVersion = (Integer) entry.getValue();
}
}
if (accessPolicyVersion != null) {
return bigquery
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I worry here is if there's a divergence between this chain and the default execute(). Is it worth doing a single chain of builders with a conditional? Same with this pattern in the other RPCs (create, patch, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that approach seems cleaner. I updated the conditional to toggle only the access policy version instead of creating an entirely new RPC.

.datasets()
.get(projectId, datasetId)
.setFields(Option.FIELDS.getString(options))
.setPrettyPrint(false)
.setAccessPolicyVersion(accessPolicyVersion)
.execute();
}
return bigquery
.datasets()
.get(projectId, datasetId)
Expand Down Expand Up @@ -174,6 +189,21 @@ public Tuple<String, Iterable<Dataset>> listDatasets(String projectId, Map<Optio
public Dataset create(Dataset dataset, Map<Option, ?> options) {
try {
validateRPC();
Integer accessPolicyVersion = null;
whuffman36 marked this conversation as resolved.
Show resolved Hide resolved
for (Map.Entry<Option, ?> entry : options.entrySet()) {
if (entry.getKey() == Option.ACCESS_POLICY_VERSION && entry.getValue() != null) {
accessPolicyVersion = (Integer) entry.getValue();
}
}
if (accessPolicyVersion != null) {
return bigquery
.datasets()
.insert(dataset.getDatasetReference().getProjectId(), dataset)
.setPrettyPrint(false)
.setFields(Option.FIELDS.getString(options))
.setAccessPolicyVersion(accessPolicyVersion)
.execute();
}
return bigquery
.datasets()
.insert(dataset.getDatasetReference().getProjectId(), dataset)
Expand Down Expand Up @@ -276,7 +306,22 @@ public boolean deleteDataset(String projectId, String datasetId, Map<Option, ?>
public Dataset patch(Dataset dataset, Map<Option, ?> options) {
try {
validateRPC();
Integer accessPolicyVersion = null;
whuffman36 marked this conversation as resolved.
Show resolved Hide resolved
for (Map.Entry<Option, ?> entry : options.entrySet()) {
if (entry.getKey() == Option.ACCESS_POLICY_VERSION && entry.getValue() != null) {
accessPolicyVersion = (Integer) entry.getValue();
}
}
DatasetReference reference = dataset.getDatasetReference();
if (accessPolicyVersion != null) {
return bigquery
.datasets()
.patch(reference.getProjectId(), reference.getDatasetId(), dataset)
.setPrettyPrint(false)
.setFields(Option.FIELDS.getString(options))
.setAccessPolicyVersion(accessPolicyVersion)
.execute();
}
return bigquery
.datasets()
.patch(reference.getProjectId(), reference.getDatasetId(), dataset)
Expand Down
10 changes: 10 additions & 0 deletions google-cloud-bigquery/src/test/java/com/google/cloud/bigquery/AclTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.cloud.bigquery.Acl.Domain;
import com.google.cloud.bigquery.Acl.Entity;
import com.google.cloud.bigquery.Acl.Entity.Type;
import com.google.cloud.bigquery.Acl.Expr;
import com.google.cloud.bigquery.Acl.Group;
import com.google.cloud.bigquery.Acl.IamMember;
import com.google.cloud.bigquery.Acl.Role;
Expand Down Expand Up @@ -136,4 +137,13 @@ public void testOf() {
assertEquals(routine, acl.getEntity());
assertEquals(null, acl.getRole());
}

@Test
public void testOfWithCondition() {
Expr expr = new Expr("expression", "title", "description", "location");
Acl acl = Acl.of(Group.ofAllAuthenticatedUsers(), Role.READER, expr);
Dataset.Access pb = acl.toPb();
assertEquals(acl, Acl.fromPb(pb));
assertEquals(acl.getCondition(), expr);
}
}
15 changes: 15 additions & 0 deletions google-cloud-bigquery/src/test/java/com/google/cloud/bigquery/BigQueryImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.cloud.RetryOption;
import com.google.cloud.ServiceOptions;
import com.google.cloud.Tuple;
import com.google.cloud.bigquery.BigQuery.DatasetOption;
import com.google.cloud.bigquery.BigQuery.JobOption;
import com.google.cloud.bigquery.BigQuery.QueryResultsOption;
import com.google.cloud.bigquery.InsertAllRequest.RowToInsert;
Expand Down Expand Up @@ -572,6 +573,20 @@ public void testCreateDatasetWithSelectedFields() {
verify(bigqueryRpcMock).create(eq(DATASET_INFO_WITH_PROJECT.toPb()), capturedOptions.capture());
}

@Test
public void testCreateDatasetWithAccessPolicy() {
DatasetInfo datasetInfo = DATASET_INFO.setProjectId(OTHER_PROJECT);
DatasetOption datasetOption = DatasetOption.accessPolicyVersion(3);
when(bigqueryRpcMock.create(datasetInfo.toPb(), optionMap(datasetOption)))
.thenReturn(datasetInfo.toPb());
BigQueryOptions bigQueryOptions =
createBigQueryOptionsForProject(OTHER_PROJECT, rpcFactoryMock);
bigquery = bigQueryOptions.getService();
Dataset dataset = bigquery.create(datasetInfo, datasetOption);
assertEquals(new Dataset(bigquery, new DatasetInfo.BuilderImpl(datasetInfo)), dataset);
verify(bigqueryRpcMock).create(datasetInfo.toPb(), optionMap(datasetOption));
}

@Test
public void testGetDataset() {
when(bigqueryRpcMock.getDataset(PROJECT, DATASET, EMPTY_RPC_OPTIONS))
Expand Down
Loading
Loading