[SCB-685] Service comb chassis must support default values in query parameters #774
Conversation
acsukesh
changed the title
| @@ -43,6 +54,12 @@ public Object getValue(HttpServletRequest request) throws Exception { | |||
| value = request.getParameterValues(paramPath); | |||
| } else { | |||
| value = request.getParameter(paramPath); | |||
| if (value == null || value.equals("")) { | |||
There was a problem hiding this comment.
1."" should not change to default value?
2.only process query parameter? what about header/cookie and so on?
There was a problem hiding this comment.
Maybe we can add defaultValue to base interface
There was a problem hiding this comment.
"" comes only when user inputs the same and is considered as valid input parameter.
There was a problem hiding this comment.
OK. For http://10.57.65.159:8080/codeFirstSpringmvc/reduce?a=
give default Value is better.
| @@ -51,9 +51,12 @@ public static void main(String[] args) throws Exception { | |||
| // RestTemplate Consumer | |||
| String sayHiResult = | |||
| restTemplate.postForObject("cse://springmvc/springmvchello/sayhi?name=Java Chassis", null, String.class); | |||
| String sayHiDefaultResult = | |||
There was a problem hiding this comment.
It's better to add test cases in demo projects. Sample are short examples, and demos folder are integration tests, you can add as many test cases you like.
And maybe you need to test some primitive types default value, and from the following code, they seems not properly handled.
|
|
||
| } | ||
|
|
||
| protected String getAnnotationParameterDefaultValue(Object annotation) { |
There was a problem hiding this comment.
primitive and other types default value not properly handled?
| protected String getAnnotationParameterDefaultValue(Object annotation) { | ||
| String defaultValue = ((RequestParam) annotation).defaultValue(); | ||
| if (defaultValue.equals(ValueConstants.DEFAULT_NONE)) { | ||
| return ""; |
There was a problem hiding this comment.
might cause NullPointerException, also some message might get added with null which may not be correct , so returning empty
There was a problem hiding this comment.
Maybe it's better to return ValueConstants.DEFAULT_NONE. When no default value specified and users do not give the parameter value, we can throw an BadRequest exception or give default value based on parameter type, e.g. int is 0 and String is null.
|
I'll merge this PR. Following work need to be done:
|
Follow this checklist to help us incorporate your contribution quickly and easily:
[SCB-XXX] Fixes bug in ApproximateQuantiles, where you replaceSCB-XXXwith the appropriate JIRA issue.mvn clean installto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.