You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2021/01/22 16:10:49 UTC
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #478: Feature Aware DaffodilParseXMLReader and Prefixes Fixes
mbeckerle commented on a change in pull request #478:
URL: https://github.com/apache/incubator-daffodil/pull/478#discussion_r562697796
##########
File path: daffodil-core/src/test/resources/test/example_nested_namespaces.dfdl.xsd
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<!-- derived from multi_A_02.dfdl.xsd -->
Review comment:
Can you explain what's different in this derivation?
Also, the attributes of this xs:schema element below are important, so putting them 1 per line will make it much easier to spot the xmlns bindings which are the important part I believe.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/xml/QNameBase.scala
##########
@@ -224,8 +224,7 @@ trait QNameBase extends Serializable {
* must show up in diagnostic messages. Mistakes by having the wrong prefix
* or by omitting one, are very common.
*/
- def prefix: Option[String]
- def prefixOrNull: String = prefix.orNull
+ def lexicalPrefix: Option[String]
Review comment:
Add scaladoc of what you mean by lexicalPrefix.
The term lexical is often used to mean "due to surrounding textual context", but I think your intent here is to be clear when we're dealing with the prefix part of a QName, from a prefix that is somehow implied by things elsewhere, like lexically enclosing xmlns default namespace declarations when there is also a prefix for that same namespace, or elementFormDefault on a schema so an element's name will have a namespace and that could have a lexically surrounding xmlns binding, etc.
Either of these concepts can be considered "lexical", so ... scaladoc.
##########
File path: daffodil-core/.settings/org.eclipse.core.resources.prefs
##########
@@ -0,0 +1,3 @@
+#Generated by sbteclipse
Review comment:
Not necessarily in this commit, but we're striving to get IDE-specific files out of the tree.
##########
File path: daffodil-core/src/test/scala/org/apache/daffodil/processor/TestSAXParseAPI.scala
##########
@@ -174,4 +190,46 @@ class TestSAXParseAPI {
assertTrue(pr.isError)
assertTrue(spe.getMessage.contains("Insufficient bits in data"))
}
+
+ @Test def testDaffodilParseXMLReader_parse_features_prefixes_only(): Unit = {
+ val xmlReader = dpNamespaces.newXMLReaderInstance
+ val baos = new ByteArrayOutputStream()
+ val parseOutputStreamContentHandler = new DaffodilParseOutputStreamContentHandler(baos)
+ xmlReader.setContentHandler(parseOutputStreamContentHandler)
+ xmlReader.setFeature(XMLUtils.SAX_NAMESPACES_FEATURE, false)
+ xmlReader.setFeature(XMLUtils.SAX_NAMESPACE_PREFIXES_FEATURE, true)
+ val inArray = testNamespacesData.getBytes()
+ xmlReader.parse(inArray)
+ val pr = xmlReader.getProperty(XMLUtils.DAFFODIL_SAX_URN_PARSERESULT).asInstanceOf[ParseResult]
+ assertTrue(!pr.isError)
+ assertEquals(testNamespacesInfoset, scala.xml.XML.loadString(baos.toString))
Review comment:
Coding for debug guidelines say you should have
```
val actualInfoset = scala.xml.XML.loadString(baos.toString)
```
If these tests fail that's first thing I want to look at. Also makes this a little bit more obvious what's going on.
Also lines 196 to 204 are the same I think in all 4 tests, save the two booleans on lines 199, 200, so factoring that out would make the fact that these booleans are really the only difference in the way the tests work clear. I think you want
```
val actualInfoset = saxParseWithFeatures(false, true)
```
where that method captures lines 195 to 204.
##########
File path: daffodil-core/src/test/resources/test/example_no_targetnamespace.dfdl.xsd
##########
@@ -0,0 +1,31 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<!-- copied from multi_C_02.dfdl.xsd -->
Review comment:
If this is a copy, why not just use the original one by reference?
##########
File path: daffodil-core/src/test/scala/org/apache/daffodil/processor/TestSAXParseAPI.scala
##########
@@ -174,4 +190,46 @@ class TestSAXParseAPI {
assertTrue(pr.isError)
assertTrue(spe.getMessage.contains("Insufficient bits in data"))
}
+
+ @Test def testDaffodilParseXMLReader_parse_features_prefixes_only(): Unit = {
Review comment:
Despite the long names, I still don't grok really what is being tested.
Or are there really just 4 combinations of the two prefix/namespace "features" 3 of which are legal, and you just want to test all combinations?
Some scaladoc on these tests to explain their specifics would be helpful.
##########
File path: daffodil-core/src/test/resources/test/example_a02_targetnamespace.dfdl.xsd
##########
@@ -0,0 +1,31 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<!-- derived from multi_C_02.dfdl.xsd -->
+<xs:schema targetNamespace="http://a02.com" xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/">
+
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+
+ <xs:annotation>
+ <xs:appinfo source="http://www.ogf.org/dfdl/">
+ <dfdl:format ref="GeneralFormat" separator="" initiator="" separatorPosition="infix" ignoreCase="no" separatorSuppressionPolicy="anyEmpty" terminator="" occursCountKind="parsed" initiatedContent="no" representation="text" textNumberRep="standard" encoding="ASCII" textTrimKind="none" leadingSkip='0'/>
Review comment:
line too long
##########
File path: daffodil-core/src/test/scala/org/apache/daffodil/processor/TestSAXParseUnparseAPI.scala
##########
@@ -49,11 +50,21 @@ object TestSAXParseUnparseAPI {
</xs:sequence>
</xs:complexType>
)
- val testInfoset: Elem = <list xmlns="http://example.com"><w>9</w><w>1</w><w>0</w></list>
- val testInfosetString: String = testInfoset.toString()
- val testData = "910"
+ lazy val testInfoset: Elem = <list xmlns="http://example.com"><w>9</w><w>1</w><w>0</w></list>
+ lazy val testInfosetString: String = testInfoset.toString()
+ lazy val testData: String = "910"
+
+ lazy val schemaFile: File = getResource("/test/example_nested_namespaces.dfdl.xsd")
+ lazy val testNamespacesSchema: Elem = scala.xml.XML.loadFile(schemaFile)
+
+ lazy val testNamespacesInfoset: Elem =
+ <b02:seq xmlns:b02="http://b02.com" xmlns:a02="http://a02.com"><b02:seq2><a02:inty>3</a02:inty></b02:seq2><b02:seq2><b02:inty>4</b02:inty></b02:seq2><b02:seq2><a02:inty>2</a02:inty></b02:seq2><b02:seq2><b02:inty>1</b02:inty></b02:seq2><b02:seq2><b02:inty>44</b02:inty></b02:seq2><b02:seq2><b02:inty>643</b02:inty></b02:seq2><b02:seq2><a02:inty>3</a02:inty></b02:seq2><b02:seq2><a02:inty>5</a02:inty></b02:seq2><b02:seq2><b02:inty>1</b02:inty></b02:seq2></b02:seq>
Review comment:
line too long. Needs indentation.
##########
File path: daffodil-core/src/test/scala/org/apache/daffodil/processor/TestSAXParseUnparseAPI.scala
##########
@@ -71,6 +82,15 @@ object TestSAXParseUnparseAPI {
}
dp
}
+
+ private def getResource(resPath: String): File = {
+ val f = try {
+ new File(this.getClass().getResource(resPath).toURI())
+ } catch {
+ case _: Throwable => null
Review comment:
Do you really depend on this catch? Are there places where your testing needs a null back from this?
If not I suggest just removing the try/catch here.
----------------------------------------------------------------
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