openEHR / archie

OpenEHR library implementing ADL 2, AOM 2, BMM, RM 1.0.4 and many tools
Apache License 2.0
53 stars 25 forks source link

openEHR ISO8601 DURATION not fully supported #209

Closed chevalleyc closed 4 years ago

chevalleyc commented 4 years ago

Steps to reproduce

At the moment, Iso8601_duration is not fully supported at Archie level:

(extract from ehrbase/ehrbase#294)

The parsing of a DvDuration depends on its prefix and then is serviced by the JAVA api as follows:

"PT": Duration: 'A time-based amount of time, such as '34.5 seconds' see https://docs.oracle.com/javase/8/docs/api/java/time/Duration.html "": Period: "A date-based amount of time in the ISO-8601 calendar system, such as '2 years, 3 months and 4 days'' see https://docs.oracle.com/javase/8/docs/api/java/time/Period.html

F.e. the expression: P12DT23H51M59S fails to be parsed (as a Period), however, an invocation of Java Duration returns a TemporalAmount as expected.

Expected result

Iso8601_duration should be fully supported as specified in https://specifications.openehr.org/releases/BASE/latest/foundation_types.html#_iso8601_duration_class

pieterbos commented 4 years ago

Thank you for the bug repot. This may very well be related to https://github.com/openEHR/archie/issues/113

pieterbos commented 4 years ago

So this is not supported currently because the java Period and Duration classes do not support this: Period is only years, days and months, Duration is only hours, minutes seconds, etc.

We need a combination of the two to fix that. Luckily, that is very possible as a class implementing TemporalAmount, and the combination is part of threeten-extras: https://www.threeten.org/threeten-extra/apidocs/org.threeten.extra/org/threeten/extra/PeriodDuration.html

I think that would solve the problem?

There's still the matter left of Period not supporting Weeks natively, converting it to days in the process of parsing. So we may need an even better class, that has a PeriodPossiblyWithWeeksDuration?

pieterbos commented 4 years ago

Note that the example given in this issue with just days and a time does parse with Duration, converting it to hours - but years cannot be parsed as a java Duration. Currently working on negative duration values, should be easier to apply a fix for this after that is done, because then we have native parsers/serializers for all duration parsing in jackson and JAXB anyway, plus tests to cover them.

pieterbos commented 4 years ago

@chevalleyc and @ppazos , see the fix in the linked pull request. Could you review if that is what we need, or perhaps that we need an even better solution that also supports weeks natively, instead of converting it to days?

chevalleyc commented 4 years ago

I found an issue in the rendering of negative duration. Reusing the test provided, the actual toString() results as: P-10Y-10DT-12H-20S (expected: -P10Y10DT12H20S). Used json for this is:

                  "value": {
                    "_type": "DV_DURATION",
                    "value": "-P10Y10DT12H20S"
                  }
pieterbos commented 4 years ago

Ah, thanks, that's easy to fix. Is it correct that this is only the toString of DvDuration, and JSON, ADL, XML and ODIN serialization are correct?

pieterbos commented 4 years ago

Ah, I checked your test. This is not an issue of Archie.

You are doing a path lookup to the value attribute of a DvDuration. That is of course a PeriodDuration in this case, as specified. If you do a toString on that, you get the toString of a PeriodDuration, which is a different serialisation than the OpenEHR serialisation. Because there is no standard negative format in ISO 8601, unfortunately different formats exist.

DvDuration itself does not have a toString method - so there cannot be a problem in its implementation.

So, don't use the native toString of PeriodDuration. Instead, use the method com.nedap.archie.datetime.DateTimeSerializerFormatters.serializeDuration(TemporalAmount duration) to obtain the correct OpenEHR serialization format.

An alternative would be to implement our own TemporalAmount subclass. Then it can support weeks natively, and have the correct serialization builtin. However, we have not currently planned that development.

ppazos commented 4 years ago

@pieterbos @chevalleyc are negative durations something valid in ISO8601? I think the duration per se is positive and you can use it to operate over dates with +/- operators, but the operators is not part of the duration. Can you verify this?

pieterbos commented 4 years ago

negative durations are not valid in ISO 8601. Java supports it by allowing each part (year, month, hour, etc) to be positive or negative. OpenEHR only allows the total duration to be positive or negative. That's the difference you are seeing here.

ppazos commented 4 years ago

thanks @pieterbos I think all date/time in openEHR should be ISO8601 compatible, not sure where the negative durations were introduced, but might be a problem. Do you have a reference to the spec where that is? I can ask on the SEC.

pieterbos commented 4 years ago

Ian already answered that I think. Anyway, Archie version 0.16 should have no problems handling negative durations correctly, in the rare case that you need them. But use the serializer instead of Period/Duration/PeriodDuration.toString.