NIFI-15629 Add ConnectorRequestContext#10924
NIFI-15629 Add ConnectorRequestContext#10924kevdoran wants to merge 2 commits intoapache:NIFI-15258from
Conversation
- Add ConnectorRequestContext to forward web context details to ConnectorConfigurationProvider implementations
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for putting this together @kevdoran. I made some minor comments regarding the interface method names, the general approach looks good on initial review.
| * @param headerName the header name to look up | ||
| * @return the first header value, or {@code null} if not present | ||
| */ | ||
| String getFirstHeaderValue(String headerName); |
There was a problem hiding this comment.
This is convenient, but with getFirst() on a List, do you think it is needed?
There was a problem hiding this comment.
I don't feel strongly, but given that in theory HTTP headers can be repeated, but in practice applications often expect / want one, I think it is is a useful convenience method to have an accessor method that has "first or null|Optional" semantics. I could use getHeaderValues(String).getFirst() but that throws a NoSuchElementException for an empty list.
I don't feel strongly though, open to idea on naming, signature, or removing altogether.
There was a problem hiding this comment.
Thanks for the reply, on further consideration, this method, returning a nullable String, looks good.
| * | ||
| * @return the authenticated NiFi user, or {@code null} if not available | ||
| */ | ||
| NiFiUser getNiFiUser(); |
There was a problem hiding this comment.
Recommend changing this to getApplicationUser() or getAuthenticatedUser()
| * @param headerName the header name to check | ||
| * @return {@code true} if the header is present, {@code false} otherwise | ||
| */ | ||
| boolean hasHeader(String headerName); |
There was a problem hiding this comment.
Recommend including Request to align with getReaderHeaders()
| boolean hasHeader(String headerName); | |
| boolean hasRequestHeader(String headerName); |
| * @param headerName the header name to look up | ||
| * @return an unmodifiable list of header values, or an empty list if not present | ||
| */ | ||
| List<String> getHeaderValues(String headerName); |
There was a problem hiding this comment.
| List<String> getHeaderValues(String headerName); | |
| List<String> getRequestHeaderValues(String headerName); |
|
Thanks for the review @exceptionfactory! I pushed an update that addressed most/all of the comments, but let's discuss if there is anything more needed on this PR to get it merged. |
3 participants
Summary
NIFI-15629
Add ConnectorRequestContext to forward web context details to ConnectorConfigurationProvider implementations