Skip to content

Commit

Permalink
fix(sql): Ensure that Application.createTs is reliably returned whe…
Browse files Browse the repository at this point in the history
…n sql is used (#1014)

For historical reasons, the original `createTs` value on the Application object itself
is not reliably stored.

When sql backends are used, the `created_at` column is the most accurate representation
of the time at which an Application was created.

This PR will override any `createTs` value that happens to exist in the Application
json blob with the value of the db column.
  • Loading branch information
ajordens authored Feb 16, 2021
1 parent ca8db52 commit 9fd2fc7
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.netflix.spinnaker.front50.model;

import com.fasterxml.jackson.annotation.JsonIgnore;

public interface Timestamped {
/**
* TODO(rz): Move this method into new Identifiable interface. Has nothing to do with timestamps.
Expand All @@ -31,4 +33,13 @@ public interface Timestamped {
String getLastModifiedBy();

void setLastModifiedBy(String lastModifiedBy);

default void setCreatedAt(Long createdAt) {
// do nothing
}

@JsonIgnore
default Long getCreatedAt() {
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,18 @@ public void setLastModified(Long lastModified) {
this.updateTs = lastModified.toString();
}

@Override
public void setCreatedAt(Long createdAt) {
if (createdAt != null) {
this.createTs = createdAt.toString();
}
}

@Override
public Long getCreatedAt() {
return Strings.isNullOrEmpty(createTs) ? null : Long.valueOf(createTs);
}

@Override
public String toString() {
return "Application{"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@ class SqlStorageService(
val result = withPool(poolName) {
jooq.withRetry(sqlRetryProperties.reads) { ctx ->
ctx
.select(field("body", String::class.java))
.select(
field("body", String::class.java),
field("created_at", Long::class.java)
)
.from(definitionsByType[objectType]!!.tableName)
.where(
field("id", String::class.java).eq(objectKey).and(
Expand All @@ -101,7 +104,12 @@ class SqlStorageService(
} ?: throw NotFoundException("Object not found (key: $objectKey)")
}

return objectMapper.readValue(result.get(field("body", String::class.java)), objectType.clazz as Class<T>)
return objectMapper.readValue(
result.get(field("body", String::class.java)),
objectType.clazz as Class<T>
).apply {
this.createdAt = result.get(field("created_at", Long::class.java))
}
}

override fun <T : Timestamped> loadObjects(objectType: ObjectType, objectKeys: List<String>): List<T> {
Expand All @@ -110,23 +118,30 @@ class SqlStorageService(
val timeToLoadObjects = measureTimeMillis {
objects.addAll(
objectKeys.chunked(chunkSize).flatMap { keys ->
val bodies = withPool(poolName) {
val records = withPool(poolName) {
jooq.withRetry(sqlRetryProperties.reads) { ctx ->
ctx
.select(field("body", String::class.java))
.select(
field("body", String::class.java),
field("created_at", Long::class.java)
)
.from(definitionsByType[objectType]!!.tableName)
.where(
field("id", String::class.java).`in`(keys).and(
DSL.field("is_deleted", Boolean::class.java).eq(false)
)
)
.fetch()
.getValues(field("body", String::class.java))
}
}

bodies.map {
objectMapper.readValue(it, objectType.clazz as Class<T>)
records.map {
objectMapper.readValue(
it.getValue(field("body", String::class.java)),
objectType.clazz as Class<T>
).apply {
this.createdAt = it.getValue(field("created_at", Long::class.java))
}
}
}
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import org.jooq.impl.DSL
import strikt.api.expectThat
import strikt.api.expectThrows
import strikt.assertions.isEqualTo
import strikt.assertions.isNotEmpty
import strikt.assertions.isNotNull

internal object SqlStorageServiceTests : JUnit5Minutests {

Expand Down Expand Up @@ -74,6 +76,7 @@ internal object SqlStorageServiceTests : JUnit5Minutests {

var application = sqlStorageService.loadObject<Application>(ObjectType.APPLICATION, "application001")
expectThat(application.description).isEqualTo("my first application!")
expectThat(application.createdAt).isNotNull()

// verify that an application can be updated
sqlStorageService.storeObject(
Expand Down Expand Up @@ -114,6 +117,15 @@ internal object SqlStorageServiceTests : JUnit5Minutests {

application = sqlStorageService.loadObject(ObjectType.APPLICATION, "application001")
expectThat(application.description).isEqualTo("my updated application!")

val allApplications = sqlStorageService.loadObjects<Application>(
ObjectType.APPLICATION,
sqlStorageService.listObjectKeys(ObjectType.APPLICATION).keys.toList()
)
expectThat(allApplications).isNotEmpty()
allApplications.forEach {
expectThat(it.createdAt).isNotNull()
}
}
}

Expand Down

0 comments on commit 9fd2fc7

Please sign in to comment.