Fix bug in export() applied to policies in the local chain #673
Conversation
davidor
force-pushed
the
fix-local-chain-export
branch
from
a4bb7bd to
21dba56
Compare
| @@ -45,6 +46,9 @@ local rewrite = _M.rewrite | |||
|
|
|||
| function _M:rewrite(context, ...) | |||
| build_chain(context) | |||
There was a problem hiding this comment.
Wouldn't it be nicer if build_chain would return the policy_chain object? Then we would not have to call find_policy_chain twice.
There was a problem hiding this comment.
You're right. It looks more readable. Changed in new commit.
| @@ -45,6 +46,9 @@ local rewrite = _M.rewrite | |||
|
|
|||
| function _M:rewrite(context, ...) | |||
| build_chain(context) | |||
|
|
|||
| context = LinkedList.readwrite(context, find_policy_chain(context):export()) | |||
There was a problem hiding this comment.
Shouldn't this call self:export() instead of find_local_policy_chain ?
There was a problem hiding this comment.
I don't see it. export is not defined in the local chain class.
There was a problem hiding this comment.
I see. Would it make sense to add it to:
https://github.com/3scale/apicast/blob/85a14f2b5cecb199b856264df1b2e2de6b4cd9da/gateway/src/apicast/policy.lua#L11-L17
Then we should be able to just call it and it would create proper delegator.
There was a problem hiding this comment.
I'm not sure. The case of export is a bit different from the rest of phases.
In the rest of phases, we just call that phase for each of the policies. export needs to build a list instead. Like in https://github.com/3scale/apicast/blob/869aa91a3e435cdb1f30cdc5fac698d3adba83f5/gateway/src/apicast/policy_chain.lua#L114
I wouldn't change this now.
There was a problem hiding this comment.
Only the PolicyChain merges those exports. The rest of policies just export something. I think that is exactly the same as the other phases. And if every policy would have an export function we could get rid of the or:
https://github.com/3scale/apicast/blob/869aa91a3e435cdb1f30cdc5fac698d3adba83f5/gateway/src/apicast/policy_chain.lua#L120
Making it easier for LuaJIT.
No need to change this. I just thought it would be even nicer cleanup to treat all phases the same.
There was a problem hiding this comment.
I'd leave this for a future PR.
Policies can define an export function to expose the data they need to share with other policies. export() worked fine for policies defined in the global chain, but it did not for policies include in a local (service level) chain. Whatever was returned in export() did not end up begin in the shared context. This commit solves the issue.
Makes the code a bit more readable.
davidor
force-pushed
the
fix-local-chain-export
branch
from
85a14f2 to
5bfeba3
Compare
|
Rebased to solve the changelog conflicts. |
Fixes #672