-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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-transformer) initialize rt_body_chunks
and `rt_body_ch…
#3524
Conversation
…unk_number` if needed
-- did not run - and hence `rt_body_chunks` and `rt_body_chunk_number` | ||
-- were not initialized | ||
ctx.rt_body_chunks = ctx.rt_body_chunks or {} | ||
ctx.rt_body_chunk_number = ctx.rt_body_chunk_number or 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The access handler above does nothing but setting the defaults, so that entire handler can be removed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tieske access
handler removed
|
||
-- Initializes context here in case this plugin's access phase | ||
-- did not run - and hence `rt_body_chunks` and `rt_body_chunk_number` | ||
-- were not initialized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these comments are now outdated
@nateslo Could you bump the version to |
@@ -97,5 +116,19 @@ for _, strategy in helpers.each_strategy() do | |||
assert.equals([[this is a / test]], json.uri_args) | |||
end) | |||
end) | |||
|
|||
describe("unauthorized", function() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rename this block to "regressions"? Thanks
@@ -97,5 +116,19 @@ for _, strategy in helpers.each_strategy() do | |||
assert.equals([[this is a / test]], json.uri_args) | |||
end) | |||
end) | |||
|
|||
describe("unauthorized", function() | |||
it("doesn't err when unauthorized", function() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within this block, it'd be nice to include a link to the issue this regression test fixes just like https://github.com/Kong/kong/blob/master/spec-old-api/01-unit/010-router_spec.lua#L927
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also be more explicit in the name of this test:
it("does not throw an error when request was short-circuited in access phase")
When `ResponseTransformerHandler:access` is not executed, `ResponseTransformerHandler:body_filter` attempts to access `nil` `ctx.rt_body_chunks` and `nil` `ctx.rt_body_chunk_number`. This patch initially added checks for both and initialized them to defaults if `nil`, but now even gets rid of the `:access()` phase an lazily initializes `ngx.ctx` values in `:body_filter()`. Fix #3521 From #3524
Manually merged to master, thank you! |
Summary
When
ResponseTransformerHandler:access
is not executed,ResponseTransformerHandler:body_filter
attempts to access nilctx.rt_body_chunks
and nilctx.rt_body_chunk_number
. This PR adds checks for both and initializes them to defaults if nil.A test case has been added to verify there is no error if
ResponseTransformerHandler:access
is not executed.Full changelog
ctx.rt_body_chunks
andctx.rt_body_chunk_number
inResponseTransformerHandler:body_filter
ResponseTransformerHandler:access
)access
handler - ctx variables now initialized inbody_filter
Issues resolved
Fix #3521