Hi Andy
2010/10/13 Andy Schwartz <andy.schwartz(a)oracle.com>
Here I have to mention something
Oh, interesting. This does seem extremely important. We've got two types
of viewIds:
- Raw: deriveViewId() logic has not yet been applied
- Derived: deriveViewId() has been applied
ViewHandler.getViewDeclarationLanguage() needs to be specific about what
type of viewId is expected. Based on the above code snippets, it looks like
Mojarra's implementation expects raw viewIds. MyFaces expects derived
viewIds. However, as you said, for the most part, Mojarra is passing
derived viewIds into ViewHandler.getViewDeclarationLanguage(). This means
that Mojarra is repeatedly re-deriving viewIds on each
ViewHandler.getViewDeclarationLanguage() call. Since there can be many
calls to this method per-request, and since checking for the physical
existence of a view isn't always cheap, this seems bad.
Perhaps the spec needs to be updated to make it clear that
ViewHandler.getViewDeclarationLanguage() operates on derived viewIds?
In my opinion, yes.
In this case, MyFaces "derive" the physical viewId before call
> getViewDeclarationLanguage
> from RestoreViewPhase. In theory, we should call from RestoreViewPhase
> algorithm a method
> exposed by ViewHandler.
>
Right: ViewHandler.deriveViewId().
Right now, ViewHandler.deriveViewId() is used by NavigationHandler and in
that use case it is necessary the viewId is being checked for existence, or
in other words, if the physical resource tied to the viewId exists or not
(for preemptive navigation). Here we have two options:
- Add a method like ViewHandler.getViewDeclarationLanguageFromRawViewId
- Add another method that do the same as deriveViewId() but do not check for
viewId existence, to allow call it from Restore View Phase and do not break
existing TCK tests.
Getting back to the question of how to check whether a particular view
exists... I agree that at a minimum in the Facelets case we need to be
consistent about always including the ResourceResolver in this decision.
The new existence check method on ViewDeclarationLanguageFactory() that you
proposed would definitely help. Actually, something like this would be
tremendously helpful for us here (ADF Faces), since at the moment we have
logic scattered across ResourceResolver + ExternalContext. This split
approach is particularly awkward since ExternalContext.getResource() is used
more generically than ResourceResolver - ie. ExternalContext.getResource()
may be called for arbitrary resource retrieval, whereas we are particularly
interested in view resolving.
In theory just override or wrap ResourceResolver should be enough, and we
should prevent override ExternalContext.getResource().
I wanted to mention an alternate to the
ViewDeclarationLanguageFactory
approach, though I am not sure whether it is an improvement. In our case,
we wouldn't mind using the ResourceResolver generically across VDLs, ie. we
wouldn't mind seeing ResourceResolver promoted to a first class/VDL-agnostic
API. Our ResourceResolver implementation is used to serve up resources from
the class path as well as from remote repositories (eg. a database). This
ability could be useful not just for Facelets, but for arbitrary VDLs. It
would be nice if we could implement this behavior one time in a
standard/portable way rather than requiring a new solution for each VDL.
If we had access to an application-level (VDL-independent)
ResourceResolver, the JSF implementations could use this to check for the
existence of views when deriving view ids (and elsewhere) without having to
go through the ViewDeclarationLanguageFactory.
I agree with this one too. In theory, the VDL is responsible to build views,
so it should use some strategy to retrieve them (it is the "client"). Use a
strategy per VDL seems theorically reasonable but in practice, since each
VDL only handle files mutually exclusive with other VDL's, use a shared
ResourceResolver per all VDL is ok in my opinion.
I don't think it would be especially hard to make the
ResourceResolver more
general-purpose. We would likely want to add a new version of this class
outside of the Facelets packaging and possibly re-parent the existing
Facelets ResourceResolver to the new API. Or, at least, we would want to
provide the ability to transparently adopt the old Facelets ResourceResolver
to the new generic ResourceResolver.
In my opinion, it should be an application scope object and developers
should be able to add wrappers through faces-config.xml inside <application>
tag. It would be great!.
Anyway, there is still open the question about if the spec should allow
create wrapper per VDL from faces-config.xml. There are some use cases when
this could be helpful, like allow extend facelets an add custom "attached
objects".
Another small change that might be helpful... in order to answer the
question of whether or not a resource exists, we currently need to literally
cough up an URL. In our case, we might be able to optimize if we just had
to test for existence as opposed to return a reference to the resource.
Perhaps we could consider adding exists() method to the generic
ResourceResolver. By default this would just do "resolveUrl() != null", but
this could be optimized in other implementations.
Yes it could be, but anyway we need the method existsViewId on
ViewDeclarationLanguage (and that one will call the hypothetical method
ResourceResolver.exists() ). Why? because the VDL should be responsible to
indicate if the viewId is valid before check for existence. In the patch
proposed this check is done before call ResourceResolver stuff.
Of course, if we generalize the ResourceResolver in this way and call
this
from ViewHandler.deriveViewId(), this does introduce a new assumption: that
ViewDeclarationLanguages will also use the ResourceResolver for loading
views (like Facelets does). One case where this isn't necessarily true is
the JSP ViewDeclarationLanguage, since the JSP engine itself is responsible
for loading the view. FWIW, in our particular case, our JSP engine actually
does contain resource resolving logic that matches our ResourceResolver
implementation. As such, we would prefer to use the ResourceResolver to
check for view existence even in the JSP case, though I imagine our
requirements are a bit unusual in this area.
Thoughts?
+1 on fix all that stuff before get 2.1 out.
best regards,
Leonardo Uribe