WIP: Draft - Config feature to allow XMLConversionControl to preserve CRLF and CR in data #912

mbeckerle · 2023-01-14T09:10:58Z

Review ideas.

Ignore JAPI/SAPI changes for now.

Look only at XMLTextInfosetInputter and XMLTextInfosetOutputter, and ignore all the other variants thereof.

Because the changes across JAPI/SAPI and these inputters/outputters are very redundant. All same everywhere.

The real guts of this thing are:

changes to config file so that we can add

<xmlConversionControl carriageReturnMapping="PreserveCR"/>

Which causes CRs to be presreved by remapping to the PUA.

This can be extended with other attributes to control other aspects of XML Conversion as is needed for other open tickets (DAFFODIL-2346).

The actual remapping code has been consolidated and revised, but is mostly the same.

DAFFODIL-1559

stevedlawrence

Two main things that I'm unsure about in his change:

Lots of backwards incompatible changes with he infoset inputters/outputters now requiring XCC. I don't think this should be mandatory, most people don't deal with CR/LF acually in their data the default should be fine.
I'm no sure about storing the config in the dataprocessor. Feel like XCC should just be another paramter that the CLI/TDMLRunner etc extracts from the config (or not) just like variable/tunables/ec and passes it to the infoset inputter/ouputter.

I also sort of wish this was split up into separae PRs for refactoring of the remapping stuff and changing how the config is handled and API changes. The refactoring is uncontroversial, but the config/API changes feel more controversial and maybe there's a better approach to consider.

stevedlawrence · 2023-01-17T14:47:23Z

daffodil-cli/src/it/resources/org/apache/daffodil/CLI/input/inputWithCRLFs.txt.dat

@@ -0,0 +1,3 @@
+abc
+def
+ghi


I don't think you can rely on git not munging these CRLFs on windows. We need an alternative method to ensure that we actually send CRLF as test data. One option is to use withTempFile, write the bytes directly to that and then parse with the input file being the temp file.

stevedlawrence · 2023-01-17T14:50:34Z

daffodil-cli/src/it/scala/org/apache/daffodil/xml/TestXMLConversionControl.scala

+  // rename the src/test dir to src/test1. Rename the src/it dir to src/test.
+  // Then modify this val to be "test".
+  // Then you can run these as ordinary junit-style tests under the IDE.
+  val test = "it"


I'd prefer we solve this issue and update our entire integration suite in a separate PR. Making one-off change to a subset of our integration tests made it really difficult to update everything to be consistent.

I'm of the opinion that the 267 integration tests run fast enough now that we should just lump them in under regular src/test now, and not bother with the separate src/it.

However, it would be good to move the stage build tasks so that you don't have to wait for the javadoc/scaladoc to build every time you want to make a small changes and then rerun the tests.

Created https://issues.apache.org/jira/browse/DAFFODIL-2772 about fixing the src/it tests to be ordinary src/test.

stevedlawrence · 2023-01-17T14:51:01Z

daffodil-cli/src/it/scala/org/apache/daffodil/xml/TestXMLConversionControl.scala

+    withTempFile { output =>
+      val schema = path(s"daffodil-cli/src/$test/resources/org/apache/daffodil/CLI/aString.dfdl.xsd")
+      val config = path(s"daffodil-cli/src/$test/resources/org/apache/daffodil/CLI/config-convertCR.cfg.xml")
+      val input = path(s"daffodil-cli/src/$test/resources/org/apache/daffodil/CLI/input/inputWithCRLFs.txt.dat")


Suggest moving these outside the withTempFile block since they don't need it. Makes it more consistent with the rest of our test suite.

stevedlawrence · 2023-01-17T14:58:33Z

daffodil-cli/src/it/scala/org/apache/daffodil/xml/TestXMLConversionControl.scala

+
+
+        val xml = FileUtils.readFileToString(xmlOut.toFile, StandardCharsets.UTF_8)
+        println(xml)


Remove prinln

stevedlawrence · 2023-01-17T15:00:33Z

daffodil-cli/src/it/scala/org/apache/daffodil/xml/TestXMLConversionControl.scala

+
+        var cmd = args"parse -s $schema -c $config --root a -o $xmlOut $input "
+        runCLI(cmd) { cli =>
+          cli.expect(s"")


The body of he runCLI function can be empty. You don't need to expect the empty string.

stevedlawrence · 2023-01-17T16:24:02Z

daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd

-      </xs:all>
-    </xs:complexType>
-  </xs:element>
+  <xs:element name="tunables" type="tns:tunablesType"/>


Why this change? It's hard to tell if there are actual changes to the tunables.

Yeah, I'm inclined to remove all changes to the config file stuff, switch to a simple properties file for the xml controls, and treat the existing config file stuff as legacy.

However, it has become clear to me that some schemas just require a config file in order to be used, and the same will hold for these xml conversion control parameters. Without them the CLI parse/unparse, the TDML runner, or using them via API are going to always require a config along with certain XML conversion controls.

stevedlawrence · 2023-01-17T16:25:40Z

daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd

+      <!--
+      TODO: really these all should have been XML attributes
+      -->
+      <xs:element name="allowExpressionResultCoercion" type="xs:boolean" default="true" minOccurs="0" form="unqualified">


Is this all copy paste from above, just with a different type? Is there a way around this? This is going to be a pain to keep these in sync.

stevedlawrence · 2023-01-17T16:27:37Z

daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd

+      <xs:choice>
+        <!-- allow qualified or unqualified form -->
+        <xs:element ref="tns:tunables" minOccurs="0"/>
+        <xs:element name="tunables" minOccurs="0" type="tns:tunablesTypeUnqualified" form="unqualified"/>


I suggest we just pick one and go with it (or switch to a easier config format like .properties) and not worry about backwards compa for config files. I don't think they are used that often, and it's easy to change them.

stevedlawrence · 2023-01-17T16:30:19Z

daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala

    val bos = new java.io.ByteArrayOutputStream()
-    val xml = new XMLTextInfosetOutputter(bos, true)
+    val xml = new XMLTextInfosetOutputter(bos, true, daffodilConfig.xmlConversionControl)


I suggest the debugger is just opinionated about this XML conversions, or if we do want it to be modifiable then it be a debugger setting with the set command. Having to pass around this config for one option feels unnecessary.

stevedlawrence · 2023-01-17T16:32:28Z

daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/NullInfosetInputter.scala

+  override val xmlConversionControl: XMLConversionControl) extends InfosetInputter
+  with XMLInfosetInputterMixin {
+
+  lazy val events: Array[NullInfosetInputter.Event] = toEvents(is)


I think this is incorrect. Converting to an array of events must happen outside of the creation of the NullInfosetInputer to avoid including converting the stream to events in performance metrics. Which means converting it to events must know about the XCC and not the infoset inputter.

I think changes to this file need to be reverted.

mbeckerle

Switching this back to a WIP/Draft PR.

There is much work to do on this before it is sensible to review it again.

mbeckerle · 2023-01-17T01:04:08Z

daffodil-cli/src/it/resources/org/apache/daffodil/CLI/input/test_DFDL-714.txt

@@ -1 +1 @@
-Hello World 
+Hello World 


I don't know why this file showed up here. It is identical to prior version.

mbeckerle · 2023-01-17T01:04:27Z

daffodil-cli/src/it/resources/org/apache/daffodil/CLI/trace_input.dfdl.xsd

@@ -1,56 +1,56 @@
-<?xml version="1.0" encoding="UTF-8"?>


This file also identical. I don't know why it is here in this changeset.

mbeckerle · 2023-01-17T21:23:29Z

daffodil-cli/src/it/scala/org/apache/daffodil/xml/TestXMLConversionControl.scala

+  // rename the src/test dir to src/test1. Rename the src/it dir to src/test.
+  // Then modify this val to be "test".
+  // Then you can run these as ordinary junit-style tests under the IDE.
+  val test = "it"


I'm of the opinion that the 267 integration tests run fast enough now that we should just lump them in under regular src/test now, and not bother with the separate src/it.

However, it would be good to move the stage build tasks so that you don't have to wait for the javadoc/scaladoc to build every time you want to make a small changes and then rerun the tests.

mbeckerle · 2023-01-17T21:24:28Z

daffodil-cli/src/it/scala/org/apache/daffodil/xml/TestXMLConversionControl.scala

+  }
+
+  //
+  // Illustrates a problem with the expect library (perhaps?)


Working as designed then. I will just remove this test.

mbeckerle · 2023-01-17T21:35:54Z

daffodil-core/src/test/scala/org/apache/daffodil/dsom/TestExternalVariables.scala

@@ -332,7 +332,7 @@ class TestExternalVariables {
    val dp1 = pf.onPath("/")
    val dp2 = pf.onPath("/").withExternalVariables(variables)

-    val outputter = new ScalaXMLInfosetOutputter()
+    val outputter = new ScalaXMLInfosetOutputter(dp2.daffodilConfig.xmlConversionControl)


This is a good point.

I only attached the config to the dp because i learned by trying it, that the xmlConversionControl object needed to get passed to exactly everywhere the dp was already being passed. Because to run something, in the CLI, or in the TDML runner, you have to configure both the daffodil DP, AND at least one infoset inputter/outputter, which usually means one of the XML ones. So the two objects (DP and config info) just go together everywhere.

But I will look into refactoring this so that the DP object doesn't carry the config at all.

That will straighten out the NullInfosetInputter as well.

mbeckerle · 2023-01-17T21:42:23Z

daffodil-lib/src/main/scala/org/apache/daffodil/util/CharacterSetRemapper.scala

+    if (len != 0)
+      while (pos < len) {
+        c = s(pos)
+        if (remapChar(c) != c)


Bug. Yes, a lone surrogate (which you can only tell from context) must be remapped.

mbeckerle · 2023-01-17T21:42:30Z

daffodil-lib/src/main/scala/org/apache/daffodil/util/CharacterSetRemapper.scala

+   */
+  protected def remap (prev: Int, curr: Int, next: Int): Int
+
+  private def needsRemapping(s: String): Boolean = {


That's a good fix. Avoids on average 1/2 a pass over the data. Fixes surrogate bug with needsRemapping.

mbeckerle · 2023-01-17T21:48:22Z

daffodil-propgen/src/main/resources/org/apache/daffodil/xsd/dafext.xsd

-      </xs:all>
-    </xs:complexType>
-  </xs:element>
+  <xs:element name="tunables" type="tns:tunablesType"/>


Yeah, I'm inclined to remove all changes to the config file stuff, switch to a simple properties file for the xml controls, and treat the existing config file stuff as legacy.

However, it has become clear to me that some schemas just require a config file in order to be used, and the same will hold for these xml conversion control parameters. Without them the CLI parse/unparse, the TDML runner, or using them via API are going to always require a config along with certain XML conversion controls.

DAFFODIL-1559

mbeckerle · 2023-08-16T20:52:45Z

Scrapping this PR.

mbeckerle force-pushed the daf-1559-crlf branch 4 times, most recently from 14878d8 to 01f2964 Compare January 14, 2023 10:03

mbeckerle changed the title ~~DRAFT: Checkpoint on work to disable CRLF->LF~~ Config feature to allow XMLConversionControl to preserve CRLF and CR in data Jan 17, 2023

mbeckerle requested review from stevedlawrence and jadams-tresys January 17, 2023 13:07

mbeckerle mentioned this pull request Jan 17, 2023

Allow <![CDATA[]]> to preserve whitespace in XML text content #821

Merged

stevedlawrence reviewed Jan 17, 2023

View reviewed changes

mbeckerle changed the title ~~Config feature to allow XMLConversionControl to preserve CRLF and CR in data~~ WIP: Draft - Config feature to allow XMLConversionControl to preserve CRLF and CR in data Jan 17, 2023

mbeckerle commented Jan 17, 2023

View reviewed changes

mbeckerle removed the request for review from jadams-tresys January 17, 2023 21:51

mbeckerle marked this pull request as draft January 17, 2023 21:51

mbeckerle force-pushed the daf-1559-crlf branch from bd3bcba to d1a9e7f Compare February 13, 2023 23:32

Earlier commit (f5ca77) missed this useful comment.

b106d7b

DAFFODIL-1559

mbeckerle force-pushed the daf-1559-crlf branch from d1a9e7f to b106d7b Compare August 16, 2023 20:52

mbeckerle closed this Aug 16, 2023

mbeckerle deleted the daf-1559-crlf branch August 17, 2023 11:47

mbeckerle mentioned this pull request May 20, 2024

WIP: checkpoint on preserve CR issue #1244

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Draft - Config feature to allow XMLConversionControl to preserve CRLF and CR in data #912

WIP: Draft - Config feature to allow XMLConversionControl to preserve CRLF and CR in data #912

mbeckerle commented Jan 14, 2023 •

edited

Loading

stevedlawrence left a comment

stevedlawrence Jan 17, 2023

stevedlawrence Jan 17, 2023

mbeckerle Jan 17, 2023

mbeckerle Jan 18, 2023

stevedlawrence Jan 17, 2023

stevedlawrence Jan 17, 2023

stevedlawrence Jan 17, 2023

stevedlawrence Jan 17, 2023

mbeckerle Jan 17, 2023

stevedlawrence Jan 17, 2023

stevedlawrence Jan 17, 2023

stevedlawrence Jan 17, 2023

stevedlawrence Jan 17, 2023

mbeckerle left a comment

mbeckerle Jan 17, 2023

mbeckerle Jan 17, 2023

mbeckerle Jan 17, 2023

mbeckerle Jan 17, 2023

mbeckerle Jan 17, 2023

mbeckerle Jan 17, 2023

mbeckerle Jan 17, 2023

mbeckerle Jan 17, 2023

mbeckerle commented Aug 16, 2023

@@ @@ -0,0 +1,3 @@ @@
+              abc
+              def
+              ghi



		val xml = FileUtils.readFileToString(xmlOut.toFile, StandardCharsets.UTF_8)
		println(xml)

		@@ -1 +1 @@
		Hello World
		Hello World

WIP: Draft - Config feature to allow XMLConversionControl to preserve CRLF and CR in data #912

WIP: Draft - Config feature to allow XMLConversionControl to preserve CRLF and CR in data #912

Conversation

mbeckerle commented Jan 14, 2023 • edited Loading

stevedlawrence left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbeckerle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbeckerle commented Aug 16, 2023

mbeckerle commented Jan 14, 2023 •

edited

Loading