Validate datetime (flyteorg#309)

* Add datetime validation Signed-off-by: Lucia Pasarin <[email protected]> * Clean up Signed-off-by: Lucia Pasarin <[email protected]> * Adjusted validation + added test Signed-off-by: Lucia Pasarin <[email protected]> * Use datetime validation as part of parameter map validation Signed-off-by: Lucia Pasarin <[email protected]> * Add test for map validation + fix part of datetime in map validation Signed-off-by: Lucia Pasarin <[email protected]> * Fix validation by splitting intermediate steps Signed-off-by: Lucia Pasarin <[email protected]> * Clean up Signed-off-by: Lucia Pasarin <[email protected]> * Simplify validation since field should only be datetime in this case because it is protobuf generated Signed-off-by: Lucia Pasarin <[email protected]> * Remove //go:build integration Signed-off-by: Lucia Pasarin <[email protected]> * Fix README goimports + add validation in literal map too Signed-off-by: Lucia Pasarin <[email protected]> * Complete test cases Signed-off-by: Lucia Pasarin <[email protected]> * Stop using reflection for isDateTime check Signed-off-by: Lucia Pasarin <[email protected]> * Use direct access instead of casting + fix string format in test Signed-off-by: Lucia Pasarin <[email protected]> * Throw error in case of nil datetime + fix assertion Signed-off-by: Lucia Pasarin <[email protected]> * Add one more test case to check for Instant.MIN Signed-off-by: Lucia Pasarin <[email protected]> * Fix flaky tests Signed-off-by: Lucia Pasarin <[email protected]> * Fix test Signed-off-by: Lucia Pasarin <[email protected]> Co-authored-by: Lucia Pasarin <[email protected]>
eapolinario · Jan 11, 2022 · be9e098 · be9e098
1 parent c15ec5a
commit be9e098
Show file tree

Hide file tree

Showing 3 changed files with 223 additions and 53 deletions.
diff --git a/README.md b/README.md
@@ -24,6 +24,11 @@ Flyte Admin has a few useful make targets for linting and testing. Please use th
 minor bugs and linting errors.
 
 ```
+  # Please make sure you have all the dependencies installed:
+  $ make install
+  
+  # In case you are only missing goimports:
+  $ go install golang.org/x/tools/cmd/goimports@latest
   $ make goimports
 ```
 

diff --git a/pkg/manager/impl/validation/validation.go b/pkg/manager/impl/validation/validation.go
@@ -29,15 +29,15 @@ func ValidateEmptyStringField(field, fieldName string) error {
 	return nil
 }
 
-// Validates that a string field does not exceed a certain character count
+// ValidateMaxLengthStringField Validates that a string field does not exceed a certain character count
 func ValidateMaxLengthStringField(field string, fieldName string, limit int) error {
 	if len(field) > limit {
 		return errors.NewFlyteAdminErrorf(codes.InvalidArgument, "%s cannot exceed %d characters", fieldName, limit)
 	}
 	return nil
 }
 
-// Validates that a map field does not exceed a certain amount of entries
+// ValidateMaxMapLengthField Validates that a map field does not exceed a certain amount of entries
 func ValidateMaxMapLengthField(m map[string]string, fieldName string, limit int) error {
 	if len(m) > limit {
 		return errors.NewFlyteAdminErrorf(codes.InvalidArgument, "%s map cannot exceed %d entries", fieldName, limit)
@@ -91,7 +91,7 @@ func ValidateIdentifierFieldsSet(id *core.Identifier) error {
 	return nil
 }
 
-// Validates that all required fields for an identifier are present.
+// ValidateIdentifier Validates that all required fields for an identifier are present.
 func ValidateIdentifier(id *core.Identifier, expectedType common.Entity) error {
 	if id == nil {
 		return shared.GetMissingArgumentError(shared.ID)
@@ -104,7 +104,7 @@ func ValidateIdentifier(id *core.Identifier, expectedType common.Entity) error {
 	return ValidateIdentifierFieldsSet(id)
 }
 
-// Validates that all required fields for an identifier are present.
+// ValidateNamedEntityIdentifier Validates that all required fields for an identifier are present.
 func ValidateNamedEntityIdentifier(id *admin.NamedEntityIdentifier) error {
 	if id == nil {
 		return shared.GetMissingArgumentError(shared.ID)
@@ -199,6 +199,10 @@ func validateLiteralMap(inputMap *core.LiteralMap, fieldName string) error {
 			if fixedInput == nil || fixedInput.GetValue() == nil {
 				return errors.NewFlyteAdminErrorf(codes.InvalidArgument, "missing valid literal in %s %s", fieldName, name)
 			}
+			if isDateTime(fixedInput) {
+				// Make datetime specific validations
+				return ValidateDatetime(fixedInput)
+			}
 		}
 	}
 	return nil
@@ -228,13 +232,17 @@ func validateParameterMap(inputMap *core.ParameterMap, fieldName string) error {
 						"Type mismatch for Parameter %s in %s has type %s, expected %s", name, fieldName,
 						defaultInput.GetVar().GetType().String(), inputType.String())
 				}
+				if defaultInput.GetVar().GetType().GetSimple() == core.SimpleType_DATETIME {
+					// Make datetime specific validations
+					return ValidateDatetime(defaultValue)
+				}
 			}
 		}
 	}
 	return nil
 }
 
-// Offsets are encoded as string tokens to enable future api pagination changes. In addition to validating that an
+// ValidateToken Offsets are encoded as string tokens to enable future api pagination changes. In addition to validating that an
 // offset is a valid integer, we assert that it is non-negative.
 func ValidateToken(token string) (int, error) {
 	if token == "" {
@@ -268,3 +276,21 @@ func ValidateOutputData(outputData *core.LiteralMap, maxSizeInBytes int64) error
 	}
 	return errors.NewFlyteAdminErrorf(codes.ResourceExhausted, "Output data size exceeds platform configured threshold (%+v > %v)", outputSizeInBytes, maxSizeInBytes)
 }
+
+func ValidateDatetime(literal *core.Literal) error {
+	if literal == nil {
+		return errors.NewFlyteAdminErrorf(codes.InvalidArgument, "Found invalid nil datetime")
+	}
+
+	timestamp := literal.GetScalar().GetPrimitive().GetDatetime()
+
+	err := timestamp.CheckValid()
+	if err != nil {
+		return errors.NewFlyteAdminErrorf(codes.InvalidArgument, err.Error())
+	}
+	return nil
+}
+
+func isDateTime(input *core.Literal) bool {
+	return input.GetScalar() != nil && input.GetScalar().GetPrimitive() != nil && input.GetScalar().GetPrimitive().GetDatetime() != nil
+}
diff --git a/pkg/manager/impl/validation/validation_test.go b/pkg/manager/impl/validation/validation_test.go
@@ -2,6 +2,9 @@ package validation
 
 import (
 	"testing"
+	"time"
+
+	"google.golang.org/protobuf/types/known/timestamppb"
 
 	"github.com/flyteorg/flyteidl/clients/go/coreutils"
 
@@ -153,63 +156,84 @@ func TestValidateListTaskRequest_MissingLimit(t *testing.T) {
 }
 
 func TestValidateParameterMap(t *testing.T) {
-	exampleMap := core.ParameterMap{
-		Parameters: map[string]*core.Parameter{
-			"foo": {
-				Var: &core.Variable{
-					Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_STRING}},
-				},
-				Behavior: &core.Parameter_Default{
-					Default: coreutils.MustMakeLiteral("foo-value"),
+	t.Run("valid field", func(t *testing.T) {
+		exampleMap := core.ParameterMap{
+			Parameters: map[string]*core.Parameter{
+				"foo": {
+					Var: &core.Variable{
+						Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_STRING}},
+					},
+					Behavior: &core.Parameter_Default{
+						Default: coreutils.MustMakeLiteral("foo-value"),
+					},
 				},
 			},
-		},
-	}
-	err := validateParameterMap(&exampleMap, "foo")
-	assert.NoError(t, err)
-
-	exampleMap = core.ParameterMap{
-		Parameters: map[string]*core.Parameter{
-			"foo": {
-				Var: &core.Variable{
-					Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_STRING}},
+		}
+		err := validateParameterMap(&exampleMap, "foo")
+		assert.NoError(t, err)
+	})
+	t.Run("invalid because missing required and defaults", func(t *testing.T) {
+		exampleMap := core.ParameterMap{
+			Parameters: map[string]*core.Parameter{
+				"foo": {
+					Var: &core.Variable{
+						Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_STRING}},
+					},
+					Behavior: nil, // neither required or defaults
 				},
-				Behavior: nil, // neither required or defaults
 			},
-		},
-	}
-	err = validateParameterMap(&exampleMap, "some text")
-	assert.Error(t, err)
-
-	exampleMap = core.ParameterMap{
-		Parameters: map[string]*core.Parameter{
-			"foo": {
-				Var: &core.Variable{
-					Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_STRING}},
-				},
-				Behavior: &core.Parameter_Required{
-					Required: true,
+		}
+		err := validateParameterMap(&exampleMap, "some text")
+		assert.Error(t, err)
+	})
+	t.Run("valid with required true", func(t *testing.T) {
+		exampleMap := core.ParameterMap{
+			Parameters: map[string]*core.Parameter{
+				"foo": {
+					Var: &core.Variable{
+						Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_STRING}},
+					},
+					Behavior: &core.Parameter_Required{
+						Required: true,
+					},
 				},
 			},
-		},
-	}
-	err = validateParameterMap(&exampleMap, "some text")
-	assert.NoError(t, err)
-
-	exampleMap = core.ParameterMap{
-		Parameters: map[string]*core.Parameter{
-			"foo": {
-				Var: &core.Variable{
-					Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_STRING}},
+		}
+		err := validateParameterMap(&exampleMap, "some text")
+		assert.NoError(t, err)
+	})
+	t.Run("invalid because not required and no default provided", func(t *testing.T) {
+		exampleMap := core.ParameterMap{
+			Parameters: map[string]*core.Parameter{
+				"foo": {
+					Var: &core.Variable{
+						Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_STRING}},
+					},
+					Behavior: &core.Parameter_Required{
+						Required: false,
+					},
 				},
-				Behavior: &core.Parameter_Required{
-					Required: false,
+			},
+		}
+		err := validateParameterMap(&exampleMap, "some text")
+		assert.Error(t, err)
+	})
+	t.Run("valid datetime field", func(t *testing.T) {
+		exampleMap := core.ParameterMap{
+			Parameters: map[string]*core.Parameter{
+				"foo": {
+					Var: &core.Variable{
+						Type: &core.LiteralType{Type: &core.LiteralType_Simple{Simple: core.SimpleType_DATETIME}},
+					},
+					Behavior: &core.Parameter_Default{
+						Default: coreutils.MustMakeLiteral(time.Now()),
+					},
 				},
 			},
-		},
-	}
-	err = validateParameterMap(&exampleMap, "some text")
-	assert.Error(t, err)
+		}
+		err := validateParameterMap(&exampleMap, "some text")
+		assert.NoError(t, err)
+	})
 }
 
 func TestValidateToken(t *testing.T) {
@@ -334,3 +358,118 @@ func TestValidateOutputData(t *testing.T) {
 		assert.Equal(t, codes.ResourceExhausted, err.(errors.FlyteAdminError).Code())
 	})
 }
+
+func TestValidateDatetime(t *testing.T) {
+	t.Run("no datetime", func(t *testing.T) {
+		assert.EqualError(t, ValidateDatetime(nil), "Found invalid nil datetime")
+	})
+	t.Run("datetime with valid format and value", func(t *testing.T) {
+		assert.NoError(t, ValidateDatetime(&core.Literal{
+			Value: &core.Literal_Scalar{
+				Scalar: &core.Scalar{
+					Value: &core.Scalar_Primitive{
+						Primitive: &core.Primitive{
+							Value: &core.Primitive_Datetime{Datetime: timestamppb.Now()},
+						},
+					},
+				},
+			},
+		}))
+	})
+	t.Run("datetime with value below min", func(t *testing.T) {
+		// given
+		timestamp := timestamppb.Timestamp{Seconds: -62135596801, Nanos: 999999999} // = 0000-12-31T23:59:59.999999999Z
+		expectedErrStr := "before 0001-01-01"
+
+		// when
+		result := ValidateDatetime(&core.Literal{
+			Value: &core.Literal_Scalar{
+				Scalar: &core.Scalar{
+					Value: &core.Scalar_Primitive{
+						Primitive: &core.Primitive{
+							Value: &core.Primitive_Datetime{Datetime: &timestamp},
+						},
+					},
+				},
+			},
+		})
+
+		// then
+		assert.NotNil(t, result)
+		assert.Containsf(t, result.Error(), expectedErrStr, "Found unexpected error message")
+	})
+	t.Run("datetime with value equals Instant.MIN", func(t *testing.T) {
+		// given
+		timestamp := timestamppb.Timestamp{Seconds: -31557014167219200, Nanos: 0} // = -1000000000-01-01T00:00Z
+		expectedErrStr := "timestamp (seconds:-31557014167219200) before 0001-01-01"
+
+		// when
+		result := ValidateDatetime(&core.Literal{
+			Value: &core.Literal_Scalar{
+				Scalar: &core.Scalar{
+					Value: &core.Scalar_Primitive{
+						Primitive: &core.Primitive{
+							Value: &core.Primitive_Datetime{Datetime: &timestamp},
+						},
+					},
+				},
+			},
+		})
+
+		// then
+		assert.NotNil(t, result)
+		assert.Containsf(t, result.Error(), expectedErrStr, "Found unexpected error message")
+	})
+	t.Run("datetime with min valid value", func(t *testing.T) {
+		timestamp := timestamppb.Timestamp{Seconds: -62135596800, Nanos: 0} // = 0001-01-01T00:00:00Z
+
+		assert.NoError(t, ValidateDatetime(&core.Literal{
+			Value: &core.Literal_Scalar{
+				Scalar: &core.Scalar{
+					Value: &core.Scalar_Primitive{
+						Primitive: &core.Primitive{
+							Value: &core.Primitive_Datetime{Datetime: &timestamp},
+						},
+					},
+				},
+			},
+		}))
+	})
+	t.Run("datetime with max valid value", func(t *testing.T) {
+		timestamp := timestamppb.Timestamp{Seconds: 253402300799, Nanos: 999999999} // = 9999-12-31T23:59:59.999999999Z
+
+		assert.NoError(t, ValidateDatetime(&core.Literal{
+			Value: &core.Literal_Scalar{
+				Scalar: &core.Scalar{
+					Value: &core.Scalar_Primitive{
+						Primitive: &core.Primitive{
+							Value: &core.Primitive_Datetime{Datetime: &timestamp},
+						},
+					},
+				},
+			},
+		}))
+	})
+	t.Run("datetime with value above max", func(t *testing.T) {
+		// given
+		timestamp := timestamppb.Timestamp{Seconds: 253402300800, Nanos: 0} // = 0000-12-31T23:59:59.999999999Z
+		expectedErrStr := "timestamp (seconds:253402300800) after 9999-12-31"
+
+		// when
+		result := ValidateDatetime(&core.Literal{
+			Value: &core.Literal_Scalar{
+				Scalar: &core.Scalar{
+					Value: &core.Scalar_Primitive{
+						Primitive: &core.Primitive{
+							Value: &core.Primitive_Datetime{Datetime: &timestamp},
+						},
+					},
+				},
+			},
+		})
+
+		// then
+		assert.NotNil(t, result)
+		assert.Containsf(t, result.Error(), expectedErrStr, "Found unexpected error message")
+	})
+}