Hey Leonardo -

On 10/15/10 12:33 AM, Leonardo Uribe wrote:

2010/10/13 Andy Schwartz <andy.schwartz@oracle.com>

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.

I prefer the second form, since this would allow us to have a single bottleneck through which the VDL is retrieved (ViewHandler.getViewDeclarationLanguage()).

Though I am still a bit confused about the nature of the TCK tests that are failing. Does the TCK require that a view can be restored in the case where no physical resource is associated with the view? If we make the changes that we have been discussing relating to how we check for view existence, would this help the tests pass, or are there still cases where restore view needs to handle viewIds that fail the existence check?

In any case, adding a new ViewHandler method that performs deriveViewId() logic without the final existence check seems reasonable. (Note that in the suffix case, deriveViewId() will still perform existence checks as it tests out various configured file extensions.)

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,

Right.

and we should prevent override ExternalContext.getResource().

Well, maybe not prevent - should still be allowed - but hopefully this won't be necessary anymore.

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.

Cool. I don't think this is a hard requirement, but would simplify life for anyone who wants to tweak view resolving/loading.

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!.

I agree.

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".

Definitely. I mentioned a case where I very much need this in my previous email. My guess is that by default the VDL for JSP will implement the view existence check by calling ExternalContext.getResource() - always been this way. However, in my case, I have the ability to execute JSPs off of the classpath or even JSPs loaded from a remote repository, so I will want to update the view existence check to reflect this. In theory I should be able to do this by providing my own ViewDeclarationLanguageFactory that creates a wrapper around the JSP VDL, allowing me to insert my own existence checking logic.

The problem: I specifically want to wrap the JSP VDL and not any other. However, with our current APIs, my ViewDeclarationLanguageFactory has no way to know which VDL *is* the JSP VDL. A simple way to solve this would be to:

- Introduce the notion of a VDL id, ie. add a getId() (or getViewDeclarationLanguageId()) method to ViewDeclarationLanguage.
- Specify standard ids for the VDLs provided by the specification (eg. "javax.faces.Jsp", "javax.faces.Facelets")

This would allow ViewDeclarationLanguageFactory implementations to target individual VDLs. It would also provide a starting point from which we could build a declarative solution, ie. instead of implementing a ViewDeclarationLanguageFactory to wrap a specific VDL, it would be nice to be able to do something like:

<view-declaration-language>
<view-declaration-language-id>javax.faces.Jsp</view-declaration-language-id>
<view-declaration-language-class>my.bizarre.vdl.WrapperForJspOnly</view-declaration-language-class>
</view-declaration-language>

To achieve the same behavior.

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.

I like the idea of having the existence check on ViewDeclarationLanguage. Two suggestions:

- Let's call this viewExists() instead of existsViewId().
- Let's not expose this on ViewDeclarationLanguageFactory.

The second suggestion would mean that we would replace code like this:

if (vdlFactory.existsViewId(context, viewId)) { ... }

With:

ViewDeclarationLanguage vdl = getViewDeclarationLanguage(viewId)
if (vdl.viewExists(context, viewId)) { ... }

These achieve the same result, but the second approach keeps the ViewDeclarationLangaugeFactory contract/implementation as simple as possible.

Thoughts?

+1 on fix all that stuff before get 2.1 out.

I would very much like to see much of this addressed for 2.1 as well. We've covered quite a bit of ground here. Let's recap what we are looking for.

These seem to be the minimal requirements:

1. The ViewHandler.getViewDeclarationLanguage() should be clear that it expects "derived" view ids.
2. We need some way to safely derive view ids from the restore view phase. This means that we either need to be able to use the existing ViewHandler.deriveViewId() without triggering TCK failures, or we need a new method on ViewHandler that performs the same functionality as deriveViewId() without requiring that the derived view id passes the existence check.
3. We need a method on ViewDeclarationLanguage for testing view existence, eg. viewExists(). We should use this instead of directly calling ExternalContext.getResource() when checking to see whether a viewId corresponds to a physical view.
4. We need some way to wrap a specific VDL (eg. the JSP VDL). The minimal solution would be to introduce a VDL id (eg. ViewDeclarationLanguage.getId()) and define standard ids for the JSP and Facelets VDLs.

In addition, these requirements would be good to address:

5. Generalize the ResourceResolver API so that it can be shared across VDLs. This would involve: creating a new version of this contract that lives outside of javax.faces.view.facelets, allowing resource resolvers to be declared in faces-config.xml, exposing an Application-level API for retrieving the ResourceResolver.
6. Allow #4 to be accomplished declaratively via faces-config.xml.

Did I miss anything? Or, I should say, did I miss anything from this branch of the discussion? I still need to go back and review some of the topics that I ignored from the original discussion. :-)

For me, #1-#4 are the key requirements that I would like to address in 2.1. #5 and #6 would be improvements that I would also welcome, but for my own needs I can get by if we have to defer these until 2.2.

Andy

best regards,

Leonardo Uribe