Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce that synchronous drivers require a synchronous schema initializer #4013

Merged
merged 9 commits into from
Mar 28, 2023

Conversation

AlecKazakova
Copy link
Collaborator

  • And provide an extension method for interoping in a multiplatform environment that requires both asynchronous and synchronous schemas.

Closes #3964

open to feedback on this idea as a way to solve it. Don't love the duplicated code in the android/native drivers but I think I prefer that over creating some new shared module for drivers

Copy link
Collaborator

@dellisd dellisd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be out of scope for this PR, but what do we actually gain from using the built in creation/migration APIs from Android (and the native driver)? Especially if we're going to be shipping an implementation of a SQLite-compatible auto upgrade function (#3737), I feel like it would be easier to create an async-compatible version of that function that could be used across all of our SQLite-based drivers rather than adding this quirky API.

I'm not very familiar with the native driver, or the inner workings of Android's SQLite support though so I'm likely missing some context.

@AlecKazakova
Copy link
Collaborator Author

For Android our hand is kind of forced, there's a lot of complex connection management that eventually invokes those schema methods to deal with concurrent requests to get the database before its been initialized. It feels probably like the wrong thing to do to try and reinvent that, with pretty low upside. The Native driver is modeled after the support android stuff so I would expect it to be similar.

TBH this is a big reason why I don't really want the JDBC creation helpers either. I don't like the idea of communicating to consumers that we have a mechanism for managing your db version, because thats kind of out of scope for sqldelight right now

@AlecKazakova
Copy link
Collaborator Author

I feel like there is probably a better typesafe way of expressing that the generated code is asyncronous and that a driver is syncronous and failing at compile time. Like a sealed type for the schema, and drivers either accept an asynchronous, synchronous, or both schema. 🤔

@AlecKazakova
Copy link
Collaborator Author

@dellisd
Copy link
Collaborator

dellisd commented Mar 28, 2023

https://github.com/cashapp/sqldelight/compare/astrong/2023-03-27/enforce-syncronous-schemas...astrong/2023-03-27/alternative-synchronous?expand=1

Oh using a generic there is clever.

I had to think for a moment if there would be any compatibility issues with drivers that can support both, but I guess that would be done by just passing QueryResult<Unit> as the generic parameter.

:shipit:

AlecKazakova and others added 8 commits March 28, 2023 08:55
…izer

 - And provide an extension method for interoping in a multiplatform
   environment that requires both asynchronous and synchronous schemas.
…r/android/AndroidSqliteDriver.kt

Co-authored-by: Derek Ellis <derekellis@squareup.com>
…t/driver/native/NativeSqlDatabase.kt

Co-authored-by: Derek Ellis <derekellis@squareup.com>
@AlecKazakova AlecKazakova force-pushed the astrong/2023-03-27/enforce-syncronous-schemas branch 2 times, most recently from 55faa69 to 2a3afa8 Compare March 28, 2023 13:01
@AlecKazakova AlecKazakova force-pushed the astrong/2023-03-27/enforce-syncronous-schemas branch from 2a3afa8 to 737a57f Compare March 28, 2023 13:22
@AlecKazakova AlecKazakova merged commit 16526c3 into master Mar 28, 2023
@AlecKazakova AlecKazakova deleted the astrong/2023-03-27/enforce-syncronous-schemas branch March 28, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema is not created when using Android sql driver
2 participants