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

Fix bug in delete propagation query #7074

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions java/arcs/android/storage/database/DatabaseImpl.kt
Original file line number Diff line number Diff line change
Expand Up @@ -1348,7 +1348,10 @@ class DatabaseImpl(
ON fields.is_collection IN $COLLECTION_FIELDS
AND collection_entries.collection_id = field_values.value_id
LEFT JOIN entity_refs
-- exclude primitive fields
ON fields.type_id > $LARGEST_PRIMITIVE_TYPE_ID
-- exclude inline-entity fields, leaving references fields only.
AND fields.is_collection NOT IN $INLINE_ENTITIES_FIELDS
AND entity_refs.id = field_value_id
WHERE entity_refs.backing_storage_key = ?
AND entity_refs.entity_id = ?
Expand Down Expand Up @@ -2726,6 +2729,19 @@ class DatabaseImpl(
private val INLINE_ENTITY_COLLECTIONS =
FIELD_CLASSES_FOR_ENTITY_COLLECTIONS.joinToString(prefix = "(", postfix = ")")

/**
* The field classes for which the value of the field points to one or more inline entities.
*/
private val FIELD_CLASSES_FOR_INLINE_ENTITIES = listOf(
FieldClass.InlineEntity.ordinal,
FieldClass.InlineEntityCollection.ordinal,
FieldClass.InlineEntityList.ordinal,
)

/** A version of FIELD_CLASSES_FOR_INLINE_ENTITIES to use in SQL IN statements */
private val INLINE_ENTITIES_FIELDS =
FIELD_CLASSES_FOR_INLINE_ENTITIES.joinToString(prefix = "(", postfix = ")")

/**
* The id and name of a sentinel type, to ensure references are namespaced separately to
* primitive types. Changing this value will require a DB migration!
Expand Down
60 changes: 60 additions & 0 deletions javatests/arcs/android/storage/database/DatabaseImplTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3045,6 +3045,66 @@ class DatabaseImplTest(private val parameters: ParameterizedBuildFlags) {
assertThat(database.removeEntitiesHardReferencing(foreignKey, "refId")).isEqualTo(0)
}

@Test
fun removeEntitiesReferencing_inlineField() = runBlockingTest {
/*
* Regression test for b/184594207: this test carefully reproduce a setup where a field_value
* entry is pointing to a inline entity, and has the same value as another field_value entry
* pointing to a foreign reference. When modifying this test, please be careful to maintain this
* property.
*/
newSchema("child")
newSchema("inlineHash")
val schema = newSchema(
"hash",
SchemaFields(
singletons = mapOf(
"ref" to FieldType.EntityRef("child"),
"inline" to FieldType.InlineEntity("inlineHash")
),
collections = mapOf()
)
)
val collectionKey = DummyStorageKey("collection")
val backingKey = DummyStorageKey("backing")
val entity1Key = DummyStorageKey("backing/entity1")
val entity2Key = DummyStorageKey("backing/entity2")
val foreignKey = DummyStorageKey("foreign")

// The ref field of this entity points to row #2 in entity_refs.
val entity1 = RawEntity(
"entity1",
mapOf(
"ref" to RawReference("refId", foreignKey, null, isHardReference = true),
"inline" to RawEntity("ie1")
)
).toDatabaseData(schema)
// The inline field of this entity points to row #2 in storage_keys..
val entity2 = RawEntity(
"entity2",
mapOf(
"ref" to RawReference("refId2", foreignKey, null, isHardReference = true),
"inline" to RawEntity("ie2")
)
).toDatabaseData(schema)
val collection = dbCollection(backingKey, schema, entity1, entity2)

database.insertOrUpdate(entity2Key, entity2)
database.insertOrUpdate(entity1Key, entity1)
database.insertOrUpdate(collectionKey, collection)

assertThat(database.removeEntitiesHardReferencing(foreignKey, "refId")).isEqualTo(1)
assertThat(database.getAllHardReferenceIds(foreignKey)).doesNotContain("refId")

// Entities 1 should be cleared.
assertThat(database.getEntity(entity1Key, schema)).isEqualTo(entity1.nulled())
assertThat(database.getEntity(entity2Key, schema)).isEqualTo(entity2)

// Only entity 2 is left in the collection.
assertThat(database.getCollection(collectionKey, schema))
.isEqualTo(dbCollection(backingKey, schema, entity2).copy(databaseVersion = 2))
}

@Test
fun removeEntitiesReferencingInline() = runBlockingTest {
newSchema("child")
Expand Down