Skip to content

fix: The events array after splitting may contain empty strings, w… #12155

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: The events array after splitting may contain empty strings, w… #12155

wants to merge 3 commits into from

Conversation

vislee
Copy link

@vislee vislee commented Apr 17, 2025

Description

The ctx.ai_token_usage may not get the correct value.
The events array after splitting may contain empty strings, which can cause subsequent event to be skipped.

Which issue(s) this PR fixes:

Fixes #

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@vislee
bugfix: The events array after splitting may contain empty strings, w…
e07fc30 
…hich can cause subsequent messages to be skipped.
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working labels Apr 17, 2025
@shreemaan-abhishek
Copy link
Contributor

please add test cases too.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Apr 21, 2025
Baoyuantop
Baoyuantop reviewed Apr 22, 2025
View reviewed changes
Copy link
Contributor

@Baoyuantop Baoyuantop left a comment

Choose a reason for hiding this comment

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

Please fix failed lint CI

@Baoyuantop
Copy link
Contributor

Hi @vislee, is there time to continue with this PR?

@Baoyuantop Baoyuantop added the wait for update wait for the author's response in this issue/PR label Apr 29, 2025
@vislee vislee changed the title bugfix: The events array after splitting may contain empty strings, w… fix: The events array after splitting may contain empty strings, w… May 24, 2025
@vislee
Copy link
Author

vislee commented May 24, 2025

I've pushed the fix failed lint CI.
Btw, how to run tests locally, Are there docker images?

Hi @vislee, is there time to continue with this PR?

@Baoyuantop
Copy link
Contributor

You can refer to https://github.com/apache/apisix/blob/master/docs/en/latest/build-apisix-dev-environment-devcontainers.md

@Baoyuantop Baoyuantop removed wait for update wait for the author's response in this issue/PR user responded labels Jun 16, 2025
@Baoyuantop
Copy link
Contributor

Hi @vislee, any updates?

@Baoyuantop Baoyuantop added the wait for update wait for the author's response in this issue/PR label Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Baoyuantop Baoyuantop Baoyuantop left review comments

At least 3 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files. wait for update wait for the author's response in this issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vislee @shreemaan-abhishek @Baoyuantop