fix(balancer): Add test cases when setting upstream doesn't specify priority

[qizhendong1]

Description

The transform_node function doesn't check the priority. If the priority is nil, apisix will print a error.

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]

hi @zhendongcmss we need test cases to cover this.

[qizhendong1]

hi @zhendongcmss we need test cases to cover this.

will update

[tzssangglass]

I'm curious about the trigger condition because priority is set as a default in the JSON schema. see: https://github.com/apache/apisix/blob/e56d23950ff18f7656a605ad80b5c9a2576d3c01/apisix/schema_def.lua#L331-L335

[qizhendong1]

I'm curious about the trigger condition because priority is set as a default in the JSON schema. see:

https://github.com/apache/apisix/blob/e56d23950ff18f7656a605ad80b5c9a2576d3c01/apisix/schema_def.lua#L331-L335

I know that. But, Somtimes, we develop a new plugin on access phase and use upstream.set(ctx, "xxxx", "xxx", up_conf) to rewrite the upstreams, if the up_conf doesn't include the priority(want to use the default priority) will get an error.

[tzssangglass]

I know that. But, Somtimes, we develop a new plugin on access phase and use upstream.set(ctx, "xxxx", "xxx", up_conf) to rewrite the upstreams, if the up_conf doesn't include the priority(want to use the default priority) will get an error.

I don't really agree with the way this was handled. I think it's too late to check the parameters here.

In fact, for all configurations with default attributes that are not verified by the json schema, there are similar problems?

[qizhendong1]

schema

I think it is necessary to check the passed parameter to aviod thread crash error.

[tzssangglass]

Let's hear what others think.

[qizhendong1]

@tzssangglass @spacewander please review

[spacewander]

Please make the CI pass, thanks!

[qizhendong1]

Please make the CI pass, thanks!

There no default priority checking the test cases don't pass. How about you that the test cases are necessary and reasonable ？

The test cases call test->balancer.pick_sever -> create_server_picker -> fetch_health_nodes, there are no default priority check.

I think it is reasonable because some times we call the API function not only using jsom conf.

https://github.com/apache/apisix/runs/6316828971?check_suite_focus=true#step:11:377

@spacewander @tzssangglass

[github-actions[bot]]

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

[github-actions[bot]]

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.