Skip to content

Conversation

yhs0092
Copy link
Member

@yhs0092 yhs0092 commented Nov 21, 2017

see details in https://servicecomb.atlassian.net/browse/JAV-495

  • Set default URL pattern when RestServletInitializer is invoked and URL pattern is not specified.
  • Add a demo to demonstrate how a Spring Boot project is converted to a JavaChassis project.

…d and URL pattern is not specified. Add a demo to demonstrate how a Spring Boot project is converted to a JavaChassis project.
private static void testNull(Test test) {
TestMgr.check("code is 'null'", test.getTestString(null));
TestMgr.check(null, test.wrapParam(null));
// TestMgr.check(null, test.wrapParam(null));
Copy link
Contributor

Choose a reason for hiding this comment

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

lost this test case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I commented out this line for debugging... I will retrieve it.

import io.servicecomb.demo.helloworld.greeter.Hello;
import io.servicecomb.provider.pojo.RpcSchema;

@RpcSchema(schemaId = "helloworld.Greeter")
Copy link
Contributor

Choose a reason for hiding this comment

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

why old one no need this annotations?

Copy link
Member Author

Choose a reason for hiding this comment

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

In old edition, the schema of HelloImpl is configured in a xml file. But the schema of TestImpl is configured by annotation. After discussion with Liu Bao, we think that two kind of configuration existing in one demo is confusing, so I transform it into annotation config.

import io.servicecomb.swagger.invocation.exception.InvocationException;

@RpcSchema(schemaId = "server")
@RequestMapping(path = "/pojo/rest", produces = MediaType.APPLICATION_JSON)
Copy link
Contributor

Choose a reason for hiding this comment

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

why RpcSchema need this annotations?

Copy link
Member Author

Choose a reason for hiding this comment

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

I keep this annotation in code for the purpose that users can transform this demo back into a Spring Boot demo more conveniently.(And they can compare this two kind of framework)
But actually this annotation is redundant, I will remove it and create another PR.

LOGGER.info("If you see this log, that means this demo project has been converted to ServiceComb framework.");

invocation.next(response -> {
if (invocation.getOperationName().equals("splitParam")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

schema+operation is a unique key
OperationMeta have some qualifiedName method, you can call one of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

    invocation.next(response -> {
      if (invocation.getOperationMeta().getSchemaQualifiedName().equals("server.splitParam")) {

the judgement logic has been changed like above. Is this what you mean?

chain:
Provider:
default: myhandler
servicecomb:
Copy link
Contributor

Choose a reason for hiding this comment

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

if you want to test auto urlPattern, should this be deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

servicecomb:
  rest:
    servlet:
      urlPattern: /*

I guess you mean the configuration above should be deleted, right?

Copy link
Contributor

@wujimin wujimin left a comment

Choose a reason for hiding this comment

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

please fix the problems.

@yhs0092 yhs0092 closed this Nov 23, 2017
@yhs0092 yhs0092 reopened this Nov 23, 2017
import java.util.Arrays;
import java.util.List;

import javax.ws.rs.core.MediaType;
Copy link
Contributor

Choose a reason for hiding this comment

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

compile warning?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 87.139% when pulling 2eec158 on yhs0092:JAV-495_set_urlPattern_default_value into fc9fed2 on ServiceComb:master.

<groupId>org.springframework.cloud</groupId>
<artifactId>spring-cloud-netflix-core</artifactId>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

client depend on server
then service's microservice.yaml in classpath too, which one will override another one, it's random, except you point the order.
even you point the order, it's bad practice.

all server schema deployed to client too. do you want this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have fixed this problem by removing the dependency in client side, and organized import of RestServletInitializer.java. Please review this commit.

…-spring-boot-pojo-server to avoid the microservice.yaml file overriding
@yhs0092 yhs0092 closed this Nov 24, 2017
@yhs0092 yhs0092 reopened this Nov 24, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 86.831% when pulling 6509253 on yhs0092:JAV-495_set_urlPattern_default_value into fc9fed2 on ServiceComb:master.

@yhs0092
Copy link
Member Author

yhs0092 commented Nov 24, 2017

@wujimin I close this pull request and reopen it, and the travis-ci build passed. I can't find out the failure cause.

@WillemJiang WillemJiang merged commit 86a8a41 into apache:master Nov 25, 2017
@yhs0092 yhs0092 deleted the JAV-495_set_urlPattern_default_value branch July 23, 2018 12:17
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.

5 participants