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.

Sign up for GitHub

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

Jump to bottom

[SCB-685] Service comb chassis must support default values in query parameters #774

Merged
merged 2 commits into from Jun 25, 2018
Merged

[SCB-685] Service comb chassis must support default values in query parameters #774

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4978b89
[SCB-685] Service comb chassis must support default values
acsukesh Jun 22, 2018
72289dd
[SCB-685] Service comb chassis must support default values
acsukesh Jun 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 22 additions & 1 deletion ...t/src/main/java/org/apache/servicecomb/common/rest/codec/param/QueryProcessorCreator.java
View file
Open in desktop
Expand Up @@ -27,11 +27,22 @@
import com.fasterxml.jackson.databind.type.TypeFactory;

import io.swagger.models.parameters.Parameter;
import io.swagger.models.parameters.QueryParameter;

public class QueryProcessorCreator implements ParamValueProcessorCreator {
public static final String PARAMTYPE = "query";

public static class QueryProcessor extends AbstractParamProcessor {
Object defaultValue;

public Object getDefaultValue() {
return defaultValue;
}

public void setDefaultValue(Object defaultValue) {
this.defaultValue = defaultValue;
}

public QueryProcessor(String paramPath, JavaType targetType) {
super(paramPath, targetType);
}
Expand All @@ -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("")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

1."" should not change to default value?
2.only process query parameter? what about header/cookie and so on?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add defaultValue to base interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"" comes only when user inputs the same and is considered as valid input parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. For http://10.57.65.159:8080/codeFirstSpringmvc/reduce?a=
give default Value is better.

Object defaultValue = getDefaultValue();
if (defaultValue != null) {
value = defaultValue;
}
}
}

return convertValue(value, targetType);
Expand All @@ -66,6 +83,10 @@ public QueryProcessorCreator() {
@Override
public ParamValueProcessor create(Parameter parameter, Type genericParamType) {
JavaType targetType = TypeFactory.defaultInstance().constructType(genericParamType);
return new QueryProcessor(parameter.getName(), targetType);
QueryProcessor queryProcessor = new QueryProcessor(parameter.getName(), targetType);
if (parameter instanceof QueryParameter) {
queryProcessor.setDefaultValue(((QueryParameter) parameter).getDefaultValue());
}
return queryProcessor;
}
}
10 changes: 6 additions & 4 deletions ...rc/main/java/org/apache/servicecomb/samples/springmvc/consumer/SpringmvcConsumerMain.java
View file
Open in desktop
Expand Up @@ -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 =
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

restTemplate.postForObject("cse://springmvc/springmvchello/sayhi", null, String.class);
String sayHelloResult = restTemplate.postForObject("cse://springmvc/springmvchello/sayhello", person, String.class);
System.out.println("RestTemplate Consumer or POJO Consumer. You can choose whatever you like.");
System.out.println("RestTemplate consumer sayhi services: " + sayHiResult);
System.out.println("RestTemplate consumer sayHiDefault services: " + sayHiDefaultResult);
System.out.println("RestTemplate consumer sayhello services: " + sayHelloResult);

// POJO Consumer
Expand All @@ -70,8 +73,8 @@ public static void main(String[] args) throws Exception {
HttpEntity<Person> entity = new HttpEntity<>(person);
ListenableFuture<ResponseEntity<String>> listenableFuture = cseAsyncRestTemplate
.exchange("cse://springmvc/springmvchello/sayhello", HttpMethod.POST, entity, String.class);
// ResponseEntity<String> responseEntity1 = listenableFuture.get();
// System.out.println("AsyncRestTemplate Consumer sayHello services: " + responseEntity1.getBody());
// ResponseEntity<String> responseEntity1 = listenableFuture.get();
// System.out.println("AsyncRestTemplate Consumer sayHello services: " + responseEntity1.getBody());

listenableFuture.addCallback(
new ListenableFutureCallback<ResponseEntity<String>>() {
Expand All @@ -84,8 +87,7 @@ public void onFailure(Throwable ex) {
public void onSuccess(ResponseEntity<String> result) {
System.out.println("AsyncRestTemplate Consumer sayHello services: " + result.getBody());
}
}
);
});
}

public static void init() throws Exception {
Expand Down
2 changes: 1 addition & 1 deletion ...r/src/main/java/org/apache/servicecomb/samples/springmvc/provider/SpringmvcHelloImpl.java
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@
public class SpringmvcHelloImpl implements Hello {
@Override
@RequestMapping(path = "/sayhi", method = RequestMethod.POST)
public String sayHi(@RequestParam(name = "name") String name) {
public String sayHi(@RequestParam(name = "name", defaultValue = "test") String name) {
return "Hello " + name;
}

Expand Down
14 changes: 14 additions & 0 deletions ...he/servicecomb/swagger/generator/core/processor/parameter/AbstractParameterProcessor.java
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@

package org.apache.servicecomb.swagger.generator.core.processor.parameter;

import org.apache.commons.lang3.StringUtils;
import org.apache.servicecomb.swagger.generator.core.OperationGenerator;
import org.apache.servicecomb.swagger.generator.core.ParameterAnnotationProcessor;
import org.apache.servicecomb.swagger.generator.core.utils.ParamUtils;
Expand All @@ -38,6 +39,7 @@ protected void fillParameter(Object annotation, OperationGenerator operationGene
T parameter) {
setParameterName(annotation, operationGenerator, paramIdx, parameter);
setParameterType(operationGenerator, paramIdx, parameter);
setParameterDefaultValue(annotation, parameter);
}

protected void setParameterType(OperationGenerator operationGenerator, int paramIdx,
Expand All @@ -55,6 +57,18 @@ protected void setParameterName(Object annotation, OperationGenerator operationG
parameter.setName(paramName);
}

protected void setParameterDefaultValue(Object annotation, T parameter) {
String defaultValue = getAnnotationParameterDefaultValue(annotation);
if (StringUtils.isNotEmpty(defaultValue)) {
parameter.setDefaultValue(defaultValue);
}

}

protected String getAnnotationParameterDefaultValue(Object annotation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

primitive and other types default value not properly handled?

return "";
}

protected abstract T createParameter();

protected abstract String getAnnotationParameterName(Object annotation);
Expand Down
10 changes: 10 additions & 0 deletions ...omb/swagger/generator/springmvc/processor/annotation/RequestParamAnnotationProcessor.java
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@

import org.apache.servicecomb.swagger.generator.core.processor.parameter.AbstractParameterProcessor;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ValueConstants;

import io.swagger.models.parameters.QueryParameter;

Expand All @@ -32,4 +33,13 @@ protected QueryParameter createParameter() {
protected String getAnnotationParameterName(Object annotation) {
return ((RequestParam) annotation).name();
}

@Override
protected String getAnnotationParameterDefaultValue(Object annotation) {
String defaultValue = ((RequestParam) annotation).defaultValue();
if (defaultValue.equals(ValueConstants.DEFAULT_NONE)) {
return "";
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe null is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

might cause NullPointerException, also some message might get added with null which may not be correct , so returning empty

Copy link
Contributor

@liubao68 liubao68 Jun 25, 2018 •
edited

Choose a reason for hiding this comment

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

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.

}
return defaultValue;
}
}