refactor: Add strong typing to document creation #2161

fredcarle · 2024-01-02T22:43:43Z

Relevant issue(s)

Resolves #935
Resolves #1703

Description

This PR adds strong document typing to document creation by using the field descriptions to determine Go types. Datetime is now properly supported and formatting is enforced.

Note that a lot of docIDs have changed as a result of this refactor.

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?

make test

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

MacOS

codecov · 2024-01-02T23:04:13Z

Codecov Report

Attention: 77 lines in your changes are missing coverage. Please review.

Comparison is base (9da4dab) 74.35% compared to head (30f2e6a) 74.10%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2161      +/-   ##
===========================================
- Coverage    74.35%   74.10%   -0.25%     
===========================================
  Files          252      252              
  Lines        25217    25242      +25     
===========================================
- Hits         18749    18704      -45     
- Misses        5184     5255      +71     
+ Partials      1284     1283       -1

Flag	Coverage Δ
all-tests	`74.10% <74.33%> (-0.25%)`	⬇️

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

Files	Coverage Δ
client/errors.go	`56.86% <100.00%> (+1.76%)`	⬆️
core/encoding.go	`64.23% <100.00%> (-0.48%)`	⬇️
db/backup.go	`59.64% <100.00%> (+1.07%)`	⬆️
db/collection.go	`69.12% <ø> (-0.83%)`	⬇️
db/collection_get.go	`72.22% <100.00%> (ø)`
db/collection_index.go	`92.66% <100.00%> (ø)`
db/errors.go	`68.15% <ø> (-1.58%)`	⬇️
db/fetcher/encoded_doc.go	`76.14% <100.00%> (-1.28%)`	⬇️
http/client_collection.go	`35.25% <100.00%> (-0.37%)`	⬇️
http/handler_collection.go	`60.35% <100.00%> (ø)`
... and 5 more

... and 16 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 9da4dab...30f2e6a. Read the comment docs.

client/document.go

nasdf

Great work Fred!

client/document.go

AndrewSisley

LGTM - just have a few small questions/todos for you :)

cli/collection_create.go

client/document.go

AndrewSisley · 2024-01-04T15:28:09Z

client/document.go

+	}
+
+	docs := make([]*Document, len(a))
+	for _, v := range a {


suggestion: I would suggest refactoring this block (single doc jsonObj) with NewDocFromJSON so that they both use exactly the same logic. It feels like it would be more maintainable, and it feels strange that they are not doing so.

It's not really practical to do so since here we already have a fastjson.Object and not a []byte

You would probably want to change NewDocFromJSON to create a fastjson.Object and pass that into the shared code.

They already share setWithFastJSONObject found within SetWithJSON. I don't think it would simplify anything.

client/document.go

AndrewSisley · 2024-01-04T15:33:06Z

client/document_test.go

@@ -61,31 +75,31 @@ func TestNewFromJSON(t *testing.T) {
 	assert.Equal(t, doc.fields["Name"].Type(), LWW_REGISTER)
 	assert.Equal(t, doc.fields["Age"].Name(), "Age")
 	assert.Equal(t, doc.fields["Age"].Type(), LWW_REGISTER)
-	assert.Equal(t, doc.fields["Address"].Name(), "Address")
-	assert.Equal(t, doc.fields["Address"].Type(), OBJECT)
+	// assert.Equal(t, doc.fields["Address"].Name(), "Address")


todo: Why are these commented out? Please either document why, or put them back :)

Because in develop it used to create a doc with a sub object which we currently don't support. I should probably just remove the commented code.

Ah I see my bad :) Thanks for removing them

db/collection.go

db/collection_update.go

AndrewSisley · 2024-01-04T15:41:15Z

tests/gen/gen_auto_test.go

@@ -1352,7 +1352,7 @@ func TestAutoGenerate_IfColDefinitionsAreValid_ShouldGenerate(t *testing.T) {
 						Kind: client.FieldKind_STRING,
 					},
 					{
-						Name:   "owner",
+						Name:   "owner_id",


question: This change seems wrong, and I can't spot anything in the PR description. Why has this changed?

internally the relationship is set on owner_id and not owner. For this test to work properly it has to be owner_id.

I am a bit confused about this one too

When we define a schema, in the SDL only owner would be defined but internally we also define owner_id and this is the field that hold the foreign docID. For this test, the code will look specifically for owner_id to set the foreign docID to. It previously worked before but was accidental and probably a bug.

I don't think this is correct though - Kind is client.FieldKind_FOREIGN_OBJECT, but foo_id fields should be of kind INTERNAL_ID IIRC

I fixed it to be representative of what it would be.

{ Name: "owner_id", Kind: client.FieldKind_DocKey, RelationType: client.Relation_Type_INTERNAL_ID, }

tests/integration/mutation/update/field_kinds/one_to_many/with_alias_test.go

tests/integration/query/simple/with_filter/with_gt_datetime_test.go

client/document_test.go

AndrewSisley

LGTM - Thanks a bunch Fred :)

I mentioned this to Shahzad - his rename PR seemed to touch a lot of the lines that this PR does, strongly suggest checking in with him before merging if you guys haven't already.

fredcarle · 2024-01-04T19:32:30Z

LGTM - Thanks a bunch Fred :)

I mentioned this to Shahzad - his rename PR seemed to touch a lot of the lines that this PR does, strongly suggest checking in with him before merging if you guys haven't already.

Thanks Andy! And yes we have discussed it. He will merge first :)

## Relevant issue(s) Resolves sourcenetwork#935 Resolves sourcenetwork#1703 ## Description This PR adds strong document typing to document creation by using the field descriptions to determine Go types. Datetime is now properly supported and formatting is enforced. Note that a lot of docIDs have changed as a result of this refactor.

fredcarle added the refactor This issue specific to or requires *notable* refactoring of existing codebases and components label Jan 2, 2024

fredcarle added this to the DefraDB v0.9 milestone Jan 2, 2024

fredcarle requested a review from a team January 2, 2024 22:43

fredcarle self-assigned this Jan 2, 2024

fredcarle force-pushed the fredcarle/refactor/document-strong-typing branch from 56e6c2f to 709909e Compare January 2, 2024 22:45

AndrewSisley reviewed Jan 3, 2024

View reviewed changes

client/document.go Outdated Show resolved Hide resolved

AndrewSisley reviewed Jan 3, 2024

View reviewed changes

client/document.go Show resolved Hide resolved

AndrewSisley reviewed Jan 3, 2024

View reviewed changes

client/document.go Show resolved Hide resolved

nasdf reviewed Jan 3, 2024

View reviewed changes

client/document.go Show resolved Hide resolved

client/document.go Outdated Show resolved Hide resolved

client/document.go Outdated Show resolved Hide resolved

client/document.go Outdated Show resolved Hide resolved

fredcarle force-pushed the fredcarle/refactor/document-strong-typing branch from 06aee19 to 39b7b30 Compare January 3, 2024 23:27

fredcarle requested review from nasdf, AndrewSisley and a team January 4, 2024 15:02

AndrewSisley requested changes Jan 4, 2024

View reviewed changes

nasdf approved these changes Jan 4, 2024

View reviewed changes

shahzadlone reviewed Jan 4, 2024

View reviewed changes

client/document_test.go Outdated Show resolved Hide resolved

shahzadlone reviewed Jan 4, 2024

View reviewed changes

client/document_test.go Outdated Show resolved Hide resolved

shahzadlone reviewed Jan 4, 2024

View reviewed changes

client/document_test.go Outdated Show resolved Hide resolved

shahzadlone reviewed Jan 4, 2024

View reviewed changes

client/document_test.go Outdated Show resolved Hide resolved

AndrewSisley approved these changes Jan 4, 2024

View reviewed changes

fredcarle added 8 commits January 5, 2024 14:35

add strong typing to document creation

6e54fb4

add breaking change documentation

bbb597a

change http client to use NewDocFromJSON

b4056bf

apply feedback

d37a72d

apply more feedback

8d0b8b4

fix RelatedObjectID suffix assignement on doc.Set

421291a

fix relation validation

9702298

fix asserting expected time result

ab29c84

fredcarle added 6 commits January 5, 2024 14:41

fix getting non nillable array

000055d

clean doc on cli wrapper get

4d72565

remove restrictions caused by 1703

eefd2d5

add doc yo NewDocsFromJSON

9844271

apply feedback

962f4c4

fix field definition

95ee825

fredcarle force-pushed the fredcarle/refactor/document-strong-typing branch from 1b8d13f to 6da093a Compare January 5, 2024 14:09

resolving post merge issues

30f2e6a

fredcarle force-pushed the fredcarle/refactor/document-strong-typing branch from 6da093a to 30f2e6a Compare January 5, 2024 14:49

fredcarle merged commit 0c1c4fe into sourcenetwork:develop Jan 5, 2024
30 of 31 checks passed

fredcarle deleted the fredcarle/refactor/document-strong-typing branch January 5, 2024 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Add strong typing to document creation #2161

refactor: Add strong typing to document creation #2161

fredcarle commented Jan 2, 2024 •

edited

Loading

codecov bot commented Jan 2, 2024 •

edited

Loading

nasdf left a comment

AndrewSisley left a comment

AndrewSisley Jan 4, 2024

fredcarle Jan 4, 2024

AndrewSisley Jan 4, 2024

fredcarle Jan 4, 2024

AndrewSisley Jan 4, 2024

fredcarle Jan 4, 2024

AndrewSisley Jan 4, 2024

AndrewSisley Jan 4, 2024

fredcarle Jan 4, 2024

shahzadlone Jan 4, 2024

fredcarle Jan 4, 2024

AndrewSisley Jan 4, 2024

fredcarle Jan 4, 2024

AndrewSisley left a comment •

edited

Loading

fredcarle commented Jan 4, 2024

refactor: Add strong typing to document creation #2161

refactor: Add strong typing to document creation #2161

Conversation

fredcarle commented Jan 2, 2024 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Jan 2, 2024 • edited Loading

Codecov Report

nasdf left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment • edited Loading

Choose a reason for hiding this comment

fredcarle commented Jan 4, 2024

fredcarle commented Jan 2, 2024 •

edited

Loading

codecov bot commented Jan 2, 2024 •

edited

Loading

AndrewSisley left a comment •

edited

Loading