Copilot commented on code in PR #13195: URL: https://github.com/apache/gravitino/pull/13195#discussion_r4016522970
########## trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/DeferredConnectorMetadata.java: ########## @@ -0,0 +1,108 @@ +/* + * 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.trino.connector; + +import io.trino.spi.connector.ConnectorMetadata; +import io.trino.spi.connector.ConnectorSession; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.util.Objects; +import java.util.function.Function; +import javax.annotation.Nullable; + +/** Lazily obtains native metadata without authenticating metadata-only management queries. */ +final class DeferredConnectorMetadata { + private final Function<ConnectorSession, ConnectorMetadata> factory; + private final ConnectorSession querySession; + @Nullable private ConnectorMetadata delegate; + @Nullable private ConnectorSession pendingBegin; + private boolean closed; + + private DeferredConnectorMetadata( + ConnectorSession querySession, Function<ConnectorSession, ConnectorMetadata> factory) { + this.querySession = Objects.requireNonNull(querySession, "querySession"); + this.factory = Objects.requireNonNull(factory, "factory"); + } + + static ConnectorMetadata create( + ConnectorSession querySession, Function<ConnectorSession, ConnectorMetadata> factory) { + DeferredConnectorMetadata handler = new DeferredConnectorMetadata(querySession, factory); + return (ConnectorMetadata) + Proxy.newProxyInstance( + ConnectorMetadata.class.getClassLoader(), + new Class<?>[] {ConnectorMetadata.class}, + handler::invoke); + } + + @Nullable + private synchronized Object invoke(Object proxy, Method method, @Nullable Object[] args) + throws Throwable { + if (method.getDeclaringClass() == Object.class) { + switch (method.getName()) { + case "toString": + return "DeferredConnectorMetadata"; + case "hashCode": + return System.identityHashCode(proxy); + case "equals": + return proxy == args[0]; + default: + throw new UnsupportedOperationException(method.getName()); + } + } + if (method.getName().equals("cleanupQuery")) { + closed = true; + pendingBegin = null; + if (delegate == null) { + return null; + } + } else { + if (closed) { + throw new IllegalStateException("Metadata query is already closed"); + } + if (method.getName().equals("beginQuery") && delegate == null) { + pendingBegin = (ConnectorSession) args[0]; + return null; + } Review Comment: `cleanupQuery` is a per-query callback, but setting `closed` here permanently closes this proxy. Trino caches the connector metadata in a catalog transaction and invokes `beginQuery`/`cleanupQuery` for each query, so a second query in the same transaction reaches the `closed` check and fails before delegating. Reset the state at the next `beginQuery` (while still rejecting calls between cleanup and the next begin) and add a regression test that reuses one proxy across two query lifecycles. ########## trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java: ########## @@ -122,6 +121,33 @@ public ConnectorMetadata getMetadata( metadata, catalogConnectorContext.getMetadataAdapter(), internalMetadata); } + /** + * Defers native REST authentication until a data operation needs it. Catalog registration uses a + * password-authenticated management session without a delegated user token. + * + * @param session the authenticated query session + * @param transactionHandle the native transaction handle + * @return metadata that preserves user authentication at first data access + */ + protected ConnectorMetadata getInternalMetadata( + ConnectorSession session, ConnectorTransactionHandle transactionHandle) { + if ("lakehouse-iceberg".equals(catalogConnectorContext.getCatalog().getProvider()) + && "OAUTH2_PASSTHROUGH" + .equalsIgnoreCase( + catalogConnectorContext + .getConfig() + .getIcebergRestCatalogConfig() + .get("iceberg.rest-catalog.security"))) { Review Comment: This check only reads the cluster-level `gravitino.iceberg.rest-catalog.security` map. The effective Iceberg REST configuration also accepts per-catalog `trino.bypass.iceberg.rest-catalog.*` properties, so a catalog with `trino.bypass.iceberg.rest-catalog.security=OAUTH2_PASSTHROUGH` still eagerly calls the native connector here and loses the promised deferral. Resolve the mode using the same cluster-over-catalog precedence as `IcebergCatalogPropertyConverter` and add a regression case for the catalog property. -- 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]
