Copilot commented on code in PR #10834: URL: https://github.com/apache/gravitino/pull/10834#discussion_r3147006101
########## core/src/test/java/org/apache/gravitino/hook/TestPolicyHookDispatcher.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.hook; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.authorization.OwnerDispatcher; +import org.apache.gravitino.meta.PolicyEntity; +import org.apache.gravitino.policy.PolicyDispatcher; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class TestPolicyHookDispatcher { + + private PolicyHookDispatcher hookDispatcher; + private PolicyDispatcher mockDispatcher; + private OwnerDispatcher mockOwnerDispatcher; + + @BeforeEach + public void setUp() throws IllegalAccessException { + mockDispatcher = mock(PolicyDispatcher.class); + mockOwnerDispatcher = mock(OwnerDispatcher.class); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + hookDispatcher = new PolicyHookDispatcher(mockDispatcher); + } + + @AfterEach + public void tearDown() throws IllegalAccessException { + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", null, true); Review Comment: This test overwrites `GravitinoEnv.ownerDispatcher` and unconditionally sets it to `null` in teardown. To keep tests isolated (and avoid breaking other tests that may have initialized `GravitinoEnv`), capture the original field value in `setUp` and restore it in `tearDown` (similar to `TestCatalogHookDispatcher`’s save/restore pattern). ```suggestion private OwnerDispatcher originalOwnerDispatcher; @BeforeEach public void setUp() throws IllegalAccessException { mockDispatcher = mock(PolicyDispatcher.class); mockOwnerDispatcher = mock(OwnerDispatcher.class); originalOwnerDispatcher = (OwnerDispatcher) FieldUtils.readField(GravitinoEnv.getInstance(), "ownerDispatcher", true); FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); hookDispatcher = new PolicyHookDispatcher(mockDispatcher); } @AfterEach public void tearDown() throws IllegalAccessException { FieldUtils.writeField( GravitinoEnv.getInstance(), "ownerDispatcher", originalOwnerDispatcher, true); ``` ########## core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java: ########## @@ -58,11 +67,74 @@ public static void initialize() throws IOException, IllegalAccessException { catalogManager = Mockito.mock(CatalogManager.class); FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogManager", catalogManager, true); BaseCatalog catalog = Mockito.mock(BaseCatalog.class); + Mockito.when(catalog.capability()).thenReturn(Capability.DEFAULT); + CatalogManager.CatalogWrapper catalogWrapper = + Mockito.mock(CatalogManager.CatalogWrapper.class); + Mockito.when(catalogWrapper.catalog()).thenReturn(catalog); + Mockito.when(catalogWrapper.capabilities()).thenReturn(Capability.DEFAULT); Mockito.when(catalogManager.loadCatalog(any())).thenReturn(catalog); + Mockito.when(catalogManager.loadCatalogAndWrap(any())).thenReturn(catalogWrapper); authorizationPlugin = Mockito.mock(AuthorizationPlugin.class); Mockito.when(catalog.getAuthorizationPlugin()).thenReturn(authorizationPlugin); } + @Test + public void testCreateTopicSetsOwnerWithNormalizedIdentifier() throws Exception { + // Self-contained: use a fresh hook with a directly-mocked TopicDispatcher and a case- + // insensitive catalog so we can verify the helper passes a normalized ident to setOwner. + CatalogManager savedCatalogManager = GravitinoEnv.getInstance().catalogManager(); + OwnerDispatcher savedOwnerDispatcher = GravitinoEnv.getInstance().ownerDispatcher(); + + CatalogManager mockCatalogManager = Mockito.mock(CatalogManager.class); + CatalogManager.CatalogWrapper mockWrapper = Mockito.mock(CatalogManager.CatalogWrapper.class); + Mockito.when(mockWrapper.capabilities()).thenReturn(new CaseInsensitiveCapability()); + Mockito.when(mockCatalogManager.loadCatalogAndWrap(any())).thenReturn(mockWrapper); + + OwnerDispatcher mockOwnerDispatcher = Mockito.mock(OwnerDispatcher.class); + TopicDispatcher mockTopicDispatcher = Mockito.mock(TopicDispatcher.class); + Mockito.when(mockTopicDispatcher.createTopic(any(), any(), any(), any())) + .thenReturn(Mockito.mock(Topic.class)); + + FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogManager", mockCatalogManager, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + + try { + TopicHookDispatcher localHook = new TopicHookDispatcher(mockTopicDispatcher); + NameIdentifier ident = NameIdentifier.of(metalake, catalog, "schema_norm", "MY_TOPIC"); + localHook.createTopic(ident, "comment", null, ImmutableMap.of()); + + ArgumentCaptor<MetadataObject> captor = ArgumentCaptor.forClass(MetadataObject.class); + Mockito.verify(mockOwnerDispatcher) + .setOwner(eq(metalake), captor.capture(), any(), eq(Owner.Type.USER)); + Assertions.assertEquals("my_topic", captor.getValue().name()); + } finally { + FieldUtils.writeField( + GravitinoEnv.getInstance(), "catalogManager", savedCatalogManager, true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "ownerDispatcher", savedOwnerDispatcher, true); + } + } + + @Test + public void testCreateTopicSucceedsEvenIfSetOwnerFails() throws IllegalAccessException { + OwnerDispatcher mockOwnerDispatcher = Mockito.mock(OwnerDispatcher.class); + Mockito.doThrow(new RuntimeException("Set owner failed")) + .when(mockOwnerDispatcher) + .setOwner(any(), any(), any(), any()); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + + try { + Namespace topicNs = Namespace.of(metalake, catalog, "schema_owner_fail"); + Map<String, String> props = ImmutableMap.of("k1", "v1", "k2", "v2"); + schemaHookDispatcher.createSchema(NameIdentifier.of(topicNs.levels()), "comment", props); + + NameIdentifier topicIdent = NameIdentifier.of(topicNs, "topic_owner_fail"); + topicHookDispatcher.createTopic(topicIdent, "comment", null, props); + } finally { + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", null, true); + } Review Comment: This test overwrites `GravitinoEnv.ownerDispatcher` but resets it to `null` in `finally`. Please save the pre-test value and restore it afterward (instead of assuming it should be `null`) to avoid singleton state leakage across tests. ########## core/src/main/java/org/apache/gravitino/GravitinoEnv.java: ########## @@ -593,29 +597,32 @@ private void initGravitinoServerComponents() { FilesetOperationDispatcher filesetOperationDispatcher = new FilesetOperationDispatcher(catalogManager, entityStore, idGenerator); - FilesetHookDispatcher filesetHookDispatcher = - new FilesetHookDispatcher(filesetOperationDispatcher); FilesetNormalizeDispatcher filesetNormalizeDispatcher = - new FilesetNormalizeDispatcher(filesetHookDispatcher, catalogManager); - this.filesetDispatcher = new FilesetEventDispatcher(eventBus, filesetNormalizeDispatcher); + new FilesetNormalizeDispatcher(filesetOperationDispatcher, catalogManager); + FilesetEventDispatcher filesetEventDispatcher = + new FilesetEventDispatcher(eventBus, filesetNormalizeDispatcher); + this.filesetDispatcher = new FilesetHookDispatcher(filesetEventDispatcher); TopicOperationDispatcher topicOperationDispatcher = new TopicOperationDispatcher(catalogManager, entityStore, idGenerator); - TopicHookDispatcher topicHookDispatcher = new TopicHookDispatcher(topicOperationDispatcher); TopicNormalizeDispatcher topicNormalizeDispatcher = - new TopicNormalizeDispatcher(topicHookDispatcher, catalogManager); - this.topicDispatcher = new TopicEventDispatcher(eventBus, topicNormalizeDispatcher); + new TopicNormalizeDispatcher(topicOperationDispatcher, catalogManager); + TopicEventDispatcher topicEventDispatcher = + new TopicEventDispatcher(eventBus, topicNormalizeDispatcher); + this.topicDispatcher = new TopicHookDispatcher(topicEventDispatcher); ModelOperationDispatcher modelOperationDispatcher = new ModelOperationDispatcher(catalogManager, entityStore, idGenerator); - ModelHookDispatcher modelHookDispatcher = new ModelHookDispatcher(modelOperationDispatcher); ModelNormalizeDispatcher modelNormalizeDispatcher = - new ModelNormalizeDispatcher(modelHookDispatcher, catalogManager); - this.modelDispatcher = new ModelEventDispatcher(eventBus, modelNormalizeDispatcher); - - // TODO: Add FunctionHookDispatcher when needed - // The operation chain is: - // FunctionEventDispatcher -> FunctionNormalizeDispatcher -> FunctionOperationDispatcher + new ModelNormalizeDispatcher(modelOperationDispatcher, catalogManager); + ModelEventDispatcher modelEventDispatcher = + new ModelEventDispatcher(eventBus, modelNormalizeDispatcher); + this.modelDispatcher = new ModelHookDispatcher(modelEventDispatcher); + + // TODO: Add FunctionHookDispatcher and FunctionEventDispatcher when needed + // The operation chain should be: + // FunctionHookDispatcher -> FunctionNormalizeDispatcher -> FunctionEventDispatcher -> Review Comment: The Function dispatcher wiring comment is inconsistent with the actual chain created below (the code wires `FunctionEventDispatcher -> FunctionNormalizeDispatcher -> FunctionOperationDispatcher`). If/when a FunctionHookDispatcher is added, it should follow the same `Hook -> Event -> Normalize -> Manager/Operation` order used above. Please update the comment to match the real/current chain (or the intended future chain) to avoid misleading future changes. ```suggestion // TODO: Add FunctionHookDispatcher when needed. // The current operation chain is: // FunctionEventDispatcher -> FunctionNormalizeDispatcher -> FunctionOperationDispatcher // If a FunctionHookDispatcher is added in the future, it should wrap the current chain as: // FunctionHookDispatcher -> FunctionEventDispatcher -> FunctionNormalizeDispatcher -> ``` ########## core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java: ########## @@ -80,11 +88,81 @@ public static void initialize() throws IOException, IllegalAccessException { catalogManager = Mockito.mock(CatalogManager.class); FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogManager", catalogManager, true); BaseCatalog catalog = Mockito.mock(BaseCatalog.class); + Mockito.when(catalog.capability()).thenReturn(Capability.DEFAULT); + CatalogManager.CatalogWrapper catalogWrapper = + Mockito.mock(CatalogManager.CatalogWrapper.class); + Mockito.when(catalogWrapper.catalog()).thenReturn(catalog); + Mockito.when(catalogWrapper.capabilities()).thenReturn(Capability.DEFAULT); Mockito.when(catalogManager.loadCatalog(any())).thenReturn(catalog); + Mockito.when(catalogManager.loadCatalogAndWrap(any())).thenReturn(catalogWrapper); authorizationPlugin = Mockito.mock(AuthorizationPlugin.class); Mockito.when(catalog.getAuthorizationPlugin()).thenReturn(authorizationPlugin); } + @Test + public void testCreateFilesetSetsOwnerWithNormalizedIdentifier() throws Exception { + // Self-contained: use a fresh hook with a directly-mocked FilesetDispatcher and a case- + // insensitive catalog so we can verify the helper passes a normalized ident to setOwner. + CatalogManager savedCatalogManager = GravitinoEnv.getInstance().catalogManager(); + OwnerDispatcher savedOwnerDispatcher = GravitinoEnv.getInstance().ownerDispatcher(); + + CatalogManager mockCatalogManager = Mockito.mock(CatalogManager.class); + CatalogManager.CatalogWrapper mockWrapper = Mockito.mock(CatalogManager.CatalogWrapper.class); + Mockito.when(mockWrapper.capabilities()).thenReturn(new CaseInsensitiveCapability()); + Mockito.when(mockCatalogManager.loadCatalogAndWrap(any())).thenReturn(mockWrapper); + + OwnerDispatcher mockOwnerDispatcher = Mockito.mock(OwnerDispatcher.class); + FilesetDispatcher mockFilesetDispatcher = Mockito.mock(FilesetDispatcher.class); + Mockito.when( + mockFilesetDispatcher.createMultipleLocationFileset(any(), any(), any(), any(), any())) + .thenReturn(Mockito.mock(Fileset.class)); + + FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogManager", mockCatalogManager, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + + try { + FilesetHookDispatcher localHook = new FilesetHookDispatcher(mockFilesetDispatcher); + NameIdentifier ident = NameIdentifier.of(metalake, catalog, "schema_norm", "MY_FILESET"); + localHook.createMultipleLocationFileset( + ident, + "comment", + Fileset.Type.MANAGED, + ImmutableMap.of("default", "/tmp/loc"), + ImmutableMap.of()); + + ArgumentCaptor<MetadataObject> captor = ArgumentCaptor.forClass(MetadataObject.class); + Mockito.verify(mockOwnerDispatcher) + .setOwner(eq(metalake), captor.capture(), any(), eq(Owner.Type.USER)); + Assertions.assertEquals("my_fileset", captor.getValue().name()); + } finally { + FieldUtils.writeField( + GravitinoEnv.getInstance(), "catalogManager", savedCatalogManager, true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "ownerDispatcher", savedOwnerDispatcher, true); + } + } + + @Test + public void testCreateFilesetSucceedsEvenIfSetOwnerFails() throws IllegalAccessException { + OwnerDispatcher mockOwnerDispatcher = Mockito.mock(OwnerDispatcher.class); + Mockito.doThrow(new RuntimeException("Set owner failed")) + .when(mockOwnerDispatcher) + .setOwner(any(), any(), any(), any()); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + + try { + Namespace filesetNs = Namespace.of(metalake, catalog, "schema_owner_fail"); + Map<String, String> props = ImmutableMap.of("k1", "v1", "k2", "v2"); + schemaHookDispatcher.createSchema(NameIdentifier.of(filesetNs.levels()), "comment", props); + + NameIdentifier filesetIdent = NameIdentifier.of(filesetNs, "fileset_owner_fail"); + filesetHookDispatcher.createFileset( + filesetIdent, "comment", Fileset.Type.MANAGED, "fileset_owner", props); + } finally { + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", null, true); + } Review Comment: This test sets `GravitinoEnv.ownerDispatcher` to a mock but then resets it to `null` in the `finally` block. Please capture and restore the original ownerDispatcher value instead, so this test doesn't accidentally break other tests that rely on a pre-configured `GravitinoEnv`. ########## core/src/test/java/org/apache/gravitino/hook/TestJobHookDispatcher.java: ########## @@ -0,0 +1,92 @@ +/* + * 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.hook; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.Collections; +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.authorization.OwnerDispatcher; +import org.apache.gravitino.job.JobOperationDispatcher; +import org.apache.gravitino.meta.JobEntity; +import org.apache.gravitino.meta.JobTemplateEntity; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class TestJobHookDispatcher { + + private JobHookDispatcher hookDispatcher; + private JobOperationDispatcher mockDispatcher; + private OwnerDispatcher mockOwnerDispatcher; + + @BeforeEach + public void setUp() throws IllegalAccessException { + mockDispatcher = mock(JobOperationDispatcher.class); + mockOwnerDispatcher = mock(OwnerDispatcher.class); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + hookDispatcher = new JobHookDispatcher(mockDispatcher); + } + + @AfterEach + public void tearDown() throws IllegalAccessException { + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", null, true); + } Review Comment: `setUp` overwrites `GravitinoEnv.ownerDispatcher`, but `tearDown` sets it to `null` rather than restoring the previous value. Please save the original value in `setUp` and restore it in `tearDown` (as done in `TestCatalogHookDispatcher`) to avoid leaking state across tests. ########## core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.hook; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.Collections; +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Schema; +import org.apache.gravitino.authorization.Owner; +import org.apache.gravitino.authorization.OwnerDispatcher; +import org.apache.gravitino.catalog.CatalogManager; +import org.apache.gravitino.catalog.SchemaDispatcher; +import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.connector.capability.CapabilityResult; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +public class TestSchemaHookDispatcher { + + private SchemaHookDispatcher hookDispatcher; + private SchemaDispatcher mockDispatcher; + private OwnerDispatcher mockOwnerDispatcher; + private CatalogManager mockCatalogManager; + private CatalogManager.CatalogWrapper mockCatalogWrapper; + + @BeforeEach + public void setUp() throws Exception { + mockDispatcher = mock(SchemaDispatcher.class); + mockOwnerDispatcher = mock(OwnerDispatcher.class); + mockCatalogManager = mock(CatalogManager.class); + mockCatalogWrapper = mock(CatalogManager.CatalogWrapper.class); + when(mockCatalogManager.loadCatalogAndWrap(any())).thenReturn(mockCatalogWrapper); + when(mockCatalogWrapper.capabilities()).thenReturn(Capability.DEFAULT); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogManager", mockCatalogManager, true); + hookDispatcher = new SchemaHookDispatcher(mockDispatcher); + } + + @AfterEach + public void tearDown() throws IllegalAccessException { + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", null, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogManager", null, true); + } Review Comment: This test replaces `GravitinoEnv.ownerDispatcher` and `GravitinoEnv.catalogManager` but teardown unconditionally sets both fields to `null`. Please save the original field values and restore them in `@AfterEach` to keep the singleton environment isolated across test classes (matching the pattern used in `TestCatalogHookDispatcher`). ########## core/src/test/java/org/apache/gravitino/hook/TestAccessControlHookDispatcher.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.hook; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.Collections; +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.authorization.AccessControlDispatcher; +import org.apache.gravitino.authorization.OwnerDispatcher; +import org.apache.gravitino.authorization.Role; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class TestAccessControlHookDispatcher { + + private AccessControlHookDispatcher hookDispatcher; + private AccessControlDispatcher mockDispatcher; + private OwnerDispatcher mockOwnerDispatcher; + + @BeforeEach + public void setUp() throws IllegalAccessException { + mockDispatcher = mock(AccessControlDispatcher.class); + mockOwnerDispatcher = mock(OwnerDispatcher.class); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + hookDispatcher = new AccessControlHookDispatcher(mockDispatcher); + } + + @AfterEach + public void tearDown() throws IllegalAccessException { + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", null, true); Review Comment: This test modifies the global `GravitinoEnv` singleton (`ownerDispatcher`) but teardown resets it to `null` instead of restoring the prior value. Please save the original dispatcher (and restore it in `@AfterEach`) to prevent cross-test interference when other tests have already configured `GravitinoEnv`. ```suggestion private OwnerDispatcher originalOwnerDispatcher; @BeforeEach public void setUp() throws IllegalAccessException { mockDispatcher = mock(AccessControlDispatcher.class); mockOwnerDispatcher = mock(OwnerDispatcher.class); originalOwnerDispatcher = (OwnerDispatcher) FieldUtils.readField(GravitinoEnv.getInstance(), "ownerDispatcher", true); FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); hookDispatcher = new AccessControlHookDispatcher(mockDispatcher); } @AfterEach public void tearDown() throws IllegalAccessException { FieldUtils.writeField( GravitinoEnv.getInstance(), "ownerDispatcher", originalOwnerDispatcher, true); ``` ########## core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java: ########## @@ -180,6 +189,88 @@ public void testDropAuthorizationPrivilege() { }); } + @Test + public void testCreateTableSetsOwnerWithNormalizedIdentifier() throws Exception { + // Self-contained: use a fresh hook with a directly-mocked TableDispatcher and a case- + // insensitive catalog so we can verify the helper passes a normalized ident to setOwner. + CatalogManager savedCatalogManager = GravitinoEnv.getInstance().catalogManager(); + OwnerDispatcher savedOwnerDispatcher = GravitinoEnv.getInstance().ownerDispatcher(); + + CatalogManager mockCatalogManager = Mockito.mock(CatalogManager.class); + CatalogManager.CatalogWrapper mockWrapper = Mockito.mock(CatalogManager.CatalogWrapper.class); + Mockito.when(mockWrapper.capabilities()).thenReturn(new CaseInsensitiveCapability()); + Mockito.when(mockCatalogManager.loadCatalogAndWrap(any())).thenReturn(mockWrapper); + + OwnerDispatcher mockOwnerDispatcher = Mockito.mock(OwnerDispatcher.class); + TableDispatcher mockTableDispatcher = Mockito.mock(TableDispatcher.class); + Mockito.when( + mockTableDispatcher.createTable(any(), any(), any(), any(), any(), any(), any(), any())) + .thenReturn(Mockito.mock(Table.class)); + + FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogManager", mockCatalogManager, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + + try { + TableHookDispatcher localHook = new TableHookDispatcher(mockTableDispatcher); + NameIdentifier ident = NameIdentifier.of(metalake, catalog, "schema_norm", "MY_TABLE"); + localHook.createTable( + ident, + new Column[0], + "comment", + ImmutableMap.of(), + new Transform[0], + Distributions.NONE, + new SortOrder[0], + new Index[0]); + + ArgumentCaptor<MetadataObject> captor = ArgumentCaptor.forClass(MetadataObject.class); + Mockito.verify(mockOwnerDispatcher) + .setOwner(eq(metalake), captor.capture(), any(), eq(Owner.Type.USER)); + Assertions.assertEquals("my_table", captor.getValue().name()); + } finally { + FieldUtils.writeField( + GravitinoEnv.getInstance(), "catalogManager", savedCatalogManager, true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "ownerDispatcher", savedOwnerDispatcher, true); + } + } + + @Test + public void testCreateTableSucceedsEvenIfSetOwnerFails() throws IllegalAccessException { + OwnerDispatcher mockOwnerDispatcher = Mockito.mock(OwnerDispatcher.class); + Mockito.doThrow(new RuntimeException("Set owner failed")) + .when(mockOwnerDispatcher) + .setOwner(any(), any(), any(), any()); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + + try { + Namespace tableNs = Namespace.of(metalake, catalog, "schema_owner_fail"); + Map<String, String> props = ImmutableMap.of("k1", "v1", "k2", "v2"); + schemaHookDispatcher.createSchema(NameIdentifier.of(tableNs.levels()), "comment", props); + + NameIdentifier tableIdent = NameIdentifier.of(tableNs, "table_owner_fail"); + Column[] columns = + new Column[] { + TestColumn.builder() + .withName("col1") + .withPosition(0) + .withType(Types.StringType.get()) + .build() + }; + tableHookDispatcher.createTable( + tableIdent, + columns, + "comment", + props, + new Transform[0], + Distributions.NONE, + new SortOrder[0], + new Index[0]); + } finally { + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", null, true); + } Review Comment: This test overwrites the global `GravitinoEnv.ownerDispatcher` but restores it to `null` in the `finally` block. Please save the original ownerDispatcher before overwriting it and restore that original value afterward (rather than assuming `null`), to prevent leaking singleton state across tests (see `TestCatalogHookDispatcher`’s save/restore pattern). ########## core/src/test/java/org/apache/gravitino/hook/TestMetalakeHookDispatcher.java: ########## @@ -0,0 +1,119 @@ +/* + * 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.hook; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.Collections; +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.Metalake; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.authorization.AccessControlDispatcher; +import org.apache.gravitino.authorization.OwnerDispatcher; +import org.apache.gravitino.exceptions.UserAlreadyExistsException; +import org.apache.gravitino.metalake.MetalakeDispatcher; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class TestMetalakeHookDispatcher { + + private MetalakeHookDispatcher hookDispatcher; + private MetalakeDispatcher mockDispatcher; + private OwnerDispatcher mockOwnerDispatcher; + private AccessControlDispatcher mockAccessControlDispatcher; + + @BeforeEach + public void setUp() throws IllegalAccessException { + mockDispatcher = mock(MetalakeDispatcher.class); + mockOwnerDispatcher = mock(OwnerDispatcher.class); + mockAccessControlDispatcher = mock(AccessControlDispatcher.class); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + FieldUtils.writeField( + GravitinoEnv.getInstance(), "accessControlDispatcher", mockAccessControlDispatcher, true); + hookDispatcher = new MetalakeHookDispatcher(mockDispatcher); + } + + @AfterEach + public void tearDown() throws IllegalAccessException { + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", null, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "accessControlDispatcher", null, true); + } Review Comment: This test class mutates global `GravitinoEnv` fields (`ownerDispatcher`, `accessControlDispatcher`) and teardown resets them to `null`. To avoid polluting global state when other tests have initialized these fields, please capture the original values in `setUp` and restore them in `tearDown` (see `TestCatalogHookDispatcher`’s save/restore approach). ########## core/src/test/java/org/apache/gravitino/hook/TestModelHookDispatcher.java: ########## @@ -0,0 +1,136 @@ +/* + * 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.hook; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.Collections; +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.authorization.Owner; +import org.apache.gravitino.authorization.OwnerDispatcher; +import org.apache.gravitino.catalog.CatalogManager; +import org.apache.gravitino.catalog.ModelDispatcher; +import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.connector.capability.CapabilityResult; +import org.apache.gravitino.model.Model; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +public class TestModelHookDispatcher { + + private ModelHookDispatcher hookDispatcher; + private ModelDispatcher mockDispatcher; + private OwnerDispatcher mockOwnerDispatcher; + private CatalogManager mockCatalogManager; + private CatalogManager.CatalogWrapper mockCatalogWrapper; + + @BeforeEach + public void setUp() throws Exception { + mockDispatcher = mock(ModelDispatcher.class); + mockOwnerDispatcher = mock(OwnerDispatcher.class); + mockCatalogManager = mock(CatalogManager.class); + mockCatalogWrapper = mock(CatalogManager.CatalogWrapper.class); + when(mockCatalogManager.loadCatalogAndWrap(any())).thenReturn(mockCatalogWrapper); + when(mockCatalogWrapper.capabilities()).thenReturn(Capability.DEFAULT); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogManager", mockCatalogManager, true); + hookDispatcher = new ModelHookDispatcher(mockDispatcher); + } + + @AfterEach + public void tearDown() throws IllegalAccessException { + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", null, true); + FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogManager", null, true); + } Review Comment: This test overwrites `GravitinoEnv.ownerDispatcher` and `GravitinoEnv.catalogManager`, but `tearDown` resets them to `null` rather than restoring the prior values. Please capture the original values in `setUp` and restore them in `tearDown` to avoid leaking singleton state into other tests. ########## core/src/test/java/org/apache/gravitino/hook/TestTagHookDispatcher.java: ########## @@ -0,0 +1,72 @@ +/* + * 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.hook; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.Collections; +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.authorization.OwnerDispatcher; +import org.apache.gravitino.tag.Tag; +import org.apache.gravitino.tag.TagDispatcher; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class TestTagHookDispatcher { + + private TagHookDispatcher hookDispatcher; + private TagDispatcher mockDispatcher; + private OwnerDispatcher mockOwnerDispatcher; + + @BeforeEach + public void setUp() throws IllegalAccessException { + mockDispatcher = mock(TagDispatcher.class); + mockOwnerDispatcher = mock(OwnerDispatcher.class); + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); + hookDispatcher = new TagHookDispatcher(mockDispatcher); + } + + @AfterEach + public void tearDown() throws IllegalAccessException { + FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", null, true); Review Comment: This test mutates the global `GravitinoEnv` singleton (`ownerDispatcher`) and then resets it to `null` in teardown. To avoid cross-test interference when other tests have already initialized `GravitinoEnv` fields, please save the original value (e.g., via `FieldUtils.readField`) and restore it in `@AfterEach` (see the pattern in `core/src/test/java/org/apache/gravitino/hook/TestCatalogHookDispatcher.java:40-84`). ```suggestion private OwnerDispatcher originalOwnerDispatcher; @BeforeEach public void setUp() throws IllegalAccessException { mockDispatcher = mock(TagDispatcher.class); mockOwnerDispatcher = mock(OwnerDispatcher.class); originalOwnerDispatcher = (OwnerDispatcher) FieldUtils.readField(GravitinoEnv.getInstance(), "ownerDispatcher", true); FieldUtils.writeField(GravitinoEnv.getInstance(), "ownerDispatcher", mockOwnerDispatcher, true); hookDispatcher = new TagHookDispatcher(mockDispatcher); } @AfterEach public void tearDown() throws IllegalAccessException { FieldUtils.writeField( GravitinoEnv.getInstance(), "ownerDispatcher", originalOwnerDispatcher, true); ``` -- 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]
