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

Jump to bottom

feat: add support for custom document id field #1884

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: add support for custom document id field #1884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 embedded/document/document_id.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func NewDocumentIDFromHexEncodedString(hexEncodedDocID string) (DocumentID, erro

_, err := hex.Decode(buf, []byte(hexEncodedDocID))
if err != nil {
return nil, err
return nil, ErrInvalidHex
}

return NewDocumentIDFromRawBytes(buf)
Expand Down
4 changes: 2 additions & 2 deletions embedded/document/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,8 +716,8 @@ func (e *Engine) upsertDocuments(ctx context.Context, sqlTx *sql.SQLTx, collecti

provisionedDocID, docIDProvisioned := doc.Fields[docIDFieldName]
if docIDProvisioned {
if isInsert {
return 0, nil, fmt.Errorf("%w: field (%s) should NOT be specified when inserting a document", ErrIllegalArguments, docIDFieldName)
if isInsert && docIDFieldName == DefaultDocumentIDField {
Copy link
Contributor

Choose a reason for hiding this comment

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

if I read correctly, if inserted document does not have custom id, then immudb will autogenerate one. I would block that, as it is doing something by the user behind the scenes, so something that I would not expect when the user explicitly specifies that document id is provided externally. I.e. even if ids are just hex encoded strings at the end, they might have more meaning from the user perspective.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed. In case of missing custom ID provided by the user it will be auto-generated. This is similar strategy for example Elasticsearch uses. If user doesn't specify an identifier it is automatically generated. Each approach has pros and cons of course. In #1755 I mentioned that maybe it's not worth giving a user flexibility to provide document DB with custom ID field just force the name of ID field but let user customize the value. In such a case it's even more important to have auto-generated _id if not provided or use the one from the user if available.

return 0, nil, fmt.Errorf("%w: default document id field (%s) is autogenerated and should NOT be specified when inserting a document", ErrIllegalArguments, docIDFieldName)
}

docID, err = NewDocumentIDFromHexEncodedString(provisionedDocID.GetStringValue())
Expand Down
95 changes: 95 additions & 0 deletions embedded/document/engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1283,6 +1283,101 @@ func TestDocumentUpdate(t *testing.T) {
require.Len(t, revisions, 1)
})
}
func TestCustomDocumentID(t *testing.T) {
prog8 marked this conversation as resolved.
Show resolved Hide resolved
// Create a new engine instance
ctx := context.Background()
engine := makeEngine(t)

// Create a test collection with a single document
collectionName := "test_collection"
customIDField := "customID"

err := engine.CreateCollection(
context.Background(),
"admin",
collectionName,
"customID",
[]*protomodel.Field{
{Name: "name", Type: protomodel.FieldType_STRING},
{Name: "age", Type: protomodel.FieldType_DOUBLE},
},
[]*protomodel.Index{},
)
require.NoError(t, err)

t.Run("insert document with custom doc ID should pass", func(t *testing.T) {
_, docID, err := engine.InsertDocument(context.Background(), "admin", collectionName, &structpb.Struct{
Fields: map[string]*structpb.Value{
customIDField: structpb.NewStringValue("1234"),
"name": structpb.NewStringValue("Alice"),
"age": structpb.NewNumberValue(30),
},
})
require.NoError(t, err)
require.Equal(t, "1234", docID.EncodeToHexString())

// query document
query := &protomodel.Query{
CollectionName: collectionName,
Expressions: []*protomodel.QueryExpression{{
FieldComparisons: []*protomodel.FieldComparison{{
Field: customIDField, Operator: protomodel.ComparisonOperator_EQ, Value: structpb.NewStringValue("1234"),
}},
}},
}

reader, err := engine.GetDocuments(ctx, query, 0)
require.NoError(t, err)
defer reader.Close()

doc, err := reader.Read(ctx)
require.NoError(t, err)
// require.Equal(t, txID, doc.)
require.Equal(t, "1234", doc.Document.Fields[customIDField].GetStringValue())
})

t.Run("update document with custom doc ID", func(t *testing.T) {
txID, _, err := engine.InsertDocument(context.Background(), "admin", collectionName, &structpb.Struct{
Fields: map[string]*structpb.Value{
customIDField: structpb.NewStringValue("1235"),
"name": structpb.NewStringValue("Bob"),
"age": structpb.NewNumberValue(50),
},
})
require.NoError(t, err)

query := &protomodel.Query{
CollectionName: collectionName,
Expressions: []*protomodel.QueryExpression{{
FieldComparisons: []*protomodel.FieldComparison{{
Field: customIDField, Operator: protomodel.ComparisonOperator_EQ, Value: structpb.NewStringValue("1235"),
}},
}},
}

docs, err := engine.ReplaceDocuments(context.Background(), "admin", query, &structpb.Struct{
Fields: map[string]*structpb.Value{
customIDField: structpb.NewStringValue("1235"),
"age": structpb.NewNumberValue(51),
},
})
require.NoError(t, err)
require.Len(t, docs, 1)
require.Equal(t, txID+1, docs[0].TransactionId)
require.Equal(t, "1235", docs[0].DocumentId)
})

t.Run("insert document not hex should fail", func(t *testing.T) {
_, _, err := engine.InsertDocument(context.Background(), "admin", collectionName, &structpb.Struct{
Fields: map[string]*structpb.Value{
customIDField: structpb.NewStringValue("test_not_hex"),
"name": structpb.NewStringValue("Alice"),
"age": structpb.NewNumberValue(30),
},
})
require.Error(t, err, ErrInvalidHex)
})
}

func TestFloatSupport(t *testing.T) {
ctx := context.Background()
Expand Down
1 change: 1 addition & 0 deletions embedded/document/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ var (
ErrReservedName = errors.New("reserved name")
ErrLimitedIndexCreation = errors.New("unique index creation is only supported on empty collections")
ErrConflict = errors.New("conflict due to uniqueness contraint violation or read document was updated by another transaction")
ErrInvalidHex = errors.New("value is not valid hex, has to have even number of characters [0-9a-f]")
)

func mayTranslateError(err error) error {
Expand Down
Loading