Use Instant instead of Timestamp to ensure we always use UTC
Internally Gerrit events use milliseconds since the Epoch for event
timestamps which are in UTC by definition. Reflect this throughout the
events-log plugin by switching from `java.sql.Timestamp` to
`java.time.Instant` to ensure it always represents timestamps in UTC.
On Spanner this didn't work as intended since it assumes timezone PST
if the timezone isn't given explicitly.
Change-Id: I9f0e46fc3c33bf5502e9bfbfa755cccbe32b96e9
diff --git a/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLClient.java b/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLClient.java
index d0a6af4..f422cbd 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLClient.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLClient.java
@@ -22,7 +22,6 @@
import static java.lang.String.format;
import static java.util.concurrent.TimeUnit.DAYS;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
-import static java.util.concurrent.TimeUnit.SECONDS;
import com.ericsson.gerrit.plugins.eventslog.EventsLogException;
import com.ericsson.gerrit.plugins.eventslog.MalformedQueryException;
@@ -120,7 +119,7 @@
void storeEvent(ProjectEvent event) throws SQLException {
storeEvent(
event.getProjectNameKey().get(),
- new Timestamp(SECONDS.toMillis(event.eventCreatedOn)),
+ Instant.ofEpochSecond(event.eventCreatedOn),
gson.toJson(event));
}
@@ -128,23 +127,12 @@
* Store the event in the database.
*
* @param projectName The project in which this event happened
- * @param timestamp The time at which this event took place
+ * @param timestamp The instant at which this event took place
* @param event The event as a string
* @throws SQLException If there was a problem with the database
*/
- void storeEvent(String projectName, Timestamp timestamp, String event) throws SQLException {
- String values;
- switch (databaseDialect) {
- case SPANNER:
- // Workaround since Spanner assumes timestamps without explicit time zone to be US/LA
- Instant instant = (timestamp != null) ? timestamp.toInstant() : null;
- values = format("VALUES('%s', '%s', '%s')", projectName, instant, event);
- break;
- default:
- values = format("VALUES('%s', '%s', '%s')", projectName, timestamp, event);
- break;
- }
-
+ void storeEvent(String projectName, Instant timestamp, String event) throws SQLException {
+ String values = format("VALUES('%s', '%s', '%s')", projectName, timestamp, event);
execute(
format("INSERT INTO %s(%s, %s, %s) ", TABLE_NAME, PROJECT_ENTRY, DATE_ENTRY, EVENT_ENTRY)
+ values);
@@ -209,7 +197,7 @@
entries.add(
new SQLEntry(
rs.getString(PROJECT_ENTRY),
- rs.getTimestamp(DATE_ENTRY),
+ rs.getTimestamp(DATE_ENTRY).toInstant(),
rs.getString(EVENT_ENTRY),
rs.getObject(PRIMARY_ENTRY)));
}
@@ -225,7 +213,7 @@
SQLEntry entry =
new SQLEntry(
rs.getString(PROJECT_ENTRY),
- rs.getTimestamp(DATE_ENTRY),
+ rs.getTimestamp(DATE_ENTRY).toInstant(),
rs.getString(EVENT_ENTRY),
rs.getObject(PRIMARY_ENTRY));
result.put(rs.getString(PROJECT_ENTRY), entry);
diff --git a/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLEntry.java b/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLEntry.java
index b853eef..99fc51a 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLEntry.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLEntry.java
@@ -14,16 +14,16 @@
package com.ericsson.gerrit.plugins.eventslog.sql;
-import java.sql.Timestamp;
+import java.time.Instant;
import java.util.Objects;
class SQLEntry implements Comparable<SQLEntry> {
private String name;
- private Timestamp timestamp;
+ private Instant timestamp;
private String event;
private Object id;
- SQLEntry(String name, Timestamp timestamp, String event, Object id) {
+ SQLEntry(String name, Instant timestamp, String event, Object id) {
this.name = name;
this.timestamp = timestamp;
this.event = event;
@@ -34,7 +34,7 @@
return name;
}
- public Timestamp getTimestamp() {
+ public Instant getTimestamp() {
return timestamp;
}
diff --git a/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLQueryMaker.java b/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLQueryMaker.java
index 09bba6d..49ffe62 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLQueryMaker.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLQueryMaker.java
@@ -34,16 +34,19 @@
private static final int TWO = 2;
private static final String TIME_ONE = "t1";
private static final String TIME_TWO = "t2";
+ private static final String UTC = "Z";
private static final DateTimeFormatter DATE_TIME_FORMAT =
DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
private static final DateTimeFormatter DATE_ONLY_FORMAT =
DateTimeFormatter.ofPattern("yyyy-MM-dd");
private final int returnLimit;
+ private final SQLDialect databaseDialect;
@Inject
SQLQueryMaker(EventsLogConfig config) {
this.returnLimit = config.getReturnLimit();
+ this.databaseDialect = SQLDialect.fromJdbcUrl(config.getStoreUrl());
}
@Override
@@ -58,9 +61,17 @@
} catch (DateTimeParseException e) {
throw new MalformedQueryException(e);
}
- return String.format(
- "SELECT * FROM %s WHERE %s BETWEEN '%s' and '%s' ORDER BY date_created LIMIT %d",
- TABLE_NAME, DATE_ENTRY, dates[0], dates[1], returnLimit);
+
+ switch (databaseDialect) {
+ case SPANNER:
+ return String.format(
+ "SELECT * FROM %s WHERE %s BETWEEN '%s%s' and '%s%s' ORDER BY date_created LIMIT %d",
+ TABLE_NAME, DATE_ENTRY, dates[0], UTC, dates[1], UTC, returnLimit);
+ default:
+ return String.format(
+ "SELECT * FROM %s WHERE %s BETWEEN '%s' and '%s' ORDER BY date_created LIMIT %d",
+ TABLE_NAME, DATE_ENTRY, dates[0], dates[1], returnLimit);
+ }
}
@Override
diff --git a/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/QueryMakerTest.java b/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/QueryMakerTest.java
index cd2a61d..a2ef2bc 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/QueryMakerTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/QueryMakerTest.java
@@ -33,6 +33,7 @@
private static final String T1 = "t1";
private static final String OLD_DATE = "2013-10-10 10:00:00";
private static final String NEW_DATE = "2014-10-10 10:00:00";
+ private static final String TEST_DATABASE = "cloudspanner";
private QueryMaker queryMaker;
private String defaultQuery;
@@ -44,6 +45,7 @@
@Before
public void setUp() throws Exception {
when(cfgMock.getReturnLimit()).thenReturn(10);
+ when(cfgMock.getStoreUrl()).thenReturn(TEST_DATABASE);
queryMaker = new SQLQueryMaker(cfgMock);
defaultQuery = queryMaker.getDefaultQuery();
}
@@ -61,13 +63,13 @@
@Test
public void dateOneOnly() throws Exception {
query = queryMaker.formQueryFromRequestParameters(ImmutableMap.of(T1, OLD_DATE));
- assertThat(query).contains(String.format("'%s' and ", OLD_DATE));
+ assertThat(query).contains(String.format("'%sZ' and ", OLD_DATE));
}
@Test
public void dateTwoOnly() throws Exception {
query = queryMaker.formQueryFromRequestParameters(ImmutableMap.of(T2, OLD_DATE));
- assertThat(query).contains(String.format("'%s' and ", OLD_DATE));
+ assertThat(query).contains(String.format("'%sZ' and ", OLD_DATE));
}
@Test(expected = MalformedQueryException.class)
@@ -78,10 +80,10 @@
@Test
public void dateOrdering() throws Exception {
query = queryMaker.formQueryFromRequestParameters(ImmutableMap.of(T1, OLD_DATE, T2, NEW_DATE));
- assertThat(query).contains(String.format("'%s' and '%s'", OLD_DATE, NEW_DATE));
+ assertThat(query).contains(String.format("'%sZ' and '%sZ'", OLD_DATE, NEW_DATE));
query = queryMaker.formQueryFromRequestParameters(ImmutableMap.of(T1, NEW_DATE, T2, OLD_DATE));
- assertThat(query).contains(String.format("'%s' and '%s'", OLD_DATE, NEW_DATE));
+ assertThat(query).contains(String.format("'%sZ' and '%sZ'", OLD_DATE, NEW_DATE));
}
@Test
diff --git a/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLEntryTest.java b/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLEntryTest.java
index 0c1064f..a904802 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLEntryTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLEntryTest.java
@@ -16,7 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
-import java.sql.Timestamp;
+import java.time.Instant;
import java.util.Calendar;
import org.junit.Before;
import org.junit.Test;
@@ -34,9 +34,9 @@
@Before
public void setUp() {
- Timestamp timestamp = new Timestamp(NOW);
- Timestamp past = new Timestamp(0);
- Timestamp future = new Timestamp(NOW + 60000);
+ Instant timestamp = Instant.ofEpochMilli(NOW);
+ Instant past = Instant.ofEpochMilli(0);
+ Instant future = Instant.ofEpochMilli(NOW + 60000);
entry1 = new SQLEntry("name1", future, "event1", Integer.MAX_VALUE);
entry2 = new SQLEntry("name2", past, "event2", Integer.MIN_VALUE);
entry3 = new SQLEntry("name3", timestamp, "event3", 0);
@@ -53,7 +53,7 @@
@Test
public void testGetTimestamp() throws Exception {
- assertThat(entry3.getTimestamp()).isEqualTo(new Timestamp(NOW));
+ assertThat(entry3.getTimestamp()).isEqualTo(Instant.ofEpochMilli(NOW));
}
@Test
diff --git a/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLStoreTest.java b/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLStoreTest.java
index 223f37c..963c0ae 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLStoreTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLStoreTest.java
@@ -41,7 +41,7 @@
import java.sql.DriverManager;
import java.sql.SQLException;
import java.sql.Statement;
-import java.sql.Timestamp;
+import java.time.Instant;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.ScheduledExecutorService;
@@ -54,6 +54,7 @@
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
@@ -118,6 +119,29 @@
}
@Test
+ public void storeThenCheckInstantStored() throws Exception {
+ eventsDb = mock(SQLClient.class);
+ config.setJdbcUrl(TEST_LOCAL_URL);
+ localEventsDb = new SQLClient(config);
+ localEventsDb.createDBIfNotCreated();
+ localEventsDb.storeEvent(mockEvent);
+ store =
+ new SQLStore(
+ cfgMock,
+ eventsDb,
+ localEventsDb,
+ poolMock,
+ permissionBackendMock,
+ logCleanerMock,
+ PLUGIN_NAME);
+
+ store.start();
+ ArgumentCaptor<Instant> captor = ArgumentCaptor.forClass(Instant.class);
+ verify(eventsDb).storeEvent(any(String.class), captor.capture(), any(String.class));
+ assertThat(captor.getValue()).isEqualTo(Instant.ofEpochSecond(mockEvent.eventCreatedOn));
+ }
+
+ @Test
public void storeThenQueryNotVisible() throws Exception {
when(permissionBackendMock.currentUser()).thenReturn(withUserMock);
when(withUserMock.project(any(Project.NameKey.class))).thenReturn(forProjectMock);
@@ -455,7 +479,7 @@
store.start();
verify(eventsDb).queryOne();
- verify(eventsDb).storeEvent(any(String.class), any(Timestamp.class), any(String.class));
+ verify(eventsDb).storeEvent(any(String.class), any(Instant.class), any(String.class));
List<SQLEntry> entries = localEventsDb.getAll();
assertThat(entries).isEmpty();
}
