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.

Sign up for GitHub

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

Add index to local entities #6261

Merged
merged 3 commits into from
Jul 19, 2024
Merged

Add index to local entities #6261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11c837b
Use sealed class for saved/new entities
seadowg Jul 10, 2024
6ad3ad2
Add index to entities and use as multiplicty
seadowg Jul 10, 2024
961c46e
Make tests stricter
seadowg Jul 19, 2024
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
24 changes: 17 additions & 7 deletions collect_app/src/main/java/org/odk/collect/android/entities/JsonFileEntitiesRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,18 @@ class JsonFileEntitiesRepository(directory: File) : EntitiesRepository {
return readJson().keys
}

override fun getEntities(list: String): List<Entity> {
return readEntities().filter { it.list == list }
override fun getEntities(list: String): List<Entity.Saved> {
return readEntities().filter { it.list == list }.mapIndexed { index, entity ->
Copy link
Member Author

@seadowg seadowg Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Our future "query" APIs would just filter the result of this.

This sort of thing is obviously going to be much more awkward to implement in SQL, but I think we have some options. Worst case, we'll need to do more work in form download (or in some new background job kicked off after form download) to give ourselves an "index".

Entity.Saved(
entity.list,
entity.id,
entity.label,
entity.version,
entity.properties,
entity.state,
index
)
}
}

override fun save(vararg entities: Entity) {
Expand All @@ -35,7 +45,7 @@ class JsonFileEntitiesRepository(directory: File) : EntitiesRepository {

entityList.remove(existing)
entityList.add(
Entity(
Entity.New(
entity.list,
entity.id,
entity.label ?: existing.label,
Expand Down Expand Up @@ -72,7 +82,7 @@ class JsonFileEntitiesRepository(directory: File) : EntitiesRepository {
writeEntities(existing)
}

private fun writeEntities(entities: MutableList<Entity>) {
private fun writeEntities(entities: List<Entity.New>) {
val map = mutableMapOf<String, MutableList<JsonEntity>>()
entities.forEach {
map.getOrPut(it.list) { mutableListOf() }.add(it.toJson())
Expand All @@ -81,7 +91,7 @@ class JsonFileEntitiesRepository(directory: File) : EntitiesRepository {
writeJson(map)
}

private fun readEntities(): MutableList<Entity> {
private fun readEntities(): MutableList<Entity.New> {
return readJson().entries.flatMap { (list, entities) ->
entities.map { it.toEntity(list) }
}.toMutableList()
Expand Down Expand Up @@ -146,14 +156,14 @@ class JsonFileEntitiesRepository(directory: File) : EntitiesRepository {
val offline: Boolean
)

private fun JsonEntity.toEntity(list: String): Entity {
private fun JsonEntity.toEntity(list: String): Entity.New {
val state = if (this.offline) {
Entity.State.OFFLINE
} else {
Entity.State.ONLINE
}

return Entity(
return Entity.New(
list,
this.id,
this.label,
Expand Down
131 changes: 98 additions & 33 deletions collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import org.hamcrest.Matchers.contains
import org.hamcrest.Matchers.containsInAnyOrder
import org.hamcrest.Matchers.equalTo
import org.junit.Test
import org.odk.collect.android.entities.support.EntitySameAsMatcher.Companion.sameEntityAs
import org.odk.collect.entities.storage.EntitiesRepository
import org.odk.collect.entities.storage.Entity

Expand All @@ -16,8 +17,8 @@ abstract class EntitiesRepositoryTest {
fun `#getLists returns lists for saved entities`() {
val repository = buildSubject()

val wine = Entity("wines", "1", "Léoville Barton 2008")
val whisky = Entity("whiskys", "2", "Lagavulin 16")
val wine = Entity.New("wines", "1", "Léoville Barton 2008")
val whisky = Entity.New("whiskys", "2", "Lagavulin 16")
repository.save(wine)
repository.save(whisky)

Expand All @@ -28,97 +29,97 @@ abstract class EntitiesRepositoryTest {
fun `#getEntities returns entities for list`() {
val repository = buildSubject()

val wine = Entity("wines", "1", "Léoville Barton 2008")
val whisky = Entity("whiskys", "2", "Lagavulin 16")
val wine = Entity.New("wines", "1", "Léoville Barton 2008")
val whisky = Entity.New("whiskys", "2", "Lagavulin 16")
repository.save(wine)
repository.save(whisky)

val wines = repository.getEntities("wines")
assertThat(wines.size, equalTo(1))
assertThat(wines[0], equalTo(wine))
assertThat(wines[0], sameEntityAs(wine))

val whiskys = repository.getEntities("whiskys")
assertThat(whiskys.size, equalTo(1))
assertThat(whiskys[0], equalTo(whisky))
assertThat(whiskys[0], sameEntityAs(whisky))
}

@Test
fun `#save updates existing entity with matching id`() {
val repository = buildSubject()

val wine = Entity("wines", "1", "Léoville Barton 2008", version = 1)
val wine = Entity.New("wines", "1", "Léoville Barton 2008", version = 1)
repository.save(wine)

val updatedWine = Entity("wines", wine.id, "Léoville Barton 2009", version = 2)
val updatedWine = Entity.New("wines", wine.id, "Léoville Barton 2009", version = 2)
repository.save(updatedWine)

val wines = repository.getEntities("wines")
assertThat(wines, contains(updatedWine))
assertThat(wines, contains(sameEntityAs(updatedWine)))
}

@Test
fun `#save creates entity with matching id in different list`() {
val repository = buildSubject()

val wine = Entity("wines", "1", "Léoville Barton 2008", version = 1)
val wine = Entity.New("wines", "1", "Léoville Barton 2008", version = 1)
repository.save(wine)

val updatedWine = Entity("whisky", wine.id, "Edradour 10", version = 2)
val updatedWine = Entity.New("whisky", wine.id, "Edradour 10", version = 2)
repository.save(updatedWine)

val wines = repository.getEntities("wines")
assertThat(wines, contains(wine))
assertThat(wines, contains(sameEntityAs(wine)))
val whiskys = repository.getEntities("whisky")
assertThat(whiskys, contains(updatedWine))
assertThat(whiskys, contains(sameEntityAs(updatedWine)))
}

@Test
fun `#save updates existing entity with matching id and version`() {
val repository = buildSubject()

val wine = Entity("wines", "1", "Léoville Barton 2008", version = 1)
val wine = Entity.New("wines", "1", "Léoville Barton 2008", version = 1)
repository.save(wine)

val updatedWine = wine.copy(label = "Léoville Barton 2009")
repository.save(updatedWine)

val wines = repository.getEntities("wines")
assertThat(wines, contains(updatedWine))
assertThat(wines, contains(sameEntityAs(updatedWine)))
}

@Test
fun `#save updates state on existing entity when it is offline`() {
val repository = buildSubject()

val wine = Entity("wines", "1", "Léoville Barton 2008", state = Entity.State.OFFLINE)
val wine = Entity.New("wines", "1", "Léoville Barton 2008", state = Entity.State.OFFLINE)
repository.save(wine)

val updatedWine = wine.copy(state = Entity.State.ONLINE)
repository.save(updatedWine)

val wines = repository.getEntities("wines")
assertThat(wines, contains(updatedWine))
assertThat(wines, contains(sameEntityAs(updatedWine)))
}

@Test
fun `#save does not update state on existing entity when it is online`() {
val repository = buildSubject()

val wine = Entity("wines", "1", "Léoville Barton 2008", state = Entity.State.ONLINE)
val wine = Entity.New("wines", "1", "Léoville Barton 2008", state = Entity.State.ONLINE)
repository.save(wine)

val updatedWine = wine.copy(state = Entity.State.OFFLINE)
repository.save(updatedWine)

val wines = repository.getEntities("wines")
assertThat(wines, contains(wine))
assertThat(wines, contains(sameEntityAs(wine)))
}

@Test
fun `#save adds new properties`() {
val repository = buildSubject()

val wine = Entity(
val wine = Entity.New(
"wines",
"1",
"Léoville Barton 2008",
Expand All @@ -127,7 +128,7 @@ abstract class EntitiesRepositoryTest {
)
repository.save(wine)

val updatedWine = Entity(
val updatedWine = Entity.New(
"wines",
wine.id,
"Léoville Barton 2008",
Expand All @@ -145,7 +146,7 @@ abstract class EntitiesRepositoryTest {
fun `#save updates existing properties`() {
val repository = buildSubject()

val wine = Entity(
val wine = Entity.New(
"wines",
"1",
"Léoville Barton 2008",
Expand All @@ -154,7 +155,7 @@ abstract class EntitiesRepositoryTest {
)
repository.save(wine)

val updatedWine = Entity(
val updatedWine = Entity.New(
"wines",
wine.id,
"Léoville Barton 2008",
Expand All @@ -172,7 +173,7 @@ abstract class EntitiesRepositoryTest {
fun `#save does not update existing label if new one is null`() {
val repository = buildSubject()

val wine = Entity(
val wine = Entity.New(
"wines",
"1",
"Léoville Barton 2008",
Expand All @@ -181,7 +182,7 @@ abstract class EntitiesRepositoryTest {
)
repository.save(wine)

val updatedWine = Entity(
val updatedWine = Entity.New(
"wines",
wine.id,
null,
Expand All @@ -204,16 +205,16 @@ abstract class EntitiesRepositoryTest {
repository.addList("blah")
assertThat(repository.getLists(), containsInAnyOrder("wines", "blah"))

repository.save(Entity("wines", "blah", "Blah"))
repository.save(Entity.New("wines", "blah", "Blah"))
assertThat(repository.getLists(), containsInAnyOrder("wines", "blah"))
}

@Test
fun `#clear deletes all entities`() {
val repository = buildSubject()

val wine = Entity("wines", "1", "Léoville Barton 2008")
val whisky = Entity("whiskys", "2", "Lagavulin 16")
val wine = Entity.New("wines", "1", "Léoville Barton 2008")
val whisky = Entity.New("whiskys", "2", "Lagavulin 16")
repository.save(wine)
repository.save(whisky)

Expand All @@ -227,13 +228,55 @@ abstract class EntitiesRepositoryTest {
fun `#save can save multiple entities`() {
val repository = buildSubject()

val wine = Entity("wines", "1", "Léoville Barton 2008")
val whisky = Entity("whiskys", "2", "Lagavulin 16")
val wine = Entity.New("wines", "1", "Léoville Barton 2008")
val whisky = Entity.New("whiskys", "2", "Lagavulin 16")
repository.save(wine, whisky)

assertThat(repository.getLists(), containsInAnyOrder("wines", "whiskys"))
}

@Test
fun `#save assigns an index to each entity in insert order when saving multiple entities`() {
Copy link
Member Author

Choose a reason for hiding this comment

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

They don't strictly need to be in insert order most likely, but this behaviour is much easier to test, and will mean the resulting multiplicity within form entry will match the order in the CSVs.

val first = Entity.New("wines", "1", "Léoville Barton 2008")
val second = Entity.New("wines", "2", "Pontet Canet 2014")

val repository = buildSubject()
repository.save(first, second)

val entities = repository.getEntities("wines")
assertThat(entities[0].index, equalTo(0))
assertThat(entities[1].index, equalTo(1))
}

@Test
fun `#save assigns an index to each entity in insert order when saving single entities`() {
val first = Entity.New("wines", "1", "Léoville Barton 2008")
val second = Entity.New("wines", "2", "Pontet Canet 2014")

val repository = buildSubject()
repository.save(first)
repository.save(second)

val entities = repository.getEntities("wines")
assertThat(entities[0].index, equalTo(0))
assertThat(entities[1].index, equalTo(1))
}

@Test
fun `#save does not change index when updating an existing entity`() {
seadowg marked this conversation as resolved.
Show resolved Hide resolved
val repository = buildSubject()

val first = Entity.New("wines", "1", "Léoville Barton 2008")
val second = Entity.New("wines", "2", "Pontet Canet 2014")
repository.save(first, second)
assertThat(repository.getEntities("wines")[0].index, equalTo(0))

val updatedWine = first.copy(label = "Léoville Barton 2009")
repository.save(updatedWine)

assertThat(repository.getEntities("wines")[0].index, equalTo(0))
}

@Test
fun `#addList adds a list with no entities`() {
val repository = buildSubject()
Expand All @@ -247,12 +290,34 @@ abstract class EntitiesRepositoryTest {
fun `#delete removes an entity`() {
val repository = buildSubject()

val leoville = Entity("wines", "1", "Léoville Barton 2008")
val canet = Entity("wines", "2", "Pontet-Canet 2014")
val leoville = Entity.New("wines", "1", "Léoville Barton 2008")
val canet = Entity.New("wines", "2", "Pontet-Canet 2014")
repository.save(leoville, canet)

repository.delete("1")

assertThat(repository.getEntities("wines"), containsInAnyOrder(canet))
assertThat(repository.getEntities("wines"), containsInAnyOrder(sameEntityAs(canet)))
}

@Test
fun `#delete updates index values so that they are always in sequence and start at 0`() {
val repository = buildSubject()

val leoville = Entity.New("wines", "1", "Léoville Barton 2008")
val canet = Entity.New("wines", "2", "Pontet-Canet 2014")
val gloria = Entity.New("wines", "3", "Chateau Gloria 2016")
repository.save(leoville, canet, gloria)

repository.delete("1")

var wines = repository.getEntities("wines")
assertThat(wines[0].index, equalTo(0))
assertThat(wines[1].index, equalTo(1))

repository.save(leoville)
wines = repository.getEntities("wines")
assertThat(wines[0].index, equalTo(0))
assertThat(wines[1].index, equalTo(1))
assertThat(wines[2].index, equalTo(2))
}
}
9 changes: 5 additions & 4 deletions collect_app/src/test/java/org/odk/collect/android/entities/JsonFileEntitiesRepositoryTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import org.hamcrest.MatcherAssert.assertThat
import org.hamcrest.Matchers.contains
import org.hamcrest.Matchers.equalTo
import org.junit.Test
import org.odk.collect.android.entities.support.EntitySameAsMatcher.Companion.sameEntityAs
import org.odk.collect.entities.storage.EntitiesRepository
import org.odk.collect.entities.storage.Entity
import org.odk.collect.shared.TempFiles
Expand All @@ -24,18 +25,18 @@ class JsonFileEntitiesRepositoryTest : EntitiesRepositoryTest() {
val two = JsonFileEntitiesRepository(directory)
val three = JsonFileEntitiesRepository(File(TempFiles.getPathInTempDir()))

val entity = Entity("stuff", "1", "A thing")
val entity = Entity.New("stuff", "1", "A thing")
one.save(entity)
assertThat(two.getLists(), contains("stuff"))
assertThat(two.getEntities("stuff"), contains(entity))
assertThat(two.getEntities("stuff"), contains(sameEntityAs(entity)))
assertThat(three.getLists().size, equalTo(0))
}

@Test
fun `clears data if backing file can't be parsed by current code`() {
val repository = buildSubject()
repository.addList("stuff")
repository.save(Entity("stuff", "123", null))
repository.save(Entity.New("stuff", "123", null))

val filesInDir = directory.listFiles()
assertThat(filesInDir!!.size, equalTo(1))
Expand All @@ -44,7 +45,7 @@ class JsonFileEntitiesRepositoryTest : EntitiesRepositoryTest() {

assertThat(repository.getEntities("stuff").size, equalTo(0))

repository.save(Entity("stuff", "123", null))
repository.save(Entity.New("stuff", "123", null))
assertThat(repository.getEntities("stuff").size, equalTo(1))
}
}
Loading