OPENNLP-1933: Parse event lines, MASC ids, and dictionary columns as intended

Review pass over the scans on this branch. Where a scan had copied a
String.split or replaceFirst quirk, it now does what the format means:

- Event lines: RealValueFileEventStream.parseEvent takes the outcome as
  the first whitespace separated field, shared with RealBasicEventStream.
  A tab or a run after the outcome no longer breaks the line, an outcome
  only line is an event without contexts instead of the end of the
  stream, and a blank line fails with an InvalidFormatException.
- SimpleEventStreamBuilder splits the outcome at the first slash, so a
  context may contain one, and rejects an event without contexts.
- MascIdentifiers.parseId requires the prefix at the start followed by
  ASCII digits only; parseIds parses link targets separated by whitespace
  runs. A doubled or missing prefix is reported instead of parsed.
- FrequencyDictionaryLoader.splitColumns returns the non-empty columns
  only, so leading and repeated separators make no empty column.

Tests no longer compare a scan with the pattern it replaced; they pin
the accepted and rejected inputs directly. The manual describes the
event file line format, the Leipzig file name rule, and the dictionary
column separators.

Author: krickert

opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/leipzig/LeipzigLanguageSampleStream.java

@@ -44,6 +44,9 @@
 
 public class LeipzigLanguageSampleStream implements ObjectStream<LanguageSample> {
 
+  /** The number of leading file name characters that form the ISO 639-3 language code. */
+  private static final int LANG_CODE_LENGTH = 3;
+
   private class LeipzigSentencesStream implements ObjectStream<LanguageSample> {
 
     private final String lang;
@@ -134,9 +137,6 @@ public LanguageSample read() throws IOException {
     }
   }
 
-  /** The number of leading file name characters that carry the ISO 639-3 language code. */
-  private static final int LANG_CODE_LENGTH = 3;
-
   private final int sentencesPerSample;
 
   private final Map<String, Integer> langSampleCounts;

opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/masc/MascIdentifiers.java

@@ -17,10 +17,13 @@
 
 package opennlp.tools.formats.masc;
 
+import opennlp.tools.tokenize.WhitespaceTokenizer;
+import opennlp.tools.util.StringUtil;
+
 /**
  * Shared handling of the identifier attributes in MASC annotation files. Node, region,
- * and named entity identifiers carry a fixed text prefix followed by a number; the parsers
- * remove the prefix before parsing the number.
+ * and named entity identifiers are a fixed text prefix followed by a number, as in
+ * {@code penn-n7}; the parsers read the number and require the prefix.
  */
 final class MascIdentifiers {
 
@@ -37,19 +40,46 @@ private MascIdentifiers() {
   }
 
   /**
-   * Removes the first occurrence of {@code literal} from {@code input}. Later occurrences
-   * stay in place, and {@code input} is returned unchanged if it does not contain
-   * {@code literal}. The search is a plain text comparison.
+   * Parses the number of an identifier that starts with {@code prefix} and continues with
+   * one or more ASCII digits only.
    *
-   * @param input The text to search. Must not be {@code null}.
-   * @param literal The text to remove. Must not be {@code null}.
-   * @return {@code input} without its first occurrence of {@code literal}.
+   * @param id The identifier, such as {@code penn-n7}.
+   * @param prefix The expected prefix, such as {@link #PENN_TOKEN_ID_PREFIX}.
+   * @return The number after the prefix.
+   * @throws IllegalArgumentException If {@code id} is {@code null}, does not start with
+   *         {@code prefix}, or is not followed by digits only.
    */
-  static String removeFirst(String input, String literal) {
-    final int start = input.indexOf(literal);
-    if (start < 0) {
-      return input;
+  static int parseId(String id, String prefix) {
+    if (id == null || !id.startsWith(prefix) || id.length() == prefix.length()
+        || StringUtil.endOfAsciiDigits(id, prefix.length()) != id.length()) {
+      throw new IllegalArgumentException(
+          "MASC identifier must be " + prefix + " followed by digits: " + id);
+    }
+    return Integer.parseInt(id, prefix.length(), id.length(), 10);
+  }
+
+  /**
+   * Parses a whitespace separated list of identifiers, each as {@link #parseId(String, String)}
+   * does.
+   *
+   * @param ids The identifiers, such as {@code seg-r1 seg-r2}.
+   * @param prefix The expected prefix of each identifier.
+   * @return The numbers in order.
+   * @throws IllegalArgumentException If {@code ids} is {@code null}, names no identifier, or
+   *         contains one that {@link #parseId(String, String)} rejects.
+   */
+  static int[] parseIds(String ids, String prefix) {
+    if (ids == null) {
+      throw new IllegalArgumentException("MASC identifier list must not be null");
+    }
+    String[] tokens = WhitespaceTokenizer.INSTANCE.tokenize(ids);
+    if (tokens.length == 0) {
+      throw new IllegalArgumentException("MASC identifier list must name at least one identifier");
+    }
+    int[] numbers = new int[tokens.length];
+    for (int i = 0; i < tokens.length; i++) {
+      numbers[i] = parseId(tokens[i], prefix);
     }
-    return input.substring(0, start) + input.substring(start + literal.length());
+    return numbers;
   }
 }

opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/masc/MascNamedEntityParser.java

@@ -53,8 +53,8 @@ public void startElement(String uri, String localName, String qName, Attributes
 
     try {
       if (qName.equals("a")) {
-        int entityID = Integer.parseInt(
-            MascIdentifiers.removeFirst(attributes.getValue("ref"), MascIdentifiers.NAMED_ENTITY_ID_PREFIX));
+        int entityID = MascIdentifiers.parseId(attributes.getValue("ref"),
+            MascIdentifiers.NAMED_ENTITY_ID_PREFIX);
         String label = attributes.getValue("label");
         if (entityIDtoEntityType.containsKey(entityID)) {
           throw new SAXException("Multiple labels for one named entity");
@@ -64,10 +64,10 @@ public void startElement(String uri, String localName, String qName, Attributes
       }
 
       if (qName.equals("edge")) {
-        int entityID = Integer.parseInt(
-            MascIdentifiers.removeFirst(attributes.getValue("from"), MascIdentifiers.NAMED_ENTITY_ID_PREFIX));
-        int tokenID = Integer.parseInt(
-            MascIdentifiers.removeFirst(attributes.getValue("to"), MascIdentifiers.PENN_TOKEN_ID_PREFIX));
+        int entityID = MascIdentifiers.parseId(attributes.getValue("from"),
+            MascIdentifiers.NAMED_ENTITY_ID_PREFIX);
+        int tokenID = MascIdentifiers.parseId(attributes.getValue("to"),
+            MascIdentifiers.PENN_TOKEN_ID_PREFIX);
 
         if (!entityIDsToTokens.containsKey(entityID)) {
           List<Integer> tokens = new ArrayList<>();

opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/masc/MascPennTagParser.java

@@ -55,29 +55,23 @@ public void startElement(String uri, String localName, String qName, Attributes
     try {
       //get the link between region and Penn tag
       if (qName.equals("node")) {
-        tokenStack.push(Integer.parseInt(MascIdentifiers.removeFirst(
-            attributes.getValue("xml:id"), MascIdentifiers.PENN_TOKEN_ID_PREFIX)));
+        tokenStack.push(MascIdentifiers.parseId(
+            attributes.getValue("xml:id"), MascIdentifiers.PENN_TOKEN_ID_PREFIX));
       }
 
       if (qName.equals("link")) {
         if (tokenStack.isEmpty()) {
           throw new SAXException("The linking of tokens to quarks is broken.");
         }
 
-        String[] targets = attributes.getValue("targets")
-            .replace(MascIdentifiers.REGION_ID_PREFIX, "").split(" ");
-
-        int[] regions = new int[targets.length];
-        for (int i = 0; i < targets.length; i++) {
-          int region = Integer.parseInt(targets[i]);
-          regions[i] = region;
-        }
+        int[] regions = MascIdentifiers.parseIds(
+            attributes.getValue("targets"), MascIdentifiers.REGION_ID_PREFIX);
         tokenToQuarks.put(tokenStack.pop(), regions);
       }
 
       if (qName.equals("a")) {
-        tokenStackTag.push(Integer.parseInt(MascIdentifiers.removeFirst(
-            attributes.getValue("ref"), MascIdentifiers.PENN_TOKEN_ID_PREFIX)));
+        tokenStackTag.push(MascIdentifiers.parseId(
+            attributes.getValue("ref"), MascIdentifiers.PENN_TOKEN_ID_PREFIX));
       }
 
       if (qName.equals("f")) {

opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/masc/MascWordParser.java

@@ -42,8 +42,8 @@ public void startElement(String uri, String localName, String qName, Attributes
     try {
       // create a word and put it into the list of words
       if (qName.equalsIgnoreCase("region")) {
-        int id = Integer.parseInt(MascIdentifiers.removeFirst(
-            attributes.getValue("xml:id"), MascIdentifiers.REGION_ID_PREFIX));
+        int id = MascIdentifiers.parseId(
+            attributes.getValue("xml:id"), MascIdentifiers.REGION_ID_PREFIX);
         String[] anchors = attributes.getValue("anchors").split(" ");
 
         int left = Integer.parseInt(anchors[0]);

opennlp-core/opennlp-formats/src/test/java/opennlp/tools/formats/leipzig/LeipzigLanguageSampleStreamTest.java

@@ -100,21 +100,19 @@ void testReadSentenceFilesWithEmptyDir() {
   @ValueSource(strings = {"a", "eng", "dan", "abcdefghijklmnopqrstuvwxyz"})
   void testIsAsciiLowerCaseWordAccepts(String text) {
     Assertions.assertTrue(LeipzigLanguageSampleStream.isAsciiLowerCaseWord(text));
-    Assertions.assertTrue(text.matches("[a-z]+"));
   }
 
   @ParameterizedTest
-  @ValueSource(strings = {"", "Eng", "eNg", "en1", "123", "e-g", "en ", " en", "\u00e9ng",
-      "\u0130ng", "\uD835\uDC1Abc", "en\u00A0"})
+  @ValueSource(strings = {"", "Eng", "eNg", "en1", "123", "e-g", "en_", "en ", " en", "\u00e9ng",
+      "\u0130ng", "\uD835\uDC1Abc", "en\u00A0", "\uFF45ng"})
   void testIsAsciiLowerCaseWordRejects(String text) {
     Assertions.assertFalse(LeipzigLanguageSampleStream.isAsciiLowerCaseWord(text));
-    Assertions.assertFalse(text.matches("[a-z]+"));
   }
 
   @Test
   void testOnlyFilesWithLowerCaseAsciiLanguageCodesAreRead() throws IOException {
     String[] names = {"eng-sentences.txt", "Eng-sentences.txt", "en1-sentences.txt",
-        "e-g-sentences.txt", "\u00e9ng-sentences.txt", "en"};
+        "e-g-sentences.txt", "en_sentences.txt", "\u00e9ng-sentences.txt", "en"};
     for (String name : names) {
       Files.writeString(new File(emptyTempDir, name).toPath(),
           "1\tThis is a sentence.\n2\tThis is another sentence.\n", StandardCharsets.UTF_8);

opennlp-core/opennlp-formats/src/test/java/opennlp/tools/formats/masc/MascIdentifiersTest.java

@@ -17,37 +17,62 @@
 
 package opennlp.tools.formats.masc;
 
-import java.util.stream.Stream;
-
 import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.Arguments;
-import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.CsvSource;
+import org.junit.jupiter.params.provider.ValueSource;
 
 public class MascIdentifiersTest {
 
-  private static Stream<Arguments> removals() {
-    return Stream.of(
-        Arguments.of("ne-n7", MascIdentifiers.NAMED_ENTITY_ID_PREFIX, "7"),
-        Arguments.of("penn-n12", MascIdentifiers.PENN_TOKEN_ID_PREFIX, "12"),
-        Arguments.of("seg-r0", MascIdentifiers.REGION_ID_PREFIX, "0"),
-        Arguments.of("xne-n7", MascIdentifiers.NAMED_ENTITY_ID_PREFIX, "x7"),
-        Arguments.of("ne-nne-n7", MascIdentifiers.NAMED_ENTITY_ID_PREFIX, "ne-n7"),
-        Arguments.of("penn-n7penn-n", MascIdentifiers.PENN_TOKEN_ID_PREFIX, "7penn-n"),
-        Arguments.of("seg-r", MascIdentifiers.REGION_ID_PREFIX, ""),
-        Arguments.of("7", MascIdentifiers.NAMED_ENTITY_ID_PREFIX, "7"),
-        Arguments.of("", MascIdentifiers.NAMED_ENTITY_ID_PREFIX, ""),
-        Arguments.of("NE-N7", MascIdentifiers.NAMED_ENTITY_ID_PREFIX, "NE-N7"),
-        Arguments.of("ne\u2011n7", MascIdentifiers.NAMED_ENTITY_ID_PREFIX, "ne\u2011n7"),
-        Arguments.of("\uD83D\uDE00ne-n1\uD83D\uDE00", MascIdentifiers.NAMED_ENTITY_ID_PREFIX,
-            "\uD83D\uDE001\uD83D\uDE00"),
-        Arguments.of("abc", "", "abc"));
+  @ParameterizedTest
+  @CsvSource({"ne-n7, ne-n, 7", "penn-n12, penn-n, 12", "seg-r0, seg-r, 0",
+      "penn-n007, penn-n, 7", "ne-n2147483647, ne-n, 2147483647"})
+  void testParseIdReadsTheNumberAfterThePrefix(String id, String prefix, int expected) {
+    Assertions.assertEquals(expected, MascIdentifiers.parseId(id, prefix));
+  }
+
+  @ParameterizedTest
+  // other or missing prefix, prefix later in the text, doubled prefix, no digits, sign,
+  // digits of another script, trailing text, whitespace, and an overflowing number
+  @ValueSource(strings = {"7", "xne-n7", "NE-N7", "ne\u2011n7", "ne-nne-n7", "ne-n", "ne-n-7",
+      "ne-n+7", "ne-n\u0661", "ne-n\uFF17", "ne-n7x", "ne-n7 ", " ne-n7", "ne-n7\n",
+      "ne-n99999999999", ""})
+  void testParseIdRejectsAnythingElse(String id) {
+    Assertions.assertThrows(IllegalArgumentException.class,
+        () -> MascIdentifiers.parseId(id, MascIdentifiers.NAMED_ENTITY_ID_PREFIX));
+  }
+
+  @Test
+  void testParseIdRejectsNull() {
+    Assertions.assertThrows(IllegalArgumentException.class,
+        () -> MascIdentifiers.parseId(null, MascIdentifiers.NAMED_ENTITY_ID_PREFIX));
   }
 
   @ParameterizedTest
-  @MethodSource("removals")
-  void testRemoveFirstRemovesOnlyTheFirstOccurrence(String input, String literal, String expected) {
-    Assertions.assertEquals(expected, MascIdentifiers.removeFirst(input, literal));
-    Assertions.assertEquals(input.replaceFirst(literal, ""), MascIdentifiers.removeFirst(input, literal));
+  @ValueSource(strings = {"seg-r1 seg-r2", "seg-r1\tseg-r2", "  seg-r1   seg-r2  ",
+      "seg-r1\u00A0seg-r2", "seg-r1\u3000seg-r2", "seg-r1\r\nseg-r2"})
+  void testParseIdsSplitsOnWhitespaceRuns(String ids) {
+    Assertions.assertArrayEquals(new int[] {1, 2},
+        MascIdentifiers.parseIds(ids, MascIdentifiers.REGION_ID_PREFIX));
+  }
+
+  @Test
+  void testParseIdsReadsASingleIdentifier() {
+    Assertions.assertArrayEquals(new int[] {5},
+        MascIdentifiers.parseIds("seg-r5", MascIdentifiers.REGION_ID_PREFIX));
+  }
+
+  @ParameterizedTest
+  @ValueSource(strings = {"", " ", "\t", "seg-r1 penn-n2", "seg-r1 seg-r", "seg-r1,seg-r2"})
+  void testParseIdsRejectsEmptyOrMalformedLists(String ids) {
+    Assertions.assertThrows(IllegalArgumentException.class,
+        () -> MascIdentifiers.parseIds(ids, MascIdentifiers.REGION_ID_PREFIX));
+  }
+
+  @Test
+  void testParseIdsRejectsNull() {
+    Assertions.assertThrows(IllegalArgumentException.class,
+        () -> MascIdentifiers.parseIds(null, MascIdentifiers.REGION_ID_PREFIX));
   }
 }

opennlp-core/opennlp-formats/src/test/java/opennlp/tools/formats/masc/MascNamedEntityParserTest.java

@@ -23,6 +23,8 @@
 
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.xml.sax.SAXException;
 
 import opennlp.tools.util.XmlUtil;
@@ -47,12 +49,25 @@ void testEntityAndTokenIdsLoseTheirPrefix() throws Exception {
     Assertions.assertEquals(List.of(4, 15), parser.getEntityIDsToTokens().get(3));
   }
 
-  @Test
-  void testOnlyTheFirstPrefixOccurrenceIsRemoved() {
+  @ParameterizedTest
+  // doubled prefix, missing prefix, other prefix, no digits, trailing text
+  @ValueSource(strings = {"ne-nne-n3", "3", "penn-n3", "ne-n", "ne-n3x"})
+  void testMalformedEntityIdsAreRejected(String ref) {
     Assertions.assertThrows(SAXException.class, () -> parse(
-        "<graph><a ref=\"ne-nne-n3\" label=\"person\"/></graph>"));
+        "<graph><a ref=\"" + ref + "\" label=\"person\"/></graph>"));
+  }
+
+  @ParameterizedTest
+  @ValueSource(strings = {"penn-npenn-n4", "4", "seg-r4", "penn-n", "penn-n4x"})
+  void testMalformedTokenIdsAreRejected(String to) {
     Assertions.assertThrows(SAXException.class, () -> parse(
         "<graph><a ref=\"ne-n3\" label=\"person\"/>"
-        + "<edge from=\"ne-n3\" to=\"penn-npenn-n4\"/></graph>"));
+        + "<edge from=\"ne-n3\" to=\"" + to + "\"/></graph>"));
+  }
+
+  @Test
+  void testMissingIdAttributeIsRejected() {
+    Assertions.assertThrows(SAXException.class, () -> parse(
+        "<graph><a label=\"person\"/></graph>"));
   }
 }

opennlp-core/opennlp-formats/src/test/java/opennlp/tools/formats/masc/MascPennTagParserTest.java

@@ -22,6 +22,8 @@
 
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.xml.sax.SAXException;
 
 import opennlp.tools.util.XmlUtil;
@@ -48,11 +50,31 @@ void testTokenIdsLoseTheirPrefix() throws Exception {
     Assertions.assertEquals("test", parser.getBases().get(10));
   }
 
-  @Test
-  void testOnlyTheFirstPrefixOccurrenceIsRemoved() {
+  @ParameterizedTest
+  @ValueSource(strings = {"seg-r0\tseg-r1", "  seg-r0   seg-r1  ", "seg-r0\u00A0seg-r1",
+      "seg-r0\nseg-r1"})
+  void testLinkTargetsAreSeparatedByWhitespaceRuns(String targets) throws Exception {
+    MascPennTagParser parser = parse("<graph>"
+        + "<node xml:id=\"penn-n10\"><link targets=\"" + targets + "\"/></node>"
+        + "</graph>");
+    Assertions.assertArrayEquals(new int[] {0, 1}, parser.getTokenToQuarks().get(10));
+  }
+
+  @ParameterizedTest
+  // doubled prefix, missing prefix, other prefix, no digits, trailing text
+  @ValueSource(strings = {"penn-npenn-n2", "2", "ne-n2", "penn-n", "penn-n2x"})
+  void testMalformedTokenIdsAreRejected(String id) {
+    Assertions.assertThrows(SAXException.class, () -> parse(
+        "<graph><node xml:id=\"" + id + "\"><link targets=\"seg-r0\"/></node></graph>"));
     Assertions.assertThrows(SAXException.class, () -> parse(
-        "<graph><node xml:id=\"penn-npenn-n2\"><link targets=\"seg-r0\"/></node></graph>"));
+        "<graph><a ref=\"" + id + "\"><fs><f name=\"msd\" value=\"NN\"/></fs></a></graph>"));
+  }
+
+  @ParameterizedTest
+  // empty list, one malformed entry, other prefix, comma separated
+  @ValueSource(strings = {"", " ", "seg-r0 seg-r", "seg-r0 penn-n1", "seg-r0,seg-r1"})
+  void testMalformedLinkTargetsAreRejected(String targets) {
     Assertions.assertThrows(SAXException.class, () -> parse(
-        "<graph><a ref=\"penn-npenn-n2\"><fs><f name=\"msd\" value=\"NN\"/></fs></a></graph>"));
+        "<graph><node xml:id=\"penn-n2\"><link targets=\"" + targets + "\"/></node></graph>"));
   }
 }

opennlp-core/opennlp-formats/src/test/java/opennlp/tools/formats/masc/MascWordParserTest.java

@@ -23,6 +23,8 @@
 
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.xml.sax.SAXException;
 
 import opennlp.tools.util.XmlUtil;
@@ -51,9 +53,11 @@ void testRegionIdsLoseTheirPrefix() throws Exception {
     Assertions.assertEquals(7, words.get(1).getEnd());
   }
 
-  @Test
-  void testOnlyTheFirstPrefixOccurrenceIsRemoved() {
+  @ParameterizedTest
+  // doubled prefix, missing prefix, other prefix, no digits, trailing text
+  @ValueSource(strings = {"seg-rseg-r3", "3", "penn-n3", "seg-r", "seg-r3x"})
+  void testMalformedRegionIdsAreRejected(String id) {
     Assertions.assertThrows(SAXException.class, () -> parse(
-        "<graph><region xml:id=\"seg-rseg-r3\" anchors=\"0 4\"/></graph>"));
+        "<graph><region xml:id=\"" + id + "\" anchors=\"0 4\"/></graph>"));
   }
 }