PAYARA-2081 create a new roles allowed cdi annotation #2247
Conversation
|
jenkins test please |
|
Quick build and test passed! |
| } | ||
|
|
||
| public boolean checkRoles(Roles roles) { | ||
| if (roles != null) { |
There was a problem hiding this comment.
Roles can't/shouldn't really be null in an annotation
| */ | ||
| public class RolesCDIExtension implements Extension { | ||
|
|
||
| void beforeBeanDiscovery(@Observes BeforeBeanDiscovery bbd, BeanManager bm) { |
There was a problem hiding this comment.
Try to avoid abbreviations of variable names, unless very common.
| * @default OR | ||
| */ | ||
| @Nonbinding | ||
| String semantics() default "OR"; |
There was a problem hiding this comment.
Could possibly be an enum here instead of a String
| <artifactId>packages</artifactId> | ||
| <version>4.1.2.181-SNAPSHOT</version> | ||
| </parent> | ||
| <artifactId>roles-package</artifactId> |
There was a problem hiding this comment.
Is "roles-package" the clearest name here one can think of?
| @@ -70,6 +70,7 @@ | |||
| <module>environment-warning</module> | |||
| <module>payara-rest-endpoints</module> | |||
| <module>rest-monitoring</module> | |||
| <module>roles-api</module> | |||
There was a problem hiding this comment.
Is it really a "roles api"?
|
|
||
| void beforeBeanDiscovery(@Observes BeforeBeanDiscovery bbd, BeanManager bm) { | ||
| bbd.addInterceptorBinding(Roles.class); | ||
| AnnotatedType<RolesCDIInterceptor> cpat = bm.createAnnotatedType(RolesCDIInterceptor.class); |
| * only if the new code is made subject to such option by the copyright | ||
| * holder. | ||
| */ | ||
| package fish.payara.appserver.roles.api; |
There was a problem hiding this comment.
In a similar vein to Arjan's comments on the module name, I'm not a fan of this package name - this isn't an api, it's an interceptor.
| this.securityContext = CDI.current().select(SecurityContext.class).get(); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
2 more empty lines than necessary
| import org.glassfish.security.common.Role; | ||
|
|
||
| /** | ||
| * |
There was a problem hiding this comment.
Description would be nice :)
michaelranaldo
force-pushed
the
PAYARA-2081-Create-A-New-Roles-Allowed-CDI-Annotation
branch
4 times, most recently
from
9805fa9 to
dd1eeda
Compare
michaelranaldo
dismissed
arjantijms’s stale review
Added request changes and updated copyright to 2018
michaelranaldo
force-pushed
the
PAYARA-2081-Create-A-New-Roles-Allowed-CDI-Annotation
branch
from
dd1eeda to
10b3c4e
Compare
michaelranaldo
force-pushed
the
PAYARA-2081-Create-A-New-Roles-Allowed-CDI-Annotation
branch
from
10b3c4e to
8ee6c4e
Compare
|
jenkins test please |
|
Quick build and test passed! |
No description provided.