Copilot commented on code in PR #11658: URL: https://github.com/apache/gravitino/pull/11658#discussion_r3418293460
########## clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/HierarchicalSchemaTagPolicyIT.java: ########## @@ -0,0 +1,231 @@ +/* + * 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.client.integration.test; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import org.apache.gravitino.Catalog; +import org.apache.gravitino.Configs; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.Schema; +import org.apache.gravitino.client.GravitinoMetalake; +import org.apache.gravitino.integration.test.util.BaseIT; +import org.apache.gravitino.integration.test.util.GravitinoITUtils; +import org.apache.gravitino.policy.Policy; +import org.apache.gravitino.policy.PolicyContent; +import org.apache.gravitino.policy.PolicyContents; +import org.apache.gravitino.tag.Tag; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** + * Integration test that verifies tags and policies assigned to a parent schema are inherited along + * a multi-level (hierarchical) schema hierarchy. + * + * <p>A hierarchical schema {@code A:B:C} (using the configured {@code ":"} separator) has the + * intermediate schemas {@code A:B} and {@code A} as ancestors. Listing the tags/policies of {@code + * A:B:C} must therefore include the tags/policies assigned to {@code A:B}, {@code A} and the + * catalog as inherited (see issue #11639). The catalog is an Iceberg catalog backed by an in-memory + * H2 database, because {@code ":"} hierarchical schema names are only supported by Iceberg catalogs + * accessed through the Gravitino REST server with a configured schema separator. + */ [email protected]("gravitino-docker-test") +public class HierarchicalSchemaTagPolicyIT extends BaseIT { Review Comment: This test is not Docker-dependent (it uses embedded server + in-memory H2 and explicitly notes it needs no extra container), but it is tagged as `gravitino-docker-test`. The Gradle integration-test configuration excludes this tag when Docker isn’t available, so this test will be silently skipped in common environments. Remove the Docker tag (or use a non-docker tag) so the test actually runs by default. ########## clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/HierarchicalSchemaTagPolicyIT.java: ########## @@ -0,0 +1,231 @@ +/* + * 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.client.integration.test; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import org.apache.gravitino.Catalog; +import org.apache.gravitino.Configs; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.Schema; +import org.apache.gravitino.client.GravitinoMetalake; +import org.apache.gravitino.integration.test.util.BaseIT; +import org.apache.gravitino.integration.test.util.GravitinoITUtils; +import org.apache.gravitino.policy.Policy; +import org.apache.gravitino.policy.PolicyContent; +import org.apache.gravitino.policy.PolicyContents; +import org.apache.gravitino.tag.Tag; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** + * Integration test that verifies tags and policies assigned to a parent schema are inherited along + * a multi-level (hierarchical) schema hierarchy. + * + * <p>A hierarchical schema {@code A:B:C} (using the configured {@code ":"} separator) has the + * intermediate schemas {@code A:B} and {@code A} as ancestors. Listing the tags/policies of {@code + * A:B:C} must therefore include the tags/policies assigned to {@code A:B}, {@code A} and the + * catalog as inherited (see issue #11639). The catalog is an Iceberg catalog backed by an in-memory + * H2 database, because {@code ":"} hierarchical schema names are only supported by Iceberg catalogs + * accessed through the Gravitino REST server with a configured schema separator. + */ [email protected]("gravitino-docker-test") +public class HierarchicalSchemaTagPolicyIT extends BaseIT { + + private static final String METALAKE = + GravitinoITUtils.genRandomName("hierarchical_tag_policy_metalake"); + private static final String CATALOG = "hierarchical_tag_policy_catalog"; + + private static final String ROOT_A = "A"; + private static final String SCHEMA_AB = "A:B"; + private static final String SCHEMA_ABC = "A:B:C"; + + private static GravitinoMetalake metalake; + private static Catalog catalog; + + @BeforeAll + @Override + public void startIntegrationTest() throws Exception { + // Configure the hierarchical schema separator before the server starts. + customConfigs.put(Configs.SCHEMA_SEPARATOR.getKey(), ":"); + super.startIntegrationTest(); + + metalake = client.createMetalake(METALAKE, "comment", new HashMap<>()); + + // Create an Iceberg catalog backed by an in-memory H2 database; it is lightweight, needs no + // extra container, and supports ':' hierarchical schema names. H2 is NOT a supported + // production Iceberg backend. + Map<String, String> catalogProperties = new HashMap<>(); + catalogProperties.put("catalog-backend", "jdbc"); + catalogProperties.put("warehouse", "/tmp/gravitino-it-hierarchical-tag-policy"); + catalogProperties.put( + "uri", "jdbc:h2:mem:gravitino-it-hierarchical-tag-policy;DB_CLOSE_DELAY=-1;MODE=MYSQL"); + catalogProperties.put("jdbc-driver", "org.h2.Driver"); Review Comment: The hard-coded warehouse path and H2 in-memory DB name are shared across runs (and across parallel test execution), which can cause interference/flakiness when multiple builds run on the same host/worker. Make these identifiers unique per test run (for example, suffix them with the generated METALAKE name). ########## server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectPolicyOperations.java: ########## @@ -110,11 +111,13 @@ public Response getPolicyForObject( Optional<PolicyDTO> policyDTO = policyEntity.map(t -> PolicyOperations.toDTO(t, Optional.of(false))); - MetadataObject parentObject = MetadataObjects.parent(object); - while (!policyEntity.isPresent() && parentObject != null) { + for (MetadataObject parentObject : + MetadataObjectUtil.getParentMetadataObjects(object)) { + if (policyEntity.isPresent()) { + break; + } Review Comment: The ancestor-walk logic for GET /policies/{policy} was changed to include intermediate hierarchical schema parents, but there is no test covering the GET-by-name endpoint under a hierarchical schema (only list endpoints are covered). Add a unit test that asserts GET returns a policy inherited from `a:b`/`a` when requesting an object under `a:b:c`. ########## server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectTagOperations.java: ########## @@ -114,11 +115,13 @@ public Response getTagForObject( Optional<Tag> tag = getTagForObject(metalake, object, tagName); Optional<TagDTO> tagDTO = tag.map(t -> DTOConverters.toDTO(t, Optional.of(false))); - MetadataObject parentObject = MetadataObjects.parent(object); - while (!tag.isPresent() && parentObject != null) { + for (MetadataObject parentObject : + MetadataObjectUtil.getParentMetadataObjects(object)) { + if (tag.isPresent()) { + break; + } Review Comment: The ancestor-walk logic for GET /tags/{tag} was changed to include intermediate hierarchical schema parents, but there is no test covering the GET-by-name endpoint under a hierarchical schema (only list endpoints are covered). Add a unit test that asserts GET returns a tag/present policy inherited from `a:b`/`a` when requesting an object under `a:b:c`. -- 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]
