#30 Duplicate the jersey module to "jersey" for 2.26 and "jersey-225" for 2.25 #40
Conversation
|
|
||
| public static class Binder extends AbstractBinder { | ||
|
|
||
| private final ProfileFactoryBuilder profile; | ||
| private final OptionalProfileFactoryBuilder optProfile; | ||
| private final ProfileManagerFactoryBuilder manager; | ||
| private ProfileManagerFactoryBuilder manager; |
There was a problem hiding this comment.
this should still be final I think
There was a problem hiding this comment.
Yes it should. I'll fix this.
| @Context | ||
| Providers providers; | ||
| private Providers providers; |
There was a problem hiding this comment.
this could be final now that there is a constructor to initialise it
There was a problem hiding this comment.
Yes it could, and it will :)
| } | ||
| } | ||
|
|
||
| static class ProfileValueFactory extends AbstractContainerRequestValueFactory<CommonProfile> | ||
| implements ProfileFactory { | ||
| static class OptionalProfileValueFactory implements OptionalProfileFactory { |
There was a problem hiding this comment.
any reason why you don't rely on injection here to get the request instead of making it explicit in the interface?
I liked the idea of having an interface agnostic to the where the request was coming from… but maybe it's not possible anymore?
There was a problem hiding this comment.
Well, the main reason is that I could not find a way to make it work. A secondary reason is that the origin of the request as a parameter comes directly from the AbstractValueParamProvider.
To me it felt like the way to go, but I'm no expert.
There was a problem hiding this comment.
indeed it seems a good idea to reuse the request instead of having it injected.
btw I see you corrected the few things I noted in the other PR but not in this one
| static class OptionalProfileValueFactory extends AbstractContainerRequestValueFactory<Optional<CommonProfile>> | ||
| implements OptionalProfileFactory { | ||
| private static Optional<CommonProfile> optionalProfile(ContainerRequest containerRequest) { | ||
| RequestPac4JSecurityContext securityContext = new RequestPac4JSecurityContext(containerRequest.getSecurityContext()); |
There was a problem hiding this comment.
I think you should be able to write: new RequestPac4JSecurityContext(containerRequest)
| this.profile = profile == null ? ProfileValueFactory::new : profile; | ||
| this.optProfile = optProfile == null ? OptionalProfileValueFactory::new : optProfile; | ||
| this.manager = manager == null ? ProfileManagerValueFactory::new : manager; | ||
| this.manager = manager; |
There was a problem hiding this comment.
why don't you do the same as with profile and optProfile and initialize manager here?
There was a problem hiding this comment.
Because if the manager is not specified I wanted it to be injected. This would avoid the need for a Pac4J user to inject everything in the Feature and then having to pass it all to the Binder constructor.
It doesn't feel ideal to me because now the manager is special compared to the profile and optProfile, but I thought of it as of a good compromise.
There was a problem hiding this comment.
The user shouldn't be using the Binder with parameters but only the binder without parameters.
And anyway, in any case, the constructor didn't change so I'm not sure what you mean ^^
Let's keep it like this, if it works it's ok, we will have other occasions to clean that, I'm pretty sure there is a lot that can be done for this class...
|
thanks @pierre-l and sorry for the delay :) |
See #30, #36.