Add default set of headers to be white listed for logging#5893
Conversation
| */ | ||
| public HttpLogOptions setAllowedHeaderNames(final Set<String> allowedHeaderNames) { | ||
| this.allowedHeaderNames = allowedHeaderNames; | ||
| this.allowedHeaderNames = allowedHeaderNames == null ? this.allowedHeaderNames : allowedHeaderNames; |
There was a problem hiding this comment.
If they pass in null, just have that clear out the whitelist.
| */ | ||
| public HttpLogOptions setAllowedHeaderNames(final Set<String> allowedHeaderNames) { | ||
| this.allowedHeaderNames = allowedHeaderNames; | ||
| this.allowedHeaderNames = allowedHeaderNames == null ? this.allowedHeaderNames : allowedHeaderNames; |
There was a problem hiding this comment.
Should there be a convenience API to remove headers too? Because in most cases, users just want to redact one or two headers from the default set that may contain sensitive info.
There was a problem hiding this comment.
This can be done via getAllowedHeaderNames().remove(..), so we don't need a convenience API.
| */ | ||
| public HttpLogOptions setAllowedHeaderNames(final Set<String> allowedHeaderNames) { | ||
| this.allowedHeaderNames = allowedHeaderNames; | ||
| this.allowedHeaderNames = allowedHeaderNames == null ? this.allowedHeaderNames : allowedHeaderNames; |
There was a problem hiding this comment.
On line 75 and 114: Should we return an immutable copy of the HashSet instead of returning the reference to the HashSet this class uses? If we return the reference, then user can add/delete to the original set outside of this class.
| public HttpLogOptions() { | ||
| logLevel = HttpLogDetailLevel.NONE; | ||
| allowedHeaderNames = new HashSet<>(); | ||
| allowedHeaderNames = new HashSet<>(Arrays.asList( |
There was a problem hiding this comment.
Extract the Arrays.asList() into a static field and reuse. Don't have to create a list and a hashset each time an instance of HttpLogOptions is created.
There was a problem hiding this comment.
I partially agree. The Arrays.asList(...) call can be a private static final field, but there should be a new set per instance, so that it can be modified via the API.
| if (!allowedHeaderNames.isEmpty()) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (HttpHeader header : requestResponseHeaders) { | ||
| sb.append(header.getName()).append(":"); |
There was a problem hiding this comment.
This should be case insensitive
There was a problem hiding this comment.
fa68466 to
dc90cb6
Compare
3 participants
Reference: #5890