From 885707a9c139eb53ac862029614d3fe8ad8d9a71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luc=C3=ADa=20Pasarin?= <50679871+lupasarin@users.noreply.github.com> Date: Tue, 11 Jan 2022 19:02:23 +0100 Subject: [PATCH] Validate datetime (#309) * Add datetime validation Signed-off-by: Lucia Pasarin * Clean up Signed-off-by: Lucia Pasarin * Adjusted validation + added test Signed-off-by: Lucia Pasarin * Use datetime validation as part of parameter map validation Signed-off-by: Lucia Pasarin * Add test for map validation + fix part of datetime in map validation Signed-off-by: Lucia Pasarin * Fix validation by splitting intermediate steps Signed-off-by: Lucia Pasarin * Clean up Signed-off-by: Lucia Pasarin * Simplify validation since field should only be datetime in this case because it is protobuf generated Signed-off-by: Lucia Pasarin * Remove //go:build integration Signed-off-by: Lucia Pasarin * Fix README goimports + add validation in literal map too Signed-off-by: Lucia Pasarin * Complete test cases Signed-off-by: Lucia Pasarin * Stop using reflection for isDateTime check Signed-off-by: Lucia Pasarin * Use direct access instead of casting + fix string format in test Signed-off-by: Lucia Pasarin * Throw error in case of nil datetime + fix assertion Signed-off-by: Lucia Pasarin * Add one more test case to check for Instant.MIN Signed-off-by: Lucia Pasarin * Fix flaky tests Signed-off-by: Lucia Pasarin * Fix test Signed-off-by: Lucia Pasarin Co-authored-by: Lucia Pasarin --- flyteadmin/README.md | 5 + .../pkg/manager/impl/validation/validation.go | 36 ++- .../impl/validation/validation_test.go | 235 ++++++++++++++---- 3 files changed, 223 insertions(+), 53 deletions(-) diff --git a/flyteadmin/README.md b/flyteadmin/README.md index 036a4600a0..ab65e601c0 100644 --- a/flyteadmin/README.md +++ b/flyteadmin/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/flyteadmin/pkg/manager/impl/validation/validation.go b/flyteadmin/pkg/manager/impl/validation/validation.go index a8f9960170..18e13ec445 100644 --- a/flyteadmin/pkg/manager/impl/validation/validation.go +++ b/flyteadmin/pkg/manager/impl/validation/validation.go @@ -29,7 +29,7 @@ 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) @@ -37,7 +37,7 @@ func ValidateMaxLengthStringField(field string, fieldName string, limit int) err 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/flyteadmin/pkg/manager/impl/validation/validation_test.go b/flyteadmin/pkg/manager/impl/validation/validation_test.go index 0be9c29294..417ad9e5eb 100644 --- a/flyteadmin/pkg/manager/impl/validation/validation_test.go +++ b/flyteadmin/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: ×tamp}, + }, + }, + }, + }, + }) + + // 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: ×tamp}, + }, + }, + }, + }, + }) + + // 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: ×tamp}, + }, + }, + }, + }, + })) + }) + 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: ×tamp}, + }, + }, + }, + }, + })) + }) + 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: ×tamp}, + }, + }, + }, + }, + }) + + // then + assert.NotNil(t, result) + assert.Containsf(t, result.Error(), expectedErrStr, "Found unexpected error message") + }) +}