stevedlawrence commented on code in PR #1610:
URL: https://github.com/apache/daffodil/pull/1610#discussion_r2683239732
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -118,7 +118,16 @@ final class ProcessorFactory private (
def getDiagnostics: java.util.List[api.Diagnostic] = diagnostics.asJava
- override def onPath(xpath: String): api.DataProcessor = sset.onPath(xpath)
+ override def onPath(xpath: String): api.DataProcessor = {
+ try {
+ sset.onPath(xpath)
+ } catch {
+ case sde: SchemaDefinitionError => {
+ sset.error(sde)
+ null
Review Comment:
I don't think this wants to return null, we should still return a
DataProcessor, it just wants to be one where isError is true and it contains
the SDE as a diagnostic. In our conversation, the null I suggested was the
parser and unparser memebers of the DataProcessor.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/oolag/OOLAG.scala:
##########
@@ -487,7 +487,7 @@ object OOLAG {
* that have been created and activated at the time this is called.
*/
def isError: Boolean = isErrorOnce
- private lazy val isErrorOnce: Boolean = {
+ private def isErrorOnce: Boolean = {
Review Comment:
Feels like this wants to stay a val, otherwise we could possibly recalculate
it many times.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaSet.scala:
##########
@@ -556,7 +556,7 @@ final class SchemaSet private (
* and finally the AST objects are checked for errors, which recursively
* demands that all other aspects of compilation occur.
*/
- override lazy val isError: Boolean = {
+ override def isError: Boolean = {
Review Comment:
Feels like this wants to stay a lazy val so we don't have to recalculate it
every time it's called. I imagine few thing call it twice but if they do we
don't want to have to recalculate anything.
##########
daffodil-core/src/test/scala/org/apache/daffodil/sexample/TestAPI.scala:
##########
@@ -226,6 +226,19 @@ class TestAPI {
assertEquals("42", bos.toString())
}
}
+
+ // This is a duplicate of testAPII with an incorrect ProcessorFactory path
+ @Test
+ def testAPI1_B(): Unit = {
+ val c = Daffodil.compiler
+ val schemaFile = getResource("/test/api/mySchema1.dfdl.xsd")
+ val pf = c.compileFile(schemaFile)
+ val dp = pf.onPath("/something/else")
+ assertNotNull(dp)
+ assertTrue(dp.isError)
+ assertTrue(pf.isError)
Review Comment:
I don't think this test is correct. If pf.isError is true, it should not be
valid to call onPath.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -190,7 +190,7 @@ class DataProcessor(
copy(variableMap = newVariableMap)
}
- override def isError = false
+ override def isError = ssrd == null || ssrd.parser == null && ssrd.unparser
== null
Review Comment:
Does the `&&` want to be `||`, so that if either parser or unparser is null
it is considered an error?
That said, how did this work at all? How did DataProccessor.isError ever
return true? Did onPath just never really fail?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -146,7 +155,7 @@ final class ProcessorFactory private (
codeGenerator
}
- override lazy val isError: Boolean = sset.isError
+ override def isError: Boolean = sset.isError
Review Comment:
Why did this need to change to a def?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -118,7 +118,16 @@ final class ProcessorFactory private (
def getDiagnostics: java.util.List[api.Diagnostic] = diagnostics.asJava
- override def onPath(xpath: String): api.DataProcessor = sset.onPath(xpath)
+ override def onPath(xpath: String): api.DataProcessor = {
+ try {
+ sset.onPath(xpath)
+ } catch {
+ case sde: SchemaDefinitionError => {
+ sset.error(sde)
+ sset.createDataProcessor(p = null, u = null)
+ }
+ }
+ }
Review Comment:
I'm wondering if we can avoid this extra complication?
It seems like this is all needed just becomes of the check in
SchemaSetRuntime1Mixin.onPath checking if the element is OVC?
Two thoughts on this:
1. This feels like an odd place for this particular check. Can we move it
somewhere more appropriate that will capture SDE's correctly? Maybe it could be
done in Root.scala, or maybe somewhere else that can check if it's a root
element?
2. Or, can we just remove the check? I can't find anywhere in the spec that
says it's illegal to have OVC on the root element. It doesn't seem particularly
useful, in the real world, but it could be useful for tests. We do allow IVC on
root element for testing purposes, so why not OVC?
Or are there other things that can lead to an SDE being thrown?
##########
daffodil-core/src/main/java/org/apache/daffodil/api/DaffodilParseXMLReader.java:
##########
@@ -149,36 +153,36 @@ public interface DaffodilParseXMLReader extends XMLReader
{
*
* @param input data to be parsed
*/
- void parse(InputSource input);
+ void parse(InputSource input) throws IOException;
/**
* Parse data from a system identifier/URI. This method is not supported by
the API.
*
* @param systemId URI for data to be parsed
*/
- void parse(String systemId);
+ void parse(String systemId) throws IOException;
/**
* Parse input data from an InputSourceDataInputStream. Infoset can
retrieved via the registered
* contentHandler and diagnostics via the registered errorHandler
*
* @param isdis data to be parsed
*/
- void parse(InputSourceDataInputStream isdis);
+ void parse(InputSourceDataInputStream isdis) throws SAXException;
/**
* Parse input data from an InputStream. Infoset can retrieved via the
registered contentHandler
* and diagnostics via the registered errorHandler
*
* @param stream data to be parsed
*/
- void parse(InputStream stream);
+ void parse(InputStream stream) throws SAXException;
/**
* Parse input data from an array of bytes. Infoset can be retrieved via the
registered
* contentHandler and diagnostics via the registered errorHandler
*
* @param arr data to be parsed
*/
- void parse(byte[] arr);
+ void parse(byte[] arr) throws SAXException;
}
Review Comment:
These are not backwards compatible changes--users will now need to catch
these exceptions. I think this is the correct change and users should need to
catch these since that's how the XMLReader API work, but it needs a
`Deprecation/Compatibility` note in the commit message.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/ProcessorFactory.java:
##########
@@ -46,5 +47,5 @@ public interface ProcessorFactory extends WithDiagnostics {
* @return a {@link CodeGenerator} to generate code from a DFDL schema to
parse or unparse data
*/
@Experimental
- CodeGenerator forLanguage(String language);
+ CodeGenerator forLanguage(String language) throws InvalidParserException;
Review Comment:
This is probably not the right exception forLanguage to be throwing. This
exception should really only be used by DataProcessor.reload() to throw if the
thing to reload isn't a valid parser. I'm guessing the C code generator was
just a copy/paste error that no one noticed. We should probably change this to
something else. Doesn't necessarily need to be done as this PR.
##########
daffodil-core/src/test/java/org/apache/daffodil/jexample/TestAPI.java:
##########
@@ -242,6 +243,20 @@ public void testAPI1_A_Full_SavedParser() throws Exception
{
assertNotNull(parser);
}
+ // This is a duplicate of testAPII with an incorrect ProcessorFactory path
+ @Test
+ public void testAPI1_B() throws IOException, ClassNotFoundException {
+ DebuggerRunnerForAPITest customRunner = new DebuggerRunnerForAPITest();
+ Debugger debugger = Daffodil.newDaffodilDebugger(customRunner);
+
+ org.apache.daffodil.api.Compiler c = Daffodil.compiler();
+ java.io.File schemaFile = getResource("/test/api/mySchema1.dfdl.xsd");
+ ProcessorFactory pf = c.compileFile(schemaFile);
+ DataProcessor dp = pf.onPath("/something/else");
+ assertTrue(pf.isError());
Review Comment:
Calling `pf.onPath` on a ProcessorFactory where isError is true is
considered a usage error, just like how calling `dp.parse(...)` on a
DataProcessor where isError is true is also a usage error.
So I think test should throw a usage error when onPath is called. The
isError assert probably wants to be done before onPath is called.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/DaffodilParseXMLReader.java:
##########
@@ -61,31 +65,31 @@ public interface DaffodilParseXMLReader extends XMLReader {
* @param name feature flag whose value is to be retrieved
* @return value of the feature flag
*/
- boolean getFeature(String name);
+ boolean getFeature(String name) throws SAXNotRecognizedException;
Review Comment:
Many of these functions don't match the exceptions that the XMLReader API
throws. Should we change these to match?
--
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]
