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?
antweb
force-pushed
the
sqldelight
branch
3 times, most recently
from
d7ede36
to
04b5936
Compare
antweb
force-pushed
the
sqldelight
branch
3 times, most recently
from
d835bc1
to
58026f1
Compare
| 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
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)
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.
| import java.util.Date | ||
| import java.util.TimeZone | ||
|
|
||
| class AndroidUtilDateAdapter : ColumnAdapter<Date, String> { |
There was a problem hiding this comment.
Can this adapter be used by JVM as well or is it really android specific?
There was a problem hiding this comment.
It's specific to how requery saved Date values as strings to the DB on Android. While we get 2024-11-13T09:49:28Z there, it uses a different format on a plain JVM (2024-11-13 09:49:28.210).
In theory, you can use this adapter in other places. But you probably want the JavaUtilDateAdapter for backwards compatibility on the JVM.
…droid is too old for TRUE/FALSE (PRETIXPOS-1HH)
Since SimpleDateFormat is not thread safe, we might be running into race conditions here actually. Re-creating a new instance is expensive, so this is only a temporary fix while we validate a change to DateTimeFormatter.
Should guarantee thread safety without constantly re-creating formatters.
No description provided.