CAMEL-14780 camel-undertow: Enhance SPI API for HttpHandler change #3677
Conversation
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
LGTM, I saw the PR template was completely ignored.. that's what I was afraid of, I think we can safely remove it @davsclaus
| @Metadata(label = "security") | ||
| private String allowedRoles; | ||
|
|
||
| private UndertowSecurityProvider securityProvider; |
There was a problem hiding this comment.
@davsclaus this is not parameter which comes from configuration, it is just private variable that is used by component when SPI implementation is searched for. From my POV there should be annotation like @nometadata in tis case.
| this.allowedRoles = allowedRoles; | ||
| } | ||
|
|
||
| public UndertowSecurityProvider getSecurityProvider() { |
There was a problem hiding this comment.
Add setter so you can set this explicit
There was a problem hiding this comment.
I can do it also this way, in that case please ignore my previous comment
| protected void doStart() throws Exception { | ||
| super.doStart(); | ||
|
|
||
| initSecurityProvider(); |
There was a problem hiding this comment.
Only init if the security provider is null
There was a problem hiding this comment.
I'll change it
|
|
||
| public UndertowSecurityProvider getSecurityProvider() { | ||
| return securityProvider; | ||
| if (this.securityProvider == null) { |
There was a problem hiding this comment.
getter/setters should be plain, not these tricks. Instead when creating the endpoint in the component, then set the security provider on the endpoint correctly
There was a problem hiding this comment.
ok, I'll do it, thanks for explanation
|
|
||
|
|
||
| public String getAllowedRoles() { | ||
| return allowedRoles == null ? getComponent().getAllowedRoles() : allowedRoles; |
There was a problem hiding this comment.
See prev comment, getter/setter should be plain
There was a problem hiding this comment.
I'll change it
|
@oscerd I've seen the template, I'll read it, validated all steps. Should it be used in any other way? |
|
There was a contribution about that, but I'm not really sure is useful, too much text.. People may lose attention |
… method to change HttpHandler
JiriOndrusek
force-pushed
the
CAMEL-14780_SPI_API_httpHandler
branch
from
54a3cbe to
a77ee78
Compare
|
@davsclaus All requested changes added |
4 participants
Issue: https://issues.apache.org/jira/browse/CAMEL-14780
Change adds method co change httpHandler into SPI API.
Extension of API is back compatible with previous version of API.
Added JUnit test validating htppHandler 'hook' and test for usage of security parameters defined on component.
[ ] Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
[ ] Each commit in the pull request should have a meaningful subject line and body.
[ ] If you're unsure, you can format the pull request title like
[CAMEL-XXX] Fixes bug in camel-file component, where you replaceCAMEL-XXXwith the appropriate JIRA issue.[ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
[ ] Run
mvn clean install -Psourcecheckin your module with source check enabled to make sure basic checks pass and there are no checkstyle violations. A more thorough check will be performed on your pull request automatically.Below are the contribution guidelines:
https://github.com/apache/camel/blob/master/CONTRIBUTING.md