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

fix: 修复4v请求下text参数缺少引发的nil #1634

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

ye4293
Copy link
Collaborator

@ye4293 ye4293 commented Jul 10, 2024

close #1633

我已确认该 PR 已自测通过,相关截图如下:
(此处放上测试通过的截图,如果不涉及前端改动或从 UI 上无法看出,请放终端启动成功的截图)
image
image
现在不会引发nil了,会最后直接把错错误抛出

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 1.21%. Comparing base (6ad1699) to head (adbb5cc).

Files Patch % Lines
relay/adaptor/openai/token.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main   #1634      +/-   ##
========================================
- Coverage   1.22%   1.21%   -0.01%     
========================================
  Files        137     137              
  Lines       9917    9919       +2     
========================================
  Hits         121     121              
- Misses      9782    9784       +2     
  Partials      14      14              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Laisky
Copy link
Collaborator

Laisky commented Jul 11, 2024

这是一次兼容性更新,防止 request 中 message.type == "text" 但是 message.text 的类型不为 string 导致程序错误?

一个小建议,发现 message.text 不合规时将其置空会不会更好?防止发送一些乱七八糟的东西给上游导致被意外扣费。

@ye4293
Copy link
Collaborator Author

ye4293 commented Jul 11, 2024 via email

@Laisky
Copy link
Collaborator

Laisky commented Jul 11, 2024

不是,是缺失text这个参数,导致获取不到就是nil了。text参数可以为空,但是不能不传 Laisky.Cai @.> 于2024年7月11日周四 10:23写道:

这是一次兼容性更新,防止 request 中 message.type == "text" 但是 message.text 的类型不为 string 导致程序错误? — Reply to this email directly, view it on GitHub <#1634 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAVUTO5QISQJJBLK6ONA4KTZLXUA3AVCNFSM6AAAAABKVDPDP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRRHA3DGNJZGQ . You are receiving this because you authored the thread.Message ID: @.
>

过去的行为是报错,现在的行为是格式合法才计费,不合法就原样传递给上游。

我觉得为了保持兼容性,不合法的时候将 text 置空是不是更好?

@ye4293
Copy link
Collaborator Author

ye4293 commented Jul 11, 2024

主要是和openai的保持一致,所以我采取的是保留报错。主要是想看看大家的意见,如果觉得置空更方便当热也可以

@Laisky
Copy link
Collaborator

Laisky commented Jul 11, 2024

主要是和openai的保持一致,所以我采取的是保留报错。主要是想看看大家的意见,如果觉得置空更方便当热也可以

以前不合法的内容不会发给上游,现在改完后会发给上游了。

优点是能拿到上游的报错信息,缺点是无法预判上游会如何处理这些信息,万一上游接受了并计费,就 bypass 了 one-api 的计费了。不过我觉得目前应该问题不大,以后有问题再改吧…

Ps. 这个 codecov/patch CI,不 pass 是不是不能合

Copy link
Collaborator

@Laisky Laisky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@songquanpeng
Copy link
Owner

patch的检查我之前已经去掉了

@songquanpeng songquanpeng merged commit 65acb94 into songquanpeng:main Jul 13, 2024
3 of 4 checks passed
@songquanpeng
Copy link
Owner

thx 各位

jinjianming pushed a commit to jinjianming/one-api that referenced this pull request Jul 17, 2024
@ye4293 ye4293 deleted the test branch August 31, 2024 06:20
mxdlzg pushed a commit to mxdlzg/one-api that referenced this pull request Oct 15, 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.

4v格式的请求下,缺少“text”参数程序崩溃会报错nil
4 participants