From fc8cce22149acdfab158ca3f44531fe678a1575a Mon Sep 17 00:00:00 2001 From: ylexus Date: Mon, 28 Jun 2021 23:03:59 +0100 Subject: [PATCH] #105 fix a bug with any errors during drive space refresh were hidden --- app/build.gradle | 2 +- .../core/DriveSpaceTrackerImpl.java | 10 +++++-- .../ui/UploadPaneControllerImpl.java | 25 +++++++++++++++- .../main/resources/i18n/Resources.properties | 1 + .../resources/i18n/Resources_es.properties | 2 ++ .../resources/i18n/Resources_nl.properties | 2 ++ .../resources/i18n/Resources_ru.properties | 1 + .../i18n/Resources_zh_Hans.properties | 2 ++ .../i18n/Resources_zh_Hant.properties | 2 ++ app/src/main/resources/log4j2-ui.yaml | 2 +- .../core/IntegrationTest.java | 30 +++++++++++++++++-- .../core/MockGoogleDriveModule.java | 11 ++++++- .../core/RecordingProgressStatusFactory.java | 8 +++++ 13 files changed, 88 insertions(+), 10 deletions(-) diff --git a/app/build.gradle b/app/build.gradle index 6058918..74fbc0e 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -49,7 +49,7 @@ dependencies { ext.orgJunitJupiterVersion = '5.7.2' ext.orgMockitoVersion = '3.10.0' ext.orgImmutablesVersion = '2.8.8' - ext.netYudichevJiottyVersion = '2.0.4' + ext.netYudichevJiottyVersion = '2.1-SNAPSHOT' ext.orgApacheLoggingLog4jVersion = '2.14.1' annotationProcessor "org.immutables:value:$orgImmutablesVersion" diff --git a/app/src/main/java/net/yudichev/googlephotosupload/core/DriveSpaceTrackerImpl.java b/app/src/main/java/net/yudichev/googlephotosupload/core/DriveSpaceTrackerImpl.java index 00dcd0d..c40661b 100644 --- a/app/src/main/java/net/yudichev/googlephotosupload/core/DriveSpaceTrackerImpl.java +++ b/app/src/main/java/net/yudichev/googlephotosupload/core/DriveSpaceTrackerImpl.java @@ -17,7 +17,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; -import static java.util.concurrent.CompletableFuture.supplyAsync; import static net.yudichev.jiotty.common.lang.Locks.inLock; import static net.yudichev.jiotty.common.lang.MoreThrowables.getAsUnchecked; @@ -55,13 +54,17 @@ final class DriveSpaceTrackerImpl implements DriveSpaceTracker { @Override public CompletableFuture reset() { - return supplyAsync(() -> { + return CompletableFuture.supplyAsync(() -> { inLock(lock, () -> { //noinspection AssignmentToNull assigned in the method called next driveSpaceStatus = null; refreshDriveQuota(); }); return null; + }).whenComplete((ignored, e) -> { + if (driveSpaceStatus != null && e != null) { + driveSpaceStatus.close(false); + } }); } @@ -125,7 +128,8 @@ private void refreshDriveQuota() { driveSpaceStatus.updateSuccess((int) toMegabytes(usage)); refreshStatusDescription(); } - }); + }) + .getNow(null); // otherwise any exceptions will be silently swallowed; } private void validateUsage() { diff --git a/app/src/main/java/net/yudichev/googlephotosupload/ui/UploadPaneControllerImpl.java b/app/src/main/java/net/yudichev/googlephotosupload/ui/UploadPaneControllerImpl.java index 3795c7b..4e63c8b 100644 --- a/app/src/main/java/net/yudichev/googlephotosupload/ui/UploadPaneControllerImpl.java +++ b/app/src/main/java/net/yudichev/googlephotosupload/ui/UploadPaneControllerImpl.java @@ -1,5 +1,6 @@ package net.yudichev.googlephotosupload.ui; +import com.google.api.client.googleapis.json.GoogleJsonResponseException; import javafx.event.ActionEvent; import javafx.scene.control.Button; import javafx.scene.layout.VBox; @@ -21,7 +22,9 @@ import java.util.ResourceBundle; import java.util.concurrent.CompletableFuture; +import static com.google.api.client.http.HttpStatusCodes.STATUS_CODE_FORBIDDEN; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Throwables.getCausalChain; import static javafx.application.Platform.runLater; import static net.yudichev.jiotty.common.lang.HumanReadableExceptionMessage.humanReadableMessage; @@ -119,7 +122,7 @@ private void onUploadComplete(@Nullable Throwable exception) { logger.error("Upload failed", exception); logArea.getStyleClass().add("failed-background"); logAreaChildren.add(new Text(resourceBundle.getString("uploadPaneLogAreaFailurePrefix") + " ")); - var failureText = new Text(humanReadableMessage(exception)); + var failureText = new Text(toHumanReadableMessage(exception)); failureText.getStyleClass().add("failed-text"); logAreaChildren.add(failureText); } @@ -129,6 +132,26 @@ private void onUploadComplete(@Nullable Throwable exception) { }); } + private String toHumanReadableMessage(Throwable exception) { + return getCausalChain(exception).stream() + .filter(throwable -> throwable instanceof GoogleJsonResponseException) + .findFirst() + .map(throwable -> (GoogleJsonResponseException) throwable) + .map(jsonResponseException -> { + // better error for GoogleJsonResponseException, otherwise there's too much technical details. + var details = jsonResponseException.getDetails(); + if (details != null && details.getMessage() != null) { + if (details.getCode() == STATUS_CODE_FORBIDDEN) { + return details.getMessage() + ' ' + resourceBundle.getString("uploadPanePermissionErrorSuffix"); + } else { + return details.getMessage(); + } + } + return null; + }) + .orElseGet(() -> humanReadableMessage(exception)); + } + @Override public void stopUpload() { stopButton.setDisable(true); diff --git a/app/src/main/resources/i18n/Resources.properties b/app/src/main/resources/i18n/Resources.properties index 6520306..603af5c 100644 --- a/app/src/main/resources/i18n/Resources.properties +++ b/app/src/main/resources/i18n/Resources.properties @@ -85,6 +85,7 @@ supportPaneReportIssueSentence=Problems? Suggestions? supportPaneReportIssueLinkSentence=Raise an issue here. uploadPaneStopButtonText=Stop uploadPaneUploadMoreButtonText=Upload more... +uploadPanePermissionErrorSuffix=Have you denied the app any requested permissions during login? directoryStructureSupplierProgressTitle=Looking for files cloudAlbumsProviderProgressTitle=Loading albums in Google Photos uploaderAlbumProgressTitle=Adding to albums diff --git a/app/src/main/resources/i18n/Resources_es.properties b/app/src/main/resources/i18n/Resources_es.properties index 5b9af23..c3fcd4b 100644 --- a/app/src/main/resources/i18n/Resources_es.properties +++ b/app/src/main/resources/i18n/Resources_es.properties @@ -98,6 +98,8 @@ supportPaneReportIssueSentence=Problemas? Sugerencias? supportPaneReportIssueLinkSentence=Plantea un problema aquí. uploadPaneStopButtonText=Detener uploadPaneUploadMoreButtonText=Subir más... +#TODO translate +uploadPanePermissionErrorSuffix=Have you denied the app any requested permissions during login? directoryStructureSupplierProgressTitle=Buscando archivos cloudAlbumsProviderProgressTitle=Cargando albumes en Google Photos uploaderAlbumProgressTitle=Añadiendo a los álbumes diff --git a/app/src/main/resources/i18n/Resources_nl.properties b/app/src/main/resources/i18n/Resources_nl.properties index 8712748..30596bb 100644 --- a/app/src/main/resources/i18n/Resources_nl.properties +++ b/app/src/main/resources/i18n/Resources_nl.properties @@ -91,6 +91,8 @@ uiAuthorisationBrowserTitle=Login in Google uploadPaneLogAreaSuccessLabel=Success damen en heren uploadPaneLogAreaFailurePrefix=Er is iets fout gegaan: uploadPaneLogAreaPartialSuccessLabel=Klaar met enkele fouten, Om de fouten te bekijken klik op de "%s" links +#TODO translate +uploadPanePermissionErrorSuffix=Have you denied the app any requested permissions during login? folderSelectorDragHereLabel=Sleep je folder met media hierheen of folderSelectorBrowseButtonLabel=Bladeren... folderSelectorResumeCheckboxLabel=Ga door vanaf je laatste poging diff --git a/app/src/main/resources/i18n/Resources_ru.properties b/app/src/main/resources/i18n/Resources_ru.properties index 7f9636c..0f9c149 100644 --- a/app/src/main/resources/i18n/Resources_ru.properties +++ b/app/src/main/resources/i18n/Resources_ru.properties @@ -69,6 +69,7 @@ uiAuthorisationBrowserTitle=Войдите в Google uploadPaneLogAreaSuccessLabel=Дамы и господа, полёт завершён успешно! uploadPaneLogAreaFailurePrefix=Что-то пошло не так: uploadPaneLogAreaPartialSuccessLabel=Завершено, но с нескольмими ошибками. Ссылки "%s" выше покажут, что случилось. +uploadPanePermissionErrorSuffix=Возможно, приложению не дан доступ к некоторым запрошенным ресурсам во время входа в аккаунт? folderSelectorDragHereLabel=Перетащите сюда папку с медиа-файлами или folderSelectorBrowseButtonLabel=Выберите на диске... folderSelectorResumeCheckboxLabel=Продолжить с места последней попытки diff --git a/app/src/main/resources/i18n/Resources_zh_Hans.properties b/app/src/main/resources/i18n/Resources_zh_Hans.properties index be57f5a..b6ccf8f 100644 --- a/app/src/main/resources/i18n/Resources_zh_Hans.properties +++ b/app/src/main/resources/i18n/Resources_zh_Hans.properties @@ -73,6 +73,8 @@ uiAuthorisationBrowserTitle=登录Google uploadPaneLogAreaSuccessLabel=全部成功上传! uploadPaneLogAreaFailurePrefix=有错误产生: uploadPaneLogAreaPartialSuccessLabel=已完成，有部分失败，检视请按"%s"。 +#TODO translate +uploadPanePermissionErrorSuffix=Have you denied the app any requested permissions during login? folderSelectorDragHereLabel=拖曳文件匣或文檔到这里，或 folderSelectorBrowseButtonLabel=导航... folderSelectorResumeCheckboxLabel=从上次继续 diff --git a/app/src/main/resources/i18n/Resources_zh_Hant.properties b/app/src/main/resources/i18n/Resources_zh_Hant.properties index 498f148..2853eea 100644 --- a/app/src/main/resources/i18n/Resources_zh_Hant.properties +++ b/app/src/main/resources/i18n/Resources_zh_Hant.properties @@ -73,6 +73,8 @@ uiAuthorisationBrowserTitle=登入Google uploadPaneLogAreaSuccessLabel=全部成功上傳! uploadPaneLogAreaFailurePrefix=有錯誤產生: uploadPaneLogAreaPartialSuccessLabel=已完成，有部分失敗，檢視請按"%s"。 +#TODO translate +uploadPanePermissionErrorSuffix=Have you denied the app any requested permissions during login? folderSelectorDragHereLabel=拖曳資料匣或檔案到這裡，或 folderSelectorBrowseButtonLabel=瀏覽... folderSelectorResumeCheckboxLabel=從上次繼續 diff --git a/app/src/main/resources/log4j2-ui.yaml b/app/src/main/resources/log4j2-ui.yaml index e8a528f..f97d365 100644 --- a/app/src/main/resources/log4j2-ui.yaml +++ b/app/src/main/resources/log4j2-ui.yaml @@ -21,7 +21,7 @@ Configuration: Loggers: logger: - - name: net.yudichev.jiotty.connector.google.photos + - name: net.yudichev.jiotty.connector.google level: debug includeLocation: true - name: net.yudichev.googlephotosupload diff --git a/app/src/test/java/net/yudichev/googlephotosupload/core/IntegrationTest.java b/app/src/test/java/net/yudichev/googlephotosupload/core/IntegrationTest.java index 294071e..f5db5ab 100644 --- a/app/src/test/java/net/yudichev/googlephotosupload/core/IntegrationTest.java +++ b/app/src/test/java/net/yudichev/googlephotosupload/core/IntegrationTest.java @@ -1,11 +1,17 @@ package net.yudichev.googlephotosupload.core; +import com.google.api.client.googleapis.json.GoogleJsonError; +import com.google.api.client.googleapis.json.GoogleJsonResponseException; +import com.google.api.client.http.HttpHeaders; +import com.google.api.client.http.HttpResponseException; +import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import net.yudichev.googlephotosupload.core.RecordingGooglePhotosClient.Album; import net.yudichev.googlephotosupload.core.RecordingGooglePhotosClient.MediaItem; import net.yudichev.jiotty.common.app.Application; import net.yudichev.jiotty.common.lang.Json; import net.yudichev.jiotty.common.varstore.VarStore; +import net.yudichev.jiotty.connector.google.drive.InMemoryGoogleDriveClient; import net.yudichev.jiotty.connector.google.photos.GoogleMediaItem; import net.yudichev.jiotty.connector.google.photos.GooglePhotosAlbum; import org.apache.commons.cli.CommandLineParser; @@ -67,6 +73,7 @@ final class IntegrationTest { private RecordingGooglePhotosClient googlePhotosClient; private RecordingProgressStatusFactory progressStatusFactory; private Path settingsRootPath; + private InMemoryGoogleDriveClient googleDriveClient; @BeforeEach void setUp() throws IOException { @@ -81,6 +88,7 @@ void setUp() throws IOException { TestTimeModule.resetTime(); setDefaultPreferences(); + googleDriveClient = new InMemoryGoogleDriveClient(); } @AfterEach @@ -773,6 +781,22 @@ void customisedRelevantFolderDepth1() throws Exception { )); } + @Test + void failsUploadIfDriveSpaceRefreshFails() throws InterruptedException { + var details = new GoogleJsonError(); + details.setCode(403); + details.setMessage("Nope"); + var rootCause = new GoogleJsonResponseException( + new HttpResponseException.Builder(403, "Nope", new HttpHeaders()), + details); + googleDriveClient.getBehaviour().failOnCreateFileWith(new RuntimeException(rootCause)); + + doExecuteUpload(); + assertThat(getLastFailure().map(Throwables::getRootCause), optionalWithValue(equalTo(rootCause))); + var spaceUsedProgress = progressStatusFactory.getStatusByName().get("Google Account Space Used (currently unreliable!)"); + assertThat(spaceUsedProgress.getClosedWithSuccess(), is(optionalWithValue(equalTo(false)))); + } + private void createStandardTestFiles() throws IOException { /* outerAlbum @@ -877,6 +901,7 @@ private void doVerifyGoogleClientAlbumState() { } private void doVerifyGoogleClientItemState() { + assertThat(googlePhotosClient.getAllItems(), containsInAnyOrder( allOf( itemForFile(rootPhoto), @@ -908,7 +933,7 @@ private void doExecuteUpload(String... additionalCommandLineOptions) throws Inte .addModule(TestTimeModule::new) .addModule(() -> new CoreDependenciesModule(settingsModule.getAuthDataStoreRootPath())) .addModule(() -> new MockGooglePhotosModule(googlePhotosClient)) - .addModule(MockGoogleDriveModule::new) + .addModule(() -> new MockGoogleDriveModule(googleDriveClient)) .addModule(ResourceBundleModule::new) .addModule(() -> new UploadPhotosModule(Duration.ofMillis(1))) .addModule(() -> new IntegrationTestUploadStarterModule(commandLine, progressStatusFactory)) @@ -948,8 +973,7 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOEx return delete(dir); } - @SuppressWarnings("SameReturnValue") - private FileVisitResult delete(Path path) throws IOException { + private static FileVisitResult delete(Path path) throws IOException { Files.delete(path); return CONTINUE; } diff --git a/app/src/test/java/net/yudichev/googlephotosupload/core/MockGoogleDriveModule.java b/app/src/test/java/net/yudichev/googlephotosupload/core/MockGoogleDriveModule.java index dea1b40..8437ac0 100644 --- a/app/src/test/java/net/yudichev/googlephotosupload/core/MockGoogleDriveModule.java +++ b/app/src/test/java/net/yudichev/googlephotosupload/core/MockGoogleDriveModule.java @@ -4,9 +4,18 @@ import net.yudichev.jiotty.connector.google.drive.GoogleDriveClient; import net.yudichev.jiotty.connector.google.drive.InMemoryGoogleDriveClient; +import static com.google.common.base.Preconditions.checkNotNull; + final class MockGoogleDriveModule extends AbstractModule { + + private final InMemoryGoogleDriveClient googleDriveClient; + + MockGoogleDriveModule(InMemoryGoogleDriveClient googleDriveClient) { + this.googleDriveClient = checkNotNull(googleDriveClient); + } + @Override protected void configure() { - bind(GoogleDriveClient.class).toInstance(new InMemoryGoogleDriveClient()); + bind(GoogleDriveClient.class).toInstance(googleDriveClient); } } diff --git a/app/src/test/java/net/yudichev/googlephotosupload/core/RecordingProgressStatusFactory.java b/app/src/test/java/net/yudichev/googlephotosupload/core/RecordingProgressStatusFactory.java index c0ef970..7c430e7 100644 --- a/app/src/test/java/net/yudichev/googlephotosupload/core/RecordingProgressStatusFactory.java +++ b/app/src/test/java/net/yudichev/googlephotosupload/core/RecordingProgressStatusFactory.java @@ -44,6 +44,7 @@ static final class RecordingProgressStatus implements ProgressStatus { private Optional totalCount; private int successCount; private String description; + private Optional closedWithSuccess = Optional.empty(); private RecordingProgressStatus(Optional totalCount) { this.totalCount = checkNotNull(totalCount); @@ -83,6 +84,9 @@ public void onBackoffDelay(long backoffDelayMs) { @Override public void close(boolean success) { + inLock(lock, () -> { + closedWithSuccess = Optional.of(success); + }); } @Override @@ -105,5 +109,9 @@ public int getSuccessCount() { public String getDescription() { return inLock(lock, () -> description); } + + public Optional getClosedWithSuccess() { + return inLock(lock, () -> closedWithSuccess); + } } }