refactor: Deprecate CollectionDescription.Schema #1939

AndrewSisley · 2023-10-06T21:18:41Z

Relevant issue(s)

Resolves #1955

Description

Deprecate CollectionDescription.Schema. Schema is not a sub-property of collection.

Removes as many references to CollectionDescription.Schema as possible without making any breaking changes. Breaking changes will be made in a later PR. An exception has been made to the http API, which does have a breaking change in this PR - it can be pulled out of here if people want it more obvious in the develop commit history/change log.

This should minimise the conflicts generated by getting the large broad changes sorted out early, instead of being bogged down behind the more complex, breaking, changes coming later.

Commits should be clean, and should bundle up the work into more readable chunks, would recommend reviewing them one by one.

codecov · 2023-10-06T21:36:57Z

Codecov Report

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

Comparison is base (1e255e7) 74.94% compared to head (ae291b3) 74.81%.

❗ Current head ae291b3 differs from pull request most recent head 1fc3e0d. Consider uploading reports for the commit 1fc3e0d to get more accurate results

@@             Coverage Diff             @@
##           develop    #1939      +/-   ##
===========================================
- Coverage    74.94%   74.81%   -0.13%     
===========================================
  Files          241      240       -1     
  Lines        23616    23645      +29     
===========================================
- Hits         17699    17689      -10     
- Misses        4715     4751      +36     
- Partials      1202     1205       +3

Flag	Coverage Δ
all-tests	`74.81% <91.27%> (-0.13%)`	⬇️

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

Files	Coverage Δ
cli/collection_describe.go	`91.43% <100.00%> (ø)`
db/collection_get.go	`72.22% <100.00%> (-0.51%)`	⬇️
db/collection_index.go	`97.25% <100.00%> (+0.01%)`	⬆️
db/collection_update.go	`73.30% <100.00%> (+0.14%)`	⬆️
db/errors.go	`78.23% <ø> (-2.21%)`	⬇️
db/fetcher/fetcher.go	`77.17% <100.00%> (+0.63%)`	⬆️
db/fetcher/indexer.go	`78.89% <100.00%> (ø)`
db/index.go	`97.20% <100.00%> (-0.05%)`	⬇️
lens/fetcher.go	`66.51% <100.00%> (ø)`
net/process.go	`88.24% <100.00%> (ø)`
... and 19 more

... and 8 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 1e255e7...1fc3e0d. Read the comment docs.

islamaliev · 2023-10-13T16:44:38Z

cli/collection_describe.go

@@ -16,6 +16,11 @@ import (
 	"github.com/sourcenetwork/defradb/client"
 )

+type CollectionDefinition struct {


question: does it need to be a public definition of cli package?

Seems like it doesn't belong here

It is the public output type of the command. And this is the only place that outputs it.

Where were you thinking it should be?

The struct doesn't need to be public. Only its fields.

The struct doesn't need to be public. Only its fields.

That is technically correct, but, IMO conceptually incorrect. This is a public return type, it is just that it is returned as json - declaring it as private would be misleading.

It's specific to this CLI command and not used anywhere else. I really think this struct shouldn't be public. If you absolutely want it to be public, please at some documentation to the type.

It is returned from a public endpoint but that public endpoint isn't a public Go function. That endpoint won't show up in the godocs and the function itself is returning an error. The type/shape should be documented elsewhere but not with a public Go type.

My preference would be to define it in the client package and reference it in the cli and http. That would also get rid of the duplicate definition in the http client.

I would be happy with that option :)

Nice okay - I'll try and figure something out. Some other stuff might get re-jigged slightly as we already have a CollectionDefinition (interface) in client, but hopefully everything will be the better for it.

Bring http and cli CollectionDefinitions into client

sorted, client.CollectionDefinition is now a struct

fredcarle · 2023-10-13T18:01:44Z

client/descriptions.go

-			if field.ID == id {
-				return field, true
-			}
+func (col CollectionDescription) GetFieldByID(id FieldID, schema *SchemaDescription) (FieldDescription, bool) {


question: This and the couple other methods bellow no longer need the CollectionDescription. Do you intend on turning those into methods of SchemaDescription in the future?

No, this and the others remain here as whilst they currently only access stuff on the schema, that is because the stuff they access is incorrectly on the schema.

For example, in this case we are getting fields by their ID, FieldID is a local concept that should exist only on collection, not on schema.

The function is located in the correct place, but for historical reasons the data structures are incorrect and need to be changed along with the implementation.

Thanks for clarifying

fredcarle · 2023-10-13T19:31:22Z

db/collection.go

 	existingDescriptionsByName map[string]client.CollectionDescription,
-	proposedDescriptionsByName map[string]client.CollectionDescription,
+	existingSchemaByName map[string]client.SchemaDescription,
+	proposedDescriptionsByName map[string]client.SchemaDescription,


thought(out of scope): I would like to point out that this in a really confusing set of params. We are updateing one collection but we are passing all collection, all schemas and all proposed schemas. In my opinion, only what is directly relevant should be passed to this function.

This one is fiddly, and I agree that it is a bit of an eyesore. However they are all used and all need to be there (or within this func, we could probably (re)fetch them) due to relational fields.

It does get improved slightly in a later PR when we are able to change all their types to SchemaDescriptions (allowing us to only pass in two maps instead of 3). Might be worth re-visiting this in #1965

I've added a todo in that PR, as one of the params still makes no sense there: https://github.com/sourcenetwork/defradb/pull/1965/files#r1358767728

Removing it leaves us with:

func (db *db) updateCollection( ctx context.Context, txn datastore.Txn, existingSchemaByName map[string]client.SchemaDescription, proposedDescriptionsByName map[string]client.SchemaDescription, schema client.SchemaDescription, setAsDefaultVersion bool, ) (client.Collection, error) {

existingSchemaByName could be re-fetched inside updateCollection, but that feels fairly wasteful atm (although a planned repository mechanic would remove that issue).

They are used but in a wasteful way as we only care about one in each of the maps. The only proposedDescription that we care about is already passed as schema so that one can be removed completely. It should probably look like this:

func (db *db) updateCollection( ctx context.Context, txn datastore.Txn, existingSchema client.SchemaDescription, schema client.SchemaDescription, desc client.CollectionDescription setAsDefaultVersion bool, ) (client.Collection, error) {

Although the checks on the schema should probably be done before calling updateCollection and we would end up with just:

func (db *db) updateCollection( ctx context.Context, txn datastore.Txn, schema client.SchemaDescription, desc client.CollectionDescription setAsDefaultVersion bool, // if we call updateCollection, this should be implicit and removed from params. ) (client.Collection, error) {

The only proposedDescription that we care about is already passed as schema so that one can be removed completely

This is not true, we must validate that all relational fields point to a valid schema. That schema is rarely the one being updated.

Although the checks on the schema should probably be done before calling updateCollection

I'me very much against that, as it moves the validation out of a request-language agnostic part of the codebase (db.collection.go) into a request-language specific area (patchSchema) of the codebase. It would mean that if we add an alterative to patchSchema (including a typed embedded Go call) we'd have to rework the validation.

Maybe the problem then is that the name of the function is updateCollection but it handles both schema and collection in the same function. Maybe once we get further in cleaning this up it won't be a problem anymore.

for-reference: We spoke about this last Friday, and there is a task on a later PR to rename this function (when it stops being updateCollection)

fredcarle · 2023-10-13T21:52:20Z

request/graphql/schema/collection.go

@@ -24,9 +24,32 @@ import (
 	"github.com/graphql-go/graphql/language/source"
 )

+type collectionDefinition struct {


thought: You'll be able to use the one in the client package after you move it there instead of redefining it here.

islamaliev · 2023-10-16T10:15:37Z

db/collection_test.go

question: you removed so many tests. Are they already covered by some other tests?

Yes, our integration tests cover all of this, in significantly more depth.

islamaliev · 2023-10-16T10:18:50Z

db/index_test.go

@@ -324,28 +304,6 @@ func TestCreateIndex_IfFieldHasNoDirection_DefaultToAsc(t *testing.T) {
 	assert.Equal(t, client.Ascending, newDesc.Fields[0].Direction)
 }

-func TestCreateIndex_IfNameIsNotSpecified_Generate(t *testing.T) {


question: Why did you remove this test?

This is documented in the commit message:

One test was removed, TestCreateIndex_IfNameIsNotSpecified_Generate - this is because it only pretends to test what it tests - the name is set later by the unit test framework, and as such is identical to other tests here.

islamaliev · 2023-10-16T10:22:45Z

http/handler_store.go

@@ -22,6 +22,11 @@ import (

 type storeHandler struct{}

+type CollectionDefinition struct {


note: whatever solution is for identical struct in cli/collection_describe.go that we discussed should also apply here I believe

It adds no value in any location, and will always return false anyway. Function likely originates from a time where schemaless collections were still being considered a possibility.

Unrelated to main body of work, but might as well fix it here.

Note: The only place that calls this only wants the field name, which long term will be available on the CollectionFieldDescription anyway. I hope this param can be removed again soon. Additionally all properties of Schema used in that calling location will soon be on Collection itself, so the reference to Schema there is hopefully a very temporary thing too.

This will be temporary, as the properties required by this function will medium-long term on the Collection field set, not the schema.

This was validating stuff on the schema. This stuff has already been validated on the schema before reaching this code block.

It should no longer be accessed via [collection] desc

Just get it from col.Schema instead of having to maintain the same prop in multiple places - this caused a bug briefly.

Just get it from col.Schema instead of having to maintain the same prop in multiple places.

The private funtion is not the one under test, and so we should not be forced to change the P2P tests every time we wish to change this private function. May be a historical artifact.

The main change in this commit is the removal of the call to `db.createCollection`, which is a private function used as a utility _only_ in these tests, it is not a function under test, but a means to an end. Using it coupled the index tests to the function, whose only reason-to-be is from within the `db.addSchema` (GQL SDL stuff), this coupling is unwanted and needs to be removed in order to change the behaviour/signature of that private function in a later commit. The unit tests now use the public `db.AddSchema` and `db.GetCollectionByName` functions. One test was removed, TestCreateIndex_IfNameIsNotSpecified_Generate - this is because it only pretends to test what it tests - the name _is_ set later by the unit test framework, and as such is identical to other tests here.

Similar to the changes to the fetcher(s), this commit replaces CollectionDescription objects with Collections. It also removes the DescriptionsRepo, replacing calls to it with calls to `store.GetCollectionByName`. At some point the repo was caching descriptions to avoid fetching them from storage multiple times, but this has since been removed, making the type fairly pointless.

Splits the collection definition/metadata out from the interface that declares what a Collection can _do_. This allows the trimmed down metadata to be passed around to code-locations which should not have access to database operations, this will be used in later commit(s).

Uses the new(er) client.CollectionDefinition interface to minimize the number of references to collectionDescription.Schema when creating new collections/schema from a GQL SDL. I do not believe the gql parser code should have access to the database, so CollectionDefinition is passed around instead of Collection. Some more work will need to be made to this function and its children as the splitting of collection and schema description is completed, however this should get us a lot of the way there without changing public behaviour/signatures.

Previously the return values would include schema within collectionDescription. This commit makes them siblings. This is a public behaviour change (I think the only one in the PR).

This function will be broken up further outside of this PR, the remaining changes will be breaking and will change both storage and the public interfaces.

fredcarle · 2023-10-16T18:34:40Z

client/collection.go

@@ -16,24 +16,34 @@ import (
 	"github.com/sourcenetwork/defradb/datastore"
 )

+// CollectionDefinition contains the metadata defining what a Collection is.
+type CollectionDefinition struct {


thought: I'm a little worried that the name is so close to CollectionDescription that it will be a source of confusion. What do you think of CollectionInfo?

I dislike CollectionInfo as it suffers from being a little ambiguous - could be read as containing info about what data is in the collection too (e.g. doc count), CollectionDefinition suffers less from that.

We should probably think of something better then.

We should probably think of something better then.

I tried, and CollectionDefinition was the best I could come up with without spending an unreasonable amount of time on it. I'm fairly happy with it given the problem.

Just noting here for future reference:

I feel like client.SchemaDescription should just be client.Schema and client.CollectionDescription should be client.Collection. The interface should have a different name. Probably something like client.CollectionAPI or client.CollectionHandler.

This way, client.CollectionDefinition would work.

client/collection.go

islamaliev

LGTM. Nice work Andy!

fredcarle · 2023-10-16T19:02:35Z

db/collection.go

+	schema := def.Schema
+	desc := def.Description


question: Why reallocate when you've already passed a copy of the definition? Using def directly would be clearer and you could assign to col.def directly from it.

It is a struct, and its children are structs, mutating them does not mutate the one on the parent, the parent must instead later be overwritten with a new inner.

// foo.bar.foobar is false foo.bar.foobar = true // foo.bar.foobar is still false, a new (and unreachable) bar instance has value true

I don't think that's true: https://go.dev/play/p/FHce-ajgFyr

😆 Maybe not, but something was very broken before I made that change. It is very deliberate and it doesnt work without it. Could be something similar in the more complex real-world code.

I just tested it and all tests are passing. Not sure what would have been broken.

I cannot see what you have changed, most likely it was schema that was important here, as that is what is being mutated most.

UpdateCollection already does this, it is safer, and we dont care about the performance cost here.

fredcarle

LGTM! Thanks Andy!

## Relevant issue(s) Resolves sourcenetwork#1955 ## Description Deprecate CollectionDescription.Schema. Schema is not a sub-property of collection. Removes as many references to CollectionDescription.Schema as possible without making any breaking changes. Breaking changes will be made in a later PR. An exception has been made to the http API, which does have a breaking change in this PR.

AndrewSisley added area/collections Related to the collections system refactor This issue specific to or requires *notable* refactoring of existing codebases and components labels Oct 6, 2023

AndrewSisley added this to the DefraDB v0.8 milestone Oct 6, 2023

AndrewSisley requested a review from a team October 6, 2023 21:18

AndrewSisley self-assigned this Oct 6, 2023

AndrewSisley force-pushed the 1913-rm-schema-from-col branch from d5a02d4 to f578d13 Compare October 6, 2023 21:19

AndrewSisley added a commit to AndrewSisley/defradb that referenced this pull request Oct 10, 2023

SQUASHED - sourcenetwork#1939

d4c61b8

AndrewSisley mentioned this pull request Oct 10, 2023

feat: Remove collection from patch schema #1957

Merged

islamaliev reviewed Oct 13, 2023

View reviewed changes

AndrewSisley force-pushed the 1913-rm-schema-from-col branch from f578d13 to 965a7e4 Compare October 13, 2023 17:37

fredcarle reviewed Oct 13, 2023

View reviewed changes

AndrewSisley mentioned this pull request Oct 13, 2023

feat: Remove CollectionDescription.Schema #1965

Merged

fredcarle reviewed Oct 13, 2023

View reviewed changes

islamaliev reviewed Oct 16, 2023

View reviewed changes

AndrewSisley added 12 commits October 16, 2023 11:06

Remove collection.IsEmpty

e337116

It adds no value in any location, and will always return false anyway. Function likely originates from a time where schemaless collections were still being considered a possibility.

Simplify index field retrieval

a9b989b

Handle field not found possibility

9bc9698

Unrelated to main body of work, but might as well fix it here.

Pass in schema to GetFieldByRelation

1b0dbbb

This will be temporary, as the properties required by this function will medium-long term on the Collection field set, not the schema.

Pass in schema to GetFieldByName

a72e05f

Remove dead and misplaced collection schema field validation

1e0d5c2

This was validating stuff on the schema. This stuff has already been validated on the schema before reaching this code block.

Include indexes when creating collection

d55c679

Host schema on collection

4185bb7

It should no longer be accessed via [collection] desc

Remove unhelpful col.schemaID property

8f254ac

Just get it from col.Schema instead of having to maintain the same prop in multiple places - this caused a bug briefly.

Remove unhelpful col.colID property

d90aa53

Just get it from col.Schema instead of having to maintain the same prop in multiple places.

Move refs from c.desc.Schema to c.Schema

c2f96c7

AndrewSisley added 9 commits October 16, 2023 11:34

Decouple P2P unit tests from private function

62cda9c

The private funtion is not the one under test, and so we should not be forced to change the P2P tests every time we wish to change this private function. May be a historical artifact.

Rework http client to split schema from colDesc

8ca2935

Previously the return values would include schema within collectionDescription. This commit makes them siblings. This is a public behaviour change (I think the only one in the PR).

Rework db.patchSchema and children to avoid col.Schema

36d1b12

This function will be broken up further outside of this PR, the remaining changes will be breaking and will change both storage and the public interfaces.

Take schema as param to newCollection

511b1bc

Deprecate collectionDescription.Schema

5617202

AndrewSisley force-pushed the 1913-rm-schema-from-col branch 3 times, most recently from 09f67a4 to 95edff0 Compare October 16, 2023 16:54

Make client.CollectionDefinition a struct

cf14555

AndrewSisley force-pushed the 1913-rm-schema-from-col branch from 95edff0 to cf14555 Compare October 16, 2023 16:55

AndrewSisley requested review from fredcarle, islamaliev and nasdf October 16, 2023 16:56

fredcarle reviewed Oct 16, 2023

View reviewed changes

client/collection.go Show resolved Hide resolved

PR FIXUP - Doc Collection.Definition

ae291b3

AndrewSisley requested a review from fredcarle October 16, 2023 18:39

islamaliev approved these changes Oct 16, 2023

View reviewed changes

fredcarle reviewed Oct 16, 2023

View reviewed changes

shahzadlone approved these changes Oct 16, 2023

View reviewed changes

AndrewSisley added 2 commits October 16, 2023 16:31

PR FIXUP - Remove col.def redefinition

81d8078

PR FIXUP - Return re-fetched col in createCollection

1fc3e0d

UpdateCollection already does this, it is safer, and we dont care about the performance cost here.

fredcarle approved these changes Oct 16, 2023

View reviewed changes

AndrewSisley merged commit 5c1b21e into sourcenetwork:develop Oct 16, 2023
26 checks passed

AndrewSisley deleted the 1913-rm-schema-from-col branch October 16, 2023 20:55

		@@ -22,6 +22,11 @@ import (

		type storeHandler struct{}

		type CollectionDefinition struct {

refactor: Deprecate CollectionDescription.Schema #1939

refactor: Deprecate CollectionDescription.Schema #1939

Conversation

AndrewSisley commented Oct 6, 2023 • edited Loading

Relevant issue(s)

Description

codecov bot commented Oct 6, 2023 • edited Loading

Codecov Report

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 Oct 13, 2023 • edited Loading

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 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

fredcarle Oct 13, 2023 • edited Loading

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 Oct 16, 2023 • edited Loading

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 Oct 16, 2023 • edited Loading

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

islamaliev 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

fredcarle left a comment

Choose a reason for hiding this comment

AndrewSisley commented Oct 6, 2023 •

edited

Loading

codecov bot commented Oct 6, 2023 •

edited

Loading

AndrewSisley Oct 13, 2023 •

edited

Loading

AndrewSisley Oct 13, 2023 •

edited

Loading

fredcarle Oct 13, 2023 •

edited

Loading

AndrewSisley Oct 16, 2023 •

edited

Loading

AndrewSisley Oct 16, 2023 •

edited

Loading