Handle errors in the S3 bucket explorer (#2380)

hunterwerlla · web-flow · commit 096faf5c31fe · 2021-02-04T14:59:08.000-08:00
- If any part threw, we would fail silently, catch exceptions and show error nodes
- Allow loading more again to retry on errors, if the bucket/folder itself doesn't load the refresh button can be used
diff --git a/jetbrains-core/src/software/aws/toolkits/jetbrains/services/s3/editor/S3TreeNode.kt b/jetbrains-core/src/software/aws/toolkits/jetbrains/services/s3/editor/S3TreeNode.kt
@@ -8,6 +8,9 @@ import com.intellij.openapi.fileTypes.FileTypeRegistry
 import com.intellij.openapi.util.io.FileUtilRt
 import com.intellij.ui.treeStructure.SimpleNode
 import kotlinx.coroutines.runBlocking
+import software.aws.toolkits.core.utils.error
+import software.aws.toolkits.core.utils.getLogger
+import software.aws.toolkits.jetbrains.core.utils.buildList
 import software.aws.toolkits.jetbrains.services.s3.NOT_VERSIONED_VERSION_ID
 import software.aws.toolkits.resources.message
 import java.time.Instant
@@ -61,8 +64,12 @@ abstract class S3LazyLoadParentNode<T>(bucket: S3VirtualBucket, parent: S3LazyLo
                 return
             }
 
-            loadedPages.add(continuationMarker)
-            cachedList = children.dropLastWhile { it is S3TreeContinuationNode<*> } + loadObjects(continuationMarker)
+            val more = loadObjects(continuationMarker)
+            // Only say it has loaded before if it loaded successfully
+            if (more.none { it is S3TreeErrorNode || it is S3TreeErrorContinuationNode<*> }) {
+                loadedPages.add(continuationMarker)
+            }
+            cachedList = children.dropLastWhile { it is S3TreeContinuationNode<*> || it is S3TreeErrorNode } + more
         }
     }
 
@@ -77,26 +84,41 @@ class S3TreeDirectoryNode(bucket: S3VirtualBucket, parent: S3TreeDirectoryNode?,
     override fun directoryPath(): String = key
 
     override fun loadObjects(continuationMarker: String?): List<S3TreeNode> {
-        val response = runBlocking {
-            bucket.listObjects(key, continuationMarker)
-        }
-
-        val continuation = listOfNotNull(
-            response.nextContinuationToken()?.let {
-                S3TreeContinuationNode(bucket, this, this.key, it)
+        try {
+            val response = runBlocking {
+                bucket.listObjects(key, continuationMarker)
             }
-        )
-
-        val folders = response.commonPrefixes()?.map { S3TreeDirectoryNode(bucket, this, it.prefix()) } ?: emptyList()
 
-        val s3Objects = response
-            .contents()
-            ?.filterNotNull()
-            ?.filterNot { it.key() == key }
-            ?.map { S3TreeObjectNode(this, it.key(), it.size(), it.lastModified()) as S3TreeNode }
-            ?: emptyList()
+            val continuation = listOfNotNull(
+                response.nextContinuationToken()?.let {
+                    S3TreeContinuationNode(bucket, this, this.key, it)
+                }
+            )
+
+            val folders = response.commonPrefixes()?.map { S3TreeDirectoryNode(bucket, this, it.prefix()) } ?: emptyList()
+
+            val s3Objects = response
+                .contents()
+                ?.filterNotNull()
+                ?.filterNot { it.key() == key }
+                ?.map { S3TreeObjectNode(this, it.key(), it.size(), it.lastModified()) as S3TreeNode }
+                ?: emptyList()
+
+            return (folders + s3Objects).sortedBy { it.key } + continuation
+        } catch (e: Exception) {
+            LOG.error(e) { "Loading objects failed!" }
+            return buildList {
+                if (continuationMarker != null) {
+                    add(S3TreeErrorContinuationNode(bucket, this@S3TreeDirectoryNode, this@S3TreeDirectoryNode.key, continuationMarker))
+                } else {
+                    add(S3TreeErrorNode(bucket, this@S3TreeDirectoryNode))
+                }
+            }
+        }
+    }
 
-        return (folders + s3Objects).sortedBy { it.key } + continuation
+    companion object {
+        private val LOG = getLogger<S3TreeDirectoryNode>()
     }
 }
 
@@ -129,12 +151,16 @@ class S3TreeObjectNode(parent: S3TreeDirectoryNode, key: String, override val si
     override fun fileName() = key.substringAfterLast("/")
 
     override fun loadObjects(continuationMarker: VersionContinuationToken?): List<S3TreeNode> {
-        if (showHistory) {
+        if (!showHistory) {
+            return emptyList()
+        }
+
+        try {
             val response = runBlocking {
                 bucket.listObjectVersions(key, continuationMarker?.keyMarker, continuationMarker?.versionId)
             }
 
-            return mutableListOf<S3TreeNode>().apply {
+            return buildList {
                 response?.versions()
                     ?.filter { it.key() == key && it.versionId() != NOT_VERSIONED_VERSION_ID }
                     ?.map { S3TreeObjectVersionNode(this@S3TreeObjectNode, it.versionId(), it.size(), it.lastModified()) }
@@ -154,9 +180,27 @@ class S3TreeObjectNode(parent: S3TreeDirectoryNode, key: String, override val si
                     )
                 }
             }
+        } catch (e: Exception) {
+            LOG.error(e) { "Loading objects failed!" }
+            return buildList {
+                if (continuationMarker != null) {
+                    add(
+                        S3TreeErrorContinuationNode(
+                            bucket,
+                            this@S3TreeObjectNode,
+                            this@S3TreeObjectNode.key,
+                            continuationMarker
+                        )
+                    )
+                } else {
+                    add(S3TreeErrorNode(bucket, this@S3TreeObjectNode))
+                }
+            }
         }
+    }
 
-        return emptyList()
+    companion object {
+        private val LOG = getLogger<S3TreeObjectNode>()
     }
 }
 
@@ -190,7 +234,7 @@ class S3TreeObjectVersionNode(parent: S3TreeObjectNode, override val versionId:
     override fun toString(): String = "S3TreeObjectVersionNode(key='$key', versionId='$versionId')"
 }
 
-class S3TreeContinuationNode<T>(
+open class S3TreeContinuationNode<T>(
     bucket: S3VirtualBucket,
     private val parentNode: S3LazyLoadParentNode<T>,
     key: String,
@@ -208,3 +252,27 @@ class S3TreeContinuationNode<T>(
 
     override fun getEqualityObjects(): Array<Any?> = arrayOf(bucket, key, continuationMarker)
 }
+
+class S3TreeErrorContinuationNode<T>(
+    bucket: S3VirtualBucket,
+    parentNode: S3LazyLoadParentNode<T>,
+    key: String,
+    continuationMarker: T
+) : S3TreeContinuationNode<T>(bucket, parentNode, key, continuationMarker) {
+    init {
+        icon = AllIcons.General.Error
+    }
+
+    override fun displayName(): String = message("s3.load_more_failed")
+}
+
+class S3TreeErrorNode(
+    bucket: S3VirtualBucket,
+    parentNode: S3LazyLoadParentNode<*>
+) : S3TreeNode(bucket, parentNode, "${parentNode.key}error") {
+    init {
+        icon = AllIcons.General.Error
+    }
+
+    override fun displayName(): String = message("s3.error_loading")
+}
diff --git a/jetbrains-core/src/software/aws/toolkits/jetbrains/services/s3/objectActions/CopyPathAction.kt b/jetbrains-core/src/software/aws/toolkits/jetbrains/services/s3/objectActions/CopyPathAction.kt
@@ -5,6 +5,8 @@ package software.aws.toolkits.jetbrains.services.s3.objectActions
 
 import com.intellij.openapi.ide.CopyPasteManager
 import com.intellij.openapi.project.Project
+import software.aws.toolkits.jetbrains.services.s3.editor.S3TreeContinuationNode
+import software.aws.toolkits.jetbrains.services.s3.editor.S3TreeErrorNode
 import software.aws.toolkits.jetbrains.services.s3.editor.S3TreeNode
 import software.aws.toolkits.jetbrains.services.s3.editor.S3TreeObjectVersionNode
 import software.aws.toolkits.jetbrains.services.s3.editor.S3TreeTable
@@ -18,5 +20,5 @@ class CopyPathAction(private val project: Project, treeTable: S3TreeTable) : Sin
         S3Telemetry.copyPath(project)
     }
 
-    override fun enabled(node: S3TreeNode): Boolean = node !is S3TreeObjectVersionNode
+    override fun enabled(node: S3TreeNode): Boolean = node !is S3TreeObjectVersionNode && node !is S3TreeContinuationNode<*> && node !is S3TreeErrorNode
 }
diff --git a/jetbrains-core/tst/software/aws/toolkits/jetbrains/services/s3/editor/S3TreeDirectoryNodeTest.kt b/jetbrains-core/tst/software/aws/toolkits/jetbrains/services/s3/editor/S3TreeDirectoryNodeTest.kt
@@ -8,6 +8,7 @@ import com.intellij.testFramework.ProjectRule
 import com.nhaarman.mockitokotlin2.any
 import com.nhaarman.mockitokotlin2.argumentCaptor
 import com.nhaarman.mockitokotlin2.doReturn
+import com.nhaarman.mockitokotlin2.doThrow
 import com.nhaarman.mockitokotlin2.times
 import com.nhaarman.mockitokotlin2.verify
 import org.assertj.core.api.Assertions.assertThat
@@ -149,6 +150,61 @@ class S3TreeDirectoryNodeTest {
         assertThat(secondRequest.continuationToken()).isEqualTo("Token")
     }
 
+    @Test
+    fun `get children fails shows error node`() {
+        val s3Client = delegateMock<S3Client> {
+            on { listObjectsV2(any<ListObjectsV2Request>()) } doThrow IllegalStateException("Bad!")
+        }
+
+        val bucket = S3VirtualBucket(s3Bucket, s3Client)
+        val sut = S3TreeDirectoryNode(bucket, null, "my/folder")
+
+        assertThat(sut.children).containsExactly(
+            S3TreeErrorNode(bucket, sut)
+        )
+    }
+
+    @Test
+    fun `load more fails once then succeeds`() {
+        val s3Client = delegateMock<S3Client> {
+            on { listObjectsV2(any<ListObjectsV2Request>()) }
+                .thenReturn(
+                    ListObjectsV2Response.builder()
+                        .contents(createS3Object("my/folder/file.txt"))
+                        .nextContinuationToken("Token")
+                        .build()
+                )
+                .thenThrow(IllegalStateException("Bad!"))
+                .thenReturn(
+                    ListObjectsV2Response.builder()
+                        .contents(createS3Object("my/folder/picture.png"))
+                        .build()
+                )
+        }
+
+        val bucket = S3VirtualBucket(s3Bucket, s3Client)
+        val sut = S3TreeDirectoryNode(bucket, null, "my/folder/")
+
+        assertThat(sut.children).containsExactly(
+            S3TreeObjectNode(sut, "my/folder/file.txt", objectSize, lastModifiedTime),
+            S3TreeContinuationNode(bucket, sut, sut.key, "Token")
+        )
+
+        (sut.children.last() as S3TreeContinuationNode<*>).loadMore()
+
+        assertThat(sut.children).containsExactly(
+            S3TreeObjectNode(sut, "my/folder/file.txt", objectSize, lastModifiedTime),
+            S3TreeErrorContinuationNode(bucket, sut, sut.key, "Token")
+        )
+
+        (sut.children.last() as S3TreeContinuationNode<*>).loadMore()
+
+        assertThat(sut.children).containsExactly(
+            S3TreeObjectNode(sut, "my/folder/file.txt", objectSize, lastModifiedTime),
+            S3TreeObjectNode(sut, "my/folder/picture.png", objectSize, lastModifiedTime)
+        )
+    }
+
     @Test
     fun `load more is idempotent`() {
         val requestCaptor = argumentCaptor<ListObjectsV2Request>()
diff --git a/jetbrains-core/tst/software/aws/toolkits/jetbrains/services/s3/editor/S3TreeObjectNodeTest.kt b/jetbrains-core/tst/software/aws/toolkits/jetbrains/services/s3/editor/S3TreeObjectNodeTest.kt
@@ -8,6 +8,7 @@ import com.intellij.testFramework.ProjectRule
 import com.nhaarman.mockitokotlin2.any
 import com.nhaarman.mockitokotlin2.argumentCaptor
 import com.nhaarman.mockitokotlin2.doReturn
+import com.nhaarman.mockitokotlin2.doThrow
 import com.nhaarman.mockitokotlin2.times
 import com.nhaarman.mockitokotlin2.verify
 import org.assertj.core.api.Assertions.assertThat
@@ -175,6 +176,66 @@ class S3TreeObjectNodeTest {
         )
     }
 
+    @Test
+    fun `get children fails`() {
+        val s3Client = delegateMock<S3Client> {
+            on { listObjectVersions(any<ListObjectVersionsRequest>()) } doThrow IllegalStateException("Network broke")
+        }
+        val bucket = S3VirtualBucket(s3Bucket, s3Client)
+        val parent = S3TreeDirectoryNode(bucket, null, "my/folder/")
+        val sut = S3TreeObjectNode(parent, "my/folder/picture.png", objectSize, lastModifiedTime).apply { showHistory = true }
+
+        assertThat(sut.children).containsExactly(
+            S3TreeErrorNode(bucket, sut)
+        )
+    }
+
+    @Test
+    fun `get children fails with pagination`() {
+        val s3Client = delegateMock<S3Client> {
+            on { listObjectVersions(any<ListObjectVersionsRequest>()) }
+                .thenReturn(
+                    ListObjectVersionsResponse.builder()
+                        .versions(createS3ObjectVersion("my/folder/picture.png", "1"))
+                        .nextKeyMarker("KeyToken")
+                        .nextVersionIdMarker("VersionToken")
+                        .isTruncated(true)
+                        .build()
+                )
+                .thenThrow(IllegalStateException("Network broke"))
+                .thenReturn(
+                    ListObjectVersionsResponse.builder()
+                        .versions(createS3ObjectVersion("my/folder/picture.png", "2"))
+                        .isTruncated(false)
+                        .build()
+                )
+        }
+
+        val bucket = S3VirtualBucket(s3Bucket, s3Client)
+        val parent = S3TreeDirectoryNode(bucket, null, "my/folder/")
+        val sut = S3TreeObjectNode(parent, "my/folder/picture.png", objectSize, lastModifiedTime)
+        sut.showHistory = true
+
+        assertThat(sut.children).containsExactly(
+            S3TreeObjectVersionNode(sut, "1", objectSize, lastModifiedTime),
+            S3TreeContinuationNode(bucket, sut, sut.key, VersionContinuationToken("KeyToken", "VersionToken"))
+        )
+
+        (sut.children.last() as S3TreeContinuationNode<*>).loadMore()
+
+        assertThat(sut.children).containsExactly(
+            S3TreeObjectVersionNode(sut, "1", objectSize, lastModifiedTime),
+            S3TreeErrorContinuationNode(bucket, sut, sut.key, VersionContinuationToken("KeyToken", "VersionToken"))
+        )
+
+        (sut.children.last() as S3TreeContinuationNode<*>).loadMore()
+
+        assertThat(sut.children).containsExactly(
+            S3TreeObjectVersionNode(sut, "1", objectSize, lastModifiedTime),
+            S3TreeObjectVersionNode(sut, "2", objectSize, lastModifiedTime)
+        )
+    }
+
     @Test
     fun `get children ignores versionId 'null'`() {
         val requestCaptor = argumentCaptor<ListObjectVersionsRequest>()
diff --git a/resources/resources/software/aws/toolkits/resources/localized_messages.properties b/resources/resources/software/aws/toolkits/resources/localized_messages.properties
@@ -626,8 +626,10 @@ s3.download.object.conflict.skip=Skip
 s3.download.object.conflict.skip_rest=Skip All Conflicts
 s3.download.object.failed=Failed to download object {0}
 s3.download.object.progress=Downloading ''{0}''
+s3.error_loading=Failed to load children
 s3.last_modified=Last Modified
 s3.load_more=load more...
+s3.load_more_failed=Failed to load more! load more...
 s3.name=Name
 s3.new.folder=New folder...
 s3.new.folder.name=Folder name:

Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,8 @@ package software.aws.toolkits.jetbrains.services.s3.objectActions`
`5`	`5`
`6`	`6`	`import com.intellij.openapi.ide.CopyPasteManager`
`7`	`7`	`import com.intellij.openapi.project.Project`
	`8`	`+import software.aws.toolkits.jetbrains.services.s3.editor.S3TreeContinuationNode`
	`9`	`+import software.aws.toolkits.jetbrains.services.s3.editor.S3TreeErrorNode`
`8`	`10`	`import software.aws.toolkits.jetbrains.services.s3.editor.S3TreeNode`
`9`	`11`	`import software.aws.toolkits.jetbrains.services.s3.editor.S3TreeObjectVersionNode`
`10`	`12`	`import software.aws.toolkits.jetbrains.services.s3.editor.S3TreeTable`
`@@ -18,5 +20,5 @@ class CopyPathAction(private val project: Project, treeTable: S3TreeTable) : Sin`
`18`	`20`	`S3Telemetry.copyPath(project)`
`19`	`21`	`}`
`20`	`22`
`21`		`- override fun enabled(node: S3TreeNode): Boolean = node !is S3TreeObjectVersionNode`
	`23`	`+ override fun enabled(node: S3TreeNode): Boolean = node !is S3TreeObjectVersionNode && node !is S3TreeContinuationNode<*> && node !is S3TreeErrorNode`
`22`	`24`	`}`