Merge pull request #307 from mimiro-io/fix/nil-ref-panics

handle possible panics with nil refs
mimiro-io · Jun 24, 2024 · b38de44 · b38de44
2 parents 1f9a6eb + 183e713
commit b38de44
Show file tree

Hide file tree

Showing 2 changed files with 95 additions and 17 deletions.
diff --git a/internal/server/dataset.go b/internal/server/dataset.go
@@ -454,7 +454,11 @@ func (ds *Dataset) StoreEntitiesWithTransaction(
 				case []interface{}:
 					refs = make([]string, len(v))
 					for i, v := range v {
-						refs[i] = v.(string)
+						if val, ok := v.(string); ok {
+							refs[i] = val
+						} else {
+							return newitems, fmt.Errorf("encountered nil in ref array, cannot store entity %v", e)
+						}
 					}
 				case nil:
 					return newitems, fmt.Errorf("encountered nil ref, cannot store entity %v", e)
@@ -535,18 +539,21 @@ func (ds *Dataset) StoreEntitiesWithTransaction(
 							return newitems, err
 						}
 
-						// get related
-						relatedid, _, err := ds.store.assertIDForURI(ref.(string), idCache)
-						if err != nil {
-							return newitems, err
-						}
+						// get related, but skip if it is not a string
+						refVal, ok := ref.(string)
+						if ok {
+							relatedid, _, err := ds.store.assertIDForURI(refVal, idCache)
+							if err != nil {
+								return newitems, err
+							}
 
-						if refs, ok := oldRefs[predid]; ok {
-							refs = append(refs, relatedid)
-							oldRefs[predid] = refs
-						} else {
-							refs := []uint64{relatedid}
-							oldRefs[predid] = refs
+							if refs, ok := oldRefs[predid]; ok {
+								refs = append(refs, relatedid)
+								oldRefs[predid] = refs
+							} else {
+								refs := []uint64{relatedid}
+								oldRefs[predid] = refs
+							}
 						}
 					}
 				}
@@ -597,7 +604,10 @@ func (ds *Dataset) StoreEntitiesWithTransaction(
 					case []interface{}:
 						refs = make([]string, len(v))
 						for i, val := range v {
-							refs[i] = val.(string)
+							var ok bool
+							if refs[i], ok = val.(string); !ok {
+								return newitems, fmt.Errorf("encountered nil ref in ref array, cannot store entity %v", e)
+							}
 						}
 					case nil:
 						return newitems, fmt.Errorf("encountered nil ref, cannot store entity %v", e)

diff --git a/internal/server/dataset_test.go b/internal/server/dataset_test.go
@@ -15,15 +15,17 @@
 package server
 
 import (
+	"encoding/binary"
+	"encoding/json"
 	"fmt"
-	"os"
-	"strconv"
-	"strings"
-
 	"github.com/DataDog/datadog-go/v5/statsd"
 	"github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"go.uber.org/zap"
+	"os"
+	"strconv"
+	"strings"
+	"time"
 
 	"github.com/mimiro-io/datahub/internal/conf"
 )
@@ -146,6 +148,72 @@ var _ = ginkgo.Describe("A Dataset", func() {
 		Expect(company3Seen).To(BeTrue(), "company-3 was not observed in relations")
 	})
 
+	ginkgo.It("Should not panic when storing an entity with a nil refs map", func() {
+		person := NewEntity(env.peopleNamespacePrefix+":person-1", 1)
+		person.References[env.peopleNamespacePrefix+":workedfor"] = nil
+		err := env.ds.StoreEntities([]*Entity{person})
+		Expect(err.Error()).To(ContainSubstring("encountered nil ref"), "Expected entity to be stored without error")
+		// store invalid entity anyway, and try to update it
+		entityIDBuffer := make([]byte, 24)
+		binary.BigEndian.PutUint16(entityIDBuffer, EntityIDToJSONIndexID)
+		binary.BigEndian.PutUint64(entityIDBuffer[2:], person.InternalID)
+		binary.BigEndian.PutUint32(entityIDBuffer[10:], env.ds.InternalID)
+		binary.BigEndian.PutUint64(entityIDBuffer[14:], uint64(time.Now().UnixNano()))
+		binary.BigEndian.PutUint16(entityIDBuffer[22:], uint16(0))
+
+		datasetEntitiesLatestVersionKey := make([]byte, 14)
+		binary.BigEndian.PutUint16(datasetEntitiesLatestVersionKey, DatasetLatestEntities)
+		binary.BigEndian.PutUint32(datasetEntitiesLatestVersionKey[2:], env.ds.InternalID)
+		binary.BigEndian.PutUint64(datasetEntitiesLatestVersionKey[6:], person.InternalID)
+
+		txn := env.ds.store.database.NewTransaction(true)
+		jsonData, _ := json.Marshal(person)
+		err = txn.Set(entityIDBuffer, jsonData)
+		Expect(err).To(BeNil())
+		err = txn.Set(datasetEntitiesLatestVersionKey, entityIDBuffer)
+		Expect(err).To(BeNil())
+		txn.Commit()
+
+		person.Properties[env.peopleNamespacePrefix+":Name"] = "person 1"
+		err = env.ds.StoreEntities([]*Entity{person})
+		Expect(err.Error()).To(ContainSubstring("encountered nil ref"), "Expected entity to be stored without error")
+	})
+	ginkgo.It("Should not panic when storing an entity with a nil array in refs map", func() {
+
+		person := NewEntity(env.peopleNamespacePrefix+":person-1", 1)
+		person.References[env.peopleNamespacePrefix+":workedfor"] = []any{
+			nil,
+			nil,
+		}
+		err := env.ds.StoreEntities([]*Entity{person})
+		Expect(err.Error()).To(ContainSubstring("encountered nil in ref array"), "Expected entity to be stored without error")
+
+		// store invalid entity anyway, and try to update it
+		entityIDBuffer := make([]byte, 24)
+		binary.BigEndian.PutUint16(entityIDBuffer, EntityIDToJSONIndexID)
+		binary.BigEndian.PutUint64(entityIDBuffer[2:], person.InternalID)
+		binary.BigEndian.PutUint32(entityIDBuffer[10:], env.ds.InternalID)
+		binary.BigEndian.PutUint64(entityIDBuffer[14:], uint64(time.Now().UnixNano()))
+		binary.BigEndian.PutUint16(entityIDBuffer[22:], uint16(0))
+
+		datasetEntitiesLatestVersionKey := make([]byte, 14)
+		binary.BigEndian.PutUint16(datasetEntitiesLatestVersionKey, DatasetLatestEntities)
+		binary.BigEndian.PutUint32(datasetEntitiesLatestVersionKey[2:], env.ds.InternalID)
+		binary.BigEndian.PutUint64(datasetEntitiesLatestVersionKey[6:], person.InternalID)
+
+		txn := env.ds.store.database.NewTransaction(true)
+		jsonData, _ := json.Marshal(person)
+		err = txn.Set(entityIDBuffer, jsonData)
+		Expect(err).To(BeNil())
+		err = txn.Set(datasetEntitiesLatestVersionKey, entityIDBuffer)
+		Expect(err).To(BeNil())
+		txn.Commit()
+
+		// remove invalid ref, and make sure we can store the entity and overwrite the invalid ref
+		delete(person.References, env.peopleNamespacePrefix+":workedfor")
+		err = env.ds.StoreEntities([]*Entity{person})
+		Expect(err).To(BeNil())
+	})
 	ginkgo.It("Should use it's publicNamespaces property for context", func() {
 		// first part: test that we get global namespace if nothing is overridden
 		_, _ = env.store.NamespaceManager.AssertPrefixMappingForExpansion(