Repository: sashabaranov/go-openai
Pull request #685 (2024-03-14 07:15:02)

Fix broken implementation AssistantModify implementation
This resolves https://github.com/sashabaranov/go-openai/issues/684 
both reverts the changes in https://github.com/sashabaranov/go-openai/pull/669
and modifies the code to actually match the intention of the https://github.com/sashabaranov/go-openai/pull/669

Currently the SDK does not match the API and requires all of the existing tools to be provided in every request to avoid an API error or deleting all of the tools. Please see the linked issue for repro steps and use cases including step by step cURL requests to demonstrate the existing functionality of the API

As specified in the inline documentation
A custom marshaller was required here to handle the various use cases of the API request with respect to GO-isms and to avoid breaking changes:

If Tools is nil (or undefined), the field is omitted from the JSON. > this reflects the functionality of the API and reverts the breaking change introduced in https://github.com/sashabaranov/go-openai/pull/669

If Tools is an empty slice, it's included in the JSON as an empty array ([]). > this matches the intention of https://github.com/sashabaranov/go-openai/pull/669 by allowing the ability to remove all Tools, something that was broken previously and the linked PR attempted to resolve
If Tools is populated, it's included in the JSON with the elements > and thusly replaces the Tools. This functionality existed already and simply is maintained in this PR

This is important to note because in Golang, omitempty will omit both a nil (uninitialized) slice as well as an empty slice. To provide support to allow both a non-existant slice, and an empty slice, and a populated slice and never a nil slice (see the linked issue to see the API error if it receives nil). A custom Marshaller and aliast was required

Additionally I wrapped the tests block into Run blocks to be able to isolate tests and remove variable shadowing. I also added explicit unit tests for this functionality, and the run blocks were necessary to be able to properly isolate them for testing and development. 


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/sashabaranov/go-openai/pull/685?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 98.46%. Comparing base [(`699f397`)](https://app.codecov.io/gh/sashabaranov/go-openai/commit/699f397c36d05e42210f65456436a447885cc07a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) to head [(`5bfa45b`)](https://app.codecov.io/gh/sashabaranov/go-openai/pull/685?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #685      +/-   ##
==========================================
+ Coverage   98.44%   98.46%   +0.01%     
==========================================
  Files          24       24              
  Lines        1353     1366      +13     
==========================================
+ Hits         1332     1345      +13     
  Misses         15       15              
  Partials        6        6              
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/sashabaranov/go-openai/pull/685?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).


Patch:
diff --git a/assistant.go b/assistant.go
index 7a7a765..4ca2dda 100644
--- a/assistant.go
+++ b/assistant.go
@@ -2,6 +2,7 @@ package openai
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -21,7 +22,7 @@ type Assistant struct {
 	Description  *string         `json:"description,omitempty"`
 	Model        string          `json:"model"`
 	Instructions *string         `json:"instructions,omitempty"`
-	Tools        []AssistantTool `json:"tools,omitempty"`
+	Tools        []AssistantTool `json:"tools"`
 	FileIDs      []string        `json:"file_ids,omitempty"`
 	Metadata     map[string]any  `json:"metadata,omitempty"`
 
@@ -41,16 +42,41 @@ type AssistantTool struct {
 	Function *FunctionDefinition `json:"function,omitempty"`
 }
 
+// AssistantRequest provides the assistant request parameters.
+// When modifying the tools the API functions as the following:
+// If Tools is undefined, no changes are made to the Assistant's tools.
+// If Tools is empty slice it will effectively delete all of the Assistant's tools.
+// If Tools is populated, it will replace all of the existing Assistant's tools with the provided tools.
 type AssistantRequest struct {
 	Model        string          `json:"model"`
 	Name         *string         `json:"name,omitempty"`
 	Description  *string         `json:"description,omitempty"`
 	Instructions *string         `json:"instructions,omitempty"`
-	Tools        []AssistantTool `json:"tools"`
+	Tools        []AssistantTool `json:"-"`
 	FileIDs      []string        `json:"file_ids,omitempty"`
 	Metadata     map[string]any  `json:"metadata,omitempty"`
 }
 
+// MarshalJSON provides a custom marshaller for the assistant request to handle the API use cases
+// If Tools is nil, the field is omitted from the JSON.
+// If Tools is an empty slice, it's included in the JSON as an empty array ([]).
+// If Tools is populated, it's included in the JSON with the elements.
+func (a AssistantRequest) MarshalJSON() ([]byte, error) {
+	type Alias AssistantRequest
+	assistantAlias := &struct {
+		Tools *[]AssistantTool `json:"tools,omitempty"`
+		*Alias
+	}{
+		Alias: (*Alias)(&a),
+	}
+
+	if a.Tools != nil {
+		assistantAlias.Tools = &a.Tools
+	}
+
+	return json.Marshal(assistantAlias)
+}
+
 // AssistantsList is a list of assistants.
 type AssistantsList struct {
 	Assistants []Assistant `json:"data"`


Test patch:
diff --git a/assistant_test.go b/assistant_test.go
index 48bc6f9..40de0e5 100644
--- a/assistant_test.go
+++ b/assistant_test.go
@@ -96,7 +96,7 @@ When asked a question, write and run Python code to answer the question.`
 				})
 				fmt.Fprintln(w, string(resBytes))
 			case http.MethodPost:
-				var request openai.AssistantRequest
+				var request openai.Assistant
 				err := json.NewDecoder(r.Body).Decode(&request)
 				checks.NoError(t, err, "Decode error")
 
@@ -163,44 +163,97 @@ When asked a question, write and run Python code to answer the question.`
 
 	ctx := context.Background()
 
-	_, err := client.CreateAssistant(ctx, openai.AssistantRequest{
-		Name:         &assistantName,
-		Description:  &assistantDescription,
-		Model:        openai.GPT4TurboPreview,
-		Instructions: &assistantInstructions,
+	t.Run("create_assistant", func(t *testing.T) {
+		_, err := client.CreateAssistant(ctx, openai.AssistantRequest{
+			Name:         &assistantName,
+			Description:  &assistantDescription,
+			Model:        openai.GPT4TurboPreview,
+			Instructions: &assistantInstructions,
+		})
+		checks.NoError(t, err, "CreateAssistant error")
 	})
-	checks.NoError(t, err, "CreateAssistant error")
 
-	_, err = client.RetrieveAssistant(ctx, assistantID)
-	checks.NoError(t, err, "RetrieveAssistant error")
+	t.Run("retrieve_assistant", func(t *testing.T) {
+		_, err := client.RetrieveAssistant(ctx, assistantID)
+		checks.NoError(t, err, "RetrieveAssistant error")
+	})
 
-	_, err = client.ModifyAssistant(ctx, assistantID, openai.AssistantRequest{
-		Name:         &assistantName,
-		Description:  &assistantDescription,
-		Model:        openai.GPT4TurboPreview,
-		Instructions: &assistantInstructions,
+	t.Run("delete_assistant", func(t *testing.T) {
+		_, err := client.DeleteAssistant(ctx, assistantID)
+		checks.NoError(t, err, "DeleteAssistant error")
 	})
-	checks.NoError(t, err, "ModifyAssistant error")
 
-	_, err = client.DeleteAssistant(ctx, assistantID)
-	checks.NoError(t, err, "DeleteAssistant error")
+	t.Run("list_assistant", func(t *testing.T) {
+		_, err := client.ListAssistants(ctx, &limit, &order, &after, &before)
+		checks.NoError(t, err, "ListAssistants error")
+	})
 
-	_, err = client.ListAssistants(ctx, &limit, &order, &after, &before)
-	checks.NoError(t, err, "ListAssistants error")
+	t.Run("create_assistant_file", func(t *testing.T) {
+		_, err := client.CreateAssistantFile(ctx, assistantID, openai.AssistantFileRequest{
+			FileID: assistantFileID,
+		})
+		checks.NoError(t, err, "CreateAssistantFile error")
+	})
 
-	_, err = client.CreateAssistantFile(ctx, assistantID, openai.AssistantFileRequest{
-		FileID: assistantFileID,
+	t.Run("list_assistant_files", func(t *testing.T) {
+		_, err := client.ListAssistantFiles(ctx, assistantID, &limit, &order, &after, &before)
+		checks.NoError(t, err, "ListAssistantFiles error")
 	})
-	checks.NoError(t, err, "CreateAssistantFile error")
 
-	_, err = client.ListAssistantFiles(ctx, assistantID, &limit, &order, &after, &before)
-	checks.NoError(t, err, "ListAssistantFiles error")
+	t.Run("retrieve_assistant_file", func(t *testing.T) {
+		_, err := client.RetrieveAssistantFile(ctx, assistantID, assistantFileID)
+		checks.NoError(t, err, "RetrieveAssistantFile error")
+	})
 
-	_, err = client.RetrieveAssistantFile(ctx, assistantID, assistantFileID)
-	checks.NoError(t, err, "RetrieveAssistantFile error")
+	t.Run("delete_assistant_file", func(t *testing.T) {
+		err := client.DeleteAssistantFile(ctx, assistantID, assistantFileID)
+		checks.NoError(t, err, "DeleteAssistantFile error")
+	})
 
-	err = client.DeleteAssistantFile(ctx, assistantID, assistantFileID)
-	checks.NoError(t, err, "DeleteAssistantFile error")
+	t.Run("modify_assistant_no_tools", func(t *testing.T) {
+		assistant, err := client.ModifyAssistant(ctx, assistantID, openai.AssistantRequest{
+			Name:         &assistantName,
+			Description:  &assistantDescription,
+			Model:        openai.GPT4TurboPreview,
+			Instructions: &assistantInstructions,
+		})
+		checks.NoError(t, err, "ModifyAssistant error")
+
+		if assistant.Tools != nil {
+			t.Errorf("expected nil got %v", assistant.Tools)
+		}
+	})
+
+	t.Run("modify_assistant_with_tools", func(t *testing.T) {
+		assistant, err := client.ModifyAssistant(ctx, assistantID, openai.AssistantRequest{
+			Name:         &assistantName,
+			Description:  &assistantDescription,
+			Model:        openai.GPT4TurboPreview,
+			Instructions: &assistantInstructions,
+			Tools:        []openai.AssistantTool{{Type: openai.AssistantToolTypeFunction}},
+		})
+		checks.NoError(t, err, "ModifyAssistant error")
+
+		if assistant.Tools == nil || len(assistant.Tools) != 1 {
+			t.Errorf("expected a slice got %v", assistant.Tools)
+		}
+	})
+
+	t.Run("modify_assistant_empty_tools", func(t *testing.T) {
+		assistant, err := client.ModifyAssistant(ctx, assistantID, openai.AssistantRequest{
+			Name:         &assistantName,
+			Description:  &assistantDescription,
+			Model:        openai.GPT4TurboPreview,
+			Instructions: &assistantInstructions,
+			Tools:        make([]openai.AssistantTool, 0),
+		})
+
+		checks.NoError(t, err, "ModifyAssistant error")
+
+		if assistant.Tools == nil {
+			t.Errorf("expected a slice got %v", assistant.Tools)
+		}
+	})
 }
 
 func TestAzureAssistant(t *testing.T) {