Bug 342793 - Xsl include/import reference validation doesn't signal error on Unresolved reference
Summary: Xsl include/import reference validation doesn't signal error on Unresolved re...
Status: RESOLVED FIXED
Alias: None
Product: WTP Source Editing
Classification: WebTools
Component: wst.xsl (show other bugs)
Version: 3.3.1   Edit
Hardware: PC Windows 7
: P3 major with 1 vote (vote)
Target Milestone: 3.5 M7   Edit
Assignee: Jesper Moller CLA
QA Contact: Jesper Moller CLA
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2011-04-14 03:10 EDT by Matteo TURRA CLA
Modified: 2013-04-10 20:34 EDT (History)
3 users (show)

See Also:


Attachments
TODO added (746 bytes, patch)
2012-02-28 10:45 EST, Matteo TURRA CLA
no flags Details | Diff
XSLValidator Patch (2.02 KB, patch)
2012-02-29 04:54 EST, Matteo TURRA CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matteo TURRA CLA 2011-04-14 03:10:37 EDT
Build Identifier: I20110310-1119

I wrote an xsl file with an import of an not existing file:
 <xsl:import href="thisFileNotExists.xsl"/>
Eclispe doesn't report the error even if I specify in the preference "Error" in the validation tab where "Unresolved include/import"

Reproducible: Always

Steps to Reproduce:
1. Create and empty java or dynamic 
2. Create a new xsl file 
3. Type <xsl:import href="thisFileNotExists.xsl"/> on the third row (before <template match="/">
4. Check validation preference to report error on unresolved include/import 
5. Run validation
Comment 1 David Carver CLA 2012-02-27 16:21:44 EST
Matteo, would you be willing to provide a possible patch and corresponding unit test for this?
Comment 2 Matteo TURRA CLA 2012-02-28 10:45:41 EST
Created attachment 211738 [details]
TODO added
Comment 3 Matteo TURRA CLA 2012-02-28 10:47:25 EST
Comment on attachment 211738 [details]
TODO added

I found where the error is supposed to be managed. But I don't know how to mark the error to let eclipse editor show it.
Comment 4 David Carver CLA 2012-02-28 16:21:53 EST
(In reply to comment #3)
> Comment on attachment 211738 [details]
> TODO added
> 
> I found where the error is supposed to be managed. But I don't know how to mark
> the error to let eclipse editor show it.

It looks like we need to be able to check if the HREF exists (i.e. can it be resolved). If the model doesn't already have it it problem needs a method like, importExists() which would return true or false if the import exists or not.

Notice that the model also assumes that we will always be dealing with files, which isn't necessarily true.  Most of the time we will be but in practical application we won't (i.e. href is actually a URL so we could be dealing with HTTP urls, or custom URL resolvers).

To allow the eclipse editor to show it as a Validation error, we would need enhance the XSLValidation implementation classes so that it looks for the file. It would need to check to see if the import exists, if not, flag it as an error as appropriate.

It's been a while since I looked at the code.  If I get a chance this weekend I can take a further look.  But definitely check the XSLValidation code for where to add the error marker indicators.
Comment 5 Matteo TURRA CLA 2012-02-29 04:42:27 EST
Thank you David.

I know the href could be a local file on file system or an external url, but the URIResolver can deal with this both. The StylesheetModel assume to deal with file, it will be better to use URI instead, letting the ExtensibleURIResolver to deal for it resolution. I understood this URIResolver can be extended with custom URIResolver, but I don't know how!

I think using XMLCatalog is the best method to map external resources referenced by href. Even if the org.eclipse.wst.xml.core.internal.catalog.XMLCatalogURIResolverExtension is used in xslt include/import resolution, in the XMLCatalog ui (workspace preference) xsl files are not considered as a valid entry. But this is another issue.
Comment 6 Matteo TURRA CLA 2012-02-29 04:54:51 EST
Created attachment 211784 [details]
XSLValidator Patch

I added a importOrIncludeExists method to check resolved uri starting with file prefix. 
It use XSLCore.resolveFile method to check if file exists, but this method call URIResolverPlugin.createResolver().resolve() inside, so this method is called twice.
Comment 7 Nitin Dahyabhai CLA 2012-02-29 13:22:36 EST
In light of bug 371019, is this wise?  False positives aren't nice.
Comment 8 David Carver CLA 2012-02-29 14:56:24 EST
(In reply to comment #6)
> Created attachment 211784 [details]
> XSLValidator Patch
> 
> I added a importOrIncludeExists method to check resolved uri starting with file
> prefix. 
> It use XSLCore.resolveFile method to check if file exists, but this method call
> URIResolverPlugin.createResolver().resolve() inside, so this method is called
> twice.

As part of bug 258937 I had added XML catalog support.  To try to resolve files that weren't found in local file system but could be in the XML Catalog.

Revision 1.35 changed this to use the URIResolver so that custom resolvers could be used to locate the files.  The model still needs to be updated, but it would correctly find the files if a resolver were implemented.

So the situation we need to set an error marker on is this:

1. File does not exist on the file system.
2. File can not be resolved in the XML Catalog
3. File can not be resolved via a URIResolver.

We need this for both Includes and Imports, and right now there is duplicate code that could be consolidated.   

For ifiles we assume that is in the eclipse workspace.  If it isn't the included.file should return a false for the exists.  Only then should the resolving have to occur.  I'm not sure the importOrIncludeExists is necessary, here, if it can't resolve the file via the URI Resolver it should be null.

IFile includedFile = include.getHrefAsFile();

Is used to get the included file and return an IFile.  Which is basically what:

IFile file = XSLCore.resolveFile(currentStylesheet, resolvedURI);

Does.  I'd be curious to see what the include.getHrefAsFile() method is using to try to resolve the file, and why it isn't finding it if it exists.
Comment 10 Jesper Moller CLA 2013-04-10 20:33:55 EDT
Fixed in master, tagget and released.

Thank you for the patch, Matteo!
I'm sorry the patch waited for so long.