Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save eventually issue #831

Merged
merged 12 commits into from
Jul 20, 2018
Merged
14 changes: 13 additions & 1 deletion Parse/src/main/java/com/parse/Parse.java
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,10 @@ public static boolean isLocalDatastoreEnabled() {
* @param configuration The configuration for your application.
*/
public static void initialize(Configuration configuration) {
initialize(configuration, null);
}

static void initialize(Configuration configuration, ParsePlugins parsePlugins) {
if (isInitialized()) {
PLog.w(TAG, "Parse is already initialized");
return;
Expand All @@ -256,7 +260,11 @@ public static void initialize(Configuration configuration) {
// isLocalDataStoreEnabled() to perform additional behavior.
isLocalDatastoreEnabled = configuration.localDataStoreEnabled;

ParsePlugins.initialize(configuration.context, configuration);
if (parsePlugins == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dangtz Can you explain what the reasoning is for these changes here?

Copy link
Contributor Author

@cjosepha cjosepha Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes allow to initialize Parse with a mock ParsePlugins object, for test purpose.
The mock ParsePlugins object is needed to reproduce the issue, and calling Parse.initialize() is also needed to reproduce it. These changes are the solution to have both.

ParsePlugins.initialize(configuration.context, configuration);
} else {
ParsePlugins.set(parsePlugins);
}

try {
ParseRESTCommand.server = new URL(configuration.server);
Expand Down Expand Up @@ -311,6 +319,8 @@ public Void then(Task<Void> task) throws Exception {
}

static void destroy() {
ParseObject.unregisterParseSubclasses();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this causing problems? Curious if this was added to help the tests pass, or if it was related to the bug?

Copy link
Contributor Author

@cjosepha cjosepha Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not causing the problem, and yes it's to help test teardown here by aggregating deallocations. Note that Parse.destroy() was not used anywhere in the project before my changes.


ParseEventuallyQueue queue;
synchronized (MUTEX) {
queue = eventuallyQueue;
Expand All @@ -322,6 +332,8 @@ static void destroy() {

ParseCorePlugins.getInstance().reset();
ParsePlugins.reset();

setLocalDatastore(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here, is this for the tests, or related to the bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For tests, as Parse.destroy() is only used in tests.

}

/**
Expand Down
40 changes: 26 additions & 14 deletions Parse/src/main/java/com/parse/ParseObject.java
Original file line number Diff line number Diff line change
Expand Up @@ -1420,6 +1420,20 @@ private ParseRESTObjectCommand currentSaveEventuallyCommand(
final ParseObject.State result, final ParseOperationSet operationsBeforeSave) {
Task<Void> task = Task.forResult(null);

/*
* If this object is in the offline store, then we need to make sure that we pull in any dirty
* changes it may have before merging the server data into it.
*/
final OfflineStore store = Parse.getLocalDatastore();
if (store != null) {
task = task.onSuccessTask(new Continuation<Void, Task<Void>>() {
@Override
public Task<Void> then(Task<Void> task) throws Exception {
return store.fetchLocallyAsync(ParseObject.this).makeVoid();
}
});
}

final boolean success = result != null;
synchronized (mutex) {
// Find operationsBeforeSave in the queue so that we can remove it and move to the next
Expand All @@ -1433,24 +1447,22 @@ private ParseRESTObjectCommand currentSaveEventuallyCommand(
// Merge the data from the failed save into the next save.
ParseOperationSet nextOperation = opIterator.next();
nextOperation.mergeFrom(operationsBeforeSave);
if (store != null) {
task = task.continueWithTask(new Continuation<Void, Task<Void>>() {
@Override
public Task<Void> then(Task<Void> task) throws Exception {
if (task.isFaulted()) {
return Task.forResult(null);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to not propagate the exception in case fetching the store here fails. If exception was propagated, existing ParseInstallationTests would fail (testMissingRequiredFieldWhenSaveAsync and testObjectNotFoundWhenSaveAsync) because they don't create the right conditions for the fetch to work (a CACHE_MISS exception is then thrown). Maybe we should update those tests instead, by creating the right conditions, and let the exception propagate by using onSuccessTask at line 1451 as on my previous commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think it is best to update the tests to make them have the right conditions for success/expected failure rather than modifying the library to work with the expectations of the tests. I think we can go ahead and merge this in though, as it fixes the issue at hand, and keep this in mind for potentially another pull request if that is cool with you.

} else {
return store.updateDataForObjectAsync(ParseObject.this);
}
}
});
}
return task;
}
}

/*
* If this object is in the offline store, then we need to make sure that we pull in any dirty
* changes it may have before merging the server data into it.
*/
final OfflineStore store = Parse.getLocalDatastore();
if (store != null) {
task = task.onSuccessTask(new Continuation<Void, Task<Void>>() {
@Override
public Task<Void> then(Task<Void> task) throws Exception {
return store.fetchLocallyAsync(ParseObject.this).makeVoid();
}
});
}

// fetchLocallyAsync will return an error if this object isn't in the LDS yet and that's ok
task = task.continueWith(new Continuation<Void, Void>() {
@Override
Expand Down
4 changes: 2 additions & 2 deletions Parse/src/test/java/com/parse/ParseInstallationTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ public void testMissingRequiredFieldWhenSaveAsync() throws Exception {
ParseInstallation installation = ParseInstallation.getCurrentInstallation();
assertNotNull(installation);
installation.put("key", "value");
installation.saveAsync(sessionToken, toAwait);
ParseTaskUtils.wait(installation.saveAsync(sessionToken, toAwait));
verify(controller).getAsync();
verify(objController, times(2)).saveAsync(
any(ParseObject.State.class),
Expand Down Expand Up @@ -187,7 +187,7 @@ public void testObjectNotFoundWhenSaveAsync() throws Exception {
assertNotNull(installation);
installation.setState(state);
installation.put("key", "value");
installation.saveAsync(sessionToken, toAwait);
ParseTaskUtils.wait(installation.saveAsync(sessionToken, toAwait));

verify(controller).getAsync();
verify(objController, times(2)).saveAsync(
Expand Down
53 changes: 46 additions & 7 deletions Parse/src/test/java/com/parse/ParseTestUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,15 @@
*/
package com.parse;

import android.content.Context;

import com.parse.http.ParseHttpRequest;
import com.parse.http.ParseHttpResponse;

import org.json.JSONObject;

import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;

import bolts.Task;
Expand Down Expand Up @@ -41,15 +44,51 @@ public static void setTestParseUser() {

public static ParseHttpClient mockParseHttpClientWithResponse(
JSONObject content, int statusCode, String reasonPhrase) throws IOException {
ParseHttpClient client = mock(ParseHttpClient.class);
updateMockParseHttpClientWithResponse(client, content, statusCode, reasonPhrase);
return client;
}

static void updateMockParseHttpClientWithResponse(
ParseHttpClient client, JSONObject content, int statusCode, String reasonPhrase) throws IOException {
byte[] contentBytes = content.toString().getBytes();
ParseHttpResponse response = new ParseHttpResponse.Builder()
.setContent(new ByteArrayInputStream(contentBytes))
.setStatusCode(statusCode)
.setTotalSize(contentBytes.length)
.setContentType("application/json")
.build();
ParseHttpClient client = mock(ParseHttpClient.class);
.setContent(new ByteArrayInputStream(contentBytes))
.setStatusCode(statusCode)
.setTotalSize(contentBytes.length)
.setContentType("application/json")
.build();
when(client.execute(any(ParseHttpRequest.class))).thenReturn(response);
return client;
}

static ParsePlugins mockParsePlugins(Parse.Configuration configuration) {
ParsePlugins parsePlugins = mock(ParsePlugins.class);
when(parsePlugins.applicationId()).thenReturn(configuration.applicationId);
when(parsePlugins.clientKey()).thenReturn(configuration.clientKey);
when(parsePlugins.configuration()).thenReturn(configuration);
Context applicationContext = configuration.context.getApplicationContext();
when(parsePlugins.applicationContext()).thenReturn(applicationContext);
File parseDir = createFileDir(applicationContext.getDir("Parse", Context.MODE_PRIVATE));
when(parsePlugins.installationId())
.thenReturn(
new InstallationId(new File(parseDir, "installationId")));
when(parsePlugins.getParseDir())
.thenReturn(parseDir);
when(parsePlugins.getCacheDir())
.thenReturn(createFileDir(
new File(applicationContext.getCacheDir(), "com.parse")));
when(parsePlugins.getFilesDir())
.thenReturn(createFileDir(
new File(applicationContext.getFilesDir(), "com.parse")));
return parsePlugins;
}

private static File createFileDir(File file) {
if (!file.exists()) {
if (!file.mkdirs()) {
return file;
}
}
return file;
}
}
130 changes: 127 additions & 3 deletions Parse/src/test/java/com/parse/ParseUserTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/
package com.parse;

import android.Manifest;
import android.os.Parcel;

import org.json.JSONObject;
Expand All @@ -20,14 +21,20 @@
import org.mockito.ArgumentCaptor;
import org.mockito.Matchers;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.RuntimeEnvironment;
import org.robolectric.Shadows;
import org.robolectric.annotation.Config;

import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;

import bolts.Capture;
import bolts.Continuation;
import bolts.Task;

import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -68,9 +75,19 @@ public void setUp() throws Exception {
@After
public void tearDown() throws Exception {
super.tearDown();
ParseObject.unregisterSubclass(ParseUser.class);
ParseObject.unregisterSubclass(ParseSession.class);
Parse.disableLocalDatastore();
if (ParsePlugins.get() != null) {
ParseCurrentInstallationController installationController =
ParseCorePlugins.getInstance().getCurrentInstallationController();
if (installationController != null) {
installationController.clearFromDisk();
}
ParseCurrentUserController userController =
ParseCorePlugins.getInstance().getCurrentUserController();
if (userController != null) {
userController.clearFromDisk();
}
}
Parse.destroy();
}

@Test
Expand Down Expand Up @@ -1541,4 +1558,111 @@ private static void setLazy(ParseUser user) {
anonymousAuthData.put("anonymousToken", "anonymousTest");
user.putAuthData(ParseAnonymousUtils.AUTH_TYPE, anonymousAuthData);
}

//region testSaveEventuallyWhenServerError

@Test
public void testSaveEventuallyWhenServerError() throws Exception {
Shadows.shadowOf(RuntimeEnvironment.application)
.grantPermissions(Manifest.permission.ACCESS_NETWORK_STATE);
Parse.Configuration configuration =
new Parse.Configuration.Builder(RuntimeEnvironment.application)
.applicationId(BuildConfig.APPLICATION_ID)
.server("https://api.parse.com/1")
.enableLocalDataStore()
.build();
ParsePlugins plugins = ParseTestUtils.mockParsePlugins(configuration);
JSONObject mockResponse = new JSONObject();
mockResponse.put("objectId", "objectId");
mockResponse.put("email", "[email protected]");
mockResponse.put("username", "username");
mockResponse.put("sessionToken", "r:sessionToken");
mockResponse.put("createdAt", ParseDateFormat.getInstance().format(new Date(1000)));
mockResponse.put("updatedAt", ParseDateFormat.getInstance().format(new Date(2000)));
ParseHttpClient restClient = ParseTestUtils.mockParseHttpClientWithResponse(
mockResponse,200, "OK");
when(plugins.restClient())
.thenReturn(restClient);
Parse.initialize(configuration, plugins);

ParseUser user = ParseUser.logIn("username", "password");
assertFalse(user.isDirty());

user.put("field", "data");
assertTrue(user.isDirty());

mockResponse = new JSONObject();
mockResponse.put("updatedAt", ParseDateFormat.getInstance().format(new Date(3000)));
ParseTestUtils.updateMockParseHttpClientWithResponse(
restClient, mockResponse, 200, "OK");

final CountDownLatch saveCountDown1 = new CountDownLatch(1);
final Capture<Exception> exceptionCapture = new Capture<>();
user.saveInBackground().continueWith(new Continuation<Void, Void>() {
@Override
public Void then(Task<Void> task) throws Exception {
exceptionCapture.set(task.getError());
saveCountDown1.countDown();
return null;
}
});
assertTrue(saveCountDown1.await(5, TimeUnit.SECONDS));
assertNull(exceptionCapture.get());
assertFalse(user.isDirty());

user.put("field", "other data");
assertTrue(user.isDirty());

mockResponse = new JSONObject();
mockResponse.put("error", "Save is not allowed");
mockResponse.put("code", 141);
ParseTestUtils.updateMockParseHttpClientWithResponse(
restClient, mockResponse, 400, "Bad Request");

final CountDownLatch saveEventuallyCountDown = new CountDownLatch(1);
user.saveEventually().continueWith(new Continuation<Void, Void>() {
@Override
public Void then(Task<Void> task) throws Exception {
exceptionCapture.set(task.getError());
saveEventuallyCountDown.countDown();
return null;
}
});
assertTrue(saveEventuallyCountDown.await(5, TimeUnit.SECONDS));
assertTrue(exceptionCapture.get() instanceof ParseException);
assertEquals(ParseException.SCRIPT_ERROR, ((ParseException)exceptionCapture.get()).getCode());
assertEquals("Save is not allowed", exceptionCapture.get().getMessage());
assertTrue(user.isDirty());

// Simulate reboot
Parse.destroy();
Parse.initialize(configuration, plugins);

user = ParseUser.getCurrentUser();
assertTrue(user.isDirty());

assertEquals("other data", user.get("field"));
user.put("field", "another data");

mockResponse = new JSONObject();
mockResponse.put("updatedAt", ParseDateFormat.getInstance().format(new Date(4000)));
ParseTestUtils.updateMockParseHttpClientWithResponse(
restClient, mockResponse, 200, "OK");

final CountDownLatch saveCountDown2 = new CountDownLatch(1);
user.saveInBackground().continueWith(new Continuation<Void, Void>() {
@Override
public Void then(Task<Void> task) throws Exception {
exceptionCapture.set(task.getError());
saveCountDown2.countDown();
return null;
}
});

assertTrue(saveCountDown2.await(5, TimeUnit.SECONDS));
assertNull(exceptionCapture.get());
assertFalse(user.isDirty());
}

//endregion
}