Fix NPE in UnderFileSystem getLastModifiedDate.getTime

### What changes are proposed in this pull request? Fix #14640 ### Why are the changes needed? Fix NPE in Object stores getLastModifiedDate().getTime() ### Does this PR introduce any user facing changes? No pr-link: #14641 change-id: cid-be6ee11f918d3136de46f95decd966c918c746a7
Alluxio · Dec 9, 2021 · 6d62b8a · 6d62b8a
1 parent 46116a7
commit 6d62b8a
Show file tree

Hide file tree

Showing 14 changed files with 102 additions and 35 deletions.
diff --git a/core/common/src/main/java/alluxio/underfs/ObjectUnderFileSystem.java b/core/common/src/main/java/alluxio/underfs/ObjectUnderFileSystem.java
@@ -101,27 +101,26 @@ protected ObjectUnderFileSystem(AlluxioURI uri, UnderFileSystemConfiguration ufs
    * Information about a single object in object UFS.
    */
   protected class ObjectStatus {
-    private static final String INVALID_CONTENT_HASH = "";
     private static final long INVALID_CONTENT_LENGTH = -1L;
-    private static final long INVALID_MODIFIED_TIME = -1L;
 
     private final String mContentHash;
     private final long mContentLength;
-    private final long mLastModifiedTimeMs;
+    /** Last modified epoch time in ms, or null if it is not available. */
+    private final Long mLastModifiedTimeMs;
     private final String mName;
 
     public ObjectStatus(String name, String contentHash, long contentLength,
-        long lastModifiedTimeMs) {
-      mContentHash = contentHash;
+        @Nullable Long lastModifiedTimeMs) {
+      mContentHash = contentHash == null ? UfsFileStatus.INVALID_CONTENT_HASH : contentHash;
       mContentLength = contentLength;
       mLastModifiedTimeMs = lastModifiedTimeMs;
       mName = name;
     }
 
     public ObjectStatus(String name) {
-      mContentHash = INVALID_CONTENT_HASH;
+      mContentHash = UfsFileStatus.INVALID_CONTENT_HASH;
       mContentLength = INVALID_CONTENT_LENGTH;
-      mLastModifiedTimeMs = INVALID_MODIFIED_TIME;
+      mLastModifiedTimeMs = null;
       mName = name;
     }
 
@@ -146,7 +145,8 @@ public long getContentLength() {
      *
      * @return modification time in milliseconds
      */
-    public long getLastModifiedTimeMs() {
+    @Nullable
+    public Long getLastModifiedTimeMs() {
       return mLastModifiedTimeMs;
     }
 

diff --git a/core/common/src/main/java/alluxio/underfs/UfsFileStatus.java b/core/common/src/main/java/alluxio/underfs/UfsFileStatus.java
@@ -40,8 +40,9 @@ public class UfsFileStatus extends UfsStatus {
    * @param xAttr extended attributes, if any
    * @param blockSize blocksize, -1 if unknown
    */
-  public UfsFileStatus(String name, String contentHash, long contentLength, long lastModifiedTimeMs,
-      String owner, String group, short mode, @Nullable Map<String, byte[]> xAttr, long blockSize) {
+  public UfsFileStatus(String name, String contentHash, long contentLength,
+      @Nullable Long lastModifiedTimeMs, String owner, String group, short mode,
+      @Nullable Map<String, byte[]> xAttr, long blockSize) {
     super(name, false, owner, group, mode, lastModifiedTimeMs, xAttr);
     mContentHash = contentHash;
     mContentLength = contentLength;
@@ -60,8 +61,9 @@ public UfsFileStatus(String name, String contentHash, long contentLength, long l
    * @param mode of the file
    * @param blockSize blocksize, -1 if unknown
    */
-  public UfsFileStatus(String name, String contentHash, long contentLength, long lastModifiedTimeMs,
-      String owner, String group, short mode, long blockSize) {
+  public UfsFileStatus(String name, String contentHash, long contentLength,
+      @Nullable Long lastModifiedTimeMs, String owner, String group,
+      short mode, long blockSize) {
     super(name, false, owner, group, mode, lastModifiedTimeMs, /* xattrs */ null);
     mContentHash = contentHash;
     mContentLength = contentLength;
@@ -84,8 +86,9 @@ public UfsFileStatus(String name, String contentHash, long contentLength, long l
    * @param xAttr extended attributes, if any
    */
   @Deprecated
-  public UfsFileStatus(String name, String contentHash, long contentLength, long lastModifiedTimeMs,
-      String owner, String group, short mode, @Nullable Map<String, byte[]> xAttr) {
+  public UfsFileStatus(String name, String contentHash, long contentLength,
+      @Nullable Long lastModifiedTimeMs, String owner, String group, short mode,
+      @Nullable Map<String, byte[]> xAttr) {
     this(name, contentHash, contentLength, lastModifiedTimeMs, owner, group, mode, xAttr,
         UNKNOWN_BLOCK_SIZE);
   }
@@ -105,8 +108,8 @@ public UfsFileStatus(String name, String contentHash, long contentLength, long l
    * @param mode of the file
    */
   @Deprecated
-  public UfsFileStatus(String name, String contentHash, long contentLength, long lastModifiedTimeMs,
-      String owner, String group, short mode) {
+  public UfsFileStatus(String name, String contentHash, long contentLength,
+      @Nullable Long lastModifiedTimeMs, String owner, String group, short mode) {
     this(name, contentHash, contentLength, lastModifiedTimeMs, owner, group, mode, null,
         UNKNOWN_BLOCK_SIZE);
   }

diff --git a/core/common/src/main/java/alluxio/underfs/UfsStatus.java b/core/common/src/main/java/alluxio/underfs/UfsStatus.java
@@ -49,7 +49,7 @@ public abstract class UfsStatus {
    * @param xAttrs any extended attributes on the inode
    */
   protected UfsStatus(String name, boolean isDirectory, String owner, String group, short mode,
-      Long lastModifiedTimeMs, @Nullable Map<String, byte[]> xAttrs) {
+      @Nullable Long lastModifiedTimeMs, @Nullable Map<String, byte[]> xAttrs) {
     mIsDirectory = isDirectory;
     mName = name;
     mOwner = owner;

diff --git a/core/server/common/src/main/java/alluxio/master/journal/ufs/UfsJournalGarbageCollector.java b/core/server/common/src/main/java/alluxio/master/journal/ufs/UfsJournalGarbageCollector.java
@@ -115,13 +115,17 @@ private void gcFileIfStale(UfsJournalFile file, long checkpointSequenceNumber) {
       return;
     }
 
-    long lastModifiedTimeMs;
+    Long lastModifiedTimeMs;
     try {
       lastModifiedTimeMs = mUfs.getFileStatus(file.getLocation().toString()).getLastModifiedTime();
     } catch (IOException e) {
       LOG.warn("Failed to get the last modified time for {}.", file.getLocation());
       return;
     }
+    if (lastModifiedTimeMs == null) {
+      LOG.warn("Failed to get the last modified time for {}.", file.getLocation());
+      return;
+    }
 
     long thresholdMs = file.isTmpCheckpoint()
         ? ServerConfiguration.getMs(PropertyKey.MASTER_JOURNAL_TEMPORARY_FILE_GC_THRESHOLD_MS)

diff --git a/core/server/common/src/main/java/alluxio/master/journalv0/ufs/UfsJournalReader.java b/core/server/common/src/main/java/alluxio/master/journalv0/ufs/UfsJournalReader.java
@@ -107,7 +107,12 @@ public long getCheckpointLastModifiedTimeMs() throws IOException {
     if (!mUfs.isFile(mCheckpoint.toString())) {
       throw new IOException("Checkpoint file " + mCheckpoint + " does not exist.");
     }
-    mCheckpointLastModifiedTime = mUfs.getFileStatus(mCheckpoint.toString()).getLastModifiedTime();
+    Long lastModifiedTime = mUfs.getFileStatus(mCheckpoint.toString()).getLastModifiedTime();
+    if (lastModifiedTime == null) {
+      throw new IOException("Failed to get checkpoint file "
+          + mCheckpoint + " last modified time.");
+    }
+    mCheckpointLastModifiedTime = lastModifiedTime;
     return mCheckpointLastModifiedTime;
   }
 }
diff --git a/core/server/master/src/test/java/alluxio/master/file/FileSystemMasterSyncMetadataTest.java b/core/server/master/src/test/java/alluxio/master/file/FileSystemMasterSyncMetadataTest.java
@@ -162,7 +162,7 @@ public void listStatusWithSyncMetadataAndEmptyS3Owner() throws Exception {
     // Mock nested ufs path
     AlluxioURI nestedFilePath = ufsMount.join("dir1").join("file1");
     UfsFileStatus nestedFileStatus = new UfsFileStatus(nestedFilePath.getPath(), "dummy", 0,
-        0, "", "", mode, 1024);
+        null, "", "", mode, 1024);
     Mockito.when(mUfs.getFingerprint(nestedFilePath.toString()))
         .thenReturn(Fingerprint.create("s3", nestedFileStatus).serialize());
     Mockito.when(mUfs.getStatus(nestedFilePath.toString())).thenReturn(nestedFileStatus);
@@ -178,7 +178,7 @@ public void listStatusWithSyncMetadataAndEmptyS3Owner() throws Exception {
     // Mock creating the same directory and nested file in UFS out of band
     Mockito.when(mUfs.listStatus(eq(dir1Path.toString())))
         .thenReturn(new UfsStatus[]{new UfsFileStatus("file1", "dummy", 0,
-        0, "", "", mode, 1024)});
+        null, "", "", mode, 1024)});
 
     // List with sync.interval=0
     List<FileInfo> fileInfoList =

diff --git a/underfs/gcs/src/main/java/alluxio/underfs/gcs/GCSUnderFileSystem.java b/underfs/gcs/src/main/java/alluxio/underfs/gcs/GCSUnderFileSystem.java
@@ -45,6 +45,7 @@
 import java.io.OutputStream;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
+import java.util.Date;
 import java.util.function.Supplier;
 import javax.annotation.concurrent.ThreadSafe;
 
@@ -255,8 +256,10 @@ public ObjectStatus[] getObjectStatuses() {
       StorageObject[] objects = mChunk.getObjects();
       ObjectStatus[] ret = new ObjectStatus[objects.length];
       for (int i = 0; i < ret.length; ++i) {
+        Date lastModifiedDate = objects[i].getLastModifiedDate();
+        Long lastModifiedTime = lastModifiedDate == null ? null : lastModifiedDate.getTime();
         ret[i] = new ObjectStatus(objects[i].getKey(), objects[i].getMd5HashAsBase64(),
-            objects[i].getContentLength(), objects[i].getLastModifiedDate().getTime());
+            objects[i].getContentLength(), lastModifiedTime);
       }
       return ret;
     }

diff --git a/underfs/gcs/src/main/java/alluxio/underfs/gcs/v2/GCSV2UnderFileSystem.java b/underfs/gcs/src/main/java/alluxio/underfs/gcs/v2/GCSV2UnderFileSystem.java
@@ -306,8 +306,8 @@ protected InputStream openObject(String key, OpenOptions options,
    * @return the blob status
    */
   private ObjectStatus getBlobStatus(Blob blob) {
-    long time = blob.getUpdateTime() != null ? blob.getUpdateTime()
-        : blob.getCreateTime() != null ? blob.getCreateTime() : -1;
+    Long time = blob.getUpdateTime() != null ? blob.getUpdateTime()
+        : blob.getCreateTime() != null ? blob.getCreateTime() : null;
     return new ObjectStatus(blob.getName(), blob.getMd5() == null ? DIR_HASH : blob.getMd5(),
         blob.getSize(), time);
   }

diff --git a/underfs/obs/src/main/java/alluxio/underfs/obs/OBSUnderFileSystem.java b/underfs/obs/src/main/java/alluxio/underfs/obs/OBSUnderFileSystem.java
@@ -38,6 +38,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.util.Date;
 import java.util.List;
 import javax.annotation.concurrent.ThreadSafe;
 
@@ -244,8 +245,11 @@ public ObjectStatus[] getObjectStatuses() {
       ObjectStatus[] ret = new ObjectStatus[objects.size()];
       int i = 0;
       for (ObsObject obj : objects) {
-        ret[i++] = new ObjectStatus(obj.getObjectKey(), obj.getMetadata().getEtag(),
-            obj.getMetadata().getContentLength(), obj.getMetadata().getLastModified().getTime());
+        ObjectMetadata meta = obj.getMetadata();
+        Date lastModifiedDate = meta.getLastModified();
+        Long lastModifiedTime = lastModifiedDate == null ? null : lastModifiedDate.getTime();
+        ret[i++] = new ObjectStatus(obj.getObjectKey(), meta.getEtag(),
+            meta.getContentLength(), lastModifiedTime);
       }
       return ret;
     }
@@ -291,8 +295,10 @@ protected ObjectStatus getObjectStatus(String key) {
           return null;
         }
       }
+      Date lastModifiedDate = meta.getLastModified();
+      Long lastModifiedTime = lastModifiedDate == null ? null : lastModifiedDate.getTime();
       return new ObjectStatus(key, meta.getEtag(), meta.getContentLength(),
-          meta.getLastModified().getTime());
+          lastModifiedTime);
     } catch (ObsException e) {
       LOG.warn("Failed to get Object {}, return null", key, e);
       return null;

diff --git a/underfs/obs/src/test/java/alluxio/underfs/obs/OBSUnderFileSystemTest.java b/underfs/obs/src/test/java/alluxio/underfs/obs/OBSUnderFileSystemTest.java
@@ -141,4 +141,26 @@ public void judgeDirectoryInBucket() throws Exception {
     Assert.assertTrue(mOBSUnderFileSystem.isDirectory("dir1"));
     Assert.assertFalse(mOBSUnderFileSystem.isDirectory("obs_file1"));
   }
+
+  @Test
+  public void nullObjectMetaTest() throws Exception {
+    ObjectMetadata fileMeta = new ObjectMetadata();
+    fileMeta.setContentLength(10L);
+    ObjectMetadata dirMeta = new ObjectMetadata();
+    dirMeta.setContentLength(0L);
+    /**
+     * /xx/file1/ ( File1 actually exists, which is a file) , there is / after file1 name.
+     * When OBS, the path object meta is null.
+     * When PFS, the path object meta is not null. The object meta is same as /xx/file1
+     */
+    Mockito.when(mClient.getObjectMetadata(BUCKET_NAME, "pfs_file1"))
+        .thenReturn(fileMeta);
+    Mockito.when(mClient.getObjectMetadata(BUCKET_NAME, "dir1"))
+        .thenReturn(dirMeta);
+
+    mOBSUnderFileSystem = new OBSUnderFileSystem(new AlluxioURI(""), mClient, BUCKET_NAME, "obs",
+        UnderFileSystemConfiguration.defaults(ConfigurationTestUtils.defaults()));
+    Assert.assertNotNull(mOBSUnderFileSystem.getObjectStatus("pfs_file1"));
+    Assert.assertNotNull(mOBSUnderFileSystem.getObjectStatus("dir1"));
+  }
 }
diff --git a/underfs/oss/src/main/java/alluxio/underfs/oss/OSSUnderFileSystem.java b/underfs/oss/src/main/java/alluxio/underfs/oss/OSSUnderFileSystem.java
@@ -39,6 +39,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.util.Date;
 import java.util.List;
 import javax.annotation.concurrent.ThreadSafe;
 
@@ -211,8 +212,10 @@ public ObjectStatus[] getObjectStatuses() {
       ObjectStatus[] ret = new ObjectStatus[objects.size()];
       int i = 0;
       for (OSSObjectSummary obj : objects) {
+        Date lastModifiedDate = obj.getLastModified();
+        Long lastModifiedTime = lastModifiedDate == null ? null : lastModifiedDate.getTime();
         ret[i++] = new ObjectStatus(obj.getKey(), obj.getETag(), obj.getSize(),
-            obj.getLastModified().getTime());
+            lastModifiedTime);
       }
       return ret;
     }
@@ -243,8 +246,10 @@ protected ObjectStatus getObjectStatus(String key) {
       if (meta == null) {
         return null;
       }
+      Date lastModifiedDate = meta.getLastModified();
+      Long lastModifiedTime = lastModifiedDate == null ? null : lastModifiedDate.getTime();
       return new ObjectStatus(key, meta.getETag(), meta.getContentLength(),
-          meta.getLastModified().getTime());
+          lastModifiedTime);
     } catch (ServiceException e) {
       return null;
     }

diff --git a/underfs/s3a/src/main/java/alluxio/underfs/s3a/S3AUnderFileSystem.java b/underfs/s3a/src/main/java/alluxio/underfs/s3a/S3AUnderFileSystem.java
@@ -536,8 +536,10 @@ public ObjectStatus[] getObjectStatuses() {
       ObjectStatus[] ret = new ObjectStatus[objects.size()];
       int i = 0;
       for (S3ObjectSummary obj : objects) {
+        Date lastModifiedDate = obj.getLastModified();
+        Long lastModifiedTime = lastModifiedDate == null ? null : lastModifiedDate.getTime();
         ret[i++] = new ObjectStatus(obj.getKey(), obj.getETag(), obj.getSize(),
-            obj.getLastModified().getTime());
+            lastModifiedTime);
       }
       return ret;
     }
@@ -580,8 +582,10 @@ public ObjectStatus[] getObjectStatuses() {
       ObjectStatus[] ret = new ObjectStatus[objects.size()];
       int i = 0;
       for (S3ObjectSummary obj : objects) {
+        Date lastModifiedDate = obj.getLastModified();
+        Long lastModifiedTime = lastModifiedDate == null ? null : lastModifiedDate.getTime();
         ret[i++] = new ObjectStatus(obj.getKey(), obj.getETag(), obj.getSize(),
-            obj.getLastModified().getTime());
+            lastModifiedTime);
       }
       return ret;
     }
@@ -610,8 +614,9 @@ public ObjectListingChunk getNextChunk() throws IOException {
   protected ObjectStatus getObjectStatus(String key) throws IOException {
     try {
       ObjectMetadata meta = mClient.getObjectMetadata(mBucketName, key);
-      return new ObjectStatus(key, meta.getETag(), meta.getContentLength(),
-          meta.getLastModified().getTime());
+      Date lastModifiedDate = meta.getLastModified();
+      Long lastModifiedTime = lastModifiedDate == null ? null : lastModifiedDate.getTime();
+      return new ObjectStatus(key, meta.getETag(), meta.getContentLength(), lastModifiedTime);
     } catch (AmazonServiceException e) {
       if (e.getStatusCode() == 404) { // file not found, possible for exists calls
         return null;

diff --git a/underfs/s3a/src/test/java/alluxio/underfs/s3a/S3AUnderFileSystemTest.java b/underfs/s3a/src/test/java/alluxio/underfs/s3a/S3AUnderFileSystemTest.java
@@ -29,6 +29,7 @@
 import com.amazonaws.services.s3.AmazonS3Client;
 import com.amazonaws.services.s3.model.AccessControlList;
 import com.amazonaws.services.s3.model.ListObjectsV2Request;
+import com.amazonaws.services.s3.model.ObjectMetadata;
 import com.amazonaws.services.s3.model.Owner;
 import com.amazonaws.services.s3.transfer.TransferManager;
 import com.google.common.util.concurrent.ListeningExecutorService;
@@ -253,4 +254,12 @@ public void stripPrefixIfPresent() throws Exception {
     Assert.assertEquals("", mS3UnderFileSystem.stripPrefixIfPresent(""));
     Assert.assertEquals("", mS3UnderFileSystem.stripPrefixIfPresent("/"));
   }
+
+  @Test
+  public void getNullLastModifiedTime() throws IOException {
+    Mockito.when(mClient.getObjectMetadata(Matchers.anyString(), Matchers.anyString()))
+        .thenReturn(new ObjectMetadata());
+    // throw NPE before https://github.com/Alluxio/alluxio/pull/14641
+    mS3UnderFileSystem.getObjectStatus(PATH);
+  }
 }
diff --git a/underfs/swift/src/main/java/alluxio/underfs/swift/SwiftUnderFileSystem.java b/underfs/swift/src/main/java/alluxio/underfs/swift/SwiftUnderFileSystem.java
@@ -45,6 +45,7 @@
 import java.io.OutputStream;
 import java.util.ArrayDeque;
 import java.util.Arrays;
+import java.util.Date;
 import java.util.List;
 import javax.annotation.concurrent.ThreadSafe;
 
@@ -313,9 +314,11 @@ public ObjectStatus[] getObjectStatuses() {
       ObjectStatus[] res = new ObjectStatus[objects.size()];
       for (DirectoryOrObject object : objects) {
         if (object.isObject()) {
+          Date lastModifiedDate = object.getAsObject().getLastModifiedAsDate();
+          Long lastModifiedTime = lastModifiedDate == null ? null : lastModifiedDate.getTime();
           res[i++] = new ObjectStatus(object.getName(), object.getAsObject().getEtag(),
               object.getAsObject().getContentLength(),
-              object.getAsObject().getLastModifiedAsDate().getTime());
+              lastModifiedTime);
         } else {
           res[i++] = new ObjectStatus(object.getName());
         }
@@ -345,8 +348,10 @@ protected ObjectStatus getObjectStatus(String key) {
     Container container = mAccount.getContainer(mContainerName);
     StoredObject meta = container.getObject(key);
     if (meta != null && meta.exists()) {
+      Date lastModifiedDate = meta.getLastModifiedAsDate();
+      Long lastModifiedTime = lastModifiedDate == null ? null : lastModifiedDate.getTime();
       return new ObjectStatus(key, meta.getEtag(), meta.getContentLength(),
-          meta.getLastModifiedAsDate().getTime());
+          lastModifiedTime);
     }
     return null;
   }