[ENH]: Make error messages in attach_function clearer (#5973)

## Description of changes

_Summarize the changes made by this PR._

- Improvements & Bug fixes
    - Errors fixed:
        - Param validation
        - Output collection already exists error looks nicer now
        - Function already exists error looks cleaner
        - Invalid function to be attached error added
        - Collection already has an attached function
- New functionality
    - ...

## Test plan

_How are these changes tested?_

- [ ] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust

## Migration plan

_Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?_

## Observability plan

_What is the plan to instrument and monitor this change?_

## Documentation Changes

_Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the_ [_docs section](https://github.com/chroma-core/chroma/tree/main/docs/docs.trychroma.com)?_

Author: tanujnay112

chromadb/test/distributed/test_task_api.py

@@ -7,7 +7,11 @@
 
 import pytest
 from chromadb.api.client import Client as ClientCreator
-from chromadb.api.functions import RECORD_COUNTER_FUNCTION, Function
+from chromadb.api.functions import (
+    RECORD_COUNTER_FUNCTION,
+    STATISTICS_FUNCTION,
+    Function,
+)
 from chromadb.config import System
 from chromadb.errors import ChromaError, NotFoundError
 from chromadb.test.utils.wait_for_version_increase import (
@@ -181,18 +185,24 @@ 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(
             function=RECORD_COUNTER_FUNCTION,
             name="task_2",
             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(
-            function=RECORD_COUNTER_FUNCTION,
+            function=STATISTICS_FUNCTION,
             name="task_1",
             output_collection="output_different",  # Different output collection
             params=None,
@@ -222,6 +232,55 @@ def test_functions_one_attached_function_per_collection(
     )
 
 
+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=RECORD_COUNTER_FUNCTION,
+            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=RECORD_COUNTER_FUNCTION,
+            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)

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) {