Generic Storage interface for use on all client and on the server. #834
Conversation
identity/src/commonMain/kotlin/com/android/identity/storage/StorageTable.kt
Show resolved
Hide resolved
identity/src/commonMain/kotlin/com/android/identity/storage/StorageTable.kt
Show resolved
Hide resolved
identity/src/commonMain/kotlin/com/android/identity/storage/StorageTable.kt
Show resolved
Hide resolved
identity/src/appleMain/kotlin/com/android/identity/storage/sqlite/SqliteStorageTable.kt
Show resolved
Hide resolved
| private val safeNameRegex = Regex("^[a-zA-Z][a-zA-Z0-9_]*\$") | ||
|
|
||
| const val MAX_KEY_SIZE = 1024 | ||
| // NB: MySQL does not allow table names and we need 2 characters for the prefix. Without |
There was a problem hiding this comment.
Not sure what you mean by "MySQL does not allow table names..." Do you mean it doesn't allow table names that exceed ~62 characters?
| // Known table that needs to be upgraded | ||
| spec.schemaUpgrade(existing.table) | ||
| val upgradedTable = createTable(spec) | ||
| tableMap[spec.name] = TableEntry(upgradedTable, spec) |
There was a problem hiding this comment.
Great catch, thanks!
| // Table never existed | ||
| val newTable = createTable(spec) | ||
| tableMap[spec.name.lowercase()] = TableEntry(newTable, spec) | ||
| schemaTable!!.insert(key = spec.name, data = spec.encodeToByteString()) |
There was a problem hiding this comment.
In some places we're storing the table name in its original case, in other cases we're making it lowercase. It looks like tableMap sometimes uses lowercase, schemaTable uses the original case... StorageTableSpec's equals() function is case sensitive, so it'll result in different matches than this getTable() function will.
Seems like it'd be safer to always convert the name to lowercase at the time it's passed into StorageTableSpec (and passed as an argument to functions like getTable()), so we're consistently using the lowercase version of the table names everywhere?
Or maybe change safeNameRegex and only allow lowercase?
There was a problem hiding this comment.
Only tableMap key should be case-insensitive. Added a comment about it. Some SQL implementations do convert table names to lower or upper case, makes it much harder to read error messages, it feels like a mainframe! I think we should do what, say, MacOS filesystem does - preserve casing, but disallow names that differ only in case.
There was a problem hiding this comment.
Keeping it for messages makes sense.
Would it make sense to update StorageTableSpec.equals() and .hashCode() to convert the name to lowercase for their checks? That would make the API more consistent.
There was a problem hiding this comment.
No, we do not want to allow creating tables that have the same name when compared without case sensitivity. Once the table is written to the storage, the name has to be the same whether or not underlying engine is case-sensitive. So when we detect collisions (tableMap key serves that purpose) we use case-insensitive comparison. But when we check for sameness (TableSpec comparison) we use case-sensitive way.
identity/src/commonMain/kotlin/com/android/identity/storage/base/SqlStatementMaker.kt
Show resolved
Hide resolved
| } | ||
| } | ||
| } | ||
| val map = mutableMapOf<String, String>() |
There was a problem hiding this comment.
map -> insertResults
?
...to make it easier to understand what the map means.
There was a problem hiding this comment.
Looks like the name hasn't been changed?
Sorry if my earlier comment wasn't clear...I was suggesting renaming the map variable to insertResults.
There was a problem hiding this comment.
Ah, OK, I renamed implicit parameter of the map lambda. Renamed map variable too.
identity/src/commonTest/kotlin/com/android/identity/storage/StorageTest.kt
Show resolved
Hide resolved
identity/src/commonMain/kotlin/com/android/identity/storage/StorageTableSpec.kt
Show resolved
Hide resolved
| // Also note that a large window size may lead to longer delays when loading from the | ||
| // database. And if we keep this, replace the magic number with a constant. | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) { | ||
| // The default window size of 2MB is too small for video files. |
There was a problem hiding this comment.
This comment was specific to selfie support, which won't end up needing this DB (the current implementation no longer does). If we still need support for blobs > 2MB (on OS versions that support it), we can leave this code, but I'd remove this comment.
There was a problem hiding this comment.
MySQL JDBC driver chokes on sizes above about 6MB, it seems. Removed the selfie reference and adjusted the limit down to 5MB.
identity/src/androidMain/kotlin/com/android/identity/storage/android/AndroidStorageTable.kt
Show resolved
Hide resolved
| // TODO: Older OS versions don't support setting the cursor window size. | ||
| // What should we do with older OS versions? | ||
| // Also note that a large window size may lead to longer delays when loading from the | ||
| // database. And if we keep this, replace the magic number with a constant. |
There was a problem hiding this comment.
We can remove the sentence about replacing the magic number with a constant...
identity/src/commonMain/kotlin/com/android/identity/storage/StorageTableSpec.kt
Show resolved
Hide resolved
| // Table never existed | ||
| val newTable = createTable(spec) | ||
| tableMap[spec.name.lowercase()] = TableEntry(newTable, spec) | ||
| schemaTable!!.insert(key = spec.name, data = spec.encodeToByteString()) |
There was a problem hiding this comment.
Keeping it for messages makes sense.
Would it make sense to update StorageTableSpec.equals() and .hashCode() to convert the name to lowercase for their checks? That would make the API more consistent.
| } | ||
| } | ||
| } | ||
| val map = mutableMapOf<String, String>() |
There was a problem hiding this comment.
Looks like the name hasn't been changed?
Sorry if my earlier comment wasn't clear...I was suggesting renaming the map variable to insertResults.
| if (!sql.useReturningClause) { | ||
| // Without returningSupported, SQL UPDATE silently fails when the record | ||
| // does not exist. Check for existence first | ||
| val exists = connection.prepare(sql.deleteOrUpdateCheckStatement).use { statement -> |
There was a problem hiding this comment.
If this also means removing the useReturningClause flag altogether, that'd clean up the logic a bit, so yes, I'm supportive.
If not, I'm also not opposed to leaving this as is, if it seems like we're going to need this case at some point in the future. The race condition here isn't really problematic.
sorotokin
force-pushed
the
storage
branch
4 times, most recently
from
9795fb5 to
c054185
Compare
| implementation(libs.androidx.test.junit) | ||
| implementation(libs.androidx.espresso.core) | ||
| implementation(libs.compose.junit4) | ||
| } | ||
| } | ||
|
|
||
| val appleTest by getting { |
There was a problem hiding this comment.
Should we use iosMain, iosTest instead of appleMain, appleTest? I think that's more correct since we only target iOS not MacOS...
| @@ -329,7 +330,7 @@ actual object Crypto { | |||
| signature.toDerEncoded() | |||
| } | |||
| } | |||
| Signature.getInstance(signatureAlgorithm).run { | |||
| Signature.getInstance(signatureAlgorithm, BouncyCastleProvider.PROVIDER_NAME).run { | |||
There was a problem hiding this comment.
This should be avoided since it might cause long pauses while the provider is loaded. Better to fail early so the app can initialize at app startup instead of when the pause is very noticable (for example during credential presentation).
There was a problem hiding this comment.
OK, I am only adding it for brainpool curves now where it does not work on Android otherwise.
Signed-off-by: Peter Sorotokin <[email protected]>
New interface for persistent storage to replace our current client and server interfaces. On the server side, new interface supports expiration, supports Postgresql and is much better specified and tested than the current interface. On the client side, important features are moving to sqlite-based storage and support for iOS. Unifying server and client storage also gives our provisioning code much better foundation (this is part of the code that we can choose to run either in the mobile client or in the wallet server).
New interface is non-blocking and it can be safely invoked from any thread (even from the main thread on the client). All blocking operations are run on the appropriate thread on all platforms.
This is not hooked to anything yet.
Supports ephemeral storage (no persistence), SQLite on both iOS and Android and server-side databases using jdbc.
Added unit tests to get fairly good coverage of the code and exercise all implementations in exactly the same way.
Tests in commonMain now run in android emulator too. Had to do some minor tweaks in identity/src/javaSharedMain/kotlin/com/android/identity/crypto for this to work.
Also tested by implementing existing StorageEngine interface using new interface (using runBlocking to stitch it, as the old interface is blocking) and making sure that wallet app works correctly.