Add Support for Resource URI Scheme #1451

olabusayoT · 2025-03-06T04:14:49Z

open up check for scheme in resolveCommon to allow for file, jar and resource scheme
check if there's a systemId as part of XercesError in XercesSchemaFileLocation, use that if there is and it's not an absolute version of diagnosticFile, else use diagnosticFile

- open up check for scheme in resolveCommon to allow for file, jar and resource scheme - check if there's a systemId as part of XercesError in XercesSchemaFileLocation, use that if there is and it's not an absolute version of diagnosticFile, else use diagnosticFile DAFFODIL-2973

stevedlawrence · 2025-03-06T13:10:50Z

daffodil-lib/src/main/scala/org/apache/daffodil/lib/exceptions/SchemaFileLocatable.scala

+         // 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),


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?

Correct, there are cases where systemId is defined but it's the absolute version of diagnosticFile causing us to loose the depersonalized paths. So the check is to sort of mitigate that

And the depersonalized one we have in schemaFileLocation isn't necessarily the correct file that the error is about?

Correct, that's what I found while playing around with the quarkus stuff. I'll confirm the accuracy of that again tho. It was complaining about line 577 in a 34 line file iirc

Yep just confirmed that we're getting the right paths now when we weren't before

Xerces Validation works without needing to do anything extra

As an aside, I think XercesSchemaFileLocation is only used when we run into error loading the schema. So we don't use it for validation

As an aside, I think XercesSchemaFileLocation is only used when we run into error loading the schema. So we don't use it for validation

You're talking about loading a DFDL schema right? I don't think it should be used for loading the XMLSchema_for_DFDL.xsd schema.

As such I don't think it represents an error in Daffodil.

It might not necessarily be a bug in Daffodil, it could be in Quarkus as you say, but it still represents a bug somewhere that broke functionality in Daffodil need to load the XML Schema for DFDL schemas. For example, if some tool modified daffodil jars and removed XMLSchema_for_DFDL.xsd schema, that wouldn't want to be reported as an SDE. That's a bug in the tool that broke an assertion that those schemas are available to Daffodil. Breaking that assertion should result in an some kind of exception that's not an SDE, so those tools or Daffodil can be fixed.

Hmm, we use XersesSchemaFileLocation for all loader errors. Are you suggesting we add some special logic for just XMLSchema_for_DFDL.xsd?

Kindof. I'm suggesting that when we load XMLSchema_for_DFDL.xsd, not when we validate with it, we need to detect that it wasn't loaded.

I was hoping we could wrap this line in a try/catch block:

https://github.com/apache/daffodil/blob/main/daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/DaffodilXMLLoader.scala#L737

And throw an assertion when it fails, but i'm not sure if that actually throws an exception on failure. Might need a different approach if that doesn't work.

stevedlawrence · 2025-03-06T13:16:14Z

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] = {


Suggest we call this optRelativeResourceURI to match the pattern of optRelativeJarFileURI, which looks like it does a similar thing but for jars contexts?

stevedlawrence · 2025-03-06T13:28:09Z

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") {


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:

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.

stevedlawrence · 2025-03-06T13:33:35Z

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)


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.

I know this was just copied from getResourceRelativeOnlyOption, but we should rename relURI to resolvedURI here also. relURI really isn't a good description of what this variable represents.

stevedlawrence · 2025-03-06T13:46:14Z

daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala

+    val relURI = contextURI.resolve(relPath)
+    try {
+      relURI.toURL.openStream().close()
+      this.getClass.getResource(relURI.getPath)


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:

if (this.getClass.getResource(relURI.getPath)) Some(relURI) else None

stevedlawrence · 2025-03-06T13:50:22Z

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)


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

- ensure the paths are normalized to the platform appropriate separators before comparing them - refactor getResourceRelativeOnlyOption to be more clear and more accurate since .isOpaque can't be guaranteed to always be true for only jar files; also moved file logic into a function to make more concise - use Paths.get in uriToDiagnosticFile to make more consistent with other schemes DAFFODIL-2973

olabusayoT · 2025-03-06T19:48:55Z

Created https://cwiki.apache.org/confluence/display/DAFFODIL/Working+with+Native+Executables as well as a companion to this ticket @stevedlawrence @mbeckerle @jadams-tresys

- throw invariantFailed assertion in the case that we're unable to load XMLSchema_for_DFDL.xsd so the user is aware something is broken outside of the schema rather than an SDE than indicates something is wrong with the schema DAFFODIL-2973

stevedlawrence

+1, one variable rename, and maybe the XerecesSchemaFileLocation change isn't needed anymore?

stevedlawrence · 2025-03-07T17:32:43Z

daffodil-lib/src/main/scala/org/apache/daffodil/lib/exceptions/SchemaFileLocatable.scala

+        else
+          new File(sysIdNormalizedPath)
+      }
+    }),


Can this change be reverted? Now that failures to load the validation schema result in an assertion, I think XercesSchemaFileLocation will only be created where schemaFileLocation.diagnosticFile is correct.

stevedlawrence · 2025-03-07T17:33:44Z

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)


I know this was just copied from getResourceRelativeOnlyOption, but we should rename relURI to resolvedURI here also. relURI really isn't a good description of what this variable represents.

olabusayoT requested review from mbeckerle, stevedlawrence and jadams-tresys March 6, 2025 04:14

stevedlawrence reviewed Mar 6, 2025

View reviewed changes

fixup!

066448e

- throw invariantFailed assertion in the case that we're unable to load XMLSchema_for_DFDL.xsd so the user is aware something is broken outside of the schema rather than an SDE than indicates something is wrong with the schema DAFFODIL-2973

stevedlawrence approved these changes Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for Resource URI Scheme #1451

Add Support for Resource URI Scheme #1451

olabusayoT commented Mar 6, 2025

stevedlawrence Mar 6, 2025

olabusayoT Mar 6, 2025

stevedlawrence Mar 6, 2025

olabusayoT Mar 6, 2025

olabusayoT Mar 6, 2025

olabusayoT Mar 6, 2025

olabusayoT Mar 6, 2025

stevedlawrence Mar 6, 2025

olabusayoT Mar 6, 2025

stevedlawrence Mar 6, 2025

stevedlawrence Mar 6, 2025

stevedlawrence Mar 6, 2025

stevedlawrence Mar 6, 2025

stevedlawrence Mar 7, 2025

stevedlawrence Mar 6, 2025

stevedlawrence Mar 6, 2025

olabusayoT commented Mar 6, 2025

stevedlawrence left a comment

stevedlawrence Mar 7, 2025

stevedlawrence Mar 7, 2025

Add Support for Resource URI Scheme #1451

Are you sure you want to change the base?

Add Support for Resource URI Scheme #1451

Conversation

olabusayoT commented Mar 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olabusayoT commented Mar 6, 2025

stevedlawrence left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment