`Query.getId()` should be `@Nullable`

[Incanus3]

Hi guys. We've just encountered quite a problem when trying to override Query.getId() in kotlin. Since the whole Query class is marked with @NullMarked, all values are automatically considered @NonNull, but in some cases Query.getId() actually does return null, which is a problem, because

• if we override it as override fun getId(): Any = query.getId() we get a NullPointerException when query.getId() returns null, but since the kotlin compiler understands jspecify annotations, it doesn't allow us to override it as fun getId(): Any? because "Return type of 'getId' is not a subtype of the return type of the overridden member 'fun getId(): Any'".

Example code

// the problematic code

class QueryWrapper<T : Any>(
    private val query: Query<T>,
    private val databaseService: DatabaseService,
) : SpiQuery<T> {
    override fun getId(): Any = query.getId() // <---

    override fun findList(): List<T> = withExceptionHandling { query.findList() }

    // ...

    private fun <T> withExceptionHandling(operation: () -> T): T = try {
        operation()
    } catch (e: Exception) {
        if (e is PersistenceException || e is SQLException) {
            databaseService.setStatus(ServiceStatus.Broken(e))
        }

        throw e
    }
}

// test entities

@Entity
@Suppress("unused")
class TestObject {
    @Id
    val id: String = UUID.randomUUID().toString()
}

@Entity
@Suppress("unused")
class TestConnector {
    @Id
    val id: String = UUID.randomUUID().toString()

    private var sourceId: Long? = null
    private var targetId: Long? = null

    @JsonIgnore
    @ManyToOne
    @JoinColumn(insertable = false, updatable = false)
    var source: TestObject? = null

    @JsonIgnore
    @ManyToOne(optional = false)
    @JoinColumn(insertable = false, updatable = false)
    var target: TestObject? = null
}

// test

class DatabaseServiceTest {
    private val dsConfig = DataSourceConfig().also {
        it.url = "jdbc:postgresql://localhost:123/xxx"
        it.driver = "org.postgresql.Driver"
        it.username = "xxx"
        it.password = "xxx"
        it.platform = Platform.POSTGRES.name.lowercase()

        it.isFailOnStart = false
    }

    private val dbConfig = DatabaseConfig().also {
        it.isDefaultServer = false
        it.databasePlatformName = dsConfig.platform

        it.setSkipDataSourceCheck(true)
        it.setDataSourceConfig(dsConfig)

        it.addClass(TestObject::class.java)
        it.addClass(TestConnector::class.java)
    }

    private val service = create(dbConfig).asService()

    @Test
    fun `getId works`() {
        val objectQuery = QTestObject(service).where().id.eq("1")
        val connectionQuery = QTestConnector(service).where()
            .sourceId.isIn(objectQuery.select(QTestObject._alias.id).query())

        // this fails with "getId(...) must not be null"
        connectionQuery.findList()
    }
}

If I am not mistaken, this would be fixed if @Nullable annotation was added to the getId() method.

[Incanus3]

found out I can sidestep this problem using the -Xnullability-annotations=@org.jspecify.annotations:warn compiler arg, which means I was finally able to run this test with getId(): Any?, which uncovered two more methods missing @Nullable:

• LockWait getForUpdateLockWait()
• LockType getForUpdateLockType()

[Incanus3]

wow, that was fast. thank you so much @rbygrave

[rbygrave]

Well it helps when the description is really good and clear [so thanks for that] and the change we need is as easy as it gets really :)

[Incanus3]

Just found out there are some methods with the same problem in Database, at least:

• readOnlyDataSource
• currentTransaction

and maybe others. @rbygrave should I open a new issue for those?

[rbygrave]

readOnlyDataSource

Already has @Nullable on it? Nothing required there.

currentTransaction

Ah so hmmm. In my mind we might have a design issue around this in that I think there are a lot of cases where we really expect a non-null result and would prefer to fail / throw if there isn't a current active transaction.

So yeah we could just throw a @Nullable there but maybe we need to ponder this more and review the options. It might be that we really prefer this to migrate to throw on a non-active transaction and have another method that returns an Optional<Transaction> ... but yeah we need to be careful and thoughtful here.

Hmmm. I think the currentTransaction issue should be logged as a separate issue to capture those design options and longer term considerations.

[Incanus3]

readOnlyDataSource

Already has @Nullable on it? Nothing required there.

Oh, my bad, this has been done recently and we're still on 15.11. Sorry, should have checked.

currentTransaction

Ah so hmmm. In my mind we might have a design issue around this in that I think there are a lot of cases where we really expect a non-null result and would prefer to fail / throw if there isn't a current active transaction.

So yeah we could just throw a @Nullable there but maybe we need to ponder this more and review the options. It might be that we really prefer this to migrate to throw on a non-active transaction and have another method that returns an Optional<Transaction> ... but yeah we need to be careful and thoughtful here.

Well the approach of having one strict and one nullable method definitely makes sense to me (and kotlin does this a lot in its stdlib)

Hmmm. I think the currentTransaction issue should be logged as a separate issue to capture those design options and longer term considerations.

Ok, but does it make sense for me to open it, or should it be done by you to capture all the needed context behind it?