stevedlawrence commented on code in PR #1451:
URL: https://github.com/apache/daffodil/pull/1451#discussion_r1983338385
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -216,6 +218,18 @@ object Misc {
}
}
+ def optResourceURI(contextURI: URI, relPath: String): Option[URI] = {
Review Comment:
Suggest we call this `optRelativeResourceURI` to match the pattern of
`optRelativeJarFileURI`, which looks like it does a similar thing but for jars
contexts?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/exceptions/SchemaFileLocatable.scala:
##########
@@ -125,7 +125,12 @@ class XercesSchemaFileLocation(
) extends SchemaFileLocation(
(if (xercesError.getLineNumber > 0)
Some(xercesError.getLineNumber.toString) else None),
(if (xercesError.getColumnNumber > 0)
Some(xercesError.getColumnNumber.toString) else None),
- schemaFileLocation.diagnosticFile,
+ (if (
+ xercesError.getSystemId != null &&
+ // only set to systemId if it's different from the diagnosticFile path
+
!xercesError.getSystemId.endsWith(schemaFileLocation.diagnosticFile.getPath)
+ ) new File(xercesError.getSystemId)
+ else schemaFileLocation.diagnosticFile),
Review Comment:
So is this saying that there are cases were the file where Xerces finds an
error isn't necessarily the same as the schema file location that daffodil
provider to Xerces? Like the error is in an imported file or something?
Should we just always use the systemId from Xerces if it's not null? Or are
there cases where it might be defined but diagnosticFile is the right thing to
use?
Also, some windows tests are failing that look like they might be related to
this, causing us to loose depersonlized paths in diagnostics?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -152,6 +152,8 @@ object Misc {
if (Paths.get(relURI).toFile.exists())
Some(relURI)
else None
+ } else if (contextURI.getScheme == "resource") {
Review Comment:
It might be worth doing a little refactoring to this
`getResourceRelativeOption` function to make it more clear, and more correct.
For example, the `contextURI.isOpaque` condition probably wasn't the right
check, since the logic of that branch expects the context to be a "jar" URI.
Any other opaque URI won't work and will probably lead to an weird error. I
think this only works because the only opqaue URI we support is "jars", so
isOpaque kindof implies it's a jar URI, but in theory we could add support for
other opque URI's that aren't jar and this would break.
Since this fuction always expcts contextURI to have a scheme, what if we
move the logic of the "file" scheme to a new `optRelativeFileURI`, and then
this function just becomes:
```scala
contextURI.getSchema match {
case "jar" => optRelativeJarFileURI(...)
case "file" => optRelativeFileURI(...)
case "resource" => optRelativeResourceURI(...)
case _ => throw new IllegalArgumentException(...)
}
```
I think that makes this function more clear and leaves the functions it
calls to figure out to correctly handle resolving the relative path.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -216,6 +218,18 @@ object Misc {
}
}
+ def optResourceURI(contextURI: URI, relPath: String): Option[URI] = {
+ val relURI = contextURI.resolve(relPath)
+ try {
+ relURI.toURL.openStream().close()
+ this.getClass.getResource(relURI.getPath)
Review Comment:
It looks like these two lines are doing the same thing--checking to see if
the resolved URI actually exists. So I'm not sure we need both. I imagine
calling getResource is more efficient since it doesn't need to actually open
the file as a stream and deal with exceptions? So can we remove the try/catch
and just do something like this:
```scala
if (this.getClass.getResource(relURI.getPath)) Some(relURI) else None
```
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -737,6 +751,10 @@ object Misc {
Paths.get(pathPart).toFile
}
case "file" => Paths.get(uri).toFile
+ case "resource" => {
+ val resourceFilePart = uri.getPath
+ new File(resourceFilePart)
Review Comment:
The rest of this function uses `Paths.get(foo).toFile`. I'm not sure why we
did that, maybe Paths.get is doing something additinal? Should do that here for
consistency?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/DaffodilXMLLoader.scala:
##########
@@ -228,19 +228,19 @@ class DFDLCatalogResolver private ()
* <xs:schema...
* ... xsi:schemaLocation="urn:some:namespace /some/path.xsd"
* }}}
- * Xerces takes that schemaLocation URI and absolutizes it to {{{
file:/some/path.xsd }}}
+ * Xerces takes that schemaLocation URI and absolutizes it to {{{
URISCHEME:/some/path.xsd }}}
* and passes that to our resolveEntity and in turn resolveCommon, which
while it's able
- * to find the namespace, fails to set the resolvedUri since the
file:/some/path.xsd will
+ * to find the namespace, fails to set the resolvedUri since the
URISCHEME:/some/path.xsd will
* never match anything resolved from our catalog since that'd return
something like
- * {{{ file:/some/absolute/path/to/some/path.xsd }}}
+ * {{{ URISCHEME:/some/absolute/path/to/some/path.xsd }}}
*
* This is a workaround to that bug where we convert systemId to a URI and
check if the
* path (from URI.getPath) matches the end of resolvedUri. Note: This can
ignore absolute
* URIs passed in for schemaLocation, but those are edge cases where the
user expects
* the namespace to match a different file (i.e what they provide in the
schemalocation)
* than what we find in the catalog.
*/
- lazy val systemIdPath = if (systemIdUri != null && systemIdUri.getScheme
== "file") {
+ lazy val systemIdPath = if (systemIdUri != null && systemIdUri.getScheme
!= null) {
Review Comment:
I think this changes behavior if the systemIdUri schema is "jar". Previously
"jar" schemes would fall into the false block but now they fall into the true
block and jarURI.getPath will return null. It's hard to follow what all this
code is doing, but feels like that might break things?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/DaffodilXMLLoader.scala:
##########
@@ -228,19 +228,19 @@ class DFDLCatalogResolver private ()
* <xs:schema...
* ... xsi:schemaLocation="urn:some:namespace /some/path.xsd"
* }}}
- * Xerces takes that schemaLocation URI and absolutizes it to {{{
file:/some/path.xsd }}}
+ * Xerces takes that schemaLocation URI and absolutizes it to {{{
URISCHEME:/some/path.xsd }}}
Review Comment:
Under GraalVM will Xerces absolutize it to a "resource" scheme? I would
think it would always create a "file" URIs when this bug is hit?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -216,6 +218,18 @@ object Misc {
}
}
+ def optResourceURI(contextURI: URI, relPath: String): Option[URI] = {
+ val relURI = contextURI.resolve(relPath)
Review Comment:
I think `resolvedURI` might be more clear. `relURI` makes me think this URI
is relative, but it should be an absolute URI at this point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
