Skip to content

Commit 48f5b5f

Browse files
authoredJan 12, 2022
Dont use select * from in jooq (#2231)
* Dont use select * from in jooq * Fixed review comments
1 parent f7ebe08 commit 48f5b5f

File tree

5 files changed

+96
-3
lines changed

5 files changed

+96
-3
lines changed
 

‎misk-jooq/api/misk-jooq.api

+14
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,20 @@ public final class misk/jooq/JooqTransacter$TransacterOptions {
7272
public fun toString ()Ljava/lang/String;
7373
}
7474

75+
public final class misk/jooq/listeners/AvoidUsingSelectStarException : java/lang/RuntimeException {
76+
public fun <init> (Ljava/lang/String;)V
77+
}
78+
79+
public final class misk/jooq/listeners/AvoidUsingSelectStarListener : org/jooq/impl/DefaultExecuteListener {
80+
public static final field Companion Lmisk/jooq/listeners/AvoidUsingSelectStarListener$Companion;
81+
public fun <init> ()V
82+
public fun renderEnd (Lorg/jooq/ExecuteContext;)V
83+
}
84+
85+
public final class misk/jooq/listeners/AvoidUsingSelectStarListener$Companion {
86+
public final fun getSelectStarFromRegex ()Lkotlin/text/Regex;
87+
}
88+
7589
public final class misk/jooq/listeners/JooqSQLLogger : org/jooq/impl/DefaultExecuteListener {
7690
public static final field Companion Lmisk/jooq/listeners/JooqSQLLogger$Companion;
7791
public fun <init> ()V

‎misk-jooq/src/main/kotlin/misk/jooq/JooqModule.kt

+8-2
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import misk.jdbc.DataSourceService
1111
import misk.jdbc.DatabasePool
1212
import misk.jdbc.JdbcModule
1313
import misk.jdbc.RealDatabasePool
14+
import misk.jooq.listeners.AvoidUsingSelectStarListener
1415
import misk.jooq.listeners.JooqSQLLogger
1516
import misk.jooq.listeners.JooqTimestampRecordListener
1617
import misk.jooq.listeners.JooqTimestampRecordListenerOptions
@@ -21,7 +22,7 @@ import org.jooq.conf.MappedSchema
2122
import org.jooq.conf.RenderMapping
2223
import org.jooq.conf.Settings
2324
import org.jooq.impl.DSL
24-
import org.jooq.impl.DefaultConfiguration
25+
import org.jooq.impl.DefaultExecuteListenerProvider
2526
import org.jooq.impl.DefaultTransactionProvider
2627
import java.time.Clock
2728
import javax.inject.Inject
@@ -113,9 +114,14 @@ class JooqModule(
113114
false
114115
)
115116
).apply {
117+
val executeListeners = mutableListOf(
118+
DefaultExecuteListenerProvider(AvoidUsingSelectStarListener())
119+
)
116120
if ("true" == datasourceConfig.show_sql) {
117-
set(JooqSQLLogger())
121+
executeListeners.add(DefaultExecuteListenerProvider(JooqSQLLogger()))
118122
}
123+
set(*executeListeners.toTypedArray())
124+
119125
if(jooqTimestampRecordListenerOptions.install) {
120126
set(JooqTimestampRecordListener(
121127
clock = clock,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
package misk.jooq.listeners
2+
3+
import org.jooq.ExecuteContext
4+
import org.jooq.impl.DefaultExecuteListener
5+
6+
class AvoidUsingSelectStarListener : DefaultExecuteListener() {
7+
8+
/**
9+
* This catches any query that has a select * from or select table.* from.
10+
* We don't want to use any query that uses a select * in it, as jooq has a hard time converting
11+
* the result set into a jooq table record. It captures the result set via indexes and not the
12+
* column names. If you try to fetch the result set into a jooq record, jooq will expect the order
13+
* in which the columns are returned in the query matches the order of in which the columns are
14+
* declared in the jooq generated code.
15+
* I suppose it does ResultSet.get(0), ResulSet.get(1) instead of doing ResultSet.get(<column name)
16+
*
17+
* If the databases in dev, staging and prod don't all have the same column ordering, then things
18+
* start to fail.
19+
*
20+
* Either way from a code maintainability point of view it is best to avoid `select * from` and
21+
* always specify the columns you need. If you need all the columns in a table 2 ways of doing that
22+
* in jooq
23+
* ```
24+
* ctx.selectFrom(<table name>)...
25+
* ```
26+
* If you are joining multiple tables and need the columns of only one table
27+
*
28+
* ```
29+
* ctx.select(<jooq gen table>.fields().toList()).from(<table>.innerJoin....)
30+
* ```
31+
*
32+
* DO NOT DO THIS:
33+
* ```
34+
* ctx.select(<jooq gen table>.asterisk()).from(<table>)...
35+
* ```
36+
* This listener's purpose is to catch the above and prevent it from happening.
37+
*/
38+
override fun renderEnd(ctx: ExecuteContext?) {
39+
if (ctx?.sql()?.matches(selectStarFromRegex) == true) {
40+
throw AvoidUsingSelectStarException(
41+
"Do not use select * from. " +
42+
"Please read the docs of class AvoidUsingSelectStartListener#renderEnd to learn more. " +
43+
"Generated [sql=${ctx.sql()}"
44+
)
45+
}
46+
}
47+
48+
companion object {
49+
val selectStarFromRegex = Regex(
50+
"select(.*?\\.\\*.*?)from.*",
51+
setOf(RegexOption.IGNORE_CASE, RegexOption.MULTILINE)
52+
)
53+
}
54+
}
55+
56+
class AvoidUsingSelectStarException(message: String) : RuntimeException(message) {}

‎misk-jooq/src/test/kotlin/misk/jooq/JooqTransacterTest.kt

+14
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import misk.jooq.config.ClientJooqTestingModule
66
import misk.jooq.config.DeleteOrUpdateWithoutWhereException
77
import misk.jooq.config.JooqDBIdentifier
88
import misk.jooq.config.JooqDBReadOnlyIdentifier
9+
import misk.jooq.listeners.AvoidUsingSelectStarException
910
import misk.jooq.model.Genre
1011
import misk.jooq.testgen.tables.references.MOVIE
1112
import misk.testing.MiskTest
@@ -320,4 +321,17 @@ internal class JooqTransacterTest {
320321
}
321322
}
322323

324+
@Test fun `using select star throws an exception`() {
325+
assertThatExceptionOfType(AvoidUsingSelectStarException::class.java).isThrownBy {
326+
transacter.transaction { (ctx) ->
327+
ctx.newRecord(MOVIE).apply {
328+
this.genre = Genre.COMEDY.name
329+
this.name = "Dumb and dumber"
330+
}.also { it.store() }
331+
332+
ctx.select(MOVIE.asterisk()).from(MOVIE).fetchOne()
333+
}
334+
}
335+
}
336+
323337
}

‎misk-jooq/src/test/kotlin/misk/jooq/config/ClientJooqTestingModule.kt

+4-1
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import misk.jdbc.JdbcTestingModule
1010
import misk.jooq.JooqModule
1111
import misk.jooq.listeners.JooqTimestampRecordListenerOptions
1212
import misk.logging.LogCollectorModule
13+
import org.jooq.impl.DefaultExecuteListenerProvider
1314
import wisp.deployment.TESTING
1415
import javax.inject.Qualifier
1516

@@ -47,7 +48,9 @@ class ClientJooqTestingModule : KAbstractModule() {
4748
),
4849
readerQualifier = JooqDBReadOnlyIdentifier::class
4950
) {
50-
set(DeleteOrUpdateWithoutWhereListener())
51+
val executeListeners = this.executeListenerProviders().toMutableList()
52+
.apply { add(DefaultExecuteListenerProvider(DeleteOrUpdateWithoutWhereListener())) }
53+
set(*executeListeners.toTypedArray())
5154
})
5255
install(JdbcTestingModule(JooqDBIdentifier::class))
5356
install(LogCollectorModule())

0 commit comments

Comments
 (0)
Please sign in to comment.