Hi Jeremy,
First of all, thanks a lot for reviewing and applying my patch. Also,
I really appreciate the feedback on the code.
Please see my comments in line for the problems you had discovered.
On 7/11/06, Jeremy Hughes <[EMAIL PROTECTED]> wrote:
Thanks very much for your recent patch. I have applied it in revision
420800 with some small changes:
I had a problem at OMWSDLReader line 76. This assumes if there is a :
in the input string then it is a URL. However, on windows c:\foo.wsdl
is a file and not a URL. So this logic:
int index = wsdlURI.indexOf(':');
String wsdlURL = (index != -1)
? wsdlURI
: ("file://"+wsdlURI);
yeilds c:\foo.wsdl instead of file://c:\foo.wsdl. The problem showed
up because W3CFileRepository.getFilePath() was returning a file path
instead of a URL - so I fixed that. However the problem remains in
readWSDL() should someone pass in the local string path of a WSDL on a
Windows machine - would be good to fix.
I admit that this is an ugly hack, and I didn't consider windows file
paths when thinking of the above logic! So, this will be definitely
fixed.
Actually, the fact that Woden takes a String argument to the readWSDL
method was my one of my initial points of confusion. I think you may
have noticed that I'm doing this in o.a.w.internal.util.om (line 54)
in order to read a WSDL into OM:
URI uri = new URI(strUri);
URL url = uri.toURL();
InputStream in = url.openStream();
builder = new StAXOMBuilder(in);
I was under the impression that it was a requirement from Woden to
accept only String URIs as arguments. So, I tried the above code to
work around the problem I had. As a result, as you have discovered, a
problem arises whenever non-absolute URIs are given as arguments.
But, as was suggested in this thread, if the readWSDL method is
changed to accept URLs, this file path problem can be completely
eliminated from the OM parser. Also from Axis2 point of view, if we
can directly accept InputStreams it would be even more great :). From
a previous discussion I've learnt it is necessary for the WSDL that's
read into Woden to have a base URI in order to interpret the relative
URLs. But at the moment, Axis2 seems to be setting the base URI to the
parent service archive. So, IMO it's just a matter of the user setting
the base URI in order to resolve relative URIs.
Looking a bit further: line 91 of OMWSDLReader.readWSDL() declares the
url variable and is set at line 93 but never read. I think you copied
this method from DOMWSDLReader() and added the above code for some
reason - perhaps you had a problem with local files? There was a
recent problem with StringUtils.getURL() in WSDL4J which I don't think
is fixed in Woden.
I will remove the code for checking the ":" in the String URI, and
modify the readWSDL signature to take in a URL. I should also probably
check validity of this URL and not try to correct the URL to form a
valid URL as Lawrence has pointed out.
Eclipse tells me there are some other places where local variables are
never read. There's also one in DOMWSDLReader so I'll open a JIRA for
fixing those.
Yes, there are unused variables. I'll fix those.
Just something that might slightly speed things up is the number of
times the EndpointElementTest.wsdl file is read by multiple calls to
the OMEndpointElementTest.setUp() method. I don't think every test
case needs to re-read and test the WSDL - only the
testSetAndGetBindingNameFromOM
testSetAndGetNameFromOM
methods change the parsed WSDL in object form.
Yes again! I think this should be changed in the EndpointElementTest as well.
And a small thing - we have been specifying implicit imports rather
than use * in import statements. This makes it easier to see which
package a class comes from if you're using a dumb editor like vi or
notepad.
Hmm.., I took some effort to make the code as clean as possible. But I
see that 2 import *s, has slipped from my radar in the
AllWodenTestsOM. So, my apologies.. Anyway, as a rule I have been
following the coding conventions John proposed some time back [1]
.IMHO, it would be a good idea to document these in the project site.
I'm willing to help, if required.
Please share if you have any other comments/concerns regarding the OM parser.
Thanks and Regards,
Oshani.
[1] http://mail-archives.apache.org/mod_mbox/ws-woden-dev/200606.mbox/[EMAIL
PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
