diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/230_composite.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/230_composite.yml index 0d899686c29d7..f5baf64b45dc0 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/230_composite.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/230_composite.yml @@ -511,6 +511,61 @@ setup: - match: { aggregations.test.buckets.0.doc_count: 1 } --- +"Composite aggregation with invalid format": + - skip: + version: " - 7.99.99" + reason: After key parse checking added in 7.14 (backport pending) + + - do: + catch: /created output it couldn't parse/ + search: + rest_total_hits_as_int: true + index: test + body: + aggregations: + test: + composite: + sources: [ + { + "date": { + "date_histogram": { + "field": "date", + "calendar_interval": "1d", + # mixes week based and month based + "format": "YYYY-MM-dd" + } + } + } + ] + +--- +"Composite aggregation with lossy format": + - skip: + version: " - 7.99.99" + reason: After key parse checking added in 7.14 (backport pending) + + - do: + catch: /created output it couldn't parse/ + search: + rest_total_hits_as_int: true + index: test + body: + aggregations: + test: + composite: + sources: [ + { + "date": { + "date_histogram": { + "field": "date", + "calendar_interval": "1d", + # format is lower resolution than buckets, after key will lose data + "format": "yyyy-MM" + } + } + } + ] +--- "Composite aggregation with date_histogram offset": - skip: version: " - 7.5.99" diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileValuesSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileValuesSource.java index c9306ec4e010d..11f397f03f718 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileValuesSource.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileValuesSource.java @@ -10,11 +10,10 @@ import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.index.SortedNumericDocValues; -import org.elasticsearch.core.CheckedFunction; import org.elasticsearch.common.util.BigArrays; +import org.elasticsearch.core.CheckedFunction; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.search.DocValueFormat; -import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils; import java.io.IOException; import java.util.function.LongUnaryOperator; @@ -44,7 +43,11 @@ void setAfter(Comparable value) { } else if (value instanceof Number) { afterValue = ((Number) value).longValue(); } else { - afterValue = GeoTileUtils.longEncode(value.toString()); + afterValue = format.parseLong( + value.toString(), + false, + () -> { throw new IllegalArgumentException("now() is not supported in [after] key"); } + ); } } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java index 558db937a29ba..87fcf1b19bdcb 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java @@ -405,34 +405,58 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws * Format obj using the provided {@link DocValueFormat}. * If the format is equals to {@link DocValueFormat#RAW}, the object is returned as is * for numbers and a string for {@link BytesRef}s. + * + * This method will then attempt to parse the formatted value using the specified format, + * and throw an IllegalArgumentException if parsing fails. This in turn prevents us from + * returning an after_key which we can't subsequently parse into the original value. */ static Object formatObject(Object obj, DocValueFormat format) { if (obj == null) { return null; } + Object formatted = obj; + Object parsed; if (obj.getClass() == BytesRef.class) { BytesRef value = (BytesRef) obj; if (format == DocValueFormat.RAW) { - return value.utf8ToString(); + formatted = value.utf8ToString(); } else { - return format.format(value); + formatted = format.format(value); + } + parsed = format.parseBytesRef(formatted.toString()); + if (parsed.equals(obj) == false) { + throw new IllegalArgumentException("Format [" + format + "] created output it couldn't parse for value [" + obj +"] " + + "of type [" + obj.getClass() + "]. parsed value: [" + parsed + "(" + parsed.getClass() + ")]"); } } else if (obj.getClass() == Long.class) { long value = (long) obj; if (format == DocValueFormat.RAW) { - return value; + formatted = value; } else { - return format.format(value); + formatted = format.format(value); + } + parsed = format.parseLong(formatted.toString(), false, () -> { + throw new UnsupportedOperationException("Using now() is not supported in after keys"); + }); + if (parsed.equals(((Number) obj).longValue()) == false) { + throw new IllegalArgumentException("Format [" + format + "] created output it couldn't parse for value [" + obj +"] " + + "of type [" + obj.getClass() + "]. parsed value: [" + parsed + "(" + parsed.getClass() + ")]"); } } else if (obj.getClass() == Double.class) { double value = (double) obj; if (format == DocValueFormat.RAW) { - return value; + formatted = value; } else { - return format.format(value); + formatted = format.format(value); + } + parsed = format.parseDouble(formatted.toString(), false, + () -> {throw new UnsupportedOperationException("Using now() is not supported in after keys");}); + if (parsed.equals(((Number) obj).doubleValue()) == false) { + throw new IllegalArgumentException("Format [" + format + "] created output it couldn't parse for value [" + obj +"] " + + "of type [" + obj.getClass() + "]. parsed value: [" + parsed + "(" + parsed.getClass() + ")]"); } } - return obj; + return formatted; } static class ArrayMap extends AbstractMap implements Comparable { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/InternalCompositeTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/InternalCompositeTests.java index cd47e8ae0761e..1588414067ff8 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/InternalCompositeTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/InternalCompositeTests.java @@ -377,6 +377,38 @@ public void testCompareCompositeKeyValuesHaveDifferentTypes() { containsString("java.lang.String cannot be cast to")); } + public void testFormatObjectChecked() { + DocValueFormat weekYearMonth = new DocValueFormat.DateTime( + // YYYY is week-based year. The parser will ignore MM (month-of-year) and dd (day-of-month) + DateFormatter.forPattern("YYYY-MM-dd"), + ZoneOffset.UTC, + DateFieldMapper.Resolution.MILLISECONDS + ); + long epoch = 1622060077L; // May 25th 2021, evening + expectThrows(IllegalArgumentException.class, () -> InternalComposite.formatObject(epoch, weekYearMonth)); + } + + public void testFormatDateEpochTimezone() { + DocValueFormat epochSecond = new DocValueFormat.DateTime( + // YYYY is week-based year. The parser will ignore MM (month-of-year) and dd (day-of-month) + DateFormatter.forPattern("epoch_second"), + ZoneOffset.ofHours(-2), + DateFieldMapper.Resolution.MILLISECONDS + ); + DocValueFormat epochMillis = new DocValueFormat.DateTime( + // YYYY is week-based year. The parser will ignore MM (month-of-year) and dd (day-of-month) + DateFormatter.forPattern("epoch_millis"), + ZoneOffset.ofHours(-2), + DateFieldMapper.Resolution.MILLISECONDS + ); + long epoch = 1622060077L; // May 25th 2021, evening + Object actual = InternalComposite.formatObject(epoch, epochSecond); + assertEquals("1622060.077", actual.toString()); + + actual = InternalComposite.formatObject(epoch, epochMillis); + assertEquals("1622060077", actual.toString()); + } + private InternalComposite.ArrayMap createMap(List fields, Comparable[] values) { List formats = IntStream.range(0, fields.size()) .mapToObj(i -> DocValueFormat.RAW).collect(Collectors.toList());