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: Make CollectionDescription.Name Option #2223

Merged
merged 6 commits into from
Jan 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion cli/collection.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func MakeCollectionCommand(cfg *config.Config) *cobra.Command {
fetchedCols := cols
cols = nil
for _, c := range fetchedCols {
if c.Name() == name {
if c.Name().Value() == name {
Copy link
Member

Choose a reason for hiding this comment

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

question: Why have you skipped HasValue() here before calling Value(). If IRRC the value returned is invalid unless HasValue() == true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

name cannot be "" here, and at the moment fetchedCols cannot contain any nameless collections.

The code that populates fetchedCols will likely need restructuring somewhat in the followup PR that sorts out the version stuff, this line will be looked at again at that point.

cols = append(cols, c)
break
}
Expand Down
4 changes: 3 additions & 1 deletion client/collection.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ package client
import (
"context"

"github.com/sourcenetwork/immutable"

"github.com/sourcenetwork/defradb/datastore"
)

Expand All @@ -32,7 +34,7 @@ type CollectionDefinition struct {
// Many functions on this object will interact with the underlying datastores.
type Collection interface {
// Name returns the name of this collection.
Name() string
Name() immutable.Option[string]

// ID returns the ID of this Collection.
ID() uint32
Copy link
Member

Choose a reason for hiding this comment

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

question: Will ID always be there even if Name() isn't? or should this also be optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ID will always be there.

Expand Down
96 changes: 89 additions & 7 deletions client/descriptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@
package client

import (
"encoding/json"
"fmt"

"github.com/sourcenetwork/immutable"

"github.com/sourcenetwork/defradb/client/request"
)

Expand All @@ -22,7 +25,7 @@
//
// It is conceptually local to the node hosting the DefraDB instance, but currently there
// is no means to update the local value so that it differs from the (global) schema name.
Name string
Name immutable.Option[string]

// ID is the local identifier of this collection.
//
Expand All @@ -32,12 +35,11 @@
// The ID of the schema version that this collection is at.
SchemaVersionID string

// BaseQuery contains the base query of this view, if this collection is a view.
// Sources is the set of sources from which this collection draws data.
//
// The query will be saved, and then may be accessed by other actors on demand. Actor defined
// aggregates, filters and other logic (such as LensVM transforms) will execute on top of this
// base query before the result is returned to the actor.
BaseQuery *request.Select
// Currently supported source types are:
// - [QuerySource]
Sources []any

// Indexes contains the secondary indexes that this Collection has.
Indexes []IndexDescription
Expand Down Expand Up @@ -78,13 +80,37 @@
schema *SchemaDescription,
) (FieldDescription, bool) {
for _, field := range schema.Fields {
if field.RelationName == relationName && !(col.Name == otherCollectionName && otherFieldName == field.Name) {
if field.RelationName == relationName && !(col.Name.Value() == otherCollectionName && otherFieldName == field.Name) {
Copy link
Member

Choose a reason for hiding this comment

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

question: Why have you skipped HasValue() here before calling Value(). If IRRC the value returned is invalid unless HasValue() == true

question: Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar answer to the above in that Name will always have a value here, and this function will probably to change to work with ID instead anyway.

return field, true
}
}
return FieldDescription{}, false
}

// QuerySources returns all the Sources of type [QuerySource]
func (col CollectionDescription) QuerySources() []*QuerySource {
return sourcesOfType[*QuerySource](col)
}

func sourcesOfType[ResultType any](col CollectionDescription) []ResultType {
result := []ResultType{}
for _, source := range col.Sources {
if typedSource, isOfType := source.(ResultType); isOfType {
result = append(result, typedSource)
}
}
return result
}

// QuerySource represents a collection data source from a query.
//
// The query will be executed when data from this source is requested, and the query results
// yielded to the consumer.
type QuerySource struct {
// Query contains the base query of this data source.
Query request.Select
}

// SchemaDescription describes a Schema and its associated metadata.
type SchemaDescription struct {
// Root is the version agnostic identifier for this schema.
Expand Down Expand Up @@ -322,3 +348,59 @@
func (m RelationType) IsSet(target RelationType) bool {
return m&target > 0
}

// collectionDescription is a private type used to facilitate the unmarshalling
// of json to a [CollectionDescription].
type collectionDescription struct {
// These properties are unmarshalled using the default json unmarshaller
Name immutable.Option[string]
ID uint32
SchemaVersionID string
Indexes []IndexDescription

// Properties below this line are unmarshalled using custom logic in [UnmarshalJSON]
Sources []map[string]json.RawMessage
}

func (c *CollectionDescription) UnmarshalJSON(bytes []byte) error {
var descMap collectionDescription
err := json.Unmarshal(bytes, &descMap)
if err != nil {
return err
}

Check warning on line 370 in client/descriptions.go

View check run for this annotation

Codecov / codecov/patch

client/descriptions.go#L369-L370

Added lines #L369 - L370 were not covered by tests

c.Name = descMap.Name
c.ID = descMap.ID
c.SchemaVersionID = descMap.SchemaVersionID
c.Indexes = descMap.Indexes
c.Sources = make([]any, len(descMap.Sources))

for i, source := range descMap.Sources {
sourceJson, err := json.Marshal(source)
if err != nil {
return err
}

Check warning on line 382 in client/descriptions.go

View check run for this annotation

Codecov / codecov/patch

client/descriptions.go#L381-L382

Added lines #L381 - L382 were not covered by tests

var sourceValue any
// We detect which concrete type each `Source` object is by detecting
// non-nillable fields, if the key is present it must be of that type.
// They must be non-nillable as nil values may have their keys omitted from
// the json. This also relies on the fields being unique. We may wish to change
// this later to custom-serialize with a `_type` property.
if _, ok := source["Query"]; ok {
// This must be a QuerySource, as only the `QuerySource` type has a `Query` field
var querySource QuerySource
err := json.Unmarshal(sourceJson, &querySource)
if err != nil {
return err
}

Check warning on line 396 in client/descriptions.go

View check run for this annotation

Codecov / codecov/patch

client/descriptions.go#L395-L396

Added lines #L395 - L396 were not covered by tests
sourceValue = &querySource
} else {
return ErrFailedToUnmarshalCollection
}

Check warning on line 400 in client/descriptions.go

View check run for this annotation

Codecov / codecov/patch

client/descriptions.go#L398-L400

Added lines #L398 - L400 were not covered by tests

c.Sources[i] = sourceValue
}

return nil
}
46 changes: 25 additions & 21 deletions client/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,34 +17,38 @@ import (
)

const (
errFieldNotExist string = "The given field does not exist"
errUnexpectedType string = "unexpected type"
errParsingFailed string = "failed to parse argument"
errUninitializeProperty string = "invalid state, required property is uninitialized"
errMaxTxnRetries string = "reached maximum transaction reties"
errRelationOneSided string = "relation must be defined on both schemas"
errCollectionNotFound string = "collection not found"
errFieldOrAliasToFieldNotExist string = "The given field or alias to field does not exist"
errUnknownCRDT string = "unknown crdt"
errCRDTKindMismatch string = "CRDT type %s can't be assigned to field kind %s"
errInvalidCRDTType string = "CRDT type not supported"
errFieldNotExist string = "The given field does not exist"
errUnexpectedType string = "unexpected type"
errParsingFailed string = "failed to parse argument"
errUninitializeProperty string = "invalid state, required property is uninitialized"
errMaxTxnRetries string = "reached maximum transaction reties"
errRelationOneSided string = "relation must be defined on both schemas"
errCollectionNotFound string = "collection not found"
errFieldOrAliasToFieldNotExist string = "The given field or alias to field does not exist"
errUnknownCRDT string = "unknown crdt"
errCRDTKindMismatch string = "CRDT type %s can't be assigned to field kind %s"
errInvalidCRDTType string = "CRDT type not supported"
errFailedToUnmarshalCollection string = "failed to unmarshal collection json"
errOperationNotPermittedOnNamelessCols string = "operation not permitted on nameless collection"
)

// Errors returnable from this package.
//
// This list is incomplete and undefined errors may also be returned.
// Errors returned from this package may be tested against these errors with errors.Is.
var (
ErrFieldNotExist = errors.New(errFieldNotExist)
ErrUnexpectedType = errors.New(errUnexpectedType)
ErrFieldNotObject = errors.New("trying to access field on a non object type")
ErrValueTypeMismatch = errors.New("value does not match indicated type")
ErrDocumentNotFound = errors.New("no document for the given ID exists")
ErrInvalidUpdateTarget = errors.New("the target document to update is of invalid type")
ErrInvalidUpdater = errors.New("the updater of a document is of invalid type")
ErrInvalidDeleteTarget = errors.New("the target document to delete is of invalid type")
ErrMalformedDocID = errors.New("malformed document ID, missing either version or cid")
ErrInvalidDocIDVersion = errors.New("invalid document ID version")
ErrFieldNotExist = errors.New(errFieldNotExist)
ErrUnexpectedType = errors.New(errUnexpectedType)
ErrFailedToUnmarshalCollection = errors.New(errFailedToUnmarshalCollection)
ErrOperationNotPermittedOnNamelessCols = errors.New(errOperationNotPermittedOnNamelessCols)
ErrFieldNotObject = errors.New("trying to access field on a non object type")
ErrValueTypeMismatch = errors.New("value does not match indicated type")
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: would be nice to just remove ErrValueTypeMismatch error as it is not being used anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • See if ErrValueTypeMismatch can be removed

ErrDocumentNotFound = errors.New("no document for the given ID exists")
ErrInvalidUpdateTarget = errors.New("the target document to update is of invalid type")
ErrInvalidUpdater = errors.New("the updater of a document is of invalid type")
ErrInvalidDeleteTarget = errors.New("the target document to delete is of invalid type")
ErrMalformedDocID = errors.New("malformed document ID, missing either version or cid")
ErrInvalidDocIDVersion = errors.New("invalid document ID version")
)

// NewErrFieldNotExist returns an error indicating that the given field does not exist.
Expand Down
27 changes: 17 additions & 10 deletions core/key.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

"github.com/ipfs/go-cid"
ds "github.com/ipfs/go-datastore"
"github.com/sourcenetwork/immutable"

"github.com/sourcenetwork/defradb/client"
"github.com/sourcenetwork/defradb/errors"
Expand Down Expand Up @@ -132,8 +133,8 @@

// CollectionIndexKey to a stored description of an index
type CollectionIndexKey struct {
// CollectionName is the name of the collection that the index is on
CollectionName string
// CollectionID is the id of the collection that the index is on
CollectionID immutable.Option[uint32]
// IndexName is the name of the index
IndexName string
}
Expand Down Expand Up @@ -291,22 +292,28 @@
}

// NewCollectionIndexKey creates a new CollectionIndexKey from a collection name and index name.
func NewCollectionIndexKey(colID, indexName string) CollectionIndexKey {
return CollectionIndexKey{CollectionName: colID, IndexName: indexName}
func NewCollectionIndexKey(colID immutable.Option[uint32], indexName string) CollectionIndexKey {
Copy link
Member

Choose a reason for hiding this comment

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

question: Related to my question above, since ID is optional (due to I guess name being optional) then should probably change on Collection interface as well?

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 think you misunderstand, if colID here is optional it will create an empty key that allows scanning over the entire prefix. CollectionDescription.ID is not optional.

return CollectionIndexKey{CollectionID: colID, IndexName: indexName}
}

// NewCollectionIndexKeyFromString creates a new CollectionIndexKey from a string.
// It expects the input string is in the following format:
//
// /collection/index/[CollectionName]/[IndexName]
// /collection/index/[CollectionID]/[IndexName]
//
// Where [IndexName] might be omitted. Anything else will return an error.
func NewCollectionIndexKeyFromString(key string) (CollectionIndexKey, error) {
keyArr := strings.Split(key, "/")
if len(keyArr) < 4 || len(keyArr) > 5 || keyArr[1] != "collection" || keyArr[2] != "index" {
return CollectionIndexKey{}, ErrInvalidKey
}
result := CollectionIndexKey{CollectionName: keyArr[3]}

colID, err := strconv.Atoi(keyArr[3])
if err != nil {
return CollectionIndexKey{}, err
}

Check warning on line 314 in core/key.go

View check run for this annotation

Codecov / codecov/patch

core/key.go#L313-L314

Added lines #L313 - L314 were not covered by tests

result := CollectionIndexKey{CollectionID: immutable.Some(uint32(colID))}
if len(keyArr) == 5 {
result.IndexName = keyArr[4]
}
Expand All @@ -315,13 +322,13 @@

// ToString returns the string representation of the key
// It is in the following format:
// /collection/index/[CollectionName]/[IndexName]
// if [CollectionName] is empty, the rest is ignored
// /collection/index/[CollectionID]/[IndexName]
// if [CollectionID] is empty, the rest is ignored
func (k CollectionIndexKey) ToString() string {
result := COLLECTION_INDEX

if k.CollectionName != "" {
result = result + "/" + k.CollectionName
if k.CollectionID.HasValue() {
result = result + "/" + fmt.Sprint(k.CollectionID.Value())
if k.IndexName != "" {
result = result + "/" + k.IndexName
}
Expand Down
31 changes: 16 additions & 15 deletions core/key_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"testing"

ds "github.com/ipfs/go-datastore"
"github.com/sourcenetwork/immutable"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -110,23 +111,23 @@ func TestNewDataStoreKey_GivenAStringWithExtraSuffix(t *testing.T) {
}

func TestNewIndexKey_IfEmptyParam_ReturnPrefix(t *testing.T) {
key := NewCollectionIndexKey("", "")
key := NewCollectionIndexKey(immutable.None[uint32](), "")
assert.Equal(t, "/collection/index", key.ToString())
}

func TestNewIndexKey_IfParamsAreGiven_ReturnFullKey(t *testing.T) {
key := NewCollectionIndexKey("col", "idx")
assert.Equal(t, "/collection/index/col/idx", key.ToString())
key := NewCollectionIndexKey(immutable.Some[uint32](1), "idx")
assert.Equal(t, "/collection/index/1/idx", key.ToString())
}

func TestNewIndexKey_InNoCollectionName_ReturnJustPrefix(t *testing.T) {
key := NewCollectionIndexKey("", "idx")
key := NewCollectionIndexKey(immutable.None[uint32](), "idx")
assert.Equal(t, "/collection/index", key.ToString())
}

func TestNewIndexKey_InNoIndexName_ReturnWithoutIndexName(t *testing.T) {
key := NewCollectionIndexKey("col", "")
assert.Equal(t, "/collection/index/col", key.ToString())
key := NewCollectionIndexKey(immutable.Some[uint32](1), "")
assert.Equal(t, "/collection/index/1", key.ToString())
}

func TestNewIndexKeyFromString_IfInvalidString_ReturnError(t *testing.T) {
Expand All @@ -144,17 +145,17 @@ func TestNewIndexKeyFromString_IfInvalidString_ReturnError(t *testing.T) {
}

func TestNewIndexKeyFromString_IfOnlyCollectionName_ReturnKey(t *testing.T) {
key, err := NewCollectionIndexKeyFromString("/collection/index/col")
key, err := NewCollectionIndexKeyFromString("/collection/index/1")
assert.NoError(t, err)
assert.Equal(t, key.CollectionName, "col")
assert.Equal(t, key.IndexName, "")
assert.Equal(t, immutable.Some[uint32](1), key.CollectionID)
assert.Equal(t, "", key.IndexName)
}

func TestNewIndexKeyFromString_IfFullKeyString_ReturnKey(t *testing.T) {
key, err := NewCollectionIndexKeyFromString("/collection/index/col/idx")
key, err := NewCollectionIndexKeyFromString("/collection/index/1/idx")
assert.NoError(t, err)
assert.Equal(t, key.CollectionName, "col")
assert.Equal(t, key.IndexName, "idx")
assert.Equal(t, immutable.Some[uint32](1), key.CollectionID)
assert.Equal(t, "idx", key.IndexName)
}

func toFieldValues(values ...string) [][]byte {
Expand Down Expand Up @@ -312,10 +313,10 @@ func TestIndexDatastoreKey_EqualTrue(t *testing.T) {

func TestCollectionIndexKey_Bytes(t *testing.T) {
key := CollectionIndexKey{
CollectionName: "col",
IndexName: "idx",
CollectionID: immutable.Some[uint32](1),
IndexName: "idx",
}
assert.Equal(t, []byte(COLLECTION_INDEX+"/col/idx"), key.Bytes())
assert.Equal(t, []byte(COLLECTION_INDEX+"/1/idx"), key.Bytes())
}

func TestIndexDatastoreKey_EqualFalse(t *testing.T) {
Expand Down
6 changes: 3 additions & 3 deletions db/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func (db *db) basicExport(ctx context.Context, txn datastore.Txn, config *client
}
colNameCache := map[string]struct{}{}
for _, col := range cols {
colNameCache[col.Name()] = struct{}{}
colNameCache[col.Name().Value()] = struct{}{}
Copy link
Member

Choose a reason for hiding this comment

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

question: Why have you skipped HasValue() here before calling Value(). If IRRC the value returned is invalid unless HasValue() == true

question: Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Name can't be empty here. The code is exporting queryable (i.e. named) data.

}

tempFile := config.Filepath + ".temp"
Expand Down Expand Up @@ -181,8 +181,8 @@ func (db *db) basicExport(ctx context.Context, txn datastore.Txn, config *client
// set collection
err = writeString(
f,
fmt.Sprintf("\"%s\":[", col.Name()),
fmt.Sprintf(" \"%s\": [\n", col.Name()),
fmt.Sprintf("\"%s\":[", col.Name().Value()),
fmt.Sprintf(" \"%s\": [\n", col.Name().Value()),
Comment on lines +184 to +185
Copy link
Member

Choose a reason for hiding this comment

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

question: Why have you skipped HasValue() here before calling Value(). If IRRC the value returned is invalid unless HasValue() == true

question: Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Name can't be empty here. The code is exporting queryable (i.e. named) data.

config.Pretty,
)
if err != nil {
Expand Down
Loading
Loading