roryqi commented on code in PR #13693: URL: https://github.com/apache/gravitino/pull/13693#discussion_r4219446701
########## 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: Updated in 990bb87575 and 39afea1210. The assertions now expect {TABLE} for row filters and {TABLE, COLUMN} for column masks. ########## 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: Updated in 990bb87575 and refined in 39afea1210. Row filters now report {TABLE}; column masks report {TABLE, COLUMN}, since column masks can be applied at either table or column scope. The tests and public Javadocs were updated accordingly. ########## 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( Review Comment: Fixed in 04f2e0572a. Added distinct RowFilterContent and ColumnMaskContent schemas, response/create/update schemas, all oneOf and discriminator mappings, built-in exclusions, and the nonblank/16 KiB UTF-8 documentation. ./gradlew :docs:build passed, and all six response/create/update payloads were validated. ########## 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: Added in 990bb87575. TestPolicyManager now creates a row-filter policy, reads it through the real store, updates its content through PolicyChange.updateContent, and reads the updated content back, exercising the supportedObjectTypes consistency branch. ########## 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: The enforcement work is not part of this PR, so 990bb87575 updates docs/policies.md to mark row-filter and column-mask policies as not enforced yet and explicitly warn that they do not currently restrict data access. The table now also includes system_iceberg_orphan_file_removal. ########## 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: Fixed in 990bb87575. The test now asserts UnrecognizedPropertyException specifically, checks that the rejected property is rules, and verifies that the exception message mentions rules. ########## 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: Addressed in 04f2e0572a. The OpenAPI contract now includes the two typed content models and their policy/create/update variants, with complete discriminator mappings and built-in exclusions. ./gradlew :docs:build passes. ########## 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()); + Assertions.assertEquals( + ImmutableSet.of(MetadataObject.Type.TAG), columnMask.supportedObjectTypes()); + Assertions.assertTrue(rowFilter.properties().isEmpty()); + Assertions.assertTrue(columnMask.properties().isEmpty()); + Assertions.assertDoesNotThrow(rowFilter::validate); + Assertions.assertDoesNotThrow(columnMask::validate); + } + + @Test + void testEqualityUsesPolicyTypeAndExpression() { + String expression = "filter := col(\"region\") == \"US\""; + + Assertions.assertEquals( + PolicyContents.rowFilter(expression), PolicyContents.rowFilter(expression)); + Assertions.assertNotEquals( + PolicyContents.rowFilter(expression), PolicyContents.rowFilter("filter := true")); + Assertions.assertNotEquals( + PolicyContents.rowFilter(expression), PolicyContents.columnMask(expression)); + } + + @Test + void testRejectsMissingOrBlankExpressions() { + Assertions.assertThrows(IllegalArgumentException.class, () -> PolicyContents.rowFilter(null)); + Assertions.assertThrows(IllegalArgumentException.class, () -> PolicyContents.columnMask(null)); + Assertions.assertThrows(IllegalArgumentException.class, () -> PolicyContents.rowFilter(" \t")); + Assertions.assertThrows(IllegalArgumentException.class, () -> PolicyContents.columnMask("\n")); + } + + @Test + void testValidatesUtf8ExpressionLength() { + String maximumLength = "é".repeat(ReadRestrictionContent.MAX_SOURCE_LENGTH_BYTES / 2); Review Comment: Fixed in 04f2e0572a. The test now independently asserts that MAX_SOURCE_LENGTH_BYTES is exactly 16 * 1024 while retaining the multibyte boundary cases. -- 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]
