dharmendrakshl1 / rest-assured

Automatically exported from code.google.com/p/rest-assured
0 stars 0 forks source link

Remove or fix the default behavior assuming port 8080 in favor to the HTTP default #353

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
The problem: given a base URL http://foo.bar/base. When rest-assured begins to 
GET a resource "/res" relative to the base URL, it will modify the original 
base URL and send the request to http://foo.bar:8080/base/res. 

This might be convenient for local development, but it breaks unexpectedly when 
used against a Tomcat which is proxied by an Apache. The workaround is to 
explicitly provide port 80 in the base URL - which silly, because the user must 
repeat something what is a well-known default. Another workaround would be to 
supply the actual port of the Tomcat, which means I have to provide internal 
details of the SUT (which is supposed to be a black box). Furthermore, is not 
possible to test that the Apache is forwarding correctly. 

This default behavior is quite surprising (in a bad sense), since it 
contradicts what browsers do, and doesn't respect the well-known default port 
80 for HTTP URLs. Basically, it does something else, what it is told to do.

So to me this default is quite misleading. Instead of forcing to supply port 
80, or even worse the internal port of the SUT (quite often production Tomcats 
are proxied by an Apache), it might be better to remove that default and use 80 
for a HTTP URL, following the principle of least surprise. And for local 
development the developer should be able to provide the port easily.

Greetings,
Rusi

Original issue reported on code.google.com by rfili...@gmail.com on 10 Sep 2014 at 1:56

GoogleCodeExporter commented 9 years ago
Do you mean that you have to set the port to 80 like this when you make a 
request:

RestAssured.baseURI = "http://foo.bar:80/base";
when().get("/res").. ;

Original comment by johan.ha...@gmail.com on 15 Sep 2014 at 12:07

GoogleCodeExporter commented 9 years ago
Well, I try to avoid using the static fields of the RestAssured class, since I 
don't like to have state in static fields. Actually such an API design 
encourages quite a bad practice of managing state in long-lived stateful static 
fields. Just imagine running two tests requiring two different baseURIs in 
parallel in the same JVM...(well-designed tests should have no problem running 
in parallel). That said, this is another issue.

My point is that the user must set the port explicitly to 80 in order to 
connect to the server. Otherwise (if the port is omitted) some logic in the 
Groovy code will magically set the port to 8080 and the request will fail. And 
this is not what a user would expect according to the principle of least 
astonishment..

Original comment by rfili...@gmail.com on 15 Sep 2014 at 12:41

GoogleCodeExporter commented 9 years ago
Yes I'm aware of the problems with static fields and I agree with what you're 
saying.

What I'm getting at is an example of how you write the URL in your test. Since 
if I just do:

get("http://www.google.com");

REST Assured should use the default port of 80 (which it does when I try it).

Original comment by johan.ha...@gmail.com on 15 Sep 2014 at 12:59

GoogleCodeExporter commented 9 years ago
Hi Johan,

sorry for the misunderstanding, here is some sample code which illustrates what 
we are doing:

Client Code
===========

  String baseUri = "https://teleconf.de/conference/rest3"; // configurable
  ClientFactory factory = new RestAssuredClientFactory(baseUri,"user", "secret");
  AccountClient client = factory.newAccountClient();
  XmlAccountData response = client.selectCurrentAccount()

The Factory
===========

public final class RestAssuredClientFactory implements ClientFactory {

  private final RestAssuredSpecifier specifier;

  static {
    RestAssured.config = RestAssured.config()
                                    .sslConfig(SSLConfig.sslConfig().allowAllHostnames())
                                    .decoderConfig(DecoderConfig.decoderConfig()
                                                                .defaultContentCharset("UTF-8"))
                                    .encoderConfig(EncoderConfig.encoderConfig()
                                                                .defaultContentCharset("UTF-8"));
    RestAssured.useRelaxedHTTPSValidation();
  }

  public RestAssuredClientFactory(String baseUri, String username, String password) {
    this.specifier = new RestAssuredSpecifier(baseUri, username, password);
  }

  @Override
  public AccountClient newAccountClient() {
    return new RestAssuredAccountClient(this.specifier);
  }
}

The Specifier
=============

/**
 * This class is responsible for creating and configuring REST-assured style
 * {@link RequestSpecification} and {@link ResponseSpecification} objects in a
 * uniform way.
 */
final class RestAssuredSpecifier {

  private final String baseUri;
  private final String username;
  private final String password;

  public RestAssuredSpecifier(String baseUri, String username, String password) {
    this.baseUri = Preconditions.checkNotNull(baseUri);
    this.username = Preconditions.checkNotNull(username);
    this.password = Preconditions.checkNotNull(password);
  }

  public RequestSpecification createJsonRequest() {
    RequestSpecification request = createRequest();
    request.contentType(ContentType.JSON);
    return request;
  }

  private RequestSpecification createRequest() {
    RequestSpecification request = RestAssured.given();

    // essential HTTP connection params
    request = request.baseUri(this.baseUri);
    request = request.auth().basic(this.username, this.password);

    // use JSON and UTF-8 as a transport format
    request = request.header(new Header("Accept", "application/json"));

    // log the request/response cycle
    request = request.log().all();
    request.response().log().all();

    return request;
  }
}

The Client
==========

public final class RestAssuredAccountClient implements AccountClient {

  private static final String PATH = "/account";
  private final RestAssuredSpecifier specifier;

  public RestAssuredAccountClient(RestAssuredSpecifier specifier) {
    this.specifier = specifier;
  }

  @Override
  public XmlAccountData selectCurrentAccount() throws HttpStatusException {
    RequestSpecification request = this.specifier.createJsonRequest();
    Response response = request.get(PATH);
    HttpStatus.OK.checkResponse(response);
    return response.as(XmlAccountData.class);
  }
}

Original comment by rfili...@gmail.com on 16 Sep 2014 at 11:49

GoogleCodeExporter commented 9 years ago
I just noticed, that 8080 is not automatically inserted (correct behavior) when 
HTTPS is used, but only with HTTP as a baseUri.

Original comment by rfili...@gmail.com on 16 Sep 2014 at 3:32

GoogleCodeExporter commented 9 years ago
So you mean that if I change "https://teleconf.de/conference/rest3" to 
"http://teleconf.de/conference/rest3" I ought to see the error?

Original comment by johan.ha...@gmail.com on 17 Sep 2014 at 5:14

GoogleCodeExporter commented 9 years ago
Good timing- I was planning to bring up this issue today.  Here's a simple 
example:

    @Test
    public void given_http() {
        given().baseUri("http://httpbin.org/get").get();
    }

    @Test
    public void get_http() {
        get("http://httpbin.org/get");
    }

First test tries to use port 8080 and fails.  Second test uses port 80 and 
passes.

Personally I would vote to change the default port from 8080 to 80.  Failing 
that, there's obviously some logic that causes the get() request to use port 
80- you could apply that same logic to .baseUri().

Original comment by bsfarr...@gmail.com on 17 Sep 2014 at 1:42

GoogleCodeExporter commented 9 years ago
It seems to me that the error is located in RequestSpecificationImpl.groovy, 
somewhere around this spot in getTargetUriFromUrl(URL, PathType):

    if (!hasPortDefined(url) && !useDefaultHttps && ((port != DEFAULT_HTTP_PORT && port != DEFAULT_HTTP_TEST_PORT)
            || pathType == PathType.BASE_URI || hasAuthorityEqualToLocalhost(url))) {
      builder.append(":")
      builder.append(port)
    }

PS: +1 for removing 8080 as default port. If the port is not specified, it 
should be 80, just as one would expect.

Original comment by rfili...@gmail.com on 17 Sep 2014 at 2:40

GoogleCodeExporter commented 9 years ago
8080 should only be default port if you don't specify a fully-qualified URI. 
For example:

get("/test") should call http://localhost:8080/test
get("http://someserver.com/test") should call http://someserver.com:80/test
Likewise when you specify a fully-qualified URI as base URI. So it seems to be 
a bug when using base uri's.

Original comment by johan.ha...@gmail.com on 23 Sep 2014 at 7:02

GoogleCodeExporter commented 9 years ago
I think I've fixed this now. Please try version 2.3.4-SNAPSHOT after having 
added the following maven repo:

<repositories>
        <repository>
            <id>sonatype</id>
            <url>https://oss.sonatype.org/content/repositories/snapshots/</url>
            <snapshots />
        </repository>
</repositories>

Original comment by johan.ha...@gmail.com on 23 Sep 2014 at 8:55