aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bill Farner <wfar...@apache.org>
Subject Review Request 64362: Update to guice 4.1.0, switch from jersey to resteasy
Date Wed, 06 Dec 2017 07:36:29 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message