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

fix(i): JSON object default value panic #3156

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
+60 −38
Open

fix(i): JSON object default value panic #3156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e0706db
move default value off of FieldDefinition
nasdf Oct 18, 2024
a11746c
replace map keys with FieldDefinitionKey
nasdf Oct 21, 2024
9d40ecd
make mocks
nasdf Oct 21, 2024
20cd6e4
Merge branch 'develop' into nasdf/fix/json-default-value-panic
nasdf Oct 21, 2024
b75cf79
add data format change doc
nasdf Oct 23, 2024
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: 17 additions & 0 deletions client/definitions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,15 @@ func (def CollectionDefinition) GetName() string {
return def.Schema.Name
}

// FieldDefinitionKey is used a map key to uniquely identify a FieldDefinition.
type FieldDefinitionKey struct {
// Name contains the name of the field.
Name string

// ID contains the local, internal ID to the field.
ID FieldID
}

// FieldDefinition describes the combined local and global set of properties that constitutes
// a field on a collection.
//
Expand Down Expand Up @@ -191,6 +200,14 @@ func NewSchemaOnlyFieldDefinition(global SchemaFieldDescription) FieldDefinition
}
}

// Key returns the FieldDefinitionKey for this FieldDefinition.
func (f FieldDefinition) Key() FieldDefinitionKey {
return FieldDefinitionKey{
ID: f.ID,
Name: f.Name,
}
}

// IsRelation returns true if this field is a relation.
func (f FieldDefinition) IsRelation() bool {
return f.RelationName != ""
Expand Down
3 changes: 3 additions & 0 deletions docs/data_format_changes/i3146-json-default-value.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# JSON default value panic fix

Fixed an issue with JSON default values that caused a panic when they were objects. No data format changes.
22 changes: 11 additions & 11 deletions internal/db/fetcher/encoded_doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type EncodedDocument interface {

// Properties returns a copy of the decoded property values mapped by their field
// description.
Properties(onlyFilterProps bool) (map[client.FieldDefinition]any, error)
Properties(onlyFilterProps bool) (map[client.FieldDefinitionKey]any, error)

// Reset re-initializes the EncodedDocument object.
Reset()
Expand Down Expand Up @@ -68,8 +68,8 @@ type encodedDocument struct {
id []byte
schemaVersionID string
status client.DocumentStatus
properties map[client.FieldDefinition]*encProperty
decodedPropertyCache map[client.FieldDefinition]any
properties map[client.FieldDefinitionKey]*encProperty
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Why are you using both field ID and Name here? At a single point in time either will be unique - you can't two fields with the same fieldID or with the same name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both the field ID and name are used in the downstream dependencies. If we used only one as the map key then we would need to expand the FieldDefinition API to support looking up a field by ID or copy paste a lookup function into a few places. I think having the FieldDefinitionKey is the better alternative.

Copy link
Contributor

@AndrewSisley AndrewSisley Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There should already be functions to get fields by Name if you use that as the key - I don't think you'd need to add anything to client.

Having a (public) FieldDefinitionKey struct with name and key properties suggests that they need to be combined in order to form a unique key - which is incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

The DecodeToDoc function in the fetcher doesn't have a reference to the definition so it can't lookup the field ID from the name.

I'm fine with changing the struct name but I don't see any issues with the way I implemented it.

Copy link
Contributor

Choose a reason for hiding this comment

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

DecodeToDoc just uses the field ID as an index - we could change that too. Or you could change encodedDocument.properties to a slice containing field name and value.

There are many ways to solve this internal coding problem that don't involve changing the public types, especially not in a way that seems a little misleading.

decodedPropertyCache map[client.FieldDefinitionKey]any

// tracking bitsets
// A value of 1 indicates a required field
Expand All @@ -96,7 +96,7 @@ func (encdoc *encodedDocument) Status() client.DocumentStatus {

// Reset re-initializes the EncodedDocument object.
func (encdoc *encodedDocument) Reset() {
encdoc.properties = make(map[client.FieldDefinition]*encProperty, 0)
encdoc.properties = make(map[client.FieldDefinitionKey]*encProperty, 0)
encdoc.id = nil
encdoc.filterSet = nil
encdoc.selectSet = nil
Expand Down Expand Up @@ -175,10 +175,10 @@ func DecodeToDoc(encdoc EncodedDocument, mapping *core.DocumentMapping, filter b
return doc, nil
}

func (encdoc *encodedDocument) Properties(onlyFilterProps bool) (map[client.FieldDefinition]any, error) {
result := map[client.FieldDefinition]any{}
func (encdoc *encodedDocument) Properties(onlyFilterProps bool) (map[client.FieldDefinitionKey]any, error) {
result := map[client.FieldDefinitionKey]any{}
if encdoc.decodedPropertyCache == nil {
encdoc.decodedPropertyCache = map[client.FieldDefinition]any{}
encdoc.decodedPropertyCache = map[client.FieldDefinitionKey]any{}
}

for _, prop := range encdoc.properties {
Expand All @@ -188,9 +188,9 @@ func (encdoc *encodedDocument) Properties(onlyFilterProps bool) (map[client.Fiel
}

// used cached decoded fields
cachedValue := encdoc.decodedPropertyCache[prop.Desc]
cachedValue := encdoc.decodedPropertyCache[prop.Desc.Key()]
if cachedValue != nil {
result[prop.Desc] = cachedValue
result[prop.Desc.Key()] = cachedValue
continue
}

Expand All @@ -200,8 +200,8 @@ func (encdoc *encodedDocument) Properties(onlyFilterProps bool) (map[client.Fiel
}

// cache value
encdoc.decodedPropertyCache[prop.Desc] = val
result[prop.Desc] = val
encdoc.decodedPropertyCache[prop.Desc.Key()] = val
result[prop.Desc.Key()] = val
}

return result, nil
Expand Down
2 changes: 1 addition & 1 deletion internal/db/fetcher/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ func (df *DocumentFetcher) processKV(kv *keyValue) error {

df.execInfo.FieldsFetched++

df.doc.properties[fieldDesc] = property
df.doc.properties[fieldDesc.Key()] = property

return nil
}
Expand Down
2 changes: 1 addition & 1 deletion internal/db/fetcher/indexer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ func (f *IndexFetcher) FetchNext(ctx context.Context) (EncodedDocument, ExecInfo
}
property.Raw = fieldBytes

f.doc.properties[indexedField] = property
f.doc.properties[indexedField.Key()] = property
}

if f.indexDesc.Unique && !hasNilField {
Expand Down
14 changes: 7 additions & 7 deletions internal/db/fetcher/mocks/encoded_document.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions internal/db/indexed_docs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1097,7 +1097,7 @@ type shimEncodedDocument struct {
key []byte
schemaVersionID string
status client.DocumentStatus
properties map[client.FieldDefinition]any
properties map[client.FieldDefinitionKey]any
}

var _ fetcher.EncodedDocument = (*shimEncodedDocument)(nil)
Expand All @@ -1114,15 +1114,15 @@ func (encdoc *shimEncodedDocument) Status() client.DocumentStatus {
return encdoc.status
}

func (encdoc *shimEncodedDocument) Properties(onlyFilterProps bool) (map[client.FieldDefinition]any, error) {
func (encdoc *shimEncodedDocument) Properties(onlyFilterProps bool) (map[client.FieldDefinitionKey]any, error) {
return encdoc.properties, nil
}

func (encdoc *shimEncodedDocument) Reset() {
encdoc.key = nil
encdoc.schemaVersionID = ""
encdoc.status = 0
encdoc.properties = map[client.FieldDefinition]any{}
encdoc.properties = map[client.FieldDefinitionKey]any{}
}

func TestUniqueCreate_ShouldIndexExistingDocs(t *testing.T) {
Expand Down
10 changes: 5 additions & 5 deletions internal/lens/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@
func (f *lensedFetcher) lensDocToEncodedDoc(docAsMap LensDoc) (fetcher.EncodedDocument, error) {
var key string
status := client.Active
properties := map[client.FieldDefinition]any{}
properties := map[client.FieldDefinitionKey]any{}

for fieldName, fieldByteValue := range docAsMap {
if fieldName == request.DocIDFieldName {
Expand Down Expand Up @@ -245,7 +245,7 @@
return nil, err
}

properties[fieldDesc] = fieldValue
properties[fieldDesc.Key()] = fieldValue
}

return &lensEncodedDocument{
Expand Down Expand Up @@ -333,7 +333,7 @@
key []byte
schemaVersionID string
status client.DocumentStatus
properties map[client.FieldDefinition]any
properties map[client.FieldDefinitionKey]any
}

var _ fetcher.EncodedDocument = (*lensEncodedDocument)(nil)
Expand All @@ -350,13 +350,13 @@
return encdoc.status
}

func (encdoc *lensEncodedDocument) Properties(onlyFilterProps bool) (map[client.FieldDefinition]any, error) {
func (encdoc *lensEncodedDocument) Properties(onlyFilterProps bool) (map[client.FieldDefinitionKey]any, error) {
return encdoc.properties, nil
}

func (encdoc *lensEncodedDocument) Reset() {
encdoc.key = nil
encdoc.schemaVersionID = ""
encdoc.status = 0
encdoc.properties = map[client.FieldDefinition]any{}
encdoc.properties = map[client.FieldDefinitionKey]any{}

Check warning on line 361 in internal/lens/fetcher.go

View check run for this annotation

Codecov / codecov/patch

internal/lens/fetcher.go#L361 

Added line #L361 was not covered by tests
}
10 changes: 6 additions & 4 deletions tests/integration/collection_description/with_default_fields_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestCollectionDescription_WithDefaultFieldValues(t *testing.T) {
name: String @default(string: "Bob")
age: Int @default(int: 10)
points: Float @default(float: 30)
metadata: JSON @default(json: "{\"value\":1}")
metadata: JSON @default(json: {value: 1})
image: Blob @default(blob: "ff0099")
}
`,
Expand Down Expand Up @@ -66,9 +66,11 @@ func TestCollectionDescription_WithDefaultFieldValues(t *testing.T) {
DefaultValue: "ff0099",
},
{
ID: 5,
Name: "metadata",
DefaultValue: "{\"value\":1}",
ID: 5,
Name: "metadata",
DefaultValue: map[string]any{
"value": float64(1),
},
},
{
ID: 6,
Expand Down
12 changes: 6 additions & 6 deletions tests/integration/mutation/create/with_default_values_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func TestMutationCreate_WithDefaultValues_NoValuesProvided_SetsDefaultValue(t *t
name: String @default(string: "Bob")
age: Int @default(int: 40)
points: Float @default(float: 10)
metadata: JSON @default(json: "{\"one\":1}")
metadata: JSON @default(json: {tree: "oak"})
image: Blob @default(blob: "ff0099")
}
`,
Expand Down Expand Up @@ -60,7 +60,7 @@ func TestMutationCreate_WithDefaultValues_NoValuesProvided_SetsDefaultValue(t *t
"name": "Bob",
"points": float64(10),
"created": time.Time(time.Date(2000, time.July, 23, 3, 0, 0, 0, time.UTC)),
"metadata": "{\"one\":1}",
"metadata": map[string]any{"tree": "oak"},
"image": "ff0099",
},
},
Expand All @@ -84,7 +84,7 @@ func TestMutationCreate_WithDefaultValues_NilValuesProvided_SetsNilValue(t *test
name: String @default(string: "Bob")
age: Int @default(int: 40)
points: Float @default(float: 10)
metadata: JSON @default(json: "{\"one\":1}")
metadata: JSON @default(json: {tree: "oak"})
image: Blob @default(blob: "ff0099")
}
`,
Expand Down Expand Up @@ -144,7 +144,7 @@ func TestMutationCreate_WithDefaultValues_ValuesProvided_SetsValue(t *testing.T)
name: String @default(string: "Bob")
age: Int @default(int: 40)
points: Float @default(float: 10)
metadata: JSON @default(json: "{\"one\":1}")
metadata: JSON @default(json: {tree: "oak"})
image: Blob @default(blob: "ff0099")
}
`,
Expand All @@ -156,7 +156,7 @@ func TestMutationCreate_WithDefaultValues_ValuesProvided_SetsValue(t *testing.T)
"name": "Alice",
"points": float64(5),
"created": time.Time(time.Date(2024, time.June, 18, 1, 0, 0, 0, time.UTC)),
"metadata": "{\"two\":2}",
"metadata": map[string]any{"tree": "maple"},
"image": "aabb33",
},
},
Expand All @@ -180,7 +180,7 @@ func TestMutationCreate_WithDefaultValues_ValuesProvided_SetsValue(t *testing.T)
"name": "Alice",
"points": float64(5),
"created": time.Time(time.Date(2024, time.June, 18, 1, 0, 0, 0, time.UTC)),
"metadata": "{\"two\":2}",
"metadata": map[string]any{"tree": "maple"},
"image": "aabb33",
},
},
Expand Down
Loading