OPENNLP-1880: Address review: narrow the contract, validate at the boundary, pin WNDB rejections

- Narrow the LexicalKnowledgeBase javadoc so the interface stops prescribing what
  only an implementation can promise: lemma matching semantics and thread safety
  are now stated as implementation specific and documented where they hold, on
  InMemoryWordNetLexicon, which already carries @threadsafe and describes the
  folding it applies.
- Reword the contains() javadoc to say plainly that the default implementation
  delegates to lookup(), instead of speculating about cheaper overrides.
- Move the null-element checks in MorphyLemmatizer up to the public lemmatize()
  overloads, both the array form and the list form, so validation happens once at
  the boundary the caller sees; the private lemmasOf() no longer repeats them and
  now documents that its arguments are validated by the caller.
- Reject a null argument in LemmaFolding.splitOnSpaces() rather than letting it
  fail later as a NullPointerException, and capitalize the fold() message so it
  matches the wording the other validators use.
- Document the throws clauses that the explicit validation adds, on
  LemmaKey.of() and on splitOnSpaces().
- Extract the repeated WN-LMF attribute names into ID_ATTRIBUTE,
  PART_OF_SPEECH_ATTRIBUTE, REL_TYPE_ATTRIBUTE, and TARGET_ATTRIBUTE, and the
  shared error opening into MALFORMED_PREFIX, so the element handlers stop
  repeating string literals.
- Extract the WNDB offset failure detail into MALFORMED_OFFSET, shared by the
  length check and the digit check.
- Fold the duplicated WNDB message construction into malformedMessage(), so the
  tokenizer builds the text directly instead of constructing an
  InvalidFormatException only to read getMessage() back off it.
- Drop the redundant fileName parameter from WndbReader.readAll(), which already
  names the full path it failed to open.
- Reduce the visibility of the Parser helpers in WnLmfReader: malformed() and
  line() are now private instance methods like every other helper in that class.
- Make MorphyLemmatizer.rulesFor() an instance method for the same reason, so the
  lemmatizer's private helpers are consistent.
- Add the missing javadoc on the RELATION_NAMES and POINTER_SYMBOLS lookup tables
  and on both RawSynset holders, the last undocumented members in the readers.
- Correct two stale comments: the build() comment now points at memberLemmas(),
  where the synset and member part-of-speech agreement is really checked, and the
  mutate() comment in the tests states the actual constraint, that an edit which
  changes a line's length is only safe when the reader is expected to fail on
  that line before it reads the ones after it.
- Add a parameterized WNDB test pinning eight field-level rejections that had no
  coverage: the offset length and digit checks, the synset and index part of
  speech mismatches, the base-16 word count field, the minimum word count, the
  pointer pos, the gloss separator, and the syntactic marker.
- Add pinning tests for the newly explicit validation: splitOnSpaces() on null,
  and the list lemmatize() overload with a null token and with a null tag.
- Share the fixtures instead of duplicating them: WndbReaderTest now exposes
  DOG_ID, CANID_ID, and its fixtureDirectory(), WnLmfReaderTest exposes
  fixture(), and LexiconConcurrencyTest and WordNetUsageExampleTest use those
  instead of their own loader copies and hardcoded ids.
- Document the package-private test fixture helpers and switch
  WordNetUsageExampleTest to static assertion imports, matching the other tests
  in the module.

Author: krickert

opennlp-api/src/main/java/opennlp/tools/wordnet/LexicalKnowledgeBase.java

@@ -24,14 +24,11 @@
  * identity is opaque and source-qualified (see {@link Synset#id()}). Lookups return their matches
  * in the source's sense order and never return {@code null}.
  *
- * <p>Lemma matching semantics are the implementation's concern. The reference implementations
- * match case-insensitively (case folding with the root locale) and treat the underscore some
- * formats store in multiword lemmas as a space; an implementation with different semantics must
- * document them. Returned {@link Synset#lemmas() lemmas} preserve the source's written forms,
- * with spaces in multiword lemmas.</p>
+ * <p>How a queried lemma is matched against the source's written forms is implementation
+ * specific and documented there. Returned {@link Synset#lemmas() lemmas} preserve the source's
+ * written forms, with spaces in multiword lemmas.</p>
  *
- * <p>Implementations must be immutable and thread-safe after loading: one instance is meant to
- * be shared across an application's threads for concurrent lookups.</p>
+ * <p>Thread safety is implementation specific.</p>
  */
 public interface LexicalKnowledgeBase {
 
@@ -74,9 +71,8 @@ default List<String> related(String synsetId, WordNetRelation relation) {
   }
 
   /**
-   * Tests whether the lexicon contains a lemma with a part of speech. This is the membership
-   * check morphological rules validate their candidates against; implementations may override
-   * it with a cheaper check than {@link #lookup(String, WordNetPOS)}.
+   * Tests whether the lexicon contains a lemma with a part of speech. The default implementation
+   * delegates to {@link #lookup(String, WordNetPOS)}.
    *
    * @param lemma The lemma to test. Must not be {@code null}.
    * @param pos   The part of speech to test it as. Must not be {@code null}.

opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/InMemoryWordNetLexicon.java

@@ -148,6 +148,7 @@ record LemmaKey(String lemma, WordNetPOS pos) {
      * @param writtenForm The lemma as written in the source or query. Must not be {@code null}.
      * @param pos         The part of speech. Must not be {@code null}.
      * @return The folded key.
+     * @throws IllegalArgumentException Thrown if {@code writtenForm} is {@code null}.
      */
     static LemmaKey of(String writtenForm, WordNetPOS pos) {
       return new LemmaKey(LemmaFolding.fold(writtenForm), pos);

opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/LemmaFolding.java

@@ -41,7 +41,7 @@ private LemmaFolding() {
    */
   static String fold(String writtenForm) {
     if (writtenForm == null) {
-      throw new IllegalArgumentException("writtenForm must not be null");
+      throw new IllegalArgumentException("WrittenForm must not be null");
     }
     return writtenForm.replace('_', ' ').toLowerCase(Locale.ROOT);
   }
@@ -51,8 +51,12 @@ static String fold(String writtenForm) {
    *
    * @param value The field list. Must not be {@code null}.
    * @return The non-empty fields in order, never {@code null}.
+   * @throws IllegalArgumentException Thrown if {@code value} is {@code null}.
    */
   static List<String> splitOnSpaces(String value) {
+    if (value == null) {
+      throw new IllegalArgumentException("Value must not be null");
+    }
     final List<String> parts = new ArrayList<>(4);
     int start = 0;
     while (start < value.length()) {

opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/MorphyLemmatizer.java

@@ -112,6 +112,12 @@ public String[] lemmatize(String[] toks, String[] tags) {
     }
     final String[] lemmas = new String[toks.length];
     for (int i = 0; i < toks.length; i++) {
+      if (toks[i] == null) {
+        throw new IllegalArgumentException("Toks must not contain a null element");
+      }
+      if (tags[i] == null) {
+        throw new IllegalArgumentException("Tags must not contain a null element");
+      }
       final List<String> candidates = lemmasOf(toks[i], tags[i]);
       lemmas[i] = candidates.isEmpty() ? UNKNOWN_LEMMA : candidates.get(0);
     }
@@ -138,6 +144,12 @@ public List<List<String>> lemmatize(List<String> toks, List<String> tags) {
     }
     final List<List<String>> lemmas = new ArrayList<>(toks.size());
     for (int i = 0; i < toks.size(); i++) {
+      if (toks.get(i) == null) {
+        throw new IllegalArgumentException("Toks must not contain a null element");
+      }
+      if (tags.get(i) == null) {
+        throw new IllegalArgumentException("Tags must not contain a null element");
+      }
       final List<String> candidates = lemmasOf(toks.get(i), tags.get(i));
       lemmas.add(candidates.isEmpty() ? List.of(UNKNOWN_LEMMA) : candidates);
     }
@@ -147,15 +159,12 @@ public List<List<String>> lemmatize(List<String> toks, List<String> tags) {
   /**
    * Finds all lemmas of one token, most preferred first.
    *
-   * @param token The token to lemmatize.
-   * @param tag   The part-of-speech tag.
+   * @param token The token to lemmatize. Validated at the public boundary.
+   * @param tag   The part-of-speech tag. Validated at the public boundary.
    * @return The candidate lemmas, empty when the word is unknown or the tag maps to no part of
    *     speech.
    */
   private List<String> lemmasOf(String token, String tag) {
-    if (token == null || tag == null) {
-      throw new IllegalArgumentException("Tokens and tags must not contain null elements");
-    }
     final WordNetPOS pos = posFromTag(tag);
     if (pos == null) {
       return List.of();
@@ -188,7 +197,7 @@ private List<String> lemmasOf(String token, String tag) {
    * @param pos The part of speech.
    * @return The suffix-substitution rules, empty for adverbs.
    */
-  private static String[][] rulesFor(WordNetPOS pos) {
+  private String[][] rulesFor(WordNetPOS pos) {
     return switch (pos) {
       case NOUN -> NOUN_RULES;
       case VERB -> VERB_RULES;

opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/WnLmfReader.java

@@ -66,6 +66,7 @@
  */
 public final class WnLmfReader {
 
+  /** The WN-LMF relation names this reader accepts, mapped to the contract relations. */
   private static final Map<String, WordNetRelation> RELATION_NAMES = relationNames();
 
   /** The format's escape-hatch relation type; carries no type the contract can express. */
@@ -80,6 +81,21 @@ public final class WnLmfReader {
   /** The element declaring a synset; opened and closed by the same handlers. */
   private static final String SYNSET_ELEMENT = "Synset";
 
+  /** The identifier attribute shared by entries, senses, and synsets. */
+  private static final String ID_ATTRIBUTE = "id";
+
+  /** The part-of-speech attribute shared by lemmas and synsets. */
+  private static final String PART_OF_SPEECH_ATTRIBUTE = "partOfSpeech";
+
+  /** The relation-type attribute shared by sense and synset relations. */
+  private static final String REL_TYPE_ATTRIBUTE = "relType";
+
+  /** The relation-target attribute shared by sense and synset relations. */
+  private static final String TARGET_ATTRIBUTE = "target";
+
+  /** The opening of every malformed-document message, before the resource name. */
+  private static final String MALFORMED_PREFIX = "Malformed WN-LMF document ";
+
   /** Not instantiable. */
   private WnLmfReader() {
   }
@@ -227,7 +243,7 @@ private void startElement(XMLStreamReader reader)
       final String name = reader.getLocalName();
       switch (name) {
         case LEXICAL_ENTRY_ELEMENT -> {
-          currentEntryId = requireAttribute(reader, "id");
+          currentEntryId = requireAttribute(reader, ID_ATTRIBUTE);
           if (!entryIds.add(currentEntryId)) {
             throw malformed(reader.getLocation(),
                 "Duplicate lexical entry id " + currentEntryId, null);
@@ -240,7 +256,7 @@ private void startElement(XMLStreamReader reader)
             throw malformed(reader.getLocation(), "Lemma outside a LexicalEntry", null);
           }
           currentEntryLemma = requireAttribute(reader, "writtenForm");
-          currentEntryPos = parsePos(requireAttribute(reader, "partOfSpeech"),
+          currentEntryPos = parsePos(requireAttribute(reader, PART_OF_SPEECH_ATTRIBUTE),
               reader.getLocation());
           lemmaByEntryId.put(currentEntryId, currentEntryLemma);
           posByEntryId.put(currentEntryId, currentEntryPos);
@@ -250,7 +266,7 @@ private void startElement(XMLStreamReader reader)
             throw malformed(reader.getLocation(),
                 "Sense before its entry's Lemma in LexicalEntry " + currentEntryId, null);
           }
-          currentSenseId = requireAttribute(reader, "id");
+          currentSenseId = requireAttribute(reader, ID_ATTRIBUTE);
           final String synsetId = requireAttribute(reader, "synset");
           if (synsetBySenseId.putIfAbsent(currentSenseId, synsetId) != null) {
             throw malformed(reader.getLocation(), "Duplicate sense id " + currentSenseId, null);
@@ -269,12 +285,12 @@ private void startElement(XMLStreamReader reader)
             throw malformed(reader.getLocation(), "SenseRelation outside a Sense", null);
           }
           senseRelations.add(new RawSenseRelation(currentSenseId,
-              requireAttribute(reader, "relType"), requireAttribute(reader, "target"),
-              line(reader.getLocation())));
+              requireAttribute(reader, REL_TYPE_ATTRIBUTE),
+              requireAttribute(reader, TARGET_ATTRIBUTE), line(reader.getLocation())));
         }
         case SYNSET_ELEMENT -> {
-          final String id = requireAttribute(reader, "id");
-          final WordNetPOS pos = parsePos(requireAttribute(reader, "partOfSpeech"),
+          final String id = requireAttribute(reader, ID_ATTRIBUTE);
+          final WordNetPOS pos = parsePos(requireAttribute(reader, PART_OF_SPEECH_ATTRIBUTE),
               reader.getLocation());
           currentSynset = new RawSynset(id, pos, reader.getAttributeValue(null, "members"),
               line(reader.getLocation()));
@@ -291,8 +307,8 @@ private void startElement(XMLStreamReader reader)
           if (currentSynset == null) {
             throw malformed(reader.getLocation(), "SynsetRelation outside a Synset", null);
           }
-          final String relType = requireAttribute(reader, "relType");
-          final String target = requireAttribute(reader, "target");
+          final String relType = requireAttribute(reader, REL_TYPE_ATTRIBUTE);
+          final String target = requireAttribute(reader, TARGET_ATTRIBUTE);
           // The escape-hatch type is a documented skip, not a rejection.
           if (!OTHER_RELATION.equals(relType)) {
             currentSynset.relations.add(
@@ -335,7 +351,8 @@ private void endElement(String name) {
      *     target, or a synset has no members.
      */
     LexicalKnowledgeBase build() throws InvalidFormatException {
-      // Every sense must point to a declared synset, with a consistent part of speech.
+      // Every sense must point to a declared synset; part-of-speech consistency between a
+      // synset and its member entries is checked in memberLemmas.
       for (final Map.Entry<String, String> sense : synsetBySenseId.entrySet()) {
         final RawSynset target = rawSynsets.get(sense.getValue());
         if (target == null) {
@@ -507,10 +524,10 @@ private String requireAttribute(XMLStreamReader reader, String attribute)
      * @param cause    The underlying cause, or {@code null}.
      * @return The exception to throw.
      */
-    InvalidFormatException malformed(Location location, String message, Throwable cause) {
+    private InvalidFormatException malformed(Location location, String message, Throwable cause) {
       final int line = line(location);
-      final String prefix = line < 0 ? "Malformed WN-LMF document " + resourceName + ": "
-          : "Malformed WN-LMF document " + resourceName + " at line " + line + ": ";
+      final String prefix = line < 0 ? MALFORMED_PREFIX + resourceName + ": "
+          : MALFORMED_PREFIX + resourceName + " at line " + line + ": ";
       return cause == null ? new InvalidFormatException(prefix + message)
           : new InvalidFormatException(prefix + message, cause);
     }
@@ -521,11 +538,12 @@ InvalidFormatException malformed(Location location, String message, Throwable ca
      * @param location The location, or {@code null}.
      * @return The line number, or {@code -1} when unknown.
      */
-    private static int line(Location location) {
+    private int line(Location location) {
       return location == null ? -1 : location.getLineNumber();
     }
   }
 
+  /** A parsed synset, kept until its members and relation targets can be resolved. */
   private static final class RawSynset {
     private final String id;
     private final WordNetPOS pos;

opennlp-extensions/opennlp-wordnet/src/main/java/opennlp/wordnet/WndbReader.java

@@ -57,11 +57,15 @@
  */
 public final class WndbReader {
 
+  /** The WNDB pointer symbols this reader accepts, mapped to the contract relations. */
   private static final Map<String, WordNetRelation> POINTER_SYMBOLS = pointerSymbols();
 
   /** The prefix of every synset id this reader mints. */
   private static final String SYNSET_ID_PREFIX = "wndb-";
 
+  /** The failure detail for a synset offset field that is not exactly 8 digits. */
+  private static final String MALFORMED_OFFSET = "Synset offset must be 8 digits, got: ";
+
   /** Not instantiable. */
   private WndbReader() {
   }
@@ -146,7 +150,7 @@ private enum FilePos {
   private static void parseDataFile(Path directory, FilePos filePos,
                                     Map<String, RawSynset> rawSynsets) throws IOException {
     final String fileName = "data." + filePos.suffix;
-    final byte[] bytes = readAll(directory.resolve(fileName), fileName);
+    final byte[] bytes = readAll(directory.resolve(fileName));
     int lineStart = 0;
     int lineNumber = 0;
     while (lineStart < bytes.length) {
@@ -281,7 +285,7 @@ private static void parseIndexFile(Path directory, FilePos filePos,
                                      Map<InMemoryWordNetLexicon.LemmaKey, List<String>> senses)
       throws IOException {
     final String fileName = "index." + filePos.suffix;
-    final byte[] bytes = readAll(directory.resolve(fileName), fileName);
+    final byte[] bytes = readAll(directory.resolve(fileName));
     final String content = new String(bytes, StandardCharsets.ISO_8859_1);
     int lineNumber = 0;
     int lineStart = 0;
@@ -399,13 +403,13 @@ private static String cleanLemma(String word, String fileName, int lineNumber)
    */
   private static int parseOffset(String offset, Tokenizer tokens) throws InvalidFormatException {
     if (offset.length() != 8) {
-      throw tokens.malformedToken("Synset offset must be 8 digits, got: " + offset);
+      throw tokens.malformedToken(MALFORMED_OFFSET + offset);
     }
     int value = 0;
     for (int i = 0; i < 8; i++) {
       final char c = offset.charAt(i);
       if (c < '0' || c > '9') {
-        throw tokens.malformedToken("Synset offset must be 8 digits, got: " + offset);
+        throw tokens.malformedToken(MALFORMED_OFFSET + offset);
       }
       value = value * 10 + (c - '0');
     }
@@ -433,13 +437,12 @@ private static char posChar(String pos, Tokenizer tokens) throws InvalidFormatEx
   /**
    * Reads a required database file in full.
    *
-   * @param file     The file path.
-   * @param fileName The file name, for error reporting.
+   * @param file The file path.
    * @return The file bytes.
    * @throws InvalidFormatException Thrown if the file is missing.
    * @throws IOException Thrown if reading fails.
    */
-  private static byte[] readAll(Path file, String fileName) throws IOException {
+  private static byte[] readAll(Path file) throws IOException {
     if (!Files.isRegularFile(file)) {
       throw new InvalidFormatException("Missing WNDB database file: " + file);
     }
@@ -456,8 +459,19 @@ private static byte[] readAll(Path file, String fileName) throws IOException {
    */
   private static InvalidFormatException malformed(String fileName, int lineNumber,
                                                   String message) {
-    return new InvalidFormatException(
-        "Malformed WNDB file " + fileName + " at line " + lineNumber + ": " + message);
+    return new InvalidFormatException(malformedMessage(fileName, lineNumber, message));
+  }
+
+  /**
+   * Builds the malformed-file message naming the file and line.
+   *
+   * @param fileName   The file name.
+   * @param lineNumber The 1-based line number.
+   * @param message    The failure detail.
+   * @return The message text.
+   */
+  private static String malformedMessage(String fileName, int lineNumber, String message) {
+    return "Malformed WNDB file " + fileName + " at line " + lineNumber + ": " + message;
   }
 
   /** A cursor over one line's space-separated fields. */
@@ -515,8 +529,8 @@ int nextInt(String field, int radix) throws InvalidFormatException {
       try {
         return Integer.parseInt(token, radix);
       } catch (NumberFormatException e) {
-        throw new InvalidFormatException(malformed(fileName, lineNumber,
-            "Field " + field + " is not a base-" + radix + " integer: " + token).getMessage(), e);
+        throw new InvalidFormatException(malformedMessage(fileName, lineNumber,
+            "Field " + field + " is not a base-" + radix + " integer: " + token), e);
       }
     }
 
@@ -557,6 +571,7 @@ InvalidFormatException malformedToken(String message) {
   private record RawPointer(WordNetRelation relation, String targetId, int lineNumber) {
   }
 
+  /** A parsed data-file synset, kept until its pointer targets can be resolved. */
   private static final class RawSynset {
     private final String id;
     private final WordNetPOS pos;

opennlp-extensions/opennlp-wordnet/src/test/java/opennlp/wordnet/LemmaFoldingTest.java

@@ -63,4 +63,9 @@ void testLemmaKeyAndExceptionLookupAgreeOnTheFold() {
   void testFoldRejectsNull() {
     assertThrows(IllegalArgumentException.class, () -> LemmaFolding.fold(null));
   }
+
+  @Test
+  void testSplitOnSpacesRejectsNull() {
+    assertThrows(IllegalArgumentException.class, () -> LemmaFolding.splitOnSpaces(null));
+  }
 }

opennlp-extensions/opennlp-wordnet/src/test/java/opennlp/wordnet/LexiconConcurrencyTest.java

@@ -71,14 +71,14 @@ void testConcurrentLookupsSeeConsistentResults() throws InterruptedException {
   }
 
   private static void verifyOnce(LexicalKnowledgeBase lexicon, Queue<String> problems) {
-    if (!"wndb-00001075-n".equals(lexicon.lookup("dog", WordNetPOS.NOUN).get(0).id())) {
+    if (!WndbReaderTest.DOG_ID.equals(lexicon.lookup("dog", WordNetPOS.NOUN).get(0).id())) {
       problems.add("Wrong dog lookup");
     }
     if (lexicon.lookup("run", WordNetPOS.NOUN).size() != 2) {
       problems.add("Wrong run sense count");
     }
-    if (!List.of("wndb-00001160-n")
-        .equals(lexicon.related("wndb-00001075-n", WordNetRelation.HYPERNYM))) {
+    if (!List.of(WndbReaderTest.CANID_ID)
+        .equals(lexicon.related(WndbReaderTest.DOG_ID, WordNetRelation.HYPERNYM))) {
       problems.add("Wrong dog hypernym");
     }
     if (lexicon.contains("zebra", WordNetPOS.NOUN)) {

opennlp-extensions/opennlp-wordnet/src/test/java/opennlp/wordnet/MorphyExceptionsTest.java

@@ -35,6 +35,11 @@
 
 public class MorphyExceptionsTest {
 
+  /**
+   * Loads the exception lists from the miniature WNDB fixture directory.
+   *
+   * @return The loaded fixture exception lists.
+   */
   static MorphyExceptions fixture() {
     try {
       return MorphyExceptions.load(WndbReaderTest.fixtureDirectory());