defradb icon indicating copy to clipboard operation
defradb copied to clipboard

Panic if given an index with a name that already exists

Open shahzadlone opened this issue 1 year ago • 0 comments

Describe the problem There is a panic if given an index with a name that already exists.

To Reproduce

func TestIndexGet_ShouldReturnErrorIfGivenSameIndexName(t *testing.T) {
	test := testUtils.TestCase{
		Description: "Panic bug",
		Actions: []any{
			testUtils.SchemaUpdate{
				Schema: `
					type User @index(name: "age_index", fields: ["age"]) @index(name: "age_index", fields: ["age"]) {
						name: String @index(name: "name_index")
						age: Int
					}
				`,
			},
		},
	}

	testUtils.ExecuteTestCase(t, test)
}

Expected behavior Probably a more useful error like this one in the stack: index with name already exists. Name: age_index.

Stack trace

    utils2.go:1638:
                Error Trace:    /home/shahzad/~~/source/defradb/tests/integration/utils2.go:1638
                                                        /home/shahzad/~~/source/defradb/tests/integration/utils2.go:984
                                                        /home/shahzad/~~/source/defradb/tests/integration/utils2.go:258
                                                        /home/shahzad/~~/source/defradb/tests/integration/utils2.go:208
                                                        /home/shahzad/~~/source/defradb/tests/integration/utils2.go:161
                                                        /home/shahzad/~~/source/defradb/tests/integration/index/create_get_test.go:34
                Error:          Received unexpected error:
                                index with name already exists. Name: age_index. Stack: /home/shahzad/~~/source/defradb/db/errors.go:401 (0x213f3c5)
                                        (*collection).generateIndexNameIfNeededAndCreateKey: return errors.New(
                                /home/shahzad/~~/source/defradb/db/collection_index.go:458 (0x213f347)
                                        (*collection).generateIndexNameIfNeededAndCreateKey: return core.CollectionIndexKey{}, NewErrIndexWithNameAlreadyExists(desc.Name)
                                /home/shahzad/~~/source/defradb/db/collection_index.go:203 (0x213bea5)
                                        (*collection).createIndex: indexKey, err := c.generateIndexNameIfNeededAndCreateKey(ctx, txn, &desc)
                                /home/shahzad/~~/source/defradb/db/collection.go:132 (0x212e40b)
                                        (*db).createCollection: if _, err := col.createIndex(ctx, txn, index); err != nil {
                                /home/shahzad/~~/source/defradb/db/schema.go:49 (0x2148e4c)
                                        (*db).addSchema: col, err := db.createCollection(ctx, txn, definition)
                                /home/shahzad/~~/source/defradb/db/txn_db.go:285 (0x214d90d)
                                        (*implicitTxnDB).AddSchema: cols, err := db.addSchema(ctx, txn, schemaString)
                                /home/shahzad/~~/source/defradb/tests/integration/utils2.go:983 (0x218003b)
                                        updateSchema: _, err := node.AddSchema(s.ctx, action.Schema)
                                /home/shahzad/~~/source/defradb/tests/integration/utils2.go:258 (0x217a365)
                                        performAction: updateSchema(s, action)
                                /home/shahzad/~~/source/defradb/tests/integration/utils2.go:208 (0x2179408)
                                        executeTestCase: performAction(s, i, testCase.Actions[i])
                                /home/shahzad/~~/source/defradb/tests/integration/utils2.go:161 (0x2178a33)
                                        ExecuteTestCase: executeTestCase(ctx, t, collectionNames, testCase, dbt, ct)
                                /home/shahzad/~~/source/defradb/tests/integration/index/create_get_test.go:34 (0x2191a29)
                                        TestIndexGet_ShouldReturnErrorIfGivenSameIndexName: testUtils.ExecuteTestCase(t, test)
                                /home/shahzad/.local/bin/go1.21.6/src/testing/testing.go:1595 (0x5a843f)
                                        tRunner: fn(t)
                                /home/shahzad/.local/bin/go1.21.6/src/runtime/asm_amd64.s:1650 (0x4e3b41)
                                        goexit: BYTE    $0x90   // NOP
                Test:           TestIndexGet_ShouldReturnErrorIfGivenSameIndexName
                Messages:       Getting indexes should return list of existing indexes
2024-01-23T05:43:41.238-0500, INFO, db, Closing DefraDB process...
2024-01-23T05:43:41.241-0500, INFO, db, Successfully closed running process
--- FAIL: TestIndexGet_ShouldReturnErrorIfGivenSameIndexName (0.01s)
FAIL
FAIL    github.com/sourcenetwork/defradb/tests/integration/index        0.113s
FAIL

Platform Most recent (HEAD) on develop, on my WSL2 machine.

Additional context I am not sure if there is a test, but should probably test the case if not already where similar to the above multiple index directives are provided and the index names are different, for example:

func TestIndexGet_ShouldReturnErrorIfGivenMultipleIndexNames(t *testing.T) {
	test := testUtils.TestCase{
		Description: "Does not panic and creates an extra element in list B",
		Actions: []any{
			testUtils.SchemaUpdate{
				Schema: `
					type User @index(name: "age_index", fields: ["age"]) @index(name: "age2_index", fields: ["age"]) {
						name: String @index(name: "name_index")
						age: Int
					}
				`,
			},
		},
	}

	testUtils.ExecuteTestCase(t, test)
}

Note the above creates extra elements in list B, which is expected I am guessing @islamaliev ?

Because in future we will be adding more directives like @policy for the acp stuff like:

func TestIndexGet_IndexOnFieldAndObjectWithPolicyOnObject(t *testing.T) {
	test := testUtils.TestCase{
		Description: "Index and Policy directive",
		Actions: []any{
			testUtils.SchemaUpdate{
				Schema: `
					type User @index(name: "age_index", fields: ["age"]) @policy(id: "pID123", resource: "user") {
						name: String @index(name: "name_index")
						age: Int
					}
				`,
			},
		},
	}

	testUtils.ExecuteTestCase(t, test)
}

I was hoping we could do something like this, but I am not sure if it's possible with GQL:

type User @index(name: "age_index", fields: ["age"]) {
	name: String @index(name: "name_index")
	age: Int
} @policy(id: "pID123", resource: "user") 

shahzadlone avatar Jan 23 '24 10:01 shahzadlone