[ENH]: Make error messages in attach_function better

Author: tanujnay112

chromadb/test/distributed/test_task_api.py

@@ -173,19 +173,25 @@ def test_functions_one_attached_function_per_collection(
 
     # Attempt to create a second task with a different name should fail
     # (only one attached function allowed per collection)
-    with pytest.raises(ChromaError, match="already has an attached function"):
+    with pytest.raises(
+        ChromaError,
+        match="collection already has an attached function: name=task_1, function=record_counter, output_collection=output_1",
+    ):
         collection.attach_function(
             name="task_2",
             function_id="record_counter",
             output_collection="output_2",
             params=None,
         )
 
-    # Attempt to create a task with the same name but different params should also fail
-    with pytest.raises(ChromaError, match="already exists"):
+    # Attempt to create a task with the same name but different function_id should also fail
+    with pytest.raises(
+        ChromaError,
+        match=r"collection already has an attached function: name=task_1, function=record_counter, output_collection=output_1",
+    ):
         collection.attach_function(
             name="task_1",
-            function_id="record_counter",
+            function_id="statistics",
             output_collection="output_different",  # Different output collection
             params=None,
         )
@@ -208,6 +214,55 @@ def test_functions_one_attached_function_per_collection(
     assert attached_fn2.detach(delete_output_collection=True) is True
 
 
+def test_attach_function_with_invalid_params(basic_http_client: System) -> None:
+    """Test that attach_function with non-empty params raises an error"""
+    client = ClientCreator.from_system(basic_http_client)
+    client.reset()
+
+    collection = client.create_collection(name="test_invalid_params")
+    collection.add(ids=["id1"], documents=["test document"])
+
+    # Attempt to create task with non-empty params should fail
+    # (no functions currently accept parameters)
+    with pytest.raises(
+        ChromaError,
+        match="params must be empty - no functions currently accept parameters",
+    ):
+        collection.attach_function(
+            name="invalid_params_task",
+            function_id="record_counter",
+            output_collection="output_collection",
+            params={"some_key": "some_value"},
+        )
+
+
+def test_attach_function_output_collection_already_exists(
+    basic_http_client: System,
+) -> None:
+    """Test that attach_function fails when output collection name already exists"""
+    client = ClientCreator.from_system(basic_http_client)
+    client.reset()
+
+    # Create a collection that will be used as input
+    input_collection = client.create_collection(name="input_collection")
+    input_collection.add(ids=["id1"], documents=["test document"])
+
+    # Create another collection with the name we want to use for output
+    client.create_collection(name="existing_output_collection")
+
+    # Attempt to create task with output collection name that already exists
+    with pytest.raises(
+        ChromaError,
+        match=r"Output collection \[existing_output_collection\] already exists",
+    ):
+        input_collection.attach_function(
+            name="my_task",
+            function_id="record_counter",
+            output_collection="existing_output_collection",
+            params=None,
+        )
+
+
 def test_function_remove_nonexistent(basic_http_client: System) -> None:
     """Test removing a task that doesn't exist raises NotFoundError"""
     client = ClientCreator.from_system(basic_http_client)
@@ -228,6 +283,7 @@ def test_function_remove_nonexistent(basic_http_client: System) -> None:
     with pytest.raises(NotFoundError, match="does not exist"):
         attached_fn.detach(delete_output_collection=True)
 
+
 def test_attach_to_output_collection_fails(basic_http_client: System) -> None:
     """Test that attaching a function to an output collection fails"""
     client = ClientCreator.from_system(basic_http_client)
@@ -245,7 +301,9 @@ def test_attach_to_output_collection_fails(basic_http_client: System) -> None:
     )
     output_collection = client.get_collection(name="output_collection")
 
-    with pytest.raises(ChromaError, match="cannot attach function to an output collection"):
+    with pytest.raises(
+        ChromaError, match="cannot attach function to an output collection"
+    ):
         _ = output_collection.attach_function(
             name="test_function_2",
             function_id="record_counter",
@@ -305,4 +363,4 @@ def test_delete_orphaned_output_collection(basic_http_client: System) -> None:
 
     with pytest.raises(NotFoundError):
         # Try to use the function - it should fail since it's detached
-        client.get_collection("output_collection")
+        client.get_collection("output_collection")

go/pkg/sysdb/coordinator/create_task_test.go

@@ -147,9 +147,7 @@ func (suite *AttachFunctionTestSuite) TestAttachFunction_SuccessfulCreation() {
 	MinRecordsForInvocation := uint64(100)
 
 	params := &structpb.Struct{
-		Fields: map[string]*structpb.Value{
-			"param1": structpb.NewStringValue("value1"),
-		},
+		Fields: map[string]*structpb.Value{},
 	}
 
 	request := &coordinatorpb.AttachFunctionRequest{
@@ -241,7 +239,7 @@ func (suite *AttachFunctionTestSuite) TestAttachFunction_IdempotentRequest_Alrea
 	tenantID := "test-tenant"
 	databaseName := "test-database"
 	databaseID := "database-uuid"
-	functionID := uuid.New()
+	functionID := dbmodel.FunctionRecordCounter
 	MinRecordsForInvocation := uint64(100)
 
 	params := &structpb.Struct{
@@ -288,10 +286,8 @@ func (suite *AttachFunctionTestSuite) TestAttachFunction_IdempotentRequest_Alrea
 			suite.mockAttachedFunctionDb.On("GetByCollectionID", inputCollectionID).
 				Return([]*dbmodel.AttachedFunction{existingAttachedFunction}, nil).Once()
 
-			// Validate function by ID
-			suite.mockMetaDomain.On("FunctionDb", txCtx).Return(suite.mockFunctionDb).Once()
-			suite.mockFunctionDb.On("GetByID", functionID).
-				Return(&dbmodel.Function{ID: functionID, Name: functionName}, nil).Once()
+			// Note: validateAttachedFunctionMatchesRequest uses dbmodel.GetFunctionNameByID (static lookup),
+			// not FunctionDb.GetByID, so no mock needed for FunctionDb here
 
 			// Validate database matches
 			suite.mockMetaDomain.On("DatabaseDb", txCtx).Return(suite.mockDatabaseDb).Once()
@@ -317,7 +313,6 @@ func (suite *AttachFunctionTestSuite) TestAttachFunction_IdempotentRequest_Alrea
 	// Verify all read mocks were called
 	suite.mockMetaDomain.AssertExpectations(suite.T())
 	suite.mockAttachedFunctionDb.AssertExpectations(suite.T())
-	suite.mockFunctionDb.AssertExpectations(suite.T())
 	suite.mockDatabaseDb.AssertExpectations(suite.T())
 }
 
@@ -339,16 +334,10 @@ func (suite *AttachFunctionTestSuite) TestAttachFunction_RecoveryFlow() {
 	tenantID := "test-tenant"
 	databaseName := "test-database"
 	databaseID := "database-uuid"
-	functionID := uuid.New()
+	functionID := dbmodel.FunctionRecordCounter
 	MinRecordsForInvocation := uint64(100)
 	now := time.Now()
 
-	params := &structpb.Struct{
-		Fields: map[string]*structpb.Value{
-			"param1": structpb.NewStringValue("value1"),
-		},
-	}
-
 	request := &coordinatorpb.AttachFunctionRequest{
 		Name:                    attachedFunctionName,
 		InputCollectionId:       inputCollectionID,
@@ -357,7 +346,6 @@ func (suite *AttachFunctionTestSuite) TestAttachFunction_RecoveryFlow() {
 		TenantId:                tenantID,
 		Database:                databaseName,
 		MinRecordsForInvocation: MinRecordsForInvocation,
-		Params:                  params,
 	}
 
 	// ========== FIRST ATTEMPT: Heap Push Fails ==========
@@ -423,10 +411,8 @@ func (suite *AttachFunctionTestSuite) TestAttachFunction_RecoveryFlow() {
 			suite.mockAttachedFunctionDb.On("GetByCollectionID", inputCollectionID).
 				Return([]*dbmodel.AttachedFunction{incompleteAttachedFunction}, nil).Once()
 
-			// Validate function matches
-			suite.mockMetaDomain.On("FunctionDb", txCtx).Return(suite.mockFunctionDb).Once()
-			suite.mockFunctionDb.On("GetByID", functionID).
-				Return(&dbmodel.Function{ID: functionID, Name: functionName}, nil).Once()
+			// Note: validateAttachedFunctionMatchesRequest uses dbmodel.GetFunctionNameByID (static lookup),
+			// not FunctionDb.GetByID, so no mock needed for FunctionDb here
 
 			// Validate database matches
 			suite.mockMetaDomain.On("DatabaseDb", txCtx).Return(suite.mockDatabaseDb).Once()
@@ -467,7 +453,7 @@ func (suite *AttachFunctionTestSuite) TestAttachFunction_IdempotentRequest_Param
 	tenantID := "test-tenant"
 	databaseName := "test-database"
 	databaseID := "database-uuid"
-	existingOperatorID := uuid.New()
+	existingOperatorID := dbmodel.FunctionRecordCounter
 	MinRecordsForInvocation := uint64(100)
 	now := time.Now()
 
@@ -514,31 +500,22 @@ func (suite *AttachFunctionTestSuite) TestAttachFunction_IdempotentRequest_Param
 			suite.mockAttachedFunctionDb.On("GetByCollectionID", inputCollectionID).
 				Return([]*dbmodel.AttachedFunction{existingAttachedFunction}, nil).Once()
 
-			// Validate function - returns DIFFERENT function name
-			suite.mockMetaDomain.On("FunctionDb", txCtx).Return(suite.mockFunctionDb).Once()
-			suite.mockFunctionDb.On("GetByID", existingOperatorID).
-				Return(&dbmodel.Function{
-					ID:   existingOperatorID,
-					Name: existingOperatorName, // Different from request
-				}, nil).Once()
-
-			// Database lookup happens before the error is returned
-			suite.mockMetaDomain.On("DatabaseDb", txCtx).Return(suite.mockDatabaseDb).Once()
-			suite.mockDatabaseDb.On("GetDatabases", tenantID, databaseName).
-				Return([]*dbmodel.Database{{ID: databaseID, Name: databaseName}}, nil).Once()
+			// Note: validateAttachedFunctionMatchesRequest uses dbmodel.GetFunctionNameByID (static lookup)
+			// which returns "record_counter" for FunctionRecordCounter - different from requestedOperatorName
+			// Validation returns (false, nil) early, so DatabaseDb.GetDatabases is NOT called
 
 			_ = txFunc(txCtx)
-		}).Return(status.Errorf(codes.AlreadyExists, "different function is attached with this name: existing=%s, requested=%s", existingOperatorName, requestedOperatorName)).Once()
+		}).Return(status.Errorf(codes.AlreadyExists, "collection already has an attached function: name=%s, function=%s, output_collection=%s", attachedFunctionName, existingOperatorName, outputCollectionName)).Once()
 
 	// Execute AttachFunction
 	response, err := suite.coordinator.AttachFunction(ctx, request)
 
 	// Assertions - should fail with AlreadyExists error
 	suite.Error(err)
 	suite.Nil(response)
-	suite.Contains(err.Error(), "different function is attached with this name")
+	suite.Contains(err.Error(), "collection already has an attached function")
 	suite.Contains(err.Error(), existingOperatorName)
-	suite.Contains(err.Error(), requestedOperatorName)
+	suite.Contains(err.Error(), outputCollectionName)
 
 	// Verify no writes occurred (Transaction IS called but Insert/Push are not)
 	suite.mockTxImpl.AssertNumberOfCalls(suite.T(), "Transaction", 1)
@@ -547,7 +524,6 @@ func (suite *AttachFunctionTestSuite) TestAttachFunction_IdempotentRequest_Param
 	// Verify read mocks were called
 	suite.mockMetaDomain.AssertExpectations(suite.T())
 	suite.mockAttachedFunctionDb.AssertExpectations(suite.T())
-	suite.mockFunctionDb.AssertExpectations(suite.T())
 }
 
 func TestAttachFunctionTestSuite(t *testing.T) {