stevedlawrence commented on code in PR #1065:
URL: https://github.com/apache/daffodil/pull/1065#discussion_r1532068351
##########
daffodil-core/src/test/scala/org/apache/daffodil/core/util/TestUtils.scala:
##########
@@ -314,7 +314,7 @@ object TestUtils {
val nullChannel = java.nio.channels.Channels.newChannel(nos)
val compiler = Compiler()
val uri = Misc.getRequiredResource(resourcePathString)
- val schemaSource = URISchemaSource(uri)
+ val schemaSource = URISchemaSource(Misc.uriToDiagnosticFile(uri), uri)
Review Comment:
This probably wants to be `Paths.get(resourcePathString).toFile`. I think in
most cases we should avoid using `uriToDiagnosticFile` unless we really only
have a URI and don't have the original thing that led to use getting a URI.
##########
daffodil-core/src/test/scala/org/apache/daffodil/core/xml/TestXMLLoaderWithLocation.scala:
##########
@@ -37,7 +38,8 @@ class TestXMLLoaderWithLocation {
using(new java.io.FileWriter(tmpXMLFileName)) { fw =>
fw.write(testXML.toString())
}
- val res = URISchemaSource(new File(tmpXMLFileName).toURI)
+ val res =
+ URISchemaSource(Paths.get(tmpXMLFileName).toFile, new
File(tmpXMLFileName).toURI)
Review Comment:
We already create a file, can we pull hat out and reuse it insead of doing
Paths.get.toFile?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -1473,13 +1476,22 @@ Differences were (path, expected, actual):
// absolute so we can use it as-is so getResource does not care
what package this
// class is in.
val resource = this.getClass.getResource(uri.getPath)
- if (resource != null) Some((resource.toURI, false)) else None
+ if (resource != null) {
+ val uss = URISchemaSource(Misc.uriToDiagnosticFile(uri),
resource.toURI)
Review Comment:
I don't think we need to use uriToDiagnosticFile here? I this case
`uri.getPath` is just a normal absolute path like `/foo/bar/baz.dfdl.xsd`, so
can we do something like `Paths.get(uri.getPath).toFile`? uriToDiagnosticFile
is a bit generic and just has to make guesses, but in this case we know exactly
how to convert the schemaLocation URI to a good diagnosticFile.
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -320,14 +321,22 @@ class DFDLTestSuite private[tdml] (
Logger.log.info(s"loading TDML file: ${tdmlFile}")
val uri = tdmlFile.toURI()
val newNode =
- loader.load(URISchemaSource(uri), optTDMLSchema, addPositionAttributes
= true)
+ loader.load(
+ URISchemaSource(Misc.uriToDiagnosticFile(uri), uri),
Review Comment:
Use the `file` for the diag.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -325,7 +325,7 @@ class Compiler private (
optRootName: Option[String] = None,
optRootNamespace: Option[String] = None,
): ProcessorFactory = {
- val source = URISchemaSource(file.toURI)
+ val source = URISchemaSource(Misc.uriToDiagnosticFile(file.toURI),
file.toURI)
Review Comment:
Can we pass in `file` as the diagnostic here? `file.toURI` creates an
absolute URI, so the resulting diagnostic File will be absolute even if the
original File was relative. If we pass in `file` directly, our diagnostic file
stays relative.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/api/DaffodilSchemaSource.scala:
##########
@@ -158,17 +167,19 @@ class InputStreamSchemaSource(
inSrc
}
override def uriForLoading = tempURI
+
+ override def diagnosticFile: File = Misc.uriToDiagnosticFile(tempURI)
}
protected sealed abstract class NodeSchemaSourceBase(
node: Node,
nameHint: String,
tmpDir: Option[File],
-) extends URISchemaSource({
- val tempSchemaFile = XMLUtils.convertNodeToTempFile(node, tmpDir.orNull,
nameHint)
- val tempURI = tempSchemaFile.toURI
- tempURI
- }) {
+ tempSchemaFileFromNode: File,
+) extends URISchemaSource(
+ Misc.uriToDiagnosticFile(tempSchemaFileFromNode.toURI),
Review Comment:
Pass in tempSchemaFileFromNode here, no need to convert a File to a URI back
to a File.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -1498,7 +1521,7 @@ Differences were (path, expected, actual):
// if needed. Future versions of Daffodil may want to remove this
orElse block so we
// are strict about how absolute vs relative schemaLocations are
resolved.
val pair = Option(this.getClass.getResource("/" + uri.getPath))
- .map { _.toURI }
+ .map { r => URISchemaSource(Misc.uriToDiagnosticFile(r.toURI),
r.toURI) }
Review Comment:
Similar to above, we probably want to create the diagnostic File directly.
Something like `Paths.get("/" + uri.getPath).toFile`.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -1473,13 +1476,22 @@ Differences were (path, expected, actual):
// absolute so we can use it as-is so getResource does not care
what package this
// class is in.
val resource = this.getClass.getResource(uri.getPath)
- if (resource != null) Some((resource.toURI, false)) else None
+ if (resource != null) {
+ val uss = URISchemaSource(Misc.uriToDiagnosticFile(uri),
resource.toURI)
+ Some((uss, false))
+ } else
+ None
}
.orElse {
// Search for the schemaLocation path on the file system. This
path is absolute so it
// must exist. If it does not exist, this orElse block results in
a None
val file = Paths.get(uri.getPath).toFile
- if (file.exists) Some((file.toURI, false)) else None
+ if (file.exists) {
+ val uss = URISchemaSource(Misc.uriToDiagnosticFile(uri),
file.toURI)
Review Comment:
We already have a File in the `file` variable, can we just use that as the
diagnostic instead of converting it to a URI and then back to a File via
uriToDiagnosticFile?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/api/DaffodilConfig.scala:
##########
@@ -66,7 +67,9 @@ object DaffodilConfig {
}
}
- def fromURI(uri: URI) = fromSchemaSource(URISchemaSource(uri))
+ def fromURI(uri: URI) = fromSchemaSource(
+ URISchemaSource(Misc.uriToDiagnosticFile(uri), uri),
+ )
def fromFile(file: File) = fromURI(file.toURI)
Review Comment:
I'm not sure how much this matters this is just config stuff, but we have a
File here we can use for diagnostics, e.g.
```scala
def fromFile(fiel: File) = fromSchemaSource(
URISchemaSource(file, file.toURI)
}
```
It will create slightly better diagnostics if the file is a relative path.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/externalvars/ExternalVariablesLoader.scala:
##########
@@ -80,7 +81,10 @@ object ExternalVariablesLoader {
val input = scala.io.Source.fromURI(file.toURI)(enc)
val ldr = new DaffodilXMLLoader()
val dafextURI = XMLUtils.dafextURI
- val node = ldr.load(URISchemaSource(file.toURI), Some(dafextURI))
+ val node = ldr.load(
+ URISchemaSource(Misc.uriToDiagnosticFile(file.toURI), file.toURI),
Review Comment:
Just use `file` as the diagnostic.
##########
daffodil-sapi/src/test/scala/org/apache/daffodil/example/TestScalaAPI.scala:
##########
@@ -345,7 +345,9 @@ class TestScalaAPI {
def testScalaAPI4b(): Unit = {
val c = Daffodil.compiler()
- val schemaFileName = getResource("/test/sapi/mySchema3.dfdl.xsd")
Review Comment:
Should we revert these changes so they match the Java API tests?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -1488,8 +1500,19 @@ Differences were (path, expected, actual):
val contextURI = optContextURI.get
val optResolvedRelative = None
.orElse {
+ val contextURIDiagnosticFile = contextURI.diagnosticFile
+ val relativeDiagnosticFile =
+ contextURIDiagnosticFile.toPath
+ .resolveSibling(Paths.get(uri.getPath))
+ .normalize()
+ .toFile
Review Comment:
Can we move these vals inside the map below? We probably only want to do
this logic is resolveRelativeOnly found something.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -708,4 +708,18 @@ object Misc {
}
Some(res)
}
+
+ /**
+ * Get the diagnosticFilepath from a uri
+ */
+ def uriToDiagnosticFile(uri: URI): File = {
+ uri.getScheme match {
+ // in windows, some jar uris have file as the schema, so we treat
+ // them both the same, just in case
Review Comment:
I think this comment can be removed? We dont' treat jar and file schemes the
same.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/DaffodilXMLLoader.scala:
##########
@@ -244,12 +245,18 @@ class DFDLCatalogResolver private ()
// comes from the schemaLocation attribute) on classpath or as a file.
val optURI =
try {
- val contextURI = Some(new URI(baseURIString))
+ val baseURI = new URI(baseURIString)
+ // we set diagnostic file path to empty string here because
baseURIString has a scheme
+ // and that can cause problems down the road in
resolveSchemaLocation. We don't care
+ // about the diagnosticFilepath here since this information is
from Xerces, so we're fine
+ // with it being blank
Review Comment:
I think you missed this suggestion?
--
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]
