OPENNLP-1929: Regex Removal (7/10): BasicContextGenerator takes the separator literally and splits on Unicode whitespace by default

[krickert]

BasicContextGenerator.getContext split on String.split(separator), so the separator was compiled as a regex on each call: "|" split on each character, "." returned an empty array, and "+" or "(" failed with PatternSyntaxException at prediction time.

Changes:

• The default constructor splits on runs of Unicode whitespace (StringUtil.splitOnUnicodeWhitespace), independent of opennlp.whitespace.mode. "a b" now gives [a, b], not [a, , b].
• BasicContextGenerator(String sep) takes the separator as written.
• Empty predicates are not returned. Null or empty separators, separators with an unpaired surrogate, and a null input throw IllegalArgumentException.
• The manual paragraph in machine-learning.xml states these rules and has the migration table.

Breaking for code that passed a regex: a separator containing a backslash, such as "\\|" or "\\s+", now throws IllegalArgumentException at construction, so the old escapes fail at startup instead of matching no input. "[,;]" is a plain separator. No code in the repository uses the separator constructor.

OPENNLP-1929

[rzo1]

Little time, so here is a GPT 5.6-sol review instead for now

Compatibility note. No additional splitting bug found; please document the intentional constructor behavior changes.

Validation across the combined stack: 1,856 targeted tests, zero failures, one skipped.

[rzo1]

This intentionally changes existing constructor behavior: regex separators such as \s+ become literal, and empty separators now throw. Please include these changes in the migration notes.

[rzo1]

Here are some additional comments. The literal-separator change breaks existing callers silently, so it needs a deprecation path or a documented maintainer decision.

Blocking

1. BasicContextGenerator.java:81. The default constructor splits with WhitespaceTokenizer.INSTANCE, a shared mutable singleton. TokenizerME.java:195-196 calls setKeepNewLines(...) on it on every call, so once any TokenizerME with keepNewLines=true has run, getContext("a\nb\r\nc") returns [a, \n, b, \r, \n, c]. Across threads this is a data race. Scan with a private loop instead, and add a test that sets INSTANCE.setKeepNewLines(true) first.

2. BasicContextGenerator.java:58-66. A regex separator now silently stops splitting. o.split(sep) was a regex split, so escaping was the only way to split on | or ., and exactly those callers break:

   • "\\|" on a|b: old [a, b], new [a|b]
   • "\\." on a.b: old [a, b], new [a.b]
   • "\\s+" on a b\tc: old [a, b, c], new [a b\tc]
   • "[ \t]" and "[,;]" behave the same way

   The whole line becomes one unknown predicate and the model silently returns its prior outcome. This is public maxent API. Keep the String constructor with regex semantics (compile once in the constructor), mark it @Deprecated(forRemoval = true), and add a literal BasicContextGenerator(char) or a named factory as the replacement. Otherwise, get an explicit decision on dev@ and add it to the 3.0.0 migration notes.

3. BasicContextGenerator.java:50-52, :81. The default constructor changes behavior, and the PR description doesn't mention it. It used to split on one U+0020 and now splits on whitespace runs, following the global opennlp.whitespace.mode, and drops empty parts. On 1M fuzz inputs old vs new, 858,399 differ. U+0085 splits under UNICODE but not LEGACY; U+001C–U+001F behave the other way round. "".split(" ") gave [""]; now []. Document the mode dependency (link StringUtil#isWhitespace) and state the change.

4. PR description. It is out of date:

   • "keeps the String.split shape (a leading occurrence gives an empty first element…)" is wrong; the last commit drops every empty part (",a" gives [a]).
   • The default whitespace split and the IAE for null input are not mentioned.
   • "A dot matched no boundary at all" is wrong: . matched every character, so "a.b".split(".") returned [].

   Rewrite it. Retitle, e.g. "OPENNLP-1929: BasicContextGenerator splits on whitespace by default and takes a custom separator literally".

5. machine-learning.xml:82-94. "the same definition of whitespace as the event streams" is false. FileEventStream uses StringTokenizer (ASCII \t\n\r\f), and RealValueFileEventStream/RealBasicEventStream still use split("\\s+") here; only OPENNLP-1933: Regex removal (5/10): Replace per-call String.split/matches/replaceAll patterns #1279 changes them. Event lines also start with an outcome, so "for the line format of the event streams" is misleading. Cut the claim, condense to 2-3 sentences, and mention the whitespace mode. OPENNLP-1933: Regex removal (5/10): Replace per-call String.split/matches/replaceAll patterns #1279 edits the same section, so coordinate the wording.

Minor

• BasicContextGenerator.java:35-36. "Since 3.0.0 the separator is not a regular expression … (OPENNLP-1929)" is version history in Javadoc. Remove it or reduce it to one line. Same at machine-learning.xml:91-93 ("In releases before 3.0.0 …").
• BasicContextGenerator.java:47-48. "as defined by {@link WhitespaceTokenizer}" hides the mode dependency. Link StringUtil#isWhitespace and name the property.
• BasicContextGenerator.java:83-95. Allocates an ArrayList and an array on every call. Add a fast path when indexOf(separator) == -1.
• ContextGenerator.java:24-29. No @return. Add it.
• BasicContextGeneratorTest.java:102-111. A single-value @ValueSource({""}) next to a separate null @Test. Merge into one @NullAndEmptySource test.
• BasicContextGeneratorTest.java:113-119. One test asserts both constructors. Split it or parameterize it.
• BasicContextGeneratorTest.java:33-66. Only "\\s" pins the regex break. Add "\\|", "\\.", Pattern.quote("|") and "\\s+" rows, so the incompatibility is deliberate and visible.
• BasicContextGeneratorTest.java:75-94. No mode-dependent rows (U+0085, U+001C) and no "a\nb" row. Add them, pinning WhitespaceMode with a reset in @AfterEach.
• This PR uses none of OPENNLP-1928: Regex removal (1/8): Replace trivial patterns with character scans in StringUtil #1275's helpers, and WhitespaceMode is already on main. Rebase onto main and drop the OPENNLP-1928: Regex removal (1/8): Replace trivial patterns with character scans in StringUtil #1275 commits.

Verified: the literal path is correct (1M fuzz inputs vs split(Pattern.quote(sep)) with empty parts removed: 0 differences). There is no in-repo caller of either constructor, opennlp-eval-tests included, so no eval build is needed; the risk is third-party code.

[rzo1]

Follow-up: the shared WhitespaceTokenizer.INSTANCE problem behind blocking item 1 is filed as OPENNLP-1947.

[krickert]

I did a break instead of a deprecated regex constructor. We shouldn't keep a Pattern in this class, I don't think it's justified for exclusion in the 10th regex guard ticket (#1282), and a shim does not stop the silent one-context result.

I think it's cleaner to have a separator with a backslash to throw at construction, so old escapes fails loudly.

43da7b5.

[rzo1]

Thanks, the earlier points are addressed. I'm fine with the hard break instead of keeping a Pattern. Maintainer edits don't work on the ai-pipestream fork, so here is a patch with the small fixes:

• README.md: the "Migrating from 2.x to 3.x" section claimed there are no breaking changes. It now lists this one under "Breaking changes". OPENNLP-1932: Regex removal (4/10): Match model finder wildcards without regular expressions #1278 adds the same section, so whichever PR merges second only adds its bullet.
• BasicContextGenerator: the @param sep doc is reduced to its constraints, since the rationale is already covered by @throws.
• BasicContextGeneratorTest: dropped the 2.x history comment.

Apply it with git am 1280.patch, and please don't force-push over it without applying it first.

1280.patch

From 9f1049aabcb8819d1633c1b21959efacf7e61a70 Mon Sep 17 00:00:00 2001
From: Richard Zowalla <rzo1@apache.org>
Date: Fri, 25 Sep 2026 21:09:29 +0200
Subject: [PATCH] OPENNLP-1929: Document the breaking change and trim
 commentary

- README: list the literal separator and the backslash rejection under
  breaking changes in the 2.x to 3.x migration section.
- BasicContextGenerator: reduce the sep parameter doc to its constraints.
- BasicContextGeneratorTest: drop the 2.x history comment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 README.md                                                   | 6 +++++-
 .../java/opennlp/tools/ml/maxent/BasicContextGenerator.java | 6 ++----
 .../opennlp/tools/ml/maxent/BasicContextGeneratorTest.java  | 2 --
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/README.md b/README.md
index 5a40f38e4..e02bd2598 100644
--- a/README.md
+++ b/README.md
@@ -120,7 +120,7 @@ For more details please check our [documentation](https://opennlp.apache.org/doc
 
 ## Migrating from 2.x to 3.x
 
-The 3.x release line of Apache OpenNLP introduces **no** known breaking changes but modularizes the project for better usage as a library and to support future extensibility.
+The 3.x release line of Apache OpenNLP introduces only a few breaking changes, listed below, but modularizes the project for better usage as a library and to support future extensibility.
 The core API remains stable and compatible with 2.x, but the project structure has been reorganized into multiple modules.
 
 That means, that you can continue to use the previous `opennlp-tools` artifact as a dependency. However, we strongly recommend to switch to the new modular structure 
@@ -129,6 +129,10 @@ and import only the components you need, which will result in a smaller dependen
 Only `opennlp-runtime` needs to be added as a dependency, and you can add additional modules (e.g. `opennlp-ml-maxent`, `opennlp-model-resolver`, etc.) as required by your project.
 For users of the traditional CLI toolkit, nothing changes with the 3.x release line. CLI usage remains stable as described in the [project's dev manual](https://opennlp.apache.org/docs/).
 
+### Breaking changes
+
+- `BasicContextGenerator(String)`: the separator is taken literally instead of as a regular expression, and a separator containing a backslash throws `IllegalArgumentException`. Empty predicates are no longer returned, and the default constructor splits on runs of Unicode whitespace ([OPENNLP-1929](https://issues.apache.org/jira/browse/OPENNLP-1929)).
+
 ### Thread safety
 
 Starting with 3.0.0, the core `*ME` classes (`POSTaggerME`, `TokenizerME`, `SentenceDetectorME`, `ChunkerME`, `LemmatizerME`, `NameFinderME`) are thread safe and a single instance can be shared across threads. This eliminates the need to pool or recreate ME instances per thread. The legacy `ThreadSafe*ME` wrappers from 2.x still work but are now deprecated; existing code does not need to change to upgrade.
diff --git a/opennlp-core/opennlp-ml/opennlp-ml-maxent/src/main/java/opennlp/tools/ml/maxent/BasicContextGenerator.java b/opennlp-core/opennlp-ml/opennlp-ml-maxent/src/main/java/opennlp/tools/ml/maxent/BasicContextGenerator.java
index fdebad954..118c12090 100644
--- a/opennlp-core/opennlp-ml/opennlp-ml-maxent/src/main/java/opennlp/tools/ml/maxent/BasicContextGenerator.java
+++ b/opennlp-core/opennlp-ml/opennlp-ml-maxent/src/main/java/opennlp/tools/ml/maxent/BasicContextGenerator.java
@@ -59,10 +59,8 @@ public class BasicContextGenerator implements ContextGenerator<String> {
    * whitespace is part of the predicates.
    *
    * @param sep The separator, taken as written and not as a regular expression.
-   *            Must not be {@code null} or empty, and must be well-formed text: an unpaired
-   *            surrogate is not a character and could split a code point of the input.
-   *            A backslash is rejected, so a separator escaped for the regular expression
-   *            engine, such as {@code "\\|"}, fails here instead of splitting nothing.
+   *            Must not be {@code null} or empty, and must not contain a backslash or
+   *            an unpaired surrogate.
    * @throws IllegalArgumentException If {@code sep} is {@code null}, empty, contains a
    *                                  backslash, or contains an unpaired surrogate.
    */
diff --git a/opennlp-core/opennlp-ml/opennlp-ml-maxent/src/test/java/opennlp/tools/ml/maxent/BasicContextGeneratorTest.java b/opennlp-core/opennlp-ml/opennlp-ml-maxent/src/test/java/opennlp/tools/ml/maxent/BasicContextGeneratorTest.java
index c68b9b0d2..db69eda8d 100644
--- a/opennlp-core/opennlp-ml/opennlp-ml-maxent/src/test/java/opennlp/tools/ml/maxent/BasicContextGeneratorTest.java
+++ b/opennlp-core/opennlp-ml/opennlp-ml-maxent/src/test/java/opennlp/tools/ml/maxent/BasicContextGeneratorTest.java
@@ -121,8 +121,6 @@ public class BasicContextGeneratorTest {
   @ParameterizedTest
   @ValueSource(strings = {"\\|", "\\.", "\\s", "\\s+", "\\Q|\\E", "a\\b", "\\"})
   void testEscapedSeparatorIsRejected(String separator) {
-    // the escape that a regex separator needed fails at construction, so a separator written
-    // for 2.x cannot silently stop splitting
     IllegalArgumentException e = Assertions.assertThrows(IllegalArgumentException.class,
         () -> new BasicContextGenerator(separator));
     Assertions.assertEquals("sep is taken as written and must not contain a backslash: "
-- 
2.55.0

Still open on your side:

• Please retitle to describe the user-visible change, e.g. "BasicContextGenerator takes the separator literally and splits on Unicode whitespace by default".
• Rebase onto main (the branch is behind).
• Once OPENNLP-1932: Regex removal (4/10): Match model finder wildcards without regular expressions #1278 is merged, the split loop in getContext could move to StringUtil next to splitNonEmpty instead of living here.

As mentioned on #1278: pushing your branches to apache/opennlp instead of the ai-pipestream fork would let committers push small fixes like this directly.

[krickert]

Thanks. I squashed everything with your patch applied. The migration notes are in the manual paragraph in machine-learning.xml, including the table of old regex separators and what to pass now, and the description names the break. The title uses the wording you suggested.

For now on, I'll create PRs from an apache branch. I'll try to minimize the churn after an PR is created.

[rzo1]

Thanks for the update. A few things don't match what we discussed:

1. The separator check got wider than agreed. I was fine with rejecting a backslash, but the head now also rejects [ ] ( ) { } | * + ? ^ $ . That means "|" and "." can't be used as separators at all, which is odd given the ticket is about "|" not working. Both never worked as regex separators, so nobody can depend on the old behaviour. Please limit the check to the backslash, or explain why we need the wider one.

2. The description and the commit message don't match the code anymore. "[,;]" is described as a plain separator, but the test expects it to throw. There is also no migration table in machine-learning.xml, and only the backslash is mentioned as breaking.

3. The README part of my patch is missing. It still says there are no known breaking changes.

4. The title is unchanged. Suggestion: "OPENNLP-1929: Regex Removal (7/10): BasicContextGenerator takes the separator literally and splits on Unicode whitespace by default".

5. Please rebase, the branch is 14 commits behind main.

Minor:

• BasicContextGeneratorTest:106-107: the history comment is back in other words. Please drop it.
• machine-learning.xml:144: unrelated newline change at the end of the file. Please revert.
• The list of rejected characters is in the Javadoc and in the manual. One place is enough.

The rest of my earlier comments is addressed. No eval build needed, since nothing in the repo calls these constructors.

[krickert]

Yeh, the head I pushed had the wider check and neither the README hunk nor the table.

Now only a backslash is rejected, so |, . and [,;] are plain separators, with rows in the literal separator test.

Your README hunk is applied, the manual has the table of 2.x separators and what to pass now, the test comment and the end-of-file newline change are gone, and the branch is rebased onto main. Retitled as you had suggested.

[rzo1]

@krickert Please rebase.

[krickert]

Rebased onto main. The README now lists this break under the "Breaking changes" section that #1278 added, so both bullets are there.