Skip to content

Commit

Permalink
fix(DataStore): should not crash on missing version metadata (#2849)
Browse files Browse the repository at this point in the history 
Co-authored-by: Tyler Roach <[email protected]>
  • Loading branch information
@lawmicha @tylerjroach
lawmicha and tylerjroach authored Jun 17, 2024
1 parent f2fd6f7 commit 04178b1
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 61 deletions.
8 changes: 4 additions & 4 deletions aws-datastore/src/main/java/com/amplifyframework/datastore/appsync/AppSync.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ <T extends Model> Cancelable create(
<T extends Model> Cancelable update(
@NonNull T model,
@NonNull ModelSchema modelSchema,
@NonNull Integer version,
@Nullable Integer version,
@NonNull Consumer<GraphQLResponse<ModelWithMetadata<T>>> onResponse,
@NonNull Consumer<DataStoreException> onFailure
);
Expand All @@ -132,7 +132,7 @@ <T extends Model> Cancelable update(
<T extends Model> Cancelable update(
@NonNull T model,
@NonNull ModelSchema modelSchema,
@NonNull Integer version,
@Nullable Integer version,
@NonNull QueryPredicate predicate,
@NonNull Consumer<GraphQLResponse<ModelWithMetadata<T>>> onResponse,
@NonNull Consumer<DataStoreException> onFailure
Expand All @@ -152,7 +152,7 @@ <T extends Model> Cancelable update(
<T extends Model> Cancelable delete(
@NonNull T model,
@NonNull ModelSchema modelSchema,
@NonNull Integer version,
@Nullable Integer version,
@NonNull Consumer<GraphQLResponse<ModelWithMetadata<T>>> onResponse,
@NonNull Consumer<DataStoreException> onFailure
);
Expand All @@ -172,7 +172,7 @@ <T extends Model> Cancelable delete(
<T extends Model> Cancelable delete(
@NonNull T model,
@NonNull ModelSchema modelSchema,
@NonNull Integer version,
@Nullable Integer version,
@NonNull QueryPredicate predicate,
@NonNull Consumer<GraphQLResponse<ModelWithMetadata<T>>> onResponse,
@NonNull Consumer<DataStoreException> onFailure
Expand Down
8 changes: 4 additions & 4 deletions aws-datastore/src/main/java/com/amplifyframework/datastore/appsync/AppSyncClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public <T extends Model> Cancelable create(
public <T extends Model> Cancelable update(
@NonNull T model,
@NonNull ModelSchema modelSchema,
@NonNull Integer version,
@Nullable Integer version,
@NonNull Consumer<GraphQLResponse<ModelWithMetadata<T>>> onResponse,
@NonNull Consumer<DataStoreException> onFailure) {
return update(model, modelSchema, version, QueryPredicates.all(), onResponse, onFailure);
Expand All @@ -170,7 +170,7 @@ public <T extends Model> Cancelable update(
public <T extends Model> Cancelable update(
@NonNull T model,
@NonNull ModelSchema modelSchema,
@NonNull Integer version,
@Nullable Integer version,
@NonNull QueryPredicate predicate,
@NonNull Consumer<GraphQLResponse<ModelWithMetadata<T>>> onResponse,
@NonNull Consumer<DataStoreException> onFailure) {
Expand Down Expand Up @@ -207,7 +207,7 @@ public <T extends Model> Cancelable update(
public <T extends Model> Cancelable delete(
@NonNull T model,
@NonNull ModelSchema modelSchema,
@NonNull Integer version,
@Nullable Integer version,
@NonNull Consumer<GraphQLResponse<ModelWithMetadata<T>>> onResponse,
@NonNull Consumer<DataStoreException> onFailure) {
return delete(model, modelSchema, version, QueryPredicates.all(), onResponse, onFailure);
Expand All @@ -218,7 +218,7 @@ public <T extends Model> Cancelable delete(
public <T extends Model> Cancelable delete(
@NonNull T model,
@NonNull ModelSchema modelSchema,
@NonNull Integer version,
@Nullable Integer version,
@NonNull QueryPredicate predicate,
@NonNull Consumer<GraphQLResponse<ModelWithMetadata<T>>> onResponse,
@NonNull Consumer<DataStoreException> onFailure) {
Expand Down
8 changes: 6 additions & 2 deletions ...datastore/src/main/java/com/amplifyframework/datastore/appsync/AppSyncRequestFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,9 @@ static <M extends Model> AppSyncGraphQLRequest<ModelWithMetadata<M>> buildDeleti
throws DataStoreException {
try {
Map<String, Object> inputMap = new HashMap<>();
inputMap.put("_version", version);
if (version != null) {
inputMap.put("_version", version);
}
inputMap.putAll(GraphQLRequestHelper.getDeleteMutationInputMap(schema, model));
return buildMutation(schema, inputMap, predicate, MutationType.DELETE, strategyType);
} catch (AmplifyException amplifyException) {
Expand All @@ -184,7 +186,9 @@ static <M extends Model> AppSyncGraphQLRequest<ModelWithMetadata<M>> buildUpdate
AuthModeStrategyType strategyType) throws DataStoreException {
try {
Map<String, Object> inputMap = new HashMap<>();
inputMap.put("_version", version);
if (version != null) {
inputMap.put("_version", version);
}
inputMap.putAll(GraphQLRequestHelper.getMapOfFieldNameAndValues(schema, model, MutationType.UPDATE));
return buildMutation(schema, inputMap, predicate, MutationType.UPDATE, strategyType);
} catch (AmplifyException amplifyException) {
Expand Down
5 changes: 3 additions & 2 deletions aws-datastore/src/main/java/com/amplifyframework/datastore/syncengine/MutationProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,8 @@ private <T extends Model> Single<ModelWithMetadata<T>> update(PendingMutation<T>
this.schemaRegistry.getModelSchemaForModelClass(updatedItem.getModelName());
return versionRepository.findModelVersion(updatedItem).flatMap(version ->
publishWithStrategy(mutation, (model, onSuccess, onError) ->
appSync.update(model, updatedItemSchema, version, mutation.getPredicate(), onSuccess, onError)
appSync.update(
model, updatedItemSchema, version.orElse(null), mutation.getPredicate(), onSuccess, onError)
)
);
}
Expand All @@ -312,7 +313,7 @@ private <T extends Model> Single<ModelWithMetadata<T>> delete(PendingMutation<T>
return versionRepository.findModelVersion(deletedItem).flatMap(version ->
publishWithStrategy(mutation, (model, onSuccess, onError) ->
appSync.delete(
deletedItem, deletedItemSchema, version, mutation.getPredicate(), onSuccess, onError
deletedItem, deletedItemSchema, version.orElse(null), mutation.getPredicate(), onSuccess, onError
)
)
);
Expand Down
24 changes: 10 additions & 14 deletions aws-datastore/src/main/java/com/amplifyframework/datastore/syncengine/VersionRepository.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import com.amplifyframework.datastore.extensions.getMetadataSQLitePrimaryKey
import com.amplifyframework.datastore.storage.LocalStorageAdapter
import io.reactivex.rxjava3.core.Single
import io.reactivex.rxjava3.core.SingleEmitter
import java.util.Optional
import kotlin.coroutines.resume
import kotlin.coroutines.resumeWithException
import kotlin.coroutines.suspendCoroutine
Expand All @@ -48,8 +49,8 @@ internal class VersionRepository(private val localStorageAdapter: LocalStorageAd
* @param <T> Type of model
* @return Current version known locally
</T> */
fun <T : Model> findModelVersion(model: T): Single<Int> {
return Single.create { emitter: SingleEmitter<Int> ->
fun <T : Model> findModelVersion(model: T): Single<Optional<Int>> {
return Single.create { emitter: SingleEmitter<Optional<Int>> ->
// The ModelMetadata for the model uses the same ID as an identifier.
localStorageAdapter.query(
ModelMetadata::class.java,
Expand Down Expand Up @@ -129,32 +130,27 @@ internal class VersionRepository(private val localStorageAdapter: LocalStorageAd
* @param metadataIterator An iterator of ModelMetadata; the metadata is associated with the provided model
* @param <T> The type of model
* @return The version of the model, if available
* @throws DataStoreException If there is no version for the model, or if the version cannot be obtained
* @throws DataStoreException If there is no metadata for the model
</T> */
@Throws(DataStoreException::class)
private fun <T : Model> extractVersion(
model: T,
metadataIterator: Iterator<ModelMetadata>
): Int {
): Optional<Int> {
val results: MutableList<ModelMetadata> =
ArrayList()
while (metadataIterator.hasNext()) {
results.add(metadataIterator.next())
}

// There should be only one metadata for the model....
if (results.size != 1) {
throw DataStoreException(
"Wanted 1 metadata for item with id = " + model.primaryKeyString + ", but had " + results.size +
".",
"This is likely a bug. please report to AWS."
LOG.warn(
"Wanted 1 metadata for item with id = " + model.primaryKeyString + ", but had " + results.size + "."
)
return Optional.empty()
} else {
return Optional.ofNullable(results[0].version)
}
return results[0].version
?: throw DataStoreException(
"Metadata for item with id = " + model.primaryKeyString + " had null version.",
"This is likely a bug. Please report to AWS."
)
}

companion object {
Expand Down
53 changes: 18 additions & 35 deletions ...atastore/src/test/java/com/amplifyframework/datastore/syncengine/VersionRepositoryTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import com.amplifyframework.datastore.appsync.ModelWithMetadata
import com.amplifyframework.datastore.storage.InMemoryStorageAdapter
import com.amplifyframework.datastore.storage.SynchronousStorageAdapter
import com.amplifyframework.testmodels.commentsblog.BlogOwner
import java.util.Locale
import java.util.Optional
import java.util.Random
import java.util.concurrent.TimeUnit
import kotlin.time.Duration.Companion.seconds
Expand Down Expand Up @@ -55,12 +55,12 @@ class VersionRepositoryTest {

/**
* When you try to get a model version, but there's no metadata for that model,
* this should fail with an [DataStoreException].
* this should return empty.
* @throws InterruptedException If interrupted while awaiting terminal result in test observer
*/
@Test
@Throws(InterruptedException::class)
fun emitsErrorForNoMetadataInRepo() {
fun emitsSuccessWithEmptyValueForNoMetadataInRepo() {
// Arrange: no metadata is in the repo.
val blogOwner = BlogOwner.builder()
.name("Jameson Williams")
Expand All @@ -73,33 +73,22 @@ class VersionRepositoryTest {
val observer = versionRepository.findModelVersion(blogOwner).test()
assertTrue(observer.await(REASONABLE_WAIT_TIME, TimeUnit.MILLISECONDS))

// Assert: this failed. There was no version available.
observer.assertError { error: Throwable ->
if (error !is DataStoreException) {
return@assertError false
}
val expectedMessage = String.format(
Locale.US,
"Wanted 1 metadata for item with id = %s, but had 0.",
blogOwner.id
)
expectedMessage == error.message
}
// Assert: we got a empty version
observer
.assertNoErrors()
.assertComplete()
.assertValue(Optional.empty())
}

/**
* When you try to get the version for a model, and there is metadata for the model
* in the DataStore, BUT the version info is not populated, this should return an
* [DataStoreException].
* @throws DataStoreException
* NOT EXPECTED. This happens on failure to arrange data before test action.
* The expected DataStoreException is communicated via callback, not thrown
* on the calling thread. It's a different thing than this.
* empty optional.
* @throws InterruptedException If interrupted while awaiting terminal result in test observer
*/
@Test
@Throws(DataStoreException::class, InterruptedException::class)
fun emitsErrorWhenMetadataHasNullVersion() {
fun emitsSuccessWithEmptyValueWhenMetadataHasNullVersion() {
// Arrange a model an metadata into the store, but the metadtaa doesn't contain a valid version
val blogOwner = BlogOwner.builder()
.name("Jameson")
Expand All @@ -114,18 +103,11 @@ class VersionRepositoryTest {
val observer = versionRepository.findModelVersion(blogOwner).test()
assertTrue(observer.await(REASONABLE_WAIT_TIME, TimeUnit.MILLISECONDS))

// Assert: the single emitted a DataStoreException.
observer.assertError { error: Throwable ->
if (error !is DataStoreException) {
return@assertError false
}
val expectedMessage = String.format(
Locale.US,
"Metadata for item with id = %s had null version.",
blogOwner.id
)
expectedMessage == error.message
}
// Assert: we got a empty version
observer
.assertNoErrors()
.assertComplete()
.assertValue(Optional.empty())
}

/**
Expand All @@ -142,12 +124,13 @@ class VersionRepositoryTest {
.name("Jameson")
.build()
val maxRandomVersion = 1000
val expectedVersion = Random().nextInt(maxRandomVersion)
val randomVersion = Random().nextInt(maxRandomVersion)
val expectedVersion = Optional.ofNullable(randomVersion)
storageAdapter.save(
ModelMetadata(
owner.modelName + "|" + owner.id,
false,
expectedVersion,
randomVersion,
Temporal.Timestamp.now()
)
)
Expand Down

0 comments on commit 04178b1

Please sign in to comment.