[SCB-292] chassis support standard parameter validation #627

acsukesh · 2018-03-29T12:06:26Z

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [SCB-XXX] Fixes bug in ApproximateQuantiles, where you replace SCB-XXX with the appropriate JIRA issue.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

coveralls · 2018-03-29T12:39:20Z

Coverage increased (+0.06%) to 87.459% when pulling ad8c7dc on acsukesh:master into ae28a9f on apache:master.

wujimin · 2018-03-30T03:13:56Z

...ava/org/apache/servicecomb/swagger/invocation/extension/AbstractProducerInvokeExtension.java

+ */
+package org.apache.servicecomb.swagger.invocation.extension;
+
+public abstract class AbstractProducerInvokeExtension implements ProducerInvokeExtension {


why we always define empty abstract class?
i think it's useless.

Agreed. Java now supports defaults for interface, and no need empty abstract class any more

liubao68

Can you provide some integration tests to show the user how to use this feature?

liubao68 · 2018-03-30T09:22:54Z

swagger/swagger-invocation/invocation-core/pom.xml

@@ -37,6 +37,14 @@
 			<artifactId>swagger-generator-jaxrs</artifactId>
 			<scope>test</scope>
 		</dependency>
+		<dependency>


wujimin · 2018-03-31T00:54:30Z

core/src/test/java/org/apache/servicecomb/core/definition/schema/TestProducerSchemaFactory.java

@@ -134,6 +137,11 @@ public void testGetOrCreateProducer() throws Exception {

    SwaggerProducerOperation producerOperation = operationMeta.getExtData(Const.PRODUCER_OPERATION);

+    List<ProducerInvokeExtension> producerInvokeExtenstionList =


why need to set this manually？

wujimin · 2018-03-31T00:55:39Z

...n/java/org/apache/servicecomb/swagger/invocation/exception/ExceptionToResponseConverter.java

@@ -22,5 +22,9 @@
 public interface ExceptionToResponseConverter<T extends Throwable> {
  Class<T> getExceptionClass();

+  default int getOrder() {


interface definition default to 100,
so strange

wujimin · 2018-03-31T00:56:39Z

...c/main/java/org/apache/servicecomb/swagger/invocation/extension/ProducerInvokeExtension.java

+
+  public int getOrder();
+
+  <T> void beforeMethodInvoke(SwaggerInvocation invocation, SwaggerProducerOperation producerOperation,


add comments
args in invocation is contract args, this is not always equals to producer method args

wujimin · 2018-03-31T01:29:00Z

...or/src/main/java/org/apache/servicecomb/swagger/invocation/validator/ParameterValidator.java

+            Default.class);
+    if (violations.size() > 0) {
+      LOGGER.warn("Parameter validation failed : " + violations.toString());
+      throw new ConstraintViolationException(violations.toString(), violations);


so strange that first and second parameter have the same information

wujimin · 2018-03-31T01:32:38Z

...or/src/main/java/org/apache/servicecomb/swagger/invocation/validator/ParameterValidator.java

+
+  @Override
+  public int getOrder() {
+    return 100;


default to 100 is not enough.
order rule is: smaller order have high priority

so what will happened in ExceptionToResponseConverters when developers custom their own ConstraintViolationException converter?
had remind you this yesterday......

When user custom exception with higher order , it will override the default behavior, user can customize the error codes, message ..etc.The same is tested.

…va-chassis

liubao68 · 2018-04-10T02:36:11Z

core/src/test/java/org/apache/servicecomb/core/definition/schema/TestProducerSchemaFactory.java

@@ -1,5 +1,5 @@
 /*
- * Licensed to the Apache Software Foundation (ASF) under one or more
+	 * Licensed to the Apache Software Foundation (ASF) under one or more


liubao68 · 2018-04-10T02:37:13Z

demo/demo-validator/pom.xml

+	<parent>
+		<groupId>org.apache.servicecomb.demo</groupId>
+		<artifactId>demo-parent</artifactId>
+		<version>1.0.0-m2-SNAPSHOT</version>


Formatting. Suggest to show blanks in your IDE to check if file is correctly indented

liubao68 · 2018-04-10T02:46:18Z

demo/demo-validator/validator-client/pom.xml

+	</dependencies>
+
+	<properties>
+		<demo.main>org.apache.servicecomb.demo.jaxrs.client.JaxrsClient</demo.main>


Class name should be ValidatorClient

liubao68 · 2018-04-10T02:54:40Z

...c/main/java/org/apache/servicecomb/demo/validator/client/CodeFirstRestTemplateValidator.java

+import org.apache.servicecomb.demo.TestMgr;
+import org.springframework.web.client.RestTemplate;
+
+public class CodeFirstRestTemplateValidator extends CodeFirstRestTemplate {


This class seems useless

liubao68 · 2018-04-10T02:55:32Z

demo/demo-validator/validator-server/pom.xml

+	</dependencies>
+
+	<properties>
+		<demo.main>org.apache.servicecomb.demo.jaxrs.server.JaxrsServer</demo.main>


wrong class name

liubao68 · 2018-04-10T03:06:14Z

...c/main/java/org/apache/servicecomb/demo/validator/client/CodeFirstValidatorRestTemplate.java

+    params.put("a", "5");
+    params.put("b", "3");
+    boolean isExcep = false;
+    try {


It's better to validate the returned status code also.

wujimin · 2018-04-11T01:05:44Z

...o-jaxrs/jaxrs-client/src/main/java/org/apache/servicecomb/demo/jaxrs/client/JaxrsClient.java

+      template.postForObject(cseUrlPrefix + "add", params, Integer.class);
+    } catch (InvocationException e) {
+      isExcep = true;
+      TestMgr.check(490, e.getStatus().getStatusCode());


can we got the correct validate message?

liubao68 · 2018-04-11T01:20:49Z

...o-jaxrs/jaxrs-client/src/main/java/org/apache/servicecomb/demo/jaxrs/client/JaxrsClient.java

+          Student.class);
+    } catch (InvocationException e) {
+      isExcep = true;
+      TestMgr.check(490, e.getStatus().getStatusCode());


490 error code exposes that consumer logic got some bad encapsulation. Maybe we need to find out the cause or leave a comment here and submit an improvement issue to fix later.

wujimin · 2018-04-12T08:02:04Z

need to resolve the problem that consumer can not get validate message.
and then merge this PR.

wujimin · 2018-04-16T03:34:42Z

...o-jaxrs/jaxrs-client/src/main/java/org/apache/servicecomb/demo/jaxrs/client/JaxrsClient.java

@@ -179,6 +180,8 @@ private static void testValidatorAddFail(RestTemplate template, String cseUrlPre
    } catch (InvocationException e) {
      isExcep = true;
      TestMgr.check(400, e.getStatus().getStatusCode());
+      TestMgr.check(Status.BAD_REQUEST, e.getReasonPhrase());
+      TestMgr.check(true, e.getErrorData().toString().contains("ConstraintViolationImpl"));


why use contains logic, not verity the full message

wujimin · 2018-04-16T07:12:46Z

...o-jaxrs/jaxrs-client/src/main/java/org/apache/servicecomb/demo/jaxrs/client/JaxrsClient.java

+      TestMgr.check(400, e.getStatus().getStatusCode());
+      TestMgr.check(Status.BAD_REQUEST, e.getReasonPhrase());
+      TestMgr.check(
+          "CommonExceptionData [message=[ConstraintViolationImpl{interpolatedMessage='must be greater than or equal to 20', propertyPath=add.arg1, rootBeanClass=class org.apache.servicecomb.demo.jaxrs.server.Validator, messageTemplate='{javax.validation.constraints.Min.message}'}]]",


so strange that use exception.toString to be the message.
infact only interpolatedMessage/propertyPath/rootBeanClass is useful
we can use json to express this.

I'll submit a new PR for this minor problem and merge this one. https://issues.apache.org/jira/browse/SCB-496

it's not about test case......
root cause is that developer use exception.toString to be response information.

[SCB-292] chassis support standard parameter validation

1a84a1b

asifdxtreme requested review from wujimin, liubao68 and WillemJiang March 29, 2018 13:49

wujimin reviewed Mar 30, 2018

View reviewed changes

[SCB-292] chassis support standard parameter validation

04a1033

liubao68 reviewed Mar 30, 2018

View reviewed changes

acsukesh added 2 commits March 30, 2018 19:04

[SCB-292] chassis support standard parameter validation

cd915ec

Update ExceptionToResponseConverter.java

681d5f3

wujimin reviewed Mar 31, 2018

View reviewed changes

acsukesh added 4 commits April 9, 2018 14:05

[SCB-292] chassis support standard parameter validation

df43bdb

Merge branch 'master' of github.com:acsukesh/incubator-servicecomb-ja…

7f964d9

…va-chassis

Update ExceptionToResponseConverter.java

a661167

[SCB-292] chassis support standard parameter validation

d716a1a

liubao68 reviewed Apr 10, 2018

View reviewed changes

[SCB-292] chassis support standard parameter validation

8941827

liubao68 approved these changes Apr 10, 2018

View reviewed changes

acsukesh added 2 commits April 10, 2018 18:08

[SCB-292] chassis support standard parameter validation

269a2d2

[SCB-292] chassis support standard parameter validation

7ea5691

wujimin reviewed Apr 11, 2018

View reviewed changes

liubao68 reviewed Apr 11, 2018

View reviewed changes

acsukesh added 2 commits April 13, 2018 11:01

[SCB-292] chassis support standard parameter validation

72e5612

[SCB-292] chassis support standard parameter validation

2e85644

wujimin reviewed Apr 16, 2018

View reviewed changes

[SCB-292] chassis support standard parameter validation

ad8c7dc

wujimin reviewed Apr 16, 2018

View reviewed changes

liubao68 approved these changes Apr 16, 2018

View reviewed changes

liubao68 merged commit 01c377c into apache:master Apr 16, 2018

		@@ -134,6 +137,11 @@ public void testGetOrCreateProducer() throws Exception {

		SwaggerProducerOperation producerOperation = operationMeta.getExtData(Const.PRODUCER_OPERATION);

		List<ProducerInvokeExtension> producerInvokeExtenstionList =


		public int getOrder();

		<T> void beforeMethodInvoke(SwaggerInvocation invocation, SwaggerProducerOperation producerOperation,

[SCB-292] chassis support standard parameter validation #627

[SCB-292] chassis support standard parameter validation #627

Conversation

acsukesh commented Mar 29, 2018 • edited

coveralls commented Mar 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liubao68 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wujimin commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liubao68 Apr 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acsukesh commented Mar 29, 2018 •

edited

coveralls commented Mar 29, 2018 •

edited

liubao68 Apr 16, 2018 •

edited