Rsdev 326 backend time validation (#7)

* add time field subclass to allow validation of time formats * record factory creates newly added ExtraTimeField * use feature branch version * Revert "add time field subclass to allow validation of time formats" This reverts commit 62541d7. * Revert "record factory creates newly added ExtraTimeField" This reverts commit 1b4e87f. * add validation to sample date and time fields * add time field to template * only perform validation where time has been set. empty values are permitted * remove empty strings from invalid list of time formats * combine ifs and extract to method. remove unused imports. * update time format in error message
rspace-os · Aug 27, 2024 · b615f25 · b615f25
1 parent 1f4a494
commit b615f25
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 54 deletions.
diff --git a/src/main/java/com/researchspace/model/inventory/field/InventoryDateField.java b/src/main/java/com/researchspace/model/inventory/field/InventoryDateField.java
@@ -1,13 +1,9 @@
 package com.researchspace.model.inventory.field;
 
-import java.text.DateFormat;
-import java.text.SimpleDateFormat;
+import com.researchspace.model.field.ErrorList;
 import java.time.LocalDate;
-import java.time.ZoneId;
 import java.time.format.DateTimeFormatter;
-import java.time.format.DateTimeFormatterBuilder;
 import java.time.format.DateTimeParseException;
-
 import javax.persistence.DiscriminatorValue;
 import javax.persistence.Entity;
 
@@ -22,10 +18,7 @@ public class InventoryDateField extends SampleField {
 
 	private static final long serialVersionUID = 2350588560175243555L;
 
-	private static DateFormat SUGGESTED_DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd");
-	static {
-		SUGGESTED_DATE_FORMAT.setLenient(false);
-	}
+	private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd");
 
 	public InventoryDateField() {
 		this("");
@@ -35,23 +28,27 @@ public InventoryDateField(String name) {
 		super(FieldType.DATE, name);
 	}
 
-	/**
-	 * Date field doesn't seem to have a required format internally 
-	 * (i.e. accepts any string as data), but for stricter checks 
-	 * or type guess this can be used.
-	 */
-	private static final DateTimeFormatter LENIENT_ISO_DATE_FORMATTER = 
-		new DateTimeFormatterBuilder()
-			.appendOptional(DateTimeFormatter.ISO_DATE_TIME)
-			.appendOptional(DateTimeFormatter.ISO_DATE)
-			.appendOptional(DateTimeFormatter.BASIC_ISO_DATE)
-			.toFormatter()
-			.withZone(ZoneId.systemDefault());
+	@Override
+	public ErrorList validate(String fieldData){
+		ErrorList errors = super.validate(fieldData);
+		if(dateHasContentAndIsInvalid(fieldData)) {
+				errors.addErrorMsg(String.format("%s is an invalid date format. Valid format is yyyy-MM-dd.", fieldData));
+			}
+		return errors;
+	}
+
+	private boolean dateHasContentAndIsInvalid(String date){
+		return date != null && !date.isEmpty() && !isValidDateFormat(date);
+	}
 
 	@Override
 	public boolean isSuggestedFieldForData(String dateString) {
+		return isValidDateFormat(dateString);
+	}
+
+	private boolean isValidDateFormat(String date){
 		try {
-			LocalDate.parse(dateString, LENIENT_ISO_DATE_FORMATTER);
+			LocalDate.parse(date, DATE_FORMATTER);
 		} catch (DateTimeParseException e) {
 			return false;
 		}

diff --git a/src/main/java/com/researchspace/model/inventory/field/InventoryTimeField.java b/src/main/java/com/researchspace/model/inventory/field/InventoryTimeField.java
@@ -1,9 +1,11 @@
 package com.researchspace.model.inventory.field;
 
-import java.text.DateFormat;
-import java.text.ParseException;
-import java.text.SimpleDateFormat;
+import com.researchspace.model.field.ErrorList;
 
+import java.time.LocalTime;
+import java.time.format.DateTimeFormatter;
+import java.time.format.DateTimeParseException;
+import java.time.format.ResolverStyle;
 import javax.persistence.DiscriminatorValue;
 import javax.persistence.Entity;
 
@@ -16,17 +18,10 @@
 @Audited
 public class InventoryTimeField extends SampleField {
 
-	private static final long serialVersionUID = 8709980361506172103L;	
+	private static final long serialVersionUID = 8709980361506172103L;
 
-	/**
-	 * Time field doesn't seem to have a required format internally 
-	 * (i.e. InventoryTimeField/TimeFieldForm seems to be able to store 
-	 * any string as data), but for any checks/type guesses this can be used.
-	 */
-	private static DateFormat SUGGESTED_TIME_FORMAT = new SimpleDateFormat("HH:mm");
-	static {
-		SUGGESTED_TIME_FORMAT.setLenient(false);
-	}
+	private static final DateTimeFormatter TIME_FORMATTER = DateTimeFormatter.ofPattern("HH:mm").withResolverStyle(
+			ResolverStyle.STRICT);
 
 	public InventoryTimeField() {
 		this("");
@@ -38,9 +33,26 @@ public InventoryTimeField(String name) {
 
 	@Override
 	public boolean isSuggestedFieldForData(String data) {
+		return isValidTimeFormat(data);
+	}
+
+	@Override
+	public ErrorList validate(String fieldData){
+		ErrorList errors = super.validate(fieldData);
+		if(timeHasContentAndIsInvalid(fieldData)){
+				errors.addErrorMsg(String.format("%s is an invalid 24hour time format. Valid format is HH:mm.", fieldData));
+		}
+		return errors;
+	}
+
+	private boolean timeHasContentAndIsInvalid(String time){
+		return time != null && !time.isEmpty() && !isValidTimeFormat(time);
+	}
+
+	private boolean isValidTimeFormat(String time){
 		try {
-			SUGGESTED_TIME_FORMAT.parse(data);
-		} catch (ParseException e) {
+			LocalTime.parse(time, TIME_FORMATTER);
+		} catch (DateTimeParseException e) {
 			return false;
 		}
 		return true;
@@ -52,5 +64,4 @@ public InventoryTimeField shallowCopy() {
 		copyFields(timeField);
 		return timeField;
 	}
-
 }
diff --git a/src/main/java/com/researchspace/model/record/RecordFactory.java b/src/main/java/com/researchspace/model/record/RecordFactory.java
@@ -5,6 +5,7 @@
 
 import com.researchspace.model.inventory.Basket;
 import com.researchspace.model.inventory.DigitalObjectIdentifier;
+import com.researchspace.model.inventory.field.InventoryTimeField;
 import java.math.BigDecimal;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -370,6 +371,11 @@ public Sample createComplexSampleTemplate(String templateName, String desc, User
 		InventoryAttachmentField aff = new InventoryAttachmentField();
 		aff.setName("MyAttachment");
 		template.addSampleField(aff);
+
+		InventoryTimeField time = new InventoryTimeField();
+		time.setName("MyTime");
+		time.setFieldData("00:00");
+		template.addSampleField(time);
 
 		InventoryRadioFieldDef nodefaultsRadioFieldDef = new InventoryRadioFieldDef();
 		nodefaultsRadioFieldDef.setRadioOptionsList(Arrays.asList("option1", "option2"));

diff --git a/src/test/java/com/researchspace/model/inventory/field/SampleFieldTest.java b/src/test/java/com/researchspace/model/inventory/field/SampleFieldTest.java
@@ -141,27 +141,33 @@ void numberFieldEmptyValues(String value) {
 		field.setFieldData(value);
 	}
 
-	/*
-	 * The test is here to document current behaviour.
-	 * Validation should probably be stricter than it is.
-	 */
 	@ParameterizedTest
-	@ValueSource(strings = { "2020-01-31", " ", "2020-01-31T15:50:00", "20200131",
-			"2020.01.01", "anything", "456.44", "-123" })
+	@ValueSource(strings = { "2020-01-31", "2024-08-19", "1999-12-31" })
 	void dateFieldValidValues(String value) {
 		InventoryDateField field = new InventoryDateField("date");
 		field.setFieldData(value);
 	}
 
 	@ParameterizedTest
-	@ValueSource(strings = { "2020-01-31", "2020-01-31T15:50:00", "20200131" })
+	@ValueSource(strings = {" ", "2020-01-31T15:50:00", "20200131",
+			"2020.01.01", "anything", "456.44", "-123" })
+	void setDateFieldWithInvalidFormatThrowsException(String value) {
+		InventoryDateField field = new InventoryDateField("date");
+		IllegalArgumentException exception = assertThrows(IllegalArgumentException.class,
+				() -> field.setFieldData(value) );
+		assertTrue(exception.getMessage().contains("is invalid for field type Date"));
+	}
+
+
+	@ParameterizedTest
+	@ValueSource(strings = { "2020-01-31", "1900-01-01", "2124-08-19"})
 	void dateFieldSuggestedForValue(String value) {
 		InventoryDateField field = new InventoryDateField("date");
 		assertTrue(field.isSuggestedFieldForData(value));
 	}
 
 	@ParameterizedTest
-	@ValueSource(strings = { "2020.01.01", "anything", "456.44", "-123" })
+	@ValueSource(strings = { "2020.01.01", "anything", "456.44", "-123", "2020-01-31T15:50:00", "20200131" })
 	void dateFieldNotSuggestedForValue(String value) {
 		InventoryDateField field = new InventoryDateField("date");
 		assertFalse(field.isSuggestedFieldForData(value));
@@ -198,32 +204,37 @@ void uriFieldNotSuggestedForValue(String value) {
 		InventoryUriField field = new InventoryUriField("uri");
 		assertFalse(field.isSuggestedFieldForData(value));
 	}
-
-	/*
-	 * The test is here to document current behaviour.
-	 * Validation should probably be stricter than it is.
-	 */
+
 	@ParameterizedTest
-	@ValueSource(strings = { "12:45", " ", "time", "no time", "-1"})
+	@ValueSource(strings = { "12:45", "00:00", "09:25", "23:59"})
 	void timeFieldValidValues(String value) {
 		InventoryTimeField field = new InventoryTimeField("time");
 		field.setFieldData(value);
 	}
 
 	@ParameterizedTest
-	@ValueSource(strings = { "12:45", "00:00" })
+	@ValueSource(strings = { "12:45", "00:00", "09:25", "23:59" })
 	void timeFieldSuggestedForValue(String value) {
 		InventoryTimeField field = new InventoryTimeField("time");
 		assertTrue(field.isSuggestedFieldForData(value));
 	}
 
 	@ParameterizedTest
-	@ValueSource(strings = { "time", "no time", "-1" })
+	@ValueSource(strings = { "time", "no time", "-1", " ", "10:24am", "23:79"})
 	void timeFieldNotSuggestedForValue(String value) {
 		InventoryTimeField field = new InventoryTimeField("time");
 		assertFalse(field.isSuggestedFieldForData(value));
 	}
 
+	@ParameterizedTest
+	@ValueSource(strings = {"2020-01-31T15:50:00", "9:17", "10:24am", "23:79" })
+	void setTimeInvalidFormatThrowsError(String value) {
+		InventoryTimeField field = new InventoryTimeField("time");
+		IllegalArgumentException exception = assertThrows(IllegalArgumentException.class,
+				() -> field.setFieldData(value) );
+		assertTrue(exception.getMessage().contains("is invalid for field type Time"));
+	}
+
 	@Test
 	public void attachmentField() {
 		InventoryAttachmentField field = new InventoryAttachmentField();