OpenNLPSentenceBreakIterator.next(n) returns DONE instead of the end of the text when it lands on the last boundary

OpenNLPSentenceBreakIterator is the OpenNLP module's java.text.BreakIterator over sentences. next(n) should return the n-th boundary from the current one, the same value as n calls of next(). When that boundary is the end of the text it returns DONE (-1) instead, although it does move the iterator there: next(3) from first() on the module's three-sentence test text returns -1 instead of 102, and with no model at all next(1) returns -1 where next() returns 27. A loop that steps with next(n) until DONE loses its last group of sentences (with next(1), the last sentence of every text). No Lucene code calls next(int) on this class, so tokenization, indexing and search are unchanged. Every release from 7.3.0 to 10.6.0 (10.6.0 by reading) and main is affected; a small fix in next(int) (+4/-1 lines) was validated with the module's tests.

• Boundary: a position between sentences. For a text of three sentences the boundaries are the three sentence starts and the end of the text: 0, 31, 59 and 102 for the module's test text.
• next() / next(n): next() moves to the boundary after the current one and returns it, or returns DONE (-1) when the current boundary is already the last one. next(n) jumps n boundaries (BreakIterator#next() is documented as "Equivalent to next(1)"); it returns DONE when there are fewer than n boundaries left.

1. setText() stores only the sentence starts (OpenNLPSentenceBreakIterator.java:195-205): {0, 31, 59}. The end of the text has no entry; counted as a boundary it is number sentenceStarts.length, here 3.
2. next() (:62-72) returns DONE only when the position is already the end (:64). From the last sentence start it calls last() (:69-70), which moves to the end and returns it (:51-60).
3. next(int n) (:163-187) adds n to the counter first: currentSentence += n (:165).
4. Backwards (n < 0, :166-176) it adds one when the position is the end (:167-169), so it counts the end as boundary number 3, and it returns DONE only for a counter below 0: landing on the first boundary returns 0.
5. Forwards (n > 0, :177-185) it returns DONE when currentSentence >= sentenceStarts.length (:178), after moving to the end (:179-181). >= also matches the end boundary itself, so next(3) from 0 (counter 3, 3 >= 3) moves to 102 and returns -1. By the next() javadoc ("Equivalent to next(1)") and the next(int) javadoc example, only a counter above 3, or a call made at the end, should give DONE; the next(int) javadoc's "has been reached" sentence is the strongest contrary reading (see "Why it is not intended").

Only the returned value is wrong: in a probe with a fixed-span detector and 1 to 6 sentences, the position and the later calls it made (current(), previous(), next(), next(1), last(), following(0), preceding(end), next(-1)) gave the same results as with the fix.

• Who is exposed. Application code that uses OpenNLPSentenceBreakIterator itself as a java.text.BreakIterator and calls next(int) with n > 0. The class is public final in an exported package of the opt-in lucene-analysis-opennlp module, with no @lucene.internal tag; the public no-argument NLPSentenceDetectorOp is enough (no model file).
• Deterministic. With n = 1 it fires from the last sentence start of every non-empty text; for larger n whenever the n-th boundary is the end. Assertions on or off. Where there is no distinct end boundary to land on (an empty text, or no sentences), DONE is already right.
• Effect. One wrong return value, -1 instead of the end offset. A loop that stops at DONE loses its final group whenever the sentence count is a multiple of n; with n = 1 it loses the last sentence of every text. In a probe over 48 layouts, such a loop was wrong in 48 on main and in 23 with the fix or the JDK iterator (the overshoot cases the contract allows). A caller that uses the value as an offset unchecked gets a StringIndexOutOfBoundsException. A caller that reads current() after DONE is unaffected.
• Not in Lucene's own paths. OpenNLPTokenizer is the only in-tree user, through SegmentingTokenizerBase, which calls only setText, current() and next(); it gives the same 24 tokens and offsets on main, the fixed build and 10.5.2. The only other next(int) call outside tests, in LengthGoalBreakIterator, is marked "Not supported".
• How likely. Rare: no caller of next(int) on this class was found in Lucene, Solr, Elasticsearch, OpenSearch or the public code-search hits read, and nobody reported it in the eight years since 7.3.0.

# Lucene main 645d4fe14a20, assertions on (-ea), seed DEADBEEF: 1 test(s) run, 1 failed
    => java.lang.AssertionError: next(1) from first() expected:<13> but was:<-1>
       org.junit.Assert.fail(Assert.java:89)
       org.junit.Assert.failNotEquals(Assert.java:835)
       org.junit.Assert.assertEquals(Assert.java:647)
       org.apache.lucene.analysis.opennlp.TestSentenceBreakIteratorNextOne.testNextOneToEndOfText(TestSentenceBreakIteratorNextOne.java:29)

• The contract. BreakIterator#next(): returns DONE "If the current boundary is the last text boundary", and "Equivalent to next(1)". BreakIterator#next(int): "Returns the nth boundary from the current boundary"; its example, with three more boundaries up to the last one: "the next(2) call will return m + 2" and "A next(4) call would return BreakIterator.DONE", so next(3) returns the last boundary. @return: "The character index of the nth boundary from the current position".
• The class contradicts itself. Its next() returns the end from the last sentence start (OpenNLPSentenceBreakIterator.java:69-70; the upstream test asserts it at TestOpenNLPSentenceBreakIterator.java:90 and :130), while next(1) from there returns DONE. Its backward branch counts the end as a boundary and returns the edge boundary it lands on (OpenNLPSentenceBreakIterator.java:166-176; the test asserts next(-2) == 0 at :115).
• Nothing documents the forward exact-landing case. The upstream tests use next(2) and the overshoots next(13) and next(-8) (TestOpenNLPSentenceBreakIterator.java:114, :147-148, :226-227); no code comment, CHANGES or MIGRATE entry, issue or PR states it. The class's deliberate JDK deviations in following() and preceding() carry a JDK-8015110 comment (OpenNLPSentenceBreakIterator.java:82-84, :144-146); next(int) carries none.
• The other implementations checked return the end. The JDK and ICU4J sentence iterators return 102 for next(3) on the same text. In Lucene, SplittingBreakIterator, WholeBreakIterator and CustomSeparatorBreakIterator implement next(n) as a loop over next() and so return the end.
• History. The class was added as a "shim class" for OpenNLPTokenizer (LUCENE-2899); none of the issue's 136 comments mentions next(int).

The strongest case for "works as designed": (1) the next(int) javadoc's first sentence, "If either the first or last text boundary has been reached, it returns BreakIterator.DONE", read as "landing on the last boundary returns DONE"; (2) upstream deliberately keeps non-JDK edge behaviour in this class: following(pos) returns DONE anywhere in the last sentence (asserted at TestOpenNLPSentenceBreakIterator.java:142), and preceding() differs from the JDK (:109-110; #13170 added preceding(25) == 0 "verifying correct behavior" in 2024, where the JDK gives 22). So "the JDK does it" alone does not settle what this class should do.

Why it fails: next() is "Equivalent to next(1)" and this class's own next() returns the end, so reading (1) contradicts the class itself; the javadoc example shows DONE only on overshoot; and the backward branch returns the edge boundary it lands on, which reading (1) would forbid. The following()/preceding() deviations are commented and tested, the next(int) one is neither. A works-as-designed review by a reviewer from another model family rated the case moderate and rejected it; a separate devil's-advocate checker rejected it with "fairly high confidence" (its words).

Options A and B were each applied in a scratch worktree of main 645d4fe14a20 and run through gradle with assertions on and off (B not through check), and through the repro harness with the hunt's walk oracle. The overflow of next(Integer.MAX_VALUE) after one next() (an ArrayIndexOutOfBoundsException) is a separate observation that was not checked as a bug of its own; fix A leaves it, fix B removes it.

In next(int) (OpenNLPSentenceBreakIterator.java:177-185), return DONE only when the call starts at the end of the text or the counter passes sentenceStarts.length; when the counter equals sentenceStarts.length, set the counter to the last sentence, move to the end and fall through to return current(). This is the finder's fix.

Why the position check. At the end the counter is sentenceStarts.length - 1, as last() and next() leave it, so the counter alone cannot tell "at the end" from "at the last sentence start".

     } else if (n > 0) {
-      if (currentSentence >= sentenceStarts.length) {
+      if (text.getIndex() == text.getEndIndex() || currentSentence > sentenceStarts.length) {
         currentSentence = sentenceStarts.length - 1;
         text.setIndex(text.getEndIndex());
         return DONE;
+      } else if (currentSentence == sentenceStarts.length) { // the n-th boundary is the end
+        currentSentence = sentenceStarts.length - 1;
+        text.setIndex(text.getEndIndex());
       } else {

Trade-offs

The smallest change at the root cause, still O(1). The returned value changes only where it was -1 for an existing boundary. It leaves the separate leading-whitespace bug of this class and the huge-n overflow as they are. Compatibility (A and B alike): a public method now returns the end offset where it returned DONE; no in-tree or public caller found depends on that, but private callers that treat the terminal DONE as a stop signal cannot be ruled out.

What the checks validated

• The new test: 3 of 3 pass with assertions on and off (stock: 2 of 3 fail); the upstream test with the additions: 9 of 9 (stock: 6 of 9 fail).
• All of :lucene:analysis:opennlp:test: 6 classes, 36 tests, 0 failures, assertions on and off.
• :lucene:analysis:opennlp:check -x test passes (google-java-format, ecjLint, forbiddenApis, licenses) with the fix and both tests in the tree.
• Repro harness, fix shadowed: the new test with -ea and -da; earlier the finder's, the independent reproducer's and the devil's advocate's tests; the walk oracle's next(k) rule 0 of 84 layouts (stock: 84 of 84); the history probe passes on 7.3.0 and main.

Replace the body with two loops that call next() (n > 0) or previous() (n < 0) until n steps are done or one returns DONE, the shape of SplittingBreakIterator.next(int) in the highlighter module (+8/-21).

  @Override
  public int next(int n) {
    // n steps of next() or previous(): BreakIterator#next() is "Equivalent to next(1)"
    int result = current();
    for (; n > 0 && result != DONE; --n) {
      result = next();
    }
    for (; n < 0 && result != DONE; ++n) {
      result = previous();
    }
    return result;
  }

Trade-offs

It follows the definition of next(n) as repeated next()/previous() calls (checked on the test texts and the walk oracle's 84 layouts) and adds nothing to a counter, so the tested huge-n case returns DONE instead of overflowing; but it costs O(min(|n|, boundaries left)) per call instead of O(1), and next(-n) then depends on previous(), which throws on text that starts with whitespace (a separate bug in this class): in the walk oracle the crash count goes from 42 to 126 until that bug is fixed. With its fix (setText sets the first sentence start to the begin index), A and B both give 0 everywhere. Prefer B only if both bugs are fixed together.

What the checks validated

• The new test 3 of 3 and the upstream test with the additions 9 of 9, assertions on and off; all 36 module tests with assertions on. Not run through check.
• The walk oracle's next(k) rule 0 of 84; next(Integer.MAX_VALUE) after one next() returns DONE instead of throwing.

A javadoc note that next(n) returns DONE when it reaches the end of the text.

Trade-offs

Keeps next(1) != next() and the asymmetry with next(-n), so it contradicts the inherited BreakIterator#next() contract.

• Method (semi-formal): bounded-exhaustive internal-consistency checking of the stock jar, in the hunt's unit on the OpenNLP filters against the OpenNLP library. A stub sentence detector produced 1 to 14 sentences with start gaps of 1 to 3 characters and a leading offset of 0 or 2 (84 layouts), and three walk rules compared the navigation methods with each other: W1 backward walk = forward walk, W2 next(k) from first() = k calls of next() (and DONE past the end), W3 next(-k) from last() = k calls of previous().
• Result. W2 failed in all 84 layouts, each exactly at k = the number of sentences (the end boundary), and in none with the fix. W1 and W3 failed only in the 42 layouts with a leading offset: that is the separate leading-whitespace bug of this class, with its own fix.
• The finder cross-checked the JDK sentence iterator (it returns the end for the same jump), then wrote a public-API JUnit with the module's test model; independent checkers followed (see "Independent checks").