make sure to unlock xml resource #1266
Conversation
| } | ||
|
|
||
| private void notValidXar(String path) throws XPathException { | ||
| throw new XPathException(this, EXPathErrorCode.EXPDY001, path + " is not a valid .xar", new StringValue(path)); |
There was a problem hiding this comment.
I'd suggest to have two simple throw exceptions on line 160 and 164 ; with this you could probably change the text into "Resource not found" and "Resource is not binary" .... which helps in finding a root cause.
| DocumentImpl doc = context.getBroker().getXMLResource(uri, LockMode.READ_LOCK); | ||
| if (doc == null) { | ||
| throw new XPathException(this, EXPathErrorCode.EXPDY001, | ||
| path + " is no .xar resource", |
There was a problem hiding this comment.
"is no .xar resource" -> " does not exist"
There was a problem hiding this comment.
disagree, because your message unclear that problem around .xar
There was a problem hiding this comment.
If you insist, can you fix the language so that it reads "is not a .xar resource"?
| } else if (doc.getResourceType() != DocumentImpl.BINARY_FILE) { | ||
| doc.getUpdateLock().release(LockMode.READ_LOCK); | ||
| throw new XPathException(this, EXPathErrorCode.EXPDY001, | ||
| path + " is not a valid .xar, it's not a binary resource", |
There was a problem hiding this comment.
" is not a valid .xar, it's not a binary resource" -> " is not a valid .xar EXPath Package"
There was a problem hiding this comment.
the test is actually only if it is a binary resource.....
There was a problem hiding this comment.
so, message must include information that it fail because resource is not binary
There was a problem hiding this comment.
The spec for EXPath packages says that they must be valid Zip files. Therefore a valid Xar can only be a binary file. As such I think that means the error can be simplified and improved here
There was a problem hiding this comment.
The xar file can be invalid for many reasons: corrupted zip, missing file at zip, wrong folders structure. This error define clearly that eXist do not see that resource as binary one.
|
It's unclear for me how to write test for it. @adamretter, please do, if you know how to. |
|
I have no idea either |
|
@shabanovd @dizzzz A test just needs to call |
|
speaking for myself, I'd still have no idea :-( It looks like "InstallFunction" has quite some dependancies with other initialised classes, probably thats is why you write Mock? |
|
@adamretter @wolfgangmm Any outstanding concerns here? |
|
@joewiz Yes, it was awaiting time for me to add a test as @shabanovd didn't want to do it. However, I looked into adding a test, and there is no suitable place in the code-base to put it. Extension XQuery Modules for eXist do not currently have any JUnit tests :-(. As this would require changes to the build system, I think I will park the concern of adding a test until after the Maven'ization of eXist; Maven explicitly has a place for tests for every source code root. |
if url is xml resource then it get read-locked
(similar to #1264 )