Review Board 1.5.4

Warn users in dev mode when they set a RequestVar but don't use it

Updated 2 years, 3 months ago

Derek Chen-Becker Reviewers
138 dchenbecker, dpp, marius, charles, heiko, joni, atsuhiko, jorge, naftoli, kris, indrajit, alexb
None LiftWeb-archive 
While Marius and I were discussing the lazy-load functionality, we realized that it would be nice to warn people when they're setting a RequestVar and then never read it in the same request. I also took the opportunity to refactor some "magic strings" into a constants object.
On my local test app.
David Pollak
Posted 2 years, 3 months ago (October 27th, 2009, 4:42 p.m.)
This is going to generate a lot of spurious messages.  A fair amount of Lift's internals are built on RequestVars that self-initialize based on the current request.
  1. The part that actually generates the log messages filters out things that contain net.liftweb in the var name, which from what I was seeing should cover most, if not all, internals. When I tested this on my local app, this is the extent of what I see:

INFO - Service request (GET) /test.html took 352 Milliseconds
WARN - RequestVar com.liftworkshop.snippet.testRV$_ was set but not read
INFO - Service request (GET) /images/ajax-loader.gif took 7 Milliseconds
INFO - Service request (GET) /ajax_request/liftAjax.js took 60 Milliseconds

This is also only enabled in dev mode.
David Pollak
Posted 2 years, 3 months ago (October 27th, 2009, 9 p.m.)
There are different styles of dealing with accessing RequestVars before they are set.  For example, if it's a logic error to access a RequestVar before it's set, my style would be:

object MyVar extends RequestVar[String](throw new NullPointerException("Tried to access MyVar before setting it"))

I also understand that your approach (logging access before set by default) is reasonable.

So, please add the following to allow for supporting my approach (which is not to log, but to make explicit when it's a logic error to access before setting):

- A LiftRule (structured like allowParallelSnippets) that allows the disabling of the log messages.  You can log by default, but I'd like a way to accommodate my style with a simple one-liner in Boot.
- Allow opting out of the logging on a RequestVar by RequestVar basis.  This allows one to have a particular RequestVar that's okay to access before setting, but allow for the logging of everyone else

  1. The intent is not to log when RequestVars self-initialize with the default value, but rather to handle the case where someone sets a value on a RequestVar manually but then within the same request the value is never retrieved. The use case that Marius and I were thinking of is if someone has a snippet A and snippet B, where A sets some value in a RequestVar that B then uses. If someone decided to change A to be a lazy-load snippet, then that processing actually occurs in a separate request and so the RequestVar really isn't being used to pass data between the snippets anymore. I found a bug in the code that *would* log self-initialized RequestVars, but that's not the intent, so I'm adding a new diff.
Review request changed
Updated 2 years, 3 months ago (October 27th, 2009, 11:04 p.m.)
Fixed a bug where the default initializer caused a log statement to be output.
Ship it!
Marius Danciu
Posted 2 years, 3 months ago (October 28th, 2009, 12:34 a.m.)
I'm ok with it.
Review request changed
Updated 2 years, 3 months ago (October 28th, 2009, 2:09 p.m.)
Added global and per-RV opt-out.
Ship it!
David Pollak
Posted 2 years, 3 months ago (October 28th, 2009, 2:18 p.m.)
Looks good.