yuqi1129 commented on code in PR #13693:
URL: https://github.com/apache/gravitino/pull/13693#discussion_r4218929214
##########
common/src/main/java/org/apache/gravitino/dto/requests/PolicyCreateRequest.java:
##########
@@ -64,7 +64,13 @@ public class PolicyCreateRequest implements RESTRequest {
name = "system_iceberg_compaction"),
@JsonSubTypes.Type(
value = PolicyContentDTO.IcebergOrphanFileRemovalContentDTO.class,
- name = "system_iceberg_orphan_file_removal")
+ name = "system_iceberg_orphan_file_removal"),
+ @JsonSubTypes.Type(
+ value = PolicyContentDTO.RowFilterContentDTO.class,
+ name = "system_row_filter"),
Review Comment:
**[Important]** The two new wire types are registered here (and in
`PolicyDTO` / `PolicyUpdateRequest`), but `docs/open-api/policies.yaml` still
only knows `custom`, `system_iceberg_compaction` and
`system_iceberg_orphan_file_removal`: the `policyType` enums, the
`discriminator.mapping` blocks (~L530, ~L613, ~L740) and the `updateContent`
enum (~L705). Generated clients or validating gateways would reject
`system_row_filter` / `system_column_mask`.
Please add `RowFilterPolicy` / `ColumnMaskPolicy` plus the
create/update-content schemas (single `expression` string; the 16 KiB UTF-8
limit can go in the description) and run `./gradlew :docs:build`.
##########
api/src/test/java/org/apache/gravitino/policy/TestReadRestrictionContent.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.policy;
+
+import com.google.common.collect.ImmutableSet;
+import java.util.Map;
+import org.apache.gravitino.MetadataObject;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestReadRestrictionContent {
+
+ @Test
+ void testCreatesReadRestrictionContent() {
+ String rowFilterExpression =
+ "filter := col(\"region\") == \"US\" if is_group_member(\"analysts\")
else := false";
+ String columnMaskExpression =
+ "mask := action(\"show-last-4\") if is_group_member(\"analysts\") "
+ + "else := action(\"replace-with-null\")";
+
+ RowFilterContent rowFilter = PolicyContents.rowFilter(rowFilterExpression);
+ ColumnMaskContent columnMask =
PolicyContents.columnMask(columnMaskExpression);
+
+ Assertions.assertEquals(rowFilterExpression, rowFilter.expression());
+ Assertions.assertEquals(columnMaskExpression, columnMask.expression());
+ Assertions.assertEquals(Map.of("expression", rowFilterExpression),
rowFilter.rules());
+ Assertions.assertEquals(Map.of("expression", columnMaskExpression),
columnMask.rules());
+ Assertions.assertEquals(
+ ImmutableSet.of(MetadataObject.Type.TAG),
rowFilter.supportedObjectTypes());
Review Comment:
This assertion pins `{TAG}` as the expected value; it should follow whatever
is decided in the `supportedObjectTypes` comment on `ReadRestrictionContent`.
##########
api/src/main/java/org/apache/gravitino/policy/ReadRestrictionContent.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.policy;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.MetadataObject;
+
+/** Shared behavior for built-in policies that restrict data reads. */
+public abstract class ReadRestrictionContent implements PolicyContent {
+
+ /** Maximum UTF-8 size of an authored expression. */
+ public static final int MAX_SOURCE_LENGTH_BYTES = 16 * 1024;
+
+ private static final Set<MetadataObject.Type> SUPPORTED_OBJECT_TYPES =
Review Comment:
**[Important]** `supportedObjectTypes()` returns `{TAG}`, which conflicts
with the `PolicyContent#supportedObjectTypes` contract ("the set of metadata
object types that the policy can be applied to") and with the existing built-in
types, which declare their evaluation targets (e.g. compaction uses `{CATALOG,
SCHEMA, TABLE}`).
The design doc (`tag-based-read-restrictions-for-iceberg-rest.md`, lines
84-85 and 94) says the evaluation target is Table for `system_row_filter` and
top-level Column for `system_column_mask`, and that the tag is only the
selection carrier. Today only the update consistency check in `PolicyManager`
reads this field, so nothing breaks yet, but it is public API: once the
resolver/enforcement filters by or validates against `supportedObjectTypes`, a
row filter declared for `TAG` will never match a table, and the field stops
carrying any information for tag-selected policies.
Suggestion: use `{TABLE}` for row filters and `{COLUMN}` for column masks
(and update `TestReadRestrictionContent`). If `TAG` is intentional, please
document why here and update the interface Javadoc, since the current contract
says otherwise.
##########
api/src/main/java/org/apache/gravitino/policy/Policy.java:
##########
@@ -47,6 +47,12 @@ enum BuiltInType {
BUILT_IN_TYPE_PREFIX + "iceberg_orphan_file_removal",
IcebergOrphanFileRemovalContent.class),
+ /** Built-in policy type for filtering rows of tagged tables. */
+ ROW_FILTER(BUILT_IN_TYPE_PREFIX + "row_filter", RowFilterContent.class),
Review Comment:
**[Question]** After this PR, users can create `system_row_filter` /
`system_column_mask` policies through REST and the Java client and attach them
to tags, but nothing enforces them yet (parsing, resolution and Iceberg REST
enforcement are follow-ups). A user could reasonably believe their data is
restricted when it is not.
Is the enforcement guaranteed to land in the same release? If not, should
the server reject creating these types (or put them behind a flag) until
enforcement exists? At minimum, `docs/policies.md` could list them as "not
enforced yet" (that table also misses the existing orphan-file type).
##########
core/src/test/java/org/apache/gravitino/meta/TestPolicyEntity.java:
##########
@@ -155,4 +155,35 @@ public void testBuiltInPolicyContentTypeValidation() {
.withAuditInfo(auditInfo)
.build());
}
+
+ @Test
+ public void testReadRestrictionPolicyContentTypes() {
Review Comment:
**[Nit]** The new tests stop at the DTO / entity / `anyFieldMapper` layer;
neither `TestPolicyManager` nor the REST `TestPolicyOperations` covers the new
types. A `TestPolicyManager` case that creates a row-filter policy, updates its
content and reads it back would exercise the real store round trip and the
built-in `supportedObjectTypes` consistency branch in
`PolicyManager.updatePolicyEntity`.
##########
common/src/test/java/org/apache/gravitino/dto/policy/TestReadRestrictionPolicyDTO.java:
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.dto.policy;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import org.apache.gravitino.dto.requests.PolicyCreateRequest;
+import org.apache.gravitino.dto.requests.PolicyUpdateRequest;
+import org.apache.gravitino.dto.util.DTOConverters;
+import org.apache.gravitino.json.JsonUtils;
+import org.apache.gravitino.policy.ColumnMaskContent;
+import org.apache.gravitino.policy.Policy;
+import org.apache.gravitino.policy.PolicyContent;
+import org.apache.gravitino.policy.PolicyContents;
+import org.apache.gravitino.policy.RowFilterContent;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestReadRestrictionPolicyDTO {
+
+ @Test
+ void testRowFilterPolicyRoundTrip() throws JsonProcessingException {
+ String expression =
+ "filter := col(\"region\") == \"US\" if is_group_member(\"auditors\") "
+ + "else := col(\"owner\") == session_user()";
+ RowFilterContent content = PolicyContents.rowFilter(expression);
+ PolicyDTO policy =
+ PolicyDTO.builder()
+ .withName("owner-filter")
+ .withPolicyType("system_row_filter")
+ .withEnabled(true)
+ .withContent(DTOConverters.toDTO(content))
+ .build();
+
+ String json = JsonUtils.objectMapper().writeValueAsString(policy);
+ PolicyDTO restored = JsonUtils.objectMapper().readValue(json,
PolicyDTO.class);
+
+ Assertions.assertEquals(policy, restored);
+ Assertions.assertInstanceOf(PolicyContentDTO.RowFilterContentDTO.class,
restored.content());
+ Assertions.assertTrue(json.contains("\"expression\":\"filter :="));
+ Assertions.assertFalse(json.contains("\"rules\""));
+ Assertions.assertEquals(content,
DTOConverters.fromDTO(restored.content()));
+ Assertions.assertDoesNotThrow(restored.content()::validate);
+ }
+
+ @Test
+ void testRowFilterCreateRequestRoundTrip() throws JsonProcessingException {
+ String json =
+ "{"
+ + "\"name\":\"region-filter\","
+ + "\"policyType\":\"system_row_filter\","
+ + "\"enabled\":true,"
+ + "\"content\":{\"expression\":"
+ + "\"filter := col(\\\"region\\\") == \\\"US\\\" if "
+ + "is_group_member(\\\"auditors\\\") else := false\"}}";
+
+ PolicyCreateRequest request =
+ JsonUtils.objectMapper().readValue(json, PolicyCreateRequest.class);
+ Assertions.assertInstanceOf(
+ PolicyContentDTO.RowFilterContentDTO.class,
request.getPolicyContent());
+ Assertions.assertDoesNotThrow(request::validate);
+
+ RowFilterContent content = (RowFilterContent)
DTOConverters.fromDTO(request.getPolicyContent());
+ Assertions.assertEquals(
+ "filter := col(\"region\") == \"US\" if is_group_member(\"auditors\")
else := false",
+ content.expression());
+ }
+
+ @Test
+ void testColumnMaskCreateRequestRoundTrip() throws JsonProcessingException {
+ String json =
+ "{"
+ + "\"name\":\"pii-mask\","
+ + "\"policyType\":\"system_column_mask\","
+ + "\"enabled\":true,"
+ + "\"content\":{\"expression\":"
+ + "\"mask := action(\\\"show-last-4\\\") if "
+ + "is_group_member(\\\"pii_unmasked\\\") "
+ + "else := action(\\\"mask-alphanum\\\")\"}}";
+
+ PolicyCreateRequest request =
+ JsonUtils.objectMapper().readValue(json, PolicyCreateRequest.class);
+ Assertions.assertInstanceOf(
+ PolicyContentDTO.ColumnMaskContentDTO.class,
request.getPolicyContent());
+ Assertions.assertDoesNotThrow(request::validate);
+
+ ColumnMaskContent content =
+ (ColumnMaskContent) DTOConverters.fromDTO(request.getPolicyContent());
+ Assertions.assertEquals(
+ "mask := action(\"show-last-4\") if is_group_member(\"pii_unmasked\") "
+ + "else := action(\"mask-alphanum\")",
+ content.expression());
+ }
+
+ @Test
+ void testUpdateRequestsUseEachPolicyType() throws JsonProcessingException {
+ assertUpdateRoundTrip(
+ "system_row_filter",
+ PolicyContents.rowFilter(
+ "filter := col(\"region\") == \"US\" if session_user() ==
\"alice\" "
+ + "else := col(\"owner\") == session_user()"));
+ assertUpdateRoundTrip(
+ "system_column_mask",
+ PolicyContents.columnMask(
+ "mask := action(\"show-last-4\") if session_user() == \"alice\" "
+ + "else := action(\"replace-with-null\")"));
+ }
+
+ @Test
+ void testStoredContentRoundTripUsesEachBuiltInContentClass() throws
JsonProcessingException {
+ RowFilterContent rowFilter =
+ PolicyContents.rowFilter(
+ "filter := col(\"region\") == \"US\" if
is_group_member(\"analysts\") "
+ + "else := col(\"owner\") == session_user()");
+ ColumnMaskContent columnMask =
+ PolicyContents.columnMask(
+ "mask := action(\"show-last-4\") if is_group_member(\"analysts\") "
+ + "else := action(\"replace-with-null\")");
+
+ PolicyContent restoredRowFilter =
+ JsonUtils.anyFieldMapper()
+ .readValue(
+ JsonUtils.anyFieldMapper().writeValueAsString(rowFilter),
+ Policy.BuiltInType.ROW_FILTER.contentClass());
+ PolicyContent restoredColumnMask =
+ JsonUtils.anyFieldMapper()
+ .readValue(
+ JsonUtils.anyFieldMapper().writeValueAsString(columnMask),
+ Policy.BuiltInType.COLUMN_MASK.contentClass());
+
+ Assertions.assertEquals(rowFilter, restoredRowFilter);
+ Assertions.assertEquals(columnMask, restoredColumnMask);
+ Assertions.assertDoesNotThrow(restoredRowFilter::validate);
+ Assertions.assertDoesNotThrow(restoredColumnMask::validate);
+ }
+
+ @Test
+ void testRejectsLegacyRuleShape() {
+ String rowFilterWithRules =
+ "{"
+ + "\"name\":\"invalid-filter\","
+ + "\"policyType\":\"system_row_filter\","
+ + "\"enabled\":true,"
+ + "\"content\":{\"rules\":[{\"expression\":\"filter := true\"}]}"
+ + "}";
+ String columnMaskWithRules =
+ "{"
+ + "\"name\":\"invalid-mask\","
+ + "\"policyType\":\"system_column_mask\","
+ + "\"enabled\":true,"
+ + "\"content\":{\"rules\":[{\"action\":\"show-first-4\"}]}"
+ + "}";
+
+ Assertions.assertThrows(
+ JsonProcessingException.class,
Review Comment:
**[Nit]** `JsonProcessingException` is too broad to prove the legacy `rules`
shape is what gets rejected. As a mutation check, I removed the
`system_row_filter` subtype from `PolicyCreateRequest`: this test still passed,
because an unknown type id throws the same exception. Asserting
`UnrecognizedPropertyException` (and that the message mentions `rules`) would
pin the intended reason.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
