Add index to local entities #6261
Conversation
| 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`() { |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| @Test | ||
| fun `#delete does not change index values for remaining entities`() { |
There was a problem hiding this comment.
This actually might not be what we want, but this drives out actually persisting index (as we don't have a query method to test yet). It also felt important to make the interaction between index values and delete explicit.
Would be good to get a second opinion here @lognaturel.
There was a problem hiding this comment.
Oh actually this totally won't work: multiplicity must start at 0 and not contain any gaps (with the current version of JavaRosa). I'll mark this as a draft again and add that constraint.
seadowg
requested review from
grzesiek2010 and
lognaturel
and removed request for
lognaturel and
grzesiek2010
| 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 -> |
There was a problem hiding this comment.
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".
seadowg
force-pushed
the
entity-multiplicity
branch
3 times, most recently
from
d3ebf19
to
fcd2db9
Compare
| enum class State { | ||
| OFFLINE, | ||
| ONLINE | ||
| } | ||
|
|
||
| fun sameAs(entity: Entity): Boolean { |
There was a problem hiding this comment.
This is currently just for testing it looks like. It feels like two identical items at different positions in document order should not be considered the same but maybe it doesn't matter for the intended usage.
Is == in Kotlin only going to compare the list references for properties? Maybe that makes my point above moot.
There was a problem hiding this comment.
This is currently just for testing it looks like. It feels like two identical items at different positions in document order should not be considered the same but maybe it doesn't matter for the intended usage.
The idea here was to be able to compare entities without the data that gets added by our repo (currently just index, but might also be things like a db ID down the line.). We might have actually end up using this in real code I could imagine.
Is == in Kotlin only going to compare the list references for properties?
Because Entity is a data class it'll have an intelligent equals implementation (and Kotlin calls equals rather than doing reference equality with ==).
There was a problem hiding this comment.
In the case of Entities, we're (essentially) guaranteed a unique ID so this is fine. In the case of arbitrary secondary instances, I think the index may be important to consider because duplicates are more likely. We can likely come back to it then.
seadowg
force-pushed
the
entity-multiplicity
branch
from
fcd2db9
to
b48b130
Compare
seadowg
force-pushed
the
entity-multiplicity
branch
from
b48b130
to
6ad3ad2
Compare
There was a problem hiding this comment.
I've given enough of a look at this to feel comfortable proceeding. I know @grzesiek2010 is tied up with point release things and getting this in will unblock downstream PRs.
|
I already had my first pass so if you would like to wait today with merging then I can finish. |
collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt
Show resolved
Hide resolved
entities/src/test/java/org/odk/collect/entities/javarosa/LocalEntitiesFileInstanceParserTest.kt
Outdated
Show resolved
Hide resolved
|
Tested with Success! Verified on devices with Android 14, 10 Verified Cases:
|
Work towards #5623
This adds a new
indexproperty for local entities that can be used as the multiplicity (XPath "index") for eachTreeElementwhen we generate a secondary instance. This isn't strictly required right now, but it will be needed later when we want to queryEntitiesRepositoryin aFilterStrategyto allow for optimized entity filters (ultimately allowing larger entity lists to be used).Why is this the best possible solution? Were any other approaches considered?
The biggest change here was actually switching to a
New/Savedsealed class/ADT pattern forEntityto allow for properties that are only available after saving (withEntitiesRepository). This allows us to keep the logic of generatingindexentirely within the repository implementations. Any other quirks will be pointed out inline.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
The biggest risk here is that the changes here could mess with forms that use entity lists in selects or calculations. It would be good to play around with those along with deleting (on the server) and adding entities (both on different devices/server and offline).
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest