From 74720aac19d00ae9309d2e29dcf2d00166fbad5b Mon Sep 17 00:00:00 2001 From: Vivek Maskara Date: Mon, 17 Aug 2020 04:43:36 -0700 Subject: [PATCH] Handle errors in chunk uploads (#3899) --- .../nrw/commons/OkHttpConnectionFactory.java | 20 +++++++++++++ .../nrw/commons/contributions/ChunkInfo.kt | 8 ++++-- .../free/nrw/commons/upload/UploadClient.java | 28 +++++++++++++++---- .../nrw/commons/upload/UploadInterface.java | 3 +- .../nrw/commons/upload/UploadService.java | 23 ++++++++++++++- .../dataclient/mwapi/MwException.java | 23 +++++++++++++-- .../dataclient/mwapi/MwResponse.java | 2 +- .../dataclient/mwapi/MwServiceError.java | 6 ++++ 8 files changed, 99 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/OkHttpConnectionFactory.java b/app/src/main/java/fr/free/nrw/commons/OkHttpConnectionFactory.java index 59e33ea24..b1228ad99 100644 --- a/app/src/main/java/fr/free/nrw/commons/OkHttpConnectionFactory.java +++ b/app/src/main/java/fr/free/nrw/commons/OkHttpConnectionFactory.java @@ -3,6 +3,9 @@ package fr.free.nrw.commons; import androidx.annotation.NonNull; import java.io.File; import java.io.IOException; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; import okhttp3.Cache; import okhttp3.Interceptor; import okhttp3.OkHttpClient; @@ -62,6 +65,8 @@ public final class OkHttpConnectionFactory { } public static class UnsuccessfulResponseInterceptor implements Interceptor { + private static final List DO_NOT_INTERCEPT = Collections.singletonList( + "api.php?format=json&formatversion=2&errorformat=plaintext&action=upload&ignorewarnings=1"); private static final String ERRORS_PREFIX = "{\"error"; @@ -69,6 +74,11 @@ public final class OkHttpConnectionFactory { @NonNull public Response intercept(@NonNull final Chain chain) throws IOException { final Response rsp = chain.proceed(chain.request()); + + // Do not intercept certain requests and let the caller handle the errors + if(isExcludedUrl(chain.request())) { + return rsp; + } if (rsp.isSuccessful()) { try (final ResponseBody responseBody = rsp.peekBody(ERRORS_PREFIX.length())) { if (ERRORS_PREFIX.equals(responseBody.string())) { @@ -83,6 +93,16 @@ public final class OkHttpConnectionFactory { } throw new HttpStatusException(rsp); } + + private boolean isExcludedUrl(final Request request) { + final String requestUrl = request.url().toString(); + for(final String url: DO_NOT_INTERCEPT) { + if(requestUrl.contains(url)) { + return true; + } + } + return false; + } } private OkHttpConnectionFactory() { diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ChunkInfo.kt b/app/src/main/java/fr/free/nrw/commons/contributions/ChunkInfo.kt index 464ea3145..0229998b9 100644 --- a/app/src/main/java/fr/free/nrw/commons/contributions/ChunkInfo.kt +++ b/app/src/main/java/fr/free/nrw/commons/contributions/ChunkInfo.kt @@ -3,21 +3,23 @@ package fr.free.nrw.commons.contributions import android.os.Parcel import android.os.Parcelable import fr.free.nrw.commons.upload.UploadResult -import kotlinx.android.parcel.Parcelize data class ChunkInfo( val uploadResult: UploadResult, - val lastChunkIndex: Int + val lastChunkIndex: Int, + var isLastChunkUploaded: Boolean ) : Parcelable { constructor(parcel: Parcel) : this( parcel.readParcelable(UploadResult::class.java.classLoader), - parcel.readInt() + parcel.readInt(), + parcel.readByte() != 0.toByte() ) { } override fun writeToParcel(parcel: Parcel, flags: Int) { parcel.writeParcelable(uploadResult, flags) parcel.writeInt(lastChunkIndex) + parcel.writeByte(if (isLastChunkUploaded) 1 else 0) } override fun describeContents(): Int { diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadClient.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadClient.java index de8e942d5..370dce960 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/UploadClient.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/UploadClient.java @@ -5,13 +5,13 @@ import static fr.free.nrw.commons.di.NetworkingModule.NAMED_COMMONS_CSRF; import android.content.Context; import android.net.Uri; import androidx.annotation.Nullable; +import com.google.gson.Gson; import fr.free.nrw.commons.CommonsApplication; import fr.free.nrw.commons.contributions.ChunkInfo; import fr.free.nrw.commons.contributions.Contribution; import fr.free.nrw.commons.upload.UploadService.NotificationUpdateProgressListener; import io.reactivex.Observable; import io.reactivex.disposables.CompositeDisposable; -import io.reactivex.functions.Consumer; import java.io.File; import java.io.IOException; import java.util.Date; @@ -24,6 +24,7 @@ import okhttp3.MediaType; import okhttp3.MultipartBody; import okhttp3.RequestBody; import org.wikipedia.csrf.CsrfTokenClient; +import org.wikipedia.dataclient.mwapi.MwException; import timber.log.Timber; @Singleton @@ -39,6 +40,7 @@ public class UploadClient { private final CsrfTokenClient csrfTokenClient; private final PageContentsCreator pageContentsCreator; private final FileUtilsWrapper fileUtilsWrapper; + private final Gson gson; private boolean pauseUploads = false; private final CompositeDisposable compositeDisposable = new CompositeDisposable(); @@ -47,11 +49,12 @@ public class UploadClient { public UploadClient(final UploadInterface uploadInterface, @Named(NAMED_COMMONS_CSRF) final CsrfTokenClient csrfTokenClient, final PageContentsCreator pageContentsCreator, - final FileUtilsWrapper fileUtilsWrapper) { + final FileUtilsWrapper fileUtilsWrapper, final Gson gson) { this.uploadInterface = uploadInterface; this.csrfTokenClient = csrfTokenClient; this.pageContentsCreator = pageContentsCreator; this.fileUtilsWrapper = fileUtilsWrapper; + this.gson = gson; } /** @@ -61,6 +64,10 @@ public class UploadClient { Observable uploadFileToStash( final Context context, final String filename, final Contribution contribution, final NotificationUpdateProgressListener notificationUpdater) throws IOException { + if (contribution.getChunkInfo() != null && contribution.getChunkInfo().isLastChunkUploaded()) { + return Observable.just(new StashUploadResult(StashUploadState.SUCCESS, + contribution.getChunkInfo().getUploadResult().getFilekey())); + } pauseUploads = false; File file = new File(contribution.getLocalUri().getPath()); final Observable fileChunks = fileUtilsWrapper.getFileChunks(context, file, CHUNK_SIZE); @@ -69,6 +76,7 @@ public class UploadClient { final AtomicInteger index = new AtomicInteger(); final AtomicReference chunkInfo = new AtomicReference<>(); + Timber.d("Chunk info"); if (contribution.getChunkInfo() != null && isStashValid(contribution)) { chunkInfo.set(contribution.getChunkInfo()); } @@ -96,12 +104,15 @@ public class UploadClient { offset, filekey, countingRequestBody).subscribe(uploadResult -> { - chunkInfo.set(new ChunkInfo(uploadResult, index.incrementAndGet())); + chunkInfo.set(new ChunkInfo(uploadResult, index.incrementAndGet(), false)); notificationUpdater.onChunkUploaded(contribution, chunkInfo.get()); }, throwable -> { Timber.e(throwable, "Error occurred in uploading chunk"); })); })); + + chunkInfo.get().setLastChunkUploaded(true); + notificationUpdater.onChunkUploaded(contribution, chunkInfo.get()); if (pauseUploads) { return Observable.just(new StashUploadResult(StashUploadState.PAUSED, null)); } else if (chunkInfo.get() != null) { @@ -183,9 +194,16 @@ public class UploadClient { pageContentsCreator.createFrom(contribution), CommonsApplication.DEFAULT_EDIT_SUMMARY, uniqueFileName, - fileKey).map(UploadResponse::getUpload); + fileKey).map(uploadResponse -> { + UploadResponse uploadResult = gson.fromJson(uploadResponse, UploadResponse.class); + if (uploadResult.getUpload() == null) { + final MwException exception = gson.fromJson(uploadResponse, MwException.class); + throw new RuntimeException(exception.getErrorCode()); + } + return uploadResult.getUpload(); + }); } catch (final Throwable throwable) { - throwable.printStackTrace(); + Timber.e(throwable, "Exception occurred in uploading file from stash"); return Observable.error(throwable); } } diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadInterface.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadInterface.java index 9ee3023a6..98bae25be 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/UploadInterface.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/UploadInterface.java @@ -2,6 +2,7 @@ package fr.free.nrw.commons.upload; import androidx.annotation.NonNull; +import com.google.gson.JsonObject; import io.reactivex.Observable; import okhttp3.MultipartBody; import okhttp3.RequestBody; @@ -29,7 +30,7 @@ public interface UploadInterface { @POST(MW_API_PREFIX + "action=upload&ignorewarnings=1") @FormUrlEncoded @NonNull - Observable uploadFileFromStash(@NonNull @Field("token") String token, + Observable uploadFileFromStash(@NonNull @Field("token") String token, @NonNull @Field("text") String text, @NonNull @Field("comment") String comment, @NonNull @Field("filename") String filename, diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java index be3e1426a..57e044019 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java @@ -26,11 +26,14 @@ import fr.free.nrw.commons.wikidata.WikidataEditService; import io.reactivex.Observable; import io.reactivex.Scheduler; import io.reactivex.disposables.CompositeDisposable; +import io.reactivex.functions.Consumer; import io.reactivex.processors.PublishProcessor; import io.reactivex.schedulers.Schedulers; import java.io.IOException; +import java.util.Arrays; import java.util.Date; import java.util.HashSet; +import java.util.List; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -42,6 +45,9 @@ public class UploadService extends CommonsDaggerService { private static final String EXTRA_PREFIX = "fr.free.nrw.commons.upload"; + private static final List STASH_ERROR_CODES = Arrays + .asList("uploadstash-file-not-found", "stashfailed", "verification-error", "chunk-too-small"); + public static final String ACTION_START_SERVICE = EXTRA_PREFIX + ".upload"; public static final String EXTRA_FILES = EXTRA_PREFIX + ".files"; @Inject @@ -290,7 +296,15 @@ public class UploadService extends CommonsDaggerService { getApplicationContext(), contribution, uniqueFilename, - uploadStash.getFileKey()); + uploadStash.getFileKey()).doOnError(new Consumer() { + @Override + public void accept(Throwable throwable) throws Exception { + Timber.e(throwable, "Error occurred in uploading file from stash"); + if (STASH_ERROR_CODES.contains(throwable.getMessage())) { + clearChunks(contribution); + } + } + }); } else if (uploadStash.getState() == StashUploadState.PAUSED) { Timber.d("Contribution upload paused"); showPausedNotification(contribution); @@ -310,6 +324,13 @@ public class UploadService extends CommonsDaggerService { }); } + private void clearChunks(Contribution contribution) { + contribution.setChunkInfo(null); + compositeDisposable.add(contributionDao.update(contribution) + .subscribeOn(ioThreadScheduler) + .subscribe()); + } + private void onUpload(Contribution contribution, String notificationTag, UploadResult uploadResult) { Timber.d("Stash upload response 2 is %s", uploadResult.toString()); diff --git a/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwException.java b/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwException.java index fbe06019d..51af36196 100644 --- a/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwException.java +++ b/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwException.java @@ -2,15 +2,32 @@ package org.wikipedia.dataclient.mwapi; import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import java.util.List; public class MwException extends RuntimeException { - @SuppressWarnings("unused") @NonNull private final MwServiceError error; + @SuppressWarnings("unused") @Nullable private final MwServiceError error; - public MwException(@NonNull MwServiceError error) { + @SuppressWarnings("unused") @Nullable private final List errors; + + public MwException(@Nullable MwServiceError error, + @Nullable final List errors) { this.error = error; + this.errors = errors; } - @NonNull public MwServiceError getError() { + @NonNull + public List getErrors() { + return errors; + } + + public String getErrorCode() { + if(error!=null) { + return error.getCode(); + } + return errors != null ? errors.get(0).getCode() : null; + } + + @Nullable public MwServiceError getError() { return error; } diff --git a/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwResponse.java b/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwResponse.java index 49301f7fd..4c14abc7d 100644 --- a/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwResponse.java +++ b/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwResponse.java @@ -17,7 +17,7 @@ public abstract class MwResponse extends BaseModel implements PostProcessingType @Override public void postProcess() { if (errors != null && !errors.isEmpty()) { - throw new MwException(errors.get(0)); + throw new MwException(errors.get(0), errors); } } } diff --git a/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwServiceError.java b/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwServiceError.java index e5c819938..f2b3eb463 100644 --- a/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwServiceError.java +++ b/data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwServiceError.java @@ -13,6 +13,7 @@ import java.util.List; * Gson POJO for a MediaWiki API error. */ public class MwServiceError extends BaseModel implements ServiceError { + @SuppressWarnings("unused") @Nullable private String code; @SuppressWarnings("unused") @Nullable private String text; @SuppressWarnings("unused") @Nullable private Data data; @@ -55,6 +56,11 @@ public class MwServiceError extends BaseModel implements ServiceError { return null; } + @Nullable + public String getCode() { + return code; + } + private static final class Data { @SuppressWarnings("unused") @Nullable private List messages;