Fix property names clashing with DB column names #6495
Conversation
| val savedEntity = repository.getEntities("things")[0] | ||
| assertThat(savedEntity, sameEntityAs(entity)) | ||
|
|
||
| repository.save("things", savedEntity) |
There was a problem hiding this comment.
Why do you need to save twice?
There was a problem hiding this comment.
We're checking creating and updating.
| @@ -15,4 +19,21 @@ class DatabaseEntitiesRepositoryTest : EntitiesRepositoryTest() { | |||
| TempFiles.createTempDir().absolutePath | |||
| ) | |||
| } | |||
|
|
|||
| @Test | |||
| fun `#save supports properties with db column names saving new entities and updating existing ones`() { | |||
There was a problem hiding this comment.
Couldn't it be simply: #save supports properties with db column names?
There was a problem hiding this comment.
| @@ -399,29 +397,28 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep | |||
| } | |||
| } | |||
|
|
|||
| private fun addPropertiesToContentValues(contentValues: ContentValues, entity: Entity) { | |||
| entity.properties.forEach { (name, value) -> | |||
| contentValues.put("\"${EntitiesTable.getPropertyColumn(name)}\"", value) | |||
There was a problem hiding this comment.
Couldn't this be just:
contentValues.put(EntitiesTable.getPropertyColumn(name), value)
There was a problem hiding this comment.
You'll see that #save supports properties with dots and dashes when saving new entities and updating existing ones fails if you change to that. We need to quote the column property column names as they might contain special characters that SQL queries don't support.
|
Tested with Success Verified on device with Android 15 Verified cases:
|
Closes #6486
Why is this the best possible solution? Were any other approaches considered?
Other alternatives might have been to index properties in some way (in the
liststable for example), but using a prefix felt like the simplest way to fix this to me. Even if a property clashes with the prefix (a property namedp_idfor example) we would still add/remove the prefix correctly as it's isolated toDatabaseEntitiesRepository(the previous example would end up with a column namedp_p_id).We also decided (on a call) to go with just clearing out the entities DB when upgrading rather than writing proper migration code. This mitigates any risk of migrating data incorrectly, and has minimal impact as the entities DB only exists for beta users at this time.
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?
Along with fixing the bug, any local entities on device will be wiped. It's definitely worth playing with upgrades from the last beta to check this works as expected.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest