First steps to move to SQLDelight #47
Conversation
libpretixsync/build.gradle
Outdated
| packageName = "eu.pretix.libpretixsync.sqldelight" | ||
| srcDirs('src/main/sqldelight/sqlite', 'src/main/sqldelight/common', 'src/main/sqldelight/migrations') | ||
|
|
||
| version = 105 |
There was a problem hiding this comment.
I'm worried we'll forget to keep this in sync between build.gradle and build-postgres.gradle, can we move it to its own gradle file and import it from there?
There was a problem hiding this comment.
I'm wondering if this is needed at all, since the version it deducts from the migration is prioritized anyway? I'll check if we can just remove it. If not, I agree that it should go into a common file
| for (i in 0 until checkins.length()) { | ||
| val ci = checkins.getJSONObject(i) | ||
| val listid = ci.getLong("list") | ||
| if (known.containsKey(listid)) { |
There was a problem hiding this comment.
I see that it was the same in the old code, but reading this… this is wrong, isn't it? How did this ever work? We would need to use the server ID of the checkin here, not the list ID?
@robbi5 what do you think?
It returns the result of apiCall, which is non-null.
Since we no longer inherit from classes like AbstractReceipt, the JSON utils must be added separately.
Removes the positionid of the add-on parent from the JSON and replaces it with just the addon_to ID. With SQLDelight, we can no longer directly resolve the relation with just the addon_to field, which means that we have to pass in either the ReceiptLine that addon_to points to or its positionid. Since the toJSON() is mostly used for logging purposes and the positionid is not strictly required, we can simplify the function here and add the equivalent of addon_to.positionid to the JSON afterwards in the caller where required.
Provide the same functionality as the getters on AbstractItem, but for the SQLDelight query result.
Android uses a different date format and requires the database version to be set correctly.
Uses the string representation of the double to create a BigDecimal. The previously used constructor on the other hand used the exact representation, which - to quote the docs - "can be somewhat unpredictable". This behaviour is also closer to requery's handling of decimal values. See https://docs.oracle.com/javase/7/docs/api/java/math/BigDecimal.html#BigDecimal(double)
Useful for downstream testing.
Aligns the data types with those used for other tables.
Some dates are stored as TEXT values in the database. Since they might contain unexpected values (such as "null"), it is more robust to handle them all through a single mapper class that performs consistent conversion.
Maps all 4 byte Postgres ID columns to Long in order to be consistent with the remaining code. See PostgresIdAdapter. Moves getLastInsertedId queries to separate compat .sq file to fix conflicting query classes being generated. The .sq files under sqlite/ and postgres/ are only allowed to contain the CREATE TABLE statements. Otherwise they generate another query class that conflicts with the ones defined in common/. The solution is to create a .sq file with a different name so that it generates a query class with a different name (such as compat.sq for CompatQueries).
SQLDelight has a machanism to sort create statements for a schema based on their dependencies. To achieve this, it creates a graph and sorts it topologically. During this process it only considers column level constraints. If a foreign key is defined as a table level constraint, the dependency is not considered. We were using table level constraints until now which resulted in create statements that were sorted alphabetically after file names. Since the .sq file names match their table names, calls to Schema.create() would fail on BadgeLayoutItem, which requires the table Item to exist.
| override fun getOptions(): List<QuestionOption>? = _options | ||
|
|
||
| override fun requiresAnswer(): Boolean = required | ||
|
|
There was a problem hiding this comment.
The implementation of getDependencyValues is missing! This breaks dependencies between questions.
Here's a good event for testing:
https://staging.pretix.eu/control/event/postest/questions/questions/
There was a problem hiding this comment.
Fixed in bd295fb.
Should we make getDependency() and getDependencyValues() abstract in QuestionLike? Or are there many cases that rely on the defaults?
| } | ||
|
|
||
| override fun update(obj: BlockedTicketSecret, jsonobj: JSONObject) { | ||
| // TODO: Test new behaviour. Original version had no update case |
There was a problem hiding this comment.
There is a specialty about blocked secrets: We don't care about a BlockedTicketSecret with blocked=false. The only reason it exists on the API layer is to allow for differential sync during updates. But there is no need to store it. So instead of updating the blocked field to 0, we can and should just delete the local database entry.
There was a problem hiding this comment.
Correction, the old code does change the blocked field to false, but it never inserts a new row with blocked=false, and the new code does the same, so maybe this is correct this way.
There was a problem hiding this comment.
We also have the clean-up call (db.blockedTicketSecretQueries.deleteNotBlocked()) in download(). That should clean up any rows that have been updated to blocked=false.
The only thing I wonder is if we can simplify any of this to make it more maintainable (e.g. no special treatment in insert()/update() and just a clean-up in download() maybe?). The performance impact of inserting / updating a few rows too many shouldn't be too significant (?)
| if (autoPersist()) { | ||
| insert(jsonobj) | ||
| inserted += 1 | ||
| } |
There was a problem hiding this comment.
Since we do no batching any more, autoPersist() does not make much sense any more. In fact, with autoPersist returning false, as e.g. the case for BlockedTicketSecretSyncAdapter, no data is saved any more!
There was a problem hiding this comment.
Agree. Now that the implementations of insert/update contain the DB queries, there is no need to distinguish on BaseDownloadSyncAdapter level. And with the current code it's very easy to make the mistake of defaulting to false like I did in the BlockedTicketSecretSyncAdapter
With removed batching and the new insert()/update() methods, autoPersist() is no longer needed.
SQLDelight already uses the migration files to determine the DB version.
Question models were missing the getDependencyValues() override.
We were providing the wrong type of Instant.
Some query building code remains which should be migrated in a separate step.
SQLDelight returns the Long value directly on Postgres while it wraps it in a result type on SQLDelight.
No description provided.