From cfb01869038065defe50ab53d4d1eda4e6cdee33 Mon Sep 17 00:00:00 2001
From: Gabor Bota <gabor.bota@cloudera.com>
Date: Thu, 28 Mar 2019 15:49:56 +0000
Subject: [PATCH] HADOOP-16186. S3Guard: NPE in
 DynamoDBMetadataStore.lambda$listChildren.

Author:    Gabor Bota
---
 .../fs/s3a/s3guard/DynamoDBMetadataStore.java | 42 ++++++++++++++-----
 .../s3guard/TestDynamoDBMiscOperations.java   | 18 ++++++++
 2 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStore.java b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStore.java
index d2676f70e7..590b9c4902 100644
--- a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStore.java
+++ b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStore.java
@@ -638,22 +638,42 @@ public DirListingMetadata listChildren(final Path path) throws IOException {
             metas.add(meta);
           }
 
+          // Minor race condition here - if the path is deleted between
+          // getting the list of items and the directory metadata we might
+          // get a null in DDBPathMetadata.
           DDBPathMetadata dirPathMeta = get(path);
-          boolean isAuthoritative = false;
-          if(dirPathMeta != null) {
-            isAuthoritative = dirPathMeta.isAuthoritativeDir();
-          }
 
-          LOG.trace("Listing table {} in region {} for {} returning {}",
-              tableName, region, path, metas);
-
-          return (metas.isEmpty() && dirPathMeta == null)
-              ? null
-              : new DirListingMetadata(path, metas, isAuthoritative,
-              dirPathMeta.getLastUpdated());
+          return getDirListingMetadataFromDirMetaAndList(path, metas,
+              dirPathMeta);
         });
   }
 
+  DirListingMetadata getDirListingMetadataFromDirMetaAndList(Path path,
+      List<PathMetadata> metas, DDBPathMetadata dirPathMeta) {
+    boolean isAuthoritative = false;
+    if (dirPathMeta != null) {
+      isAuthoritative = dirPathMeta.isAuthoritativeDir();
+    }
+
+    LOG.trace("Listing table {} in region {} for {} returning {}",
+        tableName, region, path, metas);
+
+    if (!metas.isEmpty() && dirPathMeta == null) {
+      // We handle this case as the directory is deleted.
+      LOG.warn("Directory marker is deleted, but the list of the directory "
+          + "elements is not empty: {}. This case is handled as if the "
+          + "directory was deleted.", metas);
+      return null;
+    }
+
+    if(metas.isEmpty() && dirPathMeta == null) {
+      return null;
+    }
+
+    return new DirListingMetadata(path, metas, isAuthoritative,
+        dirPathMeta.getLastUpdated());
+  }
+
   /**
    * build the list of all parent entries.
    * @param pathsToCreate paths to create
diff --git a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/TestDynamoDBMiscOperations.java b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/TestDynamoDBMiscOperations.java
index 03be39f7df..fc0f8940de 100644
--- a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/TestDynamoDBMiscOperations.java
+++ b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/s3guard/TestDynamoDBMiscOperations.java
@@ -20,6 +20,7 @@
 
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.util.List;
 
 import com.amazonaws.services.dynamodbv2.model.ResourceNotFoundException;
 import com.amazonaws.waiters.WaiterTimedOutException;
@@ -27,6 +28,10 @@
 
 import org.apache.hadoop.fs.s3a.AWSClientIOException;
 import org.apache.hadoop.test.HadoopTestBase;
+import org.apache.hadoop.fs.Path;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 import static org.apache.hadoop.fs.s3a.s3guard.DynamoDBMetadataStore.translateTableWaitFailure;
 import static org.apache.hadoop.test.LambdaTestUtils.intercept;
@@ -91,4 +96,17 @@ public void testTranslateWrappedOtherException() throws Throwable {
     assertEquals(e, ex.getCause());
   }
 
+  @Test
+  public void testInnerListChildrenDirectoryNpe() throws Exception {
+    DynamoDBMetadataStore ddbms = new DynamoDBMetadataStore();
+    Path p = mock(Path.class);
+    List<PathMetadata> metas = mock(List.class);
+
+    when(metas.isEmpty()).thenReturn(false);
+    DDBPathMetadata dirPathMeta = null;
+
+    assertNull("The return value should be null.",
+        ddbms.getDirListingMetadataFromDirMetaAndList(p, metas, dirPathMeta));
+  }
+
 }