snazy commented on code in PR #2048: URL: https://github.com/apache/polaris/pull/2048#discussion_r2212550907
########## polaris-core/src/main/java/org/apache/polaris/core/policy/content/IcebergExpressionListDeserializer.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.polaris.core.policy.content; + +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JsonDeserializer; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import org.apache.iceberg.expressions.Expression; +import org.apache.iceberg.expressions.ExpressionParser; + +public class IcebergExpressionListDeserializer extends JsonDeserializer<List<Expression>> { + @Override + public List<Expression> deserialize(JsonParser p, DeserializationContext ctxt) + throws IOException { + ObjectMapper mapper = (ObjectMapper) p.getCodec(); Review Comment: This cast assumes a Jackson implementation detail and can likely break with Jackson updates. Therefore please remove this case. ########## polaris-core/src/main/java/org/apache/polaris/core/policy/content/IcebergExpressionListDeserializer.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.polaris.core.policy.content; + +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JsonDeserializer; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import org.apache.iceberg.expressions.Expression; +import org.apache.iceberg.expressions.ExpressionParser; + +public class IcebergExpressionListDeserializer extends JsonDeserializer<List<Expression>> { + @Override + public List<Expression> deserialize(JsonParser p, DeserializationContext ctxt) + throws IOException { + ObjectMapper mapper = (ObjectMapper) p.getCodec(); + JsonNode node = mapper.readTree(p); + + List<Expression> expressions = new ArrayList<>(); + if (node.isArray()) { + for (JsonNode element : node) { + // Convert each JSON element back to a string and pass it to ExpressionParser.fromJson + expressions.add(ExpressionParser.fromJson(mapper.writeValueAsString(element))); + } + } Review Comment: Why is this necessary at all? ########## polaris-core/src/main/java/org/apache/polaris/core/policy/content/PolicyContentUtil.java: ########## @@ -30,6 +31,8 @@ private static ObjectMapper configureMapper() { mapper.configure(DeserializationFeature.FAIL_ON_MISSING_CREATOR_PROPERTIES, true); // Fails if a required field is present but explicitly null, e.g., {"enable": null} mapper.configure(DeserializationFeature.FAIL_ON_NULL_CREATOR_PROPERTIES, true); + // This will make sure all the Iceberg parsers are loaded. + RESTSerializers.registerAll(mapper); Review Comment: Why do we need all the REST specific types? ########## polaris-core/src/main/java/org/apache/polaris/core/policy/content/IcebergExpressionListSerializer.java: ########## @@ -0,0 +1,50 @@ +/* + * 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.polaris.core.policy.content; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.databind.JsonSerializer; +import com.fasterxml.jackson.databind.SerializerProvider; +import java.io.IOException; +import java.util.List; +import org.apache.iceberg.expressions.Expression; +import org.apache.iceberg.expressions.ExpressionParser; + +/** + * Custom Jackson JsonSerializer for a List of Iceberg Expression objects. This serializer converts + * each Iceberg Expression into its string representation. + */ +public class IcebergExpressionListSerializer extends JsonSerializer<List<Expression>> { + + @Override + public void serialize( + List<Expression> expressions, + JsonGenerator jsonGenerator, + SerializerProvider serializerProvider) + throws IOException { + jsonGenerator.writeStartArray(); + if (expressions != null) { + for (Expression expression : expressions) { + jsonGenerator.writeString(ExpressionParser.toJson(expression)); Review Comment: Wyh "string" and not proper JSON? ########## polaris-core/src/test/java/org/apache/polaris/core/policy/AccessControlPolicyContentTest.java: ########## @@ -0,0 +1,307 @@ +/* + * 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.polaris.core.policy; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Arrays; +import java.util.List; +import org.apache.iceberg.expressions.Expression; +import org.apache.iceberg.expressions.Expressions; +import org.apache.polaris.core.policy.content.AccessControlPolicyContent; +import org.apache.polaris.core.policy.validator.InvalidPolicyException; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +class AccessControlPolicyContentTest { + @Test + @DisplayName("Should deserialize a full policy with all fields correctly") + void testFromString_fullPolicy() { + String jsonContent = + "{\n" + + " \"principalRole\": \"ANALYST\",\n" + + " \"columnProjections\": [\"name\", \"location\"],\n" + + " \"rowFilters\": [\n" + + " {\n" + + " \"type\": \"eq\",\n" + + " \"term\": \"country\",\n" + + " \"value\": \"USA\"\n" + + " },\n" + + " {\n" + + " \"type\": \"eq\",\n" + + " \"term\": \"name\",\n" + + " \"value\": \"PK\"\n" + + " }\n" + + " ]\n" + + "}"; + + AccessControlPolicyContent policy = AccessControlPolicyContent.fromString(jsonContent); + + assertNotNull(policy); + assertEquals("ANALYST", policy.getPrincipalRole()); + assertEquals(Arrays.asList("name", "location"), policy.getColumnProjections()); + + // Validate rowFilters + assertNotNull(policy.getRowFilters()); + assertEquals(2, policy.getRowFilters().size()); + + Expression filter1 = policy.getRowFilters().get(0); + Expression filter2 = policy.getRowFilters().get(1); + assertEquals("ref(name=\"country\") == \"USA\"", filter1.toString()); + assertEquals("ref(name=\"name\") == \"PK\"", filter2.toString()); + } + + @Test + @DisplayName( + "Should deserialize a full policy with all fields correctly - with context variables") + void testFromString_fullPolicy_withContextVariable() { + String jsonContent = + "{\n" + + " \"principalRole\": \"ANALYST\",\n" + + " \"columnProjections\": [\"name\", \"location\"],\n" + + " \"rowFilters\": [\n" + + " {\n" + + " \"type\": \"eq\",\n" + + " \"term\": \"country\",\n" + + " \"value\": \"USA\"\n" + + " },\n" + + " {\n" + + " \"type\": \"eq\",\n" + + " \"term\": \"$current_principal_role\",\n" + + " \"value\": \"PK\"\n" + + " }\n" + + " ]\n" + + "}"; + + AccessControlPolicyContent policy = AccessControlPolicyContent.fromString(jsonContent); + + assertNotNull(policy); + assertEquals("ANALYST", policy.getPrincipalRole()); + assertEquals(Arrays.asList("name", "location"), policy.getColumnProjections()); + + // Validate rowFilters + assertNotNull(policy.getRowFilters()); + assertEquals(2, policy.getRowFilters().size()); + + Expression filter1 = policy.getRowFilters().get(0); + Expression filter2 = policy.getRowFilters().get(1); + assertEquals("ref(name=\"country\") == \"USA\"", filter1.toString()); + assertEquals("ref(name=\"$current_principal_role\") == \"PK\"", filter2.toString()); + } + + @Test + @DisplayName( + "Should fail deserialize policy with only required fields (principalRole empty, lists empty/null)") + void testFromString_minimalPolicy() { + String jsonContent = + "{\n" + + " \"principalRole\": null,\n" + + " \"columnProjections\": [],\n" + + " \"rowFilters\": []\n" + + "}"; + assertThrows( + InvalidPolicyException.class, + () -> { + AccessControlPolicyContent.fromString(jsonContent); + }); + } + + @Test + void testFromString_missingOptionalFields() { + String jsonContent = "{\n" + " \"principalRole\": \"DATA_ENGINEER\"\n" + "}"; + assertThrows( + InvalidPolicyException.class, + () -> { + AccessControlPolicyContent.fromString(jsonContent); + }); + } + + @Test + @DisplayName("Should throw InvalidPolicyException for empty content") + void testFromString_emptyContent() { + String emptyContent = ""; + InvalidPolicyException exception = + assertThrows( + InvalidPolicyException.class, + () -> { + AccessControlPolicyContent.fromString(emptyContent); + }); + assertEquals("Policy is empty", exception.getMessage()); + } + + @Test + @DisplayName("Should throw InvalidPolicyException for null content") + void testFromString_nullContent() { + String nullContent = null; + InvalidPolicyException exception = + assertThrows( + InvalidPolicyException.class, + () -> { + AccessControlPolicyContent.fromString(nullContent); + }); + assertEquals("Policy is empty", exception.getMessage()); + } + + @Test + @DisplayName("Should throw InvalidPolicyException for invalid JSON content") + void testFromString_invalidJson() { + String invalidJson = + "{ \"principalRole\": \"ANALYST\", \"columnProjections\": [\"name\", }"; // Malformed JSON + InvalidPolicyException exception = + assertThrows( + InvalidPolicyException.class, + () -> { + AccessControlPolicyContent.fromString(invalidJson); + }); + assertNotNull( + exception.getCause()); // Check that the cause is set (Jackson's JsonParseException) + assertTrue(exception.getCause() instanceof com.fasterxml.jackson.core.JsonProcessingException); + } + + @Test + @DisplayName( + "Should handle rowFilters with different Iceberg expression types (if supported by parser)") + void testFromString_differentRowFilterTypes() { + // This test assumes your ExpressionParser can handle various types. + // Our dummy only handles 'eq' for now, but in a real scenario, you'd test 'and', 'or', 'gt', + // etc. + String jsonContent = + "{\n" + + " \"principalRole\": \"ADMIN\",\n" + + " \"rowFilters\": [\n" + + " {\n" + + " \"type\": \"gt\",\n" + + // Example of a different type + " \"term\": \"age\",\n" + + " \"value\": \"30\"\n" + + " },\n" + + " {\n" + + " \"type\": \"eq\",\n" + + " \"term\": \"status\",\n" + + " \"value\": \"active\"\n" + + " }\n" + + " ]\n" + + "}"; + + AccessControlPolicyContent policy = AccessControlPolicyContent.fromString(jsonContent); + assertNotNull(policy); + assertEquals(2, policy.getRowFilters().size()); + Expression filter1 = policy.getRowFilters().get(0); + Expression filter2 = policy.getRowFilters().get(1); + assertEquals("ref(name=\"age\") > \"30\"", filter1.toString()); + assertEquals("ref(name=\"status\") == \"active\"", filter2.toString()); + } + + @Test + @DisplayName("Should correctly handle empty lists in JSON") + void testFromString_emptyListsInJson() { + String jsonContent = + "{\n" + + " \"principalRole\": \"VIEWER\",\n" + + " \"columnProjections\": [],\n" + + " \"rowFilters\": []\n" + + "}"; + + assertThrows( + InvalidPolicyException.class, + () -> { + AccessControlPolicyContent.fromString(jsonContent); + }); + } + + @Test + void testToString_basicPolicy() { + AccessControlPolicyContent policy = new AccessControlPolicyContent(); + policy.setPrincipalRole("analyst"); + policy.setAllowedColumns(Arrays.asList("col1", "col2")); + policy.setRowFilters(null); + + String expectedJson = + "{\"principalRole\":\"analyst\",\"columnProjections\":[\"col1\",\"col2\"],\"rowFilters\":null}"; + String actualJson = AccessControlPolicyContent.toString(policy); + + assertEquals(expectedJson, actualJson); + } + + @Test + void testToString_policyWithRowFilters() { + AccessControlPolicyContent policy = new AccessControlPolicyContent(); + policy.setPrincipalRole("data_engineer"); + policy.setAllowedColumns(List.of()); + + Expression filter1 = Expressions.equal("name", "Alice"); + Expression filter2 = Expressions.greaterThan("age", 30); + policy.setRowFilters(Arrays.asList(filter1, filter2)); + + String expectedJson = + "{\"principalRole\":\"data_engineer\",\"columnProjections\":[],\"rowFilters\":[\"{\\\"type\\\":\\\"eq\\\",\\\"term\\\":\\\"name\\\",\\\"value\\\":\\\"Alice\\\"}\",\"{\\\"type\\\":\\\"gt\\\",\\\"term\\\":\\\"age\\\",\\\"value\\\":30}\"]}"; + + String actualJson = AccessControlPolicyContent.toString(policy); + + assertEquals(expectedJson, actualJson); + } + + @Test + void testToString_emptyPolicy() { + AccessControlPolicyContent policy = new AccessControlPolicyContent(); + policy.setPrincipalRole(null); + policy.setAllowedColumns(List.of()); + policy.setRowFilters(List.of()); + + String expectedJson = "{\"principalRole\":null,\"columnProjections\":[],\"rowFilters\":[]}"; + String actualJson = AccessControlPolicyContent.toString(policy); + + assertEquals(expectedJson, actualJson); + } + + @Test + void testToString_nullInput() { + String actualJson = AccessControlPolicyContent.toString(null); + Assertions.assertNull(actualJson); + } + + @Test + void testToString_exceptionHandling() { + AccessControlPolicyContent policy = new AccessControlPolicyContent(); + policy.setAllowedColumns(Arrays.asList("id")); + Assertions.assertDoesNotThrow(() -> AccessControlPolicyContent.toString(policy)); + } + + @Test + @DisplayName( + "Should handle unmapped properties if FAIL_ON_UNKNOWN_PROPERTIES is false (default for this setup)") + void testFromString_unmappedProperties() { + String jsonContent = + "{\n" + + " \"principalRole\": \"ANALYST\",\n" + + " \"extraField\": \"someValue\"\n" + + // Extra field Review Comment: What's the "extra field"? -- 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]
