feat: Default scalar field values #2997

nasdf · 2024-09-11T20:19:30Z

Relevant issue(s)

Resolves #2952

Description

This PR adds support for default field values using a new @default directive.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
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).
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 integration tests

Specify the platform(s) on which this was tested:

MacOS

codecov · 2024-09-11T22:28:14Z

Codecov Report

Attention: Patch coverage is 82.75862% with 20 lines in your changes missing coverage. Please review.

Project coverage is 79.40%. Comparing base (f5567f5) to head (0c3f6d1).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
client/document.go	57.14%	6 Missing and 6 partials ⚠️
internal/request/graphql/schema/collection.go	87.10%	4 Missing ⚠️
http/client_collection.go	33.33%	1 Missing and 1 partial ⚠️
internal/db/fetcher/encoded_doc.go	33.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2997      +/-   ##
===========================================
- Coverage    79.46%   79.40%   -0.06%     
===========================================
  Files          329      329              
  Lines        25133    25225      +92     
===========================================
+ Hits         19971    20029      +58     
- Misses        3750     3772      +22     
- Partials      1412     1424      +12

Flag	Coverage Δ
all-tests	`79.40% <82.76%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
client/collection_field_description.go	`76.47% <100.00%> (+1.47%)`	⬆️
client/definitions.go	`80.12% <100.00%> (+0.25%)`	⬆️
internal/request/graphql/schema/errors.go	`42.86% <100.00%> (+13.03%)`	⬆️
internal/request/graphql/schema/generate.go	`87.97% <100.00%> (+0.02%)`	⬆️
internal/request/graphql/schema/manager.go	`98.43% <100.00%> (+0.01%)`	⬆️
internal/request/graphql/schema/types/types.go	`100.00% <100.00%> (ø)`
http/client_collection.go	`49.32% <33.33%> (-0.45%)`	⬇️
internal/db/fetcher/encoded_doc.go	`78.38% <33.33%> (-2.18%)`	⬇️
internal/request/graphql/schema/collection.go	`88.84% <87.10%> (-0.38%)`	⬇️
client/document.go	`70.39% <57.14%> (-1.43%)`	⬇️

... and 12 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5567f5...0c3f6d1. Read the comment docs.

AndrewSisley

It looks good and is easy to read, but please add some error tests, validation for the default values (in definition_validation.go), View behaviour, and either test PatchCollection works, or temporary block it off (and test that).

AndrewSisley · 2024-09-12T12:49:49Z

internal/request/graphql/schema/collection.go

+) (any, error) {
+	astNamed, ok := field.Type.(*ast.Named)
+	if !ok {
+		return nil, NewErrDefaultValueNotAllowed(field.Name.Value, field.Type.String())


todo: Please add tests for this error condition

AndrewSisley · 2024-09-12T12:49:57Z

internal/request/graphql/schema/collection.go

+	var value any
+	for _, arg := range directive.Arguments {
+		if defaultPropNameToType[arg.Name.Value] != astNamed.Name.Value {
+			return nil, NewErrDefaultValueInvalid(astNamed.Name.Value, arg.Name.Value)


todo: Please add tests for this error condition

AndrewSisley · 2024-09-12T12:52:53Z

client/collection_field_description.go

@@ -38,6 +38,9 @@ type CollectionFieldDescription struct {
 	//
 	// Otherwise will be [None].
 	RelationName immutable.Option[string]
+
+	// DefaultValue contains the default value for this field.
+	DefaultValue any


todo: Please test the PatchCollection behaviour for this new property

AndrewSisley · 2024-09-12T12:54:46Z

client/collection_field_description.go

@@ -38,6 +38,9 @@ type CollectionFieldDescription struct {
 	//
 	// Otherwise will be [None].
 	RelationName immutable.Option[string]
+
+	// DefaultValue contains the default value for this field.
+	DefaultValue any


todo: Please test and document what happens when this is set on a View - and consider adding a validation rule to prevent that (as it makes no sense to me) (and test that, including on Patch).

I've added a test to show the current behavior. Since it has no effect on the results of the view, I think allowing it is better than having the schema definition and view definition languages diverge and have slightly different rules.

Okay cheers, I'm happy with that :) - can you please document that the property is ignored for views on the directive, and on this CollectionFieldDescription prop so that users know?

shahzadlone · 2024-09-12T16:46:57Z

internal/request/graphql/schema/collection.go

-	const (
-		typeID       string = "ID"
-		typeBoolean  string = "Boolean"
-		typeInt      string = "Int"
-		typeFloat    string = "Float"
-		typeDateTime string = "DateTime"
-		typeString   string = "String"
-		typeBlob     string = "Blob"
-		typeJSON     string = "JSON"
-	)
-


praise: Thanks for moving

tests/integration/mutation/create/with_default_values_test.go

shahzadlone

Cheers thanks for adding the requested tests

fredcarle · 2024-09-13T18:34:31Z

tests/integration/mutation/create/with_default_values_test.go

+				`,
+			},
+			testUtils.Request{
+				Request: `mutation {


question: Would it be worth using the CreateDoc action instead to test across all clients?

This applies to all the integration tests.

Yes, please use the CreateDoc action instead Keenan (nice spot Fred)

fredcarle

LGTM. Thanks for all the tests. Lets just resolve my question before merging please.

AndrewSisley · 2024-09-16T14:23:07Z

tests/integration/mutation/create/with_default_values_test.go

+		Description: "Simple create mutation, with default value, no value provided, and created twice",
+		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
+			// This test will fail if using the collection save
+			// method because it does not create two unique docs


question: I can't spot anything super obvious in the test code as to why Save would not work, would you mind expanding a little?

It seems like calling save does not actually create two unique documents when the fields have the same value. I'm guessing it checks if the docID exists and calls update instead of create.

Oh true - it does - thanks Keenan. Might be worth adding that to this code-comment then.

AndrewSisley

LGTM, thanks Keenan :)

add default directive

c2bc030

nasdf added feature New feature or request area/query Related to the query component labels Sep 11, 2024

nasdf self-assigned this Sep 11, 2024

nasdf added this to the DefraDB v0.14 milestone Sep 11, 2024

move default value to collection definition

e7c8663

nasdf changed the title ~~feat: Default field values~~ feat: Default scalar field values Sep 11, 2024

nasdf marked this pull request as ready for review September 11, 2024 22:40

nasdf requested a review from a team September 11, 2024 22:40

make docs

cf9eaee

AndrewSisley requested changes Sep 12, 2024

View reviewed changes

add more tests

9048193

shahzadlone reviewed Sep 12, 2024

View reviewed changes

tests/integration/mutation/create/with_default_values_test.go Outdated Show resolved Hide resolved

add test with unique index and duplicate

c86e686

shahzadlone approved these changes Sep 12, 2024

View reviewed changes

fredcarle reviewed Sep 13, 2024

View reviewed changes

fredcarle approved these changes Sep 13, 2024

View reviewed changes

nasdf added 2 commits September 13, 2024 15:01

add note that default directive has no effect in views

e1792f3

refactor tests to use create doc action

21e6185

nasdf requested a review from AndrewSisley September 13, 2024 22:21

AndrewSisley reviewed Sep 16, 2024

View reviewed changes

AndrewSisley approved these changes Sep 16, 2024

View reviewed changes

nasdf added 3 commits September 16, 2024 09:11

Merge branch 'develop' into nasdf/feat/default-field-value

1221729

add test comment

b3944f3

fix failing unique index test

0c3f6d1

nasdf merged commit ea3a74f into sourcenetwork:develop Sep 16, 2024
40 of 42 checks passed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Default scalar field values #2997

feat: Default scalar field values #2997

nasdf commented Sep 11, 2024

codecov bot commented Sep 11, 2024 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Sep 12, 2024

nasdf Sep 12, 2024

AndrewSisley Sep 12, 2024

nasdf Sep 12, 2024

AndrewSisley Sep 12, 2024

nasdf Sep 12, 2024

AndrewSisley Sep 12, 2024

nasdf Sep 12, 2024

AndrewSisley Sep 12, 2024

nasdf Sep 13, 2024

shahzadlone Sep 12, 2024

shahzadlone left a comment

fredcarle Sep 13, 2024

fredcarle Sep 13, 2024

AndrewSisley Sep 13, 2024

nasdf Sep 13, 2024

fredcarle left a comment

AndrewSisley Sep 16, 2024

nasdf Sep 16, 2024

AndrewSisley Sep 16, 2024

AndrewSisley left a comment

feat: Default scalar field values #2997

feat: Default scalar field values #2997

Conversation

nasdf commented Sep 11, 2024

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 11, 2024 •

edited

Loading