jerryshao commented on code in PR #11658: URL: https://github.com/apache/gravitino/pull/11658#discussion_r3418265718
########## 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") Review Comment: This test uses an in-memory H2 backend and doesn't actually need a Docker container. Tagging it `gravitino-docker-test` means it's gated behind `-PskipDockerTests=false`, excluding it from standard CI runs that could otherwise exercise this fix. Consider a lighter IT tag (or whatever marker runs the embedded-server ITs without Docker). ########## core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java: ########## @@ -139,6 +142,62 @@ public static NameIdentifier toEntityIdent(String metalakeName, MetadataObject m } } + /** + * Returns the ancestor metadata objects from which the given metadata object inherits tags and + * policies, ordered from the nearest ancestor to the outermost (the metadata object itself is not + * included). + * + * <p>This is similar to repeatedly calling {@link MetadataObjects#parent(MetadataObject)}, but it + * additionally expands hierarchical (multi-level) schemas: a schema name such as {@code a:b:c} + * (using the configured schema separator) has the intermediate schemas {@code a:b} and {@code a} + * as ancestors. Without this expansion, inheritance would jump directly from {@code a:b:c} to the + * catalog and skip the intermediate parent schemas, so tags/policies assigned to a parent schema + * would not be inherited by its child schemas and the objects within them. + * + * @param object The metadata object + * @return The ancestor metadata objects, nearest first + */ + public static List<MetadataObject> getParentMetadataObjects(MetadataObject object) { Review Comment: The public overload resolves the separator by calling `HierarchicalSchemaUtil.schemaSeparator()`, which reads `GravitinoEnv.getInstance().config()`. When `config()` is null (e.g., in unit tests that don't boot the server), `schemaSeparator()` silently falls back to the default `":"`. The new tests in `TestMetadataObjectTagOperations` and `TestMetadataObjectPolicyOperations` rely on this fallback: they use `":"` in schema names and call the public overload, which works only because the default matches. Either add a comment here linking to `schemaSeparator()`'s null-safe contract, or have the REST-layer unit tests call the package-private overload with an explicit `":"` to make the dependency visible. ########## 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"); + catalogProperties.put("jdbc-initialize", "true"); + catalog = + metalake.createCatalog( + CATALOG, Catalog.Type.RELATIONAL, "lakehouse-iceberg", "comment", catalogProperties); + + // Creating "A:B:C" auto-creates the parent chain "A" and "A:B". + catalog.asSchemas().createSchema(SCHEMA_ABC, "hierarchical schema", new HashMap<>()); + Assertions.assertEquals(SCHEMA_AB, catalog.asSchemas().loadSchema(SCHEMA_AB).name()); + Assertions.assertEquals(ROOT_A, catalog.asSchemas().loadSchema(ROOT_A).name()); + } + + @AfterAll + public void tearDown() { Review Comment: Missing `super.stopIntegrationTest()` looks like a lifecycle bug but isn't: JUnit 5 invokes `@AfterAll` methods from both the subclass and the parent class, so `BaseIT.stopIntegrationTest()` (which stops `miniGravitino` and clears `customConfigs`) still runs. Setting `client = null` here prevents the parent from double-closing it. Worth a short comment here so future readers don't add the super call thinking it was accidentally omitted. ########## server/src/test/java/org/apache/gravitino/server/web/rest/TestMetadataObjectTagOperations.java: ########## @@ -380,6 +380,60 @@ public void testListTagsForObject() { Assertions.assertFalse(resultTags8.get("tag0").inherited().get()); } + @Test Review Comment: The mocked objects (`schemaA`, `schemaB`, `schemaC`) are constructed with `":"` as separator. `MetadataObjectUtil.getParentMetadataObjects(object)` (the public overload used in the production code under test) resolves the separator from `GravitinoEnv`, which is null here, so it defaults to `":"`. The test passes incidentally because the default matches. See the note on `MetadataObjectUtil.getParentMetadataObjects(MetadataObject)` — same concern applies to the policy-operations sibling test. -- 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]
