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: [email protected]
