feat!: add executeReturning for DML statements that return records#323
Merged
driessamyn merged 1 commit intoMar 27, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds executeReturning APIs to run DML with RETURNING and map returned rows; includes Kapper interface and KapperImpl additions, Kotlin Connection extensions, unit and integration tests, and documentation/README updates. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Kapper as Kapper
participant Impl as KapperImpl
participant DB as Connection
participant RS as ResultSet
participant Mapper as Mapper
Client->>Kapper: executeReturning(clazz, conn, sql, args/obj)
Kapper->>Impl: executeReturning(..., mapper, args/obj)
alt object-args
Impl->>DB: prepareStatement(sql)
DB-->>Impl: PreparedStatement
Impl->>DB: executeQuery()
else map-args
Impl->>DB: executeQuery(queryFactory(sql), args)
end
DB-->>RS: ResultSet
loop per row
Impl->>RS: next()
RS-->>Impl: true
Impl->>RS: extractFields()
RS-->>Impl: Map<String,Field>
Impl->>Mapper: mapper(ResultSet, fields)
Mapper-->>Impl: T
Impl-->>Impl: append T to list
end
RS-->>Impl: next() = false
Impl-->>Kapper: List<T>
Kapper-->>Client: List<T>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.kt`:
- Around line 107-118: The SQLException handler currently wraps only the
result-reading code inside the executeReturning overloads, so failures thrown by
connection.executeQuery(...) escape; move the try-catch to wrap the entire
executeQuery(...).use { ... } block in both executeReturning methods (the
Args-based executeReturning and the object-args executeReturning) so any
SQLException from executeQuery or from iterating the ResultSet is caught and
rethrown as KapperQueryException with the same log message via logger.warn; also
add a regression test that simulates executeQuery() throwing to ensure the
SQLException is wrapped.
In `@core/src/main/kotlin/net/samyn/kapper/Kapper.kt`:
- Around line 231-245: The current Kapper.executeReturning overloads use a
single generic T for both the argument carrier (obj/args) and the mapped result,
preventing patterns like CreateFoo -> Foo; change the signatures to two generics
(e.g. R : Any for the returned row and A : Any for the argument carrier) in
Kapper.executeReturning and the other overload at lines 260-267, obtain the
mapper for R via mapperRegistry.get(R::class) and pass mapper::createInstance as
the row factory while keeping obj: A and args: Map<String,(A)->Any?>, and update
the delegated call to Connection.executeReturning to the new generic split so
types line up (reference symbols: Kapper.executeReturning, mapperRegistry.get,
mapper::createInstance, Connection.executeReturning).
- Around line 212-218: Add default implementations for the two abstract
executeReturning overloads so external implementations remain binary compatible:
implement the Args-based executeReturning(clazz, connection, sql, mapper, args)
to simply delegate to query(connection, sql, args, mapper) (matching the
existing delegation pattern), and implement the object-based
executeReturning(clazz, connection, sql, mapper, vararg argsObjects) to
materialize an Args map from the provided objects and then delegate to the
Args-based executeReturning; keep method signatures (executeReturning, query,
querySingle) and let KapperImpl still override for optimized paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3fa0839-1748-4311-a4a3-b89f101f1ddb
📒 Files selected for processing (4)
core/src/main/kotlin/net/samyn/kapper/Kapper.ktcore/src/main/kotlin/net/samyn/kapper/KapperKotlinExecuteReturningFun.ktcore/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.ktcore/src/test/kotlin/net/samyn/kapper/internal/KapperImplExecuteReturningTest.kt
Comment on lines
+107
to
+118
| connection.executeQuery(queryFactory(sql), args).use { rs -> | ||
| try { | ||
| val fields = rs.extractFields(connection.getDbFlavour()) | ||
| while (rs.next()) { | ||
| add(mapper(rs, fields)) | ||
| } | ||
| } catch (e: SQLException) { | ||
| "Failed to execute statement: $sql".also { | ||
| logger.warn(it, e) | ||
| throw KapperQueryException(it, e) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the KapperImpl.kt file
find . -name "KapperImpl.kt" -type fRepository: driessamyn/kapper
Length of output: 123
🏁 Script executed:
# Get the file size to determine how to read it
wc -l core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.ktRepository: driessamyn/kapper
Length of output: 125
🏁 Script executed:
# Read the specific lines mentioned in the review (107-118 and 138-149)
sed -n '100,160p' core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.kt | cat -nRepository: driessamyn/kapper
Length of output: 2753
🏁 Script executed:
# Verify the exact line numbers and context around the two methods mentioned
sed -n '95,125p' core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.kt | cat -nRepository: driessamyn/kapper
Length of output: 1273
🏁 Script executed:
# Verify the second overload (138-149 range)
sed -n '130,160p' core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.kt | cat -nRepository: driessamyn/kapper
Length of output: 1503
Move the try-catch block to wrap the entire executeQuery() call and result iteration.
In both executeReturning overloads, executeQuery() (line 107 for the Args-based variant, line 138 for the object-args variant) is called outside the SQLException handler. If executeQuery() fails—including with unsupported RETURNING syntax—the exception escapes as raw SQLException instead of being wrapped in KapperQueryException.
Restructure both methods to wrap the execute and read operations together:
Example fix structure
try {
connection.executeQuery(...).use { rs ->
// result set operations
}
} catch (e: SQLException) {
// wrap in KapperQueryException
}Add a regression test that throws directly from executeQuery().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.kt` around lines
107 - 118, The SQLException handler currently wraps only the result-reading code
inside the executeReturning overloads, so failures thrown by
connection.executeQuery(...) escape; move the try-catch to wrap the entire
executeQuery(...).use { ... } block in both executeReturning methods (the
Args-based executeReturning and the object-args executeReturning) so any
SQLException from executeQuery or from iterating the ResultSet is caught and
rethrown as KapperQueryException with the same log message via logger.warn; also
add a regression test that simulates executeQuery() throwing to ensure the
SQLException is wrapped.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.kt (1)
16-18: Remove the no-opsetupDatabaseoverride.This override only delegates to
superand doesn’t add behavior.Proposed change
- override fun setupDatabase(connection: Connection) { - super.setupDatabase(connection) - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.kt` around lines 16 - 18, The override of setupDatabase in ExecuteReturningTests (override fun setupDatabase(connection: Connection)) is a no-op that only calls super; remove this method entirely from the class so it inherits the base implementation directly and avoid dead code. Ensure there are no class-specific comments or annotations tied to that method before deleting.core/src/test/java/net/samyn/kapper/KapperJavaApiTest.java (2)
323-327: UseRETURNINGin theseexecuteReturningtest SQL strings.These tests exercise
executeReturning, but the SQL examples are plainINSERTstatements. Updating them to includeRETURNINGkeeps the Java API tests aligned with intended usage and avoids misleading examples.Proposed change
- "INSERT INTO test_table(id, name) VALUES(:id, :name)", + "INSERT INTO test_table(id, name) VALUES(:id, :name) RETURNING id, name",- "INSERT INTO test_table(id, name) VALUES(:id, :name)", + "INSERT INTO test_table(id, name) VALUES(:id, :name) RETURNING id, name",- "INSERT INTO test_table(id, name) VALUES(:id, :name)", + "INSERT INTO test_table(id, name) VALUES(:id, :name) RETURNING id, name",Also applies to: 346-350, 379-383
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/net/samyn/kapper/KapperJavaApiTest.java` around lines 323 - 327, Update the SQL strings used in the Kapper.getInstance().executeReturning calls in KapperJavaApiTest (e.g., the call to executeReturning(AutomappedTestEntity.class, mockConnection, "INSERT INTO test_table(id, name) VALUES(:id, :name)", args)) to include a RETURNING clause (for example RETURNING id, name) so the test SQL matches executeReturning semantics; make the same change for the other occurrences around the test class (the similar executeReturning calls at the other noted spots).
333-335: Also assertResultSetcleanup in newexecuteReturningtests.The new tests assert statement closure but not result-set closure. Adding
verify(mockResultSet).close()keeps cleanup checks consistent with existing query tests and catches resource-management regressions.Proposed change
verify(mockStatement).setInt(1, 1); verify(mockStatement).setString(2, "Test1"); verify(mockStatement).close(); + verify(mockResultSet).close();verify(mockStatement).setInt(1, 1); verify(mockStatement).setString(2, "Test1"); verify(mockStatement).close(); + verify(mockResultSet).close();verify(mockStatement).setInt(1, 1); verify(mockStatement).setString(2, "Test1"); verify(mockStatement).close(); + verify(mockResultSet).close();Also applies to: 366-368, 393-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/net/samyn/kapper/KapperJavaApiTest.java` around lines 333 - 335, The new executeReturning tests in KapperJavaApiTest (where you currently call verify(mockStatement).setInt/setString and verify(mockStatement).close()) are missing verification that the ResultSet is closed; add verify(mockResultSet).close() after the statement-close verifications in those test methods (also update the similar assertions around the other two test blocks that correspond to lines 366-368 and 393-395) so the tests assert ResultSet cleanup just like the existing query tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.kt`:
- Around line 85-95: The test function "SQL Insert with custom mapper returning
selected fields" currently uses RETURNING * which returns all columns and
doesn't exercise the selected-field mapping; update the SQL passed to
connection.executeReturning to return only the projected column (e.g., change
the RETURNING clause from "*" to "name" or another specific column) so the
mapper { rs, _ -> rs.getString("name") } is validated against a selected-field
result set.
---
Nitpick comments:
In `@core/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.kt`:
- Around line 16-18: The override of setupDatabase in ExecuteReturningTests
(override fun setupDatabase(connection: Connection)) is a no-op that only calls
super; remove this method entirely from the class so it inherits the base
implementation directly and avoid dead code. Ensure there are no class-specific
comments or annotations tied to that method before deleting.
In `@core/src/test/java/net/samyn/kapper/KapperJavaApiTest.java`:
- Around line 323-327: Update the SQL strings used in the
Kapper.getInstance().executeReturning calls in KapperJavaApiTest (e.g., the call
to executeReturning(AutomappedTestEntity.class, mockConnection, "INSERT INTO
test_table(id, name) VALUES(:id, :name)", args)) to include a RETURNING clause
(for example RETURNING id, name) so the test SQL matches executeReturning
semantics; make the same change for the other occurrences around the test class
(the similar executeReturning calls at the other noted spots).
- Around line 333-335: The new executeReturning tests in KapperJavaApiTest
(where you currently call verify(mockStatement).setInt/setString and
verify(mockStatement).close()) are missing verification that the ResultSet is
closed; add verify(mockResultSet).close() after the statement-close
verifications in those test methods (also update the similar assertions around
the other two test blocks that correspond to lines 366-368 and 393-395) so the
tests assert ResultSet cleanup just like the existing query tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51581962-c7bc-4b14-84c6-3da8207ebfd0
📒 Files selected for processing (4)
README.mdcore/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.ktcore/src/test/java/net/samyn/kapper/KapperJavaApiTest.javadocs/guide/queries.md
✅ Files skipped from review due to trivial changes (2)
- README.md
- docs/guide/queries.md
Comment on lines
+85
to
+95
| fun `SQL Insert with custom mapper returning selected fields`() { | ||
| assumeTrue(connection.getDbFlavour() in returningDbs, "RETURNING clause not supported on this database") | ||
| val newHero = batman.copy(id = UUID.randomUUID()) | ||
| val results = | ||
| connection.executeReturning( | ||
| """ | ||
| INSERT INTO super_heroes_$testId(id, name, email, age) | ||
| VALUES(:id, :name, :email, :age) | ||
| RETURNING * | ||
| """, | ||
| mapper = { rs, _ -> rs.getString("name") }, |
There was a problem hiding this comment.
This test says “selected fields” but currently returns all columns.
RETURNING * doesn’t validate selected-field mapping behavior. Use a projected return (e.g., RETURNING name) so the test matches its intent.
Proposed change
- RETURNING *
+ RETURNING name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.kt`
around lines 85 - 95, The test function "SQL Insert with custom mapper returning
selected fields" currently uses RETURNING * which returns all columns and
doesn't exercise the selected-field mapping; update the SQL passed to
connection.executeReturning to return only the projected column (e.g., change
the RETURNING clause from "*" to "name" or another specific column) so the
mapper { rs, _ -> rs.getString("name") } is validated against a selected-field
result set.
driessamyn
reviewed
Mar 26, 2026
Owner
driessamyn
left a comment
driessamyn
left a comment
There was a problem hiding this comment.
Thanks for the contribution @owenstanfordSC
Just a couple of minor comment from me, and I think the coderabit comments are also valid.
In particular the comment about the API comment about the same input/output type. I think we should use <I : Any, O : Any>
| data class User(val id: Long, val name: String, val email: String) | ||
|
|
||
| val inserted: List<User> = connection.executeReturning<User>( | ||
| "INSERT INTO users(name, email) VALUES(:name, :email) ...", |
Owner
There was a problem hiding this comment.
It might be nice to have a RETURNING in the example to make the purpose of the API clearer.
5 tasks
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/main/kotlin/net/samyn/kapper/KapperKotlinExecuteReturningFun.kt (1)
31-36: Exception documentation may not reflect actual behavior.The
@throws java.sql.SQLExceptionannotation documents SQLException, but according to the implementation inKapperImpl.kt, the actual exceptions thrown are:
IllegalArgumentExceptionif SQL is blankKapperQueryExceptionwrapping SQLException on execution failureConsider updating the documentation to accurately reflect the thrown exceptions, or verify that
KapperQueryExceptionis a subclass ofSQLException.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/kotlin/net/samyn/kapper/KapperKotlinExecuteReturningFun.kt` around lines 31 - 36, The KDoc on Connection.executeReturning currently lists only java.sql.SQLException but the implementation (see KapperImpl.kt) actually throws IllegalArgumentException for blank SQL and wraps SQL errors in KapperQueryException; update the documentation for executeReturning (and its overloads) to list both IllegalArgumentException and KapperQueryException (noting it wraps SQLException), or alternatively make KapperQueryException extend SQLException if you prefer a single SQLException-type; reference the Connection.executeReturning inline function and KapperImpl.kt behavior when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/src/main/kotlin/net/samyn/kapper/KapperKotlinExecuteReturningFun.kt`:
- Around line 31-36: The KDoc on Connection.executeReturning currently lists
only java.sql.SQLException but the implementation (see KapperImpl.kt) actually
throws IllegalArgumentException for blank SQL and wraps SQL errors in
KapperQueryException; update the documentation for executeReturning (and its
overloads) to list both IllegalArgumentException and KapperQueryException
(noting it wraps SQLException), or alternatively make KapperQueryException
extend SQLException if you prefer a single SQLException-type; reference the
Connection.executeReturning inline function and KapperImpl.kt behavior when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32b3aba2-8219-4fa2-9d63-3cc82aedff17
📒 Files selected for processing (4)
core/src/main/kotlin/net/samyn/kapper/Kapper.ktcore/src/main/kotlin/net/samyn/kapper/KapperKotlinExecuteReturningFun.ktcore/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.ktdocs/guide/queries.md
✅ Files skipped from review due to trivial changes (1)
- docs/guide/queries.md
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.kt
- core/src/main/kotlin/net/samyn/kapper/Kapper.kt
owenstanfordSC
force-pushed
the
feat/execute-returning
branch
from
113007e to
ea3c097
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/test/java/net/samyn/kapper/KapperJavaApiTest.java (1)
313-336: Consider verifyingmockResultSet.close()for consistency.Other similar tests in this file (e.g.,
testQueryWithCustomMapperat line 160,testQuerySingleWithCustomMapperat line 256) verify thatmockResultSet.close()is called. Adding this verification would ensure theexecuteReturningimplementation properly closes resources.🔧 Suggested addition
verify(mockStatement).setInt(1, 1); verify(mockStatement).setString(2, "Test1"); verify(mockStatement).close(); + verify(mockResultSet).close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/net/samyn/kapper/KapperJavaApiTest.java` around lines 313 - 336, The test testExecuteReturningWithMap should also verify that the ResultSet is closed to match other tests; update the test method to add a verification for mockResultSet.close() after executeReturning completes so that Kapper.getInstance().executeReturning is validated for proper resource cleanup (i.e., include a verify(mockResultSet).close() call in testExecuteReturningWithMap alongside the existing verify calls for mockStatement.close()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/src/test/java/net/samyn/kapper/KapperJavaApiTest.java`:
- Around line 313-336: The test testExecuteReturningWithMap should also verify
that the ResultSet is closed to match other tests; update the test method to add
a verification for mockResultSet.close() after executeReturning completes so
that Kapper.getInstance().executeReturning is validated for proper resource
cleanup (i.e., include a verify(mockResultSet).close() call in
testExecuteReturningWithMap alongside the existing verify calls for
mockStatement.close()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b68187a1-5fee-4b01-b629-2a4b59118226
📒 Files selected for processing (8)
README.mdcore/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.ktcore/src/main/kotlin/net/samyn/kapper/Kapper.ktcore/src/main/kotlin/net/samyn/kapper/KapperKotlinExecuteReturningFun.ktcore/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.ktcore/src/test/java/net/samyn/kapper/KapperJavaApiTest.javacore/src/test/kotlin/net/samyn/kapper/internal/KapperImplExecuteReturningTest.ktdocs/guide/queries.md
✅ Files skipped from review due to trivial changes (3)
- docs/guide/queries.md
- README.md
- core/src/main/kotlin/net/samyn/kapper/Kapper.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.kt
- core/src/test/kotlin/net/samyn/kapper/internal/KapperImplExecuteReturningTest.kt
- core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.kt
owenstanfordSC
force-pushed
the
feat/execute-returning
branch
from
90fb513 to
e38c9c8
Compare
owenstanfordSC
changed the title
Owner
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/src/main/kotlin/net/samyn/kapper/Kapper.kt (1)
212-218:⚠️ Potential issue | 🟠 MajorKeep new
executeReturningmapper overloads defaulted to avoid interface ABI break.Line 212 and Line 270 introduce new abstract methods on a public interface. External
Kapperimplementations compiled before this change can fail when these methods are called. Consider giving both overloads default bodies and delegate internally.Suggested compatibility-preserving delegation
fun <T : Any> executeReturning( clazz: Class<T>, connection: Connection, sql: String, mapper: (ResultSet, Map<String, Field>) -> T, args: Args, - ): List<T> + ): List<T> = query(clazz, connection, sql, mapper, args) @@ fun <R : Any, A : Any> executeReturning( clazz: Class<R>, connection: Connection, sql: String, mapper: (ResultSet, Map<String, Field>) -> R, obj: A, args: Map<String, (A) -> Any?>, - ): List<R> + ): List<R> = + executeReturning( + clazz = clazz, + connection = connection, + sql = sql, + mapper = mapper, + args = args.mapValues { (_, extractor) -> extractor(obj) }, + )#!/bin/bash set -euo pipefail echo "1) Show newly added executeReturning signatures in Kapper interface" rg -n 'fun <.*> executeReturning\(' core/src/main/kotlin/net/samyn/kapper/Kapper.kt -C2 echo echo "2) Check in-repo implementations of Kapper (external implementations won't be visible here)" rg -nP --type=kotlin 'class\s+\w+\s*:\s*Kapper\b|object\s+\w+\s*:\s*Kapper\b' echo echo "3) Inspect JVM default-method compiler settings affecting interface method generation" rg -n --type=gradle 'jvmDefault|-Xjvm-default|JvmDefault'Also applies to: 270-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/kotlin/net/samyn/kapper/Kapper.kt` around lines 212 - 218, The Kapper interface gained new executeReturning overloads that are abstract and break ABI; add default implementations on the Kapper interface for both new signatures (the executeReturning overload that takes clazz, connection, sql, mapper, args and the other overload at ~line 270) that delegate to the existing older methods (use the existing executeReturning/execute variants or the primary execute implementation already on Kapper), forwarding parameters and adapting the mapper call as needed so pre-compiled external implementations keep working; implement these defaults inside interface Kapper so callers use the new overloads without requiring implementors to recompile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/src/main/kotlin/net/samyn/kapper/Kapper.kt`:
- Around line 212-218: The Kapper interface gained new executeReturning
overloads that are abstract and break ABI; add default implementations on the
Kapper interface for both new signatures (the executeReturning overload that
takes clazz, connection, sql, mapper, args and the other overload at ~line 270)
that delegate to the existing older methods (use the existing
executeReturning/execute variants or the primary execute implementation already
on Kapper), forwarding parameters and adapting the mapper call as needed so
pre-compiled external implementations keep working; implement these defaults
inside interface Kapper so callers use the new overloads without requiring
implementors to recompile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b36752ea-1a90-4643-949f-afe78f7d5c85
📒 Files selected for processing (8)
README.mdcore/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.ktcore/src/main/kotlin/net/samyn/kapper/Kapper.ktcore/src/main/kotlin/net/samyn/kapper/KapperKotlinExecuteReturningFun.ktcore/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.ktcore/src/test/java/net/samyn/kapper/KapperJavaApiTest.javacore/src/test/kotlin/net/samyn/kapper/internal/KapperImplExecuteReturningTest.ktdocs/guide/queries.md
✅ Files skipped from review due to trivial changes (2)
- docs/guide/queries.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.kt
- core/src/test/java/net/samyn/kapper/KapperJavaApiTest.java
- core/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.kt
- core/src/test/kotlin/net/samyn/kapper/internal/KapperImplExecuteReturningTest.kt
- core/src/main/kotlin/net/samyn/kapper/KapperKotlinExecuteReturningFun.kt
Adds a first-class executeReturning API so callers can run INSERT/UPDATE/ UPSERT statements with a RETURNING clause and get back mapped results, rather than relying on the undocumented querySingle-for-RETURNING workaround. - Kapper interface: four overloads (map-args + object-args, each with auto-mapper and custom mapper variants) - KapperImpl: two abstract implementations — map-args reuses the existing executeQuery path; object-args combines object-based parameter binding from execute() with stmt.executeQuery() result processing - KapperKotlinExecuteReturningFun: six Kotlin extension functions mirroring the query/execute extension function style - KapperImplExecuteReturningTest: 14 unit tests covering both variants Rejected: route execute() through executeQuery() on RETURNING detection | breaking API change (return type changes from Int to List<T>) Rejected: document querySingle workaround only | fragile, breaks if DML routing ever changes Confidence: high Scope-risk: narrow Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
owenstanfordSC
force-pushed
the
feat/execute-returning
branch
from
e38c9c8 to
bc78ad6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.kt (1)
98-121: Code replicatesquery()method - consider extracting shared logic.The map-args
executeReturningimplementation (lines 98-121) is nearly identical toquery()(lines 23-46), differing only in the error message ("Failed to execute statement" vs "Failed to execute query"). This duplication could be reduced by extracting the shared iteration logic into a private helper.Also, the try-catch wraps only the ResultSet iteration (lines 108-118), not the
executeQuery()call at line 107. IfexecuteQuery()throws—e.g., due to unsupportedRETURNINGsyntax—theSQLExceptionescapes unwrapped. However, this matches the existingquery()pattern, so maintaining consistency is reasonable. If you decide to address this gap, apply the fix to both methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.kt` around lines 98 - 121, The executeReturning implementation duplicates query's ResultSet iteration and error handling; extract the shared logic into a private helper (e.g., processResultSet or iterateResultSet) that accepts the ResultSet, mapper, and SQL-context and performs rs.extractFields(...), rs.next() loop, logger.warn and throw KapperQueryException on SQLException, then call that helper from both query(...) and executeReturning(...); optionally, to prevent an executeQuery() SQLException from escaping, wrap the connection.executeQuery(queryFactory(sql), args) call with the same try-catch in both executeReturning and query so all SQLExceptions are logged and rethrown as KapperQueryException with the same message.core/src/test/kotlin/net/samyn/kapper/internal/KapperImplExecuteReturningTest.kt (1)
146-159: Consider adding a test forexecuteQuery()failure.This test verifies that
SQLExceptionfromrs.next()is wrapped asKapperQueryException. However, there's no test for whenexecuteQuery()itself throws. Adding such a test would:
- Document expected behavior when the RETURNING clause is unsupported
- Serve as a regression test if the try-catch scope is later adjusted
Example test
`@Test` fun `executeReturning throws when executeQuery fails`() { val ex = SQLException("RETURNING not supported") every { mockConnection.executeQuery(any(), any()) } throws ex shouldThrow<KapperQueryException> { kapper.executeReturning( TestEntity::class.java, mockConnection, mockSqlTemplate, mockMapper, mapOf("name" to "Superman"), ) }.cause shouldBe ex }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/kotlin/net/samyn/kapper/internal/KapperImplExecuteReturningTest.kt` around lines 146 - 159, Add a new unit test in KapperImplExecuteReturningTest that verifies kapper.executeReturning wraps exceptions thrown by mockConnection.executeQuery into KapperQueryException: create an SQLException (e.g. "RETURNING not supported"), stub mockConnection.executeQuery(any(), any()) to throw that exception, call kapper.executeReturning(TestEntity::class.java, mockConnection, mockSqlTemplate, mockMapper, mapOf("name" to "Superman")), and assert the thrown KapperQueryException.cause is the original SQLException; reference kapper.executeReturning and mockConnection.executeQuery when locating where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.kt`:
- Around line 98-121: The executeReturning implementation duplicates query's
ResultSet iteration and error handling; extract the shared logic into a private
helper (e.g., processResultSet or iterateResultSet) that accepts the ResultSet,
mapper, and SQL-context and performs rs.extractFields(...), rs.next() loop,
logger.warn and throw KapperQueryException on SQLException, then call that
helper from both query(...) and executeReturning(...); optionally, to prevent an
executeQuery() SQLException from escaping, wrap the
connection.executeQuery(queryFactory(sql), args) call with the same try-catch in
both executeReturning and query so all SQLExceptions are logged and rethrown as
KapperQueryException with the same message.
In
`@core/src/test/kotlin/net/samyn/kapper/internal/KapperImplExecuteReturningTest.kt`:
- Around line 146-159: Add a new unit test in KapperImplExecuteReturningTest
that verifies kapper.executeReturning wraps exceptions thrown by
mockConnection.executeQuery into KapperQueryException: create an SQLException
(e.g. "RETURNING not supported"), stub mockConnection.executeQuery(any(), any())
to throw that exception, call kapper.executeReturning(TestEntity::class.java,
mockConnection, mockSqlTemplate, mockMapper, mapOf("name" to "Superman")), and
assert the thrown KapperQueryException.cause is the original SQLException;
reference kapper.executeReturning and mockConnection.executeQuery when locating
where to add the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4d8c948-2b62-4b51-9c26-8b99238b752f
📒 Files selected for processing (8)
README.mdcore/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.ktcore/src/main/kotlin/net/samyn/kapper/Kapper.ktcore/src/main/kotlin/net/samyn/kapper/KapperKotlinExecuteReturningFun.ktcore/src/main/kotlin/net/samyn/kapper/internal/KapperImpl.ktcore/src/test/java/net/samyn/kapper/KapperJavaApiTest.javacore/src/test/kotlin/net/samyn/kapper/internal/KapperImplExecuteReturningTest.ktdocs/guide/queries.md
✅ Files skipped from review due to trivial changes (2)
- README.md
- docs/guide/queries.md
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/test/java/net/samyn/kapper/KapperJavaApiTest.java
- core/src/integrationTest/kotlin/net/samyn/kapper/ExecuteReturningTests.kt
- core/src/main/kotlin/net/samyn/kapper/Kapper.kt
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
2 participants
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.
Summary
executeReturning<T>()API so callers can execute DML statements that return records and map the results to typed objectsquerySingle()for DML, which is fragile if internal routing ever changesquery/executeAPI — same overload pattern, same extension function style, same test conventionsChanges
Kapper.kt— 4 new interface methods (map-args + object-args, each with auto-mapper and custom-mapper variants)KapperImpl.kt— 2 implementations: map-args reuses the existingexecuteQuerypath; object-args combines object-based parameter binding with result set processingKapperKotlinExecuteReturningFun.kt— 6 Kotlin extension functionsKapperImplExecuteReturningTest.kt— 14 unit tests, all passingTest plan
KapperImplExecuteReturningTest— all passing:kapper:testsuite passes with no regressions🤖 Generated with Claude Code
Summary by CodeRabbit