lasdf1234 commented on code in PR #14482:
URL: https://github.com/apache/gravitino/pull/14482#discussion_r4461566356
##########
server-common/src/main/java/org/apache/gravitino/server/web/filter/BaseMetadataAuthorizationMethodInterceptor.java:
##########
@@ -237,6 +238,45 @@ protected boolean
shouldSkipExpressionEvaluation(AuthorizationTarget target) {
return false;
}
+ /**
+ * Returns whether {@link AuthorizationExpression#secondaryExpression()}
should replace the
+ * primary expression for this request.
+ *
+ * <p>Protocols that understand an {@link ExpressionCondition} (for example
{@link
+ * ExpressionCondition#OVERWRITE}) override this hook. The default never
selects the secondary
+ * expression.
+ *
+ * @param condition secondary expression condition from the method annotation
+ * @param method invoked protocol method
+ * @param parameters invoked method parameters
+ * @param args invoked method arguments
+ * @return {@code true} to evaluate the secondary expression
+ */
+ protected boolean shouldUseSecondaryExpression(
+ ExpressionCondition condition, Method method, Parameter[] parameters,
Object[] args) {
+ return false;
+ }
+
+ /**
+ * Returns the entity type to authorize against when the secondary
expression is selected.
+ *
+ * <p>Defaults to the primary {@link AuthorizationTarget#entityType()}.
Protocols may return a
+ * different type when the secondary expression addresses a deeper object
that a request-specific
+ * handler added to {@code nameIdentifierMap} (for example TABLE for an
overwrite of a named
+ * table).
+ *
+ * @param annotation authorization annotation on the invoked method
+ * @param primaryTarget authorization target resolved from the primary
annotation fields
+ * @param nameIdentifierMap identifiers after request-specific handlers have
run
+ * @return entity type used for secondary expression evaluation
+ */
+ protected Entity.EntityType entityTypeForSecondaryExpression(
Review Comment:
Thanks — done in 2630649f8.
`shouldUseSecondaryExpression()` / `entityTypeForSecondaryExpression()` are
gone. `RegisterTableAuthzHandler` now evaluates the modify-table expression on
overwrite, returns `authorizationCompleted()` for that path, and exposes the
effective target via `entityType(EntityType defaultType)`, matching the Lance
overwrite handler pattern. `ExpressionCondition.OVERWRITE` was also removed.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/RegisterTableSourceResolver.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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.iceberg.service.dispatcher;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import org.apache.gravitino.iceberg.common.ops.IcebergCatalogWrapper;
+import org.apache.iceberg.TableMetadata;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Resolves live catalog tables that share a register-table metadata file.
+ *
+ * <p>This class only discovers identities. Privilege checks belong in the
Iceberg metadata
+ * authorization interceptor handlers.
+ */
+public final class RegisterTableSourceResolver {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(RegisterTableSourceResolver.class);
+
+ private RegisterTableSourceResolver() {}
+
+ /**
+ * Finds tables other than {@code target} that share the given metadata
file, UUID, or table
+ * location.
+ *
+ * @param catalogWrapper catalog to scan
+ * @param target register destination table
+ * @param metadataLocation metadata file from the register request
+ * @param metadata parsed metadata at that location
+ * @return matching live table identities
+ */
+ public static List<TableIdentifier> findSourceTables(
+ IcebergCatalogWrapper catalogWrapper,
+ TableIdentifier target,
+ String metadataLocation,
+ TableMetadata metadata) {
+ List<TableIdentifier> matches = new ArrayList<>();
+ String uuid = metadata.uuid();
+ String tableLocation = normalizePath(metadata.location());
+ String normalizedMeta = normalizePath(metadataLocation);
+ collectMatchingTables(
+ catalogWrapper, Namespace.empty(), target, uuid, tableLocation,
normalizedMeta, matches);
+ return matches;
+ }
+
+ private static void collectMatchingTables(
+ IcebergCatalogWrapper catalogWrapper,
+ Namespace parent,
+ TableIdentifier target,
+ String uuid,
+ String tableLocation,
+ String metadataLocation,
+ List<TableIdentifier> matches) {
+ for (Namespace child : catalogWrapper.listNamespace(parent).namespaces()) {
+ collectMatchingTables(
+ catalogWrapper, child, target, uuid, tableLocation,
metadataLocation, matches);
+ }
+ for (TableIdentifier identifier :
catalogWrapper.listTable(parent).identifiers()) {
Review Comment:
Thanks — fixed in 4cf351bb7.
`collectMatchingTables()` now skips `listTable` at `Namespace.empty()`
(tables always live under a named namespace), so Hive/JDBC catalogs that reject
root table listing no longer fail during source resolution. The unit test was
updated to assert root listing is not invoked.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/RegisterTableSourceResolver.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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.iceberg.service.dispatcher;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import org.apache.gravitino.iceberg.common.ops.IcebergCatalogWrapper;
+import org.apache.iceberg.TableMetadata;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Resolves live catalog tables that share a register-table metadata file.
+ *
+ * <p>This class only discovers identities. Privilege checks belong in the
Iceberg metadata
+ * authorization interceptor handlers.
+ */
+public final class RegisterTableSourceResolver {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(RegisterTableSourceResolver.class);
+
+ private RegisterTableSourceResolver() {}
+
+ /**
+ * Finds tables other than {@code target} that share the given metadata
file, UUID, or table
+ * location.
+ *
+ * @param catalogWrapper catalog to scan
+ * @param target register destination table
+ * @param metadataLocation metadata file from the register request
+ * @param metadata parsed metadata at that location
+ * @return matching live table identities
+ */
+ public static List<TableIdentifier> findSourceTables(
+ IcebergCatalogWrapper catalogWrapper,
+ TableIdentifier target,
+ String metadataLocation,
+ TableMetadata metadata) {
+ List<TableIdentifier> matches = new ArrayList<>();
+ String uuid = metadata.uuid();
+ String tableLocation = normalizePath(metadata.location());
+ String normalizedMeta = normalizePath(metadataLocation);
+ collectMatchingTables(
+ catalogWrapper, Namespace.empty(), target, uuid, tableLocation,
normalizedMeta, matches);
+ return matches;
+ }
+
+ private static void collectMatchingTables(
+ IcebergCatalogWrapper catalogWrapper,
+ Namespace parent,
+ TableIdentifier target,
+ String uuid,
+ String tableLocation,
+ String metadataLocation,
+ List<TableIdentifier> matches) {
+ for (Namespace child : catalogWrapper.listNamespace(parent).namespaces()) {
+ collectMatchingTables(
+ catalogWrapper, child, target, uuid, tableLocation,
metadataLocation, matches);
+ }
+ for (TableIdentifier identifier :
catalogWrapper.listTable(parent).identifiers()) {
+ if (identifier.equals(target)) {
+ continue;
+ }
+ if (matchesSource(catalogWrapper, identifier, uuid, tableLocation,
metadataLocation)) {
+ matches.add(identifier);
+ }
+ }
+ }
+
+ private static boolean matchesSource(
+ IcebergCatalogWrapper catalogWrapper,
+ TableIdentifier identifier,
+ String uuid,
+ String tableLocation,
+ String metadataLocation) {
+ Optional<String> currentMeta =
catalogWrapper.getTableMetadataLocation(identifier);
+ if (currentMeta.isPresent() &&
normalizePath(currentMeta.get()).equals(metadataLocation)) {
+ return true;
+ }
+ try {
+ TableMetadata existing = catalogWrapper.loadTableMetadata(identifier);
+ if (uuid != null && uuid.equals(existing.uuid())) {
+ return true;
+ }
+ return tableLocation != null
+ && !tableLocation.isEmpty()
+ && tableLocation.equals(normalizePath(existing.location()));
+ } catch (NoSuchTableException e) {
+ return false;
+ } catch (RuntimeException e) {
Review Comment:
Thanks — fixed in 2630649f8.
`matchesSource()` now only treats a confirmed `NoSuchTableException` as
“unrelated”. Any other failure while loading current metadata propagates, so an
unreadable live source can no longer be mistaken for orphaned metadata and skip
source-table authorization. Added a unit test for that path.
--
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]
