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(response-rewrite): rewrite response code using ctx.var #9633

Closed
wants to merge 1 commit into from

Conversation

zhendongcmss
Copy link
Contributor

@zhendongcmss zhendongcmss commented Jun 9, 2023

Description

I find that if I use logger plugin(clickhouse-logger) to send the access logs, the http code doesn't rewrited by response-rewrite plugin, because log-util.lua use ctx.var get the status not ngx.status.

https://github.com/apache/apisix/blob/master/apisix/utils/log-util.lua#L63

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)

@zhendongcmss zhendongcmss changed the title fix(response-rewrite): rewrite response code fix(response-rewrite): rewrite response code using ctx.var Jun 9, 2023
@Sn0rt
Copy link
Contributor

Sn0rt commented Jun 12, 2023

Are existing tests covered?

@zhendongcmss
Copy link
Contributor Author

zhendongcmss commented Jun 12, 2023

Are existing tests covered?

There no clickhouse DB, it is hard to add test case. Do you have idea ?

@pixeldin
Copy link
Contributor

Are existing tests covered?

There no clickhouse DB, it is hard to add test case. Do you have idea ?

You may running the unit test code with test-nginx framework refer to this:
https://github.com/apache/apisix/blob/dcb315bca4617cf76b9a6d2a0f2b62612d535a58/t/plugin/clickhouse-logger2.t#LL36C27-L36C27

@monkeyDluffy6017
Copy link
Contributor

Hi @zhendongcmss, it looks that your test cases have failed

@monkeyDluffy6017 monkeyDluffy6017 added wait for update wait for the author's response in this issue/PR and removed need test cases labels Jun 27, 2023
@zhendongcmss
Copy link
Contributor Author

Hi @zhendongcmss, it looks that your test cases have failed

updated

@zhendongcmss zhendongcmss force-pushed the response-rewrite branch 2 times, most recently from f6e69fb to 35891e7 Compare June 29, 2023 06:48
@monkeyDluffy6017
Copy link
Contributor

@zhendongcmss still failed

@monkeyDluffy6017
Copy link
Contributor

@zhendongcmss Please make the ci pass

@monkeyDluffy6017
Copy link
Contributor

@leslie-tsang please help to fix this pr

Copy link

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 24, 2023
Copy link

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale wait for update wait for the author's response in this issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants