Skip to content

Commit 0144292

Browse files
raphkimraphkim
authored
fix(datastore): fix crash when deleting model with many children (#1121)
1 parent 851a1c1 commit 0144292

File tree

5 files changed

+266
-48
lines changed

5 files changed

+266
-48
lines changed

‎aws-datastore/src/androidTest/java/com/amplifyframework/datastore/storage/sqlite/SQLiteStorageAdapterDeleteTest.java‎

Lines changed: 53 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
package com.amplifyframework.datastore.storage.sqlite;
1717

18+
import com.amplifyframework.core.model.Model;
1819
import com.amplifyframework.core.model.query.predicate.QueryPredicate;
1920
import com.amplifyframework.core.model.query.predicate.QueryPredicates;
2021
import com.amplifyframework.datastore.DataStoreException;
@@ -37,6 +38,8 @@
3738
import java.util.List;
3839
import java.util.Set;
3940

41+
import io.reactivex.rxjava3.observers.TestObserver;
42+
4043
import static org.junit.Assert.assertEquals;
4144
import static org.junit.Assert.assertTrue;
4245

@@ -127,19 +130,19 @@ public void deleteModelCascades() throws DataStoreException {
127130
}
128131

129132
// Observe deletions
130-
Set<String> deleted = new HashSet<>();
131-
adapter.observe()
133+
TestObserver<String> deleteObserver = adapter.observe()
132134
.filter(change -> StorageItemChange.Type.DELETE.equals(change.type()))
133135
.map(StorageItemChange::item)
134-
.subscribe(model -> deleted.add(model.getId()));
136+
.map(Model::getId)
137+
.test();
135138

136139
// Triggers a delete.
137140
// Deletes every saved model to prevent foreign key constraint violation
138141
adapter.delete(ownerModel);
139142

140143
// Assert that cascaded deletions are observed.
141-
assertEquals(13, deleted.size());
142-
assertEquals(expected, deleted);
144+
deleteObserver.assertValueCount(13);
145+
assertEquals(expected, new HashSet<>(deleteObserver.values()));
143146

144147
// Get data from the database and assert that everything is deleted.
145148
assertTrue(adapter.query(BlogOwner.class).isEmpty());
@@ -259,19 +262,19 @@ public void deleteModelTypeWithPredicateCascades() throws DataStoreException {
259262
}
260263

261264
// Observe deletions
262-
Set<String> deleted = new HashSet<>();
263-
adapter.observe()
265+
TestObserver<String> deleteObserver = adapter.observe()
264266
.filter(change -> StorageItemChange.Type.DELETE.equals(change.type()))
265267
.map(StorageItemChange::item)
266-
.subscribe(model -> deleted.add(model.getId()));
268+
.map(Model::getId)
269+
.test();
267270

268271
// Triggers a delete of all blogs.
269272
// All posts will be deleted by cascade.
270273
adapter.delete(Blog.class, QueryPredicates.all());
271274

272-
// Assert that cascaded deletions are observed.
273-
assertEquals(12, deleted.size());
274-
assertEquals(expected, deleted);
275+
// Assert 3 blogs and 9 posts are deleted.
276+
deleteObserver.assertValueCount(12);
277+
assertEquals(expected, new HashSet<>(deleteObserver.values()));
275278

276279
// Get the BlogOwner from the database. Should not have been deleted.
277280
final List<BlogOwner> blogOwners = adapter.query(BlogOwner.class);
@@ -301,4 +304,43 @@ public void deleteNoneDoesNotFail() throws DataStoreException {
301304
final List<BlogOwner> blogOwners = adapter.query(BlogOwner.class);
302305
assertEquals(Collections.singletonList(john), blogOwners);
303306
}
307+
308+
/**
309+
* Test deleting item with many children does not trigger SQLite exception.
310+
* SQLite places a hard limit of 1000 on expression tree depth, so chaining too many
311+
* predicates can potentially trigger an error.
312+
* @throws DataStoreException On unexpected failure manipulating items in/out of DataStore
313+
*/
314+
@Test
315+
public void deleteModelWithManyChildrenSucceeds() throws DataStoreException {
316+
// Create model with more than 1000 child models
317+
BlogOwner ownerModel = BlogOwner.builder()
318+
.name("BlogOwner")
319+
.build();
320+
adapter.save(ownerModel);
321+
for (int blog = 0; blog < 1001; blog++) {
322+
Blog blogModel = Blog.builder()
323+
.name("Blog " + blog)
324+
.owner(ownerModel)
325+
.build();
326+
adapter.save(blogModel);
327+
}
328+
329+
// Observe deletions
330+
TestObserver<String> deleteObserver = adapter.observe()
331+
.filter(change -> StorageItemChange.Type.DELETE.equals(change.type()))
332+
.map(StorageItemChange::item)
333+
.map(Model::getId)
334+
.test();
335+
336+
// Delete parent model
337+
adapter.delete(ownerModel);
338+
339+
// Assert 1 blog owner and 1001 blogs are deleted.
340+
deleteObserver.assertValueCount(1002);
341+
342+
// Assert the local deletion happened.
343+
assertTrue(adapter.query(BlogOwner.class).isEmpty());
344+
assertTrue(adapter.query(Blog.class).isEmpty());
345+
}
304346
}

‎aws-datastore/src/main/java/com/amplifyframework/datastore/storage/sqlite/SQLiteModelTree.java‎

Lines changed: 42 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -17,31 +17,28 @@
1717

1818
import android.database.Cursor;
1919
import android.database.sqlite.SQLiteDatabase;
20+
import android.database.sqlite.SQLiteException;
2021
import androidx.annotation.NonNull;
2122

2223
import com.amplifyframework.core.Amplify;
2324
import com.amplifyframework.core.model.Model;
2425
import com.amplifyframework.core.model.ModelAssociation;
2526
import com.amplifyframework.core.model.ModelSchema;
2627
import com.amplifyframework.core.model.ModelSchemaRegistry;
27-
import com.amplifyframework.core.model.query.QueryOptions;
28-
import com.amplifyframework.core.model.query.Where;
29-
import com.amplifyframework.core.model.query.predicate.QueryField;
30-
import com.amplifyframework.core.model.query.predicate.QueryPredicate;
31-
import com.amplifyframework.core.model.query.predicate.QueryPredicates;
32-
import com.amplifyframework.datastore.DataStoreException;
3328
import com.amplifyframework.datastore.appsync.SerializedModel;
3429
import com.amplifyframework.datastore.storage.sqlite.adapter.SQLiteTable;
3530
import com.amplifyframework.logging.Logger;
3631
import com.amplifyframework.util.Empty;
3732
import com.amplifyframework.util.GsonFactory;
33+
import com.amplifyframework.util.Wrap;
3834

3935
import com.google.gson.Gson;
4036

4137
import java.util.ArrayList;
4238
import java.util.Collection;
4339
import java.util.Collections;
4440
import java.util.HashSet;
41+
import java.util.Iterator;
4542
import java.util.LinkedHashMap;
4643
import java.util.List;
4744
import java.util.Map;
@@ -54,21 +51,17 @@ final class SQLiteModelTree {
5451
private static final Logger LOG = Amplify.Logging.forNamespace("amplify:aws-datastore");
5552

5653
private final ModelSchemaRegistry registry;
57-
private final SQLCommandFactory commandFactory;
5854
private final SQLiteDatabase database;
5955
private final Gson gson;
6056

6157
/**
6258
* Constructs a model family tree traversing utility.
6359
* @param registry model registry to search schema from
64-
* @param commandFactory SQL command factory
6560
* @param database SQLite database connection handle
6661
*/
6762
SQLiteModelTree(ModelSchemaRegistry registry,
68-
SQLCommandFactory commandFactory,
6963
SQLiteDatabase database) {
7064
this.registry = registry;
71-
this.commandFactory = commandFactory;
7265
this.database = database;
7366
this.gson = GsonFactory.instance();
7467
}
@@ -119,38 +112,28 @@ private void recurseTree(
119112
ModelSchema modelSchema,
120113
Collection<String> parentIds
121114
) {
122-
SQLiteTable parentTable = SQLiteTable.fromSchema(modelSchema);
123-
final String parentTableName = parentTable.getName();
124-
final String parentPrimaryKeyName = parentTable.getPrimaryKey().getName();
125115
for (ModelAssociation association : modelSchema.getAssociations().values()) {
126116
switch (association.getName()) {
127117
case "HasOne":
128118
case "HasMany":
129119
String childModel = association.getAssociatedType(); // model name
130120
ModelSchema childSchema = registry.getModelSchemaForModelClass(childModel);
131121
SQLiteTable childTable = SQLiteTable.fromSchema(childSchema);
132-
String childPrimaryKey = childTable.getPrimaryKey().getAliasedName();
133-
QueryField queryField = QueryField.field(parentTableName, parentPrimaryKeyName);
134-
135-
// Chain predicates with OR operator.
136-
QueryPredicate predicate = QueryPredicates.none();
137-
for (String parentId : parentIds) {
138-
QueryPredicate operation = queryField.eq(parentId);
139-
predicate = predicate.or(operation);
140-
}
122+
String childId = childTable.getPrimaryKey().getName();
123+
String parentId = childSchema.getAssociations() // get a map of associations
124+
.get(association.getAssociatedName()) // get @BelongsTo association linked to this field
125+
.getTargetName(); // get the target field (parent) name
141126

142127
// Collect every children one level deeper than current level
143-
// SELECT * FROM <CHILD_TABLE> WHERE <PARENT> = <ID_1> OR <PARENT> = <ID_2> OR ...
144-
QueryOptions options = Where.matches(predicate);
145128
Set<String> childrenIds = new HashSet<>();
146-
try (Cursor cursor = queryAll(childModel, options)) {
129+
try (Cursor cursor = queryChildren(childTable.getName(), childId, parentId, parentIds)) {
147130
if (cursor != null && cursor.moveToFirst()) {
148-
int index = cursor.getColumnIndexOrThrow(childPrimaryKey);
131+
int index = cursor.getColumnIndexOrThrow(childId);
149132
do {
150133
childrenIds.add(cursor.getString(index));
151134
} while (cursor.moveToNext());
152135
}
153-
} catch (DataStoreException exception) {
136+
} catch (SQLiteException exception) {
154137
// Don't cut the search short. Populate rest of the tree.
155138
LOG.error("Failed to query children of deleted model(s).", exception);
156139
}
@@ -172,15 +155,38 @@ private void recurseTree(
172155
}
173156
}
174157

175-
private Cursor queryAll(
176-
@NonNull String tableName,
177-
@NonNull QueryOptions options
178-
) throws DataStoreException {
179-
final ModelSchema schema = registry.getModelSchemaForModelClass(tableName);
180-
final SqlCommand sqlCommand = commandFactory.queryFor(schema, options);
181-
final String rawQuery = sqlCommand.sqlStatement();
182-
final String[] bindings = sqlCommand.getBindingsAsArray();
183-
return database.rawQuery(rawQuery, bindings);
158+
private Cursor queryChildren(
159+
@NonNull String childTable,
160+
@NonNull String childIdField,
161+
@NonNull String parentIdField,
162+
@NonNull Collection<String> parentIds
163+
) {
164+
// Wrap each ID with single quote
165+
StringBuilder quotedIds = new StringBuilder();
166+
for (Iterator<String> ids = parentIds.iterator(); ids.hasNext();) {
167+
quotedIds.append(Wrap.inSingleQuotes(ids.next()));
168+
if (ids.hasNext()) {
169+
quotedIds.append(SqlKeyword.SEPARATOR);
170+
}
171+
}
172+
// SELECT <child_id> FROM <child_table> WHERE <parent_id> IN (<id_1>, <id_2>, ...)
173+
String queryString = String.valueOf(SqlKeyword.SELECT) +
174+
SqlKeyword.DELIMITER +
175+
Wrap.inBackticks(childIdField) +
176+
SqlKeyword.DELIMITER +
177+
SqlKeyword.FROM +
178+
SqlKeyword.DELIMITER +
179+
Wrap.inBackticks(childTable) +
180+
SqlKeyword.DELIMITER +
181+
SqlKeyword.WHERE +
182+
SqlKeyword.DELIMITER +
183+
Wrap.inBackticks(parentIdField) +
184+
SqlKeyword.DELIMITER +
185+
SqlKeyword.IN +
186+
SqlKeyword.DELIMITER +
187+
Wrap.inParentheses(quotedIds.toString()) +
188+
";";
189+
return database.rawQuery(queryString, new String[0]);
184190
}
185191

186192
private String getModelName(@NonNull Model model) {

‎aws-datastore/src/main/java/com/amplifyframework/datastore/storage/sqlite/SQLiteStorageAdapter.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,6 @@ public synchronized void initialize(
222222
*/
223223
this.sqliteModelTree = new SQLiteModelTree(
224224
modelSchemaRegistry,
225-
sqlCommandFactory,
226225
databaseConnectionHandle
227226
);
228227

‎aws-datastore/src/main/java/com/amplifyframework/datastore/storage/sqlite/SqlKeyword.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,16 @@ public enum SqlKeyword {
3535
*/
3636
DELIMITER(" "),
3737

38+
/**
39+
* Acts as a separator between fields in SQL.
40+
*/
41+
SEPARATOR(", "),
42+
43+
/**
44+
* Acts as a placeholder for value to be binded onto.
45+
*/
46+
VARIABLE("?"),
47+
3848
/**
3949
* SQL operator to check for equality.
4050
*/

0 commit comments

Comments
 (0)