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

Fix incompatible types in generated bind* and cursor.get* statements #2110

Merged
merged 1 commit into from
Dec 14, 2020
Merged

Fix incompatible types in generated bind* and cursor.get* statements #2110

merged 1 commit into from
Dec 14, 2020

Conversation

veyndan
Copy link
Collaborator

@veyndan veyndan commented Dec 14, 2020

For example, when binding SMALLINT, it is (currently) bound with bindLong(smallint), but SMALLINT is implicitly kotlin.Short. With this PR we'd do bindLong(smallint.toShort()) for this to compile.

This feels like a pretty major bug as all of the non-SQLite dialects we support will fail unless you only use a specific subset of the supported dialect types (e.g., BIGINT is implicitly kotlin.Long, so there's no type conversion to do there).

| database.data_Adapter.boolean2Adapter.decode(cursor.getLong(2)!! == 1L),
| cursor.getLong(3)?.let { database.data_Adapter.boolean3Adapter.decode(it == 1L) },
| cursor.getLong(4)!!.toByte(),
| cursor.getLong(5)?.toByte(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With DialectType#decode(…) this should actually be cursor.getLong(5)?.let { it.toByte() }. The reason it is like this is that the default adapters are intercepting it before DialectType#decode(…) can. I think this is fine atm, as we're planning on removing the default adapters (#2056) and the generated code is basically the same.

@@ -351,6 +357,243 @@ class MutatorQueryTypeTest {
|""".trimMargin())
}

@Test fun `types bind fine in HSQL`(dialect: DialectPreset) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry for how verbose all these tests are 😭

Copy link
Collaborator

Choose a reason for hiding this comment

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

its np, i mostly find them important for tracking regressions than reviewing for codegen

Comment on lines +10 to +13
fun decode(value: CodeBlock): CodeBlock = value

fun encode(value: CodeBlock): CodeBlock = value

Copy link
Collaborator Author

@veyndan veyndan Dec 14, 2020

Choose a reason for hiding this comment

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

I opted to have the decode and encode functions on the DialectType itself instead of in IntermediateType to move the dialect specific kotlin types knowledge out of DialectType, and to allow different dialects to decode or encode their types how they wish (inspired by ResultSet in Java). Lmk if moving it somewhere else makes more sense

Copy link
Collaborator

@AlecKazakova AlecKazakova left a comment

Choose a reason for hiding this comment

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

dope, thank you

@@ -351,6 +357,243 @@ class MutatorQueryTypeTest {
|""".trimMargin())
}

@Test fun `types bind fine in HSQL`(dialect: DialectPreset) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

its np, i mostly find them important for tracking regressions than reviewing for codegen

@veyndan veyndan merged commit de310a6 into cashapp:master Dec 14, 2020
@veyndan veyndan deleted the fix-type-conversion branch December 14, 2020 21:03
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.

2 participants