Add validations for Intervals in the definition of archetypes (#137)

* Add validations for Intervals in the definition of archetypes

* process review comments in Interval checks

* Implement a basic TemporalAmount to Duration converter for the Interval case

* Add test for Interval of Period+Duration.has

* Added some validation tests for intervals. Removed the existence case, it is checked elsewhere

base/src/main/java/com/nedap/archie/base/Interval.java

@@ -1,11 +1,14 @@
 package com.nedap.archie.base;
 
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.nedap.archie.rminfo.RMPropertyIgnore;
+
 import javax.annotation.Nullable;
 import javax.xml.bind.annotation.XmlAccessType;
 import javax.xml.bind.annotation.XmlAccessorType;
 import javax.xml.bind.annotation.XmlAttribute;
+import javax.xml.bind.annotation.XmlTransient;
 import javax.xml.bind.annotation.XmlType;
-import java.time.Duration;
 import java.time.temporal.TemporalAmount;
 import java.util.Objects;
 
@@ -146,11 +149,11 @@ public boolean has(T value) {
         Comparable comparableValue;
         Comparable comparableLower;
         Comparable comparableUpper;
-		if (value instanceof TemporalAmount && !(value instanceof Comparable) && isNonComparableTemporalAmount(lower) && isNonComparableTemporalAmount(upper)) {
+		if (value instanceof TemporalAmount && lower instanceof TemporalAmount && upper instanceof TemporalAmount) {
             //TemporalAmount is not comparable, but can always be converted to a duration that is comparable.
-            comparableValue = value == null ? null : Duration.from((TemporalAmount) value);
-            comparableLower = lower == null ? null : Duration.from((TemporalAmount) lower);
-            comparableUpper = upper == null ? null : Duration.from((TemporalAmount) upper);
+            comparableValue = toComparable(value);
+            comparableLower = toComparable(lower);
+            comparableUpper = toComparable(upper);
 		} else if (!(isComparable(lower) && isComparable(upper) && isComparable(value))) {
             throw new UnsupportedOperationException("subclasses of interval not implementing comparable should implement their own has method");
         } else {
@@ -180,13 +183,50 @@ public boolean has(T value) {
         return true;
     }
 
+    /**
+     * Get the lower value as a comparable amount. Required because some temporal amounts are not always directly comparable
+     * @return
+     */
+    @JsonIgnore
+    @XmlTransient
+    @RMPropertyIgnore
+    public Comparable getComparableLower() {
+        return toComparable(lower);
+    }
+
+    /**
+     * Get the lower value as a comparable amount. Required because some temporal amounts are not always directly comparable
+     * @return
+     */
+    @JsonIgnore
+    @XmlTransient
+    @RMPropertyIgnore
+    public Comparable getComparableUpper() {
+        return toComparable(upper);
+    }
+
+    private Comparable toComparable(T value) {
+        if(value == null) {
+            return null;
+        }
+        if (value instanceof TemporalAmount && !(value instanceof Comparable) && isNonComparableTemporalAmount(value)) {
+            //TemporalAmount is not comparable, but can always be converted to a duration that is comparable.
+            return IntervalDurationConverter.from((TemporalAmount) value);
+
+        } else if (!isComparable(value)) {
+            throw new UnsupportedOperationException("subclasses of interval not implementing comparable should implement their own has method");
+        } else {
+            return (Comparable) value;
+        }
+    }
+
     private int compareTo(T intervalValue, T value) {
         Comparable comparableIntervalValue;
         Comparable comparableValue;
         if (value instanceof TemporalAmount && !(value instanceof Comparable) && isNonComparableTemporalAmount(intervalValue)) {
             //TemporalAmount is not comparable, but can always be converted to a duration that is comparable.
-            comparableValue = value == null ? null : Duration.from((TemporalAmount) value);
-            comparableIntervalValue = intervalValue == null ? null : Duration.from((TemporalAmount) intervalValue);
+            comparableValue = value == null ? null : IntervalDurationConverter.from((TemporalAmount) value);
+            comparableIntervalValue = intervalValue == null ? null : IntervalDurationConverter.from((TemporalAmount) intervalValue);
         } else if (!(isComparable(intervalValue) && isComparable(value))) {
             throw new UnsupportedOperationException("subclasses of interval not implementing comparable should implement their own has method");
         } else {
@@ -200,8 +240,8 @@ private boolean isNonComparableTemporalAmount(T value) {
         return value == null || (!(value instanceof Comparable) && value instanceof TemporalAmount);
     }
 
-    private boolean isComparable(T upper) {
-        return upper == null || upper instanceof Comparable;
+    private boolean isComparable(T value) {
+        return value == null || value instanceof Comparable;
     }
 
 

base/src/main/java/com/nedap/archie/base/IntervalDurationConverter.java

@@ -0,0 +1,65 @@
+package com.nedap.archie.base;
+
+
+import java.time.Duration;
+import java.time.temporal.ChronoUnit;
+import java.time.temporal.TemporalAmount;
+import java.time.temporal.TemporalUnit;
+import java.time.temporal.UnsupportedTemporalTypeException;
+import java.util.Objects;
+
+import static java.time.temporal.ChronoUnit.DAYS;
+
+/**
+ * A tool to convert Durations from TemporalAmounts in the specific use case of Intervals
+ * It handles the unit Month as an exact number of seconds to do a conversion. This is a bit tricky, since
+ * a number of months has a lower number of seconds and an upper number of seconds. but we need some way of doing this...
+ *
+ * This may need a separate method for lower and upper, which returns the lower and upper bounds of the seconds in a month?
+ */
+class IntervalDurationConverter {
+
+//    private final static long MINIMUM_SECONDS_IN_DAY = 60*60*23;
+//    private final static long MINIMUM_SECONDS_IN_MONTH = 28*MINIMUM_SECONDS_IN_DAY;
+//    private final static long MINIMUM_SECONDS_IN_WEEK = 7*MINIMUM_SECONDS_IN_DAY;
+//
+//    private final static long MAXIMUM_SECONDS_IN_DAY = 60*60*25;
+//    private final static long MAXIMUM_SECONDS_IN_WEEK = 7*MAXIMUM_SECONDS_IN_DAY;
+//    private final static long MAXIMUM_SECONDS_IN_MONTH = 28*MAXIMUM_SECONDS_IN_WEEK;
+
+    /**
+     * This is a literal copy of Duration.from, with the only change being that this supports adding
+     * estimate TemporalAmounts
+     * @param amount
+     * @return
+     */
+    public static Duration from(TemporalAmount amount) {
+        Objects.requireNonNull(amount, "amount");
+        Duration duration = Duration.ofSeconds(0);
+        for (TemporalUnit unit : amount.getUnits()) {
+            duration = plus(duration, amount.get(unit), unit);
+        }
+        return duration;
+    }
+
+
+    /**
+     * Returns a copy of this duration with the specified duration added. Works the same as {@link Duration#plus(Duration)},
+     * but does work for estimate units such as weeks, months and years
+
+     * @param amountToAdd  the amount to add, measured in terms of the unit, positive or negative
+     * @param unit  the unit that the amount is measured in, must have an exact duration, not null
+     * @return a {@code Duration} based on this duration with the specified duration added, not null
+     * @throws UnsupportedTemporalTypeException if the unit is not supported
+     * @throws ArithmeticException if numeric overflow occurs
+     */
+    private static Duration plus(Duration value, long amountToAdd, TemporalUnit unit) {
+        Objects.requireNonNull(unit, "unit");
+        if (unit != DAYS && unit.isDurationEstimated()) {
+            Duration duration = unit.getDuration().multipliedBy(amountToAdd);
+            return value.plusSeconds(duration.getSeconds()).plusNanos(duration.getNano());
+        } else {
+            return value.plus(amountToAdd, unit);
+        }
+    }
+}

base/src/test/java/com/nedap/archie/base/IntervalOfPeriodTest.java

@@ -0,0 +1,51 @@
+package com.nedap.archie.base;
+
+import org.junit.Test;
+
+import java.time.Duration;
+import java.time.Period;
+import java.time.temporal.ChronoUnit;
+import java.time.temporal.TemporalAmount;
+import java.time.temporal.TemporalUnit;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+
+public class IntervalOfPeriodTest {
+
+    @Test
+    public void testMonths() {
+        Period oneMonth = Period.of(0,1, 0);
+        Period twoMonths = Period.of(0,2, 0);
+        Interval interval = new Interval<TemporalAmount>(oneMonth, twoMonths);
+        Duration oneAndAHalfMonth = Duration.of(45, ChronoUnit.DAYS);
+        assertTrue(interval.has(oneAndAHalfMonth));
+        Duration threeMonths = Duration.of(31*3, ChronoUnit.DAYS);
+        assertFalse(interval.has(threeMonths));
+
+
+        assertTrue(interval.has(Period.of(0, 1, 0)));
+        assertTrue(interval.has(Period.of(0, 2, 0)));
+
+        //testing edge cases of Duration in hours would be even better, but once makes sense once we have proper
+        //converter support, with difference between converting lower and upper values
+
+    }
+
+    @Test
+    public void testDurations() {
+        Duration oneHour = Duration.of(1, ChronoUnit.HOURS);
+        Duration twoHour = Duration.of(2, ChronoUnit.HOURS);
+        Interval interval = new Interval<TemporalAmount>(oneHour, twoHour);
+        assertTrue(interval.has(Duration.of(60, ChronoUnit.MINUTES)));
+        assertTrue(interval.has(Duration.of(120, ChronoUnit.MINUTES)));
+
+        assertFalse(interval.has(Duration.of(59, ChronoUnit.MINUTES)));
+        assertFalse(interval.has(Duration.of(121, ChronoUnit.MINUTES)));
+
+        Duration oneAndAHalfMonth = Duration.of(45, ChronoUnit.DAYS);
+        assertFalse(interval.has(oneAndAHalfMonth));
+
+    }
+}

tools/src/main/java/com/nedap/archie/archetypevalidator/ArchetypeValidator.java

@@ -52,6 +52,7 @@ public ArchetypeValidator(MetaModels models) {
         //but there's no reason this cannot be parsed, so check them here
         validationsPhase0.add(new AttributeUniquenessValidation());
         validationsPhase0.add(new NodeIdValidation());
+        validationsPhase0.add(new BasicDefinitionObjectValidation());
 
         validationsPhase0.add(new AttributeTupleValidation());
 

tools/src/main/java/com/nedap/archie/archetypevalidator/validations/BasicDefinitionObjectValidation.java

@@ -0,0 +1,64 @@
+package com.nedap.archie.archetypevalidator.validations;
+
+import com.nedap.archie.aom.CAttribute;
+import com.nedap.archie.aom.CObject;
+import com.nedap.archie.aom.primitives.COrdered;
+import com.nedap.archie.archetypevalidator.ErrorType;
+import com.nedap.archie.archetypevalidator.ValidatingVisitor;
+import com.nedap.archie.base.Interval;
+import org.openehr.utils.message.I18n;
+
+/**
+ * Contains validation for basic object validity. Many of these checks are also included in the ADL grammar, but\
+ * of course not every AOM object comes from ADL :)
+ */
+public class BasicDefinitionObjectValidation extends ValidatingVisitor {
+
+    protected void validate(CObject cObject) {
+        if(cObject.getOccurrences() != null) {
+            validateOccurrences(cObject);
+        }
+        if(cObject instanceof COrdered) {
+            validateCOrdered((COrdered) cObject);
+        }
+    }
+
+    private void validateCOrdered(COrdered<?> cOrdered) {
+        for(Interval interval:cOrdered.getConstraint()) {
+            if(!isValidInterval(interval)) {
+                this.addMessageWithPath(ErrorType.OTHER, cOrdered.path(), I18n.t("The constraint interval for this {0} has lower > upper, this is not allowed", cOrdered.getClass().getSimpleName()));
+            }
+        }
+    }
+
+    protected void validate(CAttribute cAttribute) {
+
+        if(cAttribute.getCardinality() != null) {
+            if (cAttribute.getCardinality().getInterval() == null) {
+                this.addMessageWithPath(ErrorType.OTHER, cAttribute.path(), I18n.t("Cardinality must have an interval present, but it was null"));
+            } else if(!isValidInterval(cAttribute.getCardinality().getInterval())) {
+                this.addMessageWithPath(ErrorType.OTHER, cAttribute.path(), I18n.t("The attribute cardinality interval has lower > upper, this is not allowed"));
+            }
+        }
+
+    }
+
+
+
+    private void validateOccurrences(CObject cObject) {
+        if(!isValidInterval(cObject.getOccurrences())) {
+            this.addMessageWithPath(ErrorType.OTHER, cObject.path(), I18n.t("The occurrences interval has lower > upper, this is not allowed"));
+        }
+    }
+
+    private boolean isValidInterval(Interval<? extends Comparable> interval) {
+        if(interval.getUpper() != null && interval.getLower() != null &&
+            !interval.isLowerUnbounded() &&
+            !interval.isUpperUnbounded()){
+            if(interval.getComparableUpper().compareTo(interval.getComparableLower()) < 0){
+                return false;
+            }
+        }
+        return true;
+    }
+}

tools/src/test/java/com/nedap/archie/archetypevalidator/BigArchetypeValidatorTest.java

@@ -103,7 +103,7 @@ private FullArchetypeRepository parseAll() {
         Reflections reflections = new Reflections("adl2-tests", new ResourcesScanner());
         List<String> adlFiles = new ArrayList(reflections.getResources(Pattern.compile(".*\\.adls")));
         for(String file:adlFiles) {
-            if (file.contains("legacy_adl_1.4")) {
+            if (file.contains("legacy_adl1.4")) {
                 continue;
             }
             Archetype archetype = null;

tools/src/test/resources/adl2-tests/validity/basics/openEHR-TEST_PKG-WHOLE.OTHER_bad_cardinality.v1.0.0.adls

@@ -0,0 +1,36 @@
+archetype (adl_version=2.0.5; rm_release=1.0.2)
+	openehr-TEST_PKG-WHOLE.bad_cardinality.v1.0.0
+
+language
+	original_language = <[ISO_639-1::en]>
+
+description
+	original_author = <
+		["name"] = <"Pieter Bos">
+	>
+	details = <
+		["en"] = <
+			language = <[ISO_639-1::en]>
+			purpose = <"test archetype with bad existence interval (upper > 1)">
+			keywords = <"ADL", "test">
+		>
+	>
+	lifecycle_state = <"published">
+	other_details = <
+		["regression"] = <"OTHER">
+	>
+
+definition
+	WHOLE[id1] matches {	-- test entry
+		integer_attr3 cardinality matches {5..3}
+	}
+
+terminology
+	term_definitions = <
+		["en"] = <
+			["id1"] = <
+				text = <"test entry">
+				description = <"test entry">
+			>
+		>
+	>

tools/src/test/resources/adl2-tests/validity/basics/openEHR-TEST_PKG-WHOLE.OTHER_bad_existence.v1.0.0.adls

@@ -0,0 +1,36 @@
+archetype (adl_version=2.0.5; rm_release=1.0.2)
+	openehr-TEST_PKG-WHOLE.bad_existence.v1.0.0
+
+language
+	original_language = <[ISO_639-1::en]>
+
+description
+	original_author = <
+		["name"] = <"Pieter Bos">
+	>
+	details = <
+		["en"] = <
+			language = <[ISO_639-1::en]>
+			purpose = <"test archetype with bad existence interval (upper > 1)">
+			keywords = <"ADL", "test">
+		>
+	>
+	lifecycle_state = <"published">
+	other_details = <
+		["regression"] = <"SEXLU">
+	>
+
+definition
+	WHOLE[id1] matches {	-- test entry
+		integer_attr3 existence matches {0..5}
+	}
+
+terminology
+	term_definitions = <
+		["en"] = <
+			["id1"] = <
+				text = <"test entry">
+				description = <"test entry">
+			>
+		>
+	>