[GitHub] [incubator-daffodil] jw3 opened a new pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 opened a new pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431


   - Defines an interface for validation behavior to allow injection of validators via SPI.
   - Moves the original Validator to DefaultValidator and provides it as the "default" behavior through the original flow.
   - Extends the ValidationMode to provide and custom injection while reducing changes to existing API.
   - Adds CLI handling to extend the `--validate` flag
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 edited a comment on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 edited a comment on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707967885


   https://issues.apache.org/jira/browse/DAFFODIL-2409
   
   Will add to commit message when squashed


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504958131



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.xml.sax.ErrorHandler
+
+/**
+ * User defined validation logic.
+ * Can be added programmatically or registered via SPI.
+ */
+trait Validator {
+  def name(): String = getClass.getTypeName.toLowerCase
+  def validateXML(document: java.io.InputStream, errHandler: ErrorHandler, args: Validator.Arguments): Unit
+}
+
+object Validator {
+  /**
+   * Unstructured argument list to support inputs from non-programmatic sources (eg. cli)
+   */
+  type ArgumentList = Seq[Validator.Argument]
+
+  /**
+   * Named arguments to be passed to the validator
+   * @param x
+   */
+  case class Arguments(private val x: Map[String, AnyRef]) {
+    // todo;; add all getAs and as methods
+
+    def as[T](k: String): T = {
+      x.get(k).asInstanceOf[T]
+    }
+
+    def getAs[T](k: String): Option[T] = {
+      x.get(k).map(_.asInstanceOf[T])
+    }
+  }
+  object Arguments {
+    def apply(): Arguments = Empty
+    val Empty = Arguments(Map.empty)
+  }
+
+  /**
+   * Named input to a validator in string form.
+   * Can be validated and converted during the compilation process.
+   * @param key arg name
+   * @param value arg value
+   */
+  case class Argument(key: String, value: String)
+  object Argument {
+    val DefaultKey = "default"
+    def apply(value: String): Argument = Argument(DefaultKey, value)
+  }
+
+  /**
+   * Opt-in compilation hints
+   */
+  object CompilerOps {
+
+    /**
+     * Compiles an unstructured argument list in to a structured list
+     */
+    trait CheckArgs {
+      def checkArgs(args: ArgumentList): Validator.Arguments
+    }
+  }
+}
+
+/**
+ * Thrown when the requested validator was not found in the SPI registered list
+ * @param name
+ */
+case class ValidatorNotFoundException(name: String) extends Exception(s"$name validator was not found")
+
+/**
+ * Thrown when validator compilation fails
+ * @param message
+ */
+case class ValidatorCompilationException(message: String) extends Exception(message)
+
+/**
+ * Provides validation logic packaged with arguments.  This compiled object has had validation and
+ * resource intensive initialization operations performed and is in an operation state
+ * @param v Validator instance
+ * @param args argument list
+ */
+class CompiledValidator(val v: Validator, args: Validator.Arguments) extends Serializable {

Review comment:
       Hmmm.
   
   I'm expecting CompiledValidator to be a scala trait with just one method, def validateXML(document, errorHandler): Unit with no implementation provided.
   
   Then specific validator service providers, including the default one, would have a factory method - perhaps named compile(args) that returns an instance of a specific implementation class, e.g., called CompiledXSDXercesValidator (for the default validator implementation) or something like that. The service provider's compile method would take the args and return an instance of this class which implements the CompiledValidator trait. 
   
   All the initialization and other one-time stuff would happen in the compile method for that service provider, if... that is, the resulting CompiledValidator and it's guts member values are all serializable. If they're not serializable, then probably that work has to be done lazily on first use, with the serializable members being the raw args. 
   
   In addition, if the CompiledValidator's guts runtime structures are not thread-safe, and in Xerces they are not, then the on-first use on-the-fly compilation has to be done in the getInstance method of a thread local so that all threads get their own.
   
   Does that make 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504157057



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -702,10 +706,22 @@ class ParseResult(dp: DataProcessor, override val resultState: PState)
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
+
+    val (v, args) = dp.validationMode match {
+      case ValidationMode.Custom(name, args) =>
+        Validators.find(name).getOrElse(
+          Assert.abort(s"Validator '$name' not found")

Review comment:
       I wanted to fail faster then this point, which is done in the CLI, but when using the API you can fall through to here and have to catch the Validator doesnt exist.
   
   There is at least one way to enforce something in the API, but Im not sure its cleaner than doing nothing and just throwing late (eg. factory to produce the CustomValidationMode that ensures a validator exists).




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707949452


   > I'd like to also comment on the schematron validator.
   > 
   > I think we should fold it into Daffodil's source tree. I know you are trying to have an example of how to do it outside of daffodil, which is fine, but we need to be able to review that also.
   > 
   > For example, it is printing things. It should be creating some sort of ValidationError instances, and adding them to validation errors somewhere, not printing.
   
   @mbeckerle I am fine with rolling it in if you want it.  IMO it would be better to get the SPI changes merged while working as close to possible as the way the validator was previously working, and then in additional PRs add schematron and add other nice to haves.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504338476



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.Validator
+
+object Validators {
+  import scala.collection.JavaConverters._
+
+  private val impls = new ThreadLocal[Map[String, Validator]] {

Review comment:
       Sounds good, will leave it as is; added a comment




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504150984



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.Validator
+
+object Validators {
+  import scala.collection.JavaConverters._
+
+  private val impls = new ThreadLocal[Map[String, Validator]] {

Review comment:
       This may have been over engineered during the first pass, likely as a reaction to the schema cache in the original Validator.
   
   Can probably simplify here with a hash map and say stateless thread safe validators.  Thoughts?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707714379


   I dont understand the rat check failures...


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512149627



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       There are some other things going on there, are they needed
   
   https://github.com/apache/incubator-daffodil/blob/master/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala#L675-L677




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512181997



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -672,7 +672,7 @@ sealed class ElementRuntimeData(
 
   def isSimpleType = optPrimType.isDefined
 
-  def schemaURIStringsForFullValidation = schemaURIStringsForFullValidation1.distinct
+  lazy val schemaURIStringsForFullValidation = schemaURIStringsForFullValidation1.distinct
   private def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:
     childERDs.flatMap { _.schemaURIStringsForFullValidation1 })

Review comment:
       Interesting algorithmic point about all this lazy val stuff we do.
   
   If you change this recursion to call schemaURIStringsForFullValidation (without the "1" suffix), this algorithm will be greatly improved because we expect tons of duplicates - so squeezing them out with the "distinct" recursively, as we aggregate up the tree, that call should pay dividends. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r518079558



##########
File path: daffodil-lib/src/test/resources/test/validation/myData5.dat
##########
@@ -0,0 +1 @@
+12:345678

Review comment:
       it wasnt used. must have copied it over with the other files planning on use but didnt.
   
   removed 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510201764



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -243,6 +249,8 @@ class DataProcessor private (
 
   def withValidationMode(mode:ValidationMode.Type): DataProcessor = copy(validationMode = mode)
 
+  def withValidator(validator: Validator): DataProcessor = copy(validationMode = ValidationMode.Custom(validator))

Review comment:
       Convenience method and also illustrates that Validators are now passed without any other Config or SPI related data.
   
   @stevedlawrence 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504208707



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -702,10 +706,22 @@ class ParseResult(dp: DataProcessor, override val resultState: PState)
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
+
+    val (v, args) = dp.validationMode match {
+      case ValidationMode.Custom(name, args) =>
+        Validators.find(name).getOrElse(
+          Assert.abort(s"Validator '$name' not found")

Review comment:
       @stevedlawrence added the guard in `withValidationMode`, seems clean enough
   
   @mbeckerle 
   
   >I don't think Assert.abort is appropriate here.
   
   I recall now that the assert was there because I was hoping to guard against missing validators making it to that point.  It is changed it over to an exception now, but after the `withValidationMode` change I have the feeling it could still be an assert of some kind.  Im good either way




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512118220



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       Re-reading what I just wrote.... not very clear. I think the point is that a validator's shared state, that can be shared across DataProcessor.parse calls requesting validation, is state of the DataProcessor object, and so should be constructed as a member of that object. 
   
   Whether a given validator can in fact take advantage of this depends on the extent to which that validator "compiles" the schema/rules/etc into static data that is read-only, and then provides a validation API that is stateless.  




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512171719



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -673,7 +673,7 @@ sealed class ElementRuntimeData(
   def isSimpleType = optPrimType.isDefined
 
   def schemaURIStringsForFullValidation = schemaURIStringsForFullValidation1.distinct
-  private def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:
+  def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:

Review comment:
       > btw: this public member should definitely be a lazy val, not a def. So please change that also. No need for this to be computed over and over.
   
   done




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-717256441


   @stevedlawrence @tuxji @olabusayoT  - I think this validator code is in good shape and you should take a look see 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512107443



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       If you hoist this val v into DataProcessor lazy val validator = .... then it will be computed once when first used, but reused by that data processor over and over if many parses are called which request validation.  You'd then pass that down to the actual validation. 
   
   I'm not sure for Xerces if this will matter much, because we may have to construct a whole Xerces instance and initialize it, and read in the schema, for every thread. Maybe not. Depends on whether a Xerces validator once initialized can be called from multiple threads, and just returns a result. 
   
   Still even if a whole separate xerces object is required per thread, then this could happen once per thread per DataProcessor, so it might be helpful if threads are long-living and used to process the same kind of data over and over - which would be typical of message-processing. 
   
   But for Schematron, this could be a big win because the whole conversion of the schematron file into xslt, and construction of the transformer from the XSLT might be able to be done exactly once and reused. I am not sure about the transformer object being stateless or not. If not then that would have to be a ThreadLocal object, but at least the generation of the XSLT would be fully shared/once-only. 
   
   v.validateXML will then be responsible for being thread-safe since parse can be called on many threads simultaneously, but that means v.validateXML will have to use ThreadLocal objects, as expected, for anything stateful. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512149627



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       There are some other things going on through `schemaURIStringsForFullValidation`, are they needed
   
   https://github.com/apache/incubator-daffodil/blob/master/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala#L675-L677




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512241085



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       So then my suggestion is this:
   
   Change the validator member to:
   ```
   def validator = validatorTL.get
   
   val validatorTL = new ThreadLocal[XercesValidator]{
      def getInstance() = .....code to actually create 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-720638192


   This has two approvals. As @stevedlawrence  started reviewing this, I'd like his concurrence before merging this. 
   
   I'm inclined to wait until after 3.0.0 release occurs and then merge this to the 3.1.0 ongoing master branch. 
   
   I don't think we should worry about perfecting the balance of API vs. Pluggability for this feature before merging it. 
   
   Users need the schematron capability, and the API and pluggability for this can evolve, and probably will with the next extension capability. E.g., we know layering needs pluggability, and we know people want more post-processing after parse, and pre-processing before unparse which is all pretty closely related to this mechanism. 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707723839


   >  If new files are added without a license header
   
   thats it, thanks, Ill address it 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-722506421


   squashed and rebased.
   
   thanks to all for the reviews, this evolved nicely.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512144007



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       The DataProcessor has the ssrd (SchemaSetRuntimeData) as a member.
   The ssrd has an elementRuntimeData object which is the root element's ERD, and that has the schemaURIStringsForFullValidation member on it.  So you don't need the resultState object to get this. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504266527



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -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
+
+import org.xml.sax.ErrorHandler
+
+trait Validator {
+  def name(): String = getClass.getTypeName.toLowerCase
+
+  def checkArgs(args: Validator.Arguments): Either[String, Unit] = Right(())

Review comment:
       If a validator needs arguments specified then it needs them at all entry points.  But I see what you are saying.
   
   The question would be then how to separate the validation of arguments from the implementation of the validator?  Having them separate might organize better in the API but seems like it would be a burden to implement argument checking then.
   
   Maybe a second interface that is an opt-in for argument checking... validating the validator :exploding_head: 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510186495



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -100,3 +111,35 @@ object Validator extends NoBindingFactoryAdapter {
   }
 }
 
+class DefaultValidatorFactory extends ValidatorFactory {
+  def name(): String = "default" /// todo;; or should it be "xerces"
+
+  // to not muck around too much with the original logic above just wrapping access to the

Review comment:
       I dont really want to touch the original Validator implementation and the caching approach it uses.  So this adapter class wraps it.  It also implements the Xerces specific SAX error handling that the ParseResult previously provided.
   
   I think its also a good example of how you can use a singleton objects with inexpensive wrappers through the factory interface (this is a little complex for an example, but the concept is there).




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504225258



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.Validator
+
+object Validators {
+  import scala.collection.JavaConverters._
+
+  private val impls = new ThreadLocal[Map[String, Validator]] {

Review comment:
       When DataProcessor.parse is called and validation is on, those calls are allowed to happen on different threads, so yes, I think validation can happen simultaneously on mulitple threads. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-714537124


   The sample application has had a rework to demonstrate some of the changes to the PR
   
   https://github.com/ctc-oss/daffodil-schematron-validator/tree/master/src/main/scala/com/ctc/daffodil/ext


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707967885


   https://issues.apache.org/jira/browse/DAFFODIL-2409


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512167712



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -673,7 +673,7 @@ sealed class ElementRuntimeData(
   def isSimpleType = optPrimType.isDefined
 
   def schemaURIStringsForFullValidation = schemaURIStringsForFullValidation1.distinct
-  private def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:
+  def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:

Review comment:
       No I don't get it. Why isn't just using the public schemaURIStringForFullValidation sufficient? 
   
   This idiom, a public member that just uses a private def that is recursive to compute it, is common.
   
   btw: this public member should definitely be a lazy val, not a def. So please change that also. No need for this to be computed over and over.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r505116468



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.CompiledValidator
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.Validator.CompilerOps.CheckArgs
+import org.apache.daffodil.api.ValidatorNotFoundException
+
+import scala.collection.JavaConverters._
+
+object Validators {
+  val default: Validator = new DefaultValidatorSPIProvider
+  def compiler(): ValidatorCompiler = new Compiler
+
+  trait ValidatorCompiler {
+    def find(name: String): Option[CompiledValidator]
+    def isRegistered(name: String): Boolean
+    def compile(name: String, args: Validator.ArgumentList): CompiledValidator
+  }
+
+  private class Compiler extends ValidatorCompiler {
+    private var compiled = Map.empty[String, CompiledValidator]
+    private lazy val impls: Map[String, Validator] =
+      ServiceLoader
+        .load(classOf[Validator])
+        .iterator()
+        .asScala
+        .map(v => v.name() -> v)
+        .toMap
+

Review comment:
       Sounds good! I haven't looked in detail about @mbeckerle's approach, I'll take a more detailed look tomorrow. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r505102879



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.CompiledValidator
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.Validator.CompilerOps.CheckArgs
+import org.apache.daffodil.api.ValidatorNotFoundException
+
+import scala.collection.JavaConverters._
+
+object Validators {
+  val default: Validator = new DefaultValidatorSPIProvider
+  def compiler(): ValidatorCompiler = new Compiler
+
+  trait ValidatorCompiler {
+    def find(name: String): Option[CompiledValidator]
+    def isRegistered(name: String): Boolean
+    def compile(name: String, args: Validator.ArgumentList): CompiledValidator
+  }
+
+  private class Compiler extends ValidatorCompiler {
+    private var compiled = Map.empty[String, CompiledValidator]
+    private lazy val impls: Map[String, Validator] =
+      ServiceLoader
+        .load(classOf[Validator])
+        .iterator()
+        .asScala
+        .map(v => v.name() -> v)
+        .toMap
+

Review comment:
       I guess then I'm interested with how the API will get fleshed out. Based on how the CLI uses validators, it feels like it's going to end up as something like this:
   
   ```scala
   dp.withValidator(Validator.Custom("sch", "key1=value1 key2=value2"))
   ```
   Which feels somewhat akward to me.
   
   Yes, you get the benefit that you don't have to recompile your app when you want to change the validator, but it's sitll not just drop in a jar and go. You still need to give your app some mechanism to provide the validator name to use and provide configuration options. And the configuration options need to be representable as key/value pairs which feels limiting. And every validator would likely have differnt key names. So even though you don't need to recompile, you still need some mechamism to pick and configure different avilable validators.
   
   To me this kind of plugability seems useful for the CLI and those limitations/awkwardness feel reasonable since it is a CLI and not an pipeline orchestration engine. But as an API user, I would rather do something like:
   
   ```scala
   val validator = configureAndCompileValidatorOfChoice()
   dp.withValidation(validator)
   ```
   
   If my app needs plugablity and the validators have simple configuration options, then I could always implement it much like you have using SPI and key/value pairs. But if my app has complex Validator's that need some configuration other then key/value pair strings (e.g. an open socket to connect to a rules engine, dynamically generated validation rules) where the preferable configuration is via code, then we still have that option.
   
   Note that this feels different to me than Daffodil's UserDefinedFunctions which also use SPI. But UDFs feel different because they literally are drop and go. There's no configuration that needs to be done or selection of which ones to use. You just drop in a UDF jar on the class path and if a schema uses a UDF by that name then we use it as is.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504638599



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.Validator
+
+object Validators {
+  import scala.collection.JavaConverters._
+
+  private val impls = new ThreadLocal[Map[String, Validator]] {

Review comment:
       I think the thread safety here guards Daffodil as well, where you could have a race at init time of the Validators state.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504647514



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -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
+
+import org.xml.sax.ErrorHandler
+
+trait Validator {
+  def name(): String = getClass.getTypeName.toLowerCase
+
+  def checkArgs(args: Validator.Arguments): Either[String, Unit] = Right(())
+
+  def validateXML(document: java.io.InputStream, errHandler: ErrorHandler, args: Validator.Arguments): Unit

Review comment:
       Here is a sketch in another branch, if its close Ill move it over and we can advance it, just didnt want to blow this PR up if it was way off.
   
   https://github.com/jw3/incubator-daffodil/commit/d9f7f4346e57e12aa2d14e24c5c5ca0581124a26
   
   I like the compiled approach, it addresses a few pain points and it ends up being more flexible to allow non-spi usage for API clients as well.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504680851



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -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
+
+import org.xml.sax.ErrorHandler
+
+trait Validator {
+  def name(): String = getClass.getTypeName.toLowerCase
+
+  def checkArgs(args: Validator.Arguments): Either[String, Unit] = Right(())
+
+  def validateXML(document: java.io.InputStream, errHandler: ErrorHandler, args: Validator.Arguments): Unit

Review comment:
       This approach is working out pretty well and goes along with discussion on the thread local usage, so pushed it to this branch for review




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512205715



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       How about in the validator but handled at init time.  Simplifies some things this way.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510812067



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.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.api
+
+import com.typesafe.config.Config
+
+import scala.xml.SAXException
+
+trait Validator {
+  def validateXML(document: java.io.InputStream): ValidationResult
+}
+
+trait ValidatorFactory {
+  def name(): String
+  def make(config: Option[Config]): Validator
+}
+
+case class ValidationResult(warnings: Seq[ValidationWarning], errors: Seq[ValidationError]) {
+  def failed: Boolean = errors.nonEmpty
+}
+
+
+trait ValidationError {
+  def message: String
+  def toThrowable: Throwable
+}
+trait ValidationWarning
+// todo;; can validator return other metadata?
+//trait ValidationReport
+
+
+// todo;; can DRY this up after sanity check
+case class SaxValidationError(e: SAXException) extends ValidationError {
+  def message: String = e.getMessage
+  def toThrowable: Throwable = e

Review comment:
       > I think probably you should be.
   
   Yeah for the sax results it makes sense to extend Exception, since they are Exception based anyways.  The point of the overall design though was to avoid forcing implementations of the new traits to be Exceptions.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510184003



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.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.api
+
+import com.typesafe.config.Config
+
+import scala.xml.SAXException
+
+trait Validator {
+  def validateXML(document: java.io.InputStream): ValidationResult
+}
+
+trait ValidatorFactory {
+  def name(): String
+  def make(config: Option[Config]): Validator
+}
+
+case class ValidationResult(warnings: Seq[ValidationWarning], errors: Seq[ValidationError]) {
+  def failed: Boolean = errors.nonEmpty
+}
+
+
+trait ValidationError {
+  def message: String
+  def toThrowable: Throwable
+}
+trait ValidationWarning
+// todo;; can validator return other metadata?
+//trait ValidationReport
+
+
+// todo;; can DRY this up after sanity check
+case class SaxValidationError(e: SAXException) extends ValidationError {
+  def message: String = e.getMessage
+  def toThrowable: Throwable = e

Review comment:
       The rationale here is that the `PState` needs a `Throwable` for `validationErrorNoContext`.  So this conversion is to (1) fit into that existing API and (2) not have `ValidationError` be throwable just for that reason.
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-717267525


   Think about the name `DefaultValidator` and whether we want it renamed to `XercesValidator` or something else more appropriate.
   
   The name default came about because it was the original Validator and I needed the name Validator for the new stuff.  But we can still mark it as the default without being named Default.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504152083



##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/packageprivate/Utils.scala
##########
@@ -73,6 +73,8 @@ private[japi] object ValidationConversions {
       case ValidationMode.Off => SValidationMode.Off
       case ValidationMode.Limited => SValidationMode.Limited
       case ValidationMode.Full => SValidationMode.Full
+      // todo;; support custom in java?
+      case _ => SValidationMode.Off

Review comment:
       Do we really need the Arguments for the API? Could the API be something maybe a bit simpler, e.g.
   
   ```scala
   val myValidator = new MyCustomValidator(...)
   dp.withValidation(Validation.Custom, myValidator)
   ```
   
   So if you are using the API, then we expect you to allocate a new validator and we provide an API method to provide it to Daffodil. One potential issue is what if the validator isn't threadsafe (I believe Xerces isn't?), but perhaps the CustomValidator should be expected to handle that if they arent thread safe?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-710051712


   All fair points and I do agree that there are benefits to plugability. I think maybe we just need to find the right balance between plugablity and configurability. Those feel to me like they sort of are at odds with each other. Finding a good happy medium I think is worth the effort, and I too am confident it's possible.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512780676



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -230,11 +234,18 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
   }
 
   implicit def validateConverter = singleArgConverter[ValidationMode.Type]((s: String) => {

Review comment:
       Do you really want to use an implicit def here? I always suggest avoiding them unless you are defining some domain-specific language, a language feature (e.g., we have a typed-equality library that uses them), etc. 
   
   Is this implicit def somehow required by the idiom this code is working in? If so comments to that effect would help.

##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,117 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.util.DefaultValidator.XercesValidator
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  private val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+    val uri = new URI(fn)
+    val is = uri.toURL.openStream()
+    val stream = new StreamSource(is)
+    stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+    stream
+  }
+
+  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 validator = new ThreadLocal[XercesValidator] {
+    override def initialValue(): XercesValidator =
+      initializeValidator(schema.newValidator(), DefaultValidator.this, resolver)
+  }
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val documentSource = new StreamSource(document)
+
+    try validator.get().validate(documentSource)
+    catch {
+      // Some SAX Parse errors are thrown even if you specify an error handler to the
+      // validator. So we also need this catch
+      case e: SAXException =>
+        errors :+= SaxValidationError(e)
+        ValidationResult(warnings, errors)
+    }
+    ValidationResult(warnings, errors)
+  }
+
+  def initializeValidator(validator: javax.xml.validation.Validator, errHandler: ErrorHandler, resolver: DFDLCatalogResolver):
+    javax.xml.validation.Validator = {
+    validator.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true)
+    //
+    validator.setFeature("http://xml.org/sax/features/validation", true)
+
+    // If you enable the feature below, it seems to do no validation at all. Just passes.
+    //          validator.setFeature("http://apache.org/xml/features/validation/dynamic", true)
+
+    validator.setFeature("http://apache.org/xml/features/validation/schema", true)
+    validator.setFeature("http://apache.org/xml/features/validation/schema-full-checking", true)
+    validator.setErrorHandler(errHandler)
+    validator.setResourceResolver(resolver)
+    validator
+  }
+
+  override def warning(spe: SAXParseException): Unit =  warnings :+= SaxValidationWarning(spe)
+  override def error(spe: SAXParseException): Unit = errors :+= SaxValidationError(spe)
+  override def fatalError(spe: SAXParseException): Unit = errors :+= SaxValidationError(spe)
+}
+
+object DefaultValidator {
+  type XercesValidator = javax.xml.validation.Validator

Review comment:
       Not sure you need this type any more if the DefaultValidator class name is changed to XercesValidator.

##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,117 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.util.DefaultValidator.XercesValidator
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])

Review comment:
       I think this class should be XercesValidator. Scaladoc shouldn't mention TDML runner as that is orthogonal. 

##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,117 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.util.DefaultValidator.XercesValidator
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  private val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+    val uri = new URI(fn)
+    val is = uri.toURL.openStream()
+    val stream = new StreamSource(is)
+    stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+    stream
+  }
+
+  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 validator = new ThreadLocal[XercesValidator] {
+    override def initialValue(): XercesValidator =
+      initializeValidator(schema.newValidator(), DefaultValidator.this, resolver)
+  }
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val documentSource = new StreamSource(document)
+
+    try validator.get().validate(documentSource)
+    catch {
+      // Some SAX Parse errors are thrown even if you specify an error handler to the
+      // validator. So we also need this catch
+      case e: SAXException =>
+        errors :+= SaxValidationError(e)
+        ValidationResult(warnings, errors)
+    }
+    ValidationResult(warnings, errors)
+  }
+
+  def initializeValidator(validator: javax.xml.validation.Validator, errHandler: ErrorHandler, resolver: DFDLCatalogResolver):
+    javax.xml.validation.Validator = {
+    validator.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true)
+    //
+    validator.setFeature("http://xml.org/sax/features/validation", true)
+
+    // If you enable the feature below, it seems to do no validation at all. Just passes.
+    //          validator.setFeature("http://apache.org/xml/features/validation/dynamic", true)
+
+    validator.setFeature("http://apache.org/xml/features/validation/schema", true)
+    validator.setFeature("http://apache.org/xml/features/validation/schema-full-checking", true)
+    validator.setErrorHandler(errHandler)
+    validator.setResourceResolver(resolver)
+    validator
+  }
+
+  override def warning(spe: SAXParseException): Unit =  warnings :+= SaxValidationWarning(spe)
+  override def error(spe: SAXParseException): Unit = errors :+= SaxValidationError(spe)
+  override def fatalError(spe: SAXParseException): Unit = errors :+= SaxValidationError(spe)
+}
+
+object DefaultValidator {
+  type XercesValidator = javax.xml.validation.Validator
+}
+
+class DefaultValidatorFactory extends ValidatorFactory {

Review comment:
       This, could stay DefaultValidatorFactory that happens to create a XercesValidator. 
   Or it could change to be XercesValidatorFactory. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504193151



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -234,7 +236,11 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
       case "on" => ValidationMode.Full
       case "limited" => ValidationMode.Limited
       case "off" => ValidationMode.Off
-      case _ => throw new Exception("Unrecognized ValidationMode %s.  Must be 'on', 'limited' or 'off'.".format(s))
+      case Validator.MultiArgsPattern(str, args) if Validators.exists(str) =>
+        ValidationMode.Custom(str, args.split(",").map(_.split("=")).map(kv => Validator.Argument(kv.head, kv.last)))
+      case Validator.DefaultArgPattern(str, arg) if Validators.exists(str) => ValidationMode.Custom(str, Seq(Validator.Argument(arg)))
+      case Validator.NoArgsPattern(str) if Validators.exists(str) => ValidationMode.Custom(str, Seq.empty)

Review comment:
       moved to cli




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512225139



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       Everything I have read recently says it is not.  Which makes sense, the library forces clients to synchronize across threads if needed.  Which will be true of Validator instances also.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512137383



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       The catch to moving that up to a lazy in the DP is the `resultState.infoset.runtimeData.schemaURIStringsForFullValidation` access.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504270876



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -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
+
+import org.xml.sax.ErrorHandler
+
+trait Validator {
+  def name(): String = getClass.getTypeName.toLowerCase
+
+  def checkArgs(args: Validator.Arguments): Either[String, Unit] = Right(())
+
+  def validateXML(document: java.io.InputStream, errHandler: ErrorHandler, args: Validator.Arguments): Unit

Review comment:
       OK.  That can probably work.  I see how it relates to your comment above.  Will take a stab at this compiled idea tomorrow.
   
   A gotcha might be derived arguments, for example the schema paths being passed to the DefaultValidator.
   
   https://github.com/apache/incubator-daffodil/pull/431/files#diff-f6dc861db0417ffe6d0f80f7e0a166757345c5a3505e76de8f3bb694c1d170f3R720




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707742406


   Oh, can you also create a new Jira bug to add custom infoset validators and update the commit comment to reference that bug number?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707903168


   I'd like to also comment on the schematron validator.
   
   I think we should fold it into Daffodil's source tree. I know you are trying to have an example of how to do it outside of daffodil, which is fine, but we need to be able to review that also. 
   
   For example, it is printing things. It should be creating some sort of ValidationError instances, and adding them to validation errors somewhere, not printing. 
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510186887



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -100,3 +111,35 @@ object Validator extends NoBindingFactoryAdapter {
   }
 }
 
+class DefaultValidatorFactory extends ValidatorFactory {
+  def name(): String = "default" /// todo;; or should it be "xerces"
+
+  // to not muck around too much with the original logic above just wrapping access to the
+  // object in an adapter for the new Validator behavior
+  def make(config: Option[Config]): Validator = new Validator with ErrorHandler {
+    private val schemaFiles = config.map(_.getStringList(name()).asScala).getOrElse(Seq.empty)
+    private var errors = List.empty[ValidationError]
+    private var warnings = List.empty[ValidationWarning]
+    def validateXML(document: InputStream): ValidationResult = {
+      try {
+        DefaultValidator.validateXMLSources(schemaFiles, document, this)
+        ValidationResult(warnings, errors)
+      }
+      catch {
+        //
+        // Some SAX Parse errors are thrown even if you specify an error handler to the
+        // validator.
+        //
+        // So we also need this catch
+        //
+        case e: SAXException =>
+          errors :+= SaxValidationError(e)
+          ValidationResult(warnings, errors)
+      }
+    }
+
+    override def warning(spe: SAXParseException): Unit =  errors :+= SaxValidationError(spe)

Review comment:
       Xerces handling is no longer applicable up in the ParseResult




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 edited a comment on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 edited a comment on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707714379


   I dont understand the CI rat check failures, no libs are modified in 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 edited a comment on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 edited a comment on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707710350


   Have a few questions Ill comment on here, and there is a sample validator implementation at https://github.com/ctc-oss/daffodil-schematron-validator
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504690943



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -702,10 +706,22 @@ class ParseResult(dp: DataProcessor, override val resultState: PState)
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
+
+    val (v, args) = dp.validationMode match {
+      case ValidationMode.Custom(name, args) =>
+        Validators.find(name).getOrElse(
+          Assert.abort(s"Validator '$name' not found")

Review comment:
       resolved with compilation




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504654140



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.Validator
+
+object Validators {
+  import scala.collection.JavaConverters._
+
+  private val impls = new ThreadLocal[Map[String, Validator]] {

Review comment:
       Thinking about alternatives, will get something pushed for discussion later today.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707710350


   Sample validator implementation at https://github.com/ctc-oss/daffodil-schematron-validator
   
   https://github.com/ctc-oss/daffodil-schematron-validator/blob/master/README.md


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504270876



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -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
+
+import org.xml.sax.ErrorHandler
+
+trait Validator {
+  def name(): String = getClass.getTypeName.toLowerCase
+
+  def checkArgs(args: Validator.Arguments): Either[String, Unit] = Right(())
+
+  def validateXML(document: java.io.InputStream, errHandler: ErrorHandler, args: Validator.Arguments): Unit

Review comment:
       OK.  That can probably work.  I see how it relates to your comment above.  Will take a stab at this compiled idea tomorrow.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r503922070



##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/packageprivate/Utils.scala
##########
@@ -73,6 +73,8 @@ private[japi] object ValidationConversions {
       case ValidationMode.Off => SValidationMode.Off
       case ValidationMode.Limited => SValidationMode.Limited
       case ValidationMode.Full => SValidationMode.Full
+      // todo;; support custom in java?
+      case _ => SValidationMode.Off

Review comment:
       Not sure how to approach this

##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/packageprivate/Utils.scala
##########
@@ -82,6 +84,8 @@ private[japi] object ValidationConversions {
       case SValidationMode.Off => ValidationMode.Off
       case SValidationMode.Limited => ValidationMode.Limited
       case SValidationMode.Full => ValidationMode.Full
+      // todo;; support custom in java?
+      case _ => ValidationMode.Off

Review comment:
       Not sure how to approach this

##########
File path: daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/packageprivate/Utils.scala
##########
@@ -80,6 +82,8 @@ private[sapi] object ValidationConversions {
       case SValidationMode.Off => ValidationMode.Off
       case SValidationMode.Limited => ValidationMode.Limited
       case SValidationMode.Full => ValidationMode.Full
+      // todo;; hacking this out for now
+      case _ => ValidationMode.Off

Review comment:
       Not sure how to approach this

##########
File path: daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/packageprivate/Utils.scala
##########
@@ -71,6 +71,8 @@ private[sapi] object ValidationConversions {
       case ValidationMode.Off => SValidationMode.Off
       case ValidationMode.Limited => SValidationMode.Limited
       case ValidationMode.Full => SValidationMode.Full
+      // todo;; hacking this out for now
+      case _ => SValidationMode.Off

Review comment:
       Not sure how to approach this




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512810167



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -363,6 +374,10 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
       case _ => Right(Unit)
     }
 
+    validateOpt(validate) {
+      case _ => Right(Unit)
+    }
+

Review comment:
       It doesn't look like this is doing anything. Can it be removed?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512721940



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.util
+
+import java.lang.ThreadLocal
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.util.DefaultValidator.XercesValidator
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?

Review comment:
       :+1: 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504142654



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -702,10 +706,22 @@ class ParseResult(dp: DataProcessor, override val resultState: PState)
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
+
+    val (v, args) = dp.validationMode match {
+      case ValidationMode.Custom(name, args) =>
+        Validators.find(name).getOrElse(
+          Assert.abort(s"Validator '$name' not found")

Review comment:
       `ValidatorNotFoundException` sounds good




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512137383



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       The catch to moving that up to a lazy in the DP is the `resultState.infoset.runtimeData.schemaURIStringsForFullValidation` access.
   
   ~~To have somethings lazy from up there and other things not.... could define some type of trait that indicates that processing state data is needed.  Cache the ones that dont have that need, and not cache the others.~~
   
   Can use the `ValidationMode` as a discriminator I think.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512205715



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       How about in the validator but handled at init time?  Simplifies some things this way (pushed an update).




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-709307294


   Something like that seems reasonable as far as configuration goes.
   
   I still have concerns that even with something like typesafe config, we're still limiting Validation configuration to what can be represented by some configuration language (e.g. typesafe) just so that we can get plugability. I can imagine Validators that really can only be configured with code, granted I'm not sure if anything like that exists in the real world. And I don't personally feel that plugability gains much outside of CLI support.
   
   But I won't block this change if others are fine with 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r511988526



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.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.api
+
+import com.typesafe.config.Config
+
+import scala.xml.SAXException
+
+trait Validator {
+  def validateXML(document: java.io.InputStream): ValidationResult
+}
+
+trait ValidatorFactory {
+  def name(): String
+  def make(config: Option[Config]): Validator
+}
+
+case class ValidationResult(warnings: Seq[ValidationWarning], errors: Seq[ValidationError]) {
+  def failed: Boolean = errors.nonEmpty
+}
+
+
+trait ValidationError {
+  def message: String
+  def toThrowable: Throwable
+}
+trait ValidationWarning
+// todo;; can validator return other metadata?
+//trait ValidationReport
+
+
+// todo;; can DRY this up after sanity check
+case class SaxValidationError(e: SAXException) extends ValidationError {
+  def message: String = e.getMessage
+  def toThrowable: Throwable = e

Review comment:
       Reworked this some.  Added an extra trait for results that are exception based.  See what you think.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512137383



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       The catch to moving that up to a lazy in the DP is the `resultState.infoset.runtimeData.schemaURIStringsForFullValidation` access.
   
   To have somethings lazy from up there and other things not.... could define some type of trait that indicates that processing state data is needed.  Cache the ones that dont have that need, and not cache the others.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-709506942


   One thing to consider re: pureconfig is that we probably want to be able to support Validators written in Java. Sometimes libraries writen in scala can be difficult to use in Java code. I'm not familiat enough with pureconfig to know one way or the other. This doesn't really matter if it's all abstracted away and Validators don't actually need to call pureconfig functions, but if they do, that's somethign to keep in mind.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707992549


   I am ok with the plan to not fold schematron validation in to Daffodil as yet. I will look more at the xerces-based validator then. 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512721082



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       Yeah. You are right. I was somehow thinking this validator got exposed by the object, but it's only used locally inside the validateXML method. Nevermind.
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512718030



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -672,7 +672,7 @@ sealed class ElementRuntimeData(
 
   def isSimpleType = optPrimType.isDefined
 
-  def schemaURIStringsForFullValidation = schemaURIStringsForFullValidation1.distinct
+  lazy val schemaURIStringsForFullValidation = schemaURIStringsForFullValidation1.distinct
   private def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:
     childERDs.flatMap { _.schemaURIStringsForFullValidation1 })

Review comment:
       This is interesting indeed.  It wouldnt feel right burying that change in this PR, sounds like a great performance PR though.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512929205



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -0,0 +1,57 @@
+/*
+ * 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 com.typesafe.config.Config
+
+import scala.xml.SAXException
+
+trait Validator {
+  def validateXML(document: java.io.InputStream): ValidationResult
+}
+
+trait ValidatorFactory {
+  def name(): String
+  def make(config: Config): Validator
+}
+
+case class ValidationResult(warnings: Seq[ValidationWarning], errors: Seq[ValidationFailure]) {
+  def failed: Boolean = errors.nonEmpty
+}
+
+trait ValidationFailure {
+  def getMessage: String
+}
+
+trait ValidationWarning {
+  def getMessage: String
+}
+
+trait ValidationException {
+  def getCause: Throwable
+}
+
+// todo;; can validator return other metadata?
+//trait ValidationReport
+
+sealed abstract class SaxValidationResult(e: SAXException) extends Exception(e) with ValidationException
+case class SaxValidationError(e: SAXException) extends SaxValidationResult(e) with ValidationFailure with ValidationException
+case class SaxValidationWarning(e: SAXException) extends SaxValidationResult(e) with  ValidationWarning with ValidationException

Review comment:
       Right, these exist for the Xerces validator and can be moved over there and 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512126716



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       > I think the point is that a validator's shared state, that can be shared across DataProcessor.parse calls requesting validation, is state of the DataProcessor object, and so should be constructed as a member of that object.
   
   That is what I took from the first post.  I havent been thinking about multiple runs with same processor as much as probably should have been, but I think your point is clear once that is considered.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512711185



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       Are you requesting changes to the current code?  Im not following why we want an an accessor thats not exposed by the trait, to access a private var that users dont need to know about.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-714523044


   Made a pass over this and attempted to address the API and SPI related feedback.
   
   Also addressed the return value of the validation, which I had said lets kick down the road, but it seemed more complex to kick 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504190419



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -692,7 +695,8 @@ class DataProcessor private (
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
   with WithDiagnosticsImpl
-  with ErrorHandler {
+  with ErrorHandler
+  with Logging {
 
   /**
    * To be successful here, we need to capture xerces parse/validation

Review comment:
       Removed xerces and fixed the param doc




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence merged pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence merged pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512137383



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       The catch to moving that up to a lazy in the DP is the `resultState.infoset.runtimeData.schemaURIStringsForFullValidation` access.
   
   To have somethings lazy from up there and other things not.... Could define some type of trait that indicates that processing state data is needed.  Cache the ones that dont have that need it and not cache the others.

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       The catch to moving that up to a lazy in the DP is the `resultState.infoset.runtimeData.schemaURIStringsForFullValidation` access.
   
   To have somethings lazy from up there and other things not.... could define some type of trait that indicates that processing state data is needed.  Cache the ones that dont have that need it and not cache the others.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 edited a comment on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 edited a comment on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-708839422


   Argument handling is getting out of control.  I implemented [some stuff](https://github.com/jw3/incubator-daffodil/blob/validator_spi/impl/daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala#L41-L55) today just to move the idea forward but didnt like doing it. 
   
   The complexity around the argument handling is that we need to convert the CLI arguments entered as strings to the proper type, but only the validator implementation knows how to do that. The handling of these strings and the interface needed to support the CLI makes the API usage scary.
    
   The code from today to try to bridge some of the gap was modeled after [typesafe config](https://github.com/lightbend/config).  And it hit me tonight we should just use that and it would offload a bunch of complexity and eliminate a bunch of code in this PR.  
   
   Benefits are
   
   - well known library
   - allow us to accept configuration on cli or from file
   - accept configuration in form of java props, json, or hocon
   - can pull configuration from environment
   - provides well known typed getters and setters
   - unsurprising validation and error handling
   - less of Daffodil to maintain
   - Apache 2.0 licensed
   
   Unless there are objections to using it, Im going to work it into the next iteration while responding to the latest feedback.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504150984



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.Validator
+
+object Validators {
+  import scala.collection.JavaConverters._
+
+  private val impls = new ThreadLocal[Map[String, Validator]] {

Review comment:
       This may have been over engineered during the first pass, likely as a reaction to the schema cache in the original Validator.
   
   Would daffodil call validation from multiple threads? or can we just simplify here with a hash map..




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504098892



##########
File path: daffodil-lib/src/test/scala/org/apache/daffodil/api/TestValidatorPatterns.scala
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.junit.Test
+import org.apache.daffodil.exceptions.Assert.invariantFailed
+import org.junit.Assert.assertEquals
+
+class TestValidatorPatterns {
+  @Test def testNoArgsPattern(): Unit = {
+    val vname = "vvv"
+    s"$vname" match {
+      case Validator.MultiArgsPattern(_, _) =>
+        invariantFailed("should not have matched")
+      case Validator.DefaultArgPattern(_, _) =>
+        invariantFailed("should not have matched")
+      case Validator.NoArgsPattern(v) =>
+        assertEquals(vname, v)
+      case _ =>
+        invariantFailed("did not match")

Review comment:
       This is junit. You don't need to use our Assert.invariantFailed. You can just call Junit's fail method. 
   
   There's no code invariant here, so invariantFailed isn't really the right idea. 

##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.Validator
+
+object Validators {
+  import scala.collection.JavaConverters._
+
+  private val impls = new ThreadLocal[Map[String, Validator]] {

Review comment:
       So validators are allowed to be stateful? Can you explain in comments in the code here why this has to be a thread local?

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -702,10 +706,22 @@ class ParseResult(dp: DataProcessor, override val resultState: PState)
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
+
+    val (v, args) = dp.validationMode match {
+      case ValidationMode.Custom(name, args) =>
+        Validators.find(name).getOrElse(
+          Assert.abort(s"Validator '$name' not found")

Review comment:
       I don't think Assert.abort is appropriate here. 
   
   This is a user error presumably, like their chosen validator is not on the classpath. Assert.abort is going to produce a backtrace, as if a software error is happening. In this case that's not helpful. 
   
   I think you want to throw an actual exception that the command line, or an application using the Daffodil API, can catch.  
   
   The UDF system defines UserDefinedFunctionFatalException and UserDefinedFunctionProcessingError.
   
   You probably want to define ValidatorNotFoundException, and throw that. Then the CLI wants to catch it, issue a message and exit, without a backtrace. 

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -702,10 +706,22 @@ class ParseResult(dp: DataProcessor, override val resultState: PState)
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
+
+    val (v, args) = dp.validationMode match {
+      case ValidationMode.Custom(name, args) =>
+        Validators.find(name).getOrElse(
+          Assert.abort(s"Validator '$name' not found")
+        ) -> args
+      case _ => Validators.default -> Seq(Validator.Argument(
+        resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation.mkString(",")
+      ))
+    }
+
+    Assert.usage(resultState.processorStatus eq Success)

Review comment:
       Can this line be first one in the method? Assert.usage being first makes the code a bit more self-documenting about the requirements on the function's arguments, or the state things have to be in, when calling. 
   
    

##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/packageprivate/Utils.scala
##########
@@ -73,6 +73,8 @@ private[japi] object ValidationConversions {
       case ValidationMode.Off => SValidationMode.Off
       case ValidationMode.Limited => SValidationMode.Limited
       case ValidationMode.Full => SValidationMode.Full
+      // todo;; support custom in java?
+      case _ => SValidationMode.Off

Review comment:
       Suggest for JAPI, setValidationMode/withValidationMode takes an enum, and two other optional arguments. The enum gets an additional Custom, and when Custom is specified, the other two args, which are the name and list of args per your Custom(name., args) Scala ValidationMode.Custom case class constructor. 
   
   These same additional objects get passed to modeToScala, which can actually then construct a Custom object like the CLI code does. 

##########
File path: daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/packageprivate/Utils.scala
##########
@@ -80,6 +82,8 @@ private[sapi] object ValidationConversions {
       case SValidationMode.Off => ValidationMode.Off
       case SValidationMode.Limited => ValidationMode.Limited
       case SValidationMode.Full => ValidationMode.Full
+      // todo;; hacking this out for now
+      case _ => ValidationMode.Off

Review comment:
       modeFromScala is not used. I think we can delete this. 

##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/packageprivate/Utils.scala
##########
@@ -82,6 +84,8 @@ private[japi] object ValidationConversions {
       case SValidationMode.Off => ValidationMode.Off
       case SValidationMode.Limited => ValidationMode.Limited
       case SValidationMode.Full => ValidationMode.Full
+      // todo;; support custom in java?
+      case _ => ValidationMode.Off

Review comment:
       JAPI modeFromScala might not be used at all. I searched and didn't find any usages. The whole ValidationConversions object is package private. I think modeFromScala can probably be removed. 

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -692,7 +695,8 @@ class DataProcessor private (
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
   with WithDiagnosticsImpl
-  with ErrorHandler {
+  with ErrorHandler
+  with Logging {
 
   /**
    * To be successful here, we need to capture xerces parse/validation

Review comment:
       This comment maybe shouldn't mention xerces anymore? Or should qualify what it says about xerces as that's just the default "Full" validator?

##########
File path: daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/packageprivate/Utils.scala
##########
@@ -71,6 +71,8 @@ private[sapi] object ValidationConversions {
       case ValidationMode.Off => SValidationMode.Off
       case ValidationMode.Limited => SValidationMode.Limited
       case ValidationMode.Full => SValidationMode.Full
+      // todo;; hacking this out for now
+      case _ => SValidationMode.Off

Review comment:
       Grrr. I am not sure why SAPI can't just directly use the api.ValidationMode class, and has to impose its own Enumeration. 
   
   So you can do a parallel change to what has to be done for JAPI here, i.e., add extra args to sapi.setValidationMode and withValidationMode, and use them to construct a api.ValidationMode.Custom object once you get that point. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-717330011


   There's a handful of tests that are failing. Any idea what are causing those?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510188305



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -692,40 +699,35 @@ class DataProcessor private (
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
   with WithDiagnosticsImpl
-  with ErrorHandler {

Review comment:
       This moved down into the default validator




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512697749



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       The reason  for the def validator, and the val (which should be private val, btw) is just to hide the fact that this is a threadlocal entirely. That's the implementation's business, not the user of the validator. The user of the validator shouldn't have to know that it's a threadlocal and so .get is needed, etc. 
   
   If you put the types on the rationale is clear:
   ```
   def validator: XercesValidator = validatorTL.get
   
   private val validatorTL = new ThreadLocal[XercesValidator] { ... }
   ```
   Unless the caller of the validator.validateXML method looks in the implementation, they don't know anything about this thread-local stuff.
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512711937



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -243,6 +250,18 @@ class DataProcessor private (
 
   def withValidationMode(mode:ValidationMode.Type): DataProcessor = copy(validationMode = mode)
 
+  def withValidator(validator: Validator): DataProcessor = copy(validationMode = ValidationMode.Custom(validator))
+
+  lazy val validator: Validator = {

Review comment:
       This is the validator factory right? rename.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504959769



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.CompiledValidator
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.Validator.CompilerOps.CheckArgs
+import org.apache.daffodil.api.ValidatorNotFoundException
+
+import scala.collection.JavaConverters._
+
+object Validators {
+  val default: Validator = new DefaultValidatorSPIProvider
+  def compiler(): ValidatorCompiler = new Compiler
+
+  trait ValidatorCompiler {

Review comment:
       Suggest not nesting these inside an object. A package is sufficient. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510193887



##########
File path: project/Dependencies.scala
##########
@@ -29,6 +29,7 @@ object Dependencies {
     "xml-resolver" % "xml-resolver" % "1.2",
     "commons-io" % "commons-io" % "2.6",
     "jline" % "jline" % "2.14.6",
+    "com.typesafe" % "config" % "1.4.0"

Review comment:
       Using Config as mentioned before to support configuring validators in an abstract way.  Validators being a completely unknown and non-standardized process makes us support arbitrary configuration.  To support definition and access of the arbitrary configurations in a standardized way we use this.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r505075462



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.xml.sax.ErrorHandler
+
+/**
+ * User defined validation logic.
+ * Can be added programmatically or registered via SPI.
+ */
+trait Validator {
+  def name(): String = getClass.getTypeName.toLowerCase
+  def validateXML(document: java.io.InputStream, errHandler: ErrorHandler, args: Validator.Arguments): Unit
+}
+
+object Validator {
+  /**
+   * Unstructured argument list to support inputs from non-programmatic sources (eg. cli)
+   */
+  type ArgumentList = Seq[Validator.Argument]
+
+  /**
+   * Named arguments to be passed to the validator
+   * @param x
+   */
+  case class Arguments(private val x: Map[String, AnyRef]) {
+    // todo;; add all getAs and as methods
+
+    def as[T](k: String): T = {
+      x.get(k).asInstanceOf[T]
+    }
+
+    def getAs[T](k: String): Option[T] = {
+      x.get(k).map(_.asInstanceOf[T])
+    }
+  }
+  object Arguments {
+    def apply(): Arguments = Empty
+    val Empty = Arguments(Map.empty)
+  }
+
+  /**
+   * Named input to a validator in string form.
+   * Can be validated and converted during the compilation process.
+   * @param key arg name
+   * @param value arg value
+   */
+  case class Argument(key: String, value: String)
+  object Argument {
+    val DefaultKey = "default"
+    def apply(value: String): Argument = Argument(DefaultKey, value)
+  }
+
+  /**
+   * Opt-in compilation hints
+   */
+  object CompilerOps {
+
+    /**
+     * Compiles an unstructured argument list in to a structured list
+     */
+    trait CheckArgs {
+      def checkArgs(args: ArgumentList): Validator.Arguments
+    }
+  }
+}
+
+/**
+ * Thrown when the requested validator was not found in the SPI registered list
+ * @param name
+ */
+case class ValidatorNotFoundException(name: String) extends Exception(s"$name validator was not found")
+
+/**
+ * Thrown when validator compilation fails
+ * @param message
+ */
+case class ValidatorCompilationException(message: String) extends Exception(message)
+
+/**
+ * Provides validation logic packaged with arguments.  This compiled object has had validation and
+ * resource intensive initialization operations performed and is in an operation state
+ * @param v Validator instance
+ * @param args argument list
+ */
+class CompiledValidator(val v: Validator, args: Validator.Arguments) extends Serializable {

Review comment:
       I think so.  Ill rework the impl and see how it shakes out.
   
   The most visible change will be that SPI will provide factory instances that in turn provide validator instances.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-714604786


   Just hit me that given the changes to the custom validation mode the SPI loading is only accessed through CLI.  Looking at working up an example that demonstrates the usefulness (or not) of SPI in non-CLI usage.
   
   I dont think the current approach will change much at all to demonstrate that, so feedback on the latest would be applicable.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512808941



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -230,11 +234,18 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
   }
 
   implicit def validateConverter = singleArgConverter[ValidationMode.Type]((s: String) => {

Review comment:
       This is how scallop reccommends adding new converters. The idea being that when we add new args we don't have to specify the converter and the implicit stuff finds the right converter.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r505108216



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.CompiledValidator
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.Validator.CompilerOps.CheckArgs
+import org.apache.daffodil.api.ValidatorNotFoundException
+
+import scala.collection.JavaConverters._
+
+object Validators {
+  val default: Validator = new DefaultValidatorSPIProvider
+  def compiler(): ValidatorCompiler = new Compiler
+
+  trait ValidatorCompiler {
+    def find(name: String): Option[CompiledValidator]
+    def isRegistered(name: String): Boolean
+    def compile(name: String, args: Validator.ArgumentList): CompiledValidator
+  }
+
+  private class Compiler extends ValidatorCompiler {
+    private var compiled = Map.empty[String, CompiledValidator]
+    private lazy val impls: Map[String, Validator] =
+      ServiceLoader
+        .load(classOf[Validator])
+        .iterator()
+        .asScala
+        .map(v => v.name() -> v)
+        .toMap
+

Review comment:
       > dp.withValidator(Validator.Custom("sch", "key1=value1 key2=value2"))
   
   Nah, it never would have ended up like that. I should have focused more on API before submitting PR, but here we are.
   
   Ultimately I want to get as close to type safety and convenience as we can.  There was some work towards this in the commits earlier today.
   
   > val validator = configureAndCompileValidatorOfChoice()
   > dp.withValidation(validator)
   
   Yeah, I think we are heading in that direction now.  Are you tracking @mbeckerle's compiled approach?
   
   > Note that this feels different to me than Daffodil's UserDefinedFunctions which also use SPI. But UDFs feel different because they literally are drop and go. There's no configuration that needs to be done or selection of which ones to use. You just drop in a UDF jar on the class path and if a schema uses a UDF by that name then we use it as is.
   
   Yeah, realizing I needed to handle arguments was a bit of a shock that came after thinking the implementation was simple :) and then the shock of needing multiple arguments followed that.
   
   Have an idea Ill post at the top level that just came to me when discussing this.  Thanks




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504152834



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -234,7 +236,11 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
       case "on" => ValidationMode.Full
       case "limited" => ValidationMode.Limited
       case "off" => ValidationMode.Off
-      case _ => throw new Exception("Unrecognized ValidationMode %s.  Must be 'on', 'limited' or 'off'.".format(s))
+      case Validator.MultiArgsPattern(str, args) if Validators.exists(str) =>
+        ValidationMode.Custom(str, args.split(",").map(_.split("=")).map(kv => Validator.Argument(kv.head, kv.last)))
+      case Validator.DefaultArgPattern(str, arg) if Validators.exists(str) => ValidationMode.Custom(str, Seq(Validator.Argument(arg)))
+      case Validator.NoArgsPattern(str) if Validators.exists(str) => ValidationMode.Custom(str, Seq.empty)

Review comment:
       Sounds good.  They are only useful to CLI but wasnt sure where to stash them.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r517368375



##########
File path: daffodil-lib/src/test/resources/test/validation/myData5.dat
##########
@@ -0,0 +1 @@
+12:345678

Review comment:
       Looks like these other files are UTF-16. UTF-8/ascii is prefered unless directly testing for UTF-16.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510419029



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/ValidationMode.scala
##########
@@ -26,4 +26,8 @@ object ValidationMode extends Enum {
   case object Off extends Type(10)
   case object Limited extends Type(20)
   case object Full extends Type(30)
+
+  // todo;; revisit;; passing the validator like this for backwards compat with existing

Review comment:
       comment no longer relevant. You are passing the validator as the arg to Custom, which keeps this type safe.

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -419,14 +427,16 @@ class DataProcessor private (
     // 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) =
-    if (validationMode == ValidationMode.Full) {
-      val bos = new java.io.ByteArrayOutputStream()
-      val xmlOutputter = new XMLTextInfosetOutputter(bos, false)
-      val teeOutputter = new TeeInfosetOutputter(output, xmlOutputter)
-      (teeOutputter, One(bos))
-    } else {
-      (output, Nope)
+    val (outputter, maybeValidationBytes) = {
+      validationMode match {
+        case ValidationMode.Full | ValidationMode.Custom(_) =>
+          val bos = new java.io.ByteArrayOutputStream()
+          val xmlOutputter = new XMLTextInfosetOutputter(bos, false)
+          val teeOutputter = new TeeInfosetOutputter(output, xmlOutputter)
+          (teeOutputter, One(bos))

Review comment:
       Ok, so this is going to buffer up the data for validation. That's fine. 
   
   This is going to get really interesting when we start using the forthcoming SAX APIs. Then we could, in principle, feed SAX events to the validator while the parsing is going on. Validation errors might be reported incrementally even, before parsing is complete. 

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -692,40 +699,35 @@ class DataProcessor private (
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
   with WithDiagnosticsImpl
-  with ErrorHandler {

Review comment:
       Or any validator right? A non-default one also has to provide an error handler, yes?

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -692,40 +699,35 @@ class DataProcessor private (
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
   with WithDiagnosticsImpl
-  with ErrorHandler {
+  with Logging {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+
+    val v = dp.validationMode match {
+      case ValidationMode.Custom(cv) => cv
+      case _ =>
+        /// todo;; potential corner case (or not corner?) of needing this dynamic value to configure a validator
+        ///        resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation.mkString(",")
+        val v = ConfigValueFactory.fromAnyRef(resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation)
+        Validators.default.make(
+          Some(ConfigFactory.parseMap(Map(Validators.default.name() -> v).asJava))

Review comment:
       Yeah, I've been unhappy with this InfosetElement vs. DIElement thing in Daffodil since shortly after putting it in. Not clear having that distinction adds any real value. The notion was that InfosetElement could be available on the API, and would expose only things API users need. But we end up downcasting them to DIElement all the time. 
   
   But I don't think we need the downcast here. So I think you can shorten:
   ```
   resultState.infoset.asInstanceOf[InfosetElement].... 
   ```
   to drop the asInstanceOf. The PState.infoset is of type DIElement which isA InfosetElement already 

##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -100,3 +111,35 @@ object Validator extends NoBindingFactoryAdapter {
   }
 }
 
+class DefaultValidatorFactory extends ValidatorFactory {
+  def name(): String = "default" /// todo;; or should it be "xerces"
+
+  // to not muck around too much with the original logic above just wrapping access to the

Review comment:
       This prevents the default validator from being a good motivation for the factory API pattern here. Instead you end up wondering why the factory idiom is here at all if the factory is just going to do nothing. 
   
   I think the xerces default validator - all the cache complexity goes away if you actually implement the factory pattern. 

##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.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.api
+
+import com.typesafe.config.Config
+
+import scala.xml.SAXException
+
+trait Validator {
+  def validateXML(document: java.io.InputStream): ValidationResult
+}
+
+trait ValidatorFactory {
+  def name(): String
+  def make(config: Option[Config]): Validator
+}
+
+case class ValidationResult(warnings: Seq[ValidationWarning], errors: Seq[ValidationError]) {
+  def failed: Boolean = errors.nonEmpty
+}
+
+
+trait ValidationError {
+  def message: String
+  def toThrowable: Throwable
+}
+trait ValidationWarning
+// todo;; can validator return other metadata?
+//trait ValidationReport
+
+
+// todo;; can DRY this up after sanity check
+case class SaxValidationError(e: SAXException) extends ValidationError {
+  def message: String = e.getMessage
+  def toThrowable: Throwable = e

Review comment:
       Typically what you have as "def toThrowable" would be "def cause". Java Exception objects have two members, a message and a cause. One of the two, or both, have to be defined. (Typically one of the two.) You're not extending java's Exception class here. I think probably you should be. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510178492



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -562,6 +582,12 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
   verify()
 }
 
+object ValidatorPatterns {
+  val NoArgsPattern: Regex = "(.+?)".r.anchored
+  val MultiArgsPattern: Regex = "(.+?)=(.+?=.+[,]?)+".r.anchored

Review comment:
       Supporting typesafe config allows us to avoid rolling our own multi-arg support here which would get complicated and opinionated in favor of a more standard approach (env and .conf files).
   
   So this pattern is going to go away, just leaving it in for now to point this out. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707721986


   I'll review this later today, but the rat check makes sure that all files have correct licenses. If new files are added without a license header, the rat check will fail. You can run ``sbt ratCheck`` and it will generate a report in ``target/rat.txt`` listing files that are missing a license header.
   
   I think the files missing a license header in this PR are:
   * daffodil-lib/src/main/resources/META-INF/services/org.apache.daffodil.api.Validator
   * daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
   * daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
   * daffodil-lib/src/test/scala/org/apache/daffodil/api/TestValidatorPatterns.scala


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512816351



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -0,0 +1,57 @@
+/*
+ * 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 com.typesafe.config.Config
+
+import scala.xml.SAXException
+
+trait Validator {
+  def validateXML(document: java.io.InputStream): ValidationResult
+}
+
+trait ValidatorFactory {
+  def name(): String
+  def make(config: Config): Validator
+}
+
+case class ValidationResult(warnings: Seq[ValidationWarning], errors: Seq[ValidationFailure]) {
+  def failed: Boolean = errors.nonEmpty
+}
+
+trait ValidationFailure {
+  def getMessage: String
+}
+
+trait ValidationWarning {
+  def getMessage: String
+}
+
+trait ValidationException {
+  def getCause: Throwable
+}
+
+// todo;; can validator return other metadata?
+//trait ValidationReport
+
+sealed abstract class SaxValidationResult(e: SAXException) extends Exception(e) with ValidationException
+case class SaxValidationError(e: SAXException) extends SaxValidationResult(e) with ValidationFailure with ValidationException
+case class SaxValidationWarning(e: SAXException) extends SaxValidationResult(e) with  ValidationWarning with ValidationException

Review comment:
       Why do we need specific SAX Validation errors in the Validation API? I would expect that if a Validator used SAX, it would need to catch those exceptions and convert them to the ValidationResult/Failure/Warning class that Daffodil understands. Presumably we don't want this tied to SAX so that non-SAX implementations could work with out issue.

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -243,6 +250,18 @@ class DataProcessor private (
 
   def withValidationMode(mode:ValidationMode.Type): DataProcessor = copy(validationMode = mode)
 
+  def withValidator(validator: Validator): DataProcessor = copy(validationMode = ValidationMode.Custom(validator))
+
+  lazy val validator: Validator = {
+    validationMode match {
+      case ValidationMode.Custom(cv) => cv
+      case _ =>
+        val v = ConfigValueFactory.fromAnyRef(ssrd.elementRuntimeData.schemaURIStringsForFullValidation)
+        val f = Validators.default
+        f.make(ConfigFactory.parseMap(Map(f.name() -> v).asJava))
+    }

Review comment:
       This is a bit interseting. So Custom Validators won't have any access to the schemaURIStrings? Is it possible other custom validators will need those? I imaginge in something like schematron it might not, but other XML Schema validators that aren't our built-in xerces one might? Seems odd that only our internal validator gets access to this information? Or shold no Validators get this access, including our own, and Validators are expected to implement their own logic for this like URI resolution?

##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.
+ */
+

Review comment:
       Is this the API that custom validators must implement? If so, we shoul throuoghly add documentation to all these functions. It also might be worth adding a new projet for this so that we can include this validator API in the public documentation via unidoc, similar to what we do with UDF. Might even be worth implementing this API in Java to ensure both Java and Scala implementations can be created.

##########
File path: daffodil-japi/src/main/scala/org/apache/daffodil/japi/packageprivate/Utils.scala
##########
@@ -73,6 +73,8 @@ private[japi] object ValidationConversions {
       case ValidationMode.Off => SValidationMode.Off
       case ValidationMode.Limited => SValidationMode.Limited
       case ValidationMode.Full => SValidationMode.Full
+      // todo;; support custom in java?
+      case _ => SValidationMode.Off

Review comment:
       Was this issue ever resolved? How does one specify custom validation via the SAPI/JAPI? It would probably be worth adding Java and Scala tests to make sure this works as expected, even if a test validator doesn't do anything. That way we can ensure we have a publicly workable API to provide validator configurations.

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -419,14 +427,16 @@ class DataProcessor private (
     // 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) =
-    if (validationMode == ValidationMode.Full) {
-      val bos = new java.io.ByteArrayOutputStream()
-      val xmlOutputter = new XMLTextInfosetOutputter(bos, false)
-      val teeOutputter = new TeeInfosetOutputter(output, xmlOutputter)
-      (teeOutputter, One(bos))
-    } else {
-      (output, Nope)
+    val (outputter, maybeValidationBytes) = {
+      validationMode match {
+        case ValidationMode.Full | ValidationMode.Custom(_) =>
+          val bos = new java.io.ByteArrayOutputStream()
+          val xmlOutputter = new XMLTextInfosetOutputter(bos, false)
+          val teeOutputter = new TeeInfosetOutputter(output, xmlOutputter)
+          (teeOutputter, One(bos))

Review comment:
       Yep, and that will completely change the validator API. Perhaps we'll end up supporting two types of API's? One that gets the entire infoset as a string and one that gets indidual SAX events?

##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,117 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.util.DefaultValidator.XercesValidator
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])

Review comment:
       Agreed, I dont' think anything should be called DefaultXYZ. When a validator is used determines if it's a default. If we decide to change the default this name would no longer make 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504654140



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.Validator
+
+object Validators {
+  import scala.collection.JavaConverters._
+
+  private val impls = new ThreadLocal[Map[String, Validator]] {

Review comment:
       Thinking about alternatives, will get something pushed for discussion later today.
   
   - There may be no reason to cache the full set of SPI results
   - Does a compilation remove the need to cache anything at all
   - No caching makes some cli parts less convenient, but api usage remains the same
   - No caching simplifies the Validators object and resolves this discussion...




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 edited a comment on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 edited a comment on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707723839


   >  If new files are added without a license header
   
   thats it, thanks, Ill address it now
   
   ... done


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512214691



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       Works for me. 
   
   Do you know if Xerces is in fact thread safe to call validate() on?
   
   I found this:   
   
   > You can reuse the same validator and the same schema multiple times in series. However, only the schema is     thread safe. Validators and schema factories are not. If you validate in multiple threads simultaneously, make sure each one has its own Validator and SchemaFactory objects.
   
   On this page: https://www.ibm.com/developerworks/xml/library/x-javaxmlvalidapi/
   
   It suggests we can't hoist out to the factory without using a thread-local for the Validator object itself.
   
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512723009



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       :+1: 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510178492



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -562,6 +582,12 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
   verify()
 }
 
+object ValidatorPatterns {
+  val NoArgsPattern: Regex = "(.+?)".r.anchored
+  val MultiArgsPattern: Regex = "(.+?)=(.+?=.+[,]?)+".r.anchored

Review comment:
       Supporting typesafe config allows us to avoid rolling our own multi-arg support here which would get complicated and opinionated in favor of a more standard approach (env and .conf files).
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r505087452



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.CompiledValidator
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.Validator.CompilerOps.CheckArgs
+import org.apache.daffodil.api.ValidatorNotFoundException
+
+import scala.collection.JavaConverters._
+
+object Validators {
+  val default: Validator = new DefaultValidatorSPIProvider
+  def compiler(): ValidatorCompiler = new Compiler
+
+  trait ValidatorCompiler {
+    def find(name: String): Option[CompiledValidator]
+    def isRegistered(name: String): Boolean
+    def compile(name: String, args: Validator.ArgumentList): CompiledValidator
+  }
+
+  private class Compiler extends ValidatorCompiler {
+    private var compiled = Map.empty[String, CompiledValidator]
+    private lazy val impls: Map[String, Validator] =
+      ServiceLoader
+        .load(classOf[Validator])
+        .iterator()
+        .asScala
+        .map(v => v.name() -> v)
+        .toMap
+

Review comment:
       Sure.  The primary motivation was to drop validators into binary distributions of daffodil, both for cli and artifacts.  The sample application demonstrates the process and has installation instructions in the readme.
   
   > I feel like SPI things are useful when you just want to swap different mechanisms without any code/configuration changes, but it seems like that's not really the case here.
   
   The clearest way to describe it is as a plugin framework.  You can define an interface in your software and at runtime collect all instances of that type that exist on the classpath and use them for whatever.  No need for a client to register anything other than providing a text file in META-INF.  And also no need for the client to package up Daffodil in their binary distribution.
   
   > Swapping different validation mechanisms requires speciying the validation name somewhere (I think) and somehow providing args
   
   The name is provided by the implementation, see the Validator trait.  The args are provided either programmatically or via the CLI.  My primary motivation here was supplementing CLI usage of the binary distribution of Daffodil without recompiling Daffodil, that was what the first implementation was based on, which led to some of the feedback that rightly pointed out CLI concepts were leaking into the API.  Definitely want to make it easy to use programmatically, and I think thats still a bit of a WIP that needs some polish yet.
   
   > so swapping in a new validation mechamisnm still requires code changes.
   
   Only on the client side, Daffodil defines an API, finds things through ServiceLoader, and handles them all abstractly.
   
   Take a look at the sample application that went with the original reference impl.  Its drifted some as I have tried to adjust to the feedback, but the concepts are still the same.
   
   https://github.com/ctc-oss/daffodil-schematron-validator
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] tuxji commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

tuxji commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r518073675



##########
File path: daffodil-lib/src/test/resources/test/validation/myData5.dat
##########
@@ -0,0 +1 @@
+12:345678

Review comment:
       I noticed you also renamed a test .dat file:
   
   daffodil-lib/src/test/resources/test/validation/myData5.dat → ...t/resources/test/validation/testData1.dat 
   
   How is this file used?  I searched through all the GitHub diffs in my browser and I couldn't find any other occurrence of the filename "testData1.dat".  




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510176658



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -230,11 +236,21 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
   }
 
   implicit def validateConverter = singleArgConverter[ValidationMode.Type]((s: String) => {
+    import ValidatorPatterns._
     s.toLowerCase match {
       case "on" => ValidationMode.Full
       case "limited" => ValidationMode.Limited
       case "off" => ValidationMode.Off
-      case _ => throw new Exception("Unrecognized ValidationMode %s.  Must be 'on', 'limited' or 'off'.".format(s))
+      case DefaultArgPattern(name, arg) if Validators.isRegistered(name) =>
+        val config = arg.split(".").last match {
+          case "conf" => ConfigFactory.load(arg)
+          case _ => ConfigFactory.parseMap(Map(name -> arg).asJava)
+        }
+        val validator = Validators.get(name).make(Some(config))
+        ValidationMode.Custom(validator)
+      case NoArgsPattern(name) if Validators.isRegistered(name) =>

Review comment:
       no arg usage looks like `--validate schematron`




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r518023577



##########
File path: daffodil-lib/src/test/resources/test/validation/myData5.dat
##########
@@ -0,0 +1 @@
+12:345678

Review comment:
       updated the encoding




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504144223



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -692,7 +695,8 @@ class DataProcessor private (
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
   with WithDiagnosticsImpl
-  with ErrorHandler {
+  with ErrorHandler
+  with Logging {
 
   /**
    * To be successful here, we need to capture xerces parse/validation

Review comment:
       yeah, probably makes sense to move xerces part over to the DefaultValidator.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r511986830



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -100,3 +111,35 @@ object Validator extends NoBindingFactoryAdapter {
   }
 }
 
+class DefaultValidatorFactory extends ValidatorFactory {
+  def name(): String = "default" /// todo;; or should it be "xerces"
+
+  // to not muck around too much with the original logic above just wrapping access to the

Review comment:
       Refactored the default validator to use the factory pattern.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512156191



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -673,7 +673,7 @@ sealed class ElementRuntimeData(
   def isSimpleType = optPrimType.isDefined
 
   def schemaURIStringsForFullValidation = schemaURIStringsForFullValidation1.distinct
-  private def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:
+  def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:

Review comment:
       opened this up for https://github.com/apache/incubator-daffodil/pull/431/files#diff-f6dc861db0417ffe6d0f80f7e0a166757345c5a3505e76de8f3bb694c1d170f3R259




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504962241



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.CompiledValidator
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.Validator.CompilerOps.CheckArgs
+import org.apache.daffodil.api.ValidatorNotFoundException
+
+import scala.collection.JavaConverters._
+
+object Validators {
+  val default: Validator = new DefaultValidatorSPIProvider
+  def compiler(): ValidatorCompiler = new Compiler
+
+  trait ValidatorCompiler {
+    def find(name: String): Option[CompiledValidator]
+    def isRegistered(name: String): Boolean
+    def compile(name: String, args: Validator.ArgumentList): CompiledValidator
+  }
+
+  private class Compiler extends ValidatorCompiler {
+    private var compiled = Map.empty[String, CompiledValidator]
+    private lazy val impls: Map[String, Validator] =
+      ServiceLoader
+        .load(classOf[Validator])
+        .iterator()
+        .asScala
+        .map(v => v.name() -> v)
+        .toMap
+

Review comment:
       Can you describe the motivation for using a ServiceLoader for providing validators rather than just  providing an API where a user can provide validators (that follow the Validator API, and meeting requirements like thread safe and Serializable)? I feel like SPI things are useful when you just want to swap different mechanisms without any code/configuration changes, but it seems like that's not really the case here. Swapping different validation mechanisms requires speciying the validation name somewhere (I think) and somehow providing args, so swapping in a new validation mechamisnm still requires code 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 edited a comment on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 edited a comment on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-717267525


   Think about the name `DefaultValidator` and whether we want it renamed to `XercesValidator` or something else more appropriate.
   
   The name Default came about because it was the original `Validator` and I needed to take that name for the new stuff.  But we can still mark it as the default without being named Default.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-721755830


   +1, :+1: 
   
   Only suggestion is to fix the UTF-16 files.
   
   And I agree that we should wait until after 3.0.0 is out before merging. This will give us a release cycle to play around with it and determine if any API changes are needed before making this an official API and having to worry about backwards compatbility.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512174647



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -673,7 +673,7 @@ sealed class ElementRuntimeData(
   def isSimpleType = optPrimType.isDefined
 
   def schemaURIStringsForFullValidation = schemaURIStringsForFullValidation1.distinct
-  private def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:
+  def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:

Review comment:
       There we go.  I think its all in order 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510181022



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.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.api
+
+import com.typesafe.config.Config
+
+import scala.xml.SAXException
+
+trait Validator {
+  def validateXML(document: java.io.InputStream): ValidationResult
+}
+
+trait ValidatorFactory {
+  def name(): String
+  def make(config: Option[Config]): Validator
+}
+
+case class ValidationResult(warnings: Seq[ValidationWarning], errors: Seq[ValidationError]) {

Review comment:
       Sketching in a result here as suggested by @mbeckerle previously.  I said kick the can down the road previously, but it seems to clear up the intent of this PR to do it now.  And there werent a significant number of additional changes involved in the end.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504961049



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.CompiledValidator
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.Validator.CompilerOps.CheckArgs
+import org.apache.daffodil.api.ValidatorNotFoundException
+
+import scala.collection.JavaConverters._
+
+object Validators {
+  val default: Validator = new DefaultValidatorSPIProvider
+  def compiler(): ValidatorCompiler = new Compiler
+
+  trait ValidatorCompiler {
+    def find(name: String): Option[CompiledValidator]
+    def isRegistered(name: String): Boolean
+    def compile(name: String, args: Validator.ArgumentList): CompiledValidator
+  }
+
+  private class Compiler extends ValidatorCompiler {
+    private var compiled = Map.empty[String, CompiledValidator]
+    private lazy val impls: Map[String, Validator] =
+      ServiceLoader
+        .load(classOf[Validator])
+        .iterator()
+        .asScala
+        .map(v => v.name() -> v)
+        .toMap
+
+    private def impl(name: String): Option[Validator] = impls.get(name)
+    def find(name: String): Option[CompiledValidator] = compiled.get(name)
+    def isRegistered(name: String): Boolean = impls.contains(name)
+
+    def compile(name: String, args: Validator.ArgumentList): CompiledValidator = {
+      find(name) match {
+        case Some(cv) => cv
+        case None =>
+          val vv = impl(name).getOrElse(throw ValidatorNotFoundException(name))
+          val aa = vv match {
+            case ac: CheckArgs => ac.checkArgs(args)
+            case _             => Validator.Arguments.Empty
+          }
+          val cv = new CompiledValidator(vv, aa)

Review comment:
       Here is where you want to call the specific compilation method for this specific provider, which will return a validator-specific object that implements the CompiledValidator trait. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512170596



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -673,7 +673,7 @@ sealed class ElementRuntimeData(
   def isSimpleType = optPrimType.isDefined
 
   def schemaURIStringsForFullValidation = schemaURIStringsForFullValidation1.distinct
-  private def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:
+  def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:

Review comment:
       > No I don't get it. Why isn't just using the public schemaURIStringForFullValidation sufficient?
   
   I see, will fix it..
   
   I did make that package private in a commit that snuck in during your review.
   
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512188940



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       Everything from here to the top of the method should happen at factory time, not at validateXML time. 
   
   This is the change that motivates the whole design of Factory vs. Validator.
   
   The exception here is line 68.
   
   We may or may not need to construct a ThreadLocal that has its own schema.newValidator() per thread (for this DataProcessor object), depending on whether Xerces validation is a stateful object (I hope not), or returns everything via the result object (I hope so.)
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512170596



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -673,7 +673,7 @@ sealed class ElementRuntimeData(
   def isSimpleType = optPrimType.isDefined
 
   def schemaURIStringsForFullValidation = schemaURIStringsForFullValidation1.distinct
-  private def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:
+  def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:

Review comment:
       > No I don't get it. Why isn't just using the public schemaURIStringForFullValidation sufficient?
   
   ~~Good question.  I have no idea really, but was just trying to keep the original behavior as consistent as possible through my changes.  I havent saw any tests break while refactoring so Im thinking there is no coverage on these parts :)~~
   
   I see, will fix it..
   
   I did make that package private in a commit that snuck in during your review.
   
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-708839422


   Argument handling is getting out of control.  I implemented some stuff today just to move the idea forward but didnt like doing it. 
   
   https://github.com/jw3/incubator-daffodil/blob/validator_spi/impl/daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala#L41-L55
   
   The complexity is that we need to convert the CLI arguments as strings to the proper type.  But only the validator implementation knows how to do that.  The handling of these strings prior to conversion and the interface needed to support the CLI makes the API usage scary.
    
   The code from today to try to bridge some of the gap was modeled after [typesafe config](https://github.com/lightbend/config).  And it hit me tonight we should just use that and it would offload a bunch of complexity and eliminate a bunch of code in this PR.  
   
   Benefits are
   
   - well known library
   - allow us to accept configuration on cli or from file
   - accept configuration in form of java props, json, or hocon
   - can pull configuration from environment
   - provides well known typed getters and setters
   - unsurprising validation and error handling
   - less of Daffodil to maintain
   - Apache 2.0 licensed
   
   Unless there are objections to using it, Im going to work it into the next iteration while responding to the latest feedback.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-722363131


   Looks good! Please squash all commits into a single commit, force push, and this can be merged once 3.0.0 is released. Thanks!


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 edited a comment on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 edited a comment on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-714604786


   Just hit me that given the changes to the custom validation mode the SPI loading is only accessed through CLI.  Looking at working up an example that demonstrates the usefulness (or not) of SPI in non-CLI usage.
   
   I dont think the current approach will change much at all to demonstrate that, so any feedback on the latest should remain applicable.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504692815



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -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
+
+import org.xml.sax.ErrorHandler
+
+trait Validator {
+  def name(): String = getClass.getTypeName.toLowerCase
+
+  def checkArgs(args: Validator.Arguments): Either[String, Unit] = Right(())

Review comment:
       Moved this logic to `Validator.CompilerOps.CheckArgs` the idea being that an implementation can still provide guidance to how arguments should be formed and that guidance is consumed as part of the compilation process.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504638599



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.Validator
+
+object Validators {
+  import scala.collection.JavaConverters._
+
+  private val impls = new ThreadLocal[Map[String, Validator]] {

Review comment:
       I think the thread safety here guards Daffodil as well, where you could have a race at init time of the Validators state.
   
   After sleeping on it I like the thread local here, but if it doesnt make sense Ill pull it out, you fellas know the daffodil flow better than I.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512154007



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -691,41 +702,38 @@ class DataProcessor private (
 
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
-  with WithDiagnosticsImpl
-  with ErrorHandler {
+  with WithDiagnosticsImpl {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+    val v = dp.validationMode match {

Review comment:
       OK.  So the only issue is that `schemaURIStringsForFullValidation1` is private so it cant recurse if I copy that logic, otherwise it fits nicely in there.  Dont know if it runs.
   
   Any thoughts on opening up `schemaURIStringsForFullValidation1`?  Ill commit it as is for review.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512346188



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       hows about this
   
   ```
     private val validator = new ThreadLocal[XercesValidator] {
       override def initialValue(): XercesValidator =
         initializeValidator(schema.newValidator(), DefaultValidator.this, resolver)
     }
   ```
   
   unless you had a specific reason for the def




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504194648



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -702,10 +706,22 @@ class ParseResult(dp: DataProcessor, override val resultState: PState)
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
+
+    val (v, args) = dp.validationMode match {
+      case ValidationMode.Custom(name, args) =>
+        Validators.find(name).getOrElse(
+          Assert.abort(s"Validator '$name' not found")

Review comment:
       > Can we move much of this find logic to when a user calls withValidationMode?
   
   @stevedlawrence I missed your point there earlier.  Yeah that sounds reasonable, Ill move it and see how it looks.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504136376



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -234,7 +236,11 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
       case "on" => ValidationMode.Full
       case "limited" => ValidationMode.Limited
       case "off" => ValidationMode.Off
-      case _ => throw new Exception("Unrecognized ValidationMode %s.  Must be 'on', 'limited' or 'off'.".format(s))
+      case Validator.MultiArgsPattern(str, args) if Validators.exists(str) =>
+        ValidationMode.Custom(str, args.split(",").map(_.split("=")).map(kv => Validator.Argument(kv.head, kv.last)))
+      case Validator.DefaultArgPattern(str, arg) if Validators.exists(str) => ValidationMode.Custom(str, Seq(Validator.Argument(arg)))
+      case Validator.NoArgsPattern(str) if Validators.exists(str) => ValidationMode.Custom(str, Seq.empty)

Review comment:
       Are these patterns expected to be used via an API user? If not, they should probably only be defined in this Main.scala if the format is specific to the CLI usage.

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -702,10 +706,22 @@ class ParseResult(dp: DataProcessor, override val resultState: PState)
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
+
+    val (v, args) = dp.validationMode match {
+      case ValidationMode.Custom(name, args) =>
+        Validators.find(name).getOrElse(
+          Assert.abort(s"Validator '$name' not found")

Review comment:
       Can we move much of this find logic to when a user calls withValidationMode? That way we can detect if a validator exists and has the right args before we try to actually parse things? It would be nice if all the logic here was just the below ``v.validateXML(...)``.
   
   This feels like perhaps it might lead nicely into [DAFFODIL-1749](https://issues.apache.org/jira/browse/DAFFODIL-1749). Or perhaps that should just be done separately, since that's already how our logic behaves right now. Just something to think about as we add this new API to make sure that is possible.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510176416



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -230,11 +236,21 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
   }
 
   implicit def validateConverter = singleArgConverter[ValidationMode.Type]((s: String) => {
+    import ValidatorPatterns._
     s.toLowerCase match {
       case "on" => ValidationMode.Full
       case "limited" => ValidationMode.Limited
       case "off" => ValidationMode.Off
-      case _ => throw new Exception("Unrecognized ValidationMode %s.  Must be 'on', 'limited' or 'off'.".format(s))
+      case DefaultArgPattern(name, arg) if Validators.isRegistered(name) =>

Review comment:
       default arg usage looks like `--validate schematron=/path/to/my.sch`




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 edited a comment on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 edited a comment on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707714379


   I dont understand the CI rat check failures...


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512728138



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -243,6 +250,18 @@ class DataProcessor private (
 
   def withValidationMode(mode:ValidationMode.Type): DataProcessor = copy(validationMode = mode)
 
+  def withValidator(validator: Validator): DataProcessor = copy(validationMode = ValidationMode.Custom(validator))
+
+  lazy val validator: Validator = {

Review comment:
       Right. Maybe I need more coffee today. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504141930



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -702,10 +706,22 @@ class ParseResult(dp: DataProcessor, override val resultState: PState)
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
+
+    val (v, args) = dp.validationMode match {
+      case ValidationMode.Custom(name, args) =>
+        Validators.find(name).getOrElse(
+          Assert.abort(s"Validator '$name' not found")
+        ) -> args
+      case _ => Validators.default -> Seq(Validator.Argument(
+        resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation.mkString(",")
+      ))
+    }
+
+    Assert.usage(resultState.processorStatus eq Success)

Review comment:
       Looks like a dupe anyways with the first line.  Probably refactored a bit and missed it.  Ill remove this 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512170596



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -673,7 +673,7 @@ sealed class ElementRuntimeData(
   def isSimpleType = optPrimType.isDefined
 
   def schemaURIStringsForFullValidation = schemaURIStringsForFullValidation1.distinct
-  private def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:
+  def schemaURIStringsForFullValidation1: Seq[String] = (schemaFileLocation.uriString +:

Review comment:
       > No I don't get it. Why isn't just using the public schemaURIStringForFullValidation sufficient?
   
   Good question.  I have no idea really, but was just trying to keep the original behavior as consistent as possible through my changes.  I havent saw any tests break while refactoring so Im thinking there is no coverage on these parts :)
   
   I did make that package private in a commit that snuck in during your review.
   
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510190534



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -692,40 +699,35 @@ class DataProcessor private (
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
   with WithDiagnosticsImpl
-  with ErrorHandler {
+  with Logging {
 
   /**
-   * To be successful here, we need to capture xerces parse/validation
+   * To be successful here, we need to capture parse/validation
    * errors and add them to the Diagnostics list in the PState.
    *
-   * @param state the initial parse state.
+   * @param bytes the parsed Infoset
    */
   def validateResult(bytes: Array[Byte]): Unit = {
     Assert.usage(resultState.processorStatus eq Success)
-    val schemaURIStrings = resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation
-    try {
-      val bis = new java.io.ByteArrayInputStream(bytes)
-      Validator.validateXMLSources(schemaURIStrings, bis, this)
-    } catch {
-      //
-      // Some SAX Parse errors are thrown even if you specify an error handler to the
-      // validator.
-      //
-      // So we also need this catch
-      //
-      case e: SAXException =>
-        resultState.validationErrorNoContext(e)
+
+
+    val v = dp.validationMode match {
+      case ValidationMode.Custom(cv) => cv
+      case _ =>
+        /// todo;; potential corner case (or not corner?) of needing this dynamic value to configure a validator
+        ///        resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation.mkString(",")
+        val v = ConfigValueFactory.fromAnyRef(resultState.infoset.asInstanceOf[InfosetElement].runtimeData.schemaURIStringsForFullValidation)
+        Validators.default.make(
+          Some(ConfigFactory.parseMap(Map(Validators.default.name() -> v).asJava))

Review comment:
       will add a function somewhere to make this a little more sightly.   Would be nice if these schema names could be obtained some other way at some other point, but I have found that way yet.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512705355



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       The reason  for the def validator, and the val (which should be private val, btw) is just to hide the fact that this is a threadlocal entirely. That's the implementation's business, not the user of the validator. The user of the validator shouldn't have to know that it's a threadlocal and so .get is needed, etc. 
   
   If you put the types on the rationale is clear:
   ```
   def validator: XercesValidator = validatorTL.get
   
   private val validatorTL = new ThreadLocal[XercesValidator] { ... }
   ```
   Unless the caller of the validator.validateXML method looks in the implementation, they don't know anything about this thread-local stuff.
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510188305



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -692,40 +699,35 @@ class DataProcessor private (
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
   with WithDiagnosticsImpl
-  with ErrorHandler {

Review comment:
       `ErrorHandler` impl moves down into the default validator




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512205715



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.util
+
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?
+  with Validator
+  with ErrorHandler {
+
+  private var errors = List.empty[ValidationFailure]
+  private var warnings = List.empty[ValidationWarning]
+
+  def validateXML(document: java.io.InputStream): ValidationResult = {
+    val schemaSources: Seq[javax.xml.transform.Source] = schemaFileNames.map { fn =>
+      val uri = new URI(fn)
+      val is = uri.toURL.openStream()
+      val stream = new StreamSource(is)
+      stream.setSystemId(uri.toString) // must set this so that relative URIs will be created for import/include files.
+      stream
+    }
+
+    val factory = new org.apache.xerces.jaxp.validation.XMLSchemaFactory()
+    factory.setErrorHandler(this)
+    val resolver = DFDLCatalogResolver.get
+    factory.setResourceResolver(resolver)
+    val schema = factory.newSchema(schemaSources.toArray)
+    val validator = initializeValidator(schema.newValidator(), this, resolver)
+

Review comment:
       How about in the validator but handled at init time.  Simplifies some things this way (pushed an update).




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512348163



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.util
+
+import java.lang.ThreadLocal
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.util.DefaultValidator.XercesValidator
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?

Review comment:
       does this still apply?  compiles without 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r518080856



##########
File path: daffodil-lib/src/test/resources/test/validation/myData5.dat
##########
@@ -0,0 +1 @@
+12:345678

Review comment:
       renamed these files to prevent future confusion with the files in the other projects with the only difference being encoding... that could lead to a very bad day.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504135758



##########
File path: daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/packageprivate/Utils.scala
##########
@@ -71,6 +71,8 @@ private[sapi] object ValidationConversions {
       case ValidationMode.Off => SValidationMode.Off
       case ValidationMode.Limited => SValidationMode.Limited
       case ValidationMode.Full => SValidationMode.Full
+      // todo;; hacking this out for now
+      case _ => SValidationMode.Off

Review comment:
       I've alway sort of viewed ``.api`` as a sort of internal API and ``.sapi`` or ``.japi`` as external publicly usble APIs. For example, right now we only generate java/scala docs for for the daffodil-japi/sapi projects, so anything not in those projects will not be documented. So our sapi/japi sort of wrap things in api.
   
   Maybe this modeToScala thing was a way to get what the internal API though the validation mode was and return it as something an sapi user would understand. But it sounds like we don't actually have getters for the current validation mode, so this is never used?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504258248



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -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
+
+import org.xml.sax.ErrorHandler
+
+trait Validator {
+  def name(): String = getClass.getTypeName.toLowerCase
+
+  def checkArgs(args: Validator.Arguments): Either[String, Unit] = Right(())
+
+  def validateXML(document: java.io.InputStream, errHandler: ErrorHandler, args: Validator.Arguments): Unit

Review comment:
       I think rather than requiring validators to cache and hide the fact that validation has a compilation step in it, we should make the API allow for this compilation step explicitly. 
   
   I think the validator you find via SPI should have an explicit compile(params) step that returns a CompiledValidator, and a CompiledValidator should have a validate(xmlInput, errorHandler) call on it. as well as be Serializable so it can be saved/restored as part of the runtime objects of a compiled DFDL schema binary. 
   
   The validate(xmlInput, errorHandler) should be thread safe, and if it depends on underlying non-threadsafe libraries it should deal with using ThreadLocals so that the caller need not be aware of this. 
   
   
   

##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -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
+
+import org.xml.sax.ErrorHandler
+
+trait Validator {
+  def name(): String = getClass.getTypeName.toLowerCase
+
+  def checkArgs(args: Validator.Arguments): Either[String, Unit] = Right(())

Review comment:
       This is a CLI-specific thing? Belongs in the CLI, not in a validator class. That is to say, people defining validators should not be thinking in terms of CLI interfaces, but an 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510795979



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -692,40 +699,35 @@ class DataProcessor private (
 class ParseResult(dp: DataProcessor, override val resultState: PState)
   extends DFDL.ParseResult
   with WithDiagnosticsImpl
-  with ErrorHandler {

Review comment:
       Depends on the underlying validation, this sax handler is not required.  For example the Schematron validator doesnt use callbacks it just returns a list of results you sort through 
   
   https://github.com/ctc-oss/daffodil-schematron-validator/blob/master/src/main/scala/com/ctc/daffodil/ext/SchematronValidator.scala#L19-L22




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 edited a comment on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 edited a comment on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-707710350


   Have a few questions Ill comment on here.
   
   There is a sample validator implementation at https://github.com/ctc-oss/daffodil-schematron-validator
   
   Looking at more tests while in review.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512717040



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -243,6 +250,18 @@ class DataProcessor private (
 
   def withValidationMode(mode:ValidationMode.Type): DataProcessor = copy(validationMode = mode)
 
+  def withValidator(validator: Validator): DataProcessor = copy(validationMode = ValidationMode.Custom(validator))
+
+  lazy val validator: Validator = {

Review comment:
       No this is the cached Validator instance.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

mbeckerle commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r512704889



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.util
+
+import java.lang.ThreadLocal
+import java.net.URI
+
+import com.typesafe.config.Config
+
+import scala.xml.parsing.NoBindingFactoryAdapter
+import javax.xml.XMLConstants
+import javax.xml.transform.stream.StreamSource
+import org.apache.daffodil.api.SaxValidationError
+import org.apache.daffodil.api.SaxValidationWarning
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+import org.apache.daffodil.util.DefaultValidator.XercesValidator
+import org.apache.daffodil.xml.DFDLCatalogResolver
+import org.xml.sax.ErrorHandler
+import org.xml.sax.SAXException
+import org.xml.sax.SAXParseException
+
+import scala.collection.JavaConverters._
+
+/**
+ * Use this for extra validation passes in the TDML Runner
+ * 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 DefaultValidator(schemaFileNames: Seq[String])
+  extends NoBindingFactoryAdapter // todo;; do we need this adapter?

Review comment:
       Take it out if no longer needed. I am not sure why this was mixed in. Perhaps for the load methods? 
   Perhaps copied from other validators in Daffodil, because we validate the TDML files we load, we validate DFDL schemas against the schema-of-DFDL-schemas, etc.  NoBindingFactoryAdapter is also used by DFDLXercesAdapter which is part of DaffodilXMLLoader.scala, and also by TestXMLCatalogAndValidate.scala. 
   
   But if not needed for this usage, take it out. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-710030435


   >I still have concerns that even with something like typesafe config, we're still limiting Validation configuration to what can be represented by some configuration language (e.g. typesafe) just so that we can get plugability.
   
   I take from this your concern is that you want to ensure validator instances can be built and passed programmatically, not only be based on a SPI based  name lookup and argument bundle. 
   
   >I can imagine Validators that really can only be configured with code, granted I'm not sure if anything like that exists in the real world.
   
   Like I said somewhere above, the arg handling has gotten out of hand.  I want to reduce that and make the API aspect more ergonomic while being compatible with passing things from the CLI.  The focus on CLI first instead of API first is what has skewed the initial approach.
   
   >And I don't personally feel that plugability gains much outside of CLI support.
   
   There are plenty of applications that use SPI to supplement API usage.  Probably more useful to show compelling examples in this PR than to debate this point though, so Ill work on that and evolve this over the next few days.
   
   Im still confident the current impl can be tweaked to address the API side concerns.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504270876



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/api/Validator.scala
##########
@@ -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
+
+import org.xml.sax.ErrorHandler
+
+trait Validator {
+  def name(): String = getClass.getTypeName.toLowerCase
+
+  def checkArgs(args: Validator.Arguments): Either[String, Unit] = Right(())
+
+  def validateXML(document: java.io.InputStream, errHandler: ErrorHandler, args: Validator.Arguments): Unit

Review comment:
       OK.  That can probably work.  I see how it relates to your comment above.  Will take a stab at this compiled idea tomorrow.
   
   A gotcha might be derived arguments, for example the schema paths being passed to the DefaultValidator.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504612223



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.Validator
+
+object Validators {
+  import scala.collection.JavaConverters._
+
+  private val impls = new ThreadLocal[Map[String, Validator]] {

Review comment:
       I would expect this to not need a ThreadLocal. I would expect that if a Validator is not thread-safe then the internals of that implementation should have to handle the ThreadLocals. This way, we only incur the overhead of ThreadLocals if a particular implementation actually needs 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r510817917



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/DefaultValidator.scala
##########
@@ -100,3 +111,35 @@ object Validator extends NoBindingFactoryAdapter {
   }
 }
 
+class DefaultValidatorFactory extends ValidatorFactory {
+  def name(): String = "default" /// todo;; or should it be "xerces"
+
+  // to not muck around too much with the original logic above just wrapping access to the

Review comment:
       A rename for clarity might be useful too,  `XercesValidator`




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] jw3 commented on a change in pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

jw3 commented on a change in pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#discussion_r504685087



##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/Validators.scala
##########
@@ -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.util
+
+import java.util.ServiceLoader
+
+import org.apache.daffodil.api.Validator
+
+object Validators {
+  import scala.collection.JavaConverters._
+
+  private val impls = new ThreadLocal[Map[String, Validator]] {

Review comment:
       Pushed a commits that attempts to resolve this discussion using the validation compiler by making loading more explicit and and reducing the scope of the caching, letting it up to the client how to share 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-daffodil] tuxji commented on pull request #431: Custom Infoset validators with SPI

[GitBox <gi...@apache.org>]

tuxji commented on pull request #431:
URL: https://github.com/apache/incubator-daffodil/pull/431#issuecomment-709503610


   I took a look at [typesafe config](https://github.com/lightbend/config) and I noticed the README listed some Scala wrapper libraries.  I was interested so I looked at these as well but most of them haven't been maintained in a while with the exception of [pureconfig](https://github.com/pureconfig/pureconfig).  It might be worth taking a look at pureconfig (it seems very well documented, which is a good sign for maintainability), but you might be better off using config if 1) pureconfig adds more code overhead than it's worth for features you don't want to use, or 2) people think its license (Mozilla Public License 2.0) might not be compatible enough with Apache License 2.0.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org