[GitHub] [shiro] bmhm commented on issue #58: [SHIRO-337] basic cdi support

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [shiro] bmhm commented on issue #58: [SHIRO-337] basic cdi support

GitBox
bmhm commented on issue #58: [SHIRO-337] basic cdi support
URL: https://github.com/apache/shiro/pull/58#issuecomment-577317147
 
 
   Hi,
   
   I'm not a ASF committer, but from looking at this code, I have some questions.
   
   `SubjectProducer` is `@ApplicationScoped` and there is commented `@RequestScoped`. Why not just use `@Dependent`?
   
   Line 40 of this class is `Subject.class.cast(Proxy.newProxyInstance(` and could be changed to `(Subject) Proxy.newProxyInstance(`…
   
   This part needs some additional comments why it uses a new proxy instance. The comment should mention Shiro's ThreadContext.
   
   Why does it need a Classloader utility `Loader`?
   `ShiroExension::addSecurityManagerIfNeeded` could be updated to use a `AtomicReference::updateAndGet`
   
   The geronimo specs are still in there, please update to the jakarta dependencies. Geronimo's dependencies do not even have javadoc available.
   
   `beans.xml` could be updated to cdi2. I'd also rename the module to CDI2, just in case CDI 3 is not compatible.
   
   Also, you should not leave out `beans.xml` although it is allowed. It is still good habit to ship an empty one, just because some containers allow a setting to scan only jar files for beans which do have such a file (which speeds up deployment times massively).
   
   Also, some comments might be helpful which action in the extension is done for which purpose.
   
   I haven't used extensions a lot, but I think this code should still be elegible for inclusion.
   If merged, I'd also like to contribute another integration test using the maven invoker plugin, starting an OpenLiberty instance.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services