mbeckerle commented on code in PR #1058:
URL: https://github.com/apache/daffodil/pull/1058#discussion_r1277874633
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -340,8 +340,11 @@ class DataProcessor(
* runtime. Instead we deal with success and failure statuses.
*/
def parse(input: InputSourceDataInputStream, output: InfosetOutputter):
DFDL.ParseResult = {
- Assert.usage(!this.isError)
-
+ Assert.illegalState(
Review Comment:
This line is repeated multiple places. Should just be:
```
checkNotError()
```
##########
daffodil-japi/src/test/java/org/apache/daffodil/example/TestJavaAPI.java:
##########
@@ -1216,6 +1216,24 @@ public void testJavaAPI26() throws IOException,
ClassNotFoundException, External
assertTrue(DaffodilXMLEntityResolver.getLSResourceResolver() != null);
}
+ @Test
+ public void testJavaAPI27() throws IllegalStateException, IOException {
+ org.apache.daffodil.japi.Compiler c = Daffodil.compiler();
+ java.io.File schemaFile = getResource("/test/japi/mySchema6.dfdl.xsd");
+ ProcessorFactory pf = c.compileFile(schemaFile);
+ assertTrue(pf.isError());
+ try {
+ pf.onPath("/");
+ } catch (IllegalStateException e) {
+ String message = e.getMessage();
+ String cause = e.getCause().toString();
+ assertTrue(message.contains("ProcessorFactory onPath() must not"));
Review Comment:
Just test for "Schema Definition Error" and "nonExistant".
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -110,7 +110,11 @@ final class ProcessorFactory private (
override def onPath(xpath: String): DFDL.DataProcessor = sset.onPath(xpath)
override def forLanguage(language: String): DFDL.CodeGenerator = {
- Assert.usage(!isError)
+ Assert.illegalState(
Review Comment:
This name really bothers me.
I think what we are missing is this method
```
Assert.usage(predicate: Boolean, cause: Throwable)
```
This would be a macro that doesn't evaluate the cause unless the predicate
fails.
the cause would be a Throwable.
So then the API usage check around isError would be
```
Assert.usage(!isError, new
IllegalStateException(getDiagnostics.find(_.isError).get))
```
So you would get a usage error caused by illegal state caused by a
compilation error.
The fact that this particular illegal usage was because you called it when
in the wrong state is why you are choosing illegalStateException. The
diagnostic object being an error is the ultimate cause.
In our API we check isError a lot, so we can define:
```
def checkNotError(): Unit = {
Assert.usage(!isError, new
IllegalStateException(getDiagnostics.find(_.isError).get))
}
```
Then just change all the isError usage checks to checkNotError().
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala:
##########
@@ -68,7 +68,10 @@ trait SchemaSetRuntime1Mixin {
}.value
def onPath(xpath: String): DFDL.DataProcessor = {
- Assert.usage(!isError)
+ Assert.argCheck(
+ !isError,
+ s"Must check isError before calling onPath. ${getDiagnostics.mkString}",
+ )
Review Comment:
It would not hurt to call checkNotError() here. But since this is internal
code, other usage checks like that args are non-null should just be unadorned
`usage(test)` only with no other clutter AFAIC. An important goal for this
Assert.usage(...) and similar is no code clutter. If you even put a message
string in there that's already too much.
##########
daffodil-sapi/src/test/scala/org/apache/daffodil/example/TestScalaAPI.scala:
##########
@@ -1207,6 +1207,26 @@ class TestScalaAPI {
assertEquals(expectedData, bos.toString())
}
+ @Test
+ def testScalaAPI26(): Unit = {
+ val c = Daffodil.compiler()
+ val schemaFile = getResource("/test/sapi/mySchema6.dfdl.xsd")
+ val pf = c.compileFile(schemaFile)
+ assertTrue(pf.isError())
+ try {
+ pf.onPath("/")
+ } catch {
+ case e: IllegalStateException => {
+ val msg = e.getMessage
+ val cause = e.getCause.toString
+ assertTrue(msg.contains("ProcessorFactory onPath() must not"))
Review Comment:
These couple lines checking the message... I wouldn't bother with these
first two checks. Just that you got Schema Definition Error and "nonExistant"
is enough.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/exceptions/Assert.scala:
##########
@@ -91,6 +91,12 @@ object Assert extends Assert {
def usage(testAbortsIfFalse: Boolean, message: String): Unit = macro
AssertMacros.usageMacro2
def usage(testAbortsIfFalse: Boolean): Unit = macro AssertMacros.usageMacro1
+ /**
+ * Test for the validity of the state of the API functions
+ */
+ def illegalState(testThrowsIfFalse: Boolean, message: String, cause:
Throwable): Unit =
Review Comment:
Do we have other illegal state checks than just the isError issue?
I suggest we replace this with checkNotError().
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/exceptions/Assert.scala:
##########
Review Comment:
I think this is the wrong approach. We should not have different checkers
for public/non-public API.
I don't know why usage doesn't throw UsageException. Seems to me it should
throw that.
The definition of UsageException is incomplete and that's probably why. It
needs to be just like Abort.
```
class UsageException(m: String, th: Throwable) extends
UnsuppressableException(m, th) {
def this(th: Throwable) = this(null, th)
def this(m: String) = this(m, null)
}
```
That way it can be thrown with a message or a cause. I think this is the
right pattern any time one derives an exception/error class - you derive from
the 2-arg version, and offer two single-arg constructors for message and cause.
--
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]
