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.

Sign up for GitHub

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 29 commits into from
Oct 16, 2023
Merged

refactor: Deprecate CollectionDescription.Schema #1939

Show file tree
Hide file tree
Changes from 1 commit
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
17 changes: 3 additions & 14 deletions cli/collection_describe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@ import (
"github.com/sourcenetwork/defradb/client"
)

type CollectionDefinition struct {
Description client.CollectionDescription `json:"description"`
Schema client.SchemaDescription `json:"schema"`
}

func MakeCollectionDescribeCommand() *cobra.Command {
var cmd = &cobra.Command{
Use: "describe",
Expand All @@ -44,22 +39,16 @@ Example: view collection by version id

col, ok := tryGetCollectionContext(cmd)
if ok {
return writeJSON(cmd, CollectionDefinition{
Description: col.Description(),
Schema: col.Schema(),
})
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([]CollectionDefinition, len(cols))
colDesc := make([]client.CollectionDefinition, len(cols))
for i, col := range cols {
colDesc[i] = CollectionDefinition{
Description: col.Description(),
Schema: col.Schema(),
}
colDesc[i] = col.Definition()
}
return writeJSON(cmd, colDesc)
},
Expand Down
27 changes: 16 additions & 11 deletions client/collection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,12 @@ import (
"github.com/sourcenetwork/defradb/datastore"
)

// CollectionDefinition contains the metadata defining what this Collection is.
type CollectionDefinition interface {
// 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 •
edited
Loading

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
// Name returns the name of this collection.
Name() string
Description CollectionDescription `json:"description"`
// 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
Schema SchemaDescription `json:"schema"`
}

// Collection represents a defradb collection.
Expand All @@ -37,7 +31,18 @@ type CollectionDefinition interface {
//
// Many functions on this object will interact with the underlying datastores.
type Collection interface {
CollectionDefinition
// Name returns the name of this collection.
Name() string
// ID returns the ID of this Collection.
ID() uint32
// SchemaID returns the ID of the Schema used to define this Collection.
SchemaID() string

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.
//
Expand Down
67 changes: 35 additions & 32 deletions db/collection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ type collection struct {
// of the operation in question.
txn immutable.Option[datastore.Txn]

desc client.CollectionDescription
schema client.SchemaDescription
def client.CollectionDefinition

indexes []CollectionIndex
fetcherFactory func() fetcher.Fetcher
Expand All @@ -70,14 +69,8 @@ type collection struct {
// NewCollection returns a pointer to a newly instanciated DB Collection
func (db *db) newCollection(desc client.CollectionDescription, schema client.SchemaDescription) (*collection, error) {
return &collection{
db: db,
desc: client.CollectionDescription{
ID: desc.ID,
Name: desc.Name,
Schema: schema,
Indexes: desc.Indexes,
},
schema: schema,
db: db,
def: client.CollectionDefinition{Description: desc, Schema: schema},
}, nil
}

Expand All @@ -101,9 +94,11 @@ func (c *collection) newFetcher() fetcher.Fetcher {
func (db *db) createCollection(
ctx context.Context,
txn datastore.Txn,
desc client.CollectionDescription,
schema client.SchemaDescription,
def client.CollectionDefinition,
) (client.Collection, error) {
schema := def.Schema
desc := def.Description
Comment on lines +99 to +100
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😆 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// check if collection by this name exists
collectionKey := core.NewCollectionKey(desc.Name)
exists, err := txn.Systemstore().Has(ctx, collectionKey.ToDS())
Expand Down Expand Up @@ -152,11 +147,11 @@ func (db *db) createCollection(

schema.VersionID = schemaVersionID
schema.SchemaID = schemaID
col.schema = schema
col.desc.Schema = schema
desc.Schema = schema
col.def = client.CollectionDefinition{Description: desc, Schema: schema}

// buffer must include all the ids, as it is saved and loaded from the store later.
buf, err := json.Marshal(col.desc)
buf, err := json.Marshal(desc)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -192,6 +187,7 @@ func (db *db) createCollection(
return nil, err
}
}

return col, nil
}

Expand All @@ -209,10 +205,12 @@ func (db *db) updateCollection(
existingDescriptionsByName map[string]client.CollectionDescription,
existingSchemaByName map[string]client.SchemaDescription,
proposedDescriptionsByName map[string]client.SchemaDescription,
desc client.CollectionDescription,
schema client.SchemaDescription,
def client.CollectionDefinition,
setAsDefaultVersion bool,
) (client.Collection, error) {
schema := def.Schema
desc := def.Description

hasChanged, err := db.validateUpdateCollection(ctx, existingDescriptionsByName, desc)
if err != nil {
return nil, err
Expand All @@ -223,7 +221,7 @@ func (db *db) updateCollection(
txn,
existingSchemaByName,
proposedDescriptionsByName,
desc.Schema,
schema,
)
if err != nil {
return nil, err
Expand Down Expand Up @@ -621,7 +619,7 @@ func (db *db) setDefaultSchemaVersion(

definitions := make([]client.CollectionDefinition, len(cols))
for i, col := range cols {
definitions[i] = col
definitions[i] = col.Definition()
}

return db.parser.SetSchema(ctx, txn, definitions)
Expand Down Expand Up @@ -669,9 +667,11 @@ func (db *db) getCollectionByVersionID(
}

col := &collection{
db: db,
desc: desc,
schema: desc.Schema,
db: db,
def: client.CollectionDefinition{
Description: desc,
Schema: desc.Schema,
},
}

err = col.loadIndexes(ctx, txn)
Expand Down Expand Up @@ -824,36 +824,39 @@ func (c *collection) getAllDocKeysChan(

// Description returns the client.CollectionDescription.
func (c *collection) Description() client.CollectionDescription {
return c.desc
return c.Definition().Description
}

// Name returns the collection name.
func (c *collection) Name() string {
return c.desc.Name
return c.Description().Name
}

// Schema returns the Schema of the collection.
func (c *collection) Schema() client.SchemaDescription {
return c.schema
return c.Definition().Schema
}

// ID returns the ID of the collection.
func (c *collection) ID() uint32 {
return c.desc.ID
return c.Description().ID
}

func (c *collection) SchemaID() string {
return c.Schema().SchemaID
}

func (c *collection) Definition() client.CollectionDefinition {
return c.def
}

// WithTxn returns a new instance of the collection, with a transaction
// handle instead of a raw DB handle.
func (c *collection) WithTxn(txn datastore.Txn) client.Collection {
return &collection{
db: c.db,
txn: immutable.Some(txn),
desc: c.desc,
schema: c.schema,
def: c.def,
indexes: c.indexes,
fetcherFactory: c.fetcherFactory,
}
Expand Down Expand Up @@ -911,7 +914,7 @@ func (c *collection) getKeysFromDoc(

func (c *collection) create(ctx context.Context, txn datastore.Txn, doc *client.Document) error {
// This has to be done before dockey verification happens in the next step.
if err := doc.RemapAliasFieldsAndDockey(c.schema.Fields); err != nil {
if err := doc.RemapAliasFieldsAndDockey(c.Schema().Fields); err != nil {
return err
}

Expand Down Expand Up @@ -1067,7 +1070,7 @@ func (c *collection) save(
return cid.Undef, client.NewErrFieldNotExist(k)
}

fieldDescription, valid := c.schema.GetField(k)
fieldDescription, valid := c.Schema().GetField(k)
if !valid {
return cid.Undef, client.NewErrFieldNotExist(k)
}
Expand Down Expand Up @@ -1169,7 +1172,7 @@ func (c *collection) validateOneToOneLinkDoesntAlreadyExist(
return nil
}

objFieldDescription, ok := c.schema.GetField(strings.TrimSuffix(fieldDescription.Name, request.RelatedObjectID))
objFieldDescription, ok := c.Schema().GetField(strings.TrimSuffix(fieldDescription.Name, request.RelatedObjectID))
if !ok {
return client.NewErrFieldNotExist(strings.TrimSuffix(fieldDescription.Name, request.RelatedObjectID))
}
Expand Down Expand Up @@ -1469,7 +1472,7 @@ func (c *collection) tryGetFieldKey(key core.PrimaryDataStoreKey, fieldName stri
// tryGetSchemaFieldID returns the FieldID of the given fieldName.
// Will return false if the field is not found.
func (c *collection) tryGetSchemaFieldID(fieldName string) (uint32, bool) {
for _, field := range c.desc.Schema.Fields {
for _, field := range c.Schema().Fields {
if field.Name == fieldName {
if field.IsObject() || field.IsObjectArray() {
// We do not wish to match navigational properties, only
Expand Down
3 changes: 1 addition & 2 deletions db/collection_get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ func (c *collection) get(
) (*client.Document, error) {
// create a new document fetcher
df := c.newFetcher()
desc := &c.desc
// initialize it with the primary index
err := df.Init(ctx, txn, c, fields, nil, nil, false, showDeleted)
if err != nil {
Expand All @@ -62,7 +61,7 @@ func (c *collection) get(
}

// construct target key for DocKey
targetKey := base.MakeDocKey(*desc, key.DocKey)
targetKey := base.MakeDocKey(c.Description(), key.DocKey)
// run the doc fetcher
err = df.Start(ctx, core.NewSpans(core.NewSpan(targetKey, targetKey.PrefixEnd())))
if err != nil {
Expand Down
14 changes: 7 additions & 7 deletions db/collection_index.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ func (c *collection) createIndex(
if err != nil {
return nil, err
}
c.desc.Indexes = append(c.desc.Indexes, colIndex.Description())
c.def.Description.Indexes = append(c.def.Description.Indexes, colIndex.Description())
c.indexes = append(c.indexes, colIndex)
err = c.indexExistingDocs(ctx, txn, colIndex)
if err != nil {
Expand All @@ -239,7 +239,7 @@ func (c *collection) iterateAllDocs(
_ = df.Close()
return err
}
start := base.MakeCollectionKey(c.desc)
start := base.MakeCollectionKey(c.Description())
spans := core.NewSpans(core.NewSpan(start, start.PrefixEnd()))

err = df.Start(ctx, spans)
Expand Down Expand Up @@ -334,9 +334,9 @@ func (c *collection) dropIndex(ctx context.Context, txn datastore.Txn, indexName
return NewErrIndexWithNameDoesNotExists(indexName)
}

for i := range c.desc.Indexes {
if c.desc.Indexes[i].Name == indexName {
c.desc.Indexes = append(c.desc.Indexes[:i], c.desc.Indexes[i+1:]...)
for i := range c.Description().Indexes {
if c.Description().Indexes[i].Name == indexName {
c.def.Description.Indexes = append(c.Description().Indexes[:i], c.Description().Indexes[i+1:]...)
break
}
}
Expand Down Expand Up @@ -380,7 +380,7 @@ func (c *collection) loadIndexes(ctx context.Context, txn datastore.Txn) error {
}
colIndexes = append(colIndexes, index)
}
c.desc.Indexes = indexDescriptions
c.def.Description.Indexes = indexDescriptions
c.indexes = colIndexes
return nil
}
Expand All @@ -397,7 +397,7 @@ func (c *collection) GetIndexes(ctx context.Context) ([]client.IndexDescription,
if err != nil {
return nil, err
}
return c.desc.Indexes, nil
return c.Description().Indexes, nil
}

func (c *collection) checkExistingFields(
Expand Down
Loading