Repository navigation
Fix INSERT when the generated primary key is the last property - #132
Open
dajiaohuang wants to merge 3 commits into
Open
dajiaohuang wants to merge 3 commits into
dajiaohuang wants to merge 3 commits into
Conversation
qiaoyuang
reviewed
Oct 15, 2026
qiaoyuang
left a comment
Collaborator
There was a problem hiding this comment.
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-testpasses 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 formacosArm64and compiles forlinuxX64.
A few things before we can merge it:
- Target branch: please change the base of this PR to
feature/bug-fixes-2.6.0. We don't merge intomaindirectly: fixes go to a feature branch, then torelease/2.6.0, which is merged intomainwhen the version is released. Your branch merges intofeature/bug-fixes-2.6.0without conflicts, and the diff stays the same. - 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 onfeature/bug-fixes-2.6.0, so please merge it into your branch, or rebase onto it, first. - Style (minor): the codebase writes ranges as
0 ..< nrather than0 until n, as theappendDBColumnName(descriptor)overload right below yours does with1 ..< 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A supported
@DBRow @Serializableentity can declare its nullable Long primary key after its other properties:database { GeneratedKeyLastTable INSERT listOf(GeneratedKeyLast("first"), GeneratedKeyLast("second")) }fails with SQLite'snear ")": 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 passedgit diff --check: passedGradle ran with JDK 21,
-PonCICD -Pkotlin.native.enableKlibsCrossCompilation=false. MinGW also used an explicit localkotlin.native.homeafter 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.