-
-
Notifications
You must be signed in to change notification settings - Fork 735
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
Changes from all commits
c83fae1
3e55ffb
d751e2e
65934bf
cc047f6
ff0064b
e86df83
df31827
6dea925
605e8a2
0f5e5af
70516c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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) { | ||
ParsePlugins.initialize(configuration.context, configuration); | ||
} else { | ||
ParsePlugins.set(parsePlugins); | ||
} | ||
|
||
try { | ||
ParseRESTCommand.server = new URL(configuration.server); | ||
|
@@ -311,6 +319,8 @@ public Void then(Task<Void> task) throws Exception { | |
} | ||
|
||
static void destroy() { | ||
ParseObject.unregisterParseSubclasses(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -322,6 +332,8 @@ static void destroy() { | |
|
||
ParseCorePlugins.getInstance().reset(); | ||
ParsePlugins.reset(); | ||
|
||
setLocalDatastore(null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For tests, as Parse.destroy() is only used in tests. |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
*/ | ||
package com.parse; | ||
|
||
import android.Manifest; | ||
import android.os.Parcel; | ||
|
||
import org.json.JSONObject; | ||
|
@@ -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; | ||
|
@@ -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 | ||
|
@@ -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 | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.