Update java api to declare thrown checked exceptions #1610

olabusayoT · 2026-01-12T17:03:03Z

Wrap SDE that gets thrown in onPath method with proper error handling using sset.error.
Refactor isError methods to remove lazy evaluation, ensuring accurate error checking/reporting.
Add test cases to verify isError works as expected.
Adjust UnsuppressableException to extend RuntimeException since our Assert macros like aborts are difficult to declare. They are typically issued during runtime anyway so it's more fitting for them to be labeled as such
fix some broken javacode to use @code instead of @link

jadams-tresys · 2026-01-12T17:15:39Z

daffodil-core/src/test/java/org/apache/daffodil/jexample/TestAPI.java

+    DataProcessor dp = pf.onPath("/something/else");
+    assertTrue(pf.isError());


This feels a little weird to me to create a DataProcessor object, but then have to check the processorFactory object to see if the DataProcessor was created correctly.

While I think it's important that isError() should work as expected, is there any benefit in returning null vs allowing the SDE to continue up the chain? From the standpoint of someone using the API I feel like I'd get more useful information more quickly from just having the exception than having to check for null, then dig into the processorFactory object if I wanted the details.

So without the changes, we end up throwing the SDE which Steve said we shouldn't be doing from the ProcessorFactory, that we should instead add the error to the diagnostics. I feel you on the null comment, I think Steve said we ought to create a DataProcessor with p and u set to null, so lemme try that instead, and we can check dp.isError instead?

Checking dp.isError() definitely makes more sense to me and matches how the Compiler API works where the compiler returns a ProcessorFactory, which can then be checked with isError().

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.

jadams-tresys

+1 with one question regarding the isError() changes.

stevedlawrence · 2026-01-12T17:32:42Z

daffodil-core/src/main/scala/org/apache/daffodil/lib/oolag/OOLAG.scala

     */
    def isError: Boolean = isErrorOnce
-    private lazy val isErrorOnce: Boolean = {
+    private def isErrorOnce: Boolean = {


Feels like this wants to stay a val, otherwise we could possibly recalculate it many times.

I changed it to a def since we do need it ro recalculate each time isError is called. With the lazy eval, if it is false once, it doesn't recalculate and just always returns false. These lazy evals were causing the new test to fail, as it gets called during the compileFile and set to false, then when onPath is called and the sset.error sets it, and isError is called again, it still returns false.

With the change suggested below to move the SDE somewhere else, I think this change will go away

I wonder if that means we have a bug somewhere else then. I don't think onPath should change the result of ProcessorFactory.isError. If a failure happens during onPath I think it means whatever failed should affect the DataProcessor.isError.

stevedlawrence · 2026-01-12T17:35:15Z

daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala

+    } catch {
+      case sde: SchemaDefinitionError => {
+        sset.error(sde)
+        null


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.

stevedlawrence · 2026-01-12T17:38:58Z

daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala

  }

-  override lazy val isError: Boolean = sset.isError
+  override def isError: Boolean = sset.isError


Why did this need to change to a def?

See #1610 (comment)

stevedlawrence · 2026-01-12T17:39:37Z

daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaSet.scala

   * demands that all other aspects of compilation occur.
   */
-  override lazy val isError: Boolean = {
+  override def isError: Boolean = {


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.

stevedlawrence · 2026-01-12T17:47:06Z

daffodil-core/src/test/java/org/apache/daffodil/jexample/TestAPI.java

+    DataProcessor dp = pf.onPath("/something/else");
+    assertTrue(pf.isError());


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

stevedlawrence · 2026-01-12T18:07:05Z

daffodil-core/src/main/java/org/apache/daffodil/api/ProcessorFactory.java

   */
  @Experimental
-  CodeGenerator forLanguage(String language);
+  CodeGenerator forLanguage(String language) throws InvalidParserException;


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.

Roger! Created a new InvalidGeneratorException to throw in those cases

stevedlawrence · 2026-01-12T18:18:39Z

daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala

+        sset.createDataProcessor(p = null, u = null)
+      }
+    }
+  }


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:

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?

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?

Just that bit throws the SDE. The spec does say OVC can't be on a global element, which the root is, no? ("The element must not be optional nor an array nor be global.")

Ah yeah, the root element is global. That said, we do allow IVC on global elements, and the spec disallows that too, and we have a lot of tests that make use of. There's an argument that allowing one should allow the other.

stevedlawrence · 2026-01-12T18:22:24Z

daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala

  }

-  override def isError = false
+  override def isError = ssrd == null || ssrd.parser == null && ssrd.unparser == null


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?

I think if onPath failed, it ended up throwing an error so the user never had a chance to check dp.isError

But till do!

stevedlawrence · 2026-01-12T18:40:50Z

daffodil-core/src/test/scala/org/apache/daffodil/sexample/TestAPI.scala

+    assertTrue(dp.isError)
+    assertTrue(pf.isError)


I don't think this test is correct. If pf.isError is true, it should not be valid to call onPath.

stevedlawrence

+1, maybe with some small changes that make stronger assertions that we don't actually ever create DataProcessors with isError == true?

stevedlawrence · 2026-01-14T15:35:37Z

daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala

  }

-  override def isError = false
+  override def isError = ssrd == null || ssrd.parser == null || ssrd.unparser == null


Can this stay isError = false? I think it's impossible for a DataProcessor to actually have an error since we check everything during processor factory.

If ProcessorFactory.isError == true, then calling ProcessorFactory.onPath() should throw an usage exception and we should never get a DataProcessor.

If ProcessorFactory.isError == false, then I think it's impossible to create a DataProcessor since onPath doesn't do any new logic that could lead to any errors. The requiredEvaluates at the tope of SchemaSetRuntime1Mixin ensures eveything (e.g. parser, unparser) are evaluated when the SchemaSet is created in the ProcessorFactory.compile()

Related, I wonder if we should change the onPath function to this:

Assert.invariant(!root.isError) SchemaSetRuntimeData( parser, unparser, ... )

And remove the !root.isError check that can create null parsers/unparser? This makes it clear it's impossible for SchemaSetRuntimeData to get a null parser/unparser, and that it's impossible for a DataProcessor to have an error.

- add InvalidGeneratorException for errors during forLanguage as InvalidParserException is only for reload call - ensure DaffodilParseXMLReader's exception declarations match with XMLReader exception declarations - allow global OVC, replace thrown error for onPath, with stored error - ensure `SchemaSetRuntimeData` cannot have null parser/unparser. - Adjust `UnsuppressableException` to extend `RuntimeException` since our Assert macros like aborts are challenging to declare. They are typically issued during runtime anyway, so it's more fitting for them to be labeled as such - fix some broken javacode to use @code instead of @link Deprecation/Compatibility - Previously we didn't declare checked exceptions that could have been thrown by the API code. Now that we declare them, users of the API will need to explicitly catch these exceptions. - Unlike in previous versions of Daffodil, dfdl:outputValueCalc is now allowed on global/root elements DAFFODIL-2287

olabusayoT requested review from jadams-tresys and stevedlawrence January 12, 2026 17:03

jadams-tresys reviewed Jan 12, 2026

View reviewed changes

jadams-tresys approved these changes Jan 12, 2026

View reviewed changes

stevedlawrence reviewed Jan 12, 2026

View reviewed changes

stevedlawrence approved these changes Jan 14, 2026

View reviewed changes

olabusayoT force-pushed the daf-2287-declared-checked-exceptions branch from d3adc97 to aab8d8f Compare January 14, 2026 20:19

olabusayoT force-pushed the daf-2287-declared-checked-exceptions branch from aab8d8f to da48915 Compare January 14, 2026 22:19

olabusayoT merged commit 01e5483 into apache:main Jan 15, 2026
10 of 11 checks passed

olabusayoT deleted the daf-2287-declared-checked-exceptions branch January 15, 2026 17:03

		DataProcessor dp = pf.onPath("/something/else");
		assertTrue(pf.isError());

Update java api to declare thrown checked exceptions #1610

Update java api to declare thrown checked exceptions #1610

Uh oh!

Conversation

olabusayoT commented Jan 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jadams-tresys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevedlawrence left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants