This is an automated email from the ASF dual-hosted git repository.
slawrence pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/daffodil.git
The following commit(s) were added to refs/heads/main by this push:
new b8976946e Fix ProcessorFactory.withDistinguishedRootNode and isError
b8976946e is described below
commit b8976946ec02c0475ce665b527db3d6c7014dba9
Author: Steve Lawrence <[email protected]>
AuthorDate: Tue Nov 28 14:12:30 2023 -0500
Fix ProcessorFactory.withDistinguishedRootNode and isError
When calling withDistinguishedRootNode, we copy the ProcessorFactory and
replace the root spec with the new node. When we do this, we also copy
the existing SchemaSet. But this SchemaSet was created with the previous
root spec, which means withDistinguishedRootNode doesn't actually do
anything.
Fortunately, there is a workaround which is to pass in the root
name/namspace to the compileSource/File functions, so that the root spec
is available when the SchemaSet is created, and avoid
withDistinguishedRootNode entirely.
But to fix this, this modifies withDistinguishedRootNode to not copy the
SchemaSet, so a new SchemaSet is created and initialized with the
correct root spec. Each ProcessorFactory now has a unique SchemaSet
instance with the correct root name and namespace.
This also discovered cases in the Compiler.compileSourceInternal and
JAPI/SAPI compileFile functions where they called isError on the newly
created ProcessorFactory before returning it to the caller. This meant
that the SchemaSet inside the ProcessorFactory would be forced to be
created, even though it might never be used, like in the case of
withDistinguishedRootNode being called and a different SchemaSet being
created.
Really the isError function should only ever be called by the user when
they are ready for the object to be evaluated, and not forced by
Daffodil internals. This ensures we avoid unnecessary work until the
user is ready, since isError is what eventually causes lazy vals to be
evaluated. However, this does mean that if a user doesn't check isError
they might get exceptions/assertions, but it is well documented in the
JAPI/SAPI that isError should be checked before calling other functions.
This also means that some debug logging that relies on knowing that
status of isError must be moved elsewhere, since the user might not have
called isError yet.
This also discovered instances in the TDMLRunner and CLI where we
queried diagnostics before checking isError--isError should always be
done first, which is fixed.
That said, this also modifies ProcessorFactory.getDiagnostics so that it
calls isError before returning the diagnostics. This way if user calls
getDiagnostics without first calling isError (like we used to do in the
TDMLRunner and CLI), the compilation work will be done and correct
diagnostics generated. Without this, there would be no diagnostics
because no work would be done yet. Although technically a user should
call isError before asking for diagnostics, there may be cases where
users don't, so this allows that to keep working as expected. This is
similar to how onPath calls isError even if a user doesn't.
DAFFODIL-2864, DAFFODIL-697
---
.../main/scala/org/apache/daffodil/cli/Main.scala | 14 +++++-
.../apache/daffodil/core/compiler/Compiler.scala | 38 ++++++---------
.../core/runtime1/SchemaSetRuntime1Mixin.scala | 5 --
.../scala/org/apache/daffodil/japi/Daffodil.scala | 1 -
.../org/apache/daffodil/example/TestJavaAPI.java | 55 ++++++++++++++++++++++
.../scala/org/apache/daffodil/sapi/Daffodil.scala | 1 -
.../org/apache/daffodil/example/TestScalaAPI.scala | 52 ++++++++++++++++++++
.../processor/tdml/DaffodilTDMLDFDLProcessor.scala | 2 +-
8 files changed, 136 insertions(+), 32 deletions(-)
diff --git a/daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala
b/daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala
index 2162fc3a7..b30db8871 100644
--- a/daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala
+++ b/daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala
@@ -72,6 +72,7 @@ import
org.apache.daffodil.runtime1.debugger.TraceDebuggerRunner
import org.apache.daffodil.runtime1.externalvars.ExternalVariablesLoader
import org.apache.daffodil.runtime1.layers.LayerExecutionException
import org.apache.daffodil.runtime1.processors.DataLoc
+import org.apache.daffodil.runtime1.processors.DataProcessor
import org.apache.daffodil.runtime1.processors.ExternalVariableException
import org.apache.daffodil.runtime1.udf.UserDefinedFunctionFatalErrorException
import org.apache.daffodil.slf4j.DaffodilLogger
@@ -960,6 +961,11 @@ class Main(
// saved processor. Those are from compile time, are all warnings, and
// are just noise in a production system where we're reloading a parser.
if (!processor.isError) {
+ Logger.log.whenDebugEnabled {
+ val processorImpl = processor.asInstanceOf[DataProcessor]
+ Logger.log.debug(s"Parser = ${processorImpl.ssrd.parser.toString}")
+ Logger.log.debug(s"Unparser =
${processorImpl.ssrd.unparser.toString}")
+ }
Some(processor.withValidationMode(mode))
} else {
None
@@ -1024,10 +1030,16 @@ class Main(
val processorFactory = compiler.compileSource(schemaSource)
if (!processorFactory.isError) {
val processor =
processorFactory.onPath(path.getOrElse("/")).withValidationMode(mode)
- displayDiagnostics(processor)
if (processor.isError) {
+ displayDiagnostics(processor)
None
} else {
+ displayDiagnostics(processor)
+ Logger.log.whenDebugEnabled {
+ val processorImpl = processor.asInstanceOf[DataProcessor]
+ Logger.log.debug(s"Parser =
${processorImpl.ssrd.parser.toString}")
+ Logger.log.debug(s"Unparser =
${processorImpl.ssrd.unparser.toString}")
+ }
Some(processor)
}
} else {
diff --git
a/daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala
b/daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala
index d6a05556d..75ec86408 100644
---
a/daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala
+++
b/daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala
@@ -38,7 +38,6 @@ import org.apache.daffodil.lib.api.Diagnostic
import org.apache.daffodil.lib.api.URISchemaSource
import org.apache.daffodil.lib.api.UnitTestSchemaSource
import org.apache.daffodil.lib.exceptions.Assert
-import org.apache.daffodil.lib.util.Logger
import org.apache.daffodil.lib.util.Misc
import org.apache.daffodil.runtime1.api.DFDL
import org.apache.daffodil.runtime1.processors.DataProcessor
@@ -65,7 +64,6 @@ final class ProcessorFactory private (
val validateDFDLSchemas: Boolean,
checkAllTopLevel: Boolean,
tunables: DaffodilTunables,
- optSchemaSet: Option[SchemaSet],
) extends DFDL.ProcessorFactory {
def this(
@@ -82,7 +80,6 @@ final class ProcessorFactory private (
validateDFDLSchemas,
checkAllTopLevel,
tunables,
- None,
)
private def copy(optRootSpec: Option[RootSpec] = optRootSpec):
ProcessorFactory =
@@ -92,19 +89,29 @@ final class ProcessorFactory private (
validateDFDLSchemas,
checkAllTopLevel,
tunables,
- Some(sset),
)
lazy val sset: SchemaSet =
- optSchemaSet.getOrElse(
- SchemaSet(optRootSpec, schemaSource, validateDFDLSchemas,
checkAllTopLevel, tunables),
- )
+ SchemaSet(optRootSpec, schemaSource, validateDFDLSchemas,
checkAllTopLevel, tunables)
lazy val rootView: RootView = sset.root
def elementBaseInstanceCount: Long = sset.elementBaseInstanceCount
- def diagnostics: Seq[Diagnostic] = sset.diagnostics
+ def diagnostics: Seq[Diagnostic] = {
+ // The work to compile a schema and build diagnostics is triggered by the
user calling
+ // isError. But if a user gets diagnostics before doing so, then no work
will have been done
+ // and the diagnostics will be empty. Technically this is incorrect
usage--a user should
+ // always call isError before getting diagnostics. But there are known
instances where users
+ // have done this. We could detect this and throw a usage assertion so
users know to fix it,
+ // but if they want diagnostics then they likely expected the work to have
been done
+ // already, so lets just call isError to trigger that work so they get
what they expect.
+ // Note that we don't check the result of isError, since it is perfectly
reasonable for
+ // errors to exist when a user asks for diagnostics--we only call it for
its side-effects.
+ isError
+ sset.diagnostics
+ }
+
def getDiagnostics: Seq[Diagnostic] = diagnostics
override def onPath(xpath: String): DFDL.DataProcessor = sset.onPath(xpath)
@@ -359,21 +366,6 @@ class Compiler private (
)
}
- val err = pf.isError
- val diags = pf.getDiagnostics // might be warnings even if not isError
- if (err) {
- Assert.invariant(diags.nonEmpty)
- Logger.log.debug(
- s"Compilation (ProcessorFactory) produced ${diags.length}
errors/warnings.",
- )
- } else {
- if (diags.nonEmpty) {
- Logger.log.debug(s"Compilation (ProcessorFactory) produced
${diags.length} warnings.")
- } else {
- Logger.log.debug(s"ProcessorFactory completed with no errors.")
- }
- }
- Logger.log.debug(s"Schema had ${pf.elementBaseInstanceCount} elements.")
pf
}
diff --git
a/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala
b/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala
index 7e26c0a27..4e9ce52b1 100644
---
a/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala
+++
b/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala
@@ -72,11 +72,6 @@ trait SchemaSetRuntime1Mixin {
)
val dataProc =
new DataProcessor(ssrd, tunable, variableMap.copy(), diagnostics =
this.diagnostics)
- if (dataProc.isError) {} else {
- Logger.log.debug(s"Parser = ${ssrd.parser.toString}.")
- Logger.log.debug(s"Unparser = ${ssrd.unparser.toString}.")
- Logger.log.debug(s"Compilation (DataProcesor) completed with no errors.")
- }
dataProc
}
}
diff --git
a/daffodil-japi/src/main/scala/org/apache/daffodil/japi/Daffodil.scala
b/daffodil-japi/src/main/scala/org/apache/daffodil/japi/Daffodil.scala
index 0542badfb..ae33e22e1 100644
--- a/daffodil-japi/src/main/scala/org/apache/daffodil/japi/Daffodil.scala
+++ b/daffodil-japi/src/main/scala/org/apache/daffodil/japi/Daffodil.scala
@@ -141,7 +141,6 @@ class Compiler private[japi] (private var sCompiler:
SCompiler) {
): ProcessorFactory = {
val pf = sCompiler.compileFile(schemaFile, Option(rootName),
Option(rootNamespace))
- pf.isError
new ProcessorFactory(pf)
}
diff --git
a/daffodil-japi/src/test/java/org/apache/daffodil/example/TestJavaAPI.java
b/daffodil-japi/src/test/java/org/apache/daffodil/example/TestJavaAPI.java
index 1ad39bd5c..4ba616953 100644
--- a/daffodil-japi/src/test/java/org/apache/daffodil/example/TestJavaAPI.java
+++ b/daffodil-japi/src/test/java/org/apache/daffodil/example/TestJavaAPI.java
@@ -1346,4 +1346,59 @@ public class TestJavaAPI {
Files.delete(blobDir);
}
}
+
+ /**
+ * Verify that ProcessorFactory.withDistinguishedRootNode selects the
right node
+ */
+ @Test
+ public void testJavaAPIWithDistinguishedRootNode() throws IOException,
ClassNotFoundException {
+ org.apache.daffodil.japi.Compiler c = Daffodil.compiler();
+
+ // e3 is defined first in mySchema3.dfdl.xsd, so if
withDistinguishedRootNode is ignored,
+ // this should give a different result
+ java.io.File schemaFile = getResource("/test/japi/mySchema3.dfdl.xsd");
+ ProcessorFactory pf = c.compileFile(schemaFile);
+ pf = pf.withDistinguishedRootNode("e4", null);
+ DataProcessor dp = pf.onPath("/");
+
+ java.io.File file = getResource("/test/japi/myData16.dat");
+ java.io.FileInputStream fis = new java.io.FileInputStream(file);
+ InputSourceDataInputStream dis = new InputSourceDataInputStream(fis);
+ JDOMInfosetOutputter outputter = new JDOMInfosetOutputter();
+ ParseResult res = dp.parse(dis, outputter);
+ boolean err = res.isError();
+ assertFalse(err);
+ assertEquals(5, res.location().bytePos1b());
+ assertEquals(33, res.location().bitPos1b());
+
+ java.io.ByteArrayOutputStream bos = new
java.io.ByteArrayOutputStream();
+ java.nio.channels.WritableByteChannel wbc =
java.nio.channels.Channels.newChannel(bos);
+ JDOMInfosetInputter inputter = new
JDOMInfosetInputter(outputter.getResult());
+ UnparseResult res2 = dp.unparse(inputter, wbc);
+ err = res2.isError();
+ assertFalse(err);
+ assertEquals("9100", bos.toString());
+ }
+
+ /***
+ * Verify that a user can get diagnostics without having to call isError
+ *
+ * @throws IOException
+ */
+ @Test
+ public void testJavaAPIGetDiagnostics() throws IOException {
+ org.apache.daffodil.japi.Compiler c = Daffodil.compiler();
+ java.io.File schemaFile = new
java.io.File("/test/japi/notHere1.dfdl.xsd");
+ ProcessorFactory pf = c.compileFile(schemaFile);
+ List<Diagnostic> diags = pf.getDiagnostics();
+ boolean found1 = false;
+ for (Diagnostic d : diags) {
+ if (d.getMessage().contains("notHere1")) {
+ found1 = true;
+ }
+ }
+ assertTrue(found1);
+ assertTrue(pf.isError());
+ }
+
}
diff --git
a/daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/Daffodil.scala
b/daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/Daffodil.scala
index 805babdcc..7eec0f333 100644
--- a/daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/Daffodil.scala
+++ b/daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/Daffodil.scala
@@ -136,7 +136,6 @@ class Compiler private[sapi] (private var sCompiler:
SCompiler) {
optRootNamespace: Option[String] = None,
): ProcessorFactory = {
val pf = sCompiler.compileFile(schemaFile, optRootName, optRootNamespace)
- pf.isError
new ProcessorFactory(pf)
}
diff --git
a/daffodil-sapi/src/test/scala/org/apache/daffodil/example/TestScalaAPI.scala
b/daffodil-sapi/src/test/scala/org/apache/daffodil/example/TestScalaAPI.scala
index 14b31031b..43f551eca 100644
---
a/daffodil-sapi/src/test/scala/org/apache/daffodil/example/TestScalaAPI.scala
+++
b/daffodil-sapi/src/test/scala/org/apache/daffodil/example/TestScalaAPI.scala
@@ -1325,4 +1325,56 @@ class TestScalaAPI {
Files.delete(blobDir)
}
}
+
+ /**
+ * Verify that ProcessorFactory.withDistinguishedRootNode selects the right
node
+ */
+ @Test
+ def testScalaAPIWithDistinguishedRootNode(): Unit = {
+ val c = Daffodil.compiler()
+
+ // e3 is defined first in mySchema3.dfdl.xsd, so if
withDistinguishedRootNode is ignored,
+ // this should give a different result
+ val schemaFile = getResource("/test/sapi/mySchema3.dfdl.xsd")
+ val pf = c
+ .compileFile(schemaFile)
+ .withDistinguishedRootNode("e4", null)
+ val dp1 = pf.onPath("/")
+ val dp = reserializeDataProcessor(dp1)
+
+ val file = getResource("/test/sapi/myData16.dat")
+ val fis = new java.io.FileInputStream(file)
+ val input = new InputSourceDataInputStream(fis)
+ val outputter = new ScalaXMLInfosetOutputter()
+ val res = dp.parse(input, outputter)
+ val err = res.isError()
+ assertFalse(err)
+ assertEquals(5, res.location().bytePos1b())
+ assertEquals(33, res.location().bitPos1b())
+
+ val bos = new java.io.ByteArrayOutputStream()
+ val wbc = java.nio.channels.Channels.newChannel(bos)
+ val inputter = new ScalaXMLInfosetInputter(outputter.getResult)
+ val res2 = dp.unparse(inputter, wbc)
+ val err2 = res2.isError();
+ assertFalse(err2);
+ assertEquals("9100", bos.toString());
+ }
+
+ /**
+ * Verify that a user can get diagnostics without having to call isError
+ */
+ @Test
+ def testScalaAPIGetDiagnostics(): Unit = {
+ val c = Daffodil.compiler()
+
+ val schemaFile = new java.io.File("/test/sapi/notHere1.dfdl.xsd")
+ val pf = c.compileFile(schemaFile)
+ val diags = pf.getDiagnostics
+ val found1 = diags.exists { _.getMessage().contains("notHere1") }
+
+ assertTrue(found1)
+ assertTrue(pf.isError())
+ }
+
}
diff --git
a/daffodil-tdml-processor/src/main/scala/org/apache/daffodil/processor/tdml/DaffodilTDMLDFDLProcessor.scala
b/daffodil-tdml-processor/src/main/scala/org/apache/daffodil/processor/tdml/DaffodilTDMLDFDLProcessor.scala
index b1f933130..d1e3f7db2 100644
---
a/daffodil-tdml-processor/src/main/scala/org/apache/daffodil/processor/tdml/DaffodilTDMLDFDLProcessor.scala
+++
b/daffodil-tdml-processor/src/main/scala/org/apache/daffodil/processor/tdml/DaffodilTDMLDFDLProcessor.scala
@@ -128,8 +128,8 @@ final class TDMLDFDLProcessorFactory private (
optRootNamespace: Option[String],
): TDML.CompileResult = {
val pf = compiler.compileSource(schemaSource, optRootName,
optRootNamespace)
- val diags = pf.getDiagnostics
if (pf.isError) {
+ val diags = pf.getDiagnostics
Left(diags)
} else {
val res = this.generateProcessor(pf, useSerializedProcessor)
