Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Deprecate CollectionDescription.Schema #1939

Merged
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
e337116
Remove collection.IsEmpty
AndrewSisley Sep 27, 2023
a9b989b
Simplify index field retrieval
AndrewSisley Sep 27, 2023
9bc9698
Handle field not found possibility
AndrewSisley Sep 27, 2023
fbe58e5
Pass schema in as a param to GetFieldByID
AndrewSisley Sep 27, 2023
1b0dbbb
Pass in schema to GetFieldByRelation
AndrewSisley Sep 29, 2023
a72e05f
Pass in schema to GetFieldByName
AndrewSisley Oct 2, 2023
1e0d5c2
Remove dead and misplaced collection schema field validation
AndrewSisley Oct 2, 2023
d55c679
Include indexes when creating collection
AndrewSisley Oct 4, 2023
4185bb7
Host schema on collection
AndrewSisley Oct 2, 2023
8f254ac
Remove unhelpful col.schemaID property
AndrewSisley Oct 2, 2023
d90aa53
Remove unhelpful col.colID property
AndrewSisley Oct 2, 2023
c2f96c7
Move refs from c.desc.Schema to c.Schema
AndrewSisley Oct 2, 2023
a74d0ce
Use Collection over Description in fetcher
AndrewSisley Oct 2, 2023
7377b55
Remove unuseful unit test
AndrewSisley Oct 2, 2023
3962eda
Replace sourceInfo with Collection
AndrewSisley Oct 2, 2023
c17b367
Remove tests using newTestCollectionWithSchema
AndrewSisley Oct 2, 2023
62cda9c
Decouple P2P unit tests from private function
AndrewSisley Oct 2, 2023
2cd7d99
Rework index unit tests to avoid private functions
AndrewSisley Oct 2, 2023
7ad61ca
Use Collection over Description in mapper
AndrewSisley Oct 3, 2023
2206d7e
Split Collection interface
AndrewSisley Oct 3, 2023
84a8111
Rework db.addSchema and its children to avoid col.Schema
AndrewSisley Oct 2, 2023
8ca2935
Rework http client to split schema from colDesc
AndrewSisley Oct 4, 2023
36d1b12
Rework db.patchSchema and children to avoid col.Schema
AndrewSisley Oct 2, 2023
511b1bc
Take schema as param to newCollection
AndrewSisley Oct 5, 2023
5617202
Deprecate collectionDescription.Schema
AndrewSisley Oct 6, 2023
cf14555
Make client.CollectionDefinition a struct
AndrewSisley Oct 16, 2023
ae291b3
PR FIXUP - Doc Collection.Definition
AndrewSisley Oct 16, 2023
81d8078
PR FIXUP - Remove col.def redefinition
AndrewSisley Oct 16, 2023
1fc3e0d
PR FIXUP - Return re-fetched col in createCollection
AndrewSisley Oct 16, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cli/collection_describe.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,16 @@ Example: view collection by version id

col, ok := tryGetCollectionContext(cmd)
if ok {
return writeJSON(cmd, col.Description())
return writeJSON(cmd, col.Definition())
}
// if no collection specified list all collections
cols, err := store.GetAllCollections(cmd.Context())
if err != nil {
return err
}
colDesc := make([]client.CollectionDescription, len(cols))
colDesc := make([]client.CollectionDefinition, len(cols))
for i, col := range cols {
colDesc[i] = col.Description()
colDesc[i] = col.Definition()
}
return writeJSON(cmd, colDesc)
},
Expand Down
19 changes: 15 additions & 4 deletions client/collection.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,35 @@ import (
"github.com/sourcenetwork/defradb/datastore"
)

// CollectionDefinition contains the metadata defining what a Collection is.
type CollectionDefinition struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@AndrewSisley AndrewSisley Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably think of something better then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Description returns the CollectionDescription of this Collection.
Description CollectionDescription `json:"description"`
// Schema returns the SchemaDescription used to define this Collection.
Schema SchemaDescription `json:"schema"`
}

// Collection represents a defradb collection.
//
// A Collection is mostly analogous to a SQL table, however a collection is specific to its
// host, and many collections may share the same schema.
//
// Many functions on this object will interact with the underlying datastores.
type Collection interface {
// Description returns the CollectionDescription of this Collection.
Description() CollectionDescription
// Name returns the name of this collection.
Name() string
// Schema returns the SchemaDescription used to define this Collection.
Schema() SchemaDescription
// ID returns the ID of this Collection.
ID() uint32
// SchemaID returns the ID of the Schema used to define this Collection.
SchemaID() string

// Definition contains the metadata defining what a Collection is.
Definition() CollectionDefinition
fredcarle marked this conversation as resolved.
Show resolved Hide resolved
// Schema returns the SchemaDescription used to define this Collection.
Schema() SchemaDescription
// Description returns the CollectionDescription of this Collection.
Description() CollectionDescription

// Create a new document.
//
// Will verify the DocKey/CID to ensure that the new document is correctly formatted.
Expand Down
49 changes: 22 additions & 27 deletions client/descriptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
ID uint32

// Schema contains the data type information that this Collection uses.
//
// This property is deprecated and should not be used.
Schema SchemaDescription

// Indexes contains the secondary indexes that this Collection has.
Expand All @@ -41,12 +43,21 @@

// GetFieldByID searches for a field with the given ID. If such a field is found it
// will return it and true, if it is not found it will return false.
func (col CollectionDescription) GetFieldByID(id FieldID) (FieldDescription, bool) {
if !col.Schema.IsEmpty() {
for _, field := range col.Schema.Fields {
if field.ID == id {
return field, true
}
func (col CollectionDescription) GetFieldByID(id FieldID, schema *SchemaDescription) (FieldDescription, bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying

for _, field := range schema.Fields {
if field.ID == id {
return field, true
}
}
return FieldDescription{}, false

Check warning on line 52 in client/descriptions.go

View check run for this annotation

Codecov / codecov/patch

client/descriptions.go#L52

Added line #L52 was not covered by tests
}

// GetFieldByName returns the field for the given field name. If such a field is found it
// will return it and true, if it is not found it will return false.
func (col CollectionDescription) GetFieldByName(fieldName string, schema *SchemaDescription) (FieldDescription, bool) {
for _, field := range schema.Fields {
if field.Name == fieldName {
return field, true
}
}
return FieldDescription{}, false
Expand All @@ -57,8 +68,9 @@
relationName string,
otherCollectionName string,
otherFieldName string,
schema *SchemaDescription,
) (FieldDescription, bool) {
for _, field := range col.Schema.Fields {
for _, field := range schema.Fields {
if field.RelationName == relationName && !(col.Name == otherCollectionName && otherFieldName == field.Name) {
return field, true
}
Expand Down Expand Up @@ -93,28 +105,11 @@
Fields []FieldDescription
}

// IsEmpty returns true if the SchemaDescription is empty and uninitialized
func (sd SchemaDescription) IsEmpty() bool {
return len(sd.Fields) == 0
}

// GetFieldKey returns the field ID for the given field name.
func (sd SchemaDescription) GetFieldKey(fieldName string) uint32 {
for _, field := range sd.Fields {
if field.Name == fieldName {
return uint32(field.ID)
}
}
return uint32(0)
}

// GetField returns the field of the given name.
func (sd SchemaDescription) GetField(name string) (FieldDescription, bool) {
if !sd.IsEmpty() {
for _, field := range sd.Fields {
if field.Name == name {
return field, true
}
for _, field := range sd.Fields {
if field.Name == name {
return field, true
}
}
return FieldDescription{}, false
Expand Down
4 changes: 2 additions & 2 deletions core/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ type Parser interface {
NewFilterFromString(collectionType string, body string) (immutable.Option[request.Filter], error)

// ParseSDL parses an SDL string into a set of collection descriptions.
ParseSDL(ctx context.Context, schemaString string) ([]client.CollectionDescription, error)
ParseSDL(ctx context.Context, schemaString string) ([]client.CollectionDefinition, error)

// Adds the given schema to this parser's model.
SetSchema(ctx context.Context, txn datastore.Txn, collections []client.CollectionDescription) error
SetSchema(ctx context.Context, txn datastore.Txn, collections []client.CollectionDefinition) error
}
20 changes: 7 additions & 13 deletions db/base/collection_keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

func MakePrimaryIndexKeyForCRDT(
c client.CollectionDescription,
schema client.SchemaDescription,
ctype client.CType,
key core.DataStoreKey,
fieldName string,
Expand All @@ -42,19 +43,12 @@
case client.COMPOSITE:
return MakeCollectionKey(c).WithInstanceInfo(key).WithFieldId(core.COMPOSITE_NAMESPACE), nil
case client.LWW_REGISTER:
fieldKey := getFieldKey(c, key, fieldName)
return MakeCollectionKey(c).WithInstanceInfo(fieldKey), nil
}
return core.DataStoreKey{}, ErrInvalidCrdtType
}
field, ok := c.GetFieldByName(fieldName, &schema)
if !ok {
return core.DataStoreKey{}, client.NewErrFieldNotExist(fieldName)
}

Check warning on line 49 in db/base/collection_keys.go

View check run for this annotation

Codecov / codecov/patch

db/base/collection_keys.go#L48-L49

Added lines #L48 - L49 were not covered by tests

func getFieldKey(
c client.CollectionDescription,
key core.DataStoreKey,
fieldName string,
) core.DataStoreKey {
if !c.Schema.IsEmpty() {
return key.WithFieldId(fmt.Sprint(c.Schema.GetFieldKey(fieldName)))
return MakeCollectionKey(c).WithInstanceInfo(key).WithFieldId(fmt.Sprint(field.ID)), nil
}
return key.WithFieldId(fieldName)
return core.DataStoreKey{}, ErrInvalidCrdtType

Check warning on line 53 in db/base/collection_keys.go

View check run for this annotation

Codecov / codecov/patch

db/base/collection_keys.go#L53

Added line #L53 was not covered by tests
}
Loading