Conversation
ryanhall07
left a comment
ryanhall07
left a comment
There was a problem hiding this comment.
did a first pass on the jdbc stuff. made some general comments that could probably be applied to the majority of the code
| const val deleteAuthor = """-- name: deleteAuthor :exec | ||
| DELETE FROM authors | ||
| WHERE id = ? | ||
| """ |
There was a problem hiding this comment.
use trimIndent or trimMargin so you can indent?
There was a problem hiding this comment.
I could, but why bother? This seems like a minor style nit
|
|
||
| @Throws(SQLException::class) | ||
| fun createAuthor(arg: CreateAuthorParams): Author { | ||
| val stmt = conn.prepareStatement(createAuthor) |
There was a problem hiding this comment.
use use when working with auto closeables.
conn.prepareStatement(createAuthor).use { stmt ->
stmt.setString(..)
}
| throw SQLException("no rows in result set") | ||
| } | ||
| val ret = Author( | ||
| results.getLong(1), |
There was a problem hiding this comment.
you can select by field name, which is slightly nicer and less brittle.
results.getLong("id")
There was a problem hiding this comment.
The generator will ensure the columns match. This won't be brittle.
There was a problem hiding this comment.
I think there is also a slight performance cost with named columns but I might be wrong.
There was a problem hiding this comment.
What does sqlc do if you use * in your SELECT?
There was a problem hiding this comment.
The sqlc parser expands the * to the actual columns in your schema and rewrites your query.
| val stmt = conn.prepareStatement(getAuthor) | ||
| stmt.setLong(1, id) | ||
|
|
||
| return stmt.executeQuery().use { results -> |
There was a problem hiding this comment.
well sometimes you are using use :P be consistent, always use it
There was a problem hiding this comment.
ah yeah, I didn't realize that closing the statement also closes the ResultSet when I first wrote this. This is better
|
|
||
| class QueriesImpl(private val conn: Connection) { | ||
|
|
||
| @Throws(SQLException::class) |
There was a problem hiding this comment.
what does this do? force java code to catch SQLException?
There was a problem hiding this comment.
Yeah, basically all JDBC functions throw SQLException, so I figured it's nice to annotate it, and for it to be checked in Java
| stmt.setInt(1, bookId) | ||
|
|
||
| return stmt.executeQuery().use { results -> | ||
| if (!results.next()) { |
There was a problem hiding this comment.
I see this pattern a lot. could probably add some kind of extension to ResultSet
fun ResultSet.uniqueResultNext() {
if (!results.next()) {
throws SQLException("no results");
}
if (results.next()) {
throw SQLException("too many");
}
results.previous()
}
jdbc is gross
There was a problem hiding this comment.
Agree, but I want to defer needing a library to use this code.
| ?, | ||
| ?, | ||
| ? | ||
| ) RETURNING id |
|
|
||
| package com.example.booktest.postgresql | ||
|
|
||
| import java.time.OffsetDateTime |
There was a problem hiding this comment.
Wow I didn't actually know about this new DateTime class. I think we tend to use LocalDate and Instant but that's probably because we predate Java 8 and we always store timestamps with UTC. This does look like a good fit as a general timezoned datetime instance.
There was a problem hiding this comment.
I didn't either lol. The postgres jdbc driver explicitly doesn't support Instant
| (results.getArray(8).array as Array<String>).toList() | ||
| )) | ||
| } | ||
| ret |
| tags | ||
| FROM books | ||
| LEFT JOIN authors ON books.author_id = authors.author_id | ||
| WHERE tags && ?::varchar[] |
|
LGTM |
|
This has been merged upstream. |
3 participants
This PR is massive, but it provides a place to leave comments to code review the generated code.
If you want to actually look through the code and try it, start here https://github.com/mightyguava/sqlc/tree/kotlin-master/examples/kotlin