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.

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

SCB-333 Update to support the date time with JSR-310 #732

Closed
wants to merge 2 commits into from

Conversation

zhfeng
Copy link

@zhfeng zhfeng commented May 28, 2018

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
Copy link

coveralls commented May 28, 2018

Coverage Status

Coverage increased (+0.02%) to 87.549% when pulling 6e078bb on zhfeng:SCB-333 into 6ce5280 on apache:master.

String localDate = "\"2018-05-28\"";
String localDateTime = "\"2018-05-28T11:00:00\"";

JsonUtils.OBJ_MAPPER.readValue(period, Period.class);
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to verify the value of object here.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, it makes sense.

@wujimin
Copy link
Contributor

wujimin commented May 29, 2018

1.RestObjectMapper is for RESTful codec, it's better to add UT for it.
2.this is just for codec, i'm not sure if we can generate swagger from JSR-310 dataTypes
so it's better to add UT and IT for new dataTypes.

@zhfeng
Copy link
Author

zhfeng commented May 29, 2018

@wujimin I'm not very sure what the UT and the IT means ?

@wujimin
Copy link
Contributor

wujimin commented May 29, 2018

Unit Test
Integration Test
:)

@liubao68
Copy link
Contributor

Integrations tests mean we need to add an end-to-end use case in a RPC consumer & provider. You can add examples in demos/SpringMVCclient and SpringMVCserver.
Because we will use service definition to generate swagger, and you only changed Json mapper, this PR may not work as we expected.

@zhfeng
Copy link
Author

zhfeng commented May 29, 2018

So I also need to take a look at the swagger generator codes to make sure it can handle the localDate with the JSR310 ?

@liubao68
Copy link
Contributor

liubao68 commented May 29, 2018

Yes, I think so. Or users how to use this feature you provided? I can't figure out what is changed after this PR to developers using java-chassis. Directly using internal APIs in JsonUtils or RestObjectMapper is rarely and users can use the original API, this is not our concern I think.

@DeanLee2013
Copy link
Contributor

I think I have some pojo codes for this localDate Type testing. Now I give it a try.
https://github.com/microDevOps/microservice-test

@WillemJiang
Copy link
Member

@imlidian Please updated the verification that you did.

@DeanLee2013
Copy link
Contributor

The Error and my test case implement way as follows. Did I miss something about adding one test case?

Error:
2018-06-05 10:24:19,605 [WARN] consumer method org.apache.servicecomb.demo.springmvc.client.CodeFirstSpringmvcIntf:testObject not exist in contract. org.apache.servicecomb.swagger.engine.SwaggerEnvironment.createConsumer(SwaggerEnvironment.java:98)

Exception in thread "main" java.lang.IllegalStateException: Consumer method org.apache.servicecomb.demo.springmvc.client.CodeFirstSpringmvcIntf:testObject not exist in contract, microserviceName=springmvc, schemaId=codeFirst; new producer not running or not deployed.

Interface Side:
public interface CodeFirstSpringmvcIntf {

Object testObject(Object input);

LocalDate testLocalDate(LocalDate input);

//LocalDateTime testLocalDateTime(LocalDateTime input);

}
Server Side:
@RestSchema(schemaId = "codeFirst")
@RequestMapping(path = "/codeFirstSpringmvc", produces = MediaType.APPLICATION_JSON_VALUE)
public class CodeFirstSpringmvc {
return input;
}
@PostMapping(path = "/testLocalDate")
public LocalDate testLocalDate(@requestbody LocalDate input) {
return input;
}

Client Side:
b/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/TestObject.java
public class TestObject {
TestMgr.check("str", result);
TestMgr.check(String.class, result.getClass());

// LocalDate
result = intf.testLocalDate(LocalDate.of(1968, 12, 8));
TestMgr.check(LocalDate.of(1968, 12, 8), result);
TestMgr.check(LocalDate.class, result.getClass());

result = restTemplate.postForObject(prefix  "/object", "str", String.class);
TestMgr.check("str", result);
TestMgr.check(String.class, result.getClass());

}

@liubao68
Copy link
Contributor

liubao68 commented Jun 5, 2018

You can check your provider logs for detail messages. It usually maybe you add some new methods and microservice version not changed and the consumer cannot see the newly added method from service center. See here for more details: https://huaweicse.github.io/servicecomb-java-chassis-doc/zh_CN/question-and-answer/interface-compatibility.html

@liubao68
Copy link
Contributor

We have opened an API to let users to customize RestObjectMapper #874. If users want to use the feature provided by this PR, they can write their own RestObjectMapper.
I am going to close this PR since it's not a complete feature. Please feel free to add more comments when you have another solutions.

@liubao68 liubao68 closed this Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants