(gravitino) branch main updated: [#8198] Improvement(trino-connector): prevent NPE in GravitinoConnector (#8246)

[diqiu50]

This is an automated email from the ASF dual-hosted git repository.

diqiu50 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git


The following commit(s) were added to refs/heads/main by this push:
     new eeb0db55e9 [#8198] Improvement(trino-connector): prevent NPE in 
GravitinoConnector (#8246)
eeb0db55e9 is described below

commit eeb0db55e9deb5cb27ff2f334c6d572ebe3c0b4e
Author: Sambhavi Pandey <48976443+pandeysam...@users.noreply.github.com>
AuthorDate: Thu Aug 28 14:20:51 2025 +0530

    [#8198] Improvement(trino-connector): prevent NPE in GravitinoConnector 
(#8246)
    
    <!--
    1. Title: [#<issue>] <type>(<scope>): <subject>
       Examples:
         - "[#123] feat(operator): support xxx"
         - "[#233] fix: check null before access result in xxx"
         - "[MINOR] refactor: fix typo in variable name"
         - "[MINOR] docs: fix typo in README"
         - "[#255] test: fix flaky test NameOfTheTest"
       Reference: https://www.conventionalcommits.org/en/v1.0.0/
    2. If the PR is unfinished, please mark this PR as draft.
    -->
    
    ### What changes were proposed in this pull request?
    To fix possible NPEs in GravitinoConnector.java, added
    Preconditions.checkNotNull for internalConnector and transaction handle
    in beginTransaction method before their use.
    
    
    ### Why are the changes needed?
    An Improvement to prevent possible NPEs in GravitinoConnector.java
    
    Fix: #8198
    
    ### Does this PR introduce _any_ user-facing change?
    
    No
    
    ### How was this patch tested?
    Unit testing
    
    ---------
    
    Co-authored-by: Justin Mclean <jus...@classsoftware.com>
    Co-authored-by: Yuhui <h...@datastrato.com>
---
 .../trino/connector/GravitinoConnector.java        |  3 +-
 .../TestGravitinoConnectorNullChecks.java          | 56 ++++++++++++++++++++++
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git 
a/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java
 
b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java
index 069c795dd5..2bd489ec6d 100644
--- 
a/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java
+++ 
b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java
@@ -65,10 +65,11 @@ public class GravitinoConnector implements Connector {
   public ConnectorTransactionHandle beginTransaction(
       IsolationLevel isolationLevel, boolean readOnly, boolean autoCommit) {
     Connector internalConnector = 
catalogConnectorContext.getInternalConnector();
+    Preconditions.checkNotNull(internalConnector, "Internal connector must not 
be null");
 
     ConnectorTransactionHandle internalTransactionHandler =
         internalConnector.beginTransaction(isolationLevel, readOnly, 
autoCommit);
-    Preconditions.checkNotNull(internalConnector);
+    Preconditions.checkNotNull(internalTransactionHandler, "Transaction 
handler must not be null");
 
     return new GravitinoTransactionHandle(internalTransactionHandler);
   }
diff --git 
a/trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnectorNullChecks.java
 
b/trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnectorNullChecks.java
new file mode 100644
index 0000000000..d11d9cbfac
--- /dev/null
+++ 
b/trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/TestGravitinoConnectorNullChecks.java
@@ -0,0 +1,56 @@
+/*
+ * 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 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 static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import io.trino.spi.connector.Connector;
+import io.trino.spi.transaction.IsolationLevel;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.trino.connector.catalog.CatalogConnectorContext;
+import org.junit.jupiter.api.Test;
+
+class TestGravitinoConnectorNullChecks {
+  @Test
+  void testBeginTransactionThrowsIfInternalConnectorIsNull() {
+    CatalogConnectorContext mockContext = mock(CatalogConnectorContext.class);
+    when(mockContext.getInternalConnector()).thenReturn(null);
+    GravitinoConnector connector = new 
GravitinoConnector(mock(NameIdentifier.class), mockContext);
+    assertThrows(
+        NullPointerException.class,
+        () -> connector.beginTransaction(IsolationLevel.READ_COMMITTED, true, 
true));
+  }
+
+  @Test
+  void testBeginTransactionThrowsIfInternalTransactionHandleIsNull() {
+    CatalogConnectorContext mockContext = mock(CatalogConnectorContext.class);
+    Connector mockInternalConnector = mock(Connector.class);
+    when(mockContext.getInternalConnector()).thenReturn(mockInternalConnector);
+    when(mockInternalConnector.beginTransaction(any(), anyBoolean(), 
anyBoolean()))
+        .thenReturn(null);
+    GravitinoConnector connector = new 
GravitinoConnector(mock(NameIdentifier.class), mockContext);
+    assertThrows(
+        NullPointerException.class,
+        () -> connector.beginTransaction(IsolationLevel.READ_COMMITTED, true, 
true));
+  }
+}