You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2017/12/06 07:36:29 UTC

Review Request 64362: Update to guice 4.1.0, switch from jersey to resteasy

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64362/
-----------------------------------------------------------

Review request for Aurora, David McLaughlin, Jordan Ly, and Stephan Erb.

Repository: aurora

Description
-------

Upgrading guice was the original goal with this patch, which pulled along several other dependencies. Guice 3 [suffers from obscure errors](https://github.com/google/guice/issues/757) when creating binding error messages with java 8 and lambdas. This has been a frequent source of frustration since we first upgraded to java 8 mid-2015.

I've gone spelunking down this path numerous times, and frequently hit a wall with jersey. We needed to upgrade jersey-guice, to upgrade jersey, to upgrade guice. jersey introduced their own dependency injection (HK2) in jersey 2.0, which complicated matters. There have been some promising developments since (hk2-guice [bridge](https://javaee.github.io/hk2/guice-bridge.html#bi-directional-hk2-guice-bridge), 2.26 [abstracted HK2](https://jersey.github.io/release-notes/2.26.html), and several [projects](https://github.com/Squarespace/jersey2-guice) have emerged to solve the issue). Unfortunately, each avenue failed with some combination of not working well with our application design, or i just plain couldn't get it working. I began to look beyond jersey.

This left restlet and resteasy as the most common alternatives. I balked early at restlet due to their guice integration being [apparently](https://github.com/restlet/restlet-framework-java/commits/master/modules/org.restlet.ext.guice) [dormant](https://stackoverflow.com/questions/8227583/what-is-the-status-of-org-restlet-ext-guice).

Fortunately i achieved some early wins with resteasy! Migrating was straightforward with a small patch based on some examples.

However, i hit a hurdle with shiro-guice. It [needed to be updated](https://issues.apache.org/jira/browse/SHIRO-493) to work with guice 4, and there were some necessary API changes. No big deal, just the `filterConfig()` nesting you see in this patch. This revealed a deeper issue with binding custom `Filter`s with `ShiroWebModule`. Previously, `ShiroWebModule` would effectively only `bind()` keys [they define](https://github.com/apache/shiro/blob/f326fd3814f672464deb11c3dadadb27af618eec/support/guice/src/main/java/org/apache/shiro/guice/web/ShiroWebModule.java#L65-L86), allowing the API user to `bind()` custom keys. The [patch](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1c) to support guice 4 changed that, and `bind()` will be [called](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1cR183) on these custom keys. In our case, this caused a du
plicate binding.

The simplest workaround to this was to avoid `bind()`ing the custom `afterAuthFilter` key, and use the custom type as the key type (e.g. `Key.get(CountingFilter.class)` rather than `Key.get(Filter.class)`).

Lastly, `GuiceResteasyBootstrapServletContextListener` does not integrate with `GuiceServletContextListener` in the way `GuiceFilter` [demands](https://github.com/google/guice/blob/e7bef34ef6379735e2a58df1b23f351bb7e30e44/extensions/servlet/src/com/google/inject/servlet/ServletModule.java#L322-L323), which necessitated the passing of `ServletContext` you see in this patch.

I can't say i'm happy with the outcome here, but i am overall happier given that guice is upgraded.

Computers are hard!

Diffs
-----

build.gradle fd606a27bbe0f79a9358ddb74425437ddb42d71e
src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a
src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java a19663acb20c66ec14e41473a72fa5035146a6cc
src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java d81671c0bf3634bfce851937f4fc9135dd5f563f
src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java a3f6941f6b335791455545e0745b03cf58899f95
src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java e8ef6bd7f9b0c47c5f17da7a040ce1497922a807
src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java 43fa315a9a25bf29f53d8fd986f198d3f9ef1a74
src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java d3c7ac985bbcf734aace2cad50af970619ffb536
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java 20e3d6255921cf24da7cf66101a72c537a0a7c68
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosPermissiveAuthenticationFilterTest.java 001846765f8f9e8b970ab3b3d2a495652819319c

Diff: https://reviews.apache.org/r/64362/diff/1/

Testing
-------

end-to-end tests are broken on master, will fix separately before landing this patch

Thanks,

Bill Farner

Re: Review Request 64362: Update to guice 4.1.0, switch from jersey to resteasy

Posted by Bill Farner <wf...@apache.org>.


> On Dec. 6, 2017, 4:22 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
> > Line 211 (original), 218 (patched)
> > <https://reviews.apache.org/r/64362/diff/1/?file=1909454#file1909454line218>
> >
> >     To fail or to pass?

Whoops, stale note relating to the shiro-guice pain.  Removed.


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64362/#review192977
-----------------------------------------------------------


On Dec. 5, 2017, 11:36 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64362/
> -----------------------------------------------------------
> 
> (Updated Dec. 5, 2017, 11:36 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Jordan Ly, and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Upgrading guice was the original goal with this patch, which pulled along several other dependencies.  Guice 3 [suffers from obscure errors](https://github.com/google/guice/issues/757) when creating binding error messages with java 8 and lambdas.  This has been a frequent source of frustration since we first upgraded to java 8 mid-2015.
> 
> I've gone spelunking down this path numerous times, and frequently hit a wall with jersey.  We needed to upgrade jersey-guice, to upgrade jersey, to upgrade guice.  jersey introduced their own dependency injection (HK2) in jersey 2.0, which complicated matters.  There have been some promising developments since (hk2-guice [bridge](https://javaee.github.io/hk2/guice-bridge.html#bi-directional-hk2-guice-bridge), 2.26 [abstracted HK2](https://jersey.github.io/release-notes/2.26.html), and several [projects](https://github.com/Squarespace/jersey2-guice) have emerged to solve the issue).  Unfortunately, each avenue failed with some combination of not working well with our application design, or i just plain couldn't get it working.  I began to look beyond jersey.
> 
> This left restlet and resteasy as the most common alternatives.  I balked early at restlet due to their guice integration being [apparently](https://github.com/restlet/restlet-framework-java/commits/master/modules/org.restlet.ext.guice) [dormant](https://stackoverflow.com/questions/8227583/what-is-the-status-of-org-restlet-ext-guice).
> 
> Fortunately i achieved some early wins with resteasy!  Migrating was straightforward with a small patch based on some examples.
> 
> However, i hit a hurdle with shiro-guice.  It [needed to be updated](https://issues.apache.org/jira/browse/SHIRO-493) to work with guice 4, and there were some necessary API changes.  No big deal, just the `filterConfig()` nesting you see in this patch.  This revealed a deeper issue with binding custom `Filter`s with `ShiroWebModule`.  Previously, `ShiroWebModule` would effectively only `bind()` keys [they define](https://github.com/apache/shiro/blob/f326fd3814f672464deb11c3dadadb27af618eec/support/guice/src/main/java/org/apache/shiro/guice/web/ShiroWebModule.java#L65-L86), allowing the API user to `bind()` custom keys.  The [patch](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1c) to support guice 4 changed that, and `bind()` will be [called](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1cR183) on these custom keys.  In our case, this caused a 
 duplicate binding.
> 
> The simplest workaround to this was to avoid `bind()`ing the custom `afterAuthFilter` key, and use the custom type as the key type (e.g. `Key.get(CountingFilter.class)` rather than `Key.get(Filter.class)`).
> 
> Lastly, `GuiceResteasyBootstrapServletContextListener` does not integrate with `GuiceServletContextListener` in the way `GuiceFilter` [demands](https://github.com/google/guice/blob/e7bef34ef6379735e2a58df1b23f351bb7e30e44/extensions/servlet/src/com/google/inject/servlet/ServletModule.java#L322-L323), which necessitated the passing of `ServletContext` you see in this patch.
> 
> I can't say i'm happy with the outcome here, but i am overall happier given that guice is upgraded.
> 
> Computers are hard!
> 
> 
> Diffs
> -----
> 
>   build.gradle fd606a27bbe0f79a9358ddb74425437ddb42d71e 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java a19663acb20c66ec14e41473a72fa5035146a6cc 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java d81671c0bf3634bfce851937f4fc9135dd5f563f 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java a3f6941f6b335791455545e0745b03cf58899f95 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java e8ef6bd7f9b0c47c5f17da7a040ce1497922a807 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java 43fa315a9a25bf29f53d8fd986f198d3f9ef1a74 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java d3c7ac985bbcf734aace2cad50af970619ffb536 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java 20e3d6255921cf24da7cf66101a72c537a0a7c68 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosPermissiveAuthenticationFilterTest.java 001846765f8f9e8b970ab3b3d2a495652819319c 
> 
> 
> Diff: https://reviews.apache.org/r/64362/diff/1/
> 
> 
> Testing
> -------
> 
> end-to-end tests are broken on master, will fix separately before landing this patch
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 64362: Update to guice 4.1.0, switch from jersey to resteasy

Posted by Stephan Erb <se...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64362/#review192977
-----------------------------------------------------------


Ship it!




Wow, pretty nice! 

I feel your pain. I also tried once to update Guice and failed miserably.


build.gradle
Lines 375-377 (original), 384-386 (patched)
<https://reviews.apache.org/r/64362/#comment271414>

    License is Apache 2.0, so this looks good.
    
    https://github.com/resteasy/Resteasy/blob/master/License.html



src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
Line 211 (original), 218 (patched)
<https://reviews.apache.org/r/64362/#comment271413>

    To fail or to pass?


The new guice version is great! 

Running `./gradlew jmh -Pbenchmarks='SnapshotBenchmarks.*'` with master returns this error that gives no real clue how to fix it:

```
com.google.inject.internal.util.$ComputationException: java.lang.ArrayIndexOutOfBoundsException: 22081
	at com.google.inject.internal.util.$MapMaker$StrategyImpl.compute(MapMaker.java:553)
	at com.google.inject.internal.util.$MapMaker$StrategyImpl.compute(MapMaker.java:419)
	at com.google.inject.internal.util.$CustomConcurrentHashMap$ComputingImpl.get(CustomConcurrentHashMap.java:2041)
	at com.google.inject.internal.util.$StackTraceElements.forMember(StackTraceElements.java:53)
	at com.google.inject.internal.Errors.formatInjectionPoint(Errors.java:716)
	at com.google.inject.internal.Errors.formatSource(Errors.java:678)
	at com.google.inject.internal.Errors.format(Errors.java:555)
	at com.google.inject.CreationException.getMessage(CreationException.java:48)
	at java.lang.Throwable.getLocalizedMessage(Throwable.java:391)
	at java.lang.Throwable.toString(Throwable.java:480)
	at java.lang.Throwable.<init>(Throwable.java:311)
	at java.lang.Exception.<init>(Exception.java:102)
	at java.lang.RuntimeException.<init>(RuntimeException.java:96)
	at org.openjdk.jmh.runner.BenchmarkException.<init>(BenchmarkException.java:34)
	at org.openjdk.jmh.runner.BenchmarkHandler.runIteration(BenchmarkHandler.java:438)
	at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:263)
	at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:235)
	at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:142)
	at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76)
	at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72)
	at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84)
Caused by: java.lang.ArrayIndexOutOfBoundsException: 22081
	at com.google.inject.internal.asm.$ClassReader.<init>(Unknown Source)
	at com.google.inject.internal.asm.$ClassReader.<init>(Unknown Source)
	at com.google.inject.internal.asm.$ClassReader.<init>(Unknown Source)
	at com.google.inject.internal.util.$LineNumbers.<init>(LineNumbers.java:62)
	at com.google.inject.internal.util.$StackTraceElements$1.apply(StackTraceElements.java:36)
	at com.google.inject.internal.util.$StackTraceElements$1.apply(StackTraceElements.java:33)
	at com.google.inject.internal.util.$MapMaker$StrategyImpl.compute(MapMaker.java:549)
	... 20 more
```

Running the same benchmark with this patch:

```
com.google.inject.CreationException: Unable to create injector, see the following errors:

1) No implementation for org.apache.aurora.scheduler.TierManager was bound.
  while locating org.apache.aurora.scheduler.TierManager
    for the 1st parameter of org.apache.aurora.scheduler.storage.durability.ThriftBackfill.<init>(ThriftBackfill.java:56)
  while locating org.apache.aurora.scheduler.storage.durability.ThriftBackfill
    for the 4th parameter of org.apache.aurora.scheduler.storage.log.SnapshotStoreImpl.<init>(SnapshotStoreImpl.java:267)
  at org.apache.aurora.benchmark.SnapshotBenchmarks$RestoreSnapshotWithUpdatesBenchmark$1.configure(SnapshotBenchmarks.java:90)

2) No implementation for org.apache.aurora.scheduler.storage.Storage annotated with @org.apache.aurora.scheduler.storage.Storage$Volatile() was bound.
  while locating org.apache.aurora.scheduler.storage.Storage annotated with @org.apache.aurora.scheduler.storage.Storage$Volatile()
    for the 3rd parameter of org.apache.aurora.scheduler.storage.log.SnapshotStoreImpl.<init>(SnapshotStoreImpl.java:267)
  at org.apache.aurora.benchmark.SnapshotBenchmarks$RestoreSnapshotWithUpdatesBenchmark$1.configure(SnapshotBenchmarks.java:90)

2 errors
	at com.google.inject.internal.Errors.throwCreationExceptionIfErrorsExist(Errors.java:470)
	at com.google.inject.internal.InternalInjectorCreator.initializeStatically(InternalInjectorCreator.java:155)
	at com.google.inject.internal.InternalInjectorCreator.build(InternalInjectorCreator.java:107)
	at com.google.inject.Guice.createInjector(Guice.java:99)
	at com.google.inject.Guice.createInjector(Guice.java:73)
	at com.google.inject.Guice.createInjector(Guice.java:62)
	at org.apache.aurora.benchmark.SnapshotBenchmarks$RestoreSnapshotWithUpdatesBenchmark.getSnapshotStore(SnapshotBenchmarks.java:84)
	at org.apache.aurora.benchmark.SnapshotBenchmarks$RestoreSnapshotWithUpdatesBenchmark.setUp(SnapshotBenchmarks.java:68)
	at org.apache.aurora.benchmark.generated.SnapshotBenchmarks_RestoreSnapshotWithUpdatesBenchmark_run_jmhTest._jmh_tryInit_f_restoresnapshotwithupdatesbenchmark0_0(SnapshotBenchmarks_RestoreSnapshotWithUpdatesBenchmark_run_jmhTest.java:334)
	at org.apache.aurora.benchmark.generated.SnapshotBenchmarks_RestoreSnapshotWithUpdatesBenchmark_run_jmhTest.run_Throughput(SnapshotBenchmarks_RestoreSnapshotWithUpdatesBenchmark_run_jmhTest.java:69)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.openjdk.jmh.runner.BenchmarkHandler$BenchmarkTask.call(BenchmarkHandler.java:464)
	at org.openjdk.jmh.runner.BenchmarkHandler$BenchmarkTask.call(BenchmarkHandler.java:448)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:748)
```

- Stephan Erb


On Dec. 6, 2017, 8:36 a.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64362/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2017, 8:36 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Jordan Ly, and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Upgrading guice was the original goal with this patch, which pulled along several other dependencies.  Guice 3 [suffers from obscure errors](https://github.com/google/guice/issues/757) when creating binding error messages with java 8 and lambdas.  This has been a frequent source of frustration since we first upgraded to java 8 mid-2015.
> 
> I've gone spelunking down this path numerous times, and frequently hit a wall with jersey.  We needed to upgrade jersey-guice, to upgrade jersey, to upgrade guice.  jersey introduced their own dependency injection (HK2) in jersey 2.0, which complicated matters.  There have been some promising developments since (hk2-guice [bridge](https://javaee.github.io/hk2/guice-bridge.html#bi-directional-hk2-guice-bridge), 2.26 [abstracted HK2](https://jersey.github.io/release-notes/2.26.html), and several [projects](https://github.com/Squarespace/jersey2-guice) have emerged to solve the issue).  Unfortunately, each avenue failed with some combination of not working well with our application design, or i just plain couldn't get it working.  I began to look beyond jersey.
> 
> This left restlet and resteasy as the most common alternatives.  I balked early at restlet due to their guice integration being [apparently](https://github.com/restlet/restlet-framework-java/commits/master/modules/org.restlet.ext.guice) [dormant](https://stackoverflow.com/questions/8227583/what-is-the-status-of-org-restlet-ext-guice).
> 
> Fortunately i achieved some early wins with resteasy!  Migrating was straightforward with a small patch based on some examples.
> 
> However, i hit a hurdle with shiro-guice.  It [needed to be updated](https://issues.apache.org/jira/browse/SHIRO-493) to work with guice 4, and there were some necessary API changes.  No big deal, just the `filterConfig()` nesting you see in this patch.  This revealed a deeper issue with binding custom `Filter`s with `ShiroWebModule`.  Previously, `ShiroWebModule` would effectively only `bind()` keys [they define](https://github.com/apache/shiro/blob/f326fd3814f672464deb11c3dadadb27af618eec/support/guice/src/main/java/org/apache/shiro/guice/web/ShiroWebModule.java#L65-L86), allowing the API user to `bind()` custom keys.  The [patch](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1c) to support guice 4 changed that, and `bind()` will be [called](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1cR183) on these custom keys.  In our case, this caused a 
 duplicate binding.
> 
> The simplest workaround to this was to avoid `bind()`ing the custom `afterAuthFilter` key, and use the custom type as the key type (e.g. `Key.get(CountingFilter.class)` rather than `Key.get(Filter.class)`).
> 
> Lastly, `GuiceResteasyBootstrapServletContextListener` does not integrate with `GuiceServletContextListener` in the way `GuiceFilter` [demands](https://github.com/google/guice/blob/e7bef34ef6379735e2a58df1b23f351bb7e30e44/extensions/servlet/src/com/google/inject/servlet/ServletModule.java#L322-L323), which necessitated the passing of `ServletContext` you see in this patch.
> 
> I can't say i'm happy with the outcome here, but i am overall happier given that guice is upgraded.
> 
> Computers are hard!
> 
> 
> Diffs
> -----
> 
>   build.gradle fd606a27bbe0f79a9358ddb74425437ddb42d71e 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java a19663acb20c66ec14e41473a72fa5035146a6cc 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java d81671c0bf3634bfce851937f4fc9135dd5f563f 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java a3f6941f6b335791455545e0745b03cf58899f95 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java e8ef6bd7f9b0c47c5f17da7a040ce1497922a807 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java 43fa315a9a25bf29f53d8fd986f198d3f9ef1a74 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java d3c7ac985bbcf734aace2cad50af970619ffb536 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java 20e3d6255921cf24da7cf66101a72c537a0a7c68 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosPermissiveAuthenticationFilterTest.java 001846765f8f9e8b970ab3b3d2a495652819319c 
> 
> 
> Diff: https://reviews.apache.org/r/64362/diff/1/
> 
> 
> Testing
> -------
> 
> end-to-end tests are broken on master, will fix separately before landing this patch
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 64362: Update to guice 4.1.0, switch from jersey to resteasy

Posted by Aurora ReviewBot <wf...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64362/#review192971
-----------------------------------------------------------


Ship it!




Master (e74fb32) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On Dec. 6, 2017, 1:06 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64362/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2017, 1:06 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Jordan Ly, and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Upgrading guice was the original goal with this patch, which pulled along several other dependencies.  Guice 3 [suffers from obscure errors](https://github.com/google/guice/issues/757) when creating binding error messages with java 8 and lambdas.  This has been a frequent source of frustration since we first upgraded to java 8 mid-2015.
> 
> I've gone spelunking down this path numerous times, and frequently hit a wall with jersey.  We needed to upgrade jersey-guice, to upgrade jersey, to upgrade guice.  jersey introduced their own dependency injection (HK2) in jersey 2.0, which complicated matters.  There have been some promising developments since (hk2-guice [bridge](https://javaee.github.io/hk2/guice-bridge.html#bi-directional-hk2-guice-bridge), 2.26 [abstracted HK2](https://jersey.github.io/release-notes/2.26.html), and several [projects](https://github.com/Squarespace/jersey2-guice) have emerged to solve the issue).  Unfortunately, each avenue failed with some combination of not working well with our application design, or i just plain couldn't get it working.  I began to look beyond jersey.
> 
> This left restlet and resteasy as the most common alternatives.  I balked early at restlet due to their guice integration being [apparently](https://github.com/restlet/restlet-framework-java/commits/master/modules/org.restlet.ext.guice) [dormant](https://stackoverflow.com/questions/8227583/what-is-the-status-of-org-restlet-ext-guice).
> 
> Fortunately i achieved some early wins with resteasy!  Migrating was straightforward with a small patch based on some examples.
> 
> However, i hit a hurdle with shiro-guice.  It [needed to be updated](https://issues.apache.org/jira/browse/SHIRO-493) to work with guice 4, and there were some necessary API changes.  No big deal, just the `filterConfig()` nesting you see in this patch.  This revealed a deeper issue with binding custom `Filter`s with `ShiroWebModule`.  Previously, `ShiroWebModule` would effectively only `bind()` keys [they define](https://github.com/apache/shiro/blob/f326fd3814f672464deb11c3dadadb27af618eec/support/guice/src/main/java/org/apache/shiro/guice/web/ShiroWebModule.java#L65-L86), allowing the API user to `bind()` custom keys.  The [patch](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1c) to support guice 4 changed that, and `bind()` will be [called](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1cR183) on these custom keys.  In our case, this caused a 
 duplicate binding.
> 
> The simplest workaround to this was to avoid `bind()`ing the custom `afterAuthFilter` key, and use the custom type as the key type (e.g. `Key.get(CountingFilter.class)` rather than `Key.get(Filter.class)`).
> 
> Lastly, `GuiceResteasyBootstrapServletContextListener` does not integrate with `GuiceServletContextListener` in the way `GuiceFilter` [demands](https://github.com/google/guice/blob/e7bef34ef6379735e2a58df1b23f351bb7e30e44/extensions/servlet/src/com/google/inject/servlet/ServletModule.java#L322-L323), which necessitated the passing of `ServletContext` you see in this patch.
> 
> I can't say i'm happy with the outcome here, but i am overall happier given that guice is upgraded.
> 
> Computers are hard!
> 
> 
> Diffs
> -----
> 
>   build.gradle fd606a27bbe0f79a9358ddb74425437ddb42d71e 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java a19663acb20c66ec14e41473a72fa5035146a6cc 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java d81671c0bf3634bfce851937f4fc9135dd5f563f 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java a3f6941f6b335791455545e0745b03cf58899f95 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java e8ef6bd7f9b0c47c5f17da7a040ce1497922a807 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java 43fa315a9a25bf29f53d8fd986f198d3f9ef1a74 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java d3c7ac985bbcf734aace2cad50af970619ffb536 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java 20e3d6255921cf24da7cf66101a72c537a0a7c68 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosPermissiveAuthenticationFilterTest.java 001846765f8f9e8b970ab3b3d2a495652819319c 
> 
> 
> Diff: https://reviews.apache.org/r/64362/diff/1/
> 
> 
> Testing
> -------
> 
> end-to-end tests are broken on master, will fix separately before landing this patch
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 64362: Update to guice 4.1.0, switch from jersey to resteasy

Posted by Aurora ReviewBot <wf...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64362/#review193004
-----------------------------------------------------------



Master (e74fb32) is red with this patch.
  ./build-support/jenkins/build.sh

                     DEBUG] Flipping task state from FINALIZING to SUCCESS
                     DEBUG] Flipping task state from FINALIZING to SUCCESS
                     DEBUG] Flipping task state from FINALIZING to SUCCESS
                     DEBUG] Flipping task state from FINALIZING to SUCCESS
                     DEBUG] Flipping task state from FINALIZING to SUCCESS
                     DEBUG] Flipping task state from FINALIZING to SUCCESS
                     DEBUG] Flipping task state from FINALIZING to SUCCESS
                     DEBUG] Flipping task state from FINALIZING to SUCCESS
                     DEBUG] Flipping task state from FINALIZING to SUCCESS
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                     DEBUG] /tmp/tmpmJai22/checkpoints/hello_world-001/runner has no data (current offset = 613)
                      generated xml file: /home/jenkins/jenkins-slave/workspace/AuroraBot/dist/test-results/aaf4d108c31293299a0839bdc404a91802f80937.xml 
                     [1m[31m 1 failed, 794 passed, 6 skipped, 1 warnings in 319.93 seconds [0m
                     
FAILURE


17:41:17 06:00   [complete][31m
               FAILURE[0m


I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On Dec. 6, 2017, 3:31 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64362/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2017, 3:31 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Jordan Ly, and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Upgrading guice was the original goal with this patch, which pulled along several other dependencies.  Guice 3 [suffers from obscure errors](https://github.com/google/guice/issues/757) when creating binding error messages with java 8 and lambdas.  This has been a frequent source of frustration since we first upgraded to java 8 mid-2015.
> 
> I've gone spelunking down this path numerous times, and frequently hit a wall with jersey.  We needed to upgrade jersey-guice, to upgrade jersey, to upgrade guice.  jersey introduced their own dependency injection (HK2) in jersey 2.0, which complicated matters.  There have been some promising developments since (hk2-guice [bridge](https://javaee.github.io/hk2/guice-bridge.html#bi-directional-hk2-guice-bridge), 2.26 [abstracted HK2](https://jersey.github.io/release-notes/2.26.html), and several [projects](https://github.com/Squarespace/jersey2-guice) have emerged to solve the issue).  Unfortunately, each avenue failed with some combination of not working well with our application design, or i just plain couldn't get it working.  I began to look beyond jersey.
> 
> This left restlet and resteasy as the most common alternatives.  I balked early at restlet due to their guice integration being [apparently](https://github.com/restlet/restlet-framework-java/commits/master/modules/org.restlet.ext.guice) [dormant](https://stackoverflow.com/questions/8227583/what-is-the-status-of-org-restlet-ext-guice).
> 
> Fortunately i achieved some early wins with resteasy!  Migrating was straightforward with a small patch based on some examples.
> 
> However, i hit a hurdle with shiro-guice.  It [needed to be updated](https://issues.apache.org/jira/browse/SHIRO-493) to work with guice 4, and there were some necessary API changes.  No big deal, just the `filterConfig()` nesting you see in this patch.  This revealed a deeper issue with binding custom `Filter`s with `ShiroWebModule`.  Previously, `ShiroWebModule` would effectively only `bind()` keys [they define](https://github.com/apache/shiro/blob/f326fd3814f672464deb11c3dadadb27af618eec/support/guice/src/main/java/org/apache/shiro/guice/web/ShiroWebModule.java#L65-L86), allowing the API user to `bind()` custom keys.  The [patch](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1c) to support guice 4 changed that, and `bind()` will be [called](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1cR183) on these custom keys.  In our case, this caused a 
 duplicate binding.
> 
> The simplest workaround to this was to avoid `bind()`ing the custom `afterAuthFilter` key, and use the custom type as the key type (e.g. `Key.get(CountingFilter.class)` rather than `Key.get(Filter.class)`).
> 
> Lastly, `GuiceResteasyBootstrapServletContextListener` does not integrate with `GuiceServletContextListener` in the way `GuiceFilter` [demands](https://github.com/google/guice/blob/e7bef34ef6379735e2a58df1b23f351bb7e30e44/extensions/servlet/src/com/google/inject/servlet/ServletModule.java#L322-L323), which necessitated the passing of `ServletContext` you see in this patch.
> 
> I can't say i'm happy with the outcome here, but i am overall happier given that guice is upgraded.
> 
> Computers are hard!
> 
> 
> Diffs
> -----
> 
>   build.gradle fd606a27bbe0f79a9358ddb74425437ddb42d71e 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java a19663acb20c66ec14e41473a72fa5035146a6cc 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java d81671c0bf3634bfce851937f4fc9135dd5f563f 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java a3f6941f6b335791455545e0745b03cf58899f95 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java e8ef6bd7f9b0c47c5f17da7a040ce1497922a807 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java 43fa315a9a25bf29f53d8fd986f198d3f9ef1a74 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java d3c7ac985bbcf734aace2cad50af970619ffb536 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java 20e3d6255921cf24da7cf66101a72c537a0a7c68 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosPermissiveAuthenticationFilterTest.java 001846765f8f9e8b970ab3b3d2a495652819319c 
> 
> 
> Diff: https://reviews.apache.org/r/64362/diff/2/
> 
> 
> Testing
> -------
> 
> end-to-end tests are broken on master, will fix separately before landing this patch
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 64362: Update to guice 4.1.0, switch from jersey to resteasy

Posted by Aurora ReviewBot <wf...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64362/#review193047
-----------------------------------------------------------


Ship it!




Master (2754636) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On Dec. 6, 2017, 2:31 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64362/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2017, 2:31 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Jordan Ly, and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Upgrading guice was the original goal with this patch, which pulled along several other dependencies.  Guice 3 [suffers from obscure errors](https://github.com/google/guice/issues/757) when creating binding error messages with java 8 and lambdas.  This has been a frequent source of frustration since we first upgraded to java 8 mid-2015.
> 
> I've gone spelunking down this path numerous times, and frequently hit a wall with jersey.  We needed to upgrade jersey-guice, to upgrade jersey, to upgrade guice.  jersey introduced their own dependency injection (HK2) in jersey 2.0, which complicated matters.  There have been some promising developments since (hk2-guice [bridge](https://javaee.github.io/hk2/guice-bridge.html#bi-directional-hk2-guice-bridge), 2.26 [abstracted HK2](https://jersey.github.io/release-notes/2.26.html), and several [projects](https://github.com/Squarespace/jersey2-guice) have emerged to solve the issue).  Unfortunately, each avenue failed with some combination of not working well with our application design, or i just plain couldn't get it working.  I began to look beyond jersey.
> 
> This left restlet and resteasy as the most common alternatives.  I balked early at restlet due to their guice integration being [apparently](https://github.com/restlet/restlet-framework-java/commits/master/modules/org.restlet.ext.guice) [dormant](https://stackoverflow.com/questions/8227583/what-is-the-status-of-org-restlet-ext-guice).
> 
> Fortunately i achieved some early wins with resteasy!  Migrating was straightforward with a small patch based on some examples.
> 
> However, i hit a hurdle with shiro-guice.  It [needed to be updated](https://issues.apache.org/jira/browse/SHIRO-493) to work with guice 4, and there were some necessary API changes.  No big deal, just the `filterConfig()` nesting you see in this patch.  This revealed a deeper issue with binding custom `Filter`s with `ShiroWebModule`.  Previously, `ShiroWebModule` would effectively only `bind()` keys [they define](https://github.com/apache/shiro/blob/f326fd3814f672464deb11c3dadadb27af618eec/support/guice/src/main/java/org/apache/shiro/guice/web/ShiroWebModule.java#L65-L86), allowing the API user to `bind()` custom keys.  The [patch](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1c) to support guice 4 changed that, and `bind()` will be [called](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1cR183) on these custom keys.  In our case, this caused a 
 duplicate binding.
> 
> The simplest workaround to this was to avoid `bind()`ing the custom `afterAuthFilter` key, and use the custom type as the key type (e.g. `Key.get(CountingFilter.class)` rather than `Key.get(Filter.class)`).
> 
> Lastly, `GuiceResteasyBootstrapServletContextListener` does not integrate with `GuiceServletContextListener` in the way `GuiceFilter` [demands](https://github.com/google/guice/blob/e7bef34ef6379735e2a58df1b23f351bb7e30e44/extensions/servlet/src/com/google/inject/servlet/ServletModule.java#L322-L323), which necessitated the passing of `ServletContext` you see in this patch.
> 
> I can't say i'm happy with the outcome here, but i am overall happier given that guice is upgraded.
> 
> Computers are hard!
> 
> 
> Diffs
> -----
> 
>   build.gradle fd606a27bbe0f79a9358ddb74425437ddb42d71e 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java a19663acb20c66ec14e41473a72fa5035146a6cc 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java d81671c0bf3634bfce851937f4fc9135dd5f563f 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java a3f6941f6b335791455545e0745b03cf58899f95 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java e8ef6bd7f9b0c47c5f17da7a040ce1497922a807 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java 43fa315a9a25bf29f53d8fd986f198d3f9ef1a74 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java d3c7ac985bbcf734aace2cad50af970619ffb536 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java 20e3d6255921cf24da7cf66101a72c537a0a7c68 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosPermissiveAuthenticationFilterTest.java 001846765f8f9e8b970ab3b3d2a495652819319c 
> 
> 
> Diff: https://reviews.apache.org/r/64362/diff/2/
> 
> 
> Testing
> -------
> 
> end-to-end tests pass
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 64362: Update to guice 4.1.0, switch from jersey to resteasy

Posted by Bill Farner <wf...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64362/#review193043
-----------------------------------------------------------



@ReviewBot retry

- Bill Farner


On Dec. 6, 2017, 6:31 a.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64362/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2017, 6:31 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Jordan Ly, and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Upgrading guice was the original goal with this patch, which pulled along several other dependencies.  Guice 3 [suffers from obscure errors](https://github.com/google/guice/issues/757) when creating binding error messages with java 8 and lambdas.  This has been a frequent source of frustration since we first upgraded to java 8 mid-2015.
> 
> I've gone spelunking down this path numerous times, and frequently hit a wall with jersey.  We needed to upgrade jersey-guice, to upgrade jersey, to upgrade guice.  jersey introduced their own dependency injection (HK2) in jersey 2.0, which complicated matters.  There have been some promising developments since (hk2-guice [bridge](https://javaee.github.io/hk2/guice-bridge.html#bi-directional-hk2-guice-bridge), 2.26 [abstracted HK2](https://jersey.github.io/release-notes/2.26.html), and several [projects](https://github.com/Squarespace/jersey2-guice) have emerged to solve the issue).  Unfortunately, each avenue failed with some combination of not working well with our application design, or i just plain couldn't get it working.  I began to look beyond jersey.
> 
> This left restlet and resteasy as the most common alternatives.  I balked early at restlet due to their guice integration being [apparently](https://github.com/restlet/restlet-framework-java/commits/master/modules/org.restlet.ext.guice) [dormant](https://stackoverflow.com/questions/8227583/what-is-the-status-of-org-restlet-ext-guice).
> 
> Fortunately i achieved some early wins with resteasy!  Migrating was straightforward with a small patch based on some examples.
> 
> However, i hit a hurdle with shiro-guice.  It [needed to be updated](https://issues.apache.org/jira/browse/SHIRO-493) to work with guice 4, and there were some necessary API changes.  No big deal, just the `filterConfig()` nesting you see in this patch.  This revealed a deeper issue with binding custom `Filter`s with `ShiroWebModule`.  Previously, `ShiroWebModule` would effectively only `bind()` keys [they define](https://github.com/apache/shiro/blob/f326fd3814f672464deb11c3dadadb27af618eec/support/guice/src/main/java/org/apache/shiro/guice/web/ShiroWebModule.java#L65-L86), allowing the API user to `bind()` custom keys.  The [patch](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1c) to support guice 4 changed that, and `bind()` will be [called](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1cR183) on these custom keys.  In our case, this caused a 
 duplicate binding.
> 
> The simplest workaround to this was to avoid `bind()`ing the custom `afterAuthFilter` key, and use the custom type as the key type (e.g. `Key.get(CountingFilter.class)` rather than `Key.get(Filter.class)`).
> 
> Lastly, `GuiceResteasyBootstrapServletContextListener` does not integrate with `GuiceServletContextListener` in the way `GuiceFilter` [demands](https://github.com/google/guice/blob/e7bef34ef6379735e2a58df1b23f351bb7e30e44/extensions/servlet/src/com/google/inject/servlet/ServletModule.java#L322-L323), which necessitated the passing of `ServletContext` you see in this patch.
> 
> I can't say i'm happy with the outcome here, but i am overall happier given that guice is upgraded.
> 
> Computers are hard!
> 
> 
> Diffs
> -----
> 
>   build.gradle fd606a27bbe0f79a9358ddb74425437ddb42d71e 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 0f8528c3403b5f51f082aa54c16358f7568f439a 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java a19663acb20c66ec14e41473a72fa5035146a6cc 
>   src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java d81671c0bf3634bfce851937f4fc9135dd5f563f 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java a3f6941f6b335791455545e0745b03cf58899f95 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java e8ef6bd7f9b0c47c5f17da7a040ce1497922a807 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java 43fa315a9a25bf29f53d8fd986f198d3f9ef1a74 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java d3c7ac985bbcf734aace2cad50af970619ffb536 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java 20e3d6255921cf24da7cf66101a72c537a0a7c68 
>   src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosPermissiveAuthenticationFilterTest.java 001846765f8f9e8b970ab3b3d2a495652819319c 
> 
> 
> Diff: https://reviews.apache.org/r/64362/diff/2/
> 
> 
> Testing
> -------
> 
> end-to-end tests pass
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 64362: Update to guice 4.1.0, switch from jersey to resteasy

Posted by Bill Farner <wf...@apache.org>.

(Updated Dec. 6, 2017, 6:31 a.m.)

Review request for Aurora, David McLaughlin, Jordan Ly, and Stephan Erb.

Repository: aurora

Description
-------

Fortunately i achieved some early wins with resteasy! Migrating was straightforward with a small patch based on some examples.

I can't say i'm happy with the outcome here, but i am overall happier given that guice is upgraded.

Computers are hard!

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/64362/diff/2/

Changes: https://reviews.apache.org/r/64362/diff/1-2/

Testing
-------

end-to-end tests are broken on master, will fix separately before landing this patch

Thanks,

Bill Farner