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

deps: replace Jetty with HttpServer #433

Merged
merged 4 commits into from Feb 20, 2020

Conversation

@ericraskin
Copy link
Contributor

@ericraskin ericraskin commented Feb 18, 2020

Remove Jetty v8.2 and replace with com.sun.net.httpserver.HttpServer. Jetty has had breaking changes since 8.2 (specifically with version 9.4). This causes conflicts with newer code using later versions of Jetty. Removing dependency on Jetty resolves this issue.

This replaces previous pull request updating Jetty to 9.4.

Fixes #397.

Remove Jetty v8.2 and replace with com.sun.net.httpserver.HttpServer.  Jetty has had breaking changes since 8.2 (specifically with version 9.4).  This causes with conflicts with newer code using later versions of Jetty.  Removing dependency on Jetty resolves this issue.

Fixes googleapis#397.
@ericraskin ericraskin requested a review from as a code owner Feb 18, 2020
@chingor13
Copy link
Contributor

@chingor13 chingor13 commented Feb 18, 2020

Removing the jetty dependency make this artifact incorrectly named. If you'd like to propose a HttpServer version artifact, we can discuss creating a new artifact like com.google.oauth-client:google-ouath-client-httpserver, but please open a feature request issue to discuss this new implementation.

As for #397, it's not a simple as updating the implementation as there may be other dependencies in the ecosystem that updating jetty will break. This is more of a dependency management issue than an implementation issue.

@chingor13 chingor13 closed this Feb 18, 2020
Copy link
Collaborator

@elharo elharo left a comment

Agreed that it's a dependency management issue. That's why I suggested it might be easier to remove jetty completely. That this PR manages to do that without changing the public API, so far as I can see, suggests that we don't really need jetty here.

The artifact name is unfortunate, but I don't think it should be a blocker. As written this is a drop in replacement for the existing code.

@chingor13 chingor13 reopened this Feb 18, 2020
@elharo elharo changed the title feat: Replace Jetty with HttpServer. deps: replace Jetty with HttpServer. Feb 18, 2020
elharo
elharo approved these changes Feb 19, 2020
*Copied from Jetty findFreePort() as referenced by: https://gist.github.com/vorburger/3429822
*/

private int findOpenPort() {
Copy link
Collaborator

@elharo elharo Feb 19, 2020

Choose a reason for hiding this comment

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

this could use try with resources

@ericraskin
Copy link
Contributor Author

@ericraskin ericraskin commented Feb 19, 2020

server = HttpServer.create(new InetSocketAddress(port != -1 ? port : findOpenPort()), 0);
HttpContext context = server.createContext(callbackPath, new CallbackHandler());
server.setExecutor(null);
/*
Copy link
Collaborator

@elharo elharo Feb 19, 2020

Choose a reason for hiding this comment

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

why is this code commented out?

@ericraskin
Copy link
Contributor Author

@ericraskin ericraskin commented Feb 19, 2020

elharo
elharo approved these changes Feb 19, 2020
@elharo
Copy link
Collaborator

@elharo elharo commented Feb 20, 2020

INFO] Downloaded from central: https://repo.maven.apache.org/maven2/com/google/http-client/google-http-client-appengine/1.34.2/google-http-client-appengine-1.34.2.jar (18 kB at 901 kB/s)
/tmpfs/src/github/google-oauth-java-client/google-oauth-client-jetty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java:156:9: expecting LCURLY, found '('
[INFO] Starting audit...
/tmpfs/src/github/google-oauth-java-client/google-oauth-client-jetty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java:158:7: Got an exception - expecting EOF, found 'return'
Audit done.

[INFO] There are 1 checkstyle errors.
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:

}

/**
* Blocks until the server receives a login result, or the server is stopped
* by {@link #stop()}, to return an authorization code.
*
* @return authorization code if login succeeds; may return {@code null} if the server
* is stopped by {@link #stop()}
* is stopped by {@link #stop()}
Copy link
Collaborator

@elharo elharo Feb 20, 2020

Choose a reason for hiding this comment

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

four space indent

* @throws IOException if the server receives an error code (through an HTTP request
* parameter {@code error})
* parameter {@code error})
Copy link
Collaborator

@elharo elharo Feb 20, 2020

Choose a reason for hiding this comment

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

only four space indent, per google style

}

/*
*Copied from Jetty findFreePort() as referenced by: https://gist.github.com/vorburger/3429822
Copy link
Collaborator

@elharo elharo Feb 20, 2020

Choose a reason for hiding this comment

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

It might be the Javadoc comment that checkstyle is complaining about; I'm not sure. Try deleting it.

@@ -16,15 +16,19 @@

import com.google.api.client.extensions.java6.auth.oauth2.VerificationCodeReceiver;
import com.google.api.client.util.Throwables;
import com.sun.net.httpserver.*;
Copy link
Collaborator

@elharo elharo Feb 20, 2020

Choose a reason for hiding this comment

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

Google style is to never use wildcard imports.

@ericraskin ericraskin changed the title deps: replace Jetty with HttpServer. deps: replace Jetty with HttpServer Feb 20, 2020
@ericraskin
Copy link
Contributor Author

@ericraskin ericraskin commented Feb 20, 2020

[INFO] --- maven-checkstyle-plugin:2.6:check (default) @ google-oauth-client-jetty ---
/tmpfs/src/github/google-oauth-java-client/google-oauth-client-jetty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java:159:9: expecting LCURLY, found '('
[INFO] Starting audit...
/tmpfs/src/github/google-oauth-java-client/google-oauth-client-jetty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java:161:7: Got an exception - expecting EOF, found 'return'
Audit done.
  private int findOpenPort() {
    try (ServerSocket socket = new ServerSocket(0)) {
      socket.setReuseAddress(true);
      return socket.getLocalPort();
    } catch (IOException e) {
      throw new IllegalStateException("No free TCP/IP port to start embedded HTTP Server on");
    }
  }

The try statement is line 159 in my file. Checkstyle doesn't allow try with resources?

Copy link
Collaborator

@elharo elharo left a comment

I'm not sure why checkstyle is still complaining. I sort of wish we didn't have this as a mandatory check. It all looks fine to me. I'll see if I can find a minute to pull this branch down and figure out what's going on here.

@ericraskin
Copy link
Contributor Author

@ericraskin ericraskin commented Feb 20, 2020

@ericraskin
Copy link
Contributor Author

@ericraskin ericraskin commented Feb 20, 2020

@elharo
Copy link
Collaborator

@elharo elharo commented Feb 20, 2020

@elharo elharo merged commit bcabce2 into googleapis:master Feb 20, 2020
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants