deps: replace Jetty with HttpServer #433

ericraskin · 2020-02-18T15:50:07Z

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.

chingor13 · 2020-02-18T17:26:47Z

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.

elharo

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.

elharo · 2020-02-19T17:32:43Z

...ty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java

+   *Copied from Jetty findFreePort() as referenced by: https://gist.github.com/vorburger/3429822
+   */
+
+  private int findOpenPort() {


this could use try with resources

ericraskin · 2020-02-19T21:24:23Z

Done.

…

On Wed, Feb 19, 2020 at 12:34 PM Elliotte Rusty Harold < ***@***.***> wrote: ***@***.**** approved this pull request. ------------------------------ In google-oauth-client-jetty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java <#433 (comment)> : > } catch (Exception e) { Throwables.propagateIfPossible(e); throw new IOException(e); } - return "https://" + connector.getHost() + ":" + port + callbackPath; + return "https://" + this.getHost() + ":" + port + callbackPath; + } + + /* + *Copied from Jetty findFreePort() as referenced by: https://gist.github.com/vorburger/3429822 + */ + + private int findOpenPort() { this could use try with resources — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#433?email_source=notifications&email_token=ABVKI2FDHDANR6ZZA2IKVD3RDVUP7A5CNFSM4KXHF632YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWEOEQQ#pullrequestreview-361292354>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVKI2E4SQJY7QODGHMI77TRDVUP7ANCNFSM4KXHF63Q> .

--

------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ Eric H. Raskin 914-765-0500 x120 or *315-338-4461 (direct)* Professional Advertising Systems Inc. fax: 914-765-0500 or *315-338-4461 (direct)* 200 Business Park Drive Suite 304 [email protected] Armonk, NY 10504 http://www.paslists.com

elharo · 2020-02-19T21:49:05Z

...ty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java

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


why is this code commented out?

ericraskin · 2020-02-19T22:03:46Z

It should have been removed. It is the Jetty 9.4 code. I left it there because I wasn't sure which way we are going (updating Jetty or going with HttpServer). WIll remove it now.

…

On Wed, Feb 19, 2020 at 4:49 PM Elliotte Rusty Harold < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In google-oauth-client-jetty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java <#433 (comment)> : > @@ -115,28 +133,48 @@ public LocalServerReceiver() { @OverRide public String getRedirectUri() throws IOException { + + server = HttpServer.create(new InetSocketAddress(port != -1 ? port : findOpenPort()), 0); + HttpContext context = server.createContext(callbackPath, new CallbackHandler()); + server.setExecutor(null); +/* why is this code commented out? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#433?email_source=notifications&email_token=ABVKI2FWKJIY7PD76HDZIYDRDWSN7A5CNFSM4KXHF632YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWFXYZQ#pullrequestreview-361462886>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVKI2FPMWAUIR3YKA2N2ALRDWSN7ANCNFSM4KXHF63Q> .

--

------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ Eric H. Raskin 914-765-0500 x120 or *315-338-4461 (direct)* Professional Advertising Systems Inc. fax: 914-765-0500 or *315-338-4461 (direct)* 200 Business Park Drive Suite 304 [email protected] Armonk, NY 10504 http://www.paslists.com

…r now.

elharo · 2020-02-20T00:51:47Z

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:

elharo · 2020-02-20T00:52:36Z

...ty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java

  }

  /**
   * 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()}


four space indent

elharo · 2020-02-20T00:52:50Z

...ty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java

   * @throws IOException if the server receives an error code (through an HTTP request
-   *    parameter {@code error})
+   *                     parameter {@code error})


only four space indent, per google style

elharo · 2020-02-20T00:54:30Z

...ty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java

+  }
+
+  /*
+   *Copied from Jetty findFreePort() as referenced by: https://gist.github.com/vorburger/3429822


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

elharo · 2020-02-20T00:55:12Z

...ty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java

@@ -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.*;


Google style is to never use wildcard imports.

ericraskin · 2020-02-20T14:12:22Z

[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?

elharo

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 · 2020-02-20T14:31:37Z

Thank you! I am also searching around to see if anyone else is having this issue and how they got around it.

…

On Thu, Feb 20, 2020 at 9:28 AM Elliotte Rusty Harold < ***@***.***> wrote: ***@***.**** commented on this pull request. 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. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#433?email_source=notifications&email_token=ABVKI2D4J5XCXYX24V4D6BTRD2HQXA5CNFSM4KXHF632YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWJNJYY#pullrequestreview-361944291>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVKI2GDF663DQ6XS4UJ3YDRD2HQXANCNFSM4KXHF63Q> .

--

------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ Eric H. Raskin 914-765-0500 x120 or *315-338-4461 (direct)* Professional Advertising Systems Inc. fax: 914-765-0500 or *315-338-4461 (direct)* 200 Business Park Drive Suite 304 [email protected] Armonk, NY 10504 http://www.paslists.com

ericraskin · 2020-02-20T14:57:47Z

I also added a Checkstyle plugin to my Intellij editor. It pointed out a few things, but none of them were the try with resources. I will push the update, but of course it will fail again.

…

On Thu, Feb 20, 2020 at 9:31 AM Eric Raskin ***@***.***> wrote: Thank you! I am also searching around to see if anyone else is having this issue and how they got around it. On Thu, Feb 20, 2020 at 9:28 AM Elliotte Rusty Harold < ***@***.***> wrote: > ***@***.**** commented on this pull request. > > 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. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#433?email_source=notifications&email_token=ABVKI2D4J5XCXYX24V4D6BTRD2HQXA5CNFSM4KXHF632YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWJNJYY#pullrequestreview-361944291>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABVKI2GDF663DQ6XS4UJ3YDRD2HQXANCNFSM4KXHF63Q> > . > -- ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ Eric H. Raskin 914-765-0500 x120 or *315-338-4461 (direct)* Professional Advertising Systems Inc. fax: 914-765-0500 or *315-338-4461 (direct)* 200 Business Park Drive Suite 304 ***@***.*** Armonk, NY 10504 http://www.paslists.com

--

------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ Eric H. Raskin 914-765-0500 x120 or *315-338-4461 (direct)* Professional Advertising Systems Inc. fax: 914-765-0500 or *315-338-4461 (direct)* 200 Business Park Drive Suite 304 [email protected] Armonk, NY 10504 http://www.paslists.com

elharo · 2020-02-20T14:59:50Z

Running mvn verify from the command line might reproduce the error locally.

…

On Thu, Feb 20, 2020 at 9:57 AM ericraskin ***@***.***> wrote: I also added a Checkstyle plugin to my Intellij editor. It pointed out a few things, but none of them were the try with resources. I will push the update, but of course it will fail again. On Thu, Feb 20, 2020 at 9:31 AM Eric Raskin ***@***.***> wrote: > Thank you! I am also searching around to see if anyone else is > having this issue and how they got around it. > > On Thu, Feb 20, 2020 at 9:28 AM Elliotte Rusty Harold < > ***@***.***> wrote: > >> ***@***.**** commented on this pull request. >> >> 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. >> >> — >> You are receiving this because you authored the thread. >> Reply to this email directly, view it on GitHub >> < #433?email_source=notifications&email_token=ABVKI2D4J5XCXYX24V4D6BTRD2HQXA5CNFSM4KXHF632YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWJNJYY#pullrequestreview-361944291 >, >> or unsubscribe >> < https://github.com/notifications/unsubscribe-auth/ABVKI2GDF663DQ6XS4UJ3YDRD2HQXANCNFSM4KXHF63Q > >> . >> > > > -- > > > ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ > > Eric H. Raskin > 914-765-0500 x120 or *315-338-4461 > (direct)* > > Professional Advertising Systems Inc. > fax: 914-765-0500 or *315-338-4461 (direct)* > > 200 Business Park Drive Suite 304 > ***@***.*** > > Armonk, NY 10504 > http://www.paslists.com > > -- ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ Eric H. Raskin 914-765-0500 x120 or *315-338-4461 (direct)* Professional Advertising Systems Inc. fax: 914-765-0500 or *315-338-4461 (direct)* 200 Business Park Drive Suite 304 ***@***.*** Armonk, NY 10504 http://www.paslists.com — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#433?email_source=notifications&email_token=AAHVP2G2DQG6DSIQNCEWDN3RD2K63A5CNFSM4KXHF632YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMONKGA#issuecomment-589092120>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHVP2HCAI6OVNECUGIZVATRD2K63ANCNFSM4KXHF63Q> .

-- Elliotte Rusty Harold [email protected]

ericraskin requested a review from a team as a code owner February 18, 2020 15:50

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 18, 2020

chingor13 closed this Feb 18, 2020

elharo reviewed Feb 18, 2020

View reviewed changes

chingor13 reopened this Feb 18, 2020

elharo changed the title ~~feat: Replace Jetty with HttpServer.~~ deps: replace Jetty with HttpServer. Feb 18, 2020

elharo added the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 18, 2020

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 18, 2020

elharo approved these changes Feb 19, 2020

View reviewed changes

feat: Rewrite findOpenSocket using try with resources

48fb3f2

elharo reviewed Feb 19, 2020

View reviewed changes

fix: Remove commented out Jetty 9.4 code since we are using HttpServe…

84c457e

…r now.

elharo approved these changes Feb 19, 2020

View reviewed changes

elharo added the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020

elharo reviewed Feb 20, 2020

View reviewed changes

fix: Enforce Google coding style.

2754068

ericraskin changed the title ~~deps: replace Jetty with HttpServer.~~ deps: replace Jetty with HttpServer Feb 20, 2020

elharo added the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020

elharo reviewed Feb 20, 2020

View reviewed changes

chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 20, 2020

elharo merged commit bcabce2 into googleapis:master Feb 20, 2020

yoshi-automation mentioned this pull request Feb 21, 2020

chore: update common templates #440

Merged

release-please bot mentioned this pull request Jan 4, 2022

chore(main): release google-oauth-java-client 1.33.0 #791

Closed

This was referenced Sep 18, 2024

HttpServer artifact in addition to Jetty #434

Closed

Upgrade to Jetty 9 #130

Closed

ericraskin deleted the replace-Jetty-with-HttpServer branch September 18, 2024 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: replace Jetty with HttpServer #433

deps: replace Jetty with HttpServer #433

ericraskin commented Feb 18, 2020 •

edited

Loading

chingor13 commented Feb 18, 2020

elharo left a comment

elharo Feb 19, 2020

ericraskin commented Feb 19, 2020 via email

elharo Feb 19, 2020

ericraskin commented Feb 19, 2020 via email

elharo commented Feb 20, 2020

elharo Feb 20, 2020

elharo Feb 20, 2020

elharo Feb 20, 2020

elharo Feb 20, 2020

ericraskin commented Feb 20, 2020 •

edited

Loading

elharo left a comment

ericraskin commented Feb 20, 2020 via email

ericraskin commented Feb 20, 2020 via email

elharo commented Feb 20, 2020 via email

deps: replace Jetty with HttpServer #433

deps: replace Jetty with HttpServer #433

Conversation

ericraskin commented Feb 18, 2020 • edited Loading

chingor13 commented Feb 18, 2020

elharo left a comment

Choose a reason for hiding this comment

elharo Feb 19, 2020

Choose a reason for hiding this comment

ericraskin commented Feb 19, 2020 via email

elharo Feb 19, 2020

Choose a reason for hiding this comment

ericraskin commented Feb 19, 2020 via email

elharo commented Feb 20, 2020

elharo Feb 20, 2020

Choose a reason for hiding this comment

elharo Feb 20, 2020

Choose a reason for hiding this comment

elharo Feb 20, 2020

Choose a reason for hiding this comment

elharo Feb 20, 2020

Choose a reason for hiding this comment

ericraskin commented Feb 20, 2020 • edited Loading

elharo left a comment

Choose a reason for hiding this comment

ericraskin commented Feb 20, 2020 via email

ericraskin commented Feb 20, 2020 via email

elharo commented Feb 20, 2020 via email

ericraskin commented Feb 18, 2020 •

edited

Loading

ericraskin commented Feb 20, 2020 •

edited

Loading