Fix: Allow Most chars for variable Values #27

tkruse · 2018-06-27T02:32:17Z

Fix #25: allow any string except null as var value. XML-Unescaping is already done by the SAXParser, no code change needed. But allowing blanks for values required relaxing SystemTestDocReader. Identifiers could also be relaxed in the future that way (I see no reason beyond western-thinking esthetics to restrict to ascii Java word chars), but that is not so urgent for me, programmers should use ASCII for actual identifiers.

TODO Fix failing tests, add tests for newly valid non-word characters (blank, japanese, special)

tkruse · 2018-06-27T02:35:18Z

tcases-lib/src/main/java/org/cornutum/tcases/DefUtils.java

@@ -77,5 +100,6 @@ public static void assertPropertyIdentifiers( Collection<String> properties) thr
    }

  private static final Pattern identifierRegex_ = Pattern.compile( "[\\w\\-]+");
+  private static final Pattern varValueRegex_ = Pattern.compile( "[^\\p{Cntrl}]*");


avoiding chars like newline, escape, bell. Those could be encoded in XML1.1, but the SAX parser fails to recognize them.

tkruse · 2018-06-27T02:36:26Z

tcases-lib/src/main/java/org/cornutum/tcases/io/SystemTestDocReader.java

@@ -194,8 +202,7 @@ public String toIdPath( String attributeName, String attributeValue) throws SAXE
     */
    public String getAttribute( Attributes attributes, String attributeName)
      {
-      String value = attributes.getValue( attributeName);
-      return StringUtils.isBlank( value)? null : value;


a single whitespace seems like an important boundary test case value to me, for testing various functions.

tkruse · 2018-06-27T02:38:11Z

tcases-lib/src/main/java/org/cornutum/tcases/util/XmlWriter.java

@@ -125,7 +128,7 @@ public void writeAttribute( String name, String value)
    print( " ");
    print( name);
    print( "=\"");
-    print( value);
+    print( StringEscapeUtils.escapeXml10(value));


not sure about all the differences between XML 1.0 and 1.1. This one suggests to go with 1.0 when in doubt: https://stackoverflow.com/questions/6260975/should-i-learn-xml-1-0-or-xml-1-1

kerrykimbrough · 2018-06-29T17:41:52Z

These changes seem basically OK. But I don't see any new unit tests to verify them.

tkruse · 2018-06-30T01:22:13Z

Yes, new tests need to be added, my free time is almost over, but I can work on adding tests next week.

tkruse · 2018-06-30T06:49:15Z

I have added more tests, and an equivalent change to SystemInputDefReader. Also I allowed whitespace and some control characters additionally, as they can be represented in XML1.1.

Some control characters cannot be parsed with the JDK parser however (such as newline I think), so users would need to add a more XML1.1 compliant parser on the classpath, such as in maven adding simply:

<dependency>
  <groupId>xerces</groupId>
  <artifactId>xercesImpl</artifactId>
  <version>2.12.0</version>
  <scope>runtime</scope>
</dependency>

This replaces the SAXParser implementation automatically. I manually added to tests and saw that additional control character sequences can be parsed that way. But I did not add tests for that since tcases probably should not depend on xerces.

For other identifiers such as System and function name, I now think it would be preferable to allow international character sets, but that's more of a nuisance than a blocker, so up to you.

For properties I wish I could use some more characters like {[]}=%#, to auto-generate property names that do not clash with user-properties easily, and look decent. Not sure if you are willing to negotiate about that.

kerrykimbrough · 2018-06-30T17:59:18Z

For other identifiers such as System and function name, I now think it would be preferable to allow international character sets

I agree -- see new issue #30. This has to be done carefully, but I will investigate soon.

kerrykimbrough · 2018-06-30T18:00:34Z

For properties I wish I could use some more characters like {[]}=%#, to auto-generate property names

For now, perhaps prefixing with something like "__" might be a reasonable convention.

kerrykimbrough · 2018-06-30T18:35:25Z

tcases-lib/src/main/java/org/cornutum/tcases/util/XmlWriter.java

@@ -125,7 +128,7 @@ public void writeAttribute( String name, String value)
    print( " ");
    print( name);
    print( "=\"");
-    print( value);
+    print( NumericEntityEscaper.below(0x20).translate(StringEscapeUtils.escapeXml11(value)));


Should add a comment to explain this translation. For example, why are both NumericEntityEscaper and StringEscapeUtils required?

I added a comment

tkruse mentioned this pull request Jun 27, 2018

Allow any String as Variable Value, not just identifiers #25

Closed

tkruse commented Jun 27, 2018

View reviewed changes

tkruse force-pushed the feature/values-not-identifiers branch from 8790e78 to c8a1cb9 Compare June 27, 2018 05:26

Fix: Allow Most chars for variable Values

296203c

tkruse force-pushed the feature/values-not-identifiers branch from c8a1cb9 to 296203c Compare June 29, 2018 06:52

Kerry Kimbrough added 2 commits June 29, 2018 14:19

Minor simplifications and cleanups

d61e107

Fix failure test case: blank Var value no longer invalid

63ec852

kerrykimbrough mentioned this pull request Jun 29, 2018

configurable NA and validation #29

Closed

tkruse force-pushed the feature/values-not-identifiers branch from fd3aa83 to f041e0a Compare June 30, 2018 06:43

Additional patch to SystemInputDocReader, tests

2d685f8

tkruse force-pushed the feature/values-not-identifiers branch from f041e0a to 2d685f8 Compare June 30, 2018 06:52

kerrykimbrough reviewed Jun 30, 2018

View reviewed changes

Kerry Kimbrough and others added 3 commits June 30, 2018 13:58

writeAttributeWithValue: Fix compiler warning about resource leak

3c4b933

Fix Javadoc for new test case

8673898

javadoc

4aee214

tkruse force-pushed the feature/values-not-identifiers branch from 033e8bc to 4aee214 Compare July 1, 2018 14:04

kerrykimbrough merged commit 0871938 into Cornutum:master Jul 1, 2018

tkruse deleted the feature/values-not-identifiers branch July 8, 2018 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Allow Most chars for variable Values #27

Fix: Allow Most chars for variable Values #27

tkruse commented Jun 27, 2018 •

edited

Loading

tkruse Jun 27, 2018

tkruse Jun 27, 2018

tkruse Jun 27, 2018

kerrykimbrough commented Jun 29, 2018

tkruse commented Jun 30, 2018

tkruse commented Jun 30, 2018 •

edited

Loading

kerrykimbrough commented Jun 30, 2018

kerrykimbrough commented Jun 30, 2018

kerrykimbrough Jun 30, 2018

tkruse Jul 1, 2018

Fix: Allow Most chars for variable Values #27

Fix: Allow Most chars for variable Values #27

Conversation

tkruse commented Jun 27, 2018 • edited Loading

tkruse Jun 27, 2018

Choose a reason for hiding this comment

tkruse Jun 27, 2018

Choose a reason for hiding this comment

tkruse Jun 27, 2018

Choose a reason for hiding this comment

kerrykimbrough commented Jun 29, 2018

tkruse commented Jun 30, 2018

tkruse commented Jun 30, 2018 • edited Loading

kerrykimbrough commented Jun 30, 2018

kerrykimbrough commented Jun 30, 2018

kerrykimbrough Jun 30, 2018

Choose a reason for hiding this comment

tkruse Jul 1, 2018

Choose a reason for hiding this comment

tkruse commented Jun 27, 2018 •

edited

Loading

tkruse commented Jun 30, 2018 •

edited

Loading