Fix uploads getting stuck (#2309)

* Fix uploads getting stuck

* Fix test

* Use single instance of Gson across the app

* More logs to help debug upload failure

* Add request identifier

* wip

* Fix issues with image quality check dialogs
This commit is contained in:
Vivek Maskara 2019-01-24 13:26:18 +05:30 committed by Josephine Lim
parent 07f9af19f5
commit 532ab8aeae
16 changed files with 85 additions and 91 deletions

View file

@ -446,7 +446,7 @@ public class MainActivity extends AuthenticatedActivity implements FragmentManag
@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
if (IntentUtils.shouldContributionsListHandle(requestCode, resultCode, data)) {
if (IntentUtils.shouldContributionsHandle(requestCode, resultCode, data)) {
List<Image> images = ImagePicker.getImages(data);
Intent shareIntent = controller.handleImagesPicked(ImageUtils.getUriListFromImages(images), requestCode);
startActivity(shareIntent);

View file

@ -3,9 +3,13 @@ package fr.free.nrw.commons.di;
import android.app.Activity;
import android.content.ContentProviderClient;
import android.content.Context;
import android.net.Uri;
import android.support.v4.util.LruCache;
import android.view.inputmethod.InputMethodManager;
import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
@ -29,6 +33,8 @@ import fr.free.nrw.commons.nearby.NearbyPlaces;
import fr.free.nrw.commons.settings.Prefs;
import fr.free.nrw.commons.upload.UploadController;
import fr.free.nrw.commons.utils.ConfigUtils;
import fr.free.nrw.commons.utils.UriDeserializer;
import fr.free.nrw.commons.utils.UriSerializer;
import fr.free.nrw.commons.wikidata.WikidataEditListener;
import fr.free.nrw.commons.wikidata.WikidataEditListenerImpl;
@ -154,8 +160,8 @@ public class CommonsApplicationModule {
@Provides
@Named("direct_nearby_upload_prefs")
public JsonKvStore providesDirectNearbyUploadKvStore(Context context) {
return new JsonKvStore(context, "direct_nearby_upload_prefs");
public JsonKvStore providesDirectNearbyUploadKvStore(Context context, Gson gson) {
return new JsonKvStore(context, "direct_nearby_upload_prefs", gson);
}
/**

View file

@ -1,6 +1,7 @@
package fr.free.nrw.commons.di;
import android.content.Context;
import android.net.Uri;
import android.support.annotation.NonNull;
import com.google.gson.Gson;
@ -18,6 +19,8 @@ import fr.free.nrw.commons.BuildConfig;
import fr.free.nrw.commons.kvstore.BasicKvStore;
import fr.free.nrw.commons.mwapi.ApacheHttpClientMediaWikiApi;
import fr.free.nrw.commons.mwapi.MediaWikiApi;
import fr.free.nrw.commons.utils.UriDeserializer;
import fr.free.nrw.commons.utils.UriSerializer;
import okhttp3.Cache;
import okhttp3.HttpUrl;
import okhttp3.OkHttpClient;
@ -63,7 +66,10 @@ public class NetworkingModule {
@Provides
@Singleton
public Gson provideGson() {
return new GsonBuilder().create();
return new GsonBuilder()
.registerTypeAdapter(Uri.class, new UriSerializer())
.registerTypeAdapter(Uri.class, new UriDeserializer())
.create();
}
}

View file

@ -12,18 +12,21 @@ import java.util.Map;
import javax.annotation.Nullable;
public class JsonKvStore extends BasicKvStore {
private final Gson gson = new Gson();
private final Gson gson;
public JsonKvStore(Context context, String storeName) {
public JsonKvStore(Context context, String storeName, Gson gson) {
super(context, storeName);
this.gson = gson;
}
public JsonKvStore(Context context, String storeName, int version) {
public JsonKvStore(Context context, String storeName, int version, Gson gson) {
super(context, storeName, version);
this.gson = gson;
}
public JsonKvStore(Context context, String storeName, int version, boolean clearAllOnUpgrade) {
public JsonKvStore(Context context, String storeName, int version, boolean clearAllOnUpgrade, Gson gson) {
super(context, storeName, version, clearAllOnUpgrade);
this.gson = gson;
}
public <T> void putAllJsons(Map<String, T> jsonMap) {

View file

@ -896,7 +896,7 @@ public class ApacheHttpClientMediaWikiApi implements MediaWikiApi {
CustomApiResult result = api.upload(filename, file, dataLength, pageContents, editSummary, getCentralAuthToken(), getEditToken(), progressListener::onProgress);
Timber.wtf("Result: " + result.toString());
Timber.d("Result: %s", result.toString());
String resultStatus = result.getString("/api/upload/@result");

View file

@ -36,15 +36,15 @@ public class CustomApiResult {
this.evaluator = XPathFactory.newInstance().newXPath();
}
static CustomApiResult fromRequestBuilder(Http.HttpRequestBuilder builder, HttpClient client) throws IOException {
static CustomApiResult fromRequestBuilder(String requestIdentifier, Http.HttpRequestBuilder builder, HttpClient client) throws IOException {
try {
DocumentBuilder docBuilder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
Document doc = docBuilder.parse(builder.use(client).charset("utf-8").data("format", "xml").asResponse().getEntity().getContent());
printStringFromDocument(doc);
printStringFromDocument(requestIdentifier, doc);
return new CustomApiResult(doc);
} catch (ParserConfigurationException e) {
// I don't know wtf I can do about this on...
Timber.e(e, "Error occurred while parsing the response for method %s", requestIdentifier);
throw new RuntimeException(e);
} catch (IllegalStateException e) {
// So, this should never actually happen - since we assume MediaWiki always generates valid json
@ -52,14 +52,16 @@ public class CustomApiResult {
// Sooo... I can throw IOError
// Thanks Java, for making me spend significant time on shit that happens once in a bluemoon
// I surely am writing Nuclear Submarine controller code
Timber.e(e, "Error occurred while parsing the response for method %s", requestIdentifier);
throw new IOError(e);
} catch (SAXException e) {
// See Rant above
Timber.e(e, "Error occurred while parsing the response for method %s", requestIdentifier);
throw new IOError(e);
}
}
public static void printStringFromDocument(Document doc)
public static void printStringFromDocument(String requestIdentifier, Document doc)
{
try
{
@ -69,11 +71,11 @@ public class CustomApiResult {
TransformerFactory tf = TransformerFactory.newInstance();
Transformer transformer = tf.newTransformer();
transformer.transform(domSource, result);
Timber.d("API response is\n %s", writer.toString());
Timber.d("API response for method %s is\n %s", requestIdentifier, writer.toString());
}
catch(TransformerException ex)
{
Timber.d("Error occurred in transforming", ex);
Timber.e(ex, "Error occurred in transforming response for method %s", requestIdentifier);
}
}

View file

@ -140,6 +140,7 @@ public class CustomMwApi {
}
public CustomApiResult upload(String filename, InputStream file, long length, String text, String comment, String centralAuthToken, String token, ProgressListener uploadProgressListener) throws IOException {
Timber.d("Initiating upload for file %s", filename);
Http.HttpRequestBuilder builder = Http.multipart(apiURL)
.data("action", "upload")
.data("token", token)
@ -155,7 +156,7 @@ public class CustomMwApi {
builder.file("file", filename, file);
}
return CustomApiResult.fromRequestBuilder(builder, client);
return CustomApiResult.fromRequestBuilder("uploadFile", builder, client);
}
public void logout() throws IOException {
@ -174,7 +175,7 @@ public class CustomMwApi {
builder = Http.get(apiURL);
}
builder.data(params);
return CustomApiResult.fromRequestBuilder(builder, client);
return CustomApiResult.fromRequestBuilder(apiURL, builder, client);
}
}
;

View file

@ -34,6 +34,17 @@ public class UploadResult {
this.imageUrl = imageUrl;
}
@Override
public String toString() {
return "UploadResult{" +
"errorCode='" + errorCode + '\'' +
", resultStatus='" + resultStatus + '\'' +
", dateUploaded='" + dateUploaded.toString() + '\'' +
", imageUrl='" + imageUrl + '\'' +
", canonicalFilename='" + canonicalFilename + '\'' +
'}';
}
/**
* Gets uploaded date
* @return Upload date

View file

@ -12,7 +12,6 @@ import com.mapbox.mapboxsdk.annotations.Icon;
import com.mapbox.mapboxsdk.annotations.IconFactory;
import com.mapbox.mapboxsdk.geometry.LatLng;
import fr.free.nrw.commons.utils.UriDeserializer;
import fr.free.nrw.commons.utils.UriSerializer;
public class NearbyBaseMarker extends BaseMarkerOptions<NearbyMarker, NearbyBaseMarker> {
@ -33,10 +32,6 @@ public class NearbyBaseMarker extends BaseMarkerOptions<NearbyMarker, NearbyBase
}
private NearbyBaseMarker(Parcel in) {
Gson gson = new GsonBuilder()
.registerTypeAdapter(Uri.class, new UriDeserializer())
.create();
position(in.readParcelable(LatLng.class.getClassLoader()));
snippet(in.readString());
String iconId = in.readString();
@ -44,8 +39,7 @@ public class NearbyBaseMarker extends BaseMarkerOptions<NearbyMarker, NearbyBase
Icon icon = IconFactory.recreate(iconId, iconBitmap);
icon(icon);
title(in.readString());
String gsonString = in.readString();
place(gson.fromJson(gsonString, Place.class));
place(in.readParcelable(Place.class.getClassLoader()));
}
public NearbyBaseMarker place(Place place) {
@ -74,15 +68,11 @@ public class NearbyBaseMarker extends BaseMarkerOptions<NearbyMarker, NearbyBase
@Override
public void writeToParcel(Parcel dest, int flags) {
Gson gson = new GsonBuilder()
.registerTypeAdapter(Uri.class, new UriSerializer())
.create();
dest.writeParcelable(position, flags);
dest.writeString(snippet);
dest.writeString(icon.getId());
dest.writeParcelable(icon.getBitmap(), flags);
dest.writeString(title);
dest.writeString(gson.toJson(place));
dest.writeParcelable(place, 0);
}
}

View file

@ -74,6 +74,7 @@ public class NearbyFragment extends CommonsDaggerSupportFragment
@Inject
@Named("application_preferences")
BasicKvStore applicationKvStore;
@Inject Gson gson;
public NearbyMapFragment nearbyMapFragment;
private NearbyListFragment nearbyListFragment;
@ -293,9 +294,6 @@ public class NearbyFragment extends CommonsDaggerSupportFragment
progressBar.setVisibility(View.VISIBLE);
//TODO: This hack inserts curLatLng before populatePlaces is called (see #1440). Ideally a proper fix should be found
Gson gson = new GsonBuilder()
.registerTypeAdapter(Uri.class, new UriSerializer())
.create();
String gsonCurLatLng = gson.toJson(curLatLng);
bundle.clear();
bundle.putString("CurLatLng", gsonCurLatLng);
@ -313,9 +311,6 @@ public class NearbyFragment extends CommonsDaggerSupportFragment
} else if (locationChangeType
.equals(LOCATION_SLIGHTLY_CHANGED)) {
Gson gson = new GsonBuilder()
.registerTypeAdapter(Uri.class, new UriSerializer())
.create();
String gsonCurLatLng = gson.toJson(curLatLng);
bundle.putString("CurLatLng", gsonCurLatLng);
updateMapFragment(false,true, null, null);
@ -384,9 +379,6 @@ public class NearbyFragment extends CommonsDaggerSupportFragment
Timber.d("Populating nearby places");
List<Place> placeList = nearbyPlacesInfo.placeList;
LatLng[] boundaryCoordinates = nearbyPlacesInfo.boundaryCoordinates;
Gson gson = new GsonBuilder()
.registerTypeAdapter(Uri.class, new UriSerializer())
.create();
String gsonPlaceList = gson.toJson(placeList);
String gsonCurLatLng = gson.toJson(curLatLng);
String gsonBoundaryCoordinates = gson.toJson(boundaryCoordinates);

View file

@ -39,9 +39,6 @@ public class NearbyListFragment extends DaggerFragment {
}.getType();
private static final Type CUR_LAT_LNG_TYPE = new TypeToken<LatLng>() {
}.getType();
private static final Gson gson = new GsonBuilder()
.registerTypeAdapter(Uri.class, new UriDeserializer())
.create();
private NearbyAdapterFactory adapterFactory;
private RecyclerView recyclerView;
@ -49,6 +46,7 @@ public class NearbyListFragment extends DaggerFragment {
@Inject ContributionController controller;
@Inject @Named("direct_nearby_upload_prefs") JsonKvStore directKvStore;
@Inject @Named("default_preferences") BasicKvStore defaultKvStore;
@Inject Gson gson;
@Override
public void onCreate(Bundle savedInstanceState) {

View file

@ -134,8 +134,8 @@ public class NearbyMapFragment extends DaggerFragment {
@Inject @Named("direct_nearby_upload_prefs") JsonKvStore directKvStore;
@Inject @Named("default_preferences") BasicKvStore defaultKvStore;
@Inject BookmarkLocationsDao bookmarkLocationDao;
@Inject
ContributionController controller;
@Inject ContributionController controller;
@Inject Gson gson;
private static final double ZOOM_LEVEL = 14f;
@ -148,9 +148,6 @@ public class NearbyMapFragment extends DaggerFragment {
Timber.d("Nearby map fragment created");
Bundle bundle = this.getArguments();
Gson gson = new GsonBuilder()
.registerTypeAdapter(Uri.class, new UriDeserializer())
.create();
if (bundle != null) {
String gsonPlaceList = bundle.getString("PlaceList");
String gsonLatLng = bundle.getString("CurLatLng");
@ -220,9 +217,6 @@ public class NearbyMapFragment extends DaggerFragment {
public void updateMapSlightly() {
Timber.d("updateMapSlightly called, bundle is:"+ bundleForUpdates);
if (mapboxMap != null) {
Gson gson = new GsonBuilder()
.registerTypeAdapter(Uri.class, new UriDeserializer())
.create();
if (bundleForUpdates != null) {
String gsonLatLng = bundleForUpdates.getString("CurLatLng");
Type curLatLngType = new TypeToken<fr.free.nrw.commons.location.LatLng>() {}.getType();
@ -242,10 +236,6 @@ public class NearbyMapFragment extends DaggerFragment {
Timber.d("updateMapSignificantlyForCurrentLocation called, bundle is:"+ bundleForUpdates);
if (mapboxMap != null) {
if (bundleForUpdates != null) {
Gson gson = new GsonBuilder()
.registerTypeAdapter(Uri.class, new UriDeserializer())
.create();
String gsonPlaceList = bundleForUpdates.getString("PlaceList");
String gsonLatLng = bundleForUpdates.getString("CurLatLng");
String gsonBoundaryCoordinates = bundleForUpdates.getString("BoundaryCoord");

View file

@ -5,12 +5,14 @@ import android.content.ContentResolver;
import android.content.Context;
import android.database.Cursor;
import android.net.Uri;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import javax.inject.Inject;
import javax.inject.Named;
@ -19,20 +21,20 @@ import fr.free.nrw.commons.CommonsApplication;
import fr.free.nrw.commons.auth.SessionManager;
import fr.free.nrw.commons.contributions.Contribution;
import fr.free.nrw.commons.kvstore.BasicKvStore;
import fr.free.nrw.commons.mwapi.MediaWikiApi;
import fr.free.nrw.commons.nearby.Place;
import fr.free.nrw.commons.settings.Prefs;
import fr.free.nrw.commons.utils.ImageUtils;
import io.reactivex.Observable;
import io.reactivex.android.schedulers.AndroidSchedulers;
import io.reactivex.disposables.Disposable;
import io.reactivex.functions.Consumer;
import io.reactivex.schedulers.Schedulers;
import io.reactivex.subjects.BehaviorSubject;
import timber.log.Timber;
public class UploadModel {
private MediaWikiApi mwApi;
private static UploadItem DUMMY = new UploadItem(
Uri.EMPTY,
"",
@ -53,11 +55,9 @@ public class UploadModel {
private int currentStepIndex = 0;
private Context context;
private ContentResolver contentResolver;
private boolean useExtStorage;
private Disposable badImageSubscription;
private SessionManager sessionManager;
private Uri currentMediaUri;
private FileUtilsWrapper fileUtilsWrapper;
private FileProcessor fileProcessor;
private final ImageProcessingService imageProcessingService;
@ -67,7 +67,6 @@ public class UploadModel {
@Named("default_preferences") BasicKvStore basicKvStore,
@Named("licenses_by_name") Map<String, String> licensesByName,
Context context,
MediaWikiApi mwApi,
SessionManager sessionManager,
FileUtilsWrapper fileUtilsWrapper,
FileProcessor fileProcessor, ImageProcessingService imageProcessingService) {
@ -76,14 +75,11 @@ public class UploadModel {
this.license = basicKvStore.getString(Prefs.DEFAULT_LICENSE, Prefs.Licenses.CC_BY_SA_3);
this.licensesByName = licensesByName;
this.context = context;
this.mwApi = mwApi;
this.contentResolver = context.getContentResolver();
this.sessionManager = sessionManager;
this.fileUtilsWrapper = fileUtilsWrapper;
this.fileProcessor = fileProcessor;
useExtStorage = this.basicKvStore.getBoolean("useExternalStorage", false);
this.imageProcessingService = imageProcessingService;
useExtStorage = this.basicKvStore.getBoolean("useExternalStorage", false);
}
@SuppressLint("CheckResult")
@ -96,24 +92,30 @@ public class UploadModel {
return Observable.fromIterable(mediaUris)
.map(mediaUri -> {
if (mediaUri == null || mediaUri.getPath() == null) {
return null;
}
String filePath = mediaUri.getPath();
long fileCreatedDate = getFileCreatedDate(currentMediaUri);
String fileExt = fileUtilsWrapper.getFileExt(filePath);
GPSExtractor gpsExtractor = fileProcessor.processFileCoordinates(similarImageInterface);
fileProcessor.initFileDetails(filePath, context.getContentResolver());
UploadItem item = new UploadItem(mediaUri, mimeType, source, gpsExtractor,
fileExt, place.getWikiDataEntityId(), fileCreatedDate);
imageProcessingService.checkImageQuality(place, filePath)
UploadItem item = getUploadItem(mimeType, place, source, similarImageInterface, mediaUri);
imageProcessingService.checkImageQuality(place, mediaUri.getPath())
.subscribeOn(Schedulers.computation())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(item.imageQuality::onNext, Timber::e);
return item;
});
}
@NonNull
private UploadItem getUploadItem(String mimeType,
Place place,
String source,
SimilarImageInterface similarImageInterface,
Uri mediaUri) {
fileProcessor
.initFileDetails(Objects.requireNonNull(mediaUri.getPath()), context.getContentResolver());
long fileCreatedDate = getFileCreatedDate(mediaUri);
String fileExt = fileUtilsWrapper.getFileExt(mediaUri.getPath());
GPSExtractor gpsExtractor = fileProcessor.processFileCoordinates(similarImageInterface);
return new UploadItem(mediaUri, mimeType, source, gpsExtractor,
fileExt, place, fileCreatedDate);
}
void onItemsProcessed(Place place, List<UploadItem> uploadItems) {
items = uploadItems;
if (items.isEmpty()) {
@ -314,7 +316,9 @@ public class UploadModel {
Description.formatList(item.descriptions), -1,
null, null, sessionManager.getAuthorName(),
CommonsApplication.DEFAULT_EDIT_SUMMARY, item.gpsCoords.getCoords());
contribution.setWikiDataEntityId(item.wikidataEntityId);
if (item.place != null) {
contribution.setWikiDataEntityId(item.place.getWikiDataEntityId());
}
contribution.setCategories(categoryStringList);
contribution.setTag("mimeType", item.mimeType);
contribution.setSource(item.source);
@ -358,17 +362,17 @@ public class UploadModel {
public BehaviorSubject<Integer> imageQuality;
Title title;
List<Description> descriptions;
public String wikidataEntityId;
public Place place;
public boolean visited;
public boolean error;
public long createdTimestamp;
@SuppressLint("CheckResult")
UploadItem(Uri mediaUri, String mimeType, String source, GPSExtractor gpsCoords, String fileExt, @Nullable String wikidataEntityId, long createdTimestamp) {
UploadItem(Uri mediaUri, String mimeType, String source, GPSExtractor gpsCoords, String fileExt, @Nullable Place place, long createdTimestamp) {
title = new Title();
descriptions = new ArrayList<>();
descriptions.add(new Description());
this.wikidataEntityId = wikidataEntityId;
this.place = place;
this.mediaUri = mediaUri;
this.mimeType = mimeType;
this.source = source;

View file

@ -90,8 +90,7 @@ public class UploadPresenter {
updateCards();
updateLicenses();
updateContent();
if (uploadModel.isShowingItem())
uploadModel.subscribeBadPicture(this::handleBadPicture);
uploadModel.subscribeBadPicture(this::handleBadPicture);
}
/**

View file

@ -12,21 +12,13 @@ import static fr.free.nrw.commons.contributions.ContributionController.NEARBY_GA
public class IntentUtils {
/**
* Check if the intent should be handled by nearby list or map fragment
*/
public static boolean shouldNearbyHandle(int requestCode, int resultCode, Intent data) {
return resultCode == Activity.RESULT_OK
&& (requestCode == NEARBY_CAMERA_UPLOAD_REQUEST_CODE || requestCode == NEARBY_GALLERY_UPLOAD_REQUEST_CODE)
&& data != null;
}
/**
* Check if the intent should be handled by contributions list fragment
*/
public static boolean shouldContributionsListHandle(int requestCode, int resultCode, Intent data) {
public static boolean shouldContributionsHandle(int requestCode, int resultCode, Intent data) {
return resultCode == Activity.RESULT_OK
&& (requestCode == GALLERY_UPLOAD_REQUEST_CODE || requestCode == CAMERA_UPLOAD_REQUEST_CODE)
&& (requestCode == GALLERY_UPLOAD_REQUEST_CODE || requestCode == CAMERA_UPLOAD_REQUEST_CODE
|| requestCode == NEARBY_CAMERA_UPLOAD_REQUEST_CODE || requestCode == NEARBY_GALLERY_UPLOAD_REQUEST_CODE)
&& data != null;
}

View file

@ -59,7 +59,7 @@ class MockCommonsApplicationModule(appContext: Context) : CommonsApplicationModu
override fun provideModificationContentProviderClient(context: Context?): ContentProviderClient = modificationClient
override fun providesDirectNearbyUploadKvStore(context: Context?): JsonKvStore = uploadPrefs
override fun providesDirectNearbyUploadKvStore(context: Context?, gson: Gson): JsonKvStore = uploadPrefs
override fun providesAccountUtil(context: Context): AccountUtil = accountUtil