Skip to content

Commit 98209e4

Browse files
DaveCTurnerDaveCTurner
authored
Simplify XContent output of epoch times (elastic#114491) (elastic#114736)
Today the overloads of `XContentBuilder#timeField` do two rather different things: one formats an object as a `String` representation of a time (where the object is either an unambiguous time object or else a `long`) and the other formats only a `long` as one or two fields depending on the `?human` flag. This is trappy in a number of ways: - `long` means an absolute (epoch) time, but sometimes folks will mistakenly use this for time intervals too. - `long` means only milliseconds, there is no facility to specify a different unit. - the dependence on the `?human` flag in exactly one of the overloads is kinda weird. This commit removes the confusion by dropping support for considering a `Long` as a valid representation of a time at all, and instead requiring callers to either convert it into a proper time object or else call a method that is explicitly expecting an epoch time in milliseconds.
1 parent d82daf4 commit 98209e4

File tree

77 files changed

+398
-220
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

77 files changed

+398
-220
lines changed

‎libs/x-content/src/main/java/org/elasticsearch/xcontent/XContentBuilder.java‎

Lines changed: 39 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import java.util.ServiceLoader;
4141
import java.util.Set;
4242
import java.util.function.Function;
43+
import java.util.function.LongFunction;
4344

4445
/**
4546
* A utility to build XContent (ie json).
@@ -107,13 +108,15 @@ public static XContentBuilder builder(XContentType xContentType, Set<String> inc
107108
private static final Map<Class<?>, Writer> WRITERS;
108109
private static final Map<Class<?>, HumanReadableTransformer> HUMAN_READABLE_TRANSFORMERS;
109110
private static final Map<Class<?>, Function<Object, Object>> DATE_TRANSFORMERS;
111+
private static final LongFunction<String> UNIX_EPOCH_MILLIS_FORMATTER;
112+
110113
static {
111114
Map<Class<?>, Writer> writers = new HashMap<>();
112115
writers.put(Boolean.class, (b, v) -> b.value((Boolean) v));
113116
writers.put(boolean[].class, (b, v) -> b.values((boolean[]) v));
114117
writers.put(Byte.class, (b, v) -> b.value((Byte) v));
115118
writers.put(byte[].class, (b, v) -> b.value((byte[]) v));
116-
writers.put(Date.class, XContentBuilder::timeValue);
119+
writers.put(Date.class, XContentBuilder::timestampValue);
117120
writers.put(Double.class, (b, v) -> b.value((Double) v));
118121
writers.put(double[].class, (b, v) -> b.values((double[]) v));
119122
writers.put(Float.class, (b, v) -> b.value((Float) v));
@@ -129,8 +132,8 @@ public static XContentBuilder builder(XContentType xContentType, Set<String> inc
129132
writers.put(Locale.class, (b, v) -> b.value(v.toString()));
130133
writers.put(Class.class, (b, v) -> b.value(v.toString()));
131134
writers.put(ZonedDateTime.class, (b, v) -> b.value(v.toString()));
132-
writers.put(Calendar.class, XContentBuilder::timeValue);
133-
writers.put(GregorianCalendar.class, XContentBuilder::timeValue);
135+
writers.put(Calendar.class, XContentBuilder::timestampValue);
136+
writers.put(GregorianCalendar.class, XContentBuilder::timestampValue);
134137
writers.put(BigInteger.class, (b, v) -> b.value((BigInteger) v));
135138
writers.put(BigDecimal.class, (b, v) -> b.value((BigDecimal) v));
136139

@@ -140,6 +143,8 @@ public static XContentBuilder builder(XContentType xContentType, Set<String> inc
140143
// treat strings as already converted
141144
dateTransformers.put(String.class, Function.identity());
142145

146+
LongFunction<String> unixEpochMillisFormatter = Long::toString;
147+
143148
// Load pluggable extensions
144149
for (XContentBuilderExtension service : ServiceLoader.load(XContentBuilderExtension.class)) {
145150
Map<Class<?>, Writer> addlWriters = service.getXContentWriters();
@@ -157,11 +162,14 @@ public static XContentBuilder builder(XContentType xContentType, Set<String> inc
157162
writers.putAll(addlWriters);
158163
humanReadableTransformer.putAll(addlTransformers);
159164
dateTransformers.putAll(addlDateTransformers);
165+
166+
unixEpochMillisFormatter = service::formatUnixEpochMillis;
160167
}
161168

162169
WRITERS = Map.copyOf(writers);
163170
HUMAN_READABLE_TRANSFORMERS = Map.copyOf(humanReadableTransformer);
164171
DATE_TRANSFORMERS = Map.copyOf(dateTransformers);
172+
UNIX_EPOCH_MILLIS_FORMATTER = unixEpochMillisFormatter;
165173
}
166174

167175
@FunctionalInterface
@@ -797,52 +805,53 @@ public XContentBuilder utf8Value(byte[] bytes, int offset, int length) throws IO
797805
}
798806

799807
////////////////////////////////////////////////////////////////////////////
800-
// Date
808+
// Timestamps
801809
//////////////////////////////////
802810

803811
/**
804-
* Write a time-based field and value, if the passed timeValue is null a
805-
* null value is written, otherwise a date transformers lookup is performed.
806-
807-
* @throws IllegalArgumentException if there is no transformers for the type of object
812+
* Write a field with a timestamp value: if the passed timestamp is null then writes null, otherwise looks up the date transformer
813+
* for the type of {@code timestamp} and uses it to format the value.
814+
*
815+
* @throws IllegalArgumentException if there is no transformer for the given value type
808816
*/
809-
public XContentBuilder timeField(String name, Object timeValue) throws IOException {
810-
return field(name).timeValue(timeValue);
817+
public XContentBuilder timestampField(String name, Object timestamp) throws IOException {
818+
return field(name).timestampValue(timestamp);
811819
}
812820

813821
/**
814-
* If the {@code humanReadable} flag is set, writes both a formatted and
815-
* unformatted version of the time value using the date transformer for the
816-
* {@link Long} class.
822+
* Writes a field containing the raw number of milliseconds since the unix epoch, and also if the {@code humanReadable} flag is set,
823+
* writes a formatted representation of this value using the UNIX_EPOCH_MILLIS_FORMATTER.
817824
*/
818-
public XContentBuilder timeField(String name, String readableName, long value) throws IOException {
819-
assert name.equals(readableName) == false : "expected raw and readable field names to differ, but they were both: " + name;
825+
public XContentBuilder timestampFieldsFromUnixEpochMillis(String rawFieldName, String humanReadableFieldName, long unixEpochMillis)
826+
throws IOException {
827+
assert rawFieldName.equals(humanReadableFieldName) == false
828+
: "expected raw and readable field names to differ, but they were both: " + rawFieldName;
820829
if (humanReadable) {
821-
Function<Object, Object> longTransformer = DATE_TRANSFORMERS.get(Long.class);
822-
if (longTransformer == null) {
823-
throw new IllegalArgumentException("cannot write time value xcontent for unknown value of type Long");
824-
}
825-
field(readableName).value(longTransformer.apply(value));
830+
field(humanReadableFieldName, UNIX_EPOCH_MILLIS_FORMATTER.apply(unixEpochMillis));
826831
}
827-
field(name, value);
832+
field(rawFieldName, unixEpochMillis);
828833
return this;
829834
}
830835

831836
/**
832-
* Write a time-based value, if the value is null a null value is written,
833-
* otherwise a date transformers lookup is performed.
834-
835-
* @throws IllegalArgumentException if there is no transformers for the type of object
837+
* Write a timestamp value: if the passed timestamp is null then writes null, otherwise looks up the date transformer for the type of
838+
* {@code timestamp} and uses it to format the value.
839+
*
840+
* @throws IllegalArgumentException if there is no transformer for the given value type
836841
*/
837-
public XContentBuilder timeValue(Object timeValue) throws IOException {
838-
if (timeValue == null) {
842+
public XContentBuilder timestampValue(Object timestamp) throws IOException {
843+
if (timestamp == null) {
839844
return nullValue();
840845
} else {
841-
Function<Object, Object> transformer = DATE_TRANSFORMERS.get(timeValue.getClass());
846+
Function<Object, Object> transformer = DATE_TRANSFORMERS.get(timestamp.getClass());
842847
if (transformer == null) {
843-
throw new IllegalArgumentException("cannot write time value xcontent for unknown value of type " + timeValue.getClass());
848+
final var exception = new IllegalArgumentException(
849+
"cannot write timestamp value xcontent for value of unknown type " + timestamp.getClass()
850+
);
851+
assert false : exception;
852+
throw exception;
844853
}
845-
return value(transformer.apply(timeValue));
854+
return value(transformer.apply(timestamp));
846855
}
847856
}
848857

‎libs/x-content/src/main/java/org/elasticsearch/xcontent/XContentBuilderExtension.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,4 +68,9 @@ public interface XContentBuilderExtension {
6868
* </pre>
6969
*/
7070
Map<Class<?>, Function<Object, Object>> getDateTransformers();
71+
72+
/**
73+
* Used to format a {@code long} representing the number of milliseconds since the Unix Epoch.
74+
*/
75+
String formatUnixEpochMillis(long unixEpochMillis);
7176
}

‎modules/aggregations/src/internalClusterTest/java/org/elasticsearch/aggregations/pipeline/DateDerivativeIT.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,17 +65,17 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
6565
}
6666

6767
private static IndexRequestBuilder indexDoc(String idx, ZonedDateTime date, int value) throws Exception {
68-
return prepareIndex(idx).setSource(jsonBuilder().startObject().timeField("date", date).field("value", value).endObject());
68+
return prepareIndex(idx).setSource(jsonBuilder().startObject().timestampField("date", date).field("value", value).endObject());
6969
}
7070

7171
private IndexRequestBuilder indexDoc(int month, int day, int value) throws Exception {
7272
return prepareIndex("idx").setSource(
7373
jsonBuilder().startObject()
7474
.field("value", value)
75-
.timeField("date", date(month, day))
75+
.timestampField("date", date(month, day))
7676
.startArray("dates")
77-
.timeValue(date(month, day))
78-
.timeValue(date(month + 1, day + 1))
77+
.timestampValue(date(month, day))
78+
.timestampValue(date(month + 1, day + 1))
7979
.endArray()
8080
.endObject()
8181
);

‎modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/direct/DatabaseConfigurationMetadata.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
6666
// (we'll be a in a json map where the id is the key)
6767
builder.startObject();
6868
builder.field(VERSION.getPreferredName(), version);
69-
builder.timeField(MODIFIED_DATE_MILLIS.getPreferredName(), MODIFIED_DATE.getPreferredName(), modifiedDate);
69+
builder.timestampFieldsFromUnixEpochMillis(MODIFIED_DATE_MILLIS.getPreferredName(), MODIFIED_DATE.getPreferredName(), modifiedDate);
7070
builder.field(DATABASE.getPreferredName(), database);
7171
builder.endObject();
7272
return builder;

‎modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/direct/GetDatabaseConfigurationAction.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,11 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
110110
builder.startObject();
111111
builder.field("id", database.id()); // serialize including the id -- this is get response serialization
112112
builder.field(VERSION.getPreferredName(), item.version());
113-
builder.timeField(MODIFIED_DATE_MILLIS.getPreferredName(), MODIFIED_DATE.getPreferredName(), item.modifiedDate());
113+
builder.timestampFieldsFromUnixEpochMillis(
114+
MODIFIED_DATE_MILLIS.getPreferredName(),
115+
MODIFIED_DATE.getPreferredName(),
116+
item.modifiedDate()
117+
);
114118
builder.field(DATABASE.getPreferredName(), database);
115119
builder.endObject();
116120
}

‎server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/bucket/DateHistogramIT.java‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -88,11 +88,11 @@ private static String format(ZonedDateTime date, String pattern) {
8888
private IndexRequestBuilder indexDoc(String idx, ZonedDateTime date, int value) throws Exception {
8989
return prepareIndex(idx).setSource(
9090
jsonBuilder().startObject()
91-
.timeField("date", date)
91+
.timestampField("date", date)
9292
.field("value", value)
9393
.startArray("dates")
94-
.timeValue(date)
95-
.timeValue(date.plusMonths(1).plusDays(1))
94+
.timestampValue(date)
95+
.timestampValue(date.plusMonths(1).plusDays(1))
9696
.endArray()
9797
.endObject()
9898
);
@@ -103,10 +103,10 @@ private IndexRequestBuilder indexDoc(int month, int day, int value) throws Excep
103103
jsonBuilder().startObject()
104104
.field("value", value)
105105
.field("constant", 1)
106-
.timeField("date", date(month, day))
106+
.timestampField("date", date(month, day))
107107
.startArray("dates")
108-
.timeValue(date(month, day))
109-
.timeValue(date(month + 1, day + 1))
108+
.timestampValue(date(month, day))
109+
.timestampValue(date(month + 1, day + 1))
110110
.endArray()
111111
.endObject()
112112
);
@@ -162,53 +162,53 @@ private void getMultiSortDocs(List<IndexRequestBuilder> builders) throws IOExcep
162162
for (int i = 1; i <= 3; i++) {
163163
builders.add(
164164
prepareIndex("sort_idx").setSource(
165-
jsonBuilder().startObject().timeField("date", date(1, 1)).field("l", 1).field("d", i).endObject()
165+
jsonBuilder().startObject().timestampField("date", date(1, 1)).field("l", 1).field("d", i).endObject()
166166
)
167167
);
168168
builders.add(
169169
prepareIndex("sort_idx").setSource(
170-
jsonBuilder().startObject().timeField("date", date(1, 2)).field("l", 2).field("d", i).endObject()
170+
jsonBuilder().startObject().timestampField("date", date(1, 2)).field("l", 2).field("d", i).endObject()
171171
)
172172
);
173173
}
174174
builders.add(
175175
prepareIndex("sort_idx").setSource(
176-
jsonBuilder().startObject().timeField("date", date(1, 3)).field("l", 3).field("d", 1).endObject()
176+
jsonBuilder().startObject().timestampField("date", date(1, 3)).field("l", 3).field("d", 1).endObject()
177177
)
178178
);
179179
builders.add(
180180
prepareIndex("sort_idx").setSource(
181-
jsonBuilder().startObject().timeField("date", date(1, 3).plusHours(1)).field("l", 3).field("d", 2).endObject()
181+
jsonBuilder().startObject().timestampField("date", date(1, 3).plusHours(1)).field("l", 3).field("d", 2).endObject()
182182
)
183183
);
184184
builders.add(
185185
prepareIndex("sort_idx").setSource(
186-
jsonBuilder().startObject().timeField("date", date(1, 4)).field("l", 3).field("d", 1).endObject()
186+
jsonBuilder().startObject().timestampField("date", date(1, 4)).field("l", 3).field("d", 1).endObject()
187187
)
188188
);
189189
builders.add(
190190
prepareIndex("sort_idx").setSource(
191-
jsonBuilder().startObject().timeField("date", date(1, 4).plusHours(2)).field("l", 3).field("d", 3).endObject()
191+
jsonBuilder().startObject().timestampField("date", date(1, 4).plusHours(2)).field("l", 3).field("d", 3).endObject()
192192
)
193193
);
194194
builders.add(
195195
prepareIndex("sort_idx").setSource(
196-
jsonBuilder().startObject().timeField("date", date(1, 5)).field("l", 5).field("d", 1).endObject()
196+
jsonBuilder().startObject().timestampField("date", date(1, 5)).field("l", 5).field("d", 1).endObject()
197197
)
198198
);
199199
builders.add(
200200
prepareIndex("sort_idx").setSource(
201-
jsonBuilder().startObject().timeField("date", date(1, 5).plusHours(12)).field("l", 5).field("d", 2).endObject()
201+
jsonBuilder().startObject().timestampField("date", date(1, 5).plusHours(12)).field("l", 5).field("d", 2).endObject()
202202
)
203203
);
204204
builders.add(
205205
prepareIndex("sort_idx").setSource(
206-
jsonBuilder().startObject().timeField("date", date(1, 6)).field("l", 5).field("d", 1).endObject()
206+
jsonBuilder().startObject().timestampField("date", date(1, 6)).field("l", 5).field("d", 1).endObject()
207207
)
208208
);
209209
builders.add(
210210
prepareIndex("sort_idx").setSource(
211-
jsonBuilder().startObject().timeField("date", date(1, 7)).field("l", 5).field("d", 1).endObject()
211+
jsonBuilder().startObject().timestampField("date", date(1, 7)).field("l", 5).field("d", 1).endObject()
212212
)
213213
);
214214
}
@@ -997,7 +997,7 @@ public void testSingleValueWithTimeZone() throws Exception {
997997
IndexRequestBuilder[] reqs = new IndexRequestBuilder[5];
998998
ZonedDateTime date = date("2014-03-11T00:00:00+00:00");
999999
for (int i = 0; i < reqs.length; i++) {
1000-
reqs[i] = prepareIndex("idx2").setId("" + i).setSource(jsonBuilder().startObject().timeField("date", date).endObject());
1000+
reqs[i] = prepareIndex("idx2").setId("" + i).setSource(jsonBuilder().startObject().timestampField("date", date).endObject());
10011001
date = date.plusHours(1);
10021002
}
10031003
indexRandom(true, reqs);

‎server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/bucket/DateHistogramOffsetIT.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ private void prepareIndex(ZonedDateTime date, int numHours, int stepSizeHours, i
6363
IndexRequestBuilder[] reqs = new IndexRequestBuilder[numHours];
6464
for (int i = idxIdStart; i < idxIdStart + reqs.length; i++) {
6565
reqs[i - idxIdStart] = prepareIndex("idx2").setId("" + i)
66-
.setSource(jsonBuilder().startObject().timeField("date", date).endObject());
66+
.setSource(jsonBuilder().startObject().timestampField("date", date).endObject());
6767
date = date.plusHours(stepSizeHours);
6868
}
6969
indexRandom(true, reqs);

‎server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,10 @@ private static IndexRequestBuilder indexDoc(int month, int day, int value) throw
5858
return prepareIndex("idx").setSource(
5959
jsonBuilder().startObject()
6060
.field("value", value)
61-
.timeField("date", date(month, day))
61+
.timestampField("date", date(month, day))
6262
.startArray("dates")
63-
.timeValue(date(month, day))
64-
.timeValue(date(month + 1, day + 1))
63+
.timestampValue(date(month, day))
64+
.timestampValue(date(month + 1, day + 1))
6565
.endArray()
6666
.endObject()
6767
);
@@ -620,8 +620,8 @@ public void testScriptCaching() throws Exception {
620620
);
621621
indexRandom(
622622
true,
623-
prepareIndex("cache_test_idx").setId("1").setSource(jsonBuilder().startObject().timeField("date", date(1, 1)).endObject()),
624-
prepareIndex("cache_test_idx").setId("2").setSource(jsonBuilder().startObject().timeField("date", date(2, 1)).endObject())
623+
prepareIndex("cache_test_idx").setId("1").setSource(jsonBuilder().startObject().timestampField("date", date(1, 1)).endObject()),
624+
prepareIndex("cache_test_idx").setId("2").setSource(jsonBuilder().startObject().timestampField("date", date(2, 1)).endObject())
625625
);
626626

627627
// Make sure we are starting with a clear cache

‎server/src/main/java/org/elasticsearch/action/admin/indices/dangling/list/ListDanglingIndicesResponse.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
7979

8080
builder.field("index_name", info.indexName);
8181
builder.field("index_uuid", info.indexUUID);
82-
builder.timeField("creation_date_millis", "creation_date", info.creationDateMillis);
82+
builder.timestampFieldsFromUnixEpochMillis("creation_date_millis", "creation_date", info.creationDateMillis);
8383

8484
builder.array("node_ids", info.nodeIds.toArray(new String[0]));
8585

‎server/src/main/java/org/elasticsearch/action/admin/indices/stats/FieldUsageShardResponse.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ public FieldUsageStats getStats() {
6969
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
7070
builder.startObject();
7171
builder.field(Fields.TRACKING_ID, trackingId);
72-
builder.timeField(Fields.TRACKING_STARTED_AT_MILLIS, Fields.TRACKING_STARTED_AT, trackingStartTime);
72+
builder.timestampFieldsFromUnixEpochMillis(Fields.TRACKING_STARTED_AT_MILLIS, Fields.TRACKING_STARTED_AT, trackingStartTime);
7373
builder.startObject(Fields.ROUTING)
7474
.field(Fields.STATE, shardRouting.state())
7575
.field(Fields.PRIMARY, shardRouting.primary())

0 commit comments

Comments
 (0)