adnanhemani commented on code in PR #3293: URL: https://github.com/apache/polaris/pull/3293#discussion_r2646354561
########## runtime/service/src/test/java/org/apache/polaris/service/events/AttributeKeyTest.java: ########## @@ -0,0 +1,107 @@ +/* + * 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.service.events; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.jupiter.api.Test; + +class AttributeKeyTest { + + @Test + void testCreateAndAccessors() { + AttributeKey<String> key = AttributeKey.of("test_key", String.class); + + assertThat(key.name()).isEqualTo("test_key"); + assertThat(key.type()).isEqualTo(String.class); + } + + @Test + void testTypeCastValidValue() { Review Comment: Not sure I see value in this test case. Maybe do something like making an AttributeKey of a Long value, then give it an int and see if it casted that properly? IOW, we shouldn't really be testing `cast` if we didn't write it. We should be testing the code that we wrote. ########## runtime/service/src/test/java/org/apache/polaris/service/events/PolarisEventTest.java: ########## @@ -0,0 +1,109 @@ +/* + * 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.service.events; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.Optional; +import org.junit.jupiter.api.Test; + +class PolarisEventTest { + + private static final PolarisEventMetadata TEST_METADATA = + ImmutablePolarisEventMetadata.builder().realmId("test-realm").build(); + + @Test + void testBuilderCreatesEvent() { + PolarisEvent event = + PolarisEvent.builder(PolarisEventType.BEFORE_CREATE_TABLE, TEST_METADATA) + .attribute(EventAttributes.CATALOG_NAME, "my-catalog") Review Comment: nit: extract the magic variables for maintainability ########## runtime/service/src/test/java/org/apache/polaris/service/events/AttributeKeyTest.java: ########## @@ -0,0 +1,107 @@ +/* + * 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.service.events; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.jupiter.api.Test; + +class AttributeKeyTest { + + @Test + void testCreateAndAccessors() { + AttributeKey<String> key = AttributeKey.of("test_key", String.class); + + assertThat(key.name()).isEqualTo("test_key"); + assertThat(key.type()).isEqualTo(String.class); + } + + @Test + void testTypeCastValidValue() { + AttributeKey<String> key = AttributeKey.of("test_key", String.class); + + String result = key.type().cast("hello"); + + assertThat(result).isEqualTo("hello"); + } + + @Test + void testTypeCastNullValue() { + AttributeKey<String> key = AttributeKey.of("test_key", String.class); + + String result = key.type().cast(null); + + assertThat(result).isNull(); + } + + @Test + void testTypeCastInvalidType() { Review Comment: Similar to above, I would move this test into the test for `PolarisEvent` and test that sending an int to an AttributeKey<String> would throw when calling `key.attribute(..., 123)`. ########## runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java: ########## @@ -2360,13 +2362,15 @@ public void testEventsAreEmitted() { table.updateProperties().set(key, valOld).commit(); table.updateProperties().set(key, valNew).commit(); - var beforeRefreshEvent = - testPolarisEventListener.getLatest(IcebergRestCatalogEvents.BeforeRefreshTableEvent.class); - Assertions.assertThat(beforeRefreshEvent.tableIdentifier()).isEqualTo(TestData.TABLE); + PolarisEvent beforeRefreshEvent = + testPolarisEventListener.getLatest(PolarisEventType.BEFORE_REFRESH_TABLE); + Assertions.assertThat(beforeRefreshEvent.attribute(EventAttributes.TABLE_IDENTIFIER)) Review Comment: nit: Let's test these using `requiredAttribute` rather than `attribute`? ########## runtime/service/src/test/java/org/apache/polaris/service/events/PolarisEventTest.java: ########## @@ -0,0 +1,109 @@ +/* + * 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.service.events; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.Optional; +import org.junit.jupiter.api.Test; + +class PolarisEventTest { + + private static final PolarisEventMetadata TEST_METADATA = + ImmutablePolarisEventMetadata.builder().realmId("test-realm").build(); + + @Test + void testBuilderCreatesEvent() { + PolarisEvent event = + PolarisEvent.builder(PolarisEventType.BEFORE_CREATE_TABLE, TEST_METADATA) + .attribute(EventAttributes.CATALOG_NAME, "my-catalog") + .attribute(EventAttributes.TABLE_NAME, "my-table") + .build(); + + assertThat(event.type()).isEqualTo(PolarisEventType.BEFORE_CREATE_TABLE); + assertThat(event.metadata()).isEqualTo(TEST_METADATA); + assertThat(event.attribute(EventAttributes.CATALOG_NAME)).isEqualTo(Optional.of("my-catalog")); + assertThat(event.attribute(EventAttributes.TABLE_NAME)).isEqualTo(Optional.of("my-table")); + } + + @Test + void testAttributeReturnsEmptyForMissingKey() { + PolarisEvent event = + PolarisEvent.builder(PolarisEventType.BEFORE_CREATE_TABLE, TEST_METADATA).build(); + + assertThat(event.attribute(EventAttributes.CATALOG_NAME)).isEmpty(); + } + + @Test + void testHasAttribute() { + PolarisEvent event = + PolarisEvent.builder(PolarisEventType.BEFORE_CREATE_TABLE, TEST_METADATA) + .attribute(EventAttributes.CATALOG_NAME, "my-catalog") + .build(); + + assertThat(event.hasAttribute(EventAttributes.CATALOG_NAME)).isTrue(); + assertThat(event.hasAttribute(EventAttributes.TABLE_NAME)).isFalse(); + } + + @Test + void testAttributesReturnsUnmodifiableMap() { + PolarisEvent event = + PolarisEvent.builder(PolarisEventType.BEFORE_CREATE_TABLE, TEST_METADATA) + .attribute(EventAttributes.CATALOG_NAME, "my-catalog") + .build(); + + assertThat(event.attributes()).hasSize(1); + assertThat(event.attributes().get(EventAttributes.CATALOG_NAME)).isEqualTo("my-catalog"); + assertThatThrownBy(() -> event.attributes().put(EventAttributes.TABLE_NAME, "table")) + .isInstanceOf(UnsupportedOperationException.class); + } + + @Test + void testNullAttributeValueIsIgnored() { + PolarisEvent event = + PolarisEvent.builder(PolarisEventType.BEFORE_CREATE_TABLE, TEST_METADATA) + .attribute(EventAttributes.CATALOG_NAME, null) + .build(); + + assertThat(event.hasAttribute(EventAttributes.CATALOG_NAME)).isFalse(); + assertThat(event.attributes()).isEmpty(); + } + + @Test + void testRequiredAttributeReturnsValue() { + PolarisEvent event = + PolarisEvent.builder(PolarisEventType.BEFORE_CREATE_TABLE, TEST_METADATA) + .attribute(EventAttributes.CATALOG_NAME, "my-catalog") + .build(); + + assertThat(event.requiredAttribute(EventAttributes.CATALOG_NAME)).isEqualTo("my-catalog"); + } + + @Test + void testRequiredAttributeThrowsForMissingKey() { + PolarisEvent event = + PolarisEvent.builder(PolarisEventType.BEFORE_CREATE_TABLE, TEST_METADATA).build(); + + assertThatThrownBy(() -> event.requiredAttribute(EventAttributes.CATALOG_NAME)) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("catalog_name") Review Comment: nit: Can we test for a more descriptive message? Not looking for exact string matching but something similar to "Required attribute" and/or "not found in event". ########## runtime/service/src/main/java/org/apache/polaris/service/events/EventAttributes.java: ########## @@ -0,0 +1,248 @@ +/* + * 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.service.events; + +import java.util.Map; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.rest.requests.CommitTransactionRequest; +import org.apache.iceberg.rest.requests.CreateNamespaceRequest; +import org.apache.iceberg.rest.requests.CreateTableRequest; +import org.apache.iceberg.rest.requests.CreateViewRequest; +import org.apache.iceberg.rest.requests.RegisterTableRequest; +import org.apache.iceberg.rest.requests.RenameTableRequest; +import org.apache.iceberg.rest.requests.UpdateNamespacePropertiesRequest; +import org.apache.iceberg.rest.requests.UpdateTableRequest; +import org.apache.iceberg.rest.responses.ConfigResponse; +import org.apache.iceberg.rest.responses.LoadTableResponse; +import org.apache.iceberg.rest.responses.LoadViewResponse; +import org.apache.iceberg.rest.responses.UpdateNamespacePropertiesResponse; +import org.apache.iceberg.view.ViewMetadata; +import org.apache.polaris.core.admin.model.AddGrantRequest; +import org.apache.polaris.core.admin.model.Catalog; +import org.apache.polaris.core.admin.model.CatalogRole; +import org.apache.polaris.core.admin.model.CreatePrincipalRequest; +import org.apache.polaris.core.admin.model.CreatePrincipalRoleRequest; +import org.apache.polaris.core.admin.model.GrantResource; +import org.apache.polaris.core.admin.model.Principal; +import org.apache.polaris.core.admin.model.PrincipalRole; +import org.apache.polaris.core.admin.model.PrincipalWithCredentials; +import org.apache.polaris.core.admin.model.RevokeGrantRequest; +import org.apache.polaris.core.admin.model.UpdateCatalogRequest; +import org.apache.polaris.core.admin.model.UpdateCatalogRoleRequest; +import org.apache.polaris.core.admin.model.UpdatePrincipalRequest; +import org.apache.polaris.core.admin.model.UpdatePrincipalRoleRequest; +import org.apache.polaris.core.entity.PolarisPrivilege; +import org.apache.polaris.service.types.AttachPolicyRequest; +import org.apache.polaris.service.types.CommitViewRequest; +import org.apache.polaris.service.types.CreateGenericTableRequest; +import org.apache.polaris.service.types.CreatePolicyRequest; +import org.apache.polaris.service.types.DetachPolicyRequest; +import org.apache.polaris.service.types.GenericTable; +import org.apache.polaris.service.types.GetApplicablePoliciesResponse; +import org.apache.polaris.service.types.LoadPolicyResponse; +import org.apache.polaris.service.types.NotificationRequest; +import org.apache.polaris.service.types.UpdatePolicyRequest; + +/** + * Standard attribute keys for Polaris events. These keys provide type-safe access to common event + * attributes and enable automatic pruning/filtering logic. + */ +public final class EventAttributes { + private EventAttributes() {} + + // Catalog attributes + public static final AttributeKey<String> CATALOG_NAME = + AttributeKey.of("catalog_name", String.class); + public static final AttributeKey<Catalog> CATALOG = AttributeKey.of("catalog", Catalog.class); + public static final AttributeKey<UpdateCatalogRequest> UPDATE_CATALOG_REQUEST = + AttributeKey.of("update_catalog_request", UpdateCatalogRequest.class); + + // Namespace attributes + public static final AttributeKey<Namespace> NAMESPACE = + AttributeKey.of("namespace", Namespace.class); + public static final AttributeKey<String> NAMESPACE_STRING = + AttributeKey.of("namespace_string", String.class); + public static final AttributeKey<String> PARENT_NAMESPACE = + AttributeKey.of("parent_namespace", String.class); + public static final AttributeKey<CreateNamespaceRequest> CREATE_NAMESPACE_REQUEST = + AttributeKey.of("create_namespace_request", CreateNamespaceRequest.class); + public static final AttributeKey<UpdateNamespacePropertiesRequest> + UPDATE_NAMESPACE_PROPERTIES_REQUEST = + AttributeKey.of( + "update_namespace_properties_request", UpdateNamespacePropertiesRequest.class); + public static final AttributeKey<UpdateNamespacePropertiesResponse> + UPDATE_NAMESPACE_PROPERTIES_RESPONSE = + AttributeKey.of( + "update_namespace_properties_response", UpdateNamespacePropertiesResponse.class); + + @SuppressWarnings("unchecked") + public static final AttributeKey<Map<String, String>> NAMESPACE_PROPERTIES = + (AttributeKey<Map<String, String>>) + (AttributeKey<?>) AttributeKey.of("namespace_properties", Map.class); Review Comment: Admittedly I haven't thought through all the ramifications, but initially I agree with @adutra on this one. Maybe we just keep the bar that whatever the object is, it must be implementing `Serializable`? Personally, I'd rather have this enforced at the time of the event generation than having to deal with it later in the Event lifecycle - but not a strong opinion. -- 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]
