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.

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

[BREAKING_CHANGES] Fix update message payload #699

Merged
merged 6 commits into from
Apr 9, 2024

Conversation

qhenkart
Copy link
Contributor

Describe the change
Resolves #692
The ModifyMessage endpoint improperly wraps the payload. Including the expected map[string]any object into the method will result in an API error as the API requires it to be wrapped in "metadata".

This is a breaking change because to have successfully used this endpoint, developers would have run into this API error, debugged it and realized that the payload isn't being wrapped by the underlying code as expected by the API.

Provide OpenAI documentation link
https://platform.openai.com/docs/api-reference/messages/modifyMessage

Describe your solution
This simply wraps the payload as expected by the API and as assumed by the interface. The interface accepts a metadata object, either the interface needs to change to accept a generic payload object (which would be equally misleading). Or we update the underlying code to properly wrap the payload as expected

Tests
Unit tests

Additional context
We will need to inform that anyone who figured out how to use this endpoint will need to update their code to remove the manual wrapping of the payload.

Issue: #692

Alternatively I can avoid the breaking change by only wrapping the payload if it's not already wrapped. I think this will create complexity down the road, but it is an option

@qhenkart
Copy link
Contributor Author

I'm not sure why the Linter is breaking randomly. Did something change in the underlying build process? It doesn't look like it's due to any changes in this PR

golangci-lint run ./...
chat_test.go:330:1: cognitive complexity 30 of func `TestMultipartChatMessageSerialization` is high (> 20) (gocognit)
func TestMultipartChatMessageSerialization(t *testing.T) {
^
fine_tunes.go:118:2: directive `//nolint:goconst // Decreases readability` is unused for linter "goconst" (nolintlint)
	//nolint:goconst // Decreases readability
	^
messages.go:21:49: directive `//nolint:revive //backwards-compatibility` is unused for linter "revive" (nolintlint)
	FileIds     []string         `json:"file_ids"` //nolint:revive //backwards-compatibility
	                                               ^
messages.go:57:54: directive `//nolint:revive // backwards-compatibility` is unused for linter "revive" (nolintlint)
	FileIds  []string       `json:"file_ids,omitempty"` //nolint:revive // backwards-compatibility
	                                                    ^
run.go:27:54: directive `//nolint:revive // backwards-compatibility` is unused for linter "revive" (nolintlint)
	FileIDS        []string           `json:"file_ids"` //nolint:revive // backwards-compatibility

@qhenkart
Copy link
Contributor Author

Also worth noting. metadata should be a map[string]string. Non string types are not accepted and can also lead to an error. Would you like me to include that in this PR? It will lead to more breaking changes, but also enforce the requirements of the API which are currently unenforced and lead to error

@sashabaranov
Copy link
Owner

sashabaranov commented Apr 8, 2024

@qhenkart thank you for the PR! The master branch contains the fix for linter, you can merge/rebase it here so the sanity check would pass!

And yes, let's go with map[string]string!

@qhenkart
Copy link
Contributor Author

qhenkart commented Apr 9, 2024

@sashabaranov Okay thanks, this PR is ready for your final review

@sashabaranov sashabaranov merged commit 187f416 into sashabaranov:master Apr 9, 2024
1 check passed
jackdoe added a commit to rekki/go-openai that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ModifyMessage failed with error code 400, message: Unknown parameter
2 participants