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.

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

fix(response-transformer): fix the error when response-transformer #9463

Merged
merged 2 commits into from
Nov 4, 2022
Merged

fix(response-transformer): fix the error when response-transformer #9463

merged 2 commits into from
Nov 4, 2022

Conversation

liverpool8056
Copy link
Contributor

Summary

Fix the error that the response from upstream is a plain string with Header Content-Type: application/json, the response transformer throw exception when transforming.

@liverpool8056 liverpool8056 requested a review from a team as a code owner September 23, 2022 07:38
@chronolaw
Copy link
Contributor

chronolaw commented Oct 10, 2022 •
edited
Loading

It seems to be duplicated with PR #9491, could you take a look at it?

@chronolaw chronolaw mentioned this pull request Oct 10, 2022
Closed
@liverpool8056
Copy link
Contributor Author

liverpool8056 commented Oct 10, 2022 •
edited
Loading

@chronolaw I've checked PR 9491, it has a similar logic with this, except that in #9491, when transforming a non-json body, it will throw a warn-level log indicating the failure, while this one won't.

@chronolaw
Copy link
Contributor

@chronolaw I've checked PR 9491, it has a similar logic with this, except that in #9491, when transforming a non-json body, it will throw a warn-level log indicating the failure, while this one won't.

Great. Could you talk to @samugi and integrate these two PRs?

samugi
samugi reviewed Oct 10, 2022
View reviewed changes
kong/plugins/response-transformer/body_transformer.lua Outdated
if json_body == nil then
return
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil
return nil, "failed parsing json body"

I think it would be useful to return a warning, then log it in case of parsing failure

@samugi
Copy link
Member

samugi commented Oct 10, 2022

Thank you @chronolaw for spotting the duplicate. I think adding the warning would be useful, then we can close mine.

@liverpool8056 liverpool8056 force-pushed the fix/FTI-4352-response-transformer-for-plain-text branch 2 times, most recently from 28e8bdf to 498278d Compare October 17, 2022 06:03
@liverpool8056 liverpool8056 force-pushed the fix/FTI-4352-response-transformer-for-plain-text branch from 498278d to aa2e1e3 Compare October 17, 2022 07:14
@samugi
Copy link
Member

samugi commented Oct 20, 2022

I think it would be a good idea to cover the behaviour of body_filter too in the tests, when the "json" body is invalid, something similar to https://github.com/Kong/kong/pull/9491/files#diff-6f3111eafa8c99e31fe6715e0bbd608a9e5685ff7595ea3c4eafce0f527955c5R327-R389

@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 21, 2022
@liverpool8056 liverpool8056 force-pushed the fix/FTI-4352-response-transformer-for-plain-text branch from 850562e to e93bf9e Compare October 28, 2022 05:59
@samugi
Copy link
Member

samugi commented Nov 2, 2022

I think it would be a good idea to cover the behaviour of body_filter too in the tests, when the "json" body is invalid, something similar to https://github.com/Kong/kong/pull/9491/files#diff-6f3111eafa8c99e31fe6715e0bbd608a9e5685ff7595ea3c4eafce0f527955c5R327-R389

@liverpool8056 could you add this too? ^

@liverpool8056
Copy link
Contributor Author

@samugi It seems that I've already added a similar test.
https://github.com/Kong/kong/pull/9463/files#diff-6f3111eafa8c99e31fe6715e0bbd608a9e5685ff7595ea3c4eafce0f527955c5

@samugi
Copy link
Member

samugi commented Nov 2, 2022 •
edited
Loading

@samugi It seems that I've already added a similar test. https://github.com/Kong/kong/pull/9463/files#diff-6f3111eafa8c99e31fe6715e0bbd608a9e5685ff7595ea3c4eafce0f527955c5

@liverpool8056 The current unit test covers the transform_json_body() function, which is good, but it is not testing the correct behaviour of handler:body_filter() which was the source of this issue.

@liverpool8056 liverpool8056 force-pushed the fix/FTI-4352-response-transformer-for-plain-text branch from e93bf9e to 5787c05 Compare November 3, 2022 05:06
@liverpool8056
fix(response-transformer): fix the bug that response-transformer
3fcddb6 
plugin breaks when receiving an unexcepted body.
@liverpool8056 liverpool8056 force-pushed the fix/FTI-4352-response-transformer-for-plain-text branch from 5787c05 to 3fcddb6 Compare November 3, 2022 05:18
@liverpool8056
Copy link
Contributor Author

@samugi understood, thanks!

@fffonion fffonion merged commit c8d1f85 into master Nov 4, 2022
@fffonion fffonion deleted the fix/FTI-4352-response-transformer-for-plain-text branch November 4, 2022 06:42
@chronolaw chronolaw mentioned this pull request Nov 4, 2022
Merged
dndx pushed a commit that referenced this pull request Nov 4, 2022
@chronolaw
docs(changelog): fix typo for #9463's entry
aabf63f
@samugi samugi linked an issue Nov 7, 2022 that may be closed by this pull request
Plugins | Response transformer broken on Kong 2.8.1.4 #9461
Closed
1 task
@samugi samugi mentioned this pull request Nov 7, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samugi samugi samugi approved these changes

@chronolaw chronolaw Awaiting requested review from chronolaw

Assignees
No one assigned
Labels
changelog plugins/response-transformer size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Plugins | Response transformer broken on Kong 2.8.1.4
4 participants
@liverpool8056 @chronolaw @samugi @fffonion