Skip to content

Fix INSERT when the generated primary key is the last property - #132

Open
dajiaohuang wants to merge 3 commits into
ctripcorp:feature/bug-fixes-2.6.0from
dajiaohuang:fix/generated-key-last
Open

dajiaohuang wants to merge 3 commits into
ctripcorp:feature/bug-fixes-2.6.0from
dajiaohuang:fix/generated-key-last

Conversation

@dajiaohuang

Copy link
Copy Markdown

Problem

A supported @DBRow @Serializable entity can declare its nullable Long primary key after its other properties:

@DBRow("generated_key_last")
@Serializable
data class GeneratedKeyLast(val name: String, @PrimaryKey val id: Long? = null)

database { GeneratedKeyLastTable INSERT listOf(GeneratedKeyLast("first"), GeneratedKeyLast("second")) } fails with SQLite's near ")": syntax error. The omitted key still determines the column separators and the VALUES closing parenthesis, producing an extra column comma and an unfinished VALUES tuple.

Change

Separate the remaining column names, separate only values actually encoded, and close the VALUES tuple independently of the original last property. This preserves the existing key-omission and parameter-binding behavior. Add a public DSL regression shared by JVM, Native and Android tests, checking the inserted rows and generated IDs.

Validation

Against main 2fb681437e4c0f0f352aaee7be7f3fa6c1cf8647, the new JVM regression fails with the SQLite syntax error. With the fix:

  • :sqllin-driver:jvmTest: 7 passed
  • :sqllin-dsl-test:jvmTest: 94 passed
  • :sqllin-processor:assemble: passed
  • :sqllin-driver:mingwX64Test: 7 passed
  • :sqllin-dsl-test:mingwX64Test: 94 passed
  • git diff --check: passed

Gradle ran with JDK 21, -PonCICD -Pkotlin.native.enableKlibsCrossCompilation=false. MinGW also used an explicit local kotlin.native.home after regenerating its SQLite cinterop to avoid mixing two local Kotlin/Native installations.

Android host tests are registered but could not run: Android SDK location is not configured. Apple and Linux native execution was not available on this Windows host. No publishing tasks were run.

@qiaoyuang qiaoyuang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix and the clear write-up. I reproduced the bug and checked the fix:

  • The cause is as you describe: the encoder chose between , and ) by whether an element was the last one, so a generated key in the last position left a trailing comma in the column list and a VALUES tuple that was never closed. Separating only what is actually written, and closing the tuple at the end, is the right fix, and it no longer depends on where the key is declared.
  • On the current 2.6.0 development branch, your test fails without the fix with near ")": syntax error. With it, sqllin-dsl-test passes on the JVM (95 tests) and in the Android host tests on Robolectric API 26 and 37 (190 tests), so the Android side you couldn't run is covered. It also compiles and links for macosArm64 and compiles for linuxX64.

A few things before we can merge it:

  1. Target branch: please change the base of this PR to feature/bug-fixes-2.6.0. We don't merge into main directly: fixes go to a feature branch, then to release/2.6.0, which is merged into main when the version is released. Your branch merges into feature/bug-fixes-2.6.0 without conflicts, and the diff stays the same.
  2. CHANGELOG: every fix has an entry in CHANGELOG.md. Please add one under ## 2.6.0 / 2026-xx-xx → ### sqllin-dsl, after the existing entries, in the style of the others, e.g. * Fix: INSERT failed with a syntax error when the primary key that the database assigns, a nullable Long @PrimaryKey, was the last property of the class, .... That section only exists on feature/bug-fixes-2.6.0, so please merge it into your branch, or rebase onto it, first.
  3. Style (minor): the codebase writes ranges as 0 ..< n rather than 0 until n, as the appendDBColumnName(descriptor) overload right below yours does with 1 ..< descriptor.elementsCount.

Note that CI only runs for PRs to main and release/**, so it won't run on this PR once it is retargeted; the native tests will run when the bug-fix branch goes to release/2.6.0.

@dajiaohuang
dajiaohuang changed the base branch from main to feature/bug-fixes-2.6.0 October 15, 2026 19:10
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