You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "olabusayoT (via GitHub)" <gi...@apache.org> on 2025/04/30 21:36:46 UTC
[PR] WIP: Refactor Code and Add New Single API [daffodil]
olabusayoT opened a new pull request, #1479:
URL: https://github.com/apache/daffodil/pull/1479
I split into multiple commits for ease of review.
I marked as draft due to testing in Daffodil 3.3.5 returning internal compiler errors for private lazy val classname in Named.scala...so I have to resolve that.
- renamed original api package to iapi
- sapi/japi -> new api package in runtime
- runtime1-layers/unparser, core, io, lib moved to runtime1
- LocationInSchemaFile (aka loc).toString is now .asString because it must remain an interface, due to scala not allowing one to extend two classes (or a trait that extends a class and another abstract class...as would be the class in HasLotsOfProperties class), and interfaces don't allow override toString
- removed references to sapi/japi
- added new api functions/classes/interfaces
- fixed unidoc
DAFFODIL-2975, DAFFODIL-2902, DAFFODIL-1747
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2210780162
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Result.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import java.util.List;
+
+/**
+ * Interface for Parse and Unparse results
+ */
+public interface Result {
+ State resultState();
Review Comment:
Decided to change it anyways
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2193077686
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Diagnostic.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * Class containing diagnostic information. This does not need to
+ * be implemented by the user.
+ */
+public abstract class Diagnostic extends Exception {
+
+ public Diagnostic(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) {
+ super(message, cause, enableSuppression, writableStackTrace);
+ }
+
+ /**
+ * Get all diagnostic information, including mode name, schema context, and data location
+ * <p>
+ * Future versions of Daffodil will change getMessage to return only the message portion of a
+ * diagnostic, matching the behavior of getMessageOnly--users should call toString() instead.
+ *
+ * @return diagnostic message in string form
+ */
+ public abstract String getMessage();
+
+ /**
+ * Get only the message part of a diagnostic.
+ * <p>
+ * This does not include mode name, schema context, or data location information
+ *
+ * @return diagnostic message in string form
+ */
+ public abstract String getMessageOnly();
+
+ /**
+ * Get the diagnostic mode name
+ *
+ * @return diagnostic mode name in string form
+ */
+ public abstract String getModeName();
+
+ /**
+ * Get a string containing the mode name, message, schema location, and data location combined
+ * into a single string
+ *
+ * @return all diagnostic information as a string
+ */
+ @Override
+ public abstract String toString();
+
+
+ /**
+ * Get data location information relevant to this diagnostic object.
+ * <p>
+ * For example, this might be a file name, and position within the file.
+ *
+ * @return list of [[DataLocation]]'s related to this diagnostic
+ */
+ public abstract List<DataLocation> getDataLocations();
+
+ /**
+ * Get schema location information relevant to this diagnostic object.
+ * <p>
+ * For example, this might be a file name of a schema, and position within the schema file.
+ *
+ * @return list of [[LocationInSchemaFile]]'s related to this diagnostic.
+ */
+ public abstract List<LocationInSchemaFile> getLocationsInSchemaFiles();
+
+ /**
+ * Determine if a diagnostic object represents an error or something less serious.
+ *
+ * @return true if it represents an error, false otherwise
+ */
+ public abstract boolean isError();
+
+
+ public abstract boolean isValidation();
+
+ /**
+ * Get the cause of that cause this diagnostic
+ *
+ * @return the exception that caused the diagnostic
+ */
+ public abstract Optional<Throwable> getSomeCause();
+
+ /**
+ * Get the message that caused this diagnostic
+ *
+ * @return the message that caused the diagnostic
+ */
+ public abstract Optional<String> getSomeMessage();
Review Comment:
Looks like they weren't being used anyways
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2193114664
##########
daffodil-core/src/main/java/org/apache/daffodil/api/InfosetItem.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.net.URI;
+
+import com.ibm.icu.util.Calendar;
Review Comment:
So apparently Java doesn't have a flag to automatically warn about unused imports. So instead javac ignores unused imports. I did fix unused imports in the java code where I could tho using the IDE
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2198554814
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/debugger/InteractiveTraceDebuggerRunner.scala:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.runtime1.debugger
+
+import org.apache.daffodil.api.debugger.InteractiveDebuggerRunner
+
+/*
+ * The below class is empty and is not ever actually used. It is just a place
+ * holder. Whenever the API uses it, it is translated to the appropriate
+ * Scala debugger runner. It is marked final so that it cannot be extended,
+ * since the Scala pattern matcher would still match and use the equivalent
+ * Scala trace runner and lose any added functionality. One must extend the
+ * DebuggerRunner to create their own debugger runner.
+ */ /*
+ * The below class is empty and is not ever actually used. It is just a place
+ * holder. Whenever the API uses it, it is translated to the appropriate
+ * Scala debugger runner. It is marked final so that it cannot be extended,
+ * since the Scala pattern matcher would still match and use the equivalent
+ * Scala trace runner and lose any added functionality. One must extend the
+ * DebuggerRunner to create their own debugger runner.
+ */
+
+/**
+ * `DebuggerRunner` that writes verbose information about parsing to stdout
+ */
+final class InteractiveTraceDebuggerRunner extends InteractiveDebuggerRunner {
Review Comment:
Hmm, good catch. This has been replaced by the api.debugger.TraceDebuggerRunner interface
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2195728433
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Result.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import java.util.List;
+
+/**
+ * Interface for Parse and Unparse results
+ */
+public interface Result {
+ State resultState();
Review Comment:
Hmm, we use it in a bunch of tests and the tdml processor which use the public api, should I look into using the internal api in those cases instead?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070472940
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -31,17 +31,21 @@ import scala.collection.mutable.ArrayBuffer
import scala.util.Try
import scala.xml.Node
+import org.apache.daffodil.api.compiler.{ ProcessorFactory => JProcessorFactory }
+import org.apache.daffodil.api.exceptions.{ InvalidParserException => JInvalidParserException }
+import org.apache.daffodil.api.{ CodeGenerator => JCodeGenerator }
+import org.apache.daffodil.api.{ DataProcessor => JDataProcessor }
Review Comment:
> we needed to rename them and J seemed like the best option. Any suggestions?
One alternative could be to just `import org.apache.daffodil.api` and then classes could do something like
```scala
class DataProcessor extends api.DataProcessor {
...
}
```
Possibly removes some imports and makes also makes clear that thigns are implementing the API interface.
> Re 1, we are also integrating the new JAVA API into our current backend for compatibility @stevedlawrence is that right?
Yes, we will now just have a single API that works in both Java and Scala. Forease of use, it probably doesn't want to reference any Scala-types. This should make it easy to use for Java people, while still being easy to use for scala users.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074177153
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/validation/ValidatorsFactory.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.validation;
+
+import com.typesafe.config.Config;
+import org.apache.daffodil.lib.validation.DaffodilLimitedValidator$;
+import org.apache.daffodil.lib.validation.NoValidator$;
+import org.apache.daffodil.lib.validation.XercesValidatorFactory;
+
+import scala.collection.immutable.Seq;
+
+import java.net.URI;
+import java.util.Collections;
+
+public class ValidatorsFactory {
Review Comment:
created DAFFODIL-2996
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#issuecomment-2846094649
One more comment.
Some of the places I suggested renaming, like ValidationException to ValidationError ,... we may not want to do that because existing code
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2208232065
##########
daffodil-schematron/src/main/scala/org/apache/daffodil/validation/schematron/SchematronValidatorFactory.scala:
##########
@@ -75,7 +81,7 @@ object SchematronValidatorFactory {
schematron: InputStream,
schematronID: String,
srcfmt: SchSource,
- svrlPath: Option[Path]
+ svrlPath: Option[URI]
): SchematronValidator = {
val factory = new TransformerFactoryImpl()
factory.setURIResolver(DFDLCatalogResolver.get)
Review Comment:
DAFFODIL-3009
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2211491771
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -229,25 +153,24 @@ object DFDL {
* can be used to get the result and any diagnostics.
*/
class DaffodilUnparseErrorSAXException(unparseResult: UnparseResult)
- extends SAXException(unparseResult.getDiagnostics.mkString("\n"))
+ extends api.exceptions.DaffodilUnparseErrorSAXException(
+ unparseResult.getDiagnostics.asScala.mkString("\n")
Review Comment:
Actually String.join doesn't work with Diagnostics as it expects a charsequence or Iterable of charsequences, which Diagnostics isn't
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070476017
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -31,17 +31,21 @@ import scala.collection.mutable.ArrayBuffer
import scala.util.Try
import scala.xml.Node
+import org.apache.daffodil.api.compiler.{ ProcessorFactory => JProcessorFactory }
+import org.apache.daffodil.api.exceptions.{ InvalidParserException => JInvalidParserException }
+import org.apache.daffodil.api.{ CodeGenerator => JCodeGenerator }
+import org.apache.daffodil.api.{ DataProcessor => JDataProcessor }
Review Comment:
Ok, so the Java version of the InfosetInputter doesn't contain all the actual functionality, it just exposes the necessary function calls for external use? I wasn't sure if we had actually fully reimplemented things like the InfosetInputter in Java, in which case I didn't see the need for a J prefix. If we aren't doing that and there are separate Scala versions of these classes, then the J prefix makes sense.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070457648
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -31,17 +31,21 @@ import scala.collection.mutable.ArrayBuffer
import scala.util.Try
import scala.xml.Node
+import org.apache.daffodil.api.compiler.{ ProcessorFactory => JProcessorFactory }
+import org.apache.daffodil.api.exceptions.{ InvalidParserException => JInvalidParserException }
+import org.apache.daffodil.api.{ CodeGenerator => JCodeGenerator }
+import org.apache.daffodil.api.{ DataProcessor => JDataProcessor }
Review Comment:
Re 2, The J prefix is because alot of the classes share the same name as their Scala counterpart ex InfosetInputter, DataProcessor, so we needed to rename them and J seemed like the best option. Any suggestions?
Re 1, we are also integrating the new JAVA API into our current backend for compatibility @stevedlawrence is that right?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070617855
##########
build.sbt:
##########
@@ -28,20 +28,13 @@ lazy val genTunablesDoc = taskKey[Seq[File]]("Generate tunables doc from dafext.
lazy val daffodil = project
.in(file("."))
- .enablePlugins(JavaUnidocPlugin, ScalaUnidocPlugin)
+ .enablePlugins(ScalaUnidocPlugin)
.aggregate(
cli,
codeGenC,
- core,
- io,
- japi,
- lib,
Review Comment:
Cool, I guess there is no good performance reason to split.
> I don't understand why [daffodil-io](https://issues.apache.org/jira/browse/DAFFODIL-io) has to be blended in here
I think there is an argument daffodil-io does actually want to be in the daffodil-runtime jar. Although none of it depends on runtime1, most of the classes are things that are only used by runtime1 and are designed specifically to work well with runtime1.
> I suggest this big merged module NOT be named "runtime1". But rather "core" is the name we should carry forward.
I was going to the suggest the same thing. The only benefit I could see of keeping it daffodil-runtime1 is that if we ever do split it out into something like daffodil-compiler, daffodil-runtime1, daffdoild-codegen, with real modularity then users wouldn't have to change their dependencies from daffodil-core back to daffodil-runtime1. But I don't see that happening anytime soon, since as you point out it's probably a huge effort. So a single daffodil-core jar seems reasonable to me.
Another thought, is there value in creating a daffodil-charset and daffodil-layer directory/jar that contains the interfaces for building custom layers and charsets, similar to what we have with daffodil-udf? Or maybe we have a daffodil-plugins that contains all the udf, layer, and charset interfaces? Or do we want to merge daffodil-udf into daffodil-core so no matter what you're doing, you just depend on daffodil-core? I have no strong preference for either approach, but it seems a inconsistent to have udf be it's own jar and other plugins be part of daffodil-core.
Also, I don't know if the Charset API is very java friendly (DAFFODIL-2894), that might be another task to do for 4.0.0, but doesn't need to be done as this PR.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2182636560
##########
DEVELOP.md:
##########
@@ -225,18 +225,9 @@ daffodil/
│ ├── build.sbt - Tells sbt how to build CLI and installers
│ └── src/ - Contains CLI source code, tests, scripts, etc.
├── daffodil-codegen-c/ - Contains Daffodil's C code generator
-├── daffodil-core/ - Contains Daffodil's schema compiler
-├── daffodil-io/ - Contains Daffodil's input/output/charsets
-├── daffodil-japi/
-│ ├── build.sbt - Tells sbt how to generate javadocs
-│ └── src/ - Contains Daffodil's Java API
-├── daffodil-lib/ - Contains Daffodil's utilities
├── daffodil-macro-lib/ - Defines Daffodil's Scala 2.x macros
├── daffodil-propgen/ - Contains generators to generate more source code
-├── daffodil-runtime1/ - Contains Daffodil's Scala parser
-├── daffodil-runtime1-layers/ - Contains Daffodil's Scala layers
-├── daffodil-runtime1-unparser/ - Contains Daffodil's Scala unparser
-├── daffodil-sapi/ - Contains Daffodil's Scala API
+├── daffodil-core/ - Contains Daffodil's input/output/charsets, Scala parser, schema compiler, utilities, layers, unparsers and the API
Review Comment:
Alignment of the details looks off
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -161,23 +164,33 @@ class CLIConf(arguments: Array[String], stdout: PrintStream, stderr: PrintStream
override def argFormat(name: String): String = "[" + name + "]"
}
- implicit def validateConverter: ValueConverter[ValidationMode.Type] =
- singleArgConverter[ValidationMode.Type]((s: String) => {
+ def validateConverter(
+ schema: ScallopOption[URISchemaSource]
+ ): ValueConverter[api.validation.Validator] =
+ singleArgConverter[api.validation.Validator]((s: String) => {
import ValidatorPatterns._
+ val defaults = new Properties()
+ val rootSchema = schema.toOption match {
+ case Some(sch) => sch.uri.toString
+ case _ => ""
+ }
+ defaults.setProperty(Validator.rootSchemaKey, rootSchema)
Review Comment:
Do we want to set this only if the property has a value?
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -1213,9 +1242,10 @@ class Main(
validate
)
}
- }.map {
- _.withExternalVariables(combineExternalVariables(parseOpts.vars, optDafConfig))
- }.map { _.withValidationMode(validate) }
+ }.map { p =>
+ p.asInstanceOf[DFDL.DataProcessor]
Review Comment:
Is our public API missing the ability to set external variables? Feels like this `asInstanceOf` (and similar ones below) shouldn't be needed to set vars.
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/DaffodilCExamplesGenerator.scala:
##########
@@ -33,7 +35,7 @@ object DaffodilCExamplesGenerator {
exampleDir: os.Path
): Unit = {
// Generate example code from the sample schema
- val pf = Compiler().compileFile(schemaFile.toIO, optRootName)
+ val pf = Compiler().compileFile(schemaFile.toIO, optRootName).asInstanceOf[ProcessorFactory]
Review Comment:
What is isInstanceOf needed for here? I would think anything the example code generator could do everything via the public api.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Daffodil.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+/**
+ * Factory object to create a {@link org.apache.daffodil.api.Compiler} and set global configurations
+ */
+public class Daffodil {
+
+ private Daffodil() {
+ }
+
+ /**
+ * Create a new object used to compiled DFDL schemas
Review Comment:
s/compiled/compile
##########
daffodil-core/src/main/java/org/apache/daffodil/api/DataProcessor.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import org.apache.daffodil.api.debugger.Debugger;
+import org.apache.daffodil.api.debugger.DebuggerRunner;
+import org.apache.daffodil.api.debugger.InteractiveDebuggerRunner;
+import org.apache.daffodil.api.debugger.InteractiveDebuggerRunnerFactory;
+import org.apache.daffodil.runtime1.debugger.InteractiveTraceDebuggerRunner;
+import org.apache.daffodil.api.exceptions.ExternalVariableException;
+import org.apache.daffodil.api.infoset.InfosetInputter;
+import org.apache.daffodil.api.infoset.InfosetOutputter;
+import org.apache.daffodil.api.validation.Validator;
+import org.apache.daffodil.core.dsom.ExpressionCompilers$;
+import org.apache.daffodil.runtime1.debugger.InteractiveDebugger;
+
+
+import java.io.File;
+import java.io.Serializable;
+import java.net.URI;
+import java.nio.channels.WritableByteChannel;
+import java.util.Map;
+
+/**
+ * Compiled version of a DFDL Schema, used to parse data and get the DFDL infoset
+ */
+public interface DataProcessor extends WithDiagnostics, Serializable {
+ /**
+ * Obtain a new {@link DataProcessor} instance with debugging enabled or disabled.
+ * <p>
+ * Before enabling, {@link DataProcessor#withDebugger} or {@link DataProcessor#withDebuggerRunner} must be called to obtain
+ * a {@link DataProcessor} with a non-null debugger.
+ *
+ * @param flag true to enable debugging, false to disabled
+ */
+ DataProcessor withDebugging(boolean flag);
+
+ /**
+ * Obtain a new {@link DataProcessor} with a specified debugger runner.
+ *
+ * @param dr debugger runner
+ */
+ default DataProcessor withDebuggerRunner(DebuggerRunner dr) {
+ Debugger dbg = null;
+ InteractiveDebuggerRunner runner;
+ if (dr instanceof InteractiveTraceDebuggerRunner) {
+ runner = new org.apache.daffodil.runtime1.debugger.TraceDebuggerRunner(System.out);
+ } else if (dr != null) {
+ runner = InteractiveDebuggerRunnerFactory.get(dr);
+ } else {
+ runner = null;
+ }
+
+ if (runner != null) {
+ dbg = new InteractiveDebugger(runner, ExpressionCompilers$.MODULE$);
+ }
+ return withDebugger(dbg);
+ }
+
+ /**
+ * Obtain a new {@link DataProcessor} with a specified debugger.
+ *
+ * @param dbg debugger
+ */
+ DataProcessor withDebugger(Debugger dbg);
+
+ DataProcessor withValidator(Validator validator);
Review Comment:
Need javadoc for withValidator. Mentioned elsewhere, but does this support null, or does one need to create a NoValidator?
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Result.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import java.util.List;
+
+/**
+ * Interface for Parse and Unparse results
+ */
+public interface Result {
+ State resultState();
+
+ List<Diagnostic> diagnostics = null;
+
+ List<Diagnostic> getDiagnostics();
+
+ void addDiagnostic(Diagnostic diagnostic);
+
+ boolean isError();
+
+ boolean isProcessingError();
+
+ boolean isValidationError();
+}
Review Comment:
Just noticed all these new files end without a new line. There should be a flag in your IDE to enable this. This is usually the preferred configuration.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/State.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import org.apache.daffodil.runtime1.processors.ProcessorResult;
+
+import java.util.List;
+
+/**
+ * Interface for Parse and Unparse states
+ */
+public interface State {
+ ProcessorResult processorStatus();
+
+ boolean validationStatus();
Review Comment:
Does State and these two functions really want to be part of the public API? State feels like an internal concept that shouldn't really be made public. Internally we use Parse State and Unparse state to keep track of thigns but I'm not sure this wants to be public. Are they actually needed by API users?
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Daffodil.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+/**
+ * Factory object to create a {@link org.apache.daffodil.api.Compiler} and set global configurations
+ */
+public class Daffodil {
+
+ private Daffodil() {
+ }
+
+ /**
+ * Create a new object used to compiled DFDL schemas
+ *
+ * @return new object to compile DFDL schemas
+ */
+ public static org.apache.daffodil.core.compiler.Compiler compiler() {
Review Comment:
I think the return type of this static funtion wants to be `org.apache.daffodil.api.Compiler`. And it just happens return an internal instance of the Compile that implements the public interface.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/validation/XercesValidator.scala:
##########
@@ -44,25 +36,38 @@ import org.xml.sax.SAXParseException
*
* SPI service name: xerces
* Configuration requirements:
- * - xerces: List[String] - schema file names
+ * - daffodil.rootSchema=schema_file_uri_string
+ * - xerces=schema_file_uri_string
Review Comment:
These aren't both require right, as long as it has one of them. And xeres will be preferred over daffodil.rootSchema if they are both defined? This should be included in the documentation.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/Implicits.scala:
##########
@@ -44,6 +47,29 @@ object Implicits {
): BufferedInputStream =
new BufferedInputStream(bais)
+ implicit def JListToSeq[T](l: java.util.List[T]): Seq[T] = {
+ l.asScala.toSeq
+ }
+ implicit def SeqToJList[T](s: Seq[T]): java.util.List[T] = {
+ s.asJava
+ }
+
+ implicit def OptionalToOption[T](j: Optional[T]): Option[T] = {
+ j.toScala
+ }
+
+ implicit def OptionToOptional[T](s: Option[T]): Optional[T] = {
+ s.toJava
+ }
+
+ implicit def JMapToSMap[K, V](j: java.util.Map[K, V]): Map[K, V] = {
+ j.asScala.toMap
+ }
+
+ implicit def SMapToJMap[K, V](s: Map[K, V]): java.util.Map[K, V] = {
+ s.asJava
+ }
Review Comment:
We may want to open a bug to remove these in the future, or at least make them no longer implicits. I think the Scala devs and community have come to the conclusion that implicits are generally not a good idea.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/CodeGenerator.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+/**
+ * Source code generation and compilation is performed with a language-specific {@link CodeGenerator}},
+ * which must be interrogated for diagnostics to see if each call was successful or not.
+ */
+public interface CodeGenerator extends WithDiagnostics {
+ /**
+ * Generates language-specific code from a DFDL schema to parse or unparse data
+ *
+ * @param outputDir output directory in which to create code directory (codeDir)
+ * @return path of newly created directory (codeDir) containing generated code
+ */
+ os.Path generateCode(String outputDir);
Review Comment:
I'm not sure we should return `os.Path` anymore. That comes from the [os-lib](https://github.com/com-lihaoyi/os-lib), which is scala based, and is potentially difficult for Java users to use. This should probably either return a `java.nio.file.Path` or `java.io.File`, same with the below functions.
Internally, it's fine if our built-in code generators convert those to/from os.Path, but the interface itself probably wants to be more java friendly.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -161,23 +164,33 @@ class CLIConf(arguments: Array[String], stdout: PrintStream, stderr: PrintStream
override def argFormat(name: String): String = "[" + name + "]"
}
- implicit def validateConverter: ValueConverter[ValidationMode.Type] =
- singleArgConverter[ValidationMode.Type]((s: String) => {
+ def validateConverter(
+ schema: ScallopOption[URISchemaSource]
+ ): ValueConverter[api.validation.Validator] =
+ singleArgConverter[api.validation.Validator]((s: String) => {
import ValidatorPatterns._
+ val defaults = new Properties()
+ val rootSchema = schema.toOption match {
+ case Some(sch) => sch.uri.toString
+ case _ => ""
+ }
+ defaults.setProperty(Validator.rootSchemaKey, rootSchema)
+ val props = new Properties(defaults)
s match {
- case "on" => ValidationMode.Full
- case "limited" => ValidationMode.Limited
- case "off" => ValidationMode.Off
- case DefaultArgPattern(name, arg) if Validators.isRegistered(name) =>
- val config =
- if (arg.endsWith(".conf")) ConfigFactory.parseFile(new File(arg))
- else ConfigFactory.parseString(s"$name=$arg")
- ValidationMode.Custom(Validators.get(name).make(config))
- case NoArgsPattern(name) if Validators.isRegistered(name) =>
- ValidationMode.Custom(Validators.get(name).make(ConfigFactory.empty))
+ case DefaultArgPattern(name, arg) if validators.isRegistered(name) =>
+ if (Seq(".conf", ".properties").exists(arg.endsWith)) {
+ val is = new FileInputStream(arg)
+ props.load(is)
+ } else {
+ val uss = fileResourceToURI(arg)
+ props.setProperty(name, uss.uri.toString)
+ }
+ validators.get(name).make(props)
+ case NoArgsPattern(name) if validators.isRegistered(name) =>
+ validators.get(name).make(defaults)
case _ =>
throw new Exception(
- "Unrecognized ValidationMode %s. Must be 'on', 'limited', 'off', or name of spi validator."
+ "Unrecognized Validator name %s. Must be 'xerces', 'limited', 'off', or name of spi validator."
Review Comment:
I think there's a bug somewhere to rename "limited" as well, I think the suggestion was "daffodil"? We don't have to do it as part of this PR, but since we are changing everythign nows the time to do renames.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/validation/Validator.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.validation;
+
+import java.io.InputStream;
+
+/**
+ * Implement this trait to provide custom validation logic
+ * <p>
+ * The Validator implementations must be thread safe
+ * <p>
+ * See {@link org.apache.daffodil.lib.validation.XercesValidator} for example of using ThreadLocal for thread safety
Review Comment:
I think this documentation should probably say something that validation errors should be reported by calling the appropriate functions on the ValidationHandler.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -445,8 +465,8 @@ class CLIConf(arguments: Array[String], stdout: PrintStream, stderr: PrintStream
}
validateOpt(parser, validate) {
- case (Some(_), Some(ValidationMode.Full)) =>
- Left("The validation mode must be 'limited' or 'off' when using a saved parser.")
+ case (Some(_), Some(null)) =>
Review Comment:
How does this `null` work? That doesn't seem like it's checking that the mode is full/xerces?
Though, I think we can remove this validation entirely, and we can now allow full validation with saved parsers. The only restriction is that when a user creates a Validator.Xerces, you must ensure the properties includes the path of the schema to use for validation--Daffodil no longer figures it out. I the new validators enforce that, so this can probably be removed?
This can be done in a separate PR.
##########
build.sbt:
##########
@@ -456,59 +409,39 @@ lazy val ratSettings = Seq(
def apiDocSourceFilter(sources: Seq[File]): Seq[File] = sources.filter { source =>
val str = source.toString
val oad = "/org/apache/daffodil"
- lazy val excludedForJAPI =
- str.contains(oad + "/japi/") && {
- // Some things are excluded from the JAPI javadoc because they are internal, non-API
- // These cannot be excluded from SAPI because symbols-not-found issues with scaladoc.
- str.contains("$") || str.contains("packageprivate")
- }
lazy val included = {
str.contains(oad + "/udf/") ||
- str.contains(oad + "/sapi/") ||
- str.contains(oad + "/japi/") ||
+ str.contains(oad + "/api/") ||
str.contains(oad + "/runtime1/layers/api/")
//
// There are files in runtime1/api that are NOT part of the public, supported API.
// I tried to include all of runtime1/api, and exclude those files, but could not
// get that to work, so now we include individually each file that is part of the
// published runtime1 API
Review Comment:
Can this comment be removed? With the layer API converted to Java, are there still parts of it that are not part of the public API?
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/InfosetTypes.scala:
##########
@@ -711,7 +707,7 @@ case class EXIInfosetHandler(dataProcessor: DataProcessor, exiFactory: EXIFactor
new InfosetParseResult(pr)
}
- def unparse(data: AnyRef, output: DFDL.Output): UnparseResult = {
+ def unparse(data: AnyRef, output: DFDL.Output): api.UnparseResult = {
Review Comment:
Ther's a lot of api.* in here. Can we update the imports to import these specifically? Or was the intention to make it clear which things are part of the api when being used?
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -161,23 +164,33 @@ class CLIConf(arguments: Array[String], stdout: PrintStream, stderr: PrintStream
override def argFormat(name: String): String = "[" + name + "]"
}
- implicit def validateConverter: ValueConverter[ValidationMode.Type] =
- singleArgConverter[ValidationMode.Type]((s: String) => {
+ def validateConverter(
+ schema: ScallopOption[URISchemaSource]
+ ): ValueConverter[api.validation.Validator] =
+ singleArgConverter[api.validation.Validator]((s: String) => {
import ValidatorPatterns._
+ val defaults = new Properties()
+ val rootSchema = schema.toOption match {
+ case Some(sch) => sch.uri.toString
+ case _ => ""
+ }
+ defaults.setProperty(Validator.rootSchemaKey, rootSchema)
+ val props = new Properties(defaults)
s match {
- case "on" => ValidationMode.Full
- case "limited" => ValidationMode.Limited
- case "off" => ValidationMode.Off
- case DefaultArgPattern(name, arg) if Validators.isRegistered(name) =>
- val config =
- if (arg.endsWith(".conf")) ConfigFactory.parseFile(new File(arg))
- else ConfigFactory.parseString(s"$name=$arg")
- ValidationMode.Custom(Validators.get(name).make(config))
- case NoArgsPattern(name) if Validators.isRegistered(name) =>
- ValidationMode.Custom(Validators.get(name).make(ConfigFactory.empty))
+ case DefaultArgPattern(name, arg) if validators.isRegistered(name) =>
+ if (Seq(".conf", ".properties").exists(arg.endsWith)) {
+ val is = new FileInputStream(arg)
+ props.load(is)
+ } else {
+ val uss = fileResourceToURI(arg)
+ props.setProperty(name, uss.uri.toString)
+ }
+ validators.get(name).make(props)
+ case NoArgsPattern(name) if validators.isRegistered(name) =>
+ validators.get(name).make(defaults)
Review Comment:
For consistency, it might make sense to use `props` here, even though props will just always refer to defaults since it doesn't provide anything new. That way we ever do add something to props it will just work as expected.
##########
build.sbt:
##########
@@ -456,59 +409,39 @@ lazy val ratSettings = Seq(
def apiDocSourceFilter(sources: Seq[File]): Seq[File] = sources.filter { source =>
val str = source.toString
val oad = "/org/apache/daffodil"
- lazy val excludedForJAPI =
- str.contains(oad + "/japi/") && {
- // Some things are excluded from the JAPI javadoc because they are internal, non-API
- // These cannot be excluded from SAPI because symbols-not-found issues with scaladoc.
- str.contains("$") || str.contains("packageprivate")
- }
lazy val included = {
str.contains(oad + "/udf/") ||
- str.contains(oad + "/sapi/") ||
- str.contains(oad + "/japi/") ||
+ str.contains(oad + "/api/") ||
str.contains(oad + "/runtime1/layers/api/")
//
// There are files in runtime1/api that are NOT part of the public, supported API.
// I tried to include all of runtime1/api, and exclude those files, but could not
// get that to work, so now we include individually each file that is part of the
// published runtime1 API
- //
- // NOTE: Commented out for now. genjavadoc doesn't handle the traits in
- // these files, so for now these are undocumented.
- //
- // FIXME: DAFFODIL-2902
- // str.contains(oad + "/runtime1/api/DFDLPrimType") ||
- // str.contains(oad + "/runtime1/api/Infoset") ||
- // str.contains(oad + "/runtime1/api/Metadata")
}
- val res = included && !excludedForJAPI
+ val res = included
res
}
lazy val unidocSettings =
Seq(
- ScalaUnidoc / unidoc / unidocProjectFilter :=
- inProjects(sapi, udf, runtime1),
- ScalaUnidoc / unidoc / scalacOptions := Seq(
Review Comment:
We might want to keep this scalacOptions around. It won't be needed as long as we always generate javadocs, buy if we ever do the thing mentioned in another comment where we temporarily add a dummy .scala file to generate scaladocs we will want this config.
##########
build.sbt:
##########
@@ -75,45 +68,22 @@ lazy val slf4jLogger = Project("daffodil-slf4j-logger", file("daffodil-slf4j-log
.settings(commonSettings)
.settings(libraryDependencies ++= Dependencies.slf4jAPI)
-lazy val lib = Project("daffodil-lib", file("daffodil-lib"))
- .dependsOn(macroLib % "compile-internal, test-internal", slf4jLogger % "test")
- .settings(commonSettings, libManagedSettings, usesMacros)
-
-lazy val io = Project("daffodil-io", file("daffodil-io"))
- .dependsOn(lib, macroLib % "compile-internal, test-internal", slf4jLogger % "test")
- .settings(commonSettings, usesMacros)
-
-lazy val runtime1 = Project("daffodil-runtime1", file("daffodil-runtime1"))
- .enablePlugins(GenJavadocPlugin)
- .settings(Dependencies.genjavadocVersion) // converts scaladoc to javadoc
+lazy val core = Project("daffodil-core", file("daffodil-core"))
.dependsOn(
- io,
- lib % "compile-internal, test->test",
udf,
macroLib % "compile-internal, test-internal",
slf4jLogger % "test"
)
- .settings(commonSettings, usesMacros)
-
-lazy val runtime1Unparser =
- Project("daffodil-runtime1-unparser", file("daffodil-runtime1-unparser"))
- .dependsOn(
- runtime1,
- lib % "test->test",
- runtime1 % "test->test",
- runtime1Layers,
- slf4jLogger % "test"
- )
- .settings(commonSettings)
-
-lazy val runtime1Layers = Project("daffodil-runtime1-layers", file("daffodil-runtime1-layers"))
- .dependsOn(runtime1, lib % "test->test", slf4jLogger % "test")
- .settings(commonSettings)
+ .settings(commonSettings, libManagedSettings, usesMacros)
Review Comment:
libManaged is potentially confusing now since there's no longer a `lib` jar. It looks like everything in libManagedSettings is related to all of our generators. Maybe rename it `generatorSettings` so it isn't explicitly tied to a specific project?
##########
build.sbt:
##########
@@ -456,59 +409,39 @@ lazy val ratSettings = Seq(
def apiDocSourceFilter(sources: Seq[File]): Seq[File] = sources.filter { source =>
val str = source.toString
val oad = "/org/apache/daffodil"
- lazy val excludedForJAPI =
- str.contains(oad + "/japi/") && {
- // Some things are excluded from the JAPI javadoc because they are internal, non-API
- // These cannot be excluded from SAPI because symbols-not-found issues with scaladoc.
- str.contains("$") || str.contains("packageprivate")
- }
lazy val included = {
Review Comment:
I wonder if we can simplify this logic? I think we've made a decision that anything that is going to be public API *must* be written in java. And I think the opposite is true, that anything written in Java is officially public API? So could we say anything in src/main/java will be included? Then this essentially becomes something like
```scala
def apiDocSourceFilter(sources: Seq[File]: Seq[File] = sources.filter { source =>
source.startsWith((Compile/javaSource).value)
}
```
Note only is this simpler, it kindof self documents that everything in src/main/java is public API. And if we want something written in Scala to be public API then we need to rewrite it in Java.
I think the only questions is if there is anything in src/main/java that shouldn't be public? From what I can see, the only thing that the above logic include that isn't already included is `src/main/java/util/SimpleNamedServiceLoader.java`. Was this was written in Java because it does need to be part of the public API?
##########
build.sbt:
##########
@@ -456,59 +409,39 @@ lazy val ratSettings = Seq(
def apiDocSourceFilter(sources: Seq[File]): Seq[File] = sources.filter { source =>
val str = source.toString
val oad = "/org/apache/daffodil"
- lazy val excludedForJAPI =
- str.contains(oad + "/japi/") && {
- // Some things are excluded from the JAPI javadoc because they are internal, non-API
- // These cannot be excluded from SAPI because symbols-not-found issues with scaladoc.
- str.contains("$") || str.contains("packageprivate")
- }
lazy val included = {
str.contains(oad + "/udf/") ||
- str.contains(oad + "/sapi/") ||
- str.contains(oad + "/japi/") ||
+ str.contains(oad + "/api/") ||
str.contains(oad + "/runtime1/layers/api/")
//
// There are files in runtime1/api that are NOT part of the public, supported API.
// I tried to include all of runtime1/api, and exclude those files, but could not
// get that to work, so now we include individually each file that is part of the
// published runtime1 API
- //
- // NOTE: Commented out for now. genjavadoc doesn't handle the traits in
- // these files, so for now these are undocumented.
- //
- // FIXME: DAFFODIL-2902
- // str.contains(oad + "/runtime1/api/DFDLPrimType") ||
- // str.contains(oad + "/runtime1/api/Infoset") ||
- // str.contains(oad + "/runtime1/api/Metadata")
}
- val res = included && !excludedForJAPI
+ val res = included
res
}
lazy val unidocSettings =
Review Comment:
It's maybe worth a discussion, or maybe a comment about how scala unidoc works. Maybe something like:
> Previously we used JavaUnidoc to generate javadoc and ScalaUnidoc to generate scaladoc. But JavaUnidoc requires the genjavadoc plugin, which does not work on Scala 3. So we can only use ScalaUnidoc. But this doesn't necessarily mean we must generate scaladoc. ScalaUnidoc will generate scaladoc if there is at least on .scala file, but uses javadoc if all source files are .java. Our public API is 100% java, so we add a filter to only include files in src/main/java so we generate javadoc. This does mean scaladoc is no longer available, but javadoc can be understood by both Scala and Java devs.
If we really want scaladoc in addition to javadoc, I think what we need to do is modify our [Update Docs workflow](https://github.com/apache/daffodil-site/blob/main/.github/workflows/update-docs.yml#L96-L99) to build documentation twice. Once like normal, and again but with a dummy scala file added somewhere that provides no new API functions, but by existing forces it to build scaladocs. Note that even if we don't do this and just have javadocs, that workflow does need to be updated to not expect the scaladocs. Should maybe open a ticket somewhere to not forget.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Daffodil.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+/**
+ * Factory object to create a {@link org.apache.daffodil.api.Compiler} and set global configurations
Review Comment:
Should we remove the "and set global configurations"? It doesn't look there are any global configs it can set. All it can do is create a compiler.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -1639,7 +1671,7 @@ class Main(
case Some(conf.save) => {
val saveOpts = conf.save
- val validate = ValidationMode.Off
+ val validate = null
Review Comment:
Should this be `NoValidator`? What are the rules on calling `withValidator`? Do we allow null to mean no validator, but also allow an explicit `NoValidator`?
Doesn't seem unreasonable, but it's potentially confusing to have two different ways to turn off validation. I kindof prefer NoValidator, but it also seems a bit silly to require users to make a NoValidator. But I guess if they don't want validation they can just not call withValidator. So feels like withValidator doesn't want to accept null.
##########
daffodil-cli/src/test/scala/org/apache/daffodil/cli/cliTest/TestCLIParsing.scala:
##########
@@ -460,12 +460,28 @@ class TestCLIParsing {
"daffodil-test/src/test/resources/org/apache/daffodil/section06/entities/charClassEntities.dfdl.xsd"
)
- runCLI(args"parse -s $schema -r matrix --validate on") { cli =>
+ runCLI(args"parse -s $schema -r matrix --validate xerces") { cli =>
cli.sendLine("0,1,2", inputDone = true)
cli.expect("<tns:cell>2</tns:cell>")
}(ExitCode.LeftOverData)
}
+ @Test def test_CLI_Parsing_SimpleSchema_basicTest_validationOnDifferentValidatingSchema()
+ : Unit = {
+ val schema = path(
+ "daffodil-test/src/test/resources/org/apache/daffodil/section06/entities/charClassEntities.dfdl.xsd"
+ )
+ val validatingSchema = path(
+ "daffodil-test/src/test/resources/org/apache/daffodil/section06/entities/charClassEntities_enforced.dfdl.xsd"
+ )
+
+ runCLI(args"parse -s $schema -r matrix --validate xerces=$validatingSchema") { cli =>
Review Comment:
Nice test! This could be a very useful feature, especially for things like stringAsXml where the infoset doesn't validate against the dfdl schema.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/debugger/CLIDebuggerRunner.scala:
##########
@@ -53,6 +54,12 @@ class CLIDebuggerRunner(cmdsIter: Iterator[String], in: InputStream, out: PrintS
var reader: Option[LineReader] = None
+ def init(): Unit = {
+ throw new IllegalArgumentException(
+ "Must pass in InteractiveDebugger to the init function for CLIDebuggerRunner"
Review Comment:
I don't understand this, what is this for?
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Compiler.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import org.apache.daffodil.api.exceptions.InvalidParserException;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.URI;
+import java.nio.channels.ReadableByteChannel;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * Compile DFDL schemas into {@link ProcessorFactory}'s or
+ * reload saved parsers into {@link org.apache.daffodil.api.DataProcessor}'s.
+ * <p>
+ * Do not use the Compiler constructor to create a Compiler. Instead, use {@code Daffodil#compiler()}.
Review Comment:
I think we can remove this comment about the Compiler constructor.
There is no longer any public `Compiler` class in the API, it's only an interface, so you couldn't create a compiler this way if you wanted, unless you used some internal stuff, but we can assume people aren't doing that, or if they are they know what they are doing.
I think we can just say something like "Use Daffodil#compiler() to get a compiler instance"
##########
daffodil-core/src/main/java/org/apache/daffodil/api/InfosetSimpleElement.java:
##########
@@ -100,228 +48,192 @@
/**
* @return the value of this simple element as a Scala AnyRef, which is
- * equivalent to a Java Object.
+ * equivalent to a Java Object.
Review Comment:
Remove this function, our API does not need to reference scala specific things.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/ComplexElementMetadata.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.daffodil.api;
+
+/**
+ * Access to metadata values exclusive to elements of complex type.
+ */
+public interface ComplexElementMetadata extends ElementMetadata {
+ // no specific methods
+}
Review Comment:
Does it make sense for the metadata and metadata handler related stuff to be in different subpackage?
If I was just looking at all the classes directly in org.apache.daffodil.api, I probably would only care about things like Daffodil, Compiler, PrcoessorFatory, Validator, and a few others--the real common stuff. All the metadata classes are really only used MetadataHandlers that most people will not care about, so it might want to be at the top level.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Diagnostic.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * Class containing diagnostic information. This does not need to
+ * be implemented by the user.
+ */
+public abstract class Diagnostic extends Exception {
+
+ public Diagnostic(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) {
+ super(message, cause, enableSuppression, writableStackTrace);
+ }
+
+ /**
+ * Get all diagnostic information, including mode name, schema context, and data location
+ * <p>
+ * Future versions of Daffodil will change getMessage to return only the message portion of a
+ * diagnostic, matching the behavior of getMessageOnly--users should call toString() instead.
Review Comment:
We don't have to change this in this PR, but we will probably want to make this change for 4.0.0 and remove getMessageOnly. I believe we have a bug for this.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Compiler.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import org.apache.daffodil.api.exceptions.InvalidParserException;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.URI;
+import java.nio.channels.ReadableByteChannel;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * Compile DFDL schemas into {@link ProcessorFactory}'s or
+ * reload saved parsers into {@link org.apache.daffodil.api.DataProcessor}'s.
Review Comment:
Do we need the org.apache.daffodi.api here? We don't need it for ProessorFactory or other things. Or is this just a weird javadoc oddity where sometimes it's needed?
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/infoset/InfosetInputter.scala:
##########
@@ -79,7 +65,7 @@ object InfosetInputterEventType {
* This is necessary so that the infoset inputter can resolve element
* name + namespace into the proper ERD based on the proper dynamic context.
*/
-abstract class InfosetInputter
+final class InfosetInputter(actualInputter: api.infoset.InfosetInputter)
Review Comment:
Maybe instead of actualInputter, call this delegate? So this InfosetInputter delegates to the API InfosetInputter? Just trying to think of something that doesn't cause confusing but is a bit shorter.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Diagnostic.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * Class containing diagnostic information. This does not need to
+ * be implemented by the user.
+ */
+public abstract class Diagnostic extends Exception {
+
+ public Diagnostic(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) {
+ super(message, cause, enableSuppression, writableStackTrace);
+ }
+
+ /**
+ * Get all diagnostic information, including mode name, schema context, and data location
+ * <p>
+ * Future versions of Daffodil will change getMessage to return only the message portion of a
+ * diagnostic, matching the behavior of getMessageOnly--users should call toString() instead.
+ *
+ * @return diagnostic message in string form
+ */
+ public abstract String getMessage();
+
+ /**
+ * Get only the message part of a diagnostic.
+ * <p>
+ * This does not include mode name, schema context, or data location information
+ *
+ * @return diagnostic message in string form
+ */
+ public abstract String getMessageOnly();
+
+ /**
+ * Get the diagnostic mode name
+ *
+ * @return diagnostic mode name in string form
+ */
+ public abstract String getModeName();
+
+ /**
+ * Get a string containing the mode name, message, schema location, and data location combined
+ * into a single string
+ *
+ * @return all diagnostic information as a string
+ */
+ @Override
+ public abstract String toString();
+
+
+ /**
+ * Get data location information relevant to this diagnostic object.
+ * <p>
+ * For example, this might be a file name, and position within the file.
+ *
+ * @return list of [[DataLocation]]'s related to this diagnostic
+ */
+ public abstract List<DataLocation> getDataLocations();
+
+ /**
+ * Get schema location information relevant to this diagnostic object.
+ * <p>
+ * For example, this might be a file name of a schema, and position within the schema file.
+ *
+ * @return list of [[LocationInSchemaFile]]'s related to this diagnostic.
+ */
+ public abstract List<LocationInSchemaFile> getLocationsInSchemaFiles();
+
+ /**
+ * Determine if a diagnostic object represents an error or something less serious.
+ *
+ * @return true if it represents an error, false otherwise
+ */
+ public abstract boolean isError();
+
+
+ public abstract boolean isValidation();
Review Comment:
Edit needs java doc. Maybe something along the lines of
> Determine if a diagnostic objecty represents a validation diagnostic. This could be a validation error or a validation warning depending on isError
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -106,19 +103,20 @@ object DataProcessor {
private class SerializableDataProcessor(
ssrd: SchemaSetRuntimeData,
tunables: DaffodilTunables,
- variableMap: VariableMap, // must be explicitly reset by save method
- validationMode: ValidationMode.Type // must be explicitly turned off by save method
- ) extends DataProcessor(ssrd, tunables, variableMap, validationMode) {
+ variableMap: VariableMap // must be explicitly reset by save method
+ ) extends DataProcessor(ssrd, tunables, variableMap) {
- override def withValidationMode(mode: ValidationMode.Type): DataProcessor = {
- if (mode == ValidationMode.Full) {
+ override def withValidator(validator: api.validation.Validator): api.DataProcessor = {
Review Comment:
Related to another comment about using validation mode == xerces with saved processors.
I think this can go away too. There's no reason to disallow full validation mode saved processor. But what it does mean is that users have to be able to create a XercesValidator and they need to provide the schema to use for that. I imagine in many cases users wn't be able to provide that shema, but we no longer need to restrict it.
This can be done in a separate PR related to supporting xerces validation with saved parsers.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/validation/XercesValidator.scala:
##########
@@ -44,25 +36,38 @@ import org.xml.sax.SAXParseException
*
* SPI service name: xerces
* Configuration requirements:
- * - xerces: List[String] - schema file names
+ * - daffodil.rootSchema=schema_file_uri_string
+ * - xerces=schema_file_uri_string
*/
-class XercesValidatorFactory extends ValidatorFactory {
+class XercesValidatorFactory extends api.validation.ValidatorFactory {
def name(): String = XercesValidator.name
- def make(config: Config): Validator = XercesValidatorFactory.makeValidator(config)
+
+ def make(config: Properties): api.validation.Validator =
+ XercesValidatorFactory.makeValidator(config)
}
object XercesValidatorFactory {
- def makeValidator(config: Config): Validator = {
- val schemaFiles =
- if (config.hasPath(XercesValidator.name))
- config.getStringList(XercesValidator.name).asScala
- else Seq.empty
- XercesValidator.fromFiles(schemaFiles.toSeq)
+ def makeValidator(uriString: String): api.validation.Validator = {
+ val config = XercesValidatorFactory.makeConfig(uriString)
+ makeValidator(config)
+ }
+
+ def makeValidator(config: Properties): api.validation.Validator = {
+ val schemaPath = config.getProperty(XercesValidator.name)
+ val schemaFile =
+ if (schemaPath != null) schemaPath
+ else config.getProperty(XercesValidator.ConfigKeys.rootSchema)
+ if (Misc.isNullOrBlank(schemaFile)) null
+ else {
+ val uri = new URI(schemaFile)
+ XercesValidator.fromURI(uri)
Review Comment:
Can fromURI fail? Does this check that the passed in XSD is valid so that make() will fail and throw the appropriate exception?
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceChildBases.scala:
##########
@@ -532,7 +532,7 @@ trait EndArrayChecksMixin {
// if we should do limited validation
val shouldValidate =
state.dataProc.isDefined &&
- state.dataProc.value.validationMode == ValidationMode.Limited
+ state.dataProc.value.validator == DaffodilLimitedValidator
Review Comment:
Use `eq` here
##########
daffodil-core/src/main/java/org/apache/daffodil/api/infoset/InfosetInputter.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.infoset;
+
+import org.apache.daffodil.runtime1.dpath.NodeInfo;
+
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * Abstract class used to determine how the infoset representation should be
+ * input from a call to {@code DataProcessor.unparse(input* DataProcessor.unparse)}. This uses a Cursor API, such
+ * that each call to advance/inspect must update a cursor value, minimizing
+ * allocations. Callers of advance/inspect are expected to copy out any
+ * information from advanceAccessor and inspectAccessor if they need to retain
+ * the information after a call to advance/inspect.
+ **/
+public abstract class InfosetInputter {
+
+ private boolean supportsNamespaces;
+
+ /**
+ * Get if this infoset inputter supports namespaces. The return value of
+ * getNamespace will be ignored if this is false. Note that if this is false,
+ * some infoset representations may fail to unparse if the schema depends on
+ * namespace information to differentiate between elements.
+ */
+ public boolean getSupportsNamespaces() {
+ return supportsNamespaces;
+ }
+
+ /**
+ * Set if this infoset inputter supports namespaces. The return value of
+ * getNamespace will be ignored if this is false. Note that if this is false,
+ * some infoset representations may fail to unparse if the schema depends on
+ * namespace information to differentiate between elements.
+ */
+ public void setSupportsNamespaces(boolean supportsNamespaces) {
+ this.supportsNamespaces = supportsNamespaces;
+ }
+
+ /**
+ * Return the current infoset inputter event type
+ */
+ public abstract Infoset.InfosetInputterEventType getEventType();
+
+ /**
+ * Get the local name of the current event. This will only be called when the
+ * current event type is StartElement.
+ */
+ public abstract String getLocalName();
+
+ /**
+ * Get the namespace of the current event. This will only be called when the
+ * current event type is StartElement. If the InfosetInputter does not
+ * support namespaces, this shoud return null. This may return null to
+ * represent no namespaces.
+ */
+ public abstract String getNamespaceURI();
+
+ /**
+ * Get the content of a simple type. This will only be called when the
+ * current event type is StartElement and the element is a simple type. If
+ * the event contains complex data, it is an error and should throw
+ * NonTextFoundInSimpleContentException. If the element does not have any
+ * simple content, this should return either null or the empty string.
+ */
+ public String getSimpleText(
+ NodeInfo.Kind primType,
+ Map<String, String> runtimeProperties
+ ) {
+ return getSimpleText(primType);
Review Comment:
Is the idea here that if your infoset inputter doesn't care about runtime properties you only imlement getSimpelText(primtType), and if you do care about it you override this one, and just have an empty getSimpleText that is never called?
Why not just have this one function, and if an InfosetInputter doesn't use runtime properties then it can just ignore the parameter? Seems a bit simpler to me. That way there's just one function, and you never get into a situation where you are forced to implement a function you aren't going to use.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -377,22 +334,22 @@ class DataProcessor(
// events are created rather than writing the entire infoset in memory and
// then validating at the end of the parse. See DAFFODIL-2386
//
- val (outputter, maybeValidationBytes) = {
- validationMode match {
- case ValidationMode.Full | ValidationMode.Custom(_) =>
+ val (outputter: api.infoset.InfosetOutputter, maybeValidationBytes) = {
+ validator match {
+ case DaffodilLimitedValidator | null =>
+ (output, Nope)
+ case _ =>
Review Comment:
Do we need a case for `NoValidator`? Right now if NoValidator is set we'll drop into this case, create extra XMLTextnfosetOutputter and TeeInfosetOutputter, and the validator just wont' do anything.
Makes me think more about this NoValidator, and maybe we want to get rid of that and change validator to an Maybe[Validator]? That way we dont' accidentally do extra validator work that ultimately does nothing.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -113,113 +106,54 @@ object DFDL {
schemaSource: DaffodilSchemaSource,
optRootNodeName: Option[String] = None,
optRootNodeNamespace: Option[String] = None
- ): ProcessorFactory
-
- def reload(savedParser: File): DataProcessor
+ ): api.ProcessorFactory
}
/**
* The point of [[ProcessorFactory]] is to allow compilation of the path expression
* and/or generation of source code to process data matching the compiled schema
*/
- trait ProcessorFactory extends WithDiagnostics {
+ trait ProcessorFactory extends WithDiagnostics with api.ProcessorFactory {
/**
- * Returns a [[DataProcessor]] to process data matching a compiled XPath expression
- * @param xpath XPath expression in DFDL schema that data should match (you can use only "/" at this time)
- */
- def onPath(xpath: String): DataProcessor
-
- /**
- * Returns a [[CodeGenerator]] to generate code from a DFDL schema to parse or unparse data
+ * Returns a [[org.apache.daffodil.api.CodeGenerator]] to generate code from a DFDL schema to parse or unparse data
* @param language source language for generated code (you can use only "c" at this time)
*/
- def forLanguage(language: String): CodeGenerator
+ def forLanguage(language: String): api.CodeGenerator
Review Comment:
Should forLanguage be part of the public Java ProcessorFacotry API? The CodeGenerator is, but there is no way to publicly create it, I think? Or are we trying to keep code generator internal only? Maybe we mark forLangaue as experimental if we think it's not ready, so people can use it but it may change?
##########
daffodil-schematron/README.md:
##########
@@ -25,23 +25,25 @@ Supports standalone sch files or embedded schematron rules within the Daffodil s
### Configuration
#### Parameters
+
- `schematron`: default parameter provides path to the schematron file or resource
#### XSLT & XPATH versions
-Schematron defaults to 1.0 of both XSLT and XPath. Use 2.0 by setting `sch:queryBinding="xslt2"` on the schema.
+Schematron defaults to 1.0 of both XSLT and XPath. Use 2.0 by setting `sch:queryBinding="xslt2"` on the schema.
-See version support comments in the [XSL](src/main/resources/iso-schematron-xslt2/iso_schematron_skeleton_for_saxon.xsl) for more details.
+See version support comments in the [XSL](src/main/resources/iso-schematron-xslt2/iso_schematron_skeleton_for_saxon.xsl)
+for more details.
### Parse Results
- `SchematronValidationError` on failed rule check
### SPI registration
-Registered as `schematron`
+Registered as `schematron`
-`Validators.get("schematron")`
+`Validators.getInstance().get("schematron")`
Review Comment:
I *think* we can get rid of all instances of this getInstance() function except for instance the Validators class.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/validation/XercesValidator.scala:
##########
@@ -71,23 +76,30 @@ object XercesValidatorFactory {
* to do a validation pass on the TDML expected Infoset w.r.t. the model and to
* do a validation pass on the actual result w.r.t. the model as an XML document.
*/
-class XercesValidator(schemaSources: Seq[javax.xml.transform.Source]) extends Validator {
+class XercesValidator(schemaSource: javax.xml.transform.Source)
+ extends api.validation.Validator {
private val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
private val resolver = DFDLCatalogResolver.get
factory.setResourceResolver(resolver)
- private val schema = factory.newSchema(schemaSources.toArray)
+ private val schema = factory.newSchema(schemaSource)
private val validator = new ThreadLocal[XercesValidatorImpl] {
override def initialValue(): XercesValidatorImpl =
initializeValidator(schema.newValidator, resolver)
}
- def validateXML(document: java.io.InputStream): ValidationResult =
- validateXML(document, new XercesErrorHandler)
+ def validateXML(
+ document: java.io.InputStream,
+ validationHandler: api.validation.ValidationHandler
+ ): Unit =
+ validateXML(document, new XercesErrorHandler(validationHandler))
- def validateXML(document: java.io.InputStream, eh: ErrorHandler): ValidationResult = {
+ def validateXML(
+ document: java.io.InputStream,
+ eh: ErrorHandler
+ ): Unit = {
Review Comment:
I'd suggest we just merge these validateXML functions, and the first thing validateXML does is
```scala
val eh = new XercesErrorHandler(validationhandler)
// everything else
```
As far as I can tell, nothing directly calls the ErrorHandler variant so it just makes the logic a bit more difficult to follow. Or does something actually use this variant for testing?
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/validation/XercesValidator.scala:
##########
@@ -137,39 +137,26 @@ object XercesValidator {
private type XercesValidatorImpl = javax.xml.validation.Validator
val name = "xerces"
- def fromURIs(schemaURIs: Seq[URI]) = new XercesValidator(schemaURIs.map { uri =>
- val is = uri.toURL.openStream()
+ def fromURI(schemaURI: URI) = new XercesValidator({
+ val is = schemaURI.toURL.openStream()
val stream = new StreamSource(is)
stream.setSystemId(
- uri.toString
+ schemaURI.toString
) // must set this so that relative URIs will be created for import/include files.
stream
})
- def fromFiles(schemaFileNames: Seq[String]) =
- fromURIs(schemaFileNames.map { new URI(_) })
+ object ConfigKeys {
+ val rootSchema = api.validation.Validator.rootSchemaKey
+ }
Review Comment:
Can things not directly access api.validation.Validator.rootSchemaKey? Seems kindof unnecessary to create a special object that copies the value. But maybe this is some Java access this?
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -1363,16 +1382,16 @@ case class ParserTestCase(ptc: NodeSeq, parentArg: DFDLTestSuite)
lengthLimitInBits: Long,
warnings: Option[Seq[ExpectedWarnings]],
validationErrors: Option[Seq[ExpectedValidationErrors]],
- validationMode: ValidationMode.Type,
+ validator: api.validation.Validator,
roundTripArg: RoundTrip,
implString: Option[String],
- compileWarnings: Seq[Diagnostic]
+ compileWarnings: Seq[api.Diagnostic]
): Unit = {
val roundTrip =
roundTripArg // change to OnePassRoundTrip to force all parse tests to round trip (to see which fail to round trip)
- processor = processor.withValidationMode(validationMode)
+ processor = processor.withValidator(validator)
Review Comment:
Note that processor could by the IBM DFDL cross tester. withValidator really is not a Daffodil specific thing. We have two options
The right thing to do might be to modify the IBM DFDL cross tester so that it looks for our specific validators and disables validation if it's NoValidator, and enables it if it's anything else. Alternatively, maybe we need a different API for the `TDMLDFDLProcessor`. I'm not sure what that would be in a way that woudl allow the Daffodil TDML processor to work. But somethign to consider. We may want to open a bug to figure out how to handle validation in a way that works better for the IBM DFDL TDML processor that doesn't need to know about our internal Validator API.
Another option is the TDML Runner still only understands full, liimted, and off, and then it is up to the IBM TDML Professor and Daffodil TDML Processor to convert that to their own kind of validation. The Daffodil one will use the Vlaidator API. The IBM one will just set a validation boolean.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/InfosetItem.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.net.URI;
+
+import com.ibm.icu.util.Calendar;
Review Comment:
These imports are all unused. Maybe we need to enable some javacOptions to check for things like unused imports, similar to what we add in scalacOptions?
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Diagnostic.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * Class containing diagnostic information. This does not need to
+ * be implemented by the user.
+ */
+public abstract class Diagnostic extends Exception {
+
+ public Diagnostic(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) {
+ super(message, cause, enableSuppression, writableStackTrace);
+ }
+
+ /**
+ * Get all diagnostic information, including mode name, schema context, and data location
+ * <p>
+ * Future versions of Daffodil will change getMessage to return only the message portion of a
+ * diagnostic, matching the behavior of getMessageOnly--users should call toString() instead.
+ *
+ * @return diagnostic message in string form
+ */
+ public abstract String getMessage();
+
+ /**
+ * Get only the message part of a diagnostic.
+ * <p>
+ * This does not include mode name, schema context, or data location information
+ *
+ * @return diagnostic message in string form
+ */
+ public abstract String getMessageOnly();
+
+ /**
+ * Get the diagnostic mode name
+ *
+ * @return diagnostic mode name in string form
+ */
+ public abstract String getModeName();
+
+ /**
+ * Get a string containing the mode name, message, schema location, and data location combined
+ * into a single string
+ *
+ * @return all diagnostic information as a string
+ */
+ @Override
+ public abstract String toString();
+
+
+ /**
+ * Get data location information relevant to this diagnostic object.
+ * <p>
+ * For example, this might be a file name, and position within the file.
+ *
+ * @return list of [[DataLocation]]'s related to this diagnostic
+ */
+ public abstract List<DataLocation> getDataLocations();
+
+ /**
+ * Get schema location information relevant to this diagnostic object.
+ * <p>
+ * For example, this might be a file name of a schema, and position within the schema file.
+ *
+ * @return list of [[LocationInSchemaFile]]'s related to this diagnostic.
+ */
+ public abstract List<LocationInSchemaFile> getLocationsInSchemaFiles();
+
+ /**
+ * Determine if a diagnostic object represents an error or something less serious.
+ *
+ * @return true if it represents an error, false otherwise
+ */
+ public abstract boolean isError();
+
+
+ public abstract boolean isValidation();
+
+ /**
+ * Get the cause of that cause this diagnostic
+ *
+ * @return the exception that caused the diagnostic
+ */
+ public abstract Optional<Throwable> getSomeCause();
+
+ /**
+ * Get the message that caused this diagnostic
+ *
+ * @return the message that caused the diagnostic
+ */
+ public abstract Optional<String> getSomeMessage();
Review Comment:
Do we want to remove getSomeCause/Message, so that we follow the normal Exception api? It means we have do do null checks, but it is more Java-y.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -409,29 +366,21 @@ class DataProcessor(
val pr = if (state.processorStatus == Success) {
// validate infoset, errors are added to the PState diagnostics
- val vr = maybeValidationBytes.toOption.map { bytes =>
+ maybeValidationBytes.toOption.foreach { bytes =>
val bis = new java.io.ByteArrayInputStream(bytes.toByteArray)
- val res = validator.validateXML(bis)
- res.warnings.forEach { w => state.validationError(w.getMessage) }
- res.errors.forEach {
- case e: ValidationException =>
- state.validationErrorNoContext(e.getCause)
- case f: ValidationFailure =>
- state.validationError(f.getMessage)
- }
- res
+ validator.validateXML(bis, state)
Review Comment:
Maybe add a comment here that validateXML will add any validation diagnostics to the state, so it's clear this is mutating things.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/InfosetDocument.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+
+/**
+ * Access to the infoset document element (also known as the root element).
+ */
+public interface InfosetDocument extends InfosetItem {
+
+ /**
+ * Access to the metadata information about this element.
+ * See {@link ElementMetadata}
+ */
+ @Override
+ ElementMetadata metadata();
+}
Review Comment:
Similar to the metadata classes, do these Infoset calsses also want to be in a different package. Maybe it all goes in org.apache.api.infoset? Do they want to go somwehere else? Mayve infoset things and metadata things are in separate subpackages?
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/infoset/JDOMInfosetInputter.scala:
##########
@@ -109,16 +110,16 @@ class JDOMInfosetInputter(doc: Document) extends InfosetInputter {
text
}
- override def isNilled(): MaybeBoolean = {
+ override def isNilled(): Optional[JBoolean] = {
Review Comment:
I wonder if we can remove this Optional and just make isNilled return a JBoolean? I don't think Daffodil ever cares if a infoset element is explicitly set to false or not and I think that just makes this API more confusing. None will always be treated as false so do we get value in making infoset implementations think about this?
##########
daffodil-core/src/main/java/org/apache/daffodil/api/validation/Validators.java:
##########
@@ -15,49 +15,61 @@
* limitations under the License.
*/
-package org.apache.daffodil.lib.validation
+package org.apache.daffodil.api.validation;
-import org.apache.daffodil.lib.api.ValidatorFactory
-import org.apache.daffodil.lib.util.SimpleNamedServiceLoader
+import org.apache.daffodil.util.SimpleNamedServiceLoader;
+
+import java.util.Optional;
/**
- * Access SPI registered [[org.apache.daffodil.lib.api.ValidatorFactory]] instances.
- *
+ * Access SPI registered [[org.apache.daffodil.api.validation.ValidatorFactory]] instances.
+ * <p>
* Registered instances provide a unique name for lookup.
*/
-object Validators {
- private lazy val impls: Map[String, ValidatorFactory] = {
- SimpleNamedServiceLoader.loadClass[ValidatorFactory](classOf[ValidatorFactory])
+public class Validators {
+ private static final Validators INSTANCE = new Validators();
+ private final java.util.Map<String, ValidatorFactory> impls =
+ SimpleNamedServiceLoader.loadClass(ValidatorFactory.class);
+
+ private Validators() {
+ } // private constructor
+
+ public static Validators getInstance() {
Review Comment:
We should use the java singleton syncronization pattern to ensure this doesn't run into any issues with thread safety. I believe the INSTANCE needs to be marked as volatile, and we need a syncronized block.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -317,15 +273,13 @@ class DataProcessor(
//
// Make a copy of this object so that we can make its saved state
// different than its original state. Note other software like
- // Apache Spark may require variableMap and validationMode to be
+ // Apache Spark may require variableMap to be
// preserved. But for our save/reload purposes, we want to reset
// them back to their original values.
//
val dpToSave = this.copy(
// reset to original variables defined in schema
variableMap = ssrd.originalVariables,
- // explicitly turn off validation, reloaded processors must call withValidationMode
- validationMode = ValidationMode.Off,
Review Comment:
Why not set this to `validator = NoValidator`? Being explicit that we are disabling this would be kind of nice.
Maybe there are issues with serialziing NoValidator? Can we make that serializable?
Or maybe we want to change validator to an Option[Validator] and get rid of NoValidator and disabling validation (or calling withValidator(null) to be java friendly) will set validator to None?
##########
daffodil-core/src/main/java/org/apache/daffodil/api/InfosetSimpleElement.java:
##########
@@ -100,228 +48,192 @@
/**
* @return the value of this simple element as a Scala AnyRef, which is
- * equivalent to a Java Object.
+ * equivalent to a Java Object.
*/
- def getAnyRef: AnyRef
+ Object getAnyRef();
/**
* @return the value of this simple element as an Object (java.lang.Object),
- * which is equivalent to Scala AnyRef.
+ * which is equivalent to Scala AnyRef.
Review Comment:
Remove reference to Scala Anyref.
##########
daffodil-tdml-processor/src/main/scala/org/apache/daffodil/processor/tdml/TDMLInfosetInputter.scala:
##########
@@ -70,6 +73,12 @@ class TDMLInfosetInputter(
res
}
+ override def getSimpleText(primType: NodeInfo.Kind): String =
+ scalaInputter.getSimpleText(
+ primType,
+ Map[String, String](XMLTextInfoset.stringAsXml -> "false").asJava
Review Comment:
This looks incorrect to me. Nothing should require that stringAsXml exists.
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -760,10 +769,20 @@ abstract class TestCase(testCaseXML: NodeSeq, val parent: DFDLTestSuite) {
case "false" => false
case _ => false
}
- lazy val validationMode: ValidationMode.Type = (testCaseXML \ "@validation").text match {
- case "" => defaultValidationMode
- case mode => ValidationMode.fromString(mode)
- }
+ lazy val validatorName: String = (testCaseXML \ "@validation").text match {
+ case "" => defaultValidatorName
+ case "on" => "xerces"
+ case mode => mode
+ }
+ lazy val validator: api.validation.Validator =
+ Validators
+ .getInstance()
+ .get(validatorName)
+ .make({
+ val props = new Properties()
+ props.setProperty(Validator.rootSchemaKey, getSuppliedSchema().uriForLoading.toString)
+ props
+ })
Review Comment:
Seeing this outside of the CLI makes me reconsider if it would be useful to have a function to auto create a Properties with the rootSchemaKey set. This could all be simplified to a single line. Maybe just document that it's a helper utility, and it's not mandatory, but it does set the rootSchemaKey, which many validators can use.
##########
daffodil-tdml-processor/src/main/scala/org/apache/daffodil/processor/tdml/DaffodilTDMLDFDLProcessor.scala:
##########
@@ -198,15 +200,17 @@ class DaffodilTDMLDFDLProcessor private (private var dp: DataProcessor)
copy(dp = dp.withDebugger(d))
}
- override def withValidationMode(
- validationMode: ValidationMode.Type
+ override def withValidator(
+ validator: api.validation.Validator
Review Comment:
Yeah, this right here is maybe a bit questionable. The TDML DFDL Processor API probably doesn't want to require a Daffodil Validator, but something else. Not sure exactly how to make that work. Just open a bug and we can figure out what the right change is.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Result.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import java.util.List;
+
+/**
+ * Interface for Parse and Unparse results
+ */
+public interface Result {
+ State resultState();
Review Comment:
Feels like this the internal state doesn't really want to be public. Internally there might be a state that the implementations of these public methods use to make their decisions, but I'm not sure we want to expose that publicly. Is there something that needs this?
##########
daffodil-core/src/main/java/org/apache/daffodil/api/validation/Validators.java:
##########
@@ -15,49 +15,61 @@
* limitations under the License.
*/
-package org.apache.daffodil.lib.validation
+package org.apache.daffodil.api.validation;
-import org.apache.daffodil.lib.api.ValidatorFactory
-import org.apache.daffodil.lib.util.SimpleNamedServiceLoader
+import org.apache.daffodil.util.SimpleNamedServiceLoader;
+
+import java.util.Optional;
/**
- * Access SPI registered [[org.apache.daffodil.lib.api.ValidatorFactory]] instances.
- *
+ * Access SPI registered [[org.apache.daffodil.api.validation.ValidatorFactory]] instances.
+ * <p>
* Registered instances provide a unique name for lookup.
*/
-object Validators {
- private lazy val impls: Map[String, ValidatorFactory] = {
- SimpleNamedServiceLoader.loadClass[ValidatorFactory](classOf[ValidatorFactory])
+public class Validators {
+ private static final Validators INSTANCE = new Validators();
+ private final java.util.Map<String, ValidatorFactory> impls =
+ SimpleNamedServiceLoader.loadClass(ValidatorFactory.class);
+
+ private Validators() {
+ } // private constructor
+
+ public static Validators getInstance() {
+ return INSTANCE;
}
/**
* Get the factory by name or throw
+ *
* @param name registered name of the validator factory
+ * @return {@link org.apache.daffodil.api.validation.ValidatorFactory} the factory instance
* @throws ValidatorNotRegisteredException when factory is not found in the registered services
- * @return [[org.apache.daffodil.lib.api.ValidatorFactory]] the factory instance
*/
- @throws(classOf[ValidatorNotRegisteredException])
- def get(name: String): ValidatorFactory =
- impls.getOrElse(name, throw ValidatorNotRegisteredException(name))
+ public ValidatorFactory get(String name) throws ValidatorNotRegisteredException {
Review Comment:
Can we make all these functions static so no one every needs to do `Validators.getInstance` first? They can just do things like `Validators.get(..)` and the first thing that will do is call getInstance? Should simplify uses.
##########
daffodil-core/src/test/scala/org/apache/daffodil/core/general/TestTunables.scala:
##########
@@ -63,8 +64,8 @@ class TestTunables {
val c2 = c.withTunable("maxSkipLengthInBytes", "2048")
val pf2 = c2.compileNode(testSchema)
- val dp1 = pf1.onPath("/")
- var dp2 = pf2.onPath("/")
+ val dp1 = pf1.onPath("/").asInstanceOf[DataProcessor]
+ var dp2 = pf2.onPath("/").asInstanceOf[DataProcessor]
Review Comment:
Mentioned in another comment asking why we have withTunables on a scala data processor. If it's just for tests liek this, I would suggest we remove this test. It's just testing that we can change tunables on a DataProcessor, but that's probably not something we need to, or should, support.
Some tunables are used at compile-time, so changing them after compilation but before running could potentially lead to errors if the way things were compiled doesn't match with the way things were expected to run.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -291,7 +246,8 @@ class DataProcessor(
override def isError = false
- override def getDiagnostics = diagnostics
+ override def getDiagnostics: util.List[api.Diagnostic] =
+ diagnostics.asInstanceOf[Seq[api.Diagnostic]]
Review Comment:
This kindof implicit copy from Scala to Java structions could be an issue. I imagine diagnostics are not going to be that common, so maybe not a concner, but we should be careful about the implicit java conversions. We migt want to remove all those implicits and so we find all the places were are doing that and ensure none of them are in critical sections.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/InfosetSimpleElement.java:
##########
@@ -100,228 +48,192 @@
/**
* @return the value of this simple element as a Scala AnyRef, which is
- * equivalent to a Java Object.
+ * equivalent to a Java Object.
*/
- def getAnyRef: AnyRef
+ Object getAnyRef();
/**
* @return the value of this simple element as an Object (java.lang.Object),
- * which is equivalent to Scala AnyRef.
+ * which is equivalent to Scala AnyRef.
*/
- final def getObject: java.lang.Object = getAnyRef
-
- // Note: I could not get @throws in scaladoc to work right.
- // Complains "Could not find any member to link for ... and I tried various formulations of
- // InfosetTypeException, with package, with and without [[..]].
- // So I've just converted it to plain text.
+ default Object getObject() {
+ return getAnyRef();
+ }
/**
* @return Casts the value of this simple element as a java.math.BigDecimal
- * or throws `InfosetTypeException` if the element is not of type decimal.
+ * or @throws InfosetTypeException if the element is not of type decimal.
*/
- def getDecimal: JBigDecimal
+ BigDecimal getDecimal();
Review Comment:
An above comment (now removed) mentioned the @throws annotation didn't work. Probably some genjavadoc issue?
Now that things are Java, I imagine we can change the throws in the comments to actual @throws annotations?
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -113,113 +106,54 @@ object DFDL {
schemaSource: DaffodilSchemaSource,
optRootNodeName: Option[String] = None,
optRootNodeNamespace: Option[String] = None
- ): ProcessorFactory
-
- def reload(savedParser: File): DataProcessor
+ ): api.ProcessorFactory
}
/**
* The point of [[ProcessorFactory]] is to allow compilation of the path expression
* and/or generation of source code to process data matching the compiled schema
*/
- trait ProcessorFactory extends WithDiagnostics {
+ trait ProcessorFactory extends WithDiagnostics with api.ProcessorFactory {
/**
- * Returns a [[DataProcessor]] to process data matching a compiled XPath expression
- * @param xpath XPath expression in DFDL schema that data should match (you can use only "/" at this time)
- */
- def onPath(xpath: String): DataProcessor
-
- /**
- * Returns a [[CodeGenerator]] to generate code from a DFDL schema to parse or unparse data
+ * Returns a [[org.apache.daffodil.api.CodeGenerator]] to generate code from a DFDL schema to parse or unparse data
* @param language source language for generated code (you can use only "c" at this time)
*/
- def forLanguage(language: String): CodeGenerator
+ def forLanguage(language: String): api.CodeGenerator
}
- /**
- * Source code generation and compilation is performed with a language-specific [[CodeGenerator]],
- * which must be interrogated for diagnostics to see if each call was successful or not.
- */
- trait CodeGenerator extends WithDiagnostics {
-
- /**
- * Generates language-specific code from a DFDL schema to parse or unparse data
- * @param outputDir output directory in which to create code directory (codeDir)
- * @return path of newly created directory (codeDir) containing generated code
- */
- def generateCode(outputDir: String): os.Path
-
- /**
- * Compiles the generated code in order to run it in a TDML test
- * @param codeDir path of newly created directory containing generated code
- * @return path of newly built executable (exe) compiled from generated code
- */
- def compileCode(codeDir: os.Path): os.Path
- }
-
- trait DataProcessor extends WithDiagnostics {
+ trait DataProcessor extends api.DataProcessor with WithDiagnostics {
/**
* Returns a data processor with all the same state, but the validation mode changed to that of the argument.
*
* Note that the default validation mode is "off", that is, no validation is performed.
*/
- def withValidationMode(mode: ValidationMode.Type): DataProcessor
def withTunable(name: String, value: String): DataProcessor
def withTunables(tunables: Map[String, String]): DataProcessor
Review Comment:
I'm surprise we have withTunable on the scala DataProcessor but not part of the public API. If withTunable should only be called on the Compielr (which kindof makes sense, since changing tunables after we get a DataProcessor feels fragile), should we remove these withTunables functions?
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/infoset/XMLTextInfosetInputter.scala:
##########
@@ -290,6 +295,9 @@ class XMLTextInfosetInputter(input: java.io.InputStream) extends InfosetInputter
xmlString
}
+ override def getSimpleText(primType: NodeInfo.Kind): String =
+ getSimpleText(primType, Map[String, String](XMLTextInfoset.stringAsXml -> "false").asJava)
Review Comment:
Do we need to add a `stringAsXML -> false` entry? That's going to have to allocate a new map every single time getSimpleText is called, which could be pretty expensive. We should be able to just get the runtime properties from erd, which are calculated at compile time so should be pretty efficient.
If some logic somewhere requires this runtime property, we probably need to change it so that it handles stringAsXml not existing and treating that as false rather than forcing that the property be set.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -208,49 +195,17 @@ class DataProcessor(
*/
override def clone(): DataProcessor = copy()
- /**
- * Returns a data processor with all the same state, but the validation mode and validator changed to that of the argument.
- *
- * Note that the default validation mode is "off", that is, no validation is performed.
- */
- def withValidationMode(mode: ValidationMode.Type): DataProcessor = {
- // create the appropriate validator for the mode. If the mode isn't actually changing then
- // we won't generate a new validator and just use the same one. The Validator API requires
- // the Validator to be thread safe, so it is fine to share the same Validator with multiple
- // DataProcessors
- val newValidator = mode match {
- case `validationMode` => validator
- case ValidationMode.Off => null
- case ValidationMode.Limited => null
- case ValidationMode.Custom(cv) => cv
- case ValidationMode.Full => {
- // we only need to provide the main schema, Xerces will find imported/included schemas
- // using the Daffodil resolver set in the XercesValidatorFactory
- val sources = Seq(ssrd.mainSchemaUriForFullValidation.toString)
- val cfg = XercesValidatorFactory.makeConfig(sources)
- XercesValidatorFactory.makeValidator(cfg)
- }
- }
- copy(
- validationMode = mode,
- validator = newValidator
- )
- }
-
- def withValidator(validator: Validator): DataProcessor = {
- copy(
- validationMode = ValidationMode.Custom(validator),
- validator = validator
- )
+ override def withValidator(validator: api.validation.Validator) = {
+ copy(validator = validator).asInstanceOf[api.DataProcessor]
Review Comment:
Is this asInstanceOf needed? Copy shoudl return a runtime1.processors.DataProcessor, which implements api.Dataprocessor so it should already return the right thing.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -129,18 +127,11 @@ class DataProcessor(
val ssrd: SchemaSetRuntimeData,
val tunables: DaffodilTunables, // Compiler-set tunables
val variableMap: VariableMap,
- //
- // The below do not need to be transient
- // because this object itself isn't serialized. A SerializableDataProcessor is.
- // The values these will have (since this is a base class) are the correct default values that we want
- // back when the object is re-initialized.
- //
- val validationMode: ValidationMode.Type = ValidationMode.Off,
// The Validator API requires this to be thread-safe so this is safe to share among different
// DataProcessors
- val validator: Validator = null,
+ val validator: api.validation.Validator = null,
Review Comment:
Can we initialize this to NoValidator so that it's never null?
##########
daffodil-schematron/src/main/scala/org/apache/daffodil/validation/schematron/SchematronValidator.scala:
##########
@@ -17,40 +17,46 @@
package org.apache.daffodil.validation.schematron
+import java.io.File
import java.io.FileOutputStream
import java.io.InputStream
-import java.nio.file.Path
+import java.net.URI
import scala.util.Try
import scala.xml.Elem
import scala.xml.XML
-import org.apache.daffodil.lib.api.ValidationException
-import org.apache.daffodil.lib.api.ValidationFailure
-import org.apache.daffodil.lib.api.ValidationResult
-import org.apache.daffodil.lib.api.Validator
+import org.apache.daffodil.api
+import org.apache.daffodil.api.validation.Validator
/**
* Daffodil Validator implementation for ISO schematron
+ *
+ * Configuration
+ * - schematron.path=uri_string_to_schematron_file
+ * - schematron.svrl.file=uri_string_to_output_file
+ * - daffodil.rootSchema=schema_file_uri_string
Review Comment:
This needs additional documentation, something svrl.file is optional, and if provided a schematron report will be written there. If schematron.path is defined it will use that and expect it to be a .sch file. If that's not defined, it will use daffodil.rootSchema and attempt to extra embedded schematron rules.
Also, should `schematron.path` be changed to `schematron`? So that if you do something like this will work?
```scala
daffodil parse -s ... --validate schematron=path/to/schematron.sch
```
or is schematron complex enough that it really requires a config file?
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/validation/XercesValidator.scala:
##########
@@ -44,25 +36,38 @@ import org.xml.sax.SAXParseException
*
* SPI service name: xerces
* Configuration requirements:
- * - xerces: List[String] - schema file names
+ * - daffodil.rootSchema=schema_file_uri_string
+ * - xerces=schema_file_uri_string
*/
-class XercesValidatorFactory extends ValidatorFactory {
+class XercesValidatorFactory extends api.validation.ValidatorFactory {
def name(): String = XercesValidator.name
- def make(config: Config): Validator = XercesValidatorFactory.makeValidator(config)
+
+ def make(config: Properties): api.validation.Validator =
+ XercesValidatorFactory.makeValidator(config)
}
object XercesValidatorFactory {
- def makeValidator(config: Config): Validator = {
- val schemaFiles =
- if (config.hasPath(XercesValidator.name))
- config.getStringList(XercesValidator.name).asScala
- else Seq.empty
- XercesValidator.fromFiles(schemaFiles.toSeq)
+ def makeValidator(uriString: String): api.validation.Validator = {
+ val config = XercesValidatorFactory.makeConfig(uriString)
+ makeValidator(config)
+ }
+
+ def makeValidator(config: Properties): api.validation.Validator = {
+ val schemaPath = config.getProperty(XercesValidator.name)
+ val schemaFile =
+ if (schemaPath != null) schemaPath
Review Comment:
What is schemaPath is the empty string? Should we treat that the same as null and fall back to trying to use rootSchema? So this becomes something like
```scala
val xercesProp = config.getProperty(XercesValidator.name)
val rootSchemaProp config.getProperty(XercesValidator.ConfigKeys.rootSchema)
val schemaFile =
if (!isNullOrBlank(xercesProp)) xercesProp
else if (!isNullOrBlank(rootSchemaProp)) rootSchemaProp
else throw ValidatorInitializationException(...)
val uri = new URI(schemaFile)
XercesValidator.fromURI(uri)
```
##########
daffodil-core/src/main/java/org/apache/daffodil/api/infoset/XMLTextEscapeStyle.java:
##########
@@ -33,5 +35,18 @@ public enum XMLTextEscapeStyle {
* special characters (quotation mark, ampersand, less-than, greater-than) or
* whitespace
*/
- CDATA,
+ CDATA;
+
+ static Enumeration.Value styleToScala(XMLTextEscapeStyle style) {
+ Enumeration.Value sXMLTextEscapeStyle = null;
+ if (style == XMLTextEscapeStyle.Standard) {
+ sXMLTextEscapeStyle = org.apache.daffodil.runtime1.infoset.XMLTextEscapeStyle.Standard();
+ } else if (style == XMLTextEscapeStyle.CDATA) {
+ sXMLTextEscapeStyle = org.apache.daffodil.runtime1.infoset.XMLTextEscapeStyle.CDATA();
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Unrecognized value: %s for parameter: xmlTextEscapeStyle. Must be 'Standard' or 'CDATA'.", style));
+ }
+ return sXMLTextEscapeStyle;
+ }
Review Comment:
Do we need this toScala function anymore? Can we just use the Java enum values in our scala code?
##########
daffodil-core/src/main/java/org/apache/daffodil/api/debugger/package-info.java:
##########
@@ -19,22 +19,22 @@
* Provides the classes necessary to perform parse tracing or create a custom debugger
*
* <h2>Overview</h2>
- *
+ * <p>
* Daffodil comes with one prebuilt debugger, the {@link
- * org.apache.daffodil.japi.debugger.TraceDebuggerRunner}, which outputs
+ * org.apache.daffodil.runtime1.debugger.TraceDebuggerRunner}, which outputs
* verbose information during the parsing processes, which can be used to aid
* in debugging a DFDL schema. For example, the {@link
- * org.apache.daffodil.japi.debugger.TraceDebuggerRunner} can be use like so:
+ * org.apache.daffodil.runtime1.debugger.TraceDebuggerRunner} can be use like so:
Review Comment:
This javadoc references internal classes. Users shouldn't create these directly. We probably need a public factory to create an instance of the trace debugger runner.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/infoset/BlobMethods.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.infoset;
+
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.List;
+
+/**
+ * An available basic implementation of the BLOB methods.
+ * Stores blobs in files in directory identified by Java system property
+ * `java.io.tempdir`.
+ */
+public abstract class BlobMethods {
Review Comment:
This kindof feels like a weird name for an abstract class, though I'm not sure what a better name would be.
Though, really no one should be implementing their own BlobMethods class. It should always be used in the context of an InfosetOutputter. Should we just copy the content of this into the InfosetOutputter iterface? I'm not sure the separation provides anything useful anymore, and potentially just makes the public API more confusion.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/validation/DaffodilLimitedValidator.scala:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.lib.validation
+
+import java.io.InputStream
+import java.util.Properties
+
+import org.apache.daffodil.api
+import org.apache.daffodil.api.validation.ValidationHandler
+import org.apache.daffodil.api.validation.Validator
+import org.apache.daffodil.lib.exceptions.Assert
+
+object DaffodilLimitedValidator extends api.validation.Validator {
+ override def validateXML(
+ document: InputStream,
+ validationHandler: ValidationHandler
+ ): Unit = {
+ Assert.usageError("ValidateXML must not be called on the DaffodilLimitedValidator")
Review Comment:
Do we want to make this a no-op? That way people can still enable the limited validator, and can still call validateXML if they want, it just won't do anything extra?
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -450,6 +451,13 @@ object Misc {
rbc
}
+ def prependItemToJavaList[T](item: T, jlist: util.List[T]): util.List[T] = {
+ val jl = new util.ArrayList[T]()
+ jl.add(item)
+ jl.addAll(jlist)
+ jl
+ }
+
Review Comment:
We may want to consider changing how we handle things that use this. Creating a new list and copying the current list is going to get expensive if we do it alot. Does not have to be done in this PR, but please create a ticket for it.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/iapi/Diagnostic.scala:
##########
@@ -125,6 +129,8 @@ abstract class Diagnostic protected (
*/
override def getMessage(): String = message
Review Comment:
I think `getMessage()` wants to just call `toString()` so that it returns the context, mode, etc. so that it matches the current API.
There is a bug to change this behavior so that it returns what you currently have and we remove getMessageOnly, but that can be done in a separate PR.
Though, there is a chance changing this to toString will break things. If it does, then we can keep this change as is, as long as we plan to change the behavior and drop getMessageOnly()
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/validation/DaffodilLimitedValidator.scala:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.lib.validation
+
+import java.io.InputStream
+import java.util.Properties
+
+import org.apache.daffodil.api
+import org.apache.daffodil.api.validation.ValidationHandler
+import org.apache.daffodil.api.validation.Validator
+import org.apache.daffodil.lib.exceptions.Assert
+
+object DaffodilLimitedValidator extends api.validation.Validator {
+ override def validateXML(
+ document: InputStream,
+ validationHandler: ValidationHandler
+ ): Unit = {
+ Assert.usageError("ValidateXML must not be called on the DaffodilLimitedValidator")
+ }
+}
+
+class DaffodilLimitedValidator
+
+/**
+ * Configuration properties
+ * - none; empty Properties file or null is sufficient
+ */
Review Comment:
I think the documentation about what properties are needed (or none) is needed somewhere in the Java API. Maybe we should create `api/validator/package-info.java` and put in the information about all the built-in validators and what peropties they require. Maybe also include some examples of how to create validators via the API, or what examples of .properties file might look like?
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/validation/XercesValidator.scala:
##########
@@ -44,25 +36,38 @@ import org.xml.sax.SAXParseException
*
* SPI service name: xerces
* Configuration requirements:
- * - xerces: List[String] - schema file names
+ * - daffodil.rootSchema=schema_file_uri_string
+ * - xerces=schema_file_uri_string
*/
-class XercesValidatorFactory extends ValidatorFactory {
+class XercesValidatorFactory extends api.validation.ValidatorFactory {
def name(): String = XercesValidator.name
- def make(config: Config): Validator = XercesValidatorFactory.makeValidator(config)
+
+ def make(config: Properties): api.validation.Validator =
+ XercesValidatorFactory.makeValidator(config)
}
object XercesValidatorFactory {
- def makeValidator(config: Config): Validator = {
- val schemaFiles =
- if (config.hasPath(XercesValidator.name))
- config.getStringList(XercesValidator.name).asScala
- else Seq.empty
- XercesValidator.fromFiles(schemaFiles.toSeq)
+ def makeValidator(uriString: String): api.validation.Validator = {
+ val config = XercesValidatorFactory.makeConfig(uriString)
+ makeValidator(config)
+ }
+
+ def makeValidator(config: Properties): api.validation.Validator = {
+ val schemaPath = config.getProperty(XercesValidator.name)
+ val schemaFile =
+ if (schemaPath != null) schemaPath
+ else config.getProperty(XercesValidator.ConfigKeys.rootSchema)
+ if (Misc.isNullOrBlank(schemaFile)) null
Review Comment:
According to the documentation for make() this should throw a specific exception if it fails?
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/ElementCombinator1.scala:
##########
@@ -201,7 +202,7 @@ abstract class ElementParserBase(
// if we should do limited validation via CheckConstraints
val shouldValidate = erd.isSimpleType &&
pstate.dataProc.isDefined &&
- pstate.dataProc.value.validationMode == ValidationMode.Limited
+ pstate.dataProc.value.validator == DaffodilLimitedValidator
Review Comment:
This might want to be `eq` to ensure we're doing reference equality, which should be faster.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/validation/XercesValidator.scala:
##########
@@ -44,25 +36,38 @@ import org.xml.sax.SAXParseException
*
* SPI service name: xerces
* Configuration requirements:
- * - xerces: List[String] - schema file names
+ * - daffodil.rootSchema=schema_file_uri_string
+ * - xerces=schema_file_uri_string
*/
-class XercesValidatorFactory extends ValidatorFactory {
+class XercesValidatorFactory extends api.validation.ValidatorFactory {
def name(): String = XercesValidator.name
- def make(config: Config): Validator = XercesValidatorFactory.makeValidator(config)
+
+ def make(config: Properties): api.validation.Validator =
+ XercesValidatorFactory.makeValidator(config)
}
object XercesValidatorFactory {
- def makeValidator(config: Config): Validator = {
- val schemaFiles =
- if (config.hasPath(XercesValidator.name))
- config.getStringList(XercesValidator.name).asScala
- else Seq.empty
- XercesValidator.fromFiles(schemaFiles.toSeq)
+ def makeValidator(uriString: String): api.validation.Validator = {
+ val config = XercesValidatorFactory.makeConfig(uriString)
+ makeValidator(config)
+ }
+
+ def makeValidator(config: Properties): api.validation.Validator = {
+ val schemaPath = config.getProperty(XercesValidator.name)
+ val schemaFile =
+ if (schemaPath != null) schemaPath
+ else config.getProperty(XercesValidator.ConfigKeys.rootSchema)
+ if (Misc.isNullOrBlank(schemaFile)) null
+ else {
+ val uri = new URI(schemaFile)
+ XercesValidator.fromURI(uri)
+ }
}
- def makeConfig(uris: Seq[String]): Config = {
- val v = ConfigValueFactory.fromIterable(uris.asJava)
- ConfigFactory.parseMap(Map(XercesValidator.name -> v).asJava)
+ def makeConfig(uri: String): Properties = {
+ val config = new Properties()
+ config.setProperty(XercesValidator.ConfigKeys.rootSchema, uri)
+ config
}
Review Comment:
This makeConfig function feels a bit odd to me. Feels like it doesn't belong in the xerces parser since there isn't really anything specific to it. Maybe it wants to be part of ValidatorFactory, as just a way to create a default config, something like: this?
```java
public interface ValidatorFactory ... {
static Properties makeDefaultConfig(dfdlSchema: URI) = {
Properties config = new Properties();
config.setProperty(...);
return new Properties(config)
return config;
}
}
```
Then this can used by the CLI or other things by doing:
```scala
val properties = ValidatorFactory.makeConfig(schema)
properties.setProperty(....)
...
```
I'm not sure that gains a whole lot, but I guess it does mean users don't need to know about the rootSchema. But things get a little tricky since you don't need rootSchema for all validators, and people might thing they must call makeConfig to get a default config. So there might be confusion about how to use a limited validator if you don't have the rootSchema, which you might not have like in cases when using a saved parser.
I'm kindof the opinion that as long as we document what properties are needed, users are capable of creating their own Properties instance--so this helper isn't all that useful, and potentially confusing.
##########
daffodil-tdml-processor/src/main/scala/org/apache/daffodil/processor/tdml/DaffodilTDMLDFDLProcessor.scala:
##########
@@ -334,7 +338,9 @@ class DaffodilTDMLDFDLProcessor private (private var dp: DataProcessor)
val saxInputStream = optSaxInputStream.get
val saxOutputStream = new ByteArrayOutputStream
val saxOutputChannel = java.nio.channels.Channels.newChannel(saxOutputStream)
- val unparseContentHandler = dp.newContentHandlerInstance(saxOutputChannel)
+ val unparseContentHandler = dp
+ .newContentHandlerInstance(saxOutputChannel)
+ .asInstanceOf[DaffodilUnparseContentHandler]
Review Comment:
Is this needed only because we are using 'enableResolutionOfRelativeInfosetBlobs', which is not public? Can you add a comment about that?
##########
daffodil-schematron/src/main/scala/org/apache/daffodil/validation/schematron/SchematronValidatorFactory.scala:
##########
@@ -75,7 +81,7 @@ object SchematronValidatorFactory {
schematron: InputStream,
schematronID: String,
srcfmt: SchSource,
- svrlPath: Option[Path]
+ svrlPath: Option[URI]
): SchematronValidator = {
val factory = new TransformerFactoryImpl()
factory.setURIResolver(DFDLCatalogResolver.get)
Review Comment:
This is an existing bug, but I *think* SchematronValidator is not thread safe. makeValidator is required to returns something that is thread safe.
This creates a Schematron object which creates a Transformer. And the javadoc says transformer is not thread safe.
So I think the transformer needs to be wrapped in a ThreadLocal or something.
It it does wrap the XMLReader in a ThreadLocal, but it only gets an instance from that ThreadLocal when it initially creates the Schematron object, which is shared among thread. So I don't thin kthe use of the XMLReader is works probably.
Can you verify that I'm actually reading this all correctly, and if so, open a bug to fix this?
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/debugger/InteractiveTraceDebuggerRunner.scala:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.runtime1.debugger
+
+import org.apache.daffodil.api.debugger.InteractiveDebuggerRunner
+
+/*
+ * The below class is empty and is not ever actually used. It is just a place
+ * holder. Whenever the API uses it, it is translated to the appropriate
+ * Scala debugger runner. It is marked final so that it cannot be extended,
+ * since the Scala pattern matcher would still match and use the equivalent
+ * Scala trace runner and lose any added functionality. One must extend the
+ * DebuggerRunner to create their own debugger runner.
+ */ /*
+ * The below class is empty and is not ever actually used. It is just a place
+ * holder. Whenever the API uses it, it is translated to the appropriate
+ * Scala debugger runner. It is marked final so that it cannot be extended,
+ * since the Scala pattern matcher would still match and use the equivalent
+ * Scala trace runner and lose any added functionality. One must extend the
+ * DebuggerRunner to create their own debugger runner.
+ */
+
+/**
+ * `DebuggerRunner` that writes verbose information about parsing to stdout
+ */
+final class InteractiveTraceDebuggerRunner extends InteractiveDebuggerRunner {
Review Comment:
Why is everything empty here? It doesn't look like it is doing any "trace" logic?
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/ProcessorStateBases.scala:
##########
@@ -153,7 +156,7 @@ abstract class ParseOrUnparseState protected (
def this(
vmap: VariableMap,
- diags: List[Diagnostic],
+ diags: java.util.List[api.Diagnostic],
Review Comment:
Does this change anything by switching from a Scala list to a java List? Scala lists are immutable so prepend is constant. It also means we can easily drop diagnostics when we backtrack. Neither of those are the case with a Java list. So prepends will be O(N) , which could incur signfiicant overhead. I'm also wondering if this breaks backtracing so we may end up seeing diagnostics that shoudl have been removed when backtracking?
##########
daffodil-core/src/test/scala/org/apache/daffodil/core/infoset/TestInfosetFree.scala:
##########
@@ -68,15 +71,15 @@ object TestInfosetFree {
// events. We must walk the infoset again after the parse is complete
val parseInput = InputSourceDataInputStream(bytes)
val parseOutputter = new ScalaXMLInfosetOutputter()
- val parseResult = dp.parse(parseInput, parseOutputter)
+ val parseResult = dp.parse(parseInput, parseOutputter).asInstanceOf[ParseResult]
if (parseResult.isError) {
val msgs = parseResult.getDiagnostics.map { _.getMessage() }.mkString("\n")
fail("parse errors: " + msgs)
}
val unparseInputter = new ScalaXMLInfosetInputter(parseOutputter.getResult())
val unparseOutput = Channels.newChannel(NullOutputStream.INSTANCE)
- val unparseResult = dp.unparse(unparseInputter, unparseOutput)
+ val unparseResult = dp.unparse(unparseInputter, unparseOutput).asInstanceOf[UnparseResult]
Review Comment:
Why do thse Parse/Unparse results need asinstanceOf? Are we missing something that needs to be public?
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -937,8 +956,8 @@ abstract class TestCase(testCaseXML: NodeSeq, val parent: DFDLTestSuite) {
}
val useSerializedProcessor =
- if (validationMode == ValidationMode.Full) false
- else if (defaultValidationMode == ValidationMode.Full) false
+ if (validatorName == "xerces") false
+ else if (defaultValidatorName == "xerces") false
Review Comment:
In theory could someone set the validation="schematron", which shoudl also disable serailized processor? Shoudl we reverse this logic so we use a serialized processor the the validator is not off or limited?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2198520165
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/validation/XercesValidator.scala:
##########
@@ -71,23 +76,30 @@ object XercesValidatorFactory {
* to do a validation pass on the TDML expected Infoset w.r.t. the model and to
* do a validation pass on the actual result w.r.t. the model as an XML document.
*/
-class XercesValidator(schemaSources: Seq[javax.xml.transform.Source]) extends Validator {
+class XercesValidator(schemaSource: javax.xml.transform.Source)
+ extends api.validation.Validator {
private val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
private val resolver = DFDLCatalogResolver.get
factory.setResourceResolver(resolver)
- private val schema = factory.newSchema(schemaSources.toArray)
+ private val schema = factory.newSchema(schemaSource)
private val validator = new ThreadLocal[XercesValidatorImpl] {
override def initialValue(): XercesValidatorImpl =
initializeValidator(schema.newValidator, resolver)
}
- def validateXML(document: java.io.InputStream): ValidationResult =
- validateXML(document, new XercesErrorHandler)
+ def validateXML(
+ document: java.io.InputStream,
+ validationHandler: api.validation.ValidationHandler
+ ): Unit =
+ validateXML(document, new XercesErrorHandler(validationHandler))
- def validateXML(document: java.io.InputStream, eh: ErrorHandler): ValidationResult = {
+ def validateXML(
+ document: java.io.InputStream,
+ eh: ErrorHandler
+ ): Unit = {
Review Comment:
We use it in the DaffodilXMLLoader, the errorHandler passed in is a sax errorhandler
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074091574
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/debugger/TraceDebuggerRunner.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.debugger;
+
+import org.apache.daffodil.runtime1.debugger.InteractiveDebugger;
+
+/**
+ * {@link DebuggerRunner} that writes verbose information about parsing to stdout
+ */
+public class TraceDebuggerRunner implements InteractiveDebuggerRunner {
+ public void init() {
+ // do nothing
+ }
+
+ public void init(InteractiveDebugger id) {
+ // do nothing
+ }
+
+ public String getCommand() {
+ return "";
+ }
+
+ public void lineOutput(String line) {
+ // do nothing
Review Comment:
This is the comment associated with that class in the original japi/sapi
```scala
/*
* The below class is empty and is not ever actually used. It is just a place
* holder. Whenever the Java API uses it, it is translated to the appropriate
* Scala debugger runner. It is marked final so that it cannot be extended,
* since the Scala pattern matcher would still match and use the equivalent
* Scala trace runner and lose any added functionality. One must extend the
* DebuggerRunner to create their own debugger runner.
*/
```
As such, I've moved it and renamed it to InteractiveTraceDebuggerRunner and it is final and only used for pattern matching
It is supposed to do nothing, the actually TraceDebuggerRunner does print output
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2210741106
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -937,8 +956,8 @@ abstract class TestCase(testCaseXML: NodeSeq, val parent: DFDLTestSuite) {
}
val useSerializedProcessor =
- if (validationMode == ValidationMode.Full) false
- else if (defaultValidationMode == ValidationMode.Full) false
+ if (validatorName == "xerces") false
+ else if (defaultValidatorName == "xerces") false
Review Comment:
Hmm, reversing the logic resulted in failures in our tests since we have to check all the condition before we hit the default true case, but if we switch the logic, we don't end up checking defaultValidationMode...so I left it as is, and updated the validator ticket
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2191061490
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -1213,9 +1242,10 @@ class Main(
validate
)
}
- }.map {
- _.withExternalVariables(combineExternalVariables(parseOpts.vars, optDafConfig))
- }.map { _.withValidationMode(validate) }
+ }.map { p =>
+ p.asInstanceOf[DFDL.DataProcessor]
Review Comment:
Sort of, our api doesn't expose setting variables using a Seq[Binding]...that's internal only
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2195784464
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Result.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import java.util.List;
+
+/**
+ * Interface for Parse and Unparse results
+ */
+public interface Result {
+ State resultState();
Review Comment:
Hmm, so looked into this, and we use ResultState in a bunch of places outside tests. Not sure if it's worth the effort to change to internal api in those places
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2195877922
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/Implicits.scala:
##########
@@ -44,6 +47,29 @@ object Implicits {
): BufferedInputStream =
new BufferedInputStream(bais)
+ implicit def JListToSeq[T](l: java.util.List[T]): Seq[T] = {
+ l.asScala.toSeq
+ }
+ implicit def SeqToJList[T](s: Seq[T]): java.util.List[T] = {
+ s.asJava
+ }
+
+ implicit def OptionalToOption[T](j: Optional[T]): Option[T] = {
+ j.toScala
+ }
+
+ implicit def OptionToOptional[T](s: Option[T]): Optional[T] = {
+ s.toJava
+ }
+
+ implicit def JMapToSMap[K, V](j: java.util.Map[K, V]): Map[K, V] = {
+ j.asScala.toMap
+ }
+
+ implicit def SMapToJMap[K, V](s: Map[K, V]): java.util.Map[K, V] = {
+ s.asJava
+ }
Review Comment:
DAFFODIL-3008
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2211400381
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/util/TestUtils.scala:
##########
@@ -22,30 +22,38 @@ import java.io.InputStream
import java.nio.channels.Channels
import java.nio.channels.ReadableByteChannel
import java.nio.channels.WritableByteChannel
+import java.nio.charset.Charset
+import java.nio.file.Files
+import java.util
+import java.util.Properties
import scala.collection.mutable.ArrayBuffer
+import scala.jdk.CollectionConverters._
import scala.util.Try
import scala.xml._
+import org.apache.daffodil.api
+import org.apache.daffodil.api.ProcessorFactory
+import org.apache.daffodil.api.debugger.InteractiveDebuggerRunnerFactory
+import org.apache.daffodil.api.metadata.MetadataHandler
+import org.apache.daffodil.api.validation.Validators
import org.apache.daffodil.core.compiler.Compiler
-import org.apache.daffodil.core.compiler.ProcessorFactory
import org.apache.daffodil.core.dsom._
import org.apache.daffodil.io.InputSourceDataInputStream
import org.apache.daffodil.lib.Implicits._
-import org.apache.daffodil.lib.api._
import org.apache.daffodil.lib.exceptions.MultiException
import org.apache.daffodil.lib.externalvars.Binding
+import org.apache.daffodil.lib.iapi._
import org.apache.daffodil.lib.util._
import org.apache.daffodil.lib.xml._
Review Comment:
I created DAFFODIL-3011
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT merged PR #1479:
URL: https://github.com/apache/daffodil/pull/1479
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2240502285
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/infoset/JDOMInfosetInputter.scala:
##########
@@ -109,16 +110,16 @@ class JDOMInfosetInputter(doc: Document) extends InfosetInputter {
text
}
- override def isNilled(): MaybeBoolean = {
+ override def isNilled(): Optional[JBoolean] = {
Review Comment:
See https://issues.apache.org/jira/browse/DAFFODIL-3015
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070477864
##########
build.sbt:
##########
@@ -28,20 +28,13 @@ lazy val genTunablesDoc = taskKey[Seq[File]]("Generate tunables doc from dafext.
lazy val daffodil = project
.in(file("."))
- .enablePlugins(JavaUnidocPlugin, ScalaUnidocPlugin)
+ .enablePlugins(ScalaUnidocPlugin)
.aggregate(
cli,
codeGenC,
- core,
- io,
- japi,
- lib,
Review Comment:
Do you remember any examples of the circular dependencies? Might help guide if there's an alternative structuring that with a different API. Though, maybe there is and it's just too much work so wasn't considered viable?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074072099
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/Diagnostic.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import scala.Option;
+import scala.collection.immutable.Seq;
+
+
+/**
+ * Class containing diagnostic information
+ */
+public abstract class Diagnostic extends Exception {
Review Comment:
Yeah, not worth it. We should just document in javadoc that users are not expected to extend this class.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074096269
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/infoset/BlobMethods.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.infoset;
+
+import scala.collection.immutable.Seq;
+
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+/**
+ * An available basic implementation of the BLOB methods.
+ * Stores blobs in files in directory identified by Java system property
+ * `java.io.tempdir`.
+ */
+public abstract class BlobMethods {
+
+ private Path blobDirectory = Paths.get(System.getProperty("java.io.tmpdir"));
+ private String blobPrefix = "daffodil-";
+ private String blobSuffix = ".blob";
+ private Seq<Path> blobPaths;
+
+ /**
+ * Set the attributes for how to create blob files.
+ *
+ * @param dir the Path the the directory to create files. If the directory
+ * does not exist, Daffodil will attempt to create it before
+ * writing a blob.
+ * @param prefix the prefix string to be used in generating a blob file name
+ * @param suffix the suffix string to be used in generating a blob file name
+ */
+ public void setBlobAttributes(Path dir, String prefix, String suffix) {
Review Comment:
Because of the way we handle InfosetOutpuuter now (with it taking as argument api.infoset.InfosetOuputter), we actually need to override these functions in InfosetOuputter so it has the correct values for blobAttributes, paths, etc, otherwise it will always use the default values for them instead of the api.infoset.InfosetOutputter values
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070465720
##########
build.sbt:
##########
@@ -28,20 +28,13 @@ lazy val genTunablesDoc = taskKey[Seq[File]]("Generate tunables doc from dafext.
lazy val daffodil = project
.in(file("."))
- .enablePlugins(JavaUnidocPlugin, ScalaUnidocPlugin)
+ .enablePlugins(ScalaUnidocPlugin)
.aggregate(
cli,
codeGenC,
- core,
- io,
- japi,
- lib,
Review Comment:
We were running into circular dependency issues again where things in one module needed the new API in runtime1, but the new API also needed things in that submodule. After a convo with Mike, collapsing them all into the same module seemed like the most viable option. I haven't noticed a degradation in compilation time (though tbh I wasn't really looking for it)
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2073920976
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/api/Layer.java:
##########
Review Comment:
Since this is actually java code...I decided to move it to the java directory and now it lives in the org.apache.daffodil.api.layers package
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074002326
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/Diagnostic.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import scala.Option;
+import scala.collection.immutable.Seq;
+
+
+/**
+ * Class containing diagnostic information
+ */
+public abstract class Diagnostic extends Exception {
Review Comment:
So this results in the new interface api.Diagnostic not being able to extend Exception (which is a concrete class), making Diagnostic not a throwable. The workaround to this is just referencing DiagnosticBase everywhere we currently reference api.Diagnostic, but I'm not sure the advantage we get for such added complexity?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074223812
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/infoset/BlobMethods.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.infoset;
+
+import scala.collection.immutable.Seq;
+
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+/**
+ * An available basic implementation of the BLOB methods.
+ * Stores blobs in files in directory identified by Java system property
+ * `java.io.tempdir`.
+ */
+public abstract class BlobMethods {
+
+ private Path blobDirectory = Paths.get(System.getProperty("java.io.tmpdir"));
+ private String blobPrefix = "daffodil-";
+ private String blobSuffix = ".blob";
+ private Seq<Path> blobPaths;
+
+ /**
+ * Set the attributes for how to create blob files.
+ *
+ * @param dir the Path the the directory to create files. If the directory
+ * does not exist, Daffodil will attempt to create it before
+ * writing a blob.
+ * @param prefix the prefix string to be used in generating a blob file name
+ * @param suffix the suffix string to be used in generating a blob file name
+ */
+ public void setBlobAttributes(Path dir, String prefix, String suffix) {
Review Comment:
@stevedlawrence , no we still use InfosetOutputter in a few places ex DataProcessor.scala and PState.scala . SHould I look into replacing it with api.InfosetOutputter?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070574619
##########
build.sbt:
##########
@@ -28,20 +28,13 @@ lazy val genTunablesDoc = taskKey[Seq[File]]("Generate tunables doc from dafext.
lazy val daffodil = project
.in(file("."))
- .enablePlugins(JavaUnidocPlugin, ScalaUnidocPlugin)
+ .enablePlugins(ScalaUnidocPlugin)
.aggregate(
cli,
codeGenC,
- core,
- io,
- japi,
- lib,
Review Comment:
But in general the separation of core from runtime1 was not properly layered from the earliest days of this project. I'm culpable for much of that as I *think* I set up this Compiler -> ProcessorFactory -> DataProcessor stuff in 2010 before I even joined Owl/Tresys.
I'm not opposed to merging core and runtime1 and runtime1-unparsers together, along with daffodil-lib even.
Scala 3's requirement that the top of a class taxonomy must be a Java interface if Java is to be able to call it,... requires a huge amount of refactoring that we didn't anticipate.
We could avoid this by implement the java api by way of delegation (aka proxy objects) but as with our prior japi/sapi, that's painful to maintain. I very much prefer to just define java interfaces at the top, and our classes implement the user API.
Splitting this back out into independent separable modules is a bunch of additional work, which is necessary eventually to support goals like non-XSD-based syntax for DFDL, and to properly support DFDL's DPath expressions in the C-code-generator, and allow for other backends than runtime1, and not require runtime1 when creating a different runtime like the C-code-generator. We have JIRA tix for that sort of work. But that kind of work is a substantial rewrite of large aspects of daffodil.
But the point I guess is until that refactoring work is done, having core, runtime1, runtime1-unparsers and lib as separate modules doesn't really add any value for users. They all ultimately depend on core which depends on the others so it's all pulled in anyway currently.
And the split-out modules makes the scala 3 new Java API either impossible or quite difficult.
So I'm not opposed to merging them for now.
I suggest this big merged module NOT be named "runtime1". But rather "core" is the name we should carry forward.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2211538116
##########
daffodil-core/src/test/resources/test/api/01very_simple.txt:
##########
@@ -1,11 +1,11 @@
-Date: xsw, 15 Feb 2013 08:54:52 -0500
-From: john@doe.com
-To: jane@doe.com
-Message-ID: <OF...@doe.com>
-Subject: Hello World!
-Return-Path: <bo...@smith.com>
-Thread-Index: Ac3NbaI+1oo+ltP4SSqsh0WAhpIVagAAZVmwAALaigABACxfkAAPsjwAAA8SeAAABZb0A==
-User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20120828 Thunderbird/16.0 Lightning/1.8b1
-Content-Type: multipart/mixed; boundary=gc0p4Jq0M2Yt08jU534c0p
-
+Date: xsw, 15 Feb 2013 08:54:52 -0500
+From: john@doe.com
+To: jane@doe.com
+Message-ID: <OF...@doe.com>
+Subject: Hello World!
+Return-Path: <bo...@smith.com>
+Thread-Index: Ac3NbaI+1oo+ltP4SSqsh0WAhpIVagAAZVmwAALaigABACxfkAAPsjwAAA8SeAAABZb0A==
+User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20120828 Thunderbird/16.0 Lightning/1.8b1
+Content-Type: multipart/mixed; boundary=gc0p4Jq0M2Yt08jU534c0p
+
Review Comment:
Actually just a file rename from runtime1 to core
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2191057829
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -445,8 +465,8 @@ class CLIConf(arguments: Array[String], stdout: PrintStream, stderr: PrintStream
}
validateOpt(parser, validate) {
- case (Some(_), Some(ValidationMode.Full)) =>
- Left("The validation mode must be 'limited' or 'off' when using a saved parser.")
+ case (Some(_), Some(null)) =>
Review Comment:
Created DAFFODIL-3007
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2208184302
##########
daffodil-core/src/main/java/org/apache/daffodil/api/validation/Validators.java:
##########
@@ -15,49 +15,61 @@
* limitations under the License.
*/
-package org.apache.daffodil.lib.validation
+package org.apache.daffodil.api.validation;
-import org.apache.daffodil.lib.api.ValidatorFactory
-import org.apache.daffodil.lib.util.SimpleNamedServiceLoader
+import org.apache.daffodil.util.SimpleNamedServiceLoader;
+
+import java.util.Optional;
/**
- * Access SPI registered [[org.apache.daffodil.lib.api.ValidatorFactory]] instances.
- *
+ * Access SPI registered [[org.apache.daffodil.api.validation.ValidatorFactory]] instances.
+ * <p>
* Registered instances provide a unique name for lookup.
*/
-object Validators {
- private lazy val impls: Map[String, ValidatorFactory] = {
- SimpleNamedServiceLoader.loadClass[ValidatorFactory](classOf[ValidatorFactory])
+public class Validators {
+ private static final Validators INSTANCE = new Validators();
+ private final java.util.Map<String, ValidatorFactory> impls =
+ SimpleNamedServiceLoader.loadClass(ValidatorFactory.class);
+
+ private Validators() {
+ } // private constructor
+
+ public static Validators getInstance() {
Review Comment:
We actually don't need INSTANCE/getInstance anymore since all the classes are static
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074090443
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/InputSourceDataInputStream.java:
##########
@@ -15,89 +15,71 @@
* limitations under the License.
*/
-package org.apache.daffodil.japi.io
+package org.apache.daffodil.api;
-import java.io.InputStream
-import java.nio.ByteBuffer
+import java.io.Closeable;
+import java.io.IOException;
-import org.apache.daffodil.io.{ InputSourceDataInputStream => SInputSourceDataInputStream }
/**
* Provides Daffodil with byte data from an InputStream, ByteBuffer, or byte
* Array.
- *
+ * <p>
* Note that the InputStream variant has potential overhead due to streaming capabilities and
* support for files greater than 2GB. In some cases, better performance might come from using
* the byte array or ByteBuffer variants instead. For example, if your data is already in a byte
* array, one should use the Array[Byte] or ByteBuffer variants instead of wrapping it in a
* ByteArrayInputStream. As another example, instead of using a FileInputStream like this:
- *
- * {{{
+ * <p>
+ * {@code
* Path path = Paths.get(file);
* FileInputStream fis = Files.newInputStream(path);
* InputSourceDataInputStream input = InputSourceDataInputStream(fis);
- * }}}
- *
+ * }
+ * <p>
* You might consider mapping the file to a MappedByteBuffer like below, keeping in mind that
* MappedByteBuffers have size limitations and potentially different performance characteristics
* depending on the file size and system--it maybe not always be faster than above.
- *
- * {{{
+ * <p>
+ * {@code
* Path path = Paths.get(file);
* long size = Files.size(path);
* FileChannel fc = FileChannel.open(path, StandardOpenOption.READ);
* ByteBuffer bb = fc.map(FileChannel.MapMode.READ_ONLY, 0, size);
* fc.close();
* InputSourceDataInputStream input = new InputSourceDataInputStream(bb);
- * }}}
+ * }
*/
-class InputSourceDataInputStream private[japi] (
- private[japi] val dis: SInputSourceDataInputStream
-) extends java.io.Closeable {
-
- /**
- * Create an InputSourceDataInputStream from a java.io.InputStream
- */
- def this(is: InputStream) = this(SInputSourceDataInputStream(is))
-
- /**
- * Create an InputSourceDataInputStream from a java.nio.ByteBuffer
- */
- def this(bb: ByteBuffer) = this(SInputSourceDataInputStream(bb))
-
- /**
- * Create an InputSourceDataInputStream from a byte array
- */
- def this(arr: Array[Byte]) = this(SInputSourceDataInputStream(arr))
-
+public abstract class InputSourceDataInputStream implements Closeable {
Review Comment:
This is the comment associated with that class in the original japi/sapi
/*
* The below class is empty and is not ever actually used. It is just a place
* holder. Whenever the Java API uses it, it is translated to the appropriate
* Scala debugger runner. It is marked final so that it cannot be extended,
* since the Scala pattern matcher would still match and use the equivalent
* Scala trace runner and lose any added functionality. One must extend the
* DebuggerRunner to create their own debugger runner.
*/
As such, I've moved it and renamed it to InteractiveTraceDebuggerRunner and it is final and only used for pattern matching
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2208101034
##########
daffodil-core/src/test/scala/org/apache/daffodil/core/infoset/TestInfosetFree.scala:
##########
@@ -68,15 +71,15 @@ object TestInfosetFree {
// events. We must walk the infoset again after the parse is complete
val parseInput = InputSourceDataInputStream(bytes)
val parseOutputter = new ScalaXMLInfosetOutputter()
- val parseResult = dp.parse(parseInput, parseOutputter)
+ val parseResult = dp.parse(parseInput, parseOutputter).asInstanceOf[ParseResult]
if (parseResult.isError) {
val msgs = parseResult.getDiagnostics.map { _.getMessage() }.mkString("\n")
fail("parse errors: " + msgs)
}
val unparseInputter = new ScalaXMLInfosetInputter(parseOutputter.getResult())
val unparseOutput = Channels.newChannel(NullOutputStream.INSTANCE)
- val unparseResult = dp.unparse(unparseInputter, unparseOutput)
+ val unparseResult = dp.unparse(unparseInputter, unparseOutput).asInstanceOf[UnparseResult]
Review Comment:
Yes, they use resultState which is now private to the internal DFDL.Parse/UnparseResult
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2198629322
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -113,113 +106,54 @@ object DFDL {
schemaSource: DaffodilSchemaSource,
optRootNodeName: Option[String] = None,
optRootNodeNamespace: Option[String] = None
- ): ProcessorFactory
-
- def reload(savedParser: File): DataProcessor
+ ): api.ProcessorFactory
}
/**
* The point of [[ProcessorFactory]] is to allow compilation of the path expression
* and/or generation of source code to process data matching the compiled schema
*/
- trait ProcessorFactory extends WithDiagnostics {
+ trait ProcessorFactory extends WithDiagnostics with api.ProcessorFactory {
/**
- * Returns a [[DataProcessor]] to process data matching a compiled XPath expression
- * @param xpath XPath expression in DFDL schema that data should match (you can use only "/" at this time)
- */
- def onPath(xpath: String): DataProcessor
-
- /**
- * Returns a [[CodeGenerator]] to generate code from a DFDL schema to parse or unparse data
+ * Returns a [[org.apache.daffodil.api.CodeGenerator]] to generate code from a DFDL schema to parse or unparse data
* @param language source language for generated code (you can use only "c" at this time)
*/
- def forLanguage(language: String): CodeGenerator
+ def forLanguage(language: String): api.CodeGenerator
Review Comment:
Created an @Experimental annotation in the api package, and used that on the new api function forLanguage
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070200497
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -31,17 +31,21 @@ import scala.collection.mutable.ArrayBuffer
import scala.util.Try
import scala.xml.Node
+import org.apache.daffodil.api.compiler.{ ProcessorFactory => JProcessorFactory }
+import org.apache.daffodil.api.exceptions.{ InvalidParserException => JInvalidParserException }
+import org.apache.daffodil.api.{ CodeGenerator => JCodeGenerator }
+import org.apache.daffodil.api.{ DataProcessor => JDataProcessor }
Review Comment:
2 things:
1. Just to make sure I'm tracking the API changes here correctly, we are dropping having separate Scala and Java APIs and instead having a single unified API that is Java compatible?
2. Assuming 1 is correct do we really want to be prefixing everything with "J"? It made sense when there was separate APIs but unless there is some sort of loss of functionality when using the new API from Scala. I'm open to ideas on this but just thought it might not be necessary.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070459346
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/ValidationMode.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+/**
+ * Validation modes for validating the resulting infoset against the DFDL schema
+ */
+public enum ValidationMode {
+ /**
+ * Turn off all validation against the DFDL schema
+ */
+ Off(10),
Review Comment:
Hmm, we ended up getting rid of ValidationMode enums, but I based it off the Scala one which was not all caps iirc
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070737958
##########
build.sbt:
##########
@@ -28,20 +28,13 @@ lazy val genTunablesDoc = taskKey[Seq[File]]("Generate tunables doc from dafext.
lazy val daffodil = project
.in(file("."))
- .enablePlugins(JavaUnidocPlugin, ScalaUnidocPlugin)
+ .enablePlugins(ScalaUnidocPlugin)
.aggregate(
cli,
codeGenC,
- core,
- io,
- japi,
- lib,
Review Comment:
> value in creating a daffodil-charset and daffodil-layer directory/jar that contains the interfaces for building custom layers and charsets, similar to what we have with daffodil-udf? Or maybe we have a daffodil-plugins that contains all the udf, layer, and charset interfaces? Or do we want to merge daffodil-udf into daffodil-core so no matter what you're doing, you just depend on daffodil-core? I have no strong preference for either approach, but it seems a inconsistent to have udf be it's own jar and other plugins be part of daffodil-core.
Created DAFFODIL-2994
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070719689
##########
build.sbt:
##########
@@ -28,20 +28,13 @@ lazy val genTunablesDoc = taskKey[Seq[File]]("Generate tunables doc from dafext.
lazy val daffodil = project
.in(file("."))
- .enablePlugins(JavaUnidocPlugin, ScalaUnidocPlugin)
+ .enablePlugins(ScalaUnidocPlugin)
.aggregate(
cli,
codeGenC,
- core,
- io,
- japi,
- lib,
Review Comment:
Re io, it depended on lib, which had to be integrated into runtime1, so now it dependend on runtime1, but core, whcih depended on io, had to be integrated into runtime1, which now meant that runtime1 depended on io...which was the circular dependency issue for io, which was why it needed to be integrated.
I shall rename to core.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2240502285
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/infoset/JDOMInfosetInputter.scala:
##########
@@ -109,16 +110,16 @@ class JDOMInfosetInputter(doc: Document) extends InfosetInputter {
text
}
- override def isNilled(): MaybeBoolean = {
+ override def isNilled(): Optional[JBoolean] = {
Review Comment:
SeeDAFFODIL-3015
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/infoset/JDOMInfosetInputter.scala:
##########
@@ -109,16 +110,16 @@ class JDOMInfosetInputter(doc: Document) extends InfosetInputter {
text
}
- override def isNilled(): MaybeBoolean = {
+ override def isNilled(): Optional[JBoolean] = {
Review Comment:
See DAFFODIL-3015
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2208020719
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/ProcessorStateBases.scala:
##########
@@ -153,7 +156,7 @@ abstract class ParseOrUnparseState protected (
def this(
vmap: VariableMap,
- diags: List[Diagnostic],
+ diags: java.util.List[api.Diagnostic],
Review Comment:
In the case of a LinkedList, adds/removes are O(1), so I changed tge diagnostics to use a linkedList when being updated
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2075268070
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/infoset/BlobMethods.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.infoset;
+
+import scala.collection.immutable.Seq;
+
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+/**
+ * An available basic implementation of the BLOB methods.
+ * Stores blobs in files in directory identified by Java system property
+ * `java.io.tempdir`.
+ */
+public abstract class BlobMethods {
+
+ private Path blobDirectory = Paths.get(System.getProperty("java.io.tmpdir"));
+ private String blobPrefix = "daffodil-";
+ private String blobSuffix = ".blob";
+ private Seq<Path> blobPaths;
+
+ /**
+ * Set the attributes for how to create blob files.
+ *
+ * @param dir the Path the the directory to create files. If the directory
+ * does not exist, Daffodil will attempt to create it before
+ * writing a blob.
+ * @param prefix the prefix string to be used in generating a blob file name
+ * @param suffix the suffix string to be used in generating a blob file name
+ */
+ public void setBlobAttributes(Path dir, String prefix, String suffix) {
Review Comment:
Yeah, if an internal class doesn't provide any new functionality over the public API class and is just a proxy/wrapper I would suggest we get rid of it and directly reference the `api` interface. It just adds a level of indirection for no real benefit. I believe that is the case with InfosetOutputter.
There are cases where the internal class does provide extra functions (e.g. DaffodilUnparseContentHandler provies enableResolutionOfRelativeInfosetBlobURIs), but I don't think our internal InfosetOutputter does that.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074090443
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/InputSourceDataInputStream.java:
##########
@@ -15,89 +15,71 @@
* limitations under the License.
*/
-package org.apache.daffodil.japi.io
+package org.apache.daffodil.api;
-import java.io.InputStream
-import java.nio.ByteBuffer
+import java.io.Closeable;
+import java.io.IOException;
-import org.apache.daffodil.io.{ InputSourceDataInputStream => SInputSourceDataInputStream }
/**
* Provides Daffodil with byte data from an InputStream, ByteBuffer, or byte
* Array.
- *
+ * <p>
* Note that the InputStream variant has potential overhead due to streaming capabilities and
* support for files greater than 2GB. In some cases, better performance might come from using
* the byte array or ByteBuffer variants instead. For example, if your data is already in a byte
* array, one should use the Array[Byte] or ByteBuffer variants instead of wrapping it in a
* ByteArrayInputStream. As another example, instead of using a FileInputStream like this:
- *
- * {{{
+ * <p>
+ * {@code
* Path path = Paths.get(file);
* FileInputStream fis = Files.newInputStream(path);
* InputSourceDataInputStream input = InputSourceDataInputStream(fis);
- * }}}
- *
+ * }
+ * <p>
* You might consider mapping the file to a MappedByteBuffer like below, keeping in mind that
* MappedByteBuffers have size limitations and potentially different performance characteristics
* depending on the file size and system--it maybe not always be faster than above.
- *
- * {{{
+ * <p>
+ * {@code
* Path path = Paths.get(file);
* long size = Files.size(path);
* FileChannel fc = FileChannel.open(path, StandardOpenOption.READ);
* ByteBuffer bb = fc.map(FileChannel.MapMode.READ_ONLY, 0, size);
* fc.close();
* InputSourceDataInputStream input = new InputSourceDataInputStream(bb);
- * }}}
+ * }
*/
-class InputSourceDataInputStream private[japi] (
- private[japi] val dis: SInputSourceDataInputStream
-) extends java.io.Closeable {
-
- /**
- * Create an InputSourceDataInputStream from a java.io.InputStream
- */
- def this(is: InputStream) = this(SInputSourceDataInputStream(is))
-
- /**
- * Create an InputSourceDataInputStream from a java.nio.ByteBuffer
- */
- def this(bb: ByteBuffer) = this(SInputSourceDataInputStream(bb))
-
- /**
- * Create an InputSourceDataInputStream from a byte array
- */
- def this(arr: Array[Byte]) = this(SInputSourceDataInputStream(arr))
-
+public abstract class InputSourceDataInputStream implements Closeable {
Review Comment:
This is the comment associated with that class in the original japi/sapi
/*
* The below class is empty and is not ever actually used. It is just a place
* holder. Whenever the Java API uses it, it is translated to the appropriate
* Scala debugger runner. It is marked final so that it cannot be extended,
* since the Scala pattern matcher would still match and use the equivalent
* Scala trace runner and lose any added functionality. One must extend the
* DebuggerRunner to create their own debugger runner.
*/
As such, I've moved it and renamed it to InteractiveTraceDebuggerRunner and it is final and only used for pattern matching
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074125443
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/infoset/BlobMethods.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.infoset;
+
+import scala.collection.immutable.Seq;
+
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+/**
+ * An available basic implementation of the BLOB methods.
+ * Stores blobs in files in directory identified by Java system property
+ * `java.io.tempdir`.
+ */
+public abstract class BlobMethods {
+
+ private Path blobDirectory = Paths.get(System.getProperty("java.io.tmpdir"));
+ private String blobPrefix = "daffodil-";
+ private String blobSuffix = ".blob";
+ private Seq<Path> blobPaths;
+
+ /**
+ * Set the attributes for how to create blob files.
+ *
+ * @param dir the Path the the directory to create files. If the directory
+ * does not exist, Daffodil will attempt to create it before
+ * writing a blob.
+ * @param prefix the prefix string to be used in generating a blob file name
+ * @param suffix the suffix string to be used in generating a blob file name
+ */
+ public void setBlobAttributes(Path dir, String prefix, String suffix) {
Review Comment:
It looks like the new InfosetOutputer is just a wrapper around `api.InfosetOutputter`. Does that mean we don't actually need the infoset.InfosetOutputter and we can pass around and use `api.InfosetOutputter` directly?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2228956439
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/infoset/JDOMInfosetInputter.scala:
##########
@@ -109,16 +110,16 @@ class JDOMInfosetInputter(doc: Document) extends InfosetInputter {
text
}
- override def isNilled(): MaybeBoolean = {
+ override def isNilled(): Optional[JBoolean] = {
Review Comment:
You're right. It looks like Daffodil internally differentiates between `None` and `Some(false)` in InfosetInputter.scala like this:
```scala
val optNilled = delegate.isNilled.toScala
if (optNilled.isDefined) {
if (!erd.isNillable) {
UnparseError(
One(elem.erd.schemaFileLocation),
Nope,
"Element %s defines nil property, but is not nillable",
erd.namedQName.toExtendedSyntax
)
}
if (optNilled.get) {
elem.setNilled()
}
}
```
So this means is it is illegal to specify xsi:nil on an element that isn't nillable, even if you set `xsi:nil="false"`.
And this seems to agree with what I found in the XML Schema spec regarding local validity:
> Informally, an element is locally valid against an element declaration when:
> ...
> 3. Any xsi:nil attribute on the element obeys the rules. The element is allowed to have an xsi:nil attribute only if the element is declared nillable, and xsi:nil = 'true' is allowed only if the element itself is empty. ...
So an element should be marked as invalid if it is not nullable but has xsi:nil set, even if xsi:nil is false. Seems an odd decision to me, but that's what the spec says.
So I think this does confirm we cannot simplify the isNilled() API to just return a Boolean, and we need an Optional[Boolean] to be able to check for validity. Though, I'm not sure an UnparseError is the correct thing--seems if isNillable is false it should be a UE if optNilled.get is true and a validation error if optNilled.get is false (note that unparse does not currently support validation).
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2211558109
##########
daffodil-core/src/test/resources/test/api/01very_simple.txt:
##########
@@ -1,11 +1,11 @@
-Date: xsw, 15 Feb 2013 08:54:52 -0500
-From: john@doe.com
-To: jane@doe.com
-Message-ID: <OF...@doe.com>
-Subject: Hello World!
-Return-Path: <bo...@smith.com>
-Thread-Index: Ac3NbaI+1oo+ltP4SSqsh0WAhpIVagAAZVmwAALaigABACxfkAAPsjwAAA8SeAAABZb0A==
-User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20120828 Thunderbird/16.0 Lightning/1.8b1
-Content-Type: multipart/mixed; boundary=gc0p4Jq0M2Yt08jU534c0p
-
+Date: xsw, 15 Feb 2013 08:54:52 -0500
+From: john@doe.com
+To: jane@doe.com
+Message-ID: <OF...@doe.com>
+Subject: Hello World!
+Return-Path: <bo...@smith.com>
+Thread-Index: Ac3NbaI+1oo+ltP4SSqsh0WAhpIVagAAZVmwAALaigABACxfkAAPsjwAAA8SeAAABZb0A==
+User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20120828 Thunderbird/16.0 Lightning/1.8b1
+Content-Type: multipart/mixed; boundary=gc0p4Jq0M2Yt08jU534c0p
+
Review Comment:
(atleast intellij didn't pick up any line ending changes)
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2210907518
##########
daffodil-cli/src/test/scala/org/apache/daffodil/cli/cliTest/schematron/TestValidating.scala:
##########
@@ -67,9 +63,7 @@ class TestValidating {
val schematron = path("daffodil-schematron/src/test/resources/sch/missing-include.sch")
val input = path("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/uuid.txt")
- runCLI(args"""parse --validate schematron="${jsonEscape(
- schematron.toString
- )}" -s $schema $input""") { cli =>
+ runCLI(args"""parse --validate schematron=$schematron -s $schema $input""") { cli =>
cli.expectErr("Bad arguments")
Review Comment:
Is there a reason we no longer need the jsonEscape()'s? Just doesn't seem obvious why its no longer needed.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/ParseResult.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+/**
+ * Result of calling {@code DataProcessor.parse(input:org\.apache\.daffodil* DataProcessor.parse}, containing
+ * any diagnostic information, and the final data location
+ */
+public interface ParseResult extends Result, WithDiagnostics {
Review Comment:
Ah, were we using DFDL.ParseResult earlier because we were interacting with the Scala API and not the Java one? Feels like we should be consistent if possible.
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/DaffodilCCodeGenerator.scala:
##########
@@ -78,10 +79,10 @@ import org.apache.daffodil.runtime1.dsom.SchemaDefinitionWarning
*
* @param root Passes DFDL schema from which to generate code
*/
-class DaffodilCCodeGenerator(root: Root) extends DFDL.CodeGenerator {
+class DaffodilCCodeGenerator(root: Root) extends api.CodeGenerator {
// Note this class is not thread-safe due to mutable state needed to
// implement WithDiagnostics trait
- private var diagnostics: Seq[Diagnostic] = Nil
+ private var diagnostics: java.util.List[api.Diagnostic] = util.Collections.emptyList()
Review Comment:
I think we should be consistent with either using java.util.xxx or util.xxx. Combining both approaches, especiallhy on the same line seems really strange to me.
My preference would be to just use java.util.xxx to make it clear we are using classes from Java and not something from Scala.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -229,25 +153,24 @@ object DFDL {
* can be used to get the result and any diagnostics.
*/
class DaffodilUnparseErrorSAXException(unparseResult: UnparseResult)
- extends SAXException(unparseResult.getDiagnostics.mkString("\n"))
+ extends api.exceptions.DaffodilUnparseErrorSAXException(
+ unparseResult.getDiagnostics.asScala.mkString("\n")
Review Comment:
I've seen these .asScala.mkString("\n") in a few places. Wonder if we'd be better off just keeping it java and doing String.join("\n", unparseResult.getDiagnostics)
Curious to see if there is a significant performance impact from all the java/scala collection transformations.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -315,8 +319,9 @@ class ChoiceParser(ctxt: RuntimeData, val childParsers: Array[Parser])
}
if (!successfullyParsedChildBranch) {
+ Collections.reverse(diagnostics)
val allDiags =
- new EntireChoiceFailed(context.schemaFileLocation, pstate, diagnostics.reverse)
+ new EntireChoiceFailed(context.schemaFileLocation, pstate, diagnostics)
Review Comment:
Is the reverse no longer needed because we are using java's LinkedList?
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/InfosetTypes.scala:
##########
@@ -246,7 +244,7 @@ sealed trait InfosetHandler {
xmlReader: XMLReader,
input: InputStream,
output: DFDL.Output
- ): UnparseResult = {
+ ): api.UnparseResult = {
Review Comment:
I'm assuming there is a good reason for it, but the asymmetry of having UnparseResult under api.UnparseResult while ParseResult is DFDL.ParseResult hurts my OCD.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -261,36 +184,53 @@ object DFDL {
* Interface for Parse and Unparse states
*/
trait State {
+
+ /**
+ * @return a processorResult detailing the status of the processor
+ */
def processorStatus: ProcessorResult
+
+ /**
+ * @return validation status
+ */
def validationStatus: Boolean
- def diagnostics: Seq[Diagnostic]
+
+ /**
+ * @return list of diagnostics
+ */
+ def diagnostics: util.List[Diagnostic]
+
+ /**
+ * @return current data location
+ */
def currentLocation: DataLocation
}
/**
* Interface for Parse and Unparse results
*/
- abstract class Result {
+ abstract class Result extends api.Result {
def resultState: State
- var diagnostics: Seq[Diagnostic] = Nil
+ var diagnostics: java.util.List[api.Diagnostic] = util.Collections.emptyList()
Review Comment:
More use of "util.xxx". My vote would be to just remove all instances of "import java.util" and use it explicitly whenever it's being called from Scala.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/util/TestUtils.scala:
##########
@@ -22,30 +22,38 @@ import java.io.InputStream
import java.nio.channels.Channels
import java.nio.channels.ReadableByteChannel
import java.nio.channels.WritableByteChannel
+import java.nio.charset.Charset
+import java.nio.file.Files
+import java.util
+import java.util.Properties
import scala.collection.mutable.ArrayBuffer
+import scala.jdk.CollectionConverters._
import scala.util.Try
import scala.xml._
+import org.apache.daffodil.api
+import org.apache.daffodil.api.ProcessorFactory
+import org.apache.daffodil.api.debugger.InteractiveDebuggerRunnerFactory
+import org.apache.daffodil.api.metadata.MetadataHandler
+import org.apache.daffodil.api.validation.Validators
import org.apache.daffodil.core.compiler.Compiler
-import org.apache.daffodil.core.compiler.ProcessorFactory
import org.apache.daffodil.core.dsom._
import org.apache.daffodil.io.InputSourceDataInputStream
import org.apache.daffodil.lib.Implicits._
-import org.apache.daffodil.lib.api._
import org.apache.daffodil.lib.exceptions.MultiException
import org.apache.daffodil.lib.externalvars.Binding
+import org.apache.daffodil.lib.iapi._
import org.apache.daffodil.lib.util._
import org.apache.daffodil.lib.xml._
Review Comment:
One thing that I thought of when I saw this was that I believe they are trying to move to copying Java with doing import org.apache.daffodil.lib.iapi.* in the future. Obviously doesn't need to be something that is done in this commit and I'm sure there are many other places that would need a change too. There probably is a scalaFix rule for it whenever we want to make that change.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/util/TestUtils.scala:
##########
@@ -234,13 +244,17 @@ object TestUtils {
compiler: Compiler,
schemaSource: URISchemaSource,
output: WritableByteChannel
- ): Try[(ProcessorFactory, DFDL.DataProcessor)] = {
+ ): Try[(ProcessorFactory, api.DataProcessor)] = {
Try {
val pf = compiler.compileSource(schemaSource)
if (pf.isError) throwDiagnostics(pf.getDiagnostics)
val dp = pf.onPath("/")
dp.save(output)
- if (dp.isError) throwDiagnostics(dp.getDiagnostics ++ pf.getDiagnostics)
+ if (dp.isError) {
+ val tmp = new util.LinkedList[api.Diagnostic](dp.getDiagnostics)
Review Comment:
Here's another place where I think we should just say "java.util.LinkedList". My first thought when I see util.xxx is that it's something we wrote, not something that comes from a standard library.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/infoset/SAXInfosetInputter.scala:
##########
@@ -104,23 +108,23 @@ class SAXInfosetInputter(
}
}
- override def isNilled(): MaybeBoolean = {
- val _isNilled = if (currentEvent.nilValue.isDefined) {
+ override def isNilled(): Optional[JBoolean] = {
+ val _isNilled: Option[JBoolean] = if (currentEvent.nilValue.isDefined) {
Review Comment:
Like here
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -113,113 +111,39 @@ object DFDL {
schemaSource: DaffodilSchemaSource,
optRootNodeName: Option[String] = None,
optRootNodeNamespace: Option[String] = None
- ): ProcessorFactory
-
- def reload(savedParser: File): DataProcessor
+ ): api.ProcessorFactory
}
/**
* The point of [[ProcessorFactory]] is to allow compilation of the path expression
* and/or generation of source code to process data matching the compiled schema
*/
- trait ProcessorFactory extends WithDiagnostics {
-
- /**
- * Returns a [[DataProcessor]] to process data matching a compiled XPath expression
- * @param xpath XPath expression in DFDL schema that data should match (you can use only "/" at this time)
- */
- def onPath(xpath: String): DataProcessor
-
- /**
- * Returns a [[CodeGenerator]] to generate code from a DFDL schema to parse or unparse data
- * @param language source language for generated code (you can use only "c" at this time)
- */
- def forLanguage(language: String): CodeGenerator
- }
-
- /**
- * Source code generation and compilation is performed with a language-specific [[CodeGenerator]],
- * which must be interrogated for diagnostics to see if each call was successful or not.
- */
- trait CodeGenerator extends WithDiagnostics {
-
- /**
- * Generates language-specific code from a DFDL schema to parse or unparse data
- * @param outputDir output directory in which to create code directory (codeDir)
- * @return path of newly created directory (codeDir) containing generated code
- */
- def generateCode(outputDir: String): os.Path
-
- /**
- * Compiles the generated code in order to run it in a TDML test
- * @param codeDir path of newly created directory containing generated code
- * @return path of newly built executable (exe) compiled from generated code
- */
- def compileCode(codeDir: os.Path): os.Path
- }
-
- trait DataProcessor extends WithDiagnostics {
+ trait ProcessorFactory extends WithDiagnostics with api.ProcessorFactory
- /**
- * Returns a data processor with all the same state, but the validation mode changed to that of the argument.
- *
- * Note that the default validation mode is "off", that is, no validation is performed.
- */
- def withValidationMode(mode: ValidationMode.Type): DataProcessor
- def withTunable(name: String, value: String): DataProcessor
- def withTunables(tunables: Map[String, String]): DataProcessor
- def withExternalVariables(extVars: Map[String, String]): DataProcessor
- def withExternalVariables(extVars: File): DataProcessor
+ trait DataProcessor extends api.DataProcessor with WithDiagnostics {
def withExternalVariables(extVars: Seq[Binding]): DataProcessor
- def withDebugger(dbg: AnyRef): DataProcessor
- def withDebugging(flag: Boolean): DataProcessor
-
- def save(output: DFDL.Output): Unit
-
- def walkMetadata(handler: MetadataHandler): Unit
def tunables: DaffodilTunables
def variableMap: VariableMap
- def validationMode: ValidationMode.Type
-
- /**
- * Creates a new instance of XMLReader for SAX Parsing
- */
- def newXMLReaderInstance: DaffodilParseXMLReader
-
- /**
- * Creates a new instance of DaffodilUnparseContentHandler for SAX Unparsing
- */
- def newContentHandlerInstance(output: DFDL.Output): DaffodilUnparseContentHandler
-
- /**
- * Unparses (that is, serializes) data to the output, returns an object which contains any diagnostics.
- */
- def unparse(input: InfosetInputter, output: DFDL.Output): UnparseResult
-
- /**
- * Returns an object which contains the result, and/or diagnostic information.
- */
- def parse(input: InputSourceDataInputStream, output: InfosetOutputter): ParseResult
}
/**
* Interface of an XMLReader providing additional parse functions to support that
* data that Daffodil usually expects. Used to parse data to a infoset represent as
* SAX events
*/
- trait DaffodilParseXMLReader extends XMLReader {
- def parse(is: java.io.InputStream): Unit
+ trait DaffodilParseXMLReader extends api.DaffodilParseXMLReader {
def parse(in: InputSourceDataInputStream): Unit
- def parse(ab: Array[Byte]): Unit
+ def parse(in: api.InputSourceDataInputStream): Unit = {
+ parse(in.asInstanceOf[InputSourceDataInputStream])
+ }
}
Review Comment:
Why is the .asInstanceOf needed here?
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/infoset/SAXInfosetInputter.scala:
##########
@@ -17,21 +17,25 @@
package org.apache.daffodil.runtime1.infoset
+import java.lang.{ Boolean => JBoolean }
import java.net.URI
import java.net.URISyntaxException
+import java.util.Optional
Review Comment:
Kind of wondering if we don't want to import java.util.Optional and fully specify java.util.Optional where it's being used. It's easy to confuse java's Optional with scala's Option
##########
daffodil-core/src/test/resources/test/api/01very_simple.txt:
##########
@@ -1,11 +1,11 @@
-Date: xsw, 15 Feb 2013 08:54:52 -0500
-From: john@doe.com
-To: jane@doe.com
-Message-ID: <OF...@doe.com>
-Subject: Hello World!
-Return-Path: <bo...@smith.com>
-Thread-Index: Ac3NbaI+1oo+ltP4SSqsh0WAhpIVagAAZVmwAALaigABACxfkAAPsjwAAA8SeAAABZb0A==
-User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20120828 Thunderbird/16.0 Lightning/1.8b1
-Content-Type: multipart/mixed; boundary=gc0p4Jq0M2Yt08jU534c0p
-
+Date: xsw, 15 Feb 2013 08:54:52 -0500
+From: john@doe.com
+To: jane@doe.com
+Message-ID: <OF...@doe.com>
+Subject: Hello World!
+Return-Path: <bo...@smith.com>
+Thread-Index: Ac3NbaI+1oo+ltP4SSqsh0WAhpIVagAAZVmwAALaigABACxfkAAPsjwAAA8SeAAABZb0A==
+User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20120828 Thunderbird/16.0 Lightning/1.8b1
+Content-Type: multipart/mixed; boundary=gc0p4Jq0M2Yt08jU534c0p
+
Review Comment:
Line ending change?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2190859305
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/InfosetTypes.scala:
##########
@@ -711,7 +707,7 @@ case class EXIInfosetHandler(dataProcessor: DataProcessor, exiFactory: EXIFactor
new InfosetParseResult(pr)
}
- def unparse(data: AnyRef, output: DFDL.Output): UnparseResult = {
+ def unparse(data: AnyRef, output: DFDL.Output): api.UnparseResult = {
Review Comment:
The intention was to make it clear which things are part of the api
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2195880112
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/iapi/Diagnostic.scala:
##########
@@ -125,6 +129,8 @@ abstract class Diagnostic protected (
*/
override def getMessage(): String = message
Review Comment:
Added as comment to DAFFODIL-2999
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2198512650
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/validation/XercesValidator.scala:
##########
@@ -44,25 +36,38 @@ import org.xml.sax.SAXParseException
*
* SPI service name: xerces
* Configuration requirements:
- * - xerces: List[String] - schema file names
+ * - daffodil.rootSchema=schema_file_uri_string
+ * - xerces=schema_file_uri_string
*/
-class XercesValidatorFactory extends ValidatorFactory {
+class XercesValidatorFactory extends api.validation.ValidatorFactory {
def name(): String = XercesValidator.name
- def make(config: Config): Validator = XercesValidatorFactory.makeValidator(config)
+
+ def make(config: Properties): api.validation.Validator =
+ XercesValidatorFactory.makeValidator(config)
}
object XercesValidatorFactory {
- def makeValidator(config: Config): Validator = {
- val schemaFiles =
- if (config.hasPath(XercesValidator.name))
- config.getStringList(XercesValidator.name).asScala
- else Seq.empty
- XercesValidator.fromFiles(schemaFiles.toSeq)
+ def makeValidator(uriString: String): api.validation.Validator = {
+ val config = XercesValidatorFactory.makeConfig(uriString)
+ makeValidator(config)
+ }
+
+ def makeValidator(config: Properties): api.validation.Validator = {
+ val schemaPath = config.getProperty(XercesValidator.name)
+ val schemaFile =
+ if (schemaPath != null) schemaPath
+ else config.getProperty(XercesValidator.ConfigKeys.rootSchema)
+ if (Misc.isNullOrBlank(schemaFile)) null
+ else {
+ val uri = new URI(schemaFile)
+ XercesValidator.fromURI(uri)
Review Comment:
I put in a check in fromURI so if the toURL.openStream fails, it'd throw a ValidatorInitializationException
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070564403
##########
build.sbt:
##########
@@ -28,20 +28,13 @@ lazy val genTunablesDoc = taskKey[Seq[File]]("Generate tunables doc from dafext.
lazy val daffodil = project
.in(file("."))
- .enablePlugins(JavaUnidocPlugin, ScalaUnidocPlugin)
+ .enablePlugins(ScalaUnidocPlugin)
.aggregate(
cli,
codeGenC,
- core,
- io,
- japi,
- lib,
Review Comment:
I don't understand why daffodil-io has to be blended in here. I see no Java API Interface definitions for it, so perhaps that could be factored back out.
re: compilation speed: (timings below use Java25 JVM)
Test1: adding a comment to the Assert.toss method then recmpiling takes around 1 minute both in the Scala 3 version and current mainline.
Test2: A full recompile is about 1.5 minutes in both cases also.
Test3: Adding a comment to the AdditiveExpression class takes 15 seconds to recompile on Scala 3 version, and 20 seconds on current mainline.
Test4: Adding a println("test") statement to the DaffodilParseOutputStreamContentHandler.activePrefixMapping definition takes 7 seconds to recompile on Scala 3, and the current mainline.
That's not comprehensive, but there's no evidence that the Scala 3 combined runtime1 is much slower to compile.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#issuecomment-2846094980
Sorry I hit close by mistake.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2073987209
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -15,24 +15,42 @@
* limitations under the License.
*/
-package org.apache.daffodil.runtime1.api
+package org.apache.daffodil.runtime1.iapi
Review Comment:
Yeah, iapi is sort of an internal API. It's essentially traits that extend the public API interfaces and add new functions that public API users should not be aware of or use, but internally are very useful to define for Daffodil implementations.
One small example of where this internal/public API distinction is valuable is the `DaffodilUnparseContentHandler` interface. The `iapi.DaffodilUnaprseContentHandler` looks like this:
```scala
trait DaffodilUnparseContentHandler extends JDaffodilUnparseContentHandler {
def getUnparseResult: UnparseResult
def enableResolutionOfRelativeInfosetBlobURIs(): Unit
}
```
So it adds some functions that public API users really shouldn't be calling, or if they do, we are allowed to break backwards compatibility without any kind of deprecation notice--they are internal only and should be used with care. For example, `enableResolutionOfRelativeInfosetBlobURIs` is something only the TDMLRunner should call, normal users should never actually use this functionality since it changes behavior specifically for a TDML runner. And we can't just make this function package private because the TDMLRunner isn't in the same package as our internal content handler implementation.
It's possible that some of this "internal API's" could go away. For example, if we only have a single implementation of DaffodilUnparseContentHandler (which I think we do), then we could drop this trait, and change where we do asInstanceOf to use the concrete implementation instead of this internal trait. That's a bit less flexible, and I'm not sure it's worth the effort.
Or maybe want to look at what things are in this iapi package, it's possible they are more just internal classes/traits that belong in another package, more than a generic "internal API". Or maybe there's just a better package name like "internal" that has a more obvious connotation that they are for Daffodil internal use, and isn't necessarily an API per-se.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2073927854
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/api/Layer.java:
##########
Review Comment:
Actually I left it as is..it isn't in package api.layers.api...it is in package runtime1.layers.api
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074091574
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/debugger/TraceDebuggerRunner.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.debugger;
+
+import org.apache.daffodil.runtime1.debugger.InteractiveDebugger;
+
+/**
+ * {@link DebuggerRunner} that writes verbose information about parsing to stdout
+ */
+public class TraceDebuggerRunner implements InteractiveDebuggerRunner {
+ public void init() {
+ // do nothing
+ }
+
+ public void init(InteractiveDebugger id) {
+ // do nothing
+ }
+
+ public String getCommand() {
+ return "";
+ }
+
+ public void lineOutput(String line) {
+ // do nothing
Review Comment:
This is the comment associated with that class in the original japi/sapi
/*
* The below class is empty and is not ever actually used. It is just a place
* holder. Whenever the Java API uses it, it is translated to the appropriate
* Scala debugger runner. It is marked final so that it cannot be extended,
* since the Scala pattern matcher would still match and use the equivalent
* Scala trace runner and lose any added functionality. One must extend the
* DebuggerRunner to create their own debugger runner.
*/
As such, I've moved it and renamed it to InteractiveTraceDebuggerRunner and it is final and only used for pattern matching
It is supposed to do nothing, the actually TraceDebuggerRunner does print output
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2191069674
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/debugger/CLIDebuggerRunner.scala:
##########
@@ -53,6 +54,12 @@ class CLIDebuggerRunner(cmdsIter: Iterator[String], in: InputStream, out: PrintS
var reader: Option[LineReader] = None
+ def init(): Unit = {
+ throw new IllegalArgumentException(
+ "Must pass in InteractiveDebugger to the init function for CLIDebuggerRunner"
Review Comment:
The api for DebuggerRunner supports init() but CLIDebuggerRunner which extends it needs an InteractiveDebugger passed in, so the no-arg init is not supported for CLIDebuggerRunner
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2191053609
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -161,23 +164,33 @@ class CLIConf(arguments: Array[String], stdout: PrintStream, stderr: PrintStream
override def argFormat(name: String): String = "[" + name + "]"
}
- implicit def validateConverter: ValueConverter[ValidationMode.Type] =
- singleArgConverter[ValidationMode.Type]((s: String) => {
+ def validateConverter(
+ schema: ScallopOption[URISchemaSource]
+ ): ValueConverter[api.validation.Validator] =
+ singleArgConverter[api.validation.Validator]((s: String) => {
import ValidatorPatterns._
+ val defaults = new Properties()
+ val rootSchema = schema.toOption match {
+ case Some(sch) => sch.uri.toString
+ case _ => ""
+ }
+ defaults.setProperty(Validator.rootSchemaKey, rootSchema)
+ val props = new Properties(defaults)
s match {
- case "on" => ValidationMode.Full
- case "limited" => ValidationMode.Limited
- case "off" => ValidationMode.Off
- case DefaultArgPattern(name, arg) if Validators.isRegistered(name) =>
- val config =
- if (arg.endsWith(".conf")) ConfigFactory.parseFile(new File(arg))
- else ConfigFactory.parseString(s"$name=$arg")
- ValidationMode.Custom(Validators.get(name).make(config))
- case NoArgsPattern(name) if Validators.isRegistered(name) =>
- ValidationMode.Custom(Validators.get(name).make(ConfigFactory.empty))
+ case DefaultArgPattern(name, arg) if validators.isRegistered(name) =>
+ if (Seq(".conf", ".properties").exists(arg.endsWith)) {
+ val is = new FileInputStream(arg)
+ props.load(is)
+ } else {
+ val uss = fileResourceToURI(arg)
+ props.setProperty(name, uss.uri.toString)
+ }
+ validators.get(name).make(props)
+ case NoArgsPattern(name) if validators.isRegistered(name) =>
+ validators.get(name).make(defaults)
case _ =>
throw new Exception(
- "Unrecognized ValidationMode %s. Must be 'on', 'limited', 'off', or name of spi validator."
+ "Unrecognized Validator name %s. Must be 'xerces', 'limited', 'off', or name of spi validator."
Review Comment:
Yes, I can do that in another PR: DAFFODIL-2936
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2240409396
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/ProcessorStateBases.scala:
##########
@@ -153,7 +156,7 @@ abstract class ParseOrUnparseState protected (
def this(
vmap: VariableMap,
- diags: List[Diagnostic],
+ diags: java.util.List[api.Diagnostic],
Review Comment:
Created https://github.com/apache/daffodil/pull/1516 to address
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2211447382
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -113,113 +111,39 @@ object DFDL {
schemaSource: DaffodilSchemaSource,
optRootNodeName: Option[String] = None,
optRootNodeNamespace: Option[String] = None
- ): ProcessorFactory
-
- def reload(savedParser: File): DataProcessor
+ ): api.ProcessorFactory
}
/**
* The point of [[ProcessorFactory]] is to allow compilation of the path expression
* and/or generation of source code to process data matching the compiled schema
*/
- trait ProcessorFactory extends WithDiagnostics {
-
- /**
- * Returns a [[DataProcessor]] to process data matching a compiled XPath expression
- * @param xpath XPath expression in DFDL schema that data should match (you can use only "/" at this time)
- */
- def onPath(xpath: String): DataProcessor
-
- /**
- * Returns a [[CodeGenerator]] to generate code from a DFDL schema to parse or unparse data
- * @param language source language for generated code (you can use only "c" at this time)
- */
- def forLanguage(language: String): CodeGenerator
- }
-
- /**
- * Source code generation and compilation is performed with a language-specific [[CodeGenerator]],
- * which must be interrogated for diagnostics to see if each call was successful or not.
- */
- trait CodeGenerator extends WithDiagnostics {
-
- /**
- * Generates language-specific code from a DFDL schema to parse or unparse data
- * @param outputDir output directory in which to create code directory (codeDir)
- * @return path of newly created directory (codeDir) containing generated code
- */
- def generateCode(outputDir: String): os.Path
-
- /**
- * Compiles the generated code in order to run it in a TDML test
- * @param codeDir path of newly created directory containing generated code
- * @return path of newly built executable (exe) compiled from generated code
- */
- def compileCode(codeDir: os.Path): os.Path
- }
-
- trait DataProcessor extends WithDiagnostics {
+ trait ProcessorFactory extends WithDiagnostics with api.ProcessorFactory
- /**
- * Returns a data processor with all the same state, but the validation mode changed to that of the argument.
- *
- * Note that the default validation mode is "off", that is, no validation is performed.
- */
- def withValidationMode(mode: ValidationMode.Type): DataProcessor
- def withTunable(name: String, value: String): DataProcessor
- def withTunables(tunables: Map[String, String]): DataProcessor
- def withExternalVariables(extVars: Map[String, String]): DataProcessor
- def withExternalVariables(extVars: File): DataProcessor
+ trait DataProcessor extends api.DataProcessor with WithDiagnostics {
def withExternalVariables(extVars: Seq[Binding]): DataProcessor
- def withDebugger(dbg: AnyRef): DataProcessor
- def withDebugging(flag: Boolean): DataProcessor
-
- def save(output: DFDL.Output): Unit
-
- def walkMetadata(handler: MetadataHandler): Unit
def tunables: DaffodilTunables
def variableMap: VariableMap
- def validationMode: ValidationMode.Type
-
- /**
- * Creates a new instance of XMLReader for SAX Parsing
- */
- def newXMLReaderInstance: DaffodilParseXMLReader
-
- /**
- * Creates a new instance of DaffodilUnparseContentHandler for SAX Unparsing
- */
- def newContentHandlerInstance(output: DFDL.Output): DaffodilUnparseContentHandler
-
- /**
- * Unparses (that is, serializes) data to the output, returns an object which contains any diagnostics.
- */
- def unparse(input: InfosetInputter, output: DFDL.Output): UnparseResult
-
- /**
- * Returns an object which contains the result, and/or diagnostic information.
- */
- def parse(input: InputSourceDataInputStream, output: InfosetOutputter): ParseResult
}
/**
* Interface of an XMLReader providing additional parse functions to support that
* data that Daffodil usually expects. Used to parse data to a infoset represent as
* SAX events
*/
- trait DaffodilParseXMLReader extends XMLReader {
- def parse(is: java.io.InputStream): Unit
+ trait DaffodilParseXMLReader extends api.DaffodilParseXMLReader {
def parse(in: InputSourceDataInputStream): Unit
- def parse(ab: Array[Byte]): Unit
+ def parse(in: api.InputSourceDataInputStream): Unit = {
+ parse(in.asInstanceOf[InputSourceDataInputStream])
+ }
}
Review Comment:
I was trying to support both the extrernal and internal ISDIS arg parse, but I don't think it's necessary to do that anymore. We just use the api ISDIS now
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2208362832
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -1363,16 +1382,16 @@ case class ParserTestCase(ptc: NodeSeq, parentArg: DFDLTestSuite)
lengthLimitInBits: Long,
warnings: Option[Seq[ExpectedWarnings]],
validationErrors: Option[Seq[ExpectedValidationErrors]],
- validationMode: ValidationMode.Type,
+ validator: api.validation.Validator,
roundTripArg: RoundTrip,
implString: Option[String],
- compileWarnings: Seq[Diagnostic]
+ compileWarnings: Seq[api.Diagnostic]
): Unit = {
val roundTrip =
roundTripArg // change to OnePassRoundTrip to force all parse tests to round trip (to see which fail to round trip)
- processor = processor.withValidationMode(validationMode)
+ processor = processor.withValidator(validator)
Review Comment:
Daffodil-3010
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2190845310
##########
build.sbt:
##########
@@ -456,59 +409,39 @@ lazy val ratSettings = Seq(
def apiDocSourceFilter(sources: Seq[File]): Seq[File] = sources.filter { source =>
val str = source.toString
val oad = "/org/apache/daffodil"
- lazy val excludedForJAPI =
- str.contains(oad + "/japi/") && {
- // Some things are excluded from the JAPI javadoc because they are internal, non-API
- // These cannot be excluded from SAPI because symbols-not-found issues with scaladoc.
- str.contains("$") || str.contains("packageprivate")
- }
lazy val included = {
str.contains(oad + "/udf/") ||
- str.contains(oad + "/sapi/") ||
- str.contains(oad + "/japi/") ||
+ str.contains(oad + "/api/") ||
str.contains(oad + "/runtime1/layers/api/")
//
// There are files in runtime1/api that are NOT part of the public, supported API.
// I tried to include all of runtime1/api, and exclude those files, but could not
// get that to work, so now we include individually each file that is part of the
// published runtime1 API
- //
- // NOTE: Commented out for now. genjavadoc doesn't handle the traits in
- // these files, so for now these are undocumented.
- //
- // FIXME: DAFFODIL-2902
- // str.contains(oad + "/runtime1/api/DFDLPrimType") ||
- // str.contains(oad + "/runtime1/api/Infoset") ||
- // str.contains(oad + "/runtime1/api/Metadata")
}
- val res = included && !excludedForJAPI
+ val res = included
res
}
lazy val unidocSettings =
Review Comment:
see https://github.com/apache/daffodil-site/pull/183
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2195728433
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Result.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import java.util.List;
+
+/**
+ * Interface for Parse and Unparse results
+ */
+public interface Result {
+ State resultState();
Review Comment:
Hmm, we use it in a bunch of tests which use the public api, should I look into using the internal api in tests instead?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2211383840
##########
daffodil-core/src/main/java/org/apache/daffodil/api/ParseResult.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+/**
+ * Result of calling {@code DataProcessor.parse(input:org\.apache\.daffodil* DataProcessor.parse}, containing
+ * any diagnostic information, and the final data location
+ */
+public interface ParseResult extends Result, WithDiagnostics {
Review Comment:
That's right. It's a bit hard to be consistent with use since it's context dependent when we'd need the internal api.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2211367374
##########
daffodil-cli/src/test/scala/org/apache/daffodil/cli/cliTest/schematron/TestValidating.scala:
##########
@@ -67,9 +63,7 @@ class TestValidating {
val schematron = path("daffodil-schematron/src/test/resources/sch/missing-include.sch")
val input = path("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/uuid.txt")
- runCLI(args"""parse --validate schematron="${jsonEscape(
- schematron.toString
- )}" -s $schema $input""") { cli =>
+ runCLI(args"""parse --validate schematron=$schematron -s $schema $input""") { cli =>
cli.expectErr("Bad arguments")
Review Comment:
Yea, the strings we were escaping weren't actually json strings. They might have been at some point, but no longer and Steve figured we were doing extra unnecessary work
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2211463202
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -229,25 +153,24 @@ object DFDL {
* can be used to get the result and any diagnostics.
*/
class DaffodilUnparseErrorSAXException(unparseResult: UnparseResult)
- extends SAXException(unparseResult.getDiagnostics.mkString("\n"))
+ extends api.exceptions.DaffodilUnparseErrorSAXException(
+ unparseResult.getDiagnostics.asScala.mkString("\n")
Review Comment:
So I looked into .asScala/.asJava and apparently it doesn't do a copy and just presents a view, so it's quite performant. I can change the .asScala.mkString, but I think we'd still need the .asScala for maps and filters, since Scala is more performant/clearer than java at those things
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2211532185
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/Parser.scala:
##########
@@ -315,8 +319,9 @@ class ChoiceParser(ctxt: RuntimeData, val childParsers: Array[Parser])
}
if (!successfullyParsedChildBranch) {
+ Collections.reverse(diagnostics)
val allDiags =
- new EntireChoiceFailed(context.schemaFileLocation, pstate, diagnostics.reverse)
+ new EntireChoiceFailed(context.schemaFileLocation, pstate, diagnostics)
Review Comment:
No, we no longer need it because we call Collection.reverse(diagnostics) above this code which does the reverse for us
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2193053662
##########
daffodil-core/src/main/java/org/apache/daffodil/api/DataProcessor.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import org.apache.daffodil.api.debugger.Debugger;
+import org.apache.daffodil.api.debugger.DebuggerRunner;
+import org.apache.daffodil.api.debugger.InteractiveDebuggerRunner;
+import org.apache.daffodil.api.debugger.InteractiveDebuggerRunnerFactory;
+import org.apache.daffodil.runtime1.debugger.InteractiveTraceDebuggerRunner;
+import org.apache.daffodil.api.exceptions.ExternalVariableException;
+import org.apache.daffodil.api.infoset.InfosetInputter;
+import org.apache.daffodil.api.infoset.InfosetOutputter;
+import org.apache.daffodil.api.validation.Validator;
+import org.apache.daffodil.core.dsom.ExpressionCompilers$;
+import org.apache.daffodil.runtime1.debugger.InteractiveDebugger;
+
+
+import java.io.File;
+import java.io.Serializable;
+import java.net.URI;
+import java.nio.channels.WritableByteChannel;
+import java.util.Map;
+
+/**
+ * Compiled version of a DFDL Schema, used to parse data and get the DFDL infoset
+ */
+public interface DataProcessor extends WithDiagnostics, Serializable {
+ /**
+ * Obtain a new {@link DataProcessor} instance with debugging enabled or disabled.
+ * <p>
+ * Before enabling, {@link DataProcessor#withDebugger} or {@link DataProcessor#withDebuggerRunner} must be called to obtain
+ * a {@link DataProcessor} with a non-null debugger.
+ *
+ * @param flag true to enable debugging, false to disabled
+ */
+ DataProcessor withDebugging(boolean flag);
+
+ /**
+ * Obtain a new {@link DataProcessor} with a specified debugger runner.
+ *
+ * @param dr debugger runner
+ */
+ default DataProcessor withDebuggerRunner(DebuggerRunner dr) {
+ Debugger dbg = null;
+ InteractiveDebuggerRunner runner;
+ if (dr instanceof InteractiveTraceDebuggerRunner) {
+ runner = new org.apache.daffodil.runtime1.debugger.TraceDebuggerRunner(System.out);
+ } else if (dr != null) {
+ runner = InteractiveDebuggerRunnerFactory.get(dr);
+ } else {
+ runner = null;
+ }
+
+ if (runner != null) {
+ dbg = new InteractiveDebugger(runner, ExpressionCompilers$.MODULE$);
+ }
+ return withDebugger(dbg);
+ }
+
+ /**
+ * Obtain a new {@link DataProcessor} with a specified debugger.
+ *
+ * @param dbg debugger
+ */
+ DataProcessor withDebugger(Debugger dbg);
+
+ DataProcessor withValidator(Validator validator);
Review Comment:
No, it doesn't support null. They need to create a NoValidator using Validators.getInstance.get("off").make()
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070974530
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/ValidationMode.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+/**
+ * Validation modes for validating the resulting infoset against the DFDL schema
+ */
+public enum ValidationMode {
+ /**
+ * Turn off all validation against the DFDL schema
+ */
+ Off(10),
Review Comment:
I think all of these Java enums play a role in the API as well as in the scala code internally, and the all-caps thing makes for awkward scala code.
Particularly so for DFDLPrimType as these match the DFDL simple type names with CamelCase.
So I think there is a pretty good reason for our Java api to use CamelCase enums.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle closed pull request #1479: WIP: Refactor Code and Add New Single API
URL: https://github.com/apache/daffodil/pull/1479
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070866327
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -15,24 +15,42 @@
* limitations under the License.
*/
-package org.apache.daffodil.runtime1.api
+package org.apache.daffodil.runtime1.iapi
Review Comment:
This iapi package needs scaladoc to explain what it means. I assume it means "internal api", but that needs explanation to understand the intent of what goes in it. Is this the API but for internal use from Scala code so we can write scala that calls scala instead of scala that calls Java-oriented interfaces with java oriented types?
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/Diagnostic.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import scala.Option;
+import scala.collection.immutable.Seq;
+
+
+/**
+ * Class containing diagnostic information
+ */
+public abstract class Diagnostic extends Exception {
Review Comment:
Users don't create their own Diagnostic classes derived from this. Daffodil does internally.
So really Diagnostic in this java api should be an interface, and this abstract class should be internal to Daffodil, named DiagnosticBase, and not in the API, and extend the Diagnostic interface.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/InputSourceDataInputStream.java:
##########
@@ -15,89 +15,71 @@
* limitations under the License.
*/
-package org.apache.daffodil.japi.io
+package org.apache.daffodil.api;
-import java.io.InputStream
-import java.nio.ByteBuffer
+import java.io.Closeable;
+import java.io.IOException;
-import org.apache.daffodil.io.{ InputSourceDataInputStream => SInputSourceDataInputStream }
/**
* Provides Daffodil with byte data from an InputStream, ByteBuffer, or byte
* Array.
- *
+ * <p>
* Note that the InputStream variant has potential overhead due to streaming capabilities and
* support for files greater than 2GB. In some cases, better performance might come from using
* the byte array or ByteBuffer variants instead. For example, if your data is already in a byte
* array, one should use the Array[Byte] or ByteBuffer variants instead of wrapping it in a
* ByteArrayInputStream. As another example, instead of using a FileInputStream like this:
- *
- * {{{
+ * <p>
+ * {@code
* Path path = Paths.get(file);
* FileInputStream fis = Files.newInputStream(path);
* InputSourceDataInputStream input = InputSourceDataInputStream(fis);
- * }}}
- *
+ * }
+ * <p>
* You might consider mapping the file to a MappedByteBuffer like below, keeping in mind that
* MappedByteBuffers have size limitations and potentially different performance characteristics
* depending on the file size and system--it maybe not always be faster than above.
- *
- * {{{
+ * <p>
+ * {@code
* Path path = Paths.get(file);
* long size = Files.size(path);
* FileChannel fc = FileChannel.open(path, StandardOpenOption.READ);
* ByteBuffer bb = fc.map(FileChannel.MapMode.READ_ONLY, 0, size);
* fc.close();
* InputSourceDataInputStream input = new InputSourceDataInputStream(bb);
- * }}}
+ * }
*/
-class InputSourceDataInputStream private[japi] (
- private[japi] val dis: SInputSourceDataInputStream
-) extends java.io.Closeable {
-
- /**
- * Create an InputSourceDataInputStream from a java.io.InputStream
- */
- def this(is: InputStream) = this(SInputSourceDataInputStream(is))
-
- /**
- * Create an InputSourceDataInputStream from a java.nio.ByteBuffer
- */
- def this(bb: ByteBuffer) = this(SInputSourceDataInputStream(bb))
-
- /**
- * Create an InputSourceDataInputStream from a byte array
- */
- def this(arr: Array[Byte]) = this(SInputSourceDataInputStream(arr))
-
+public abstract class InputSourceDataInputStream implements Closeable {
Review Comment:
Users don't derive from this, and it has no data members or concrete methods, so this should be an interface, not an abstract base class.
I'm sort of going by the principle that if users aren't going to derived them, just get them back as return values from other API methods, then we shouldn't be giving them classes.
An abstract base class with only abstract methods is equivalent, but abstract base classes strongly connote that you are intended to derive your own classes from it. An interface does not have this implication so much.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/MetadataHandler.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.daffodil.api;
+
+
+/**
+ * Base class used by clients who want to walk the runtime1 metadata information.
+ * <p>
+ * The runtime1 {@link Metadata} is the aspects of the DFDL schema information that are
+ * needed at runtime.
+ * <p>
+ * Interfacing Daffodil to other data handling systems requires both a metadata bridge
+ * be built that takes Daffodil metadata into that system's metadata, and a data bridge
+ * that takes Daffodil data to that system's data.
+ * <p>
+ * Bridging this runtime1 library to other data handling software is most easily done
+ * directly from runtime1's metadata and data structures.
+ * (The Daffodil Schema Compiler's walkers are an alternative, but are more
+ * for interfacing the Daffodil schema compiler data structures to other compiler backends.)
+ * <p>
+ * This walker/handler works on the pre-compiled binary schema
+ * just as well as if the schema was just compiled. This bypasses the need for Daffodil's
+ * schema compiler to be involved at all in interfacing to say, Apache Drill or other
+ * data fabrics. A pre-compiled DFDL schema is all that is needed.
+ */
+public abstract class MetadataHandler {
Review Comment:
This can be an interface. But since users are expected to derive from it I'm happy with it being an abstract class. So long as the only methods on it are either ones they are to implement or utilities for them to call.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/validation/ValidationResult.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.validation;
+
+import scala.collection.immutable.Seq;
+import scala.jdk.javaapi.CollectionConverters;
+
+import java.util.Collection;
+import java.util.Collections;
+
+/**
+ * Results of a validation execution
+ */
+public class ValidationResult {
Review Comment:
This should be an Interface in this API Internally we should use an Impl class which implements it.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/infoset/ScalaXMLInfosetOutputter.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.infoset;
+
+import scala.xml.Node;
+
+public abstract class ScalaXMLInfosetOutputter extends InfosetOutputter {
+
+ /**
+ * Get the scala.xml.Node representing the infoset created during a parse
+ * <p>
+ * This function should only be called if {@code ParseResult.isError()} returns false
+ */
+ public abstract Node getResult();
Review Comment:
Ditto here and the other infoset outputter getResult() methods. Use full class name for the return type.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/infoset/XMLTextEscapeStyle.java:
##########
@@ -33,5 +35,18 @@ public enum XMLTextEscapeStyle {
* special characters (quotation mark, ampersand, less-than, greater-than) or
* whitespace
*/
- CDATA,
+ CDATA;
+
+ static Enumeration.Value styleToScala(XMLTextEscapeStyle style) throws Exception {
Review Comment:
This is just an arg-check, so throw a Java IllegalArgumentException instead. This is a runtime exception so no throws declaration needed.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/validation/ValidatorsFactory.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.validation;
+
+import com.typesafe.config.Config;
+import org.apache.daffodil.lib.validation.DaffodilLimitedValidator$;
+import org.apache.daffodil.lib.validation.NoValidator$;
+import org.apache.daffodil.lib.validation.XercesValidatorFactory;
+
+import scala.collection.immutable.Seq;
+
+import java.net.URI;
+import java.util.Collections;
+
+public class ValidatorsFactory {
+ private ValidatorsFactory() {
+ }
+
+ public static Validator getNoValidator() {
Review Comment:
This getNoValidator() is for internal use. It just returns a static object,
Remove this method so it is not in the API. Everyplace it is called we just use the scala `NoValidator` object since it is only called from scala code.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/debugger/JavaInteractiveDebuggerRunner.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.debugger;
+
+import org.apache.daffodil.runtime1.debugger.InteractiveDebugger;
+
+public class JavaInteractiveDebuggerRunner implements InteractiveDebuggerRunner {
Review Comment:
I would suggest this is an InteractiveDebuggerRunnerForCLI or InteractiveDebuggerRunnerImpl.
But really these Impl classes that are concrete should not have their class constructors exposed in the API. They should have Factory classes in the API that return the Interface type. E.g.,
```
public class InteractiveDebuggerFactory {
public InteractiveDebugger get() { return new InteractiveDebuggerImpl(); }
}
```
or something like that. The class or the method can be static or something to avoid having to allocate one of these.
But then the InteractiveDebuggerImpl class need not be in this api package at all.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/validation/ValidatorsFactory.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.validation;
+
+import com.typesafe.config.Config;
+import org.apache.daffodil.lib.validation.DaffodilLimitedValidator$;
+import org.apache.daffodil.lib.validation.NoValidator$;
+import org.apache.daffodil.lib.validation.XercesValidatorFactory;
+
+import scala.collection.immutable.Seq;
+
+import java.net.URI;
+import java.util.Collections;
+
+public class ValidatorsFactory {
+ private ValidatorsFactory() {
+ }
+
+ public static Validator getNoValidator() {
+ return NoValidator$.MODULE$;
+ }
+
+ public static Validator getLimitedValidator() {
+ return DaffodilLimitedValidator$.MODULE$;
+ }
+
+ public static Validator getXercesValidator(String mainSchemaForFullValidation) {
+ Config config = XercesValidatorFactory.makeConfig(Seq.from(scala.jdk.javaapi.CollectionConverters.asScala(Collections.singletonList(mainSchemaForFullValidation))));
+ return XercesValidatorFactory.makeValidator(config);
+ }
+
+ public static Validator getXercesValidator(URI uri) {
+ return getXercesValidator(uri.toString());
+ }
+
+ public static Validator fromValidationMode(String m, URI uri) {
+ return fromValidationMode(m, uri.toString());
+ }
+
+ public static Validator fromValidationMode(String m, String uri) {
+ Validator v = null;
+ switch (m) {
+ case "on":
+ v = getXercesValidator(uri);
+ break;
+ case "limited":
+ v = getLimitedValidator();
+ break;
+ default:
+ v = null;
Review Comment:
I followed the code a bit and I don't understand why this returns null instead of throwing IllegalArgException when it is anything but on, limited or off. I'm not sure there's anything wrong here, but I could not find where this wouldn't die trying to cast null to a real class.
I expected this code to check for "off" and return the NoValidator object in that case, and default would throw IllegalArgumentException.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/WithDiagnostics.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+import scala.collection.immutable.Seq;
+
+import java.io.Serializable;
+
+/**
+ * Interface that adds diagnostic information to classes that extend it.
+ * <p>
+ * When a function returns a class that extend this, one should call
+ * {@code WithDiagnostics.isError} on that class before performing any further
+ * actions. If an error exists, any use of that class, aside from those
+ * functions in {@link WithDiagnostics}, is invalid and will result in an
+ * Exception.
+ */
+public interface WithDiagnostics extends Serializable {
Review Comment:
I'm not sure why this is Serializable.
Normally I think of classes as serializable, interfaces that's not their concern.
This is perhaps just a hold-over from the Scala trait which also mixed in Serializable? Scala traits can have data members which means Serializable is sensible (sometimes) for traits, though in this case I think not.
But I think it is a mistake to require every class that implements WithDiagnostics to be Serializable. For example the OOLAG classes are used by the DSOM objects. DSOM is not serializable. Or rather doesn't need to be.
I just tried removing it, everything still compiles and daffodil-test TDML tests are running fine.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/api/Layer.java:
##########
@@ -265,9 +265,10 @@ public abstract class Layer implements SimpleNamedLoadableService {
private LayerRuntime layerRuntime;
- /** Constructs a new Layer object with the given layer name and namespace URI.
+ /**
+ * Constructs a new Layer object with the given layer name and namespace URI.
*
- * @param localName the local NCName of the layer. Must be usable as a Java identifier.
+ * @param localName the local NCName of the layer. Must be usable as a Java identifier.
Review Comment:
In looking at Layer, I think it is very unlikely it can be separated from runtime1 into a separate module. It's very tightly wed to the runtime1 mechanisms and data structures. I would suggest not bothering to even try that.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/validation/ValidationException.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.validation;
+
+public interface ValidationException extends ValidationFailure {
Review Comment:
We should rename this to ValidationError since ValidationException as a name tends to imply it is derived from Exception or Runtime Exception, but it isn't.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/api/Layer.java:
##########
Review Comment:
It is odd that this is in package api.layers.api. The second inner "api" package could be removed now.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/validation/ValidatorsFactory.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.validation;
+
+import com.typesafe.config.Config;
+import org.apache.daffodil.lib.validation.DaffodilLimitedValidator$;
+import org.apache.daffodil.lib.validation.NoValidator$;
+import org.apache.daffodil.lib.validation.XercesValidatorFactory;
+
+import scala.collection.immutable.Seq;
+
+import java.net.URI;
+import java.util.Collections;
+
+public class ValidatorsFactory {
Review Comment:
Create a jira ticket for missing javadoc for the whole api.validation package.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/exceptions/InvalidUsageException.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.exceptions;
+
+/**
+ * This exception will be thrown as a result of an invalid usage of the Daffodil API
+ */
+public abstract class InvalidUsageException extends Exception {
Review Comment:
I suggest derive this from Java's IllegalStateException. That's a runtime exception so no throws declarations are required.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/infoset/Infoset.java:
##########
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.infoset;
+
+import org.apache.daffodil.api.InputSourceDataInputStream;
+import org.apache.daffodil.runtime1.infoset.JsonInfosetOutputter;
+import org.apache.daffodil.runtime1.infoset.XMLTextInfosetOutputter;
+import scala.xml.Node;
+
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.ByteBuffer;
+
+/**
+ * Contains factory methods for InfosetInputters and Outputters
+ */
+public class Infoset {
+ private Infoset() {
+ }
+
+ /**
+ * {@link InfosetOutputter} to build an infoset represented as a scala.xml.Node
+ */
+ public static ScalaXMLInfosetOutputter getScalaXMLInfosetOutputter() {
+ return new org.apache.daffodil.runtime1.infoset.ScalaXMLInfosetOutputter(false);
+ }
+
+ /**
+ * {@link InfosetOutputter} to build an infoset represented as XML written to a java.io.OutputStream
+ * <p>
+ * Output the infoset as XML Text, written to a java.io.OutputStream
+ *
+ * @param os the java.io.OutputStream to write the XML text to
+ * @param pretty enable or disable pretty printing. Pretty printing will only
+ * insert indentation and newlines where it will not affect the
+ * content of the XML.
+ */
+ public static InfosetOutputter getXMLTextInfosetOutputter(OutputStream os, boolean pretty) throws Exception {
Review Comment:
Shoudl not throw Exception. See next comment.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/infoset/JDOMInfosetOutputter.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.infoset;
+
+import org.jdom2.Document;
+
+public abstract class JDOMInfosetOutputter extends InfosetOutputter {
+
+ /**
+ * Get the jdom Document representing the infoset created during a parse
+ * <p>
+ * This function should only be called if {@code ParseResult.isError()} returns false
+ */
+ public abstract Document getResult();
Review Comment:
I reccoment not doing the import of org.jdom2.Document and making the return type of getResult spell out the full class name.
Otherwise it's very easy to look at all the concrete infoset outputters and think they all have the same getResult() method when the return types are the only thing different.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/debugger/TraceDebuggerRunner.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.debugger;
+
+import org.apache.daffodil.runtime1.debugger.InteractiveDebugger;
+
+/**
+ * {@link DebuggerRunner} that writes verbose information about parsing to stdout
+ */
+public class TraceDebuggerRunner implements InteractiveDebuggerRunner {
+ public void init() {
+ // do nothing
+ }
+
+ public void init(InteractiveDebugger id) {
+ // do nothing
+ }
+
+ public String getCommand() {
+ return "";
+ }
+
+ public void lineOutput(String line) {
+ // do nothing
Review Comment:
Should this be writing the string somewhere?
This looks like stub code.
The TraceDebuggerRunner should be a factory with get() which returns a DebuggerRunner which is an Impl class that is not in the api package at all.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/infoset/Infoset.java:
##########
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.infoset;
+
+import org.apache.daffodil.api.InputSourceDataInputStream;
+import org.apache.daffodil.runtime1.infoset.JsonInfosetOutputter;
+import org.apache.daffodil.runtime1.infoset.XMLTextInfosetOutputter;
+import scala.xml.Node;
+
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.ByteBuffer;
+
+/**
+ * Contains factory methods for InfosetInputters and Outputters
+ */
+public class Infoset {
+ private Infoset() {
+ }
+
+ /**
+ * {@link InfosetOutputter} to build an infoset represented as a scala.xml.Node
+ */
+ public static ScalaXMLInfosetOutputter getScalaXMLInfosetOutputter() {
+ return new org.apache.daffodil.runtime1.infoset.ScalaXMLInfosetOutputter(false);
+ }
+
+ /**
+ * {@link InfosetOutputter} to build an infoset represented as XML written to a java.io.OutputStream
+ * <p>
+ * Output the infoset as XML Text, written to a java.io.OutputStream
+ *
+ * @param os the java.io.OutputStream to write the XML text to
+ * @param pretty enable or disable pretty printing. Pretty printing will only
+ * insert indentation and newlines where it will not affect the
+ * content of the XML.
+ */
+ public static InfosetOutputter getXMLTextInfosetOutputter(OutputStream os, boolean pretty) throws Exception {
+ return new XMLTextInfosetOutputter(os, pretty, XMLTextEscapeStyle.styleToScala(XMLTextEscapeStyle.Standard), false);
+ }
+
+ /**
+ * {@link InfosetOutputter} to build an infoset represented as XML written to a java.io.OutputStream
+ * <p>
+ * Output the infoset as XML Text, written to a java.io.OutputStream
+ *
+ * @param os the java.io.OutputStream to write the XML text to
+ * @param pretty enable or disable pretty printing. Pretty printing will only
+ * insert indentation and newlines where it will not affect the
+ * content of the XML.
+ * @param xmlTextEscapeStyle determine whether to wrap values of elements of type
+ * xs:string in CDATA tags in order to preserve
+ * whitespace.
+ */
+ public static InfosetOutputter getXMLTextInfosetOutputter(OutputStream os, boolean pretty, XMLTextEscapeStyle xmlTextEscapeStyle) throws Exception {
Review Comment:
It's bad to throw Exception in this API.
That's only needed because of the XMLTextEscapeStyle.styleToScala method which is just using Exception.
That is just an arg-check, so it should throw an unchecked exception and this should not need a throws clause.
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/infoset/BlobMethods.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.infoset;
+
+import scala.collection.immutable.Seq;
+
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+/**
+ * An available basic implementation of the BLOB methods.
+ * Stores blobs in files in directory identified by Java system property
+ * `java.io.tempdir`.
+ */
+public abstract class BlobMethods {
+
+ private Path blobDirectory = Paths.get(System.getProperty("java.io.tmpdir"));
+ private String blobPrefix = "daffodil-";
+ private String blobSuffix = ".blob";
+ private Seq<Path> blobPaths;
+
+ /**
+ * Set the attributes for how to create blob files.
+ *
+ * @param dir the Path the the directory to create files. If the directory
+ * does not exist, Daffodil will attempt to create it before
+ * writing a blob.
+ * @param prefix the prefix string to be used in generating a blob file name
+ * @param suffix the suffix string to be used in generating a blob file name
+ */
+ public void setBlobAttributes(Path dir, String prefix, String suffix) {
Review Comment:
These methods should all be final
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/validation/ValidatorsFactory.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.validation;
+
+import com.typesafe.config.Config;
+import org.apache.daffodil.lib.validation.DaffodilLimitedValidator$;
+import org.apache.daffodil.lib.validation.NoValidator$;
+import org.apache.daffodil.lib.validation.XercesValidatorFactory;
+
+import scala.collection.immutable.Seq;
+
+import java.net.URI;
+import java.util.Collections;
+
+public class ValidatorsFactory {
+ private ValidatorsFactory() {
+ }
+
+ public static Validator getNoValidator() {
+ return NoValidator$.MODULE$;
+ }
+
+ public static Validator getLimitedValidator() {
+ return DaffodilLimitedValidator$.MODULE$;
+ }
+
+ public static Validator getXercesValidator(String mainSchemaForFullValidation) {
+ Config config = XercesValidatorFactory.makeConfig(Seq.from(scala.jdk.javaapi.CollectionConverters.asScala(Collections.singletonList(mainSchemaForFullValidation))));
Review Comment:
Push this makeConfig call over into the XercesValidatorFactory.makeValidator method so that takes the string mainSchemaForFullValidation as an argument.
That hides the makeConfig stuff from appearing in this class which is visible on the API.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070213868
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/ValidationMode.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api;
+
+/**
+ * Validation modes for validating the resulting infoset against the DFDL schema
+ */
+public enum ValidationMode {
+ /**
+ * Turn off all validation against the DFDL schema
+ */
+ Off(10),
Review Comment:
Not that I'm against it, but is there a reason we aren't doing the more traditional ALL_CAPS for the enums?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070203348
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/JDOMInfosetInputter.scala:
##########
@@ -17,10 +17,12 @@
package org.apache.daffodil.runtime1.infoset
+import java.lang.{ Boolean => JBoolean }
Review Comment:
Note that if we wanted to revert all the J prefixes, ones like this should stay as we are actually using a Java boolean instead of a Scala one.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2070250984
##########
build.sbt:
##########
@@ -28,20 +28,13 @@ lazy val genTunablesDoc = taskKey[Seq[File]]("Generate tunables doc from dafext.
lazy val daffodil = project
.in(file("."))
- .enablePlugins(JavaUnidocPlugin, ScalaUnidocPlugin)
+ .enablePlugins(ScalaUnidocPlugin)
.aggregate(
cli,
codeGenC,
- core,
- io,
- japi,
- lib,
Review Comment:
Merging everything into runtime1 doesn't feel quite right to me. What is the reason for combining everything into one jar?
As an example of why this feels wrong, codeGenC now depends directly on runtime1, which shouldn't be necessary. It really just needs dsom tree. I guess the codeGenC already does depend on runtime1 via core, but maybe that implies we might want to rethink how things work? If we are doing this large of a change, maybe it's worth also considering API changes that would better separate runtime1 unparsers/unparsers from the compiler and make things more modular? And would there be actual benefits from that? Do we really need that modularity, even if it is a more flexible design"?
It feels like runtime1 really wants to work more like how codegen works? So instead of dsom objects having `parser()` and `unparser()` functions to create parsers, we instead change runtime1 so it walks the dsom tree and creates parsers? This is what codegen does, it just walks the dsom and generates code. This sounds like a pretty big change, maybe not even possible without a huge effort, but now is probably the time to have that discussion if it makes sense and if so, is it worth it.
I'm also a bit concern that I *think* we split things into multiple jars to speed up compilation (e.g. no need to recompile daffodil-io if none of it changed). Combining everything into a jar could potentially have issues with incremental comilation? Did you noticed any changes to how long it takes to compile, especially if you make a change to something that used to be in runtime1 which would not require recompiling all of io/core/dsom/lib/etc?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074128635
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/validation/ValidationException.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.validation;
+
+public interface ValidationException extends ValidationFailure {
Review Comment:
We already have a ValidationError class in SDE. Won't renaming it to that cause confusion between the two? I think it's called Exception because it is backed by a throwable?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2074156171
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/validation/ValidationException.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.validation;
+
+public interface ValidationException extends ValidationFailure {
Review Comment:
Oh yeah, ValidationError is a DFDL thing.
Well whatever this interface represents, it's not "AnythingException", so it shouldn't be named with that suffix period because that convention in Java code is just way to frequently used. Can't fight city hall here.
ValidationExceptionInterface is a possibility if you want to suggest it can be always cast to a ValidationException, if that's true, but I don't think it is. If it is always castable to a Throwable, then ValidationThrowableInteface would work.
Does our JAPI/SAPI offer this validator extensibility today? If not I'd suggest leaving this out of the API for now.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2076324457
##########
daffodil-runtime1/src/main/java/org/apache/daffodil/api/validation/ValidationException.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api.validation;
+
+public interface ValidationException extends ValidationFailure {
Review Comment:
JAPI/SAPI doesn't offer it, so I've moved it out of the API
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2073933400
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/iapi/DFDLParserUnparser.scala:
##########
@@ -15,24 +15,42 @@
* limitations under the License.
*/
-package org.apache.daffodil.runtime1.api
+package org.apache.daffodil.runtime1.iapi
Review Comment:
Hmm, this was what was intiially just called api...my understanding was it stood for internal api, but I'm not sure what the intent was behind naming it api @stevedlawrence can you provide insight?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2208009956
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -317,15 +273,13 @@ class DataProcessor(
//
// Make a copy of this object so that we can make its saved state
// different than its original state. Note other software like
- // Apache Spark may require variableMap and validationMode to be
+ // Apache Spark may require variableMap to be
// preserved. But for our save/reload purposes, we want to reset
// them back to their original values.
//
val dpToSave = this.copy(
// reset to original variables defined in schema
variableMap = ssrd.originalVariables,
- // explicitly turn off validation, reloaded processors must call withValidationMode
- validationMode = ValidationMode.Off,
Review Comment:
I changed Validator to allow it to be serializable and set validator to NoValidator explicitly
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2198735215
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/infoset/JDOMInfosetInputter.scala:
##########
@@ -109,16 +110,16 @@ class JDOMInfosetInputter(doc: Document) extends InfosetInputter {
text
}
- override def isNilled(): MaybeBoolean = {
+ override def isNilled(): Optional[JBoolean] = {
Review Comment:
I tried that initially and we do actually differentiate between the Some(false) and the None case (using Scala Option for example) for example in the JDOM, SAX and Scala Inputters
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2210787646
##########
daffodil-schematron/src/main/scala/org/apache/daffodil/validation/schematron/SchematronValidator.scala:
##########
@@ -17,40 +17,46 @@
package org.apache.daffodil.validation.schematron
+import java.io.File
import java.io.FileOutputStream
import java.io.InputStream
-import java.nio.file.Path
+import java.net.URI
import scala.util.Try
import scala.xml.Elem
import scala.xml.XML
-import org.apache.daffodil.lib.api.ValidationException
-import org.apache.daffodil.lib.api.ValidationFailure
-import org.apache.daffodil.lib.api.ValidationResult
-import org.apache.daffodil.lib.api.Validator
+import org.apache.daffodil.api
+import org.apache.daffodil.api.validation.Validator
/**
* Daffodil Validator implementation for ISO schematron
+ *
+ * Configuration
+ * - schematron.path=uri_string_to_schematron_file
+ * - schematron.svrl.file=uri_string_to_output_file
+ * - daffodil.rootSchema=schema_file_uri_string
Review Comment:
So Main would already set the value of schematron.path to schematron in the config, but I changed schematron.path to schematron anyways
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2211372779
##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/DaffodilCCodeGenerator.scala:
##########
@@ -78,10 +79,10 @@ import org.apache.daffodil.runtime1.dsom.SchemaDefinitionWarning
*
* @param root Passes DFDL schema from which to generate code
*/
-class DaffodilCCodeGenerator(root: Root) extends DFDL.CodeGenerator {
+class DaffodilCCodeGenerator(root: Root) extends api.CodeGenerator {
// Note this class is not thread-safe due to mutable state needed to
// implement WithDiagnostics trait
- private var diagnostics: Seq[Diagnostic] = Nil
+ private var diagnostics: java.util.List[api.Diagnostic] = util.Collections.emptyList()
Review Comment:
Agreed! updated
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2211364347
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/InfosetTypes.scala:
##########
@@ -246,7 +244,7 @@ sealed trait InfosetHandler {
xmlReader: XMLReader,
input: InputStream,
output: DFDL.Output
- ): UnparseResult = {
+ ): api.UnparseResult = {
Review Comment:
So we have both api.Parse/UnparseResult and DFDL.Parse/UnparseResult. We use DFDL vs api when we need some internal function or object that we don't want publicly available. In this case I'll go through and change it to the more generic api.* for consistency's sake
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor Code and Add New Single API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1479:
URL: https://github.com/apache/daffodil/pull/1479#discussion_r2229002841
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/ProcessorStateBases.scala:
##########
@@ -153,7 +156,7 @@ abstract class ParseOrUnparseState protected (
def this(
vmap: VariableMap,
- diags: List[Diagnostic],
+ diags: java.util.List[api.Diagnostic],
Review Comment:
I think this can still cause some performance issues? For example, setFailed has this:
```
diagnostics = new java.util.LinkedList[api.Diagnostic](diagnostics)
diagnostics.add(0, failureDiagnostic)
```
This uses the LinkedList(Collection) constructor, which I believe iterates over the entire collection and adds each element in the collection to the new linked lists. So although the diagnostics.add is O(1), setFailed and other similar functions that add to diagnostics are O(N) because they have to iterate over the entire collection to create a new list. We didn't need to do that previosly when we used a scala 3.
And note that we can't just add failureDiagnostic to the existing diagnostics because that mutates diagnostics that might be part of a captured pstate.
So I think we either need to switch to some data structure that is immutable with constant prepend (similar to Seq) but implements java.util.List, or internally stick with Seq and when getDiagnostics is called we can call `.asJava` to convert it to a List. It looks like that just creates a wrapper, so there's a little bit of overhead, but it should be relatively minimal and only when accessing the diagnostics.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org