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(proxy-cache): allow nil ctx vars in cache key #7168

Merged
merged 1 commit into from
Jun 1, 2022
Merged

fix(proxy-cache): allow nil ctx vars in cache key #7168

merged 1 commit into from
Jun 1, 2022

Conversation

worldwonderer
Copy link
Contributor

@worldwonderer worldwonderer commented May 30, 2022

Description

if proxy-cache plugin configures cache_key with ["$arg_a", "$arg_b"] and $arg_a and $arg_b are both nil, apisix will abort with error.

Fixes #7167

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)

@tzssangglass
Copy link
Member

Hi @worldwonderer , thank you for your contribution.

The test case is wrongly written.

--- request
GET /hello

will not hit location /t.

you can ref to: https://github.com/apache/apisix/blob/master/t/plugin/proxy-cache/disk.t#L318-L367

@worldwonderer
Copy link
Contributor Author

Hi @worldwonderer , thank you for your contribution.

The test case is wrongly written.

--- request
GET /hello

will not hit location /t.

you can ref to: https://github.com/apache/apisix/blob/master/t/plugin/proxy-cache/disk.t#L318-L367

Thanks for your reminder. I understand the usage of test cases now. First, request /t will create route and request /hello will goto the created route? I pushed new commit, plz trigger CI and try again.

spacewander
spacewander previously approved these changes May 31, 2022
}
--- request
GET /t
--- error_code: 200
Copy link
Member

Choose a reason for hiding this comment

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

we can remove this, it will be checked by default

Copy link
Contributor Author

@worldwonderer worldwonderer May 31, 2022

Choose a reason for hiding this comment

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

just keep this?

--- response_body
passed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Strange, removing it shouldn't affect anything. I will check it.

Copy link
Member

Choose a reason for hiding this comment

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

I've tried it locally, no exceptions, you can delete -- error_code: 200

@@ -53,6 +53,8 @@ The cache expiration time cannot be configured dynamically. It can only be set b

If the Upstream service is not available and APISIX returns a 502 or 504 status code, it will be cached for 10s.

Variable starts with `$` and will be treated as an empty string when does not exist. You can also use a combination of variables and strings, but you need to write them separately in one array, and eventually the variables will be parsed and stitched together with strings.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Variable starts with `$` and will be treated as an empty string when does not exist. You can also use a combination of variables and strings, but you need to write them separately in one array, and eventually the variables will be parsed and stitched together with strings.
Variables (start with `$`) can be specified in `cache_key` and `cache_bypass. It's worth to mention that the variable value will be an empty string if it doesn't exist.
You can also combine a number of variables and strings (constants), by writing them into an array, eventually, variables will be parsed and stitched together with strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. And I added no_cache

t/plugin/proxy-cache/disk.t Outdated Show resolved Hide resolved
@tzssangglass
Copy link
Member

LGTM, let's wait for the CI pass.

tzssangglass
tzssangglass previously approved these changes Jun 1, 2022
@spacewander spacewander merged commit 94e0616 into apache:master Jun 1, 2022
spacewander pushed a commit to spacewander/incubator-apisix that referenced this pull request Jun 20, 2022
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
spacewander pushed a commit that referenced this pull request Jun 20, 2022
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
@lijing-21
Copy link
Contributor

Hi @worldwonderer , thanks for the contribution!

Here is the Contributor T-shirt form[1], if you're interested, kindly take a look :)

[1] https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#contributor-t-shirt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: proxy-cache plugin generate_complex_value may fail due to concat nil values
5 participants