oasp / oasp4j

The Open Application Standard Platform for Java
Apache License 2.0
60 stars 303 forks source link

DiagnosticContextFilter.init buggy when correlationIdHttpHeaderName has already been set #458

Closed hohwille closed 8 years ago

hohwille commented 8 years ago

https://github.com/oasp/oasp4j/blob/develop/modules/logging/src/main/java/io/oasp/module/logging/common/impl/DiagnosticContextFilter.java#L129

This code was developed with only Servlet API in mind. With modern spring and spring-boot technology you can easily provide filters as beans from code. Then you may want to call setters in that @Bean method to configure the filter. However, due to the Servlet API lifecycle and spec, spring has to call the init method. As there is no config then our current code overrides the already preconfigured values with defaults. This is IMHO a bug.

maybeec commented 8 years ago

Which preconfigured values do you mean? I am not really getting the problem here. Actually, due to your pull request now correlationIdHttpHeaderName can remain null if there is no value provided by the filterConfig.

jomora commented 8 years ago

@maybeec I wrote a test which verifies that correlationIdHttpHeaderName does not become null under the following circumstances:

Have a look at the test class in my fork

Do you agree with this solution?

maybeec commented 8 years ago

I am fine with this. The only thing I would state as a remark regarding your test implementation is, that you should not catch any exceptions. Just state throws Exception on each test if necessary. This will make analysis easier if the test fails.