Log HTTP method in logging filters and revise log message format

[schnapster]

Ahoy,

I was surprised to neither find the http method in the log messages of the AbstractRequestLoggingFilter nor the option to turn them on.

This looks like a fairly uncontroversial, fully optional and backwards compatible change to me so I did not open an issue up front. I'm happy to open one if more discussion is required.

Two things to note for the reviewer:

- [ ] I copy-pasted the method javadocs, and I'm not 100% sure if anything else needs to be done to get

	 * <p>Should be configured using an {@code <init-param>} for parameter name
	 * "includeHttpMethod" in the filter definition in {@code web.xml}.

to work. I did some CTRL-Fing for the other values and nothing interesting came up.

- [ ] I did not add a @since 5.2 annotation as I'm not sure which version this will hit. Just amend it in :)

Cheers,
Napster

Btw, setting up the local build and running tests was a breeze, great docs!

[sbrannen]

After a quick discussion with @rstoyanchev, we have decided that we should simply always include the HTTP method in those logs messages. In other words, there's no need to make it optional.

Can you please rework this PR accordingly?

[sbrannen]

Team Decision:

After some further internal brainstorming, we have decided on the following.

• Always include the HTTP method.
• Change the format of the generated log message to the following, keeping the existing optional behavior in tact for the query string, client info, headers, and payload.

Format: <prefix><method> <uri + query string>, <client>, <session>, <user>, <headers>, <payload><suffix>

Example: Before request [GET /foo?x=2, client=..., session=..., user=..., headers=..., payload=...]

@napstr, please let us know if you are willing to rework this PR along those lines.

Thanks

[schnapster]

Will do, I'll aim to get this done over the weekend :)

Just to clarify, did you also decide to change the existing ;s to , s, @sbrannen or that a typo in the format/example?

[sbrannen]

The Spring Team would also like to point out that the logRecord() method in the DispatcherServlet is capable of logging very detailed information at DEBUG and TRACE levels.

This detailed logging in the DispatcherServlet is available since Spring Framework 5.0 and may serve as an alternative to AbstractRequestLoggingFilter and its subclasses (e.g., CommonsRequestLoggingFilter, ServletContextRequestLoggingFilter, and custom implementations).

Consult the Logging section of the Spring Web MVC documentation for further details.

[sbrannen]

Will do, I'll aim to get this done over the weekend :)

Great!

Just to clarify, did you also decide to change the existing ;s to , s, @sbrannen or that a typo in the format/example?

Yes, we decided to change the delimiter from ; to , (with the comma) in order to make it more human readable and to align with the format in org.springframework.web.servlet.DispatcherServlet.logRequest(HttpServletRequest).

[schnapster]

How does this look?

[sbrannen]

I think the changes look good.

I've requested simplifications to the tests. So please make those and perhaps add an additional test that's more inclusive -- for example, one that contains everything like the previously posted example: Before request [GET /foo?x=2, client=..., session=..., user=..., headers=..., payload=...].

Thanks

[schnapster]

@sbrannen thank you for the hints on how to improve usage of the available AssertJ api. I believe I applied all your suggestions now - let me know if anything is left.

[sbrannen]

@sbrannen thank you for the hints on how to improve usage of the available AssertJ api.

Glad to be of service.

I believe I applied all your suggestions now - let me know if anything is left.

I think that looks good now. I've accepted the changes and will merge the PR in the coming days.

Enjoy your weekend!

[sbrannen]

This has been merged into master in 33cee06, and the tests have been further polished in 52128fe.

Thanks for the PR