ffang commented on PR #3620:
URL: https://github.com/apache/cxf/pull/3620#issuecomment-6369734492
> 1. A broken servlet response could fail the whole request. In
HttpServletResponseHeaders.addHeaders, getHeaderNames() and getHeaders() are
called without any guard. Logging should never break a request, but these calls
can throw:
>
> * some wrapper or mock responses throw UnsupportedOperationException;
> * a response that was already recycled in async or continuation flows can
throw too.
>
> The caller is LoggingCallback.onClose, so an exception there aborts the
response. Wrapping the loop in try { … } catch (RuntimeException e) { /* fall
back to protocol headers */ } is cheap insurance.
>
> 2. Hard-coded transport constants (minor). The new class repeats
"HTTP.RESPONSE", "http.headers.copied" and "org.apache.cxf.http.add-headers" as
strings. The comments explain why: cxf-rt-features-logging depends only on
cxf-core. Two improvements:
>
> * Add a unit test or a @see link so renaming the constants in
AbstractHTTPDestination or Headers can’t silently break this.
> * Alternatively, move the constants to Message or a core class.
>
> 3. Multi-value headers are formatted differently depending on timing
(cosmetic). Without add-headers, copyToResponse joins multiple values with a
comma (setHeader("X", "a,b")).
>
> * After the copy (responses with a body), the log shows a,b.
> * Before the copy (no body), it shows [a, b].
>
> It’s harmless, but it’s inconsistent.
>
> 4. Nit: a class-loading error could still surface later.
isServletApiAvailable catches LinkageError. That’s fine, but
NoClassDefFoundError could still come up later, when HttpServletResponseHeaders
is first loaded. It shouldn’t happen if the class check passed.
> 5. Nit: header masking could be tidier. maskHeaders now streams the
sensitive set once per header. That’s fine for realistic sizes. A lower-cased
Set, or a TreeSet(CASE_INSENSITIVE_ORDER), would be tidier.
Hi @coheigea ,
Thanks for the thorough feedback, revised accordingly by this commit
https://github.com/ffang/cxf/commit/867621cd23486e4c6ae0ebfe9dc71d84992de465
Freeman
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
