[jira] [Commented] (WICKET-4074) RequestLogger needs a clear separation of concerns

["Martin Grigorov (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/WICKET-4074?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13109438#comment-13109438 ] 

Martin Grigorov commented on WICKET-4074:
-----------------------------------------

My review:

- move **LogData in a its own package (org.apache.wicket.request.handler.logger ?!)
- is there a need to pass the type to the ILoggableRequestHandler ? I think it can return ILogData and different impls can use covariant return type (e.g. return PageLogData)
- code from BookmarkablePageRequestHandler:
	if (logData == null)
+			logData = new PageLogData(pageProvider);
+	}
+
+	/** {@inheritDoc} */
+	public PageLogData getLogData()
+	{
+		return logData;

That looks like the logData will use the PageProvider later at logging time. Correct ?
This means that the logger can re-attach a page (provider.getXYZ()) later (after request cycle detach).
Update: I see that PageProvider is used only in the constructor to extract the needed data but maybe it will be safer to pass the needed data itself and prevent wrong usage of PageProvider in the future (like ResourceLogData does).

- make all members in ***LogData final
- chain the constructors where possible
- add some tests ?!
- should ILogData be Serializable ? Is there a chance to serialize it somewhere ?


> RequestLogger needs a clear separation of concerns
> --------------------------------------------------
>
>                 Key: WICKET-4074
>                 URL: https://issues.apache.org/jira/browse/WICKET-4074
>             Project: Wicket
>          Issue Type: Improvement
>          Components: wicket
>    Affects Versions: 1.5.1
>            Reporter: Emond Papegaaij
>         Attachments: 0001-improved-request-logger.patch
>
>
> As a follow-up on WICKET-3919 and WICKET-4029, I've been working on a patch for the RequestLogger. This patch makes RequestHandles responsible for providing data to log. The RequestLogger collects this data and logs it. RequestHandles that are to be logged, must implement ILoggableRequestHandle.
> In short, this patch changes the following things:
> ILoggableRequestHandle added and all major RequestHandles changed to implement this interface
> IStatedRequestLogger added and AbstractRequestLogger changed to implement this interface (separate logging step after the detach phase)
> Various implementations of ILogData added (data logged by request handlers)
> RequestLogger simplified to use ILoggableRequestHandle

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira