Copilot commented on code in PR #6812: URL: https://github.com/apache/hive/pull/6812#discussion_r4075075128
########## iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/ScopedDeleteFileIO.java: ########## @@ -0,0 +1,91 @@ +/* + * 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.iceberg.hive; + +import java.util.Map; +import org.apache.hadoop.fs.Path; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.OutputFile; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A {@link FileIO} decorator used by {@link HiveCatalog#dropTable(org.apache.iceberg.catalog.TableIdentifier, + * boolean)} to fence purge deletions to files under a table's own location, regardless of what a table's + * metadata or manifests actually reference. + * + * <p>This does not implement {@link org.apache.iceberg.io.SupportsBulkOperations} or + * {@link org.apache.iceberg.io.SupportsPrefixOperations} even when the delegate does, so that + * {@code CatalogUtil.dropTableData} is forced to route every deletion through {@link #deleteFile(String)}. + */ +class ScopedDeleteFileIO implements FileIO { + private static final Logger LOG = LoggerFactory.getLogger(ScopedDeleteFileIO.class); + + private final FileIO delegate; + private final String location; + + ScopedDeleteFileIO(FileIO delegate, String location) { + this.delegate = delegate; + this.location = normalize(location); + } + + @Override + public InputFile newInputFile(String path) { + return delegate.newInputFile(path); + } + + @Override + public OutputFile newOutputFile(String path) { + return delegate.newOutputFile(path); + } + + @Override + public void deleteFile(String path) { + if (!isContained(location, normalize(path))) { + LOG.warn("Skipping delete outside table location {}: {}", location, path); + return; + } + delegate.deleteFile(path); + } + + @Override + public Map<String, String> properties() { + return delegate.properties(); + } + + @Override + public void initialize(Map<String, String> properties) { + delegate.initialize(properties); + } + + @Override + public void close() { + delegate.close(); + } + + private static boolean isContained(String root, String candidate) { + return candidate.equals(root) || candidate.startsWith(root.endsWith("/") ? root : root + "/"); + } + + private static String normalize(String location) { + return new Path(location).toUri().normalize().toString(); + } Review Comment: There are now multiple, slightly different containment/normalization implementations in the codebase (this class and `IcebergAuthorizer`). To reduce drift and security regressions over time, consider extracting a shared utility (in an appropriate common module/package) and use it in both places so containment semantics remain consistent. ########## standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/BaseRESTCatalogTests.java: ########## @@ -277,4 +284,56 @@ void testStageCreateTableWithDeniedLocation() { Assertions.assertThrows(ForbiddenException.class, builder::createTransaction); Assertions.assertThrows(NoSuchTableException.class, () -> catalog.loadTable(tableIdentifier)); } + + private static String writeMetadataFile(String directory, String tableLocation) throws IOException { + var metadataLocation = directory + "/v1.metadata.json"; + Files.deleteIfExists(java.nio.file.Path.of(metadataLocation)); + var io = new HadoopFileIO(new Configuration(false)); + var metadata = TableMetadata.newTableMetadata(new Schema(), PartitionSpec.unpartitioned(), tableLocation, + Collections.emptyMap()); + TableMetadataParser.write(metadata, io.newOutputFile(metadataLocation)); + return metadataLocation; + } Review Comment: `Files.deleteIfExists(Path.of(metadataLocation))` assumes `metadataLocation` is a local filesystem path. If `directory` is ever a URI-like location (e.g., `file:/...`), `Path.of(...)` will not reference the intended file and the cleanup will be wrong. Use a URI-safe conversion (e.g., `Paths.get(URI.create(metadataLocation))` for `file:` URIs) or keep the helper consistently in local-path form and add the scheme only when passing to `FileIO`. ########## iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HiveCatalog.java: ########## @@ -241,6 +241,10 @@ public String name() { return name; } + public FileIO io() { Review Comment: Adding a new public accessor for the internal `FileIO` increases the public API surface of `HiveCatalog` and exposes service-level I/O capabilities to any in-process caller. Consider narrowing the exposure via an internal/spi-style interface (e.g., `SupportsFileIO`) implemented by `HiveCatalog`, or otherwise clearly documenting this method as internal-only to avoid unintended usage. ########## iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/ScopedDeleteFileIO.java: ########## @@ -0,0 +1,91 @@ +/* + * 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.iceberg.hive; + +import java.util.Map; +import org.apache.hadoop.fs.Path; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.OutputFile; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A {@link FileIO} decorator used by {@link HiveCatalog#dropTable(org.apache.iceberg.catalog.TableIdentifier, + * boolean)} to fence purge deletions to files under a table's own location, regardless of what a table's + * metadata or manifests actually reference. + * + * <p>This does not implement {@link org.apache.iceberg.io.SupportsBulkOperations} or + * {@link org.apache.iceberg.io.SupportsPrefixOperations} even when the delegate does, so that + * {@code CatalogUtil.dropTableData} is forced to route every deletion through {@link #deleteFile(String)}. + */ +class ScopedDeleteFileIO implements FileIO { + private static final Logger LOG = LoggerFactory.getLogger(ScopedDeleteFileIO.class); + + private final FileIO delegate; + private final String location; + + ScopedDeleteFileIO(FileIO delegate, String location) { + this.delegate = delegate; + this.location = normalize(location); + } + + @Override + public InputFile newInputFile(String path) { + return delegate.newInputFile(path); + } + + @Override + public OutputFile newOutputFile(String path) { + return delegate.newOutputFile(path); + } + + @Override + public void deleteFile(String path) { + if (!isContained(location, normalize(path))) { + LOG.warn("Skipping delete outside table location {}: {}", location, path); + return; + } Review Comment: Logging a WARN for every skipped deletion can flood logs during purge if a malicious/corrupt table references many out-of-scope files. Consider reducing verbosity (e.g., DEBUG) or rate-limiting/aggregating skipped-delete logs while still keeping enough signal for operators. ########## standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java: ########## @@ -320,6 +325,10 @@ private LoadTableResponse loadTable(Map<String, String> vars) { private LoadTableResponse registerTable(Map<String, String> vars, Object body) { Namespace namespace = namespaceFromPathVars(vars); RegisterTableRequest request = castRequest(RegisterTableRequest.class, body); + request.validate(); + Map<String, String> namespaceMetadata = asNamespaceCatalog.loadNamespaceMetadata(namespace); + FileIO io = ((HiveCatalog) catalog).io(); + icebergAuthorizer.validateRegisterTable(catalogName, namespace, namespaceMetadata, request, io); return castResponse(LoadTableResponse.class, CatalogHandlers.registerTable(catalog, namespace, request)); Review Comment: The hard cast `((HiveCatalog) catalog)` can throw `ClassCastException` at runtime if this adapter is ever constructed with a non-`HiveCatalog` implementation. Prefer making the adapter depend on a `HiveCatalog`-typed field, or check `instanceof` and fail fast with a clear `IllegalStateException` explaining the required catalog type. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
