Skip to content

Commit

Permalink
fix(i): Default field value validation (#3137)
Browse files Browse the repository at this point in the history
## Relevant issue(s)

Resolves #3133

## Description

This PR fixes a bug where default field value types were not properly
validated and would cause a panic when creating a new document.

## Tasks

- [x] I made sure the code is well commented, particularly
hard-to-understand areas.
- [x] I made sure the repository-held documentation is changed
accordingly.
- [x] I made sure the pull request title adheres to the conventional
commit style (the subset used in the project can be found in
[tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)).
- [x] I made sure to discuss its limitations such as threats to
validity, vulnerability to mistake and misuse, robustness to
invalidation of assumptions, resource requirements, ...

## How has this been tested?

Added an integration test.

Specify the platform(s) on which this was tested:
- *(modify the list accordingly*)
- Arch Linux
- Debian Linux
- MacOS
- Windows
  • Loading branch information
nasdf authored Oct 15, 2024
1 parent 4990ff5 commit 6cc39a7
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 20 deletions.
3 changes: 3 additions & 0 deletions docs/data_format_changes/i3137-default-value-fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Default Value Fix

Default value parsing has changed slightly and causes the change detector to fail.
18 changes: 18 additions & 0 deletions internal/db/definition_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ var globalValidators = []definitionValidator{
validateSelfReferences,
validateCollectionMaterialized,
validateMaterializedHasNoPolicy,
validateCollectionFieldDefaultValue,
}

var createValidators = append(
Expand Down Expand Up @@ -1018,3 +1019,20 @@ func validateMaterializedHasNoPolicy(

return nil
}

func validateCollectionFieldDefaultValue(
ctx context.Context,
db *db,
newState *definitionState,
oldState *definitionState,
) error {
for name, col := range newState.definitionsByName {
// default values are set when a doc is first created
_, err := client.NewDocFromMap(map[string]any{}, col)
if err != nil {
return NewErrDefaultFieldValueInvalid(name, err)
}
}

return nil
}
9 changes: 9 additions & 0 deletions internal/db/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ const (
errSelfReferenceWithoutSelf string = "must specify 'Self' kind for self referencing relations"
errColNotMaterialized string = "non-materialized collections are not supported"
errMaterializedViewAndACPNotSupported string = "materialized views do not support ACP"
errInvalidDefaultFieldValue string = "default field value is invalid"
)

var (
Expand Down Expand Up @@ -681,3 +682,11 @@ func NewErrMaterializedViewAndACPNotSupported(collection string) error {
errors.NewKV("Collection", collection),
)
}

func NewErrDefaultFieldValueInvalid(collection string, inner error) error {
return errors.New(
errInvalidDefaultFieldValue,
errors.NewKV("Collection", collection),
errors.NewKV("Inner", inner),
)
}
44 changes: 28 additions & 16 deletions internal/request/graphql/schema/collection.go
Original file line number Diff line number Diff line change
Expand Up @@ -427,23 +427,35 @@ func defaultFromAST(
if !ok {
return nil, NewErrDefaultValueNotAllowed(field.Name.Value, astNamed.Name.Value)
}
if len(directive.Arguments) != 1 {
return nil, NewErrDefaultValueOneArg(field.Name.Value)
}
arg := directive.Arguments[0]
if propName != arg.Name.Value {
return nil, NewErrDefaultValueType(field.Name.Value, propName, arg.Name.Value)
}
var value any
for _, arg := range directive.Arguments {
if propName != arg.Name.Value {
return nil, NewErrDefaultValueInvalid(field.Name.Value, propName, arg.Name.Value)
}
switch t := arg.Value.(type) {
case *ast.IntValue:
value = gql.Int.ParseLiteral(arg.Value, nil)
case *ast.FloatValue:
value = gql.Float.ParseLiteral(arg.Value, nil)
case *ast.BooleanValue:
value = t.Value
case *ast.StringValue:
value = t.Value
default:
value = arg.Value.GetValue()
}
switch propName {
case types.DefaultDirectivePropInt:
value = gql.Int.ParseLiteral(arg.Value, nil)
case types.DefaultDirectivePropFloat:
value = gql.Float.ParseLiteral(arg.Value, nil)
case types.DefaultDirectivePropBool:
value = gql.Boolean.ParseLiteral(arg.Value, nil)
case types.DefaultDirectivePropString:
value = gql.String.ParseLiteral(arg.Value, nil)
case types.DefaultDirectivePropDateTime:
value = gql.DateTime.ParseLiteral(arg.Value, nil)
case types.DefaultDirectivePropJSON:
value = types.JSONScalarType().ParseLiteral(arg.Value, nil)
case types.DefaultDirectivePropBlob:
value = types.BlobScalarType().ParseLiteral(arg.Value, nil)
}
// If the value is nil, then parsing has failed, or a nil value was provided.
// Since setting a default value to nil is the same as not providing one,
// it is safer to return an error to let the user know something is wrong.
if value == nil {
return nil, NewErrDefaultValueInvalid(field.Name.Value, propName)
}
return value, nil
}
Expand Down
21 changes: 19 additions & 2 deletions internal/request/graphql/schema/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ const (
errPolicyUnknownArgument string = "policy with unknown argument"
errPolicyInvalidIDProp string = "policy directive with invalid id property"
errPolicyInvalidResourceProp string = "policy directive with invalid resource property"
errDefaultValueInvalid string = "default value type must match field type"
errDefaultValueType string = "default value type must match field type"
errDefaultValueNotAllowed string = "default value is not allowed for this field type"
errDefaultValueInvalid string = "default value is invalid"
errDefaultValueOneArg string = "default value must specify one argument"
errFieldTypeNotSpecified string = "field type not specified"
)

Expand Down Expand Up @@ -141,9 +143,24 @@ func NewErrRelationNotFound(relationName string) error {
)
}

func NewErrDefaultValueInvalid(name string, expected string, actual string) error {
func NewErrDefaultValueOneArg(field string) error {
return errors.New(
errDefaultValueOneArg,
errors.NewKV("Field", field),
)
}

func NewErrDefaultValueInvalid(field string, arg string) error {
return errors.New(
errDefaultValueInvalid,
errors.NewKV("Field", field),
errors.NewKV("Arg", arg),
)
}

func NewErrDefaultValueType(name string, expected string, actual string) error {
return errors.New(
errDefaultValueType,
errors.NewKV("Name", name),
errors.NewKV("Expected", expected),
errors.NewKV("Actual", actual),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func TestCollectionDescription_WithDefaultFieldValues(t *testing.T) {
{
ID: 3,
Name: "created",
DefaultValue: "2000-07-23T03:00:00-00:00",
DefaultValue: "2000-07-23T03:00:00Z",
},
{
ID: 4,
Expand Down Expand Up @@ -90,6 +90,23 @@ func TestCollectionDescription_WithDefaultFieldValues(t *testing.T) {
testUtils.ExecuteTestCase(t, test)
}

func TestCollectionDescription_WithInvalidDefaultFieldValueType_ReturnsError(t *testing.T) {
test := testUtils.TestCase{
Actions: []any{
testUtils.SchemaUpdate{
Schema: `
type Users {
active: Boolean @default(bool: invalid)
}
`,
ExpectedError: "default value is invalid. Field: active, Arg: bool",
},
},
}

testUtils.ExecuteTestCase(t, test)
}

func TestCollectionDescription_WithIncorrectDefaultFieldValueType_ReturnsError(t *testing.T) {
test := testUtils.TestCase{
Actions: []any{
Expand All @@ -116,7 +133,7 @@ func TestCollectionDescription_WithMultipleDefaultFieldValueTypes_ReturnsError(t
name: String @default(string: "Bob", int: 10, bool: true, float: 10)
}
`,
ExpectedError: "default value type must match field type. Name: name, Expected: string, Actual: int",
ExpectedError: "default value must specify one argument. Field: name",
},
},
}
Expand Down

0 comments on commit 6cc39a7

Please sign in to comment.