Two problems: regexps are insanely slow. When doing URL rewrites on
something this link-intensive, you want to start caching the HTML.
Which in turn will play havoc with the plugin code generation.
Currently one of the biggest timeusers (if not the biggest) is the
DefaultURLConstructor.
Regexp's are slow, no question. But remember that you probably won't
need them unless you are actually transforming URLs into something
that is NOT the default case. The defaults are (and I suspect will
continue to be) URLs that look like this:
/[webapp]/Wiki.jsp?page=Foo&version=2
The key is making the default scheme nice and fast. If you wanted to
use [a different URL constructor|a filter with regexps], you could do
that separately and pay the requisite penalty.
The other problem is that I think some of our template/skin
generation stuff might break if the context is lost...
By "context" I assume you mean "wiki context", as in WikiContext.EDIT?
As much as I'd like to do away with those, and move towards strong
typing (prefer UserPreferencesActionBean.class to WikiContext.PREFS),
the legacy contexts will be with us for a while.
At present, I am working on making my earlier 3.0/Stripes integration
as backwards-compatible as possible. Example: I have a custom
annotation called @WikiRequestContext that is class-level, and permits
class authors to annotate the wiki request context string ("edit",
"prefs") etc it has. It should work quite nicely -- all we do is
slightly retrofit WikiContext so that WikiContext.PREFS gets its value
from an annotation lookup rather than a text string
(UserPreferencesActionBean
.getAnnotation(WikiRequestContext.class).value()). It pushes
responsibility for declaring stuff like this to the class itself. It
also makes it easier for authors who want to write their own
ActionBeans (no more hacking the Command classes...).
However, Java5 varargs I think give us a nice exit. How about
WikiContext.generateURL( String content, String param1, String
value1, ... )
with
PARAM_PAGE = "page"
PARAM_VERSION = "version"
and the JSP param could be "Wiki.jsp", or "rss.jsp" or "attach" or
"scripts/jspwiki.js" - direct servlet/JSP/resource names. E.g.
...
wikiContext.generateURL( "Wiki.jsp", PARAM_PAGE, newPage,
PARAM_VERSION, 1, "skin", "raw" );
...
We could then have a SimpleURLConstructor (which just attaches the
params one after the other), a RegexpURLConstructor (which extends
the SimpleURLConstructor and just passes them through a regexp
list), and so on. I like the fact that code can be optimized in
these cases quite a lot, but rules cannot.
Indeed. You might want to take a look at the Stripes URLBuilder class.
You instantiate it with a locale first, and either a path or an
ActionBean class:
builder = new URLBuilder(locale, "/Wiki.jsp" );
or
builder = new URLBuilder(locale, ViewActionBean.class );
Then, you add parameters either one-by-one or as an array:
builder.addParameter( "page", "Foo" );
Note that the second argument is a vararg, because parameters can be
multi-valued.
After that, the method build() returns the correctly formatted and
encoded String.
Now, I am not trying to persuade you to use the Stripes URLBuilder.
However, I mention it in detail to show you that it shares one
property with what you outlined. Namely, the principle of passing
parameters as separate arguments rather than the way we do now, as a
monolithic blob. This is a nice departure from what we have done, but
it's going to require us to blow up the URLConstructor class (as we
know it) and substitute something else. Either SimpleURLConstructor or
the Stripes alternative. I happen to like the Stripes URLBuilder
because it has taken localization and multi-valued properties into
account, and does not rely on "magic parameters" where authors would
have to remember to supply an even number of varargs (the first arg is
the param, the second is the value). Anyway, you might want to take a
look at the source code for URLBuilder. :)
One thing I think we should look carefully at is whether we should
keep the getURL() functions onto the WikiContext (aka the ActionBean)
or move it elsewhere. My feeling is that we should deprecate
WikiContext-level getURL() type methods and move them to the
ActionBeanContext, which is associated with each ActionBean. That's
where all of the HTTP-related stuff is, and I would rather keep the
ActionBeans focused on functionality (bean properties and events)
rather than the HTTP-related aspects of how they are presented.
WikiContext.getURL() will be around for a while (for backwards
compatibility reasons) but it should delegate to
WikiActionBeanContext.getURL(). Same for WikiContext.getHttpRequest(),
which should delegate to ActionBean.getRequest().
(In case you were wondering: the WikiActionBeanFactory, and the
StripesFilter, both guarantee that every ActionBean [WikiContext] has
an associated ActionBeanContext...)
I don't think we really need to carry the notion of the request
context that much further, as it's far clearer to point directly to
the JSP or resource instead of an abstract name. Unfortunately it
means that things like the ContentTag will break. But then again,
the stripes-based UI is probably going to break it anyway.
I agree, mostly. The act of moving functions that are grouped together
into ActionBeans means that we are better off simply specifying the
bean itself, or a class reference to it, instead of the String (the
request context). Using strong typing (the class) rather than strings
means we can do other things too. For example, the ActionBean could
have an annotation that names the content template JSP. Or (my
preference) it could be calculated as a default value, and you use
page markup or annotations for those cases where the defaults don't
work.
Example: in my local builds each ActionBean has an annotation called
@URLBinding that says how Stripes should map the 'bean to URLs. For
example ViewActionBean's URLBinding is "/Wiki.action". The top-level
JSP is easily calculated by chopping off .action and attaching ".jsp".
Thus: Wiki.jsp. The content page is *generally* the top-level JSP name
plus "Content". So -- that's the rule. Look up the URLBinding, chop
off .action, and attach the suffix. "/WikiContent.jsp" would be the
default by convention. (Except that Wiki.jsp's content page is
PageContent.jsp... so for ContentTag you tell it to use
PageContent.jsp.)
Pulling back from the detail, though, it is clear that certain things
are going to break. But maybe less than we thought. My first attempt
at Stripes integration made no attempt to be backwards compatible, and
indeed, I made it deliberately incompatible with those features I did
not like (e.g. URLConstructors, Commands). That was clearly too
ambitious. My approach now is to keep as much compatibility as
possible but aggressively deprecate methods or fields where the
alternatives are better. WikiContext request contexts are one example.
In my copious free time, I have been taking another whack at 3.0. The
primary incompatibilities are the removal of the WikiContext
contructor (use a factory instead) and the related setRequestContext()
method. In fact, if it is not too late, I would not mind removing the
WikiContext constructor NOW, in 2.8, and forcing the use of
WikiEngine.createContext(). That's only a few cases (15-20?), and
there is little/no exposure in the JSPs.
The other major difficulty is the historical "overloading" of
WikiContext to include non-page-related activities like groups and
user maintenance. This is a little harder to tackle, but it should
only affect about 30-40 places in the code.
Again, in the short term the goal is backwards compatibility, even if
we carry forward some things that we will deprecate. My previous
attempt modified about 300 classes; I think I can get that down to
about 50. JSPs are the wildcard here, but here too there are some
steps we can take to (hopefully) limit the changes to about 20 JSPs or
so, and keep them relatively minor also.
Janne, sorry for the ramble... I have had lots of "plane time" lately,
and have been trying to get my 3.0 builds re-synced with the 2.8
branch. Seemed like a good time to bring you up to date.
Andrew
