Rewrite date processing in API v2 #1006

benjamingeer · 2018-10-10T15:25:08Z

This rewrites date processing in API v2 to use ICU4J, which supports many different calendars as well as Julian Day Numbers. This PR doesn't include support for calendars other than Gregorian and Julian, but now it should be much easier to add them. It removes the remaining dependencies of API v2 code on API v1 code, and eliminates the need to convert dates to strings when converting them between JSON-LD and Julian Day Numbers.

Add new date processing classes in org.knora.webapi.util.date.CalendarDateUtilV2.
Change existing v2 code to use the new classes.
Add tests to check that everything works as before.

Resolves #803.
Resolves #928.

codecov · 2018-10-10T16:05:13Z

Codecov Report

Merging #1006 into develop will increase coverage by 0.01%.
The diff coverage is 89.88%.

@@             Coverage Diff             @@
##           develop    #1006      +/-   ##
===========================================
+ Coverage    82.93%   82.94%   +0.01%     
===========================================
  Files          154      154              
  Lines        18589    18681      +92     
  Branches      1775     1781       +6     
===========================================
+ Hits         15416    15495      +79     
- Misses        3173     3186      +13

Flag	Coverage Δ
#integration	`43.18% <26.19%> (-0.22%)`	⬇️
#unit_e2e	`81.86% <89.28%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
.../scala/org/knora/webapi/util/StringFormatter.scala	`91.11% <100%> (+0.29%)`	⬆️
...ra/webapi/responders/v2/ResourcesResponderV2.scala	`90.97% <100%> (ø)`	⬆️
...knora/webapi/responders/v2/SearchResponderV2.scala	`87.69% <100%> (ø)`	⬆️
...rg/knora/webapi/util/ConstructResponseUtilV2.scala	`96.21% <100%> (+0.04%)`	⬆️
...s/v2/responder/valuemessages/ValueMessagesV2.scala	`74.94% <87.09%> (-0.71%)`	⬇️
...rg/knora/webapi/util/date/CalendarDateUtilV2.scala	`88.79% <88.79%> (ø)`
...c/main/scala/org/knora/webapi/util/CacheUtil.scala	`78.12% <0%> (-3.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93c210d...c1977f8. Read the comment docs.

tobiasschweizer · 2018-10-12T06:55:27Z

webapi/WebapiBuild.sbt

@@ -318,6 +319,8 @@ lazy val library =

        // Java EE modules which are deprecated in Java SE 9, 10 and will be removed in Java SE 11
        val jaxbApi                = "javax.xml.bind"                % "jaxb-api"                 % "2.2.12"
+
+        val icu4j                  = "com.ibm.icu"                   % "icu4j"                    % "62.1"


Can we get rid of val jodd = "org.jodd" % "jodd" % "3.2.6"?

Or do you want to leave DateUtilV1 as it is? Could we make call v1 v2 methods?

DateUtilV1 uses jodd. I could refactor it to use the v2 code, but that would be more work. My idea was to avoid changing v1 if possible, because the priority is to finish v2.

I could refactor it to use the v2 code, but that would be more work. My idea was to avoid changing v1 if possible, because the priority is to finish v2.

I understand. My concern is that both versions of the API have to behave identically when converting from and to JDN and that this might be more difficult to assure if we use two different libraries for v1 and v2.

But of course, there could be a version update of one of those libraries and then we would have the same issue if the new version behaves differently from the old one.

The tests for v1 and v2 are identical, so if that happens, I think we'll know. I'd be willing to change v1 to use v2, but I'd rather do it later, when v2 is done.

Ok, could you create a separate issue for that so we can do it later?

tobiasschweizer · 2018-10-12T07:00:06Z

webapi/src/main/scala/org/knora/webapi/util/date/CalendarDateUtilV2.scala

+  * @param endCalendarDate the end of the range.
+  */
+case class CalendarDateRangeV2(startCalendarDate: CalendarDateV2, endCalendarDate: CalendarDateV2) {
+    if (startCalendarDate.calendarName != endCalendarDate.calendarName) {


You could also check if startCalendarDate is before or equals endCalendarDate.

Yes, good point.

Done in c1977f8.

great, thx.

tobiasschweizer · 2018-10-12T07:14:28Z

webapi/src/main/scala/org/knora/webapi/util/date/CalendarDateUtilV2.scala

+/**
+  * Represents the name of the Julian calendar.
+  */
+case object CalendarNameJulian extends CalendarNameGregorianOrJulian {


How would that be extended once we support more calendars than Gregorian and Julian?

case object CalendarNameIslamic extends CalendarNameV2 case object CalendarNameHebrew extends CalendarNameV2

And so on.

tobiasschweizer · 2018-10-12T07:21:12Z

webapi/src/main/scala/org/knora/webapi/util/date/CalendarDateUtilV2.scala

+                throw AssertionException(s"Era is required in calendar $calendarName")
+            }
+
+        case _ => ()


In this case, we would have a non supported calendar, right?

Could you explain why there is a CalendarNameV2 and a trait CalendarNameGregorianOrJulian?

Is your rationale that you would add additional traits for other calendars and include them in the match case statement?

CalendarNameGregorianOrJulian is for things like this:

def getGregorianCalendarChangeDate(calendarName: CalendarNameGregorianOrJulian): Date

That method should only accept a Gregorian or Julian calendar, and using the type CalendarNameGregorianOrJulian guarantees that this is the case.

In this case, we would have a non supported calendar, right?

That's actually not possible with this code. The case _ is just there for the future, when we have more calendars. Until then, I could remove that line, because it can never be reached.

The design pattern here is called sealed case objects, which is an alternative to using Scala enumerations. See:

https://pedrorijo.com/blog/scala-enums/

With sealed case objects, the compiler can guarantee that the pattern matching is exhaustive.

Is your rationale that you would add additional traits for other calendars and include them in the match case statement?

Yes.

We're already using sealed case objects for ontology schemas:

https://github.com/dhlab-basel/Knora/blob/develop/webapi/src/main/scala/org/knora/webapi/OntologySchema.scala

The other advantage of sealed case objects is that you can make a hierarchy of subtypes, like CalendarNameV2 -> CalendarNameGregorianOrJulian -> CalendarNameGregorian, which you can't do with enumerations.

tobiasschweizer · 2018-10-12T07:42:01Z

webapi/src/main/scala/org/knora/webapi/util/date/CalendarDateUtilV2.scala

+
+    /**
+      * Parses a string representing a single date, without the calendar. This method does does not check that the
+      * date is valid in its calendar; to do that, call `toJulianDayRange` on it.


The actual validity will only be checked if a GregorianCalendar is instantiated and leniency is set to false, right?

Yes, and this happens in toJulianDayRange.

tobiasschweizer · 2018-10-12T07:45:13Z

webapi/src/main/scala/org/knora/webapi/util/date/CalendarDateUtilV2.scala

+
+package org.knora.webapi.util.date
+
+import java.util.Date


Is the Java date class (GregorianCalendar) used anywhere in this code?

http://icu-project.org/apiref/icu4j/com/ibm/icu/util/GregorianCalendar.html

ICU's replacement for java.util.GregorianCalendar.

I'm using the ICU GregorianCalendar class instead of the one in the Java standard library, because the ICU class supports Julian Day Numbers.

… end date.

benjamingeer · 2018-10-12T11:54:20Z

OK to merge now?

tobiasschweizer

approved on 2458404

benjamingeer · 2018-10-12T12:58:45Z

Thanks! :)

Benjamin Geer added 3 commits October 9, 2018 16:38

refactor (api-v2): Start refactoring date utility code (#928).

6fe7b14

refactor (api-v2): Rewrite date processing in API v2 using ICU4J.

2bbba03

test (CalendarDateUtilV2): Add more era tests.

dcb087d

benjamingeer added enhancement improve existing code or new feature API/V2 labels Oct 10, 2018

benjamingeer requested a review from subotic October 10, 2018 15:41

benjamingeer added the ready-for-review label Oct 10, 2018

benjamingeer requested a review from tobiasschweizer October 10, 2018 15:42

test (CalendarDateUtilV2): Add remaining tests from v1.

06c73e7

benjamingeer removed the request for review from subotic October 10, 2018 16:27

Benjamin Geer added 2 commits October 11, 2018 09:55

style (api-v2): Clean up code a bit.

9f732de

test (CalendarDateUtilV2): Clean up code.

0a4bf0e

benjamingeer mentioned this pull request Oct 11, 2018

Ben's PR history #571

Open

tobiasschweizer reviewed Oct 12, 2018

View reviewed changes

feature (CalendarDateUtilV2): Check that start date is not later than…

c1977f8

… end date.

tobiasschweizer approved these changes Oct 12, 2018

View reviewed changes

benjamingeer merged commit dfcc759 into develop Oct 12, 2018

benjamingeer deleted the wip/v2-refactor-date-util branch October 12, 2018 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite date processing in API v2 #1006

Rewrite date processing in API v2 #1006

benjamingeer commented Oct 10, 2018 •

edited

Loading

codecov bot commented Oct 10, 2018 •

edited

Loading

tobiasschweizer Oct 12, 2018

tobiasschweizer Oct 12, 2018

benjamingeer Oct 12, 2018

tobiasschweizer Oct 12, 2018

benjamingeer Oct 12, 2018

tobiasschweizer Oct 12, 2018

benjamingeer Oct 12, 2018

tobiasschweizer Oct 12, 2018

benjamingeer Oct 12, 2018

benjamingeer Oct 12, 2018

tobiasschweizer Oct 12, 2018

tobiasschweizer Oct 12, 2018

benjamingeer Oct 12, 2018

tobiasschweizer Oct 12, 2018

tobiasschweizer Oct 12, 2018

benjamingeer Oct 12, 2018

benjamingeer Oct 12, 2018

benjamingeer Oct 12, 2018

benjamingeer Oct 12, 2018 •

edited

Loading

benjamingeer Oct 12, 2018

tobiasschweizer Oct 12, 2018

benjamingeer Oct 12, 2018

tobiasschweizer Oct 12, 2018

benjamingeer Oct 12, 2018

benjamingeer Oct 12, 2018

benjamingeer Oct 12, 2018

benjamingeer commented Oct 12, 2018

tobiasschweizer left a comment

benjamingeer commented Oct 12, 2018


		package org.knora.webapi.util.date

		import java.util.Date

Rewrite date processing in API v2 #1006

Rewrite date processing in API v2 #1006

Conversation

benjamingeer commented Oct 10, 2018 • edited Loading

codecov bot commented Oct 10, 2018 • edited Loading

Codecov Report

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

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

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

benjamingeer Oct 12, 2018 • edited Loading

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

Choose a reason for hiding this comment

benjamingeer commented Oct 12, 2018

tobiasschweizer left a comment

Choose a reason for hiding this comment

benjamingeer commented Oct 12, 2018

benjamingeer commented Oct 10, 2018 •

edited

Loading

codecov bot commented Oct 10, 2018 •

edited

Loading

benjamingeer Oct 12, 2018 •

edited

Loading