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 a team as a code owner February 18, 2020 15:50
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 18, 2020
@chingor13
Copy link
Contributor

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
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 18, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 18, 2020
*Copied from Jetty findFreePort() as referenced by: https://gist.github.com/vorburger/3429822
*/

private int findOpenPort() {
Copy link
Contributor

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 commented Feb 19, 2020 via email

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

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 commented Feb 19, 2020 via email

@elharo elharo added the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020
@elharo
Copy link
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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
@elharo elharo added the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020
@ericraskin
Copy link
Contributor Author

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
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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 commented Feb 20, 2020 via email

@ericraskin
Copy link
Contributor Author

ericraskin commented Feb 20, 2020 via email

@elharo
Copy link
Contributor

elharo commented Feb 20, 2020 via email

@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020
@elharo elharo merged commit bcabce2 into googleapis:master Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

google-oauth-client-jetty incompatible with Jetty 9.4+
5 participants