Skip to content

Commit

Permalink
Do not set timestamps if already set
Browse files Browse the repository at this point in the history
GitOrigin-RevId: 07c5fc448c43ae922af1f4c32c1f479fa5b59ba7
  • Loading branch information
Sethuraman authored and svc-squareup-copybara committed Jan 10, 2025
1 parent 680bca2 commit bac1676
Show file tree
Hide file tree
Showing 6 changed files with 204 additions and 19 deletions.
6 changes: 3 additions & 3 deletions misk-jooq/api/misk-jooq.api
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public final class misk/jooq/listeners/AvoidUsingSelectStarException : java/lang
public fun <init> (Ljava/lang/String;)V
}

public final class misk/jooq/listeners/AvoidUsingSelectStarListener : org/jooq/impl/DefaultExecuteListener {
public final class misk/jooq/listeners/AvoidUsingSelectStarListener : org/jooq/ExecuteListener {
public static final field Companion Lmisk/jooq/listeners/AvoidUsingSelectStarListener$Companion;
public fun <init> ()V
public fun renderEnd (Lorg/jooq/ExecuteContext;)V
Expand All @@ -116,7 +116,7 @@ public final class misk/jooq/listeners/AvoidUsingSelectStarListener$Companion {
public final fun getSelectStarFromRegex ()Lkotlin/text/Regex;
}

public final class misk/jooq/listeners/JooqSQLLogger : org/jooq/impl/DefaultExecuteListener {
public final class misk/jooq/listeners/JooqSQLLogger : org/jooq/ExecuteListener {
public static final field Companion Lmisk/jooq/listeners/JooqSQLLogger$Companion;
public fun <init> ()V
public fun exception (Lorg/jooq/ExecuteContext;)V
Expand All @@ -131,7 +131,7 @@ public final class misk/jooq/listeners/JooqSQLLogger$Companion {
public final fun getLog ()Lmu/KLogger;
}

public final class misk/jooq/listeners/JooqTimestampRecordListener : org/jooq/impl/DefaultRecordListener {
public final class misk/jooq/listeners/JooqTimestampRecordListener : org/jooq/RecordListener {
public fun <init> (Ljava/time/Clock;Ljava/lang/String;Ljava/lang/String;)V
public fun insertStart (Lorg/jooq/RecordContext;)V
public fun updateStart (Lorg/jooq/RecordContext;)V
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package misk.jooq.listeners

import org.jooq.ExecuteContext
import org.jooq.impl.DefaultExecuteListener
import org.jooq.ExecuteListener

class AvoidUsingSelectStarListener : DefaultExecuteListener() {
class AvoidUsingSelectStarListener : ExecuteListener {

/**
* This catches any query that has a select * from or select table.* from.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,20 @@ package misk.jooq.listeners

import org.jooq.Configuration
import org.jooq.ExecuteContext
import org.jooq.ExecuteListener
import org.jooq.ExecuteType.BATCH
import org.jooq.Param
import org.jooq.TXTFormat
import org.jooq.VisitContext
import org.jooq.VisitListener
import org.jooq.VisitListenerProvider
import org.jooq.impl.DSL
import org.jooq.impl.DefaultExecuteListener
import org.jooq.impl.DefaultVisitListener
import org.jooq.impl.DefaultVisitListenerProvider
import org.jooq.tools.StringUtils
import wisp.logging.getLogger
import java.util.Arrays

class JooqSQLLogger : DefaultExecuteListener() {
class JooqSQLLogger : ExecuteListener {
override fun renderEnd(ctx: ExecuteContext) {
var configuration = ctx.configuration()
val newline = if (configuration.settings().isRenderFormatted == true) "\n" else ""
Expand Down Expand Up @@ -88,7 +88,7 @@ class JooqSQLLogger : DefaultExecuteListener() {
return configuration.derive(*newProviders)
}

private class BindValueAbbreviator : DefaultVisitListener() {
private class BindValueAbbreviator : VisitListener {
private var anyAbbreviations = false
override fun visitStart(context: VisitContext) {
if (context.renderContext() != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ package misk.jooq.listeners

import misk.jooq.toLocalDateTime
import org.jooq.RecordContext
import org.jooq.RecordListener
import org.jooq.impl.DSL
import org.jooq.impl.DefaultRecordListener
import java.time.Clock
import java.time.Instant
import java.time.LocalDateTime
import java.time.temporal.ChronoUnit

/**
* A Record Listener that will automatically set the current timestamp for the [createdAtColumnName]
Expand All @@ -14,22 +17,42 @@ class JooqTimestampRecordListener(
private val clock: Clock,
private val createdAtColumnName: String,
private val updatedAtColumnName: String,
) : DefaultRecordListener() {
) : RecordListener {

override fun updateStart(ctx: RecordContext?) {
if (ctx?.record()?.field(updatedAtColumnName) != null) {
ctx.record().set(DSL.field(updatedAtColumnName), clock.instant().toLocalDateTime())
}
setTime(ctx, updatedAtColumnName)
}

override fun insertStart(ctx: RecordContext?) {
if (ctx?.record()?.field(createdAtColumnName) != null) {
ctx.record().set(DSL.field(createdAtColumnName), clock.instant().toLocalDateTime())
}
if (ctx?.record()?.field(updatedAtColumnName) != null) {
ctx.record().set(DSL.field(updatedAtColumnName), clock.instant().toLocalDateTime())
setTime(ctx, createdAtColumnName)
setTime(ctx, updatedAtColumnName)
}

private fun setTime(ctx: RecordContext?, columnName: String) {
if (
ctx?.record()?.field(columnName) != null &&
(ctx.record().get(DSL.field(columnName)) == null || !ctx.record().changed(DSL.field(columnName)))
) {
val precision = ctx.recordType().field(DSL.field(columnName))!!.dataType.precision()
ctx.record().set(DSL.field(columnName), clock.instant().truncateBasedOnPrecision(precision))
}
}

/**
* MySQL's precision for a timestamp is millis. But in the Kube Pod, where the code runs the JVM timestamp is in
* nanos. So when we store the data, the signature is computed with nanos, but when we load the data from the DB, the
* nanos are lost. It is best we keep the same precision when we store and load the data. This method truncates the
* instant based on the precision. The check with precision is required to be able to test this on a MAC. Mac JVM's
* precision is millis. So in order to test truncation we need to create a mysql timestamp with a precision of 0. This
* also allows this signature to work for any column created in prod where the precision is 0 (in the sense,
* restricted to store seconds alone).
*/
private fun Instant.truncateBasedOnPrecision(precision: Int): LocalDateTime {
return when {
precision < 3 -> truncatedTo(ChronoUnit.SECONDS)
else -> truncatedTo(ChronoUnit.MILLIS)
}.toLocalDateTime()
}
}


Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package misk.jooq.listeners

import jakarta.inject.Inject
import misk.jooq.JooqTransacter
import misk.jooq.config.ClientJooqTestingModule
import misk.jooq.config.JooqDBIdentifier
import misk.jooq.model.Genre
import misk.jooq.testgen.tables.references.MOVIE
import misk.testing.MiskTest
import misk.testing.MiskTestModule
import org.assertj.core.api.Assertions.assertThatExceptionOfType
import org.assertj.core.api.Assertions.assertThatNoException
import org.junit.jupiter.api.Test

@MiskTest(startService = true)
class AvoidUsingSelectStarListenerTest {
@SuppressWarnings("unused") @MiskTestModule private var module = ClientJooqTestingModule()
@Inject
@JooqDBIdentifier private lateinit var transacter: JooqTransacter

@Test fun `using select star throws an exception`() {
transacter.transaction { (ctx) ->
ctx.newRecord(MOVIE).apply {
this.genre = Genre.COMEDY.name
this.name = "Enter the dragon"
}.also { it.store() }
}

assertThatExceptionOfType(AvoidUsingSelectStarException::class.java).isThrownBy {
transacter.transaction {
(ctx) -> ctx.select(MOVIE.asterisk()).from(MOVIE).fetchOne()
}
}
}

@Test fun `selecting all the fields does not throw an exception`() {
transacter.transaction { (ctx) ->
ctx.newRecord(MOVIE).apply {
this.genre = Genre.COMEDY.name
this.name = "Enter the dragon"
}.also { it.store() }
}

assertThatNoException().isThrownBy {
transacter.transaction {
(ctx) -> ctx.select(*MOVIE.fields()).from(MOVIE).fetchOne()
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
package misk.jooq.listeners

import jakarta.inject.Inject
import misk.jooq.JooqTransacter
import misk.jooq.config.ClientJooqTestingModule
import misk.jooq.config.JooqDBIdentifier
import misk.jooq.model.Genre
import misk.jooq.testgen.tables.references.MOVIE
import misk.jooq.toLocalDateTime
import misk.testing.MiskTest
import misk.testing.MiskTestModule
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
import wisp.time.FakeClock
import java.time.LocalDateTime
import java.time.temporal.ChronoUnit

@MiskTest(startService = true)
class JooqTimestampRecordListenerTest {
@SuppressWarnings("unused") @MiskTestModule private var module = ClientJooqTestingModule()
@Inject
@JooqDBIdentifier private lateinit var transacter: JooqTransacter
@Inject
private lateinit var clock: FakeClock

private val anHourBefore: LocalDateTime by lazy {
clock.instant().minus(1, ChronoUnit.HOURS).toLocalDateTime()
}

private val aMinuteBefore: LocalDateTime by lazy {
clock.instant().minus(1, ChronoUnit.MINUTES).toLocalDateTime()
}

@Test fun `should set the created and updated times if not already set`() {
val movie = transacter.transaction { (ctx) ->
ctx.newRecord(MOVIE).apply {
this.genre = Genre.COMEDY.name
this.name = "Enter the dragon"
}.also { it.store() }
}

assertThat(movie.createdAt).isEqualTo(clock.instant().toLocalDateTime())
assertThat(movie.updatedAt).isEqualTo(clock.instant().toLocalDateTime())

val fetchedMovie = transacter.transaction { (ctx) ->
ctx.selectFrom(MOVIE).where(MOVIE.NAME.eq("Enter the dragon")).fetchOne()!!
}

assertThat(fetchedMovie.createdAt).isEqualTo(clock.instant().toLocalDateTime())
assertThat(fetchedMovie.updatedAt).isEqualTo(clock.instant().toLocalDateTime())
}

@Test fun `should not set the created and updated times if already set`() {
val movie = transacter.transaction { (ctx) ->
ctx.newRecord(MOVIE).apply {
this.genre = Genre.COMEDY.name
this.name = "Enter the dragon"
this.createdAt = anHourBefore
this.updatedAt = anHourBefore
}.also { it.store() }
}

assertThat(movie.createdAt).isEqualTo(anHourBefore)
assertThat(movie.updatedAt).isEqualTo(anHourBefore)

val fetchedMovie = transacter.transaction { (ctx) ->
ctx.selectFrom(MOVIE).where(MOVIE.NAME.eq("Enter the dragon")).fetchOne()!!
}

assertThat(fetchedMovie.createdAt).isEqualTo(anHourBefore)
assertThat(fetchedMovie.updatedAt).isEqualTo(anHourBefore)
}

@Test fun `updates the updated at column during an update`() {
transacter.transaction { (ctx) ->
val record = ctx.newRecord(MOVIE).apply {
this.genre = Genre.COMEDY.name
this.name = "Enter the dragon"
this.createdAt = anHourBefore
this.updatedAt = anHourBefore
}.also { it.store() }

// force an update
record.store()
}

val movie = transacter.transaction { (ctx) ->
ctx.selectFrom(MOVIE).where(MOVIE.NAME.eq("Enter the dragon")).fetchOne()!!
}
assertThat(movie.updatedAt).isEqualTo(clock.instant().toLocalDateTime())
}

@Test fun `updated at will not be set if it has been changed before store is called`() {
transacter.transaction { (ctx) ->
val record = ctx.newRecord(MOVIE).apply {
this.genre = Genre.COMEDY.name
this.name = "Enter the dragon"
this.createdAt = anHourBefore
this.updatedAt = anHourBefore
}.also { it.store() }

// force an update
record.updatedAt = aMinuteBefore
record.store()
}

val movie = transacter.transaction { (ctx) ->
ctx.selectFrom(MOVIE).where(MOVIE.NAME.eq("Enter the dragon")).fetchOne()!!
}
assertThat(movie.updatedAt).isEqualTo(aMinuteBefore)
}
}

0 comments on commit bac1676

Please sign in to comment.