2010/10/13 Andy Schwartz <andy.schwartz@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