limit_req_zone with 4 arguments not being analyzed

nginxinc / nginx-go-crossplane

A library for working with NGINX configs in Go

Apache License 2.0

46 stars 12 forks source link

limit_req_zone with 4 arguments not being analyzed #104

Closed oliveromahony closed 1 month ago

oliveromahony commented 1 month ago

Proposed changes

Limit request zone has an issue with not being throwing: invalid number of arguments in \"limit_req_zone\" directive error despite being 4 arguments.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

[x] I have read the CONTRIBUTING document
[x] If applicable, I have added tests that prove my fix is effective or that my feature works
[x] If applicable, I have checked that any relevant tests pass after adding my changes
[x] I have updated any relevant documentation (README.md)

dengsh12 commented 1 month ago

The problem comes from a mismatch in a legacy version of code generator, the flag NGX_CONF_TAKE4 of keyval and limit_req_zone mapped to ngxConfTake2 wrongly. analyze_nplus_latest_directives.go may be override to the legacy version while I was dealing with branches. I've proposed another MR #105 to fix. Thanks for your findings!

For the uint part it seems we define the bitmasks using untyped int and they can be compatible with uint, maybe we don't need to change them.

oliveromahony commented 1 month ago

The problem comes from a mismatch in a legacy version of code generator, the flag NGX_CONF_TAKE4 of keyval and limit_req_zone mapped to ngxConfTake2 wrongly. analyze_nplus_latest_directives.go may be override to the legacy version while I was dealing with branches. I've proposed another MR #105 to fix. Thanks for your findings!

For the uint part it seems we define the bitmasks using untyped int and they can be compatible with uint, maybe we don't need to change them.

Happy to close this PR. Can we add tests to cover this scenario in https://github.com/nginxinc/nginx-go-crossplane/pull/105 ?

dengsh12 commented 1 month ago

The problem comes from a mismatch in a legacy version of code generator, the flag NGX_CONF_TAKE4 of keyval and limit_req_zone mapped to ngxConfTake2 wrongly. analyze_nplus_latest_directives.go may be override to the legacy version while I was dealing with branches. I've proposed another MR #105 to fix. Thanks for your findings! For the uint part it seems we define the bitmasks using untyped int and they can be compatible with uint, maybe we don't need to change them.

Happy to close this PR. Can we add tests to cover this scenario in #105 ?

Yeah! Root reason comes from a legacy version of the generator has a wrong mapping from NGX_CONF_TAKE4 to ngxConfTake2 and I added one unit test with all bitmask in nginx to the MR of generator #101

It would also be helpful to add an unit test for limit_req_zone. Your test case in this PR is a very good example, could I add it to #105 ?

ryepup commented 1 month ago

It would also be helpful to add an unit test for limit_req_zone. Your test case in this PR is a very good example, could I add it to #105 ?

please do

oliveromahony commented 1 month ago

The problem comes from a mismatch in a legacy version of code generator, the flag NGX_CONF_TAKE4 of keyval and limit_req_zone mapped to ngxConfTake2 wrongly. analyze_nplus_latest_directives.go may be override to the legacy version while I was dealing with branches. I've proposed another MR #105 to fix. Thanks for your findings! For the uint part it seems we define the bitmasks using untyped int and they can be compatible with uint, maybe we don't need to change them.

Happy to close this PR. Can we add tests to cover this scenario in #105 ?

Yeah! Root reason comes from a legacy version of the generator has a wrong mapping from NGX_CONF_TAKE4 to ngxConfTake2 and I added one unit test with all bitmask in nginx to the MR of generator #101

It would also be helpful to add an unit test for limit_req_zone. Your test case in this PR is a very good example, could I add it to #105 ?

That would be great