Scripting: Rework joda time backcompat #33486

rjernst · 2018-09-07T00:42:20Z

This commit switches the joda time backcompat in scripting to use
augmentation over ZonedDateTime. The augmentation methods provide
compatibility with the missing methods between joda's DateTime and
java's ZonedDateTime. Due to getDayOfWeek returning an enum in the java
API, ZonedDateTime is wrapped so that the method can return int like the
joda time does. The java time api version is renamed to
getDayOfWeekEnum, which will be kept through 7.x for compatibility while
users switch back to getDayOfWeek once joda compatibility is removed.
Usage of the joda time methods trigger a deprecation warning.

This commit switches the joda time backcompat in scripting to use augmentation over ZonedDateTime. The augmentation methods provide compatibility with the missing methods between joda's DateTime and java's ZonedDateTime. Due to getDayOfWeek returning an enum in the java API, ZonedDateTime is wrapped so that the method can return int like the joda time does. The java time api version is renamed to getDayOfWeekEnum, which will be kept through 7.x for compatibility while users switch back to getDayOfWeek once joda compatibility is removed.

elasticmachine · 2018-09-07T00:42:21Z

Pinging @elastic/es-core-infra

nik9000

This makes me so happy!

I left a few smallish suggestions in line.

nik9000 · 2018-09-07T13:14:09Z

docs/reference/aggregations/bucket/datehistogram-aggregation.asciidoc

@@ -425,7 +425,7 @@ POST /sales/_search?size=0
 --------------------------------------------------
 // CONSOLE
 // TEST[setup:sales]
-// TEST[warning:The joda time api for doc values is deprecated. Use -Des.scripting.use_java_time=true to use the java time api for date field doc values]
+// TEST[warning:getDayOfWeek() is incompatible with the java time api. Use getDayOfWeekEnum().getValue()."]


Maybe "the return type of getDayOfWeek will change in 7.0. Call getDayOfWeekEnum().getValue() to be compatible with 7.0."

nik9000 · 2018-09-07T13:45:58Z

modules/lang-painless/src/main/resources/org/elasticsearch/painless/spi/joda.time.txt

-#
-
-#
-# Painless definition file. This defines the hierarchy of classes,


nik9000 · 2018-09-07T13:48:40Z

server/src/main/java/org/elasticsearch/script/JodaCompatibleZonedDateTime.java

+        new DeprecationLogger(ESLoggerFactory.getLogger(JodaCompatibleZonedDateTime.class));
+
+    private static void logDeprecated(String key, String message, Object... params) {
+        AccessController.doPrivileged((PrivilegedAction<Void>) () -> {


It is worth a comment about why this doesn't check special permission: namely that we expect this to be called by scripts which won't have it.

nik9000 · 2018-09-07T13:52:30Z

server/src/test/java/org/elasticsearch/script/JodaCompatibleZonedDateTimeTests.java

+        };
+        appender.start();
+        Loggers.addAppender(DEPRECATION_LOGGER, appender);
+        try {


Maybe a comment here like "now run the block with the same permissions as a script".

nik9000 · 2018-09-07T13:54:12Z

server/src/test/java/org/elasticsearch/script/JodaCompatibleZonedDateTimeTests.java

+    );
+
+    // Thursday, September 6, 2018 3:01:20.617 PM GMT-07:00 DST
+    private static JodaCompatibleZonedDateTime TIME =


I think it'd be nice if every test built a random time with the Java time and then the same time with Joda and then asserted that the methods return the same thing.

nik9000 · 2018-09-07T13:55:09Z

server/src/main/java/org/elasticsearch/script/JodaCompatibleZonedDateTime.java

+    }
+
+    private static void logDeprecatedMethod(String method) {
+        logDeprecated(method, "Use of joda time method [{}] is deprecated. Use the java time api instead.", method);


Could you point folks to the right java API to use?

spinscale

This is great work! I really like this solution!

I agree with randomizing the tests and comparing the output against joda and left two pretty minor comments.

spinscale · 2018-09-07T22:16:45Z

modules/lang-painless/src/main/resources/org/elasticsearch/painless/spi/org.elasticsearch.txt

+  ZonedDateTime plusSeconds(long)
+  ZonedDateTime plusWeeks(long)
+  ZonedDateTime plusYears(long)
+  Instant toInstant()


where is this one coming from?

ignore this, misread it as being something for BWC, but it's a new method

spinscale · 2018-09-07T22:18:38Z

server/src/main/java/org/elasticsearch/script/JodaCompatibleZonedDateTime.java

+    }
+
+    public ZonedDateTime truncatedTo(TemporalUnit unit) {
+        return dt.truncatedTo(unit);


I think this will fail due to forbidden APIs - I remember playing with it for the formatters

spinscale · 2018-09-07T22:21:53Z

server/src/main/java/org/elasticsearch/script/JodaCompatibleZonedDateTime.java

+
+    private ZonedDateTime dt;
+
+    public JodaCompatibleZonedDateTime(Instant instant, ZoneId zone) {


will the zone id always be UTC for non test code? If so, we could change the visibility for this ctor?

I'd like to keep the signature the same as ZonedDateTime.ofInstant, since that is what the uses will go back to in 7.0.

fair enough

rjernst · 2018-09-08T18:52:36Z

@nik9000 @spinscale I believe I have addressed all of your comments.

spinscale · 2018-09-09T20:37:24Z

I think the test failures are coming from this change (I am pretty much happy though, once CI passes)

nik9000

I left a few small things.

nik9000 · 2018-09-13T23:00:06Z

docs/reference/aggregations/bucket/datehistogram-aggregation.asciidoc

@@ -425,7 +425,7 @@ POST /sales/_search?size=0
 --------------------------------------------------
 // CONSOLE
 // TEST[setup:sales]
-// TEST[warning:The joda time api for doc values is deprecated. Use -Des.scripting.use_java_time=true to use the java time api for date field doc values]
+// TEST[warning:The return type of [getDayOfWeek()] will change to an enum in 7.0. Use getDayOfWeekEnum().getValue().]


I think you should update this to the ES 7 compatible call.

nik9000 · 2018-09-13T23:02:43Z

server/src/main/java/org/elasticsearch/common/xcontent/XContentElasticsearchExtension.java

@@ -93,6 +93,7 @@
        writers.put(Year.class, (b, v) -> b.value(v.toString()));
        writers.put(Duration.class, (b, v) -> b.value(v.toString()));
        writers.put(Period.class, (b, v) -> b.value(v.toString()));
+        //writers.put(JodaCompatibleZonedDateTime.class, XContentBuilder::timeValue);


Do you mean to uncomment this?

I will be removing it. I added it originally because I thought it was needed due to the api leakage I mentioned in my other comment, but I found a way around that and don't want to expose this compatibility class at all.

nik9000 · 2018-09-13T23:04:20Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/DocValueFieldsFetchSubPhase.java

@@ -115,7 +115,7 @@ public void hitsExecute(SearchContext context, SearchHit[] hits) throws IOExcept
                        subReaderContext = context.searcher().getIndexReader().leaves().get(readerIndex);
                        data = indexFieldData.load(subReaderContext);
                        if (format == null) {
-                            scriptValues = data.getScriptValues();
+                            scriptValues = data.getLegacyFieldValues();


Will you change this in a follow up just for 7?

I won't, but this will be removed when not specifying format is removed, separate from any scripting/joda changes. The problem was ScriptDocValues were used for more than just scripts, and it leaks into the transport client api. I fixed this by keeping the fields fetch phase returning DateTime objects through this getLegacyFieldValues(). You can see not specifying format is already deprecated, so I expect it will be removed for 7.0.

spinscale

I found one more issue where you always use UTC and do not use the timezone of a zoned date time.

Apart from that I am good (no further review needed on my side).

spinscale · 2018-09-14T15:28:32Z

server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java

+            // write the joda compatibility datetime as joda datetime
+            o.writeByte((byte) 13);
+            final JodaCompatibleZonedDateTime zonedDateTime = (JodaCompatibleZonedDateTime) v;
+            String zoneId = zonedDateTime.getZonedDateTime().getZone().getId();


zoneId is unused here?

Oops. This was because the zoneId used by joda doesn't appear to always be the same as java time. Joda does not know how to parse "Z". I'll add special casing for that.

I pushed f105096

rjernst · 2018-09-16T21:01:19Z

@elasticmachine test this please

This commit switches the joda time backcompat in scripting to use augmentation over ZonedDateTime. The augmentation methods provide compatibility with the missing methods between joda's DateTime and java's ZonedDateTime. Due to getDayOfWeek returning an enum in the java API, ZonedDateTime is wrapped so that the method can return int like the joda time does. The java time api version is renamed to getDayOfWeekEnum, which will be kept through 7.x for compatibility while users switch back to getDayOfWeek once joda compatibility is removed.

rjernst added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v7.0.0 >refactoring v6.5.0 labels Sep 7, 2018

rjernst requested a review from spinscale September 7, 2018 00:42

nik9000 reviewed Sep 7, 2018

View reviewed changes

spinscale suggested changes Sep 7, 2018

View reviewed changes

address review comments

8260827

rjernst added 9 commits September 9, 2018 21:26

add serialization of new joda compat zoneddatetime

56f1f1e

fix aggs reading of dates

9035788

Merge branch 'master' into timeapi1

d876b3d

fix test return types

f3ec719

fix docs warning

19019e7

Merge branch 'master' into timeapi1

92a227c

add some missing methods, fix another test

8cc0a3a

fix rest tests

3930cf6

rework to still return DateTime in transport client

bd83968

nik9000 approved these changes Sep 13, 2018

View reviewed changes

rjernst added 2 commits September 13, 2018 17:29

add back serialization of compat class

11e03c7

remove deprecated use from docs

eab341a

spinscale suggested changes Sep 14, 2018

View reviewed changes

fix timezone serialization for joda bwc

f105096

spinscale approved these changes Sep 14, 2018

View reviewed changes

Merge branch 'master' into timeapi1

03a28dd

rjernst merged commit 3046656 into elastic:master Sep 17, 2018

rjernst deleted the timeapi1 branch September 17, 2018 02:18

centic9 mentioned this pull request Dec 6, 2018

JodaCompatibleZonedDateTime in Elasticsearch 6.5.x breaks scripts with "The datetime zone id 'Z' is not recognised" #36306

Closed

astefan mentioned this pull request Dec 28, 2018

SQL: Handle the bwc Joda ZonedDateTime scripting class in Painless #37024

Merged

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

reta mentioned this pull request May 19, 2022

[REMOVE] Remove from scripts the deprecated methods of JodaCompatibleZonedDateTime opensearch-project/OpenSearch-Dashboards#1600

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scripting: Rework joda time backcompat #33486

Scripting: Rework joda time backcompat #33486

rjernst commented Sep 7, 2018

elasticmachine commented Sep 7, 2018

nik9000 left a comment

nik9000 Sep 7, 2018

nik9000 Sep 7, 2018

nik9000 Sep 7, 2018

nik9000 Sep 7, 2018

nik9000 Sep 7, 2018

nik9000 Sep 7, 2018

spinscale left a comment

spinscale Sep 7, 2018

spinscale Sep 7, 2018

spinscale Sep 7, 2018

spinscale Sep 7, 2018

rjernst Sep 8, 2018

spinscale Sep 8, 2018

rjernst commented Sep 8, 2018

spinscale commented Sep 9, 2018

nik9000 left a comment

nik9000 Sep 13, 2018

nik9000 Sep 13, 2018

rjernst Sep 13, 2018

nik9000 Sep 13, 2018

rjernst Sep 13, 2018

spinscale left a comment

spinscale Sep 14, 2018

rjernst Sep 14, 2018

rjernst Sep 14, 2018

rjernst commented Sep 16, 2018


		private ZonedDateTime dt;

		public JodaCompatibleZonedDateTime(Instant instant, ZoneId zone) {

Scripting: Rework joda time backcompat #33486

Scripting: Rework joda time backcompat #33486

Conversation

rjernst commented Sep 7, 2018

elasticmachine commented Sep 7, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spinscale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Sep 8, 2018

spinscale commented Sep 9, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spinscale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Sep 16, 2018