Allow registering DriverInitializer for VerifyMigrationTask with ServiceLoader mechanism

[C2H6O]

This PR will allow consumers to initialize and register their custom java.sql.Driver prior to running migrations.

On the consumer side:

1. Create an implementation of DriverInitializer

class DriverInitializerImpl(
    // optional params
) : DriverInitializer {
    override fun execute(properties: SqlDelightDatabaseProperties, driverProperties: Properties) {
        // initialize and register the driver with DriverManager
    }
}

and add a resources/META-INF/services/app.cash.sqldelight.gradle.DriverInitializer file pointing to the DriverInitializerImpl.

2. In a gradle file where you setup SqlDelight database

extensions.configure<SqlDelightExtension> {
  databases.create(name) {
    dependencies.add(configurationName, "<migration driver dependency>")
  }
}

tasks.withType(VerifyMigrationTask::class.java).configureEach {
  driverProperties.set(
    objects.mapProperty(String::class.java, String::class.java).apply {
      put("projectRoot", rootDir.path)
    }
  )
}

Where <migration driver dependency> can either be project(:your_local_project) or coordinates to your published artifact group:artifact:version

[C2H6O]

@AlecStrong @hfhbd would appreciate a review on this. This PR solves our issue with loading a custom driver during migration verification that was caused by ClassLoader isolation.

[AlecKazakova]

can you share what your implementation of the MigrationDriver is? I think the property stuff I'm okay with, the driver stuff is pretty obtuse so want to understand the use case

[hfhbd]

Please add some tests otherwise new commits/refactoring could break it.

[C2H6O]

@AlecStrong added some documentation to the VerifyMigrationDriver. Let me know what you think and I can add more context if needed if it is still unclear.

Implementation of the driver:

class TokenizingDriver: VerifyMigrationDriver {

    companion object {
        const val PROJECT_ROOT = "projectRoot"
    }

    private val wrappedDriver: JDBC = JDBC()

    init {
        // JDBC installs itself on initialization. Let's... not.
        val driverList = DriverManager.getDrivers().toList()

        // If we have other drivers installed, houston, we have a problem!
        assert(driverList.size == 1)
        DriverManager.deregisterDriver(driverList[0])

        DriverManager.registerDriver(this)
    }

    override fun acceptsURL(url: String?): Boolean =
        wrappedDriver.acceptsURL(url)

    override fun connect(url: String?, props: Properties?): Connection {
        require(props != null && props.containsKey(PROJECT_ROOT)) {
            "Ensure connectionProperties are set on the ${VerifyMigrationTask::class} when setting up SqlDelight"
        }
        props.setProperty("enable_load_extension", "true")

        val connection = wrappedDriver.connect(url, props)
        val ext = File(props.getProperty(PROJECT_ROOT), "build/sqlitetokenizer/libsqlite-fts5-synonym-tokenizer.dylib")
        connection.prepareStatement("SELECT load_extension('${ext.absolutePath}', 'sqlite3_fts5_synonym_tokenizer_init');").execute()

        return connection
    }

    override fun getMajorVersion(): Int =
        wrappedDriver.majorVersion

    override fun getMinorVersion(): Int =
        wrappedDriver.minorVersion

    override fun getPropertyInfo(url: String?, info: Properties?): Array<DriverPropertyInfo> =
        wrappedDriver.getPropertyInfo(url, info)

    override fun jdbcCompliant(): Boolean =
        wrappedDriver.jdbcCompliant()

    override fun getParentLogger(): Logger =
        wrappedDriver.parentLogger
}

@hfhbd is there a decent starting point for these tests? I couldn't find any tests currently written that verify VerifyMigrationTask logic. 🙏

[AlecKazakova]

Alright, I understand now. Instead of using a service loader is it possible to just have this type (or a lambda) be another parameter to the verify migration task, I'm imagining an ideal API where you do something like

tasks.withType<VerifyMigrationTask>().configureEach {
  driverInitializer = {
    DriverManager.registerDriver(...)
  }
}

[AlecKazakova]

also I'd wait to write tests until we get this API where we want it to save yourself the trouble

[C2H6O]

hmm, I didn't think it was possible since lambdas can't be serialized. Let me look into it, but if you have an example, I'd really appreciate it!

[C2H6O]

interface DriverInitializer {
  fun execute()
}

VerifyMigrationTask:

  @get:Input
  abstract val driverInitializer: Property<DriverInitializer>

> Cannot fingerprint input property 'driverInitializer': value 'SetupSqldelightKt$setupSqldelight$1$2$1@695e62e3' cannot be serialized.

[C2H6O]

Ah interesting, adding Serializable interface to the DriverInitializer seems to do the trick

[AlecKazakova]

(as in not annotated with @get:input)

[C2H6O]

Hmm, I'm getting this after removing the @get: Input:

  - In plugin 'app.cash.sqldelight.gradle.SqlDelightPlugin$Inject' type 'app.cash.sqldelight.gradle.VerifyMigrationTask' property 'driverInitializer' is missing an input or output annotation.
    
    Reason: A property without annotation isn't considered during up-to-date checking.
    
    Possible solutions:
      1. Add an input or output annotation.
      2. Mark it as @Internal.
    
    Please refer to https://docs.gradle.org/8.0.2/userguide/validation_problems.html#missing_annotation for more details about this problem.

So perhaps we do need it?

[AlecKazakova]

option 2? Marking it as @Internal? And then exposing a function on the task to set it

[C2H6O]

👍 I didn't have to introduce another function to set it, it seems to work just as well

[hfhbd]

Yeah, @input is a required input that is checked and fails if it is not set. @internal is optional.

[AlecKazakova]

some tests for the migration task are here

[hfhbd]

The idea of ServiceLoader was to have a proper interface managing the connection instead of using the JDBC auto service. Having an empty interface as the requirement is (almost) useless.

There is also another option, but I am unsure if we want it:
Option 3: If you only need to fix the autoregistering of JDBC, we could just expose the classpath Gradle configuration so you simply add your classes to the classpath, without any initializer/options in the tasks. But this option would prevent getting the properties from the actual db, you would need to hardcode them in your driver.

[AlecKazakova]

I've definitely seen some gradle tasks expose their classpath as ways to do things like this, I'd be pretty cool with that change

[C2H6O]

@hfhbd I need to pass the projectRoot directory to the driver and it can't be hardcoded unfortunately

[C2H6O]

@AlecStrong I've greatly simplified this PR based on your suggestions 🙏

[AlecKazakova]

yep, cool with this

[hfhbd]

I still try to understand the real problem. Isn't the real problem this hardcoded line?

sqldelight/sqlite-migrations/src/main/kotlin/app/cash/sqlite/migrations/CatalogDatabase.kt

Line 41 in 02edf13

private fun createConnection(path: String): Connection {

If so, what about refactoring this line and reusing the connectionManager from the dialect? I created a PR: #4019

[C2H6O]

@hfhbd now that I'm able to initialize the driver in the context of the worker's ClassLoader, I don't need to pass the properties down to the connect(...) call. I am able to initialize the driver with the right path as a constructor param

[hfhbd]

Could you add a test, eg using the GradleRunner, configure the parameter for example println("CALLED DriverInitializer") and check, if it is printed in the log when executing the verifying task? Or instead of printing create some file and check its existing after the task execution.

[C2H6O]

@AlecStrong @hfhbd if you don't have any objections, I'll merge this in and will revisit writing a unit test for this in the upcoming weeks. I spent some time on it yesterday but I haven't succeeded in writing a functional test and since there aren't any existing tests that do that (configure the VerifyMigrationTask directly, not through SqlDelightDatabase), it'll take me a bit of time to figure it out.

[C2H6O]

Where do I place this class for testing purposes so that it is accessible during a test run?

[hfhbd]

You could create a gradle project in sqldelight-gradle-plugin/src/test, just copy an existing test project and add the test executor here too: https://github.com/cashapp/sqldelight/blob/68a4d849d234b0018c2c2b314915952564ecfe2b/sqldelight-gradle-plugin/src/test/kotlin/app/cash/sqldelight/tests/MigrationTest.kt

[hfhbd]

Oh okay, you already added it the project. This depends on your Gradle setup, for tests you could simple move this class into build.gradle. This should work, I think.

[AlecKazakova]

yea, just having it live in the build.gradle file should work

[C2H6O]

It looks like VerifyMigrationTask needs the DriverInitializerImpl on its classpath, since now I'm getting ClassNotFoundException

[C2H6O]

How do I add the classpath of the build.gradle file to the VerifyMigrationTask? 🤔

[C2H6O]

@hfhbd any Gradle tips for getting this working? I'm kind of running out of ideas

[hfhbd]

Yeah, I took a look and played with different implementations. The only one I got to run is using service loaders. Other implementations failed with serialization errors.

[hfhbd]

I created a PR for this PR, because I can't push to this branch: https://github.com/plangrid/sqldelight/pull/2/files

[C2H6O]

@AlecStrong @hfhbd I added a test, curious if you have some ideas for placing the DriverInitializerImpl class?

[hfhbd]

Revert these changes

[C2H6O]

@hfhbd Yeah, I'll push a commit that reverts the gradle.properties changes and I believe I need to bring back the connectionProperties that I can then pass to my Driver

[hfhbd]

@C2H6O What properties do you want to pass? You can always use environment variables.

[C2H6O]

@hfhbd I need the root path of the top-level project. When TokenizingDriver gets initialized/instantiated by the ClassLoader of the gradle's worker, the driver needs access to that path. How would that (using environment variables) work in this case?

[C2H6O]

@AlecStrong @hfhbd I tested the PR as it is now and it works well for our use-case. I'm not sure about implementation -> api change that we made, but if you guys are cool with it then I'd love to merge this in!

[hfhbd]

Please run spotlessApply to merge it.

[C2H6O]

@hfhbd I ran it but it didn't produce a diff

[C2H6O]

@hfhbd @AlecStrong anything else that I should address in this PR? 🙏

[hfhbd]

The not required run of spotless is interesting 🤔 Let's finally merge it and do it in another PR.