opendev/gerrit
Code Issues Proposed changes

Fix JSON representation of Comment to indicate UTC

Browse Source 
The default behavior of Gson[1] discards timezone when writing out
Timestamps, producing a string like "Jun 22, 2015 10:11:00 AM" in the
writtenOn field of Comment. This has at least two major problems:

* It uses the local timezone of the server at comment creation time, so
  unless we are absolutely sure of the server's timezone at that time,
  we don't know the absolute instant it's referring to.
* The representation may be ambiguous, because during the 25-hour
  transition away from DST, there are 2 of some times.

Work around this situation by using a new TypeAdapter that stores
timestamps as ISO 8601 instants in UTC time, like
"2015-06-22T17:11:00Z". UTC by definition does not undergo DST, so this
representation unambiguously refers to a single instant.

This format is carefully chosen as one that is known[2] to be readable
by the existing default Gson type adapter. This allows us to start
writing the new format in a multi-master setup in a single update, since
the code to read the new format is already present.

There may be reasons that this new format is not completely ideal; for
example, it still does not record the server's (or perhaps the end
user's) timezone at the time the comment was created. It is explicitly
chosen for ease of use with our specific Gson implementation (though
it is an ISO format, so it's not completely unreasonable). Then again,
the old format was "chosen" for ease of use with Gson, so the new format
is no worse in that regard.

Write some careful tests based on some real-world data of a comment in
ReviewDb on gerrit-review that was added during the US/Pacific DST
transition in 2013[3]. The tests refer to the specific raw value in ms
that I pulled from ReviewDb, and the specific JSON serialization of that
value stored in NoteDb.

[1] http://static.javadoc.io/com.google.code.gson/gson/2.8.0/com/google/gson/GsonBuilder.html
[2] https://github.com/google/gson/issues/650#issuecomment-114298460
[3] https://gerrit-review.googlesource.com/51341

Bug: Issue 5489
Change-Id: I62eb01ac33f609604b2cbd28ea194807559854ce
This commit is contained in:
Dave Borowitz
2017-02-07 21:47:19 -05:00
parent 47a40f17d3
commit 13109182d8
3 changed files with 231 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

159
gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommentTimestampAdapterTest.java Normal file
View File

@@ -0,0 +1,159 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.notedb;
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Comment;
import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
import java.sql.Timestamp;
import java.time.ZonedDateTime;
import java.util.TimeZone;
import java.util.concurrent.TimeUnit;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
public class CommentTimestampAdapterTest {
/** Arbitrary time outside of a DST transition, as an ISO instant. */
private static final String NON_DST_STR = "2017-02-07T10:20:30.123Z";
/** Arbitrary time outside of a DST transition, as a reasonable Java 8 representation. */
private static final ZonedDateTime NON_DST = ZonedDateTime.parse(NON_DST_STR);
/** Arbitrary time outside of a DST transition, as an unreasonable Timestamp representation. */
private static final Timestamp NON_DST_TS = Timestamp.from(NON_DST.toInstant());
/**
* Real live ms since epoch timestamp of a comment that was posted during the PDT to PST
* transition in November 2013.
*/
private static final long MID_DST_MS = 1383466224175L;
/**
* Ambiguous string representation of {@link #MID_DST_MS} that was actually stored in NoteDb for
* this comment.
*/
private static final String MID_DST_STR = "Nov 3, 2013 1:10:24 AM";
private TimeZone systemTimeZone;
private Gson legacyGson;
private Gson gson;
@Before
public void setUp() {
systemTimeZone = TimeZone.getDefault();
TimeZone.setDefault(TimeZone.getTimeZone("America/Los_Angeles"));
// Match ChangeNoteUtil#gson as of 4e1f02db913d91f2988f559048e513e6093a1bce
legacyGson = new GsonBuilder().setPrettyPrinting().create();
gson = ChangeNoteUtil.newGson();
}
@After
public void tearDown() {
TimeZone.setDefault(systemTimeZone);
}
@Test
public void legacyGsonBehavesAsExpectedDuringDstTransition() {
long oneHourMs = TimeUnit.HOURS.toMillis(1);
String beforeJson = "\"Nov 3, 2013 12:10:24 AM\"";
Timestamp beforeTs = new Timestamp(MID_DST_MS - oneHourMs);
assertThat(legacyGson.toJson(beforeTs)).isEqualTo(beforeJson);
String ambiguousJson = '"' + MID_DST_STR + '"';
Timestamp duringTs = new Timestamp(MID_DST_MS);
assertThat(legacyGson.toJson(duringTs)).isEqualTo(ambiguousJson);
Timestamp afterTs = new Timestamp(MID_DST_MS + oneHourMs);
assertThat(legacyGson.toJson(afterTs)).isEqualTo(ambiguousJson);
Timestamp beforeTsTruncated = new Timestamp(beforeTs.getTime() / 1000 * 1000);
assertThat(legacyGson.fromJson(beforeJson, Timestamp.class)).isEqualTo(beforeTsTruncated);
// Gson just picks one, and it happens to be the one after the PST transition.
Timestamp afterTsTruncated = new Timestamp(afterTs.getTime() / 1000 * 1000);
assertThat(legacyGson.fromJson(ambiguousJson, Timestamp.class)).isEqualTo(afterTsTruncated);
}
@Test
public void legacyAdapterViaZonedDateTime() {
assertThat(legacyGson.toJson(NON_DST_TS)).isEqualTo("\"Feb 7, 2017 2:20:30 AM\"");
}
@Test
public void legacyAdapterCanParseOutputOfNewAdapter() {
String instantJson = gson.toJson(NON_DST_TS);
assertThat(instantJson).isEqualTo('"' + NON_DST_STR + '"');
assertThat(legacyGson.fromJson(instantJson, Timestamp.class)).isEqualTo(NON_DST_TS);
}
@Test
public void newAdapterCanParseOutputOfLegacyAdapter() {
String legacyJson = legacyGson.toJson(NON_DST_TS);
assertThat(legacyJson).isEqualTo("\"Feb 7, 2017 2:20:30 AM\"");
assertThat(gson.fromJson(legacyJson, Timestamp.class))
.isEqualTo(new Timestamp(NON_DST_TS.getTime() / 1000 * 1000));
}
@Test
public void newAdapterDisagreesWithLegacyAdapterDuringDstTransition() {
String duringJson = legacyGson.toJson(new Timestamp(MID_DST_MS));
Timestamp duringTs = legacyGson.fromJson(duringJson, Timestamp.class);
// This is unfortunate, but it's just documenting the current behavior, there is no real good
// solution here. The goal is that all these changes will be rebuilt with proper UTC instant
// strings shortly after the new adapter is live.
Timestamp newDuringTs = gson.fromJson(duringJson, Timestamp.class);
assertThat(newDuringTs.toString()).isEqualTo(duringTs.toString());
assertThat(newDuringTs).isNotEqualTo(duringTs);
}
@Test
public void newAdapterRoundTrip() {
String json = gson.toJson(NON_DST_TS);
assertThat(json).isEqualTo('"' + NON_DST_STR + '"');
assertThat(gson.fromJson(json, Timestamp.class)).isEqualTo(NON_DST_TS);
}
@Test
public void nullSafety() {
assertThat(gson.toJson(null, Timestamp.class)).isEqualTo("null");
assertThat(gson.fromJson("null", Timestamp.class)).isNull();
}
@Test
public void newAdapterRoundTripOfWholeComment() {
Comment c =
new Comment(
new Comment.Key("uuid", "filename", 1),
new Account.Id(100),
NON_DST_TS,
(short) 0,
"message",
"serverId",
false);
c.lineNbr = 1;
c.revId = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
String json = gson.toJson(c);
assertThat(json).contains("\"writtenOn\": \"" + NON_DST_STR + "\",");
assertThat(gson.fromJson(json, Comment.class)).isEqualTo(c);
}
}