FreeTextSuggester accepts a token that contains its separator byte when the token has no neighbours, and some later lookups throw NullPointerException

FreeTextSuggester builds an n-gram model: ShingleFilter joins neighbouring tokens with a separator byte (U+001E by default, any 7-bit byte if configured) and every unigram and n-gram goes into one FST. Its 4-arg constructor javadoc says "No input tokens should have this byte, otherwise IllegalArgumentException is thrown" (FreeTextSuggester.java:179-180 on main 645d4fe14a20). build() never inspects the tokens, though: it reads the shingled terms back from a temporary index and throws only if a term's separator count implies more than grams words (:305-306). A token that itself contains k separators therefore looks like a (k+1)-gram; it passes whenever every shingle it ends up in still counts at most grams (with bigrams and no position gaps: a token with one separator passes whenever it is the only token of its entry). With the default bigram model the one-token entry foo\u001ebar is accepted, while the upstream test input foo\u001ebar baz is rejected only because its shingle foo␞bar␞baz counts 3. The token is stored as the bigram (foo, bar) without a unigram foo; lookup("foo ") with a word-splitting analyzer then throws AssertionError at :576 (-ea) or NullPointerException in decodeWeight at :724 (-da, as in production), and the token can come back from the no-context (1-gram) search with a wrong score. Only the opt-in, @lucene.experimental FreeTextSuggester with an analyzer that keeps the separator inside a token is affected (KeywordAnalyzer, UnicodeWhitespaceAnalyzer, or a custom separator the tokenizer keeps); StandardAnalyzer, WhitespaceAnalyzer and SimpleAnalyzer split on U+001E. The check reads the same in every release from 4.6.0 (2013) to 10.6.0 and on main (12 releases and main executed, the rest read from source or inferred).

• Contract. DEFAULT_SEPARATOR = 0x1e: "The input tokens produced by the analyzer must not contain this character" (:147-151). The 4-arg constructor: the separator "must be an ascii (7-bit-clean) byte. No input tokens should have this byte, otherwise IllegalArgumentException is thrown" (:176-181); the other constructors delegate to it (:157-174).
• Shingling. Unless grams == 1, both analyzers get a ShingleFilter(stream, 2, grams) with the separator as token separator (:185-186, :214-235). It emits each token and each run of 2 to grams neighbouring tokens joined by the separator.
• Build. Each entry is indexed in a temporary index (:276-284); the terms are read back and countGrams(term), 1 + the number of separator bytes (:414-423), is compared with grams: only ngramCount > grams throws "tokens must not contain separator byte" (:305-313). Only terms with count 1 add to the unigram total (:315-317); every term goes into the FST (:319-320). A term read back from an index carries no position length, so the build cannot tell the token foo␞bar from the shingle of foo and bar.
• When it misses. The separator check in build() throws exactly when some emitted term has countGrams(term) > grams (build() also throws for payloads, contexts and an empty term set, :247-252, :287-290). For an entry without position gaps and with one separator-carrying token, a token with k separators passes when k + 1 <= grams and it has at most grams - 1 - k neighbours: with bigrams, an entry that is that single token; with grams = 3, a token with one separator next to one other token. Several such tokens in one shingle add up, and across a position gap ShingleFilter inserts its filler token _ (ShingleFilter.java:46), which adds one more join.
• Lookup checks exactly. For each query token it compares countGrams with the ShingleFilter position length (// Safety: make sure the recalculated count "agrees", :459-468).
• Lookup fails. For "foo " the last token becomes the context foo␞ (:494-509). That prefix exists in the FST (from foo␞bar), so lookup fetches the count of foo: Util.get returns null and assert output != null fails (:575-576); without assertions decodeWeight(null) unboxes null (:724).

After a build over foo␞bar and hello world with UnicodeWhitespaceAnalyzer: lookup("foo ") throws and lookup("f") returns foo␞bar/0.50 (the single entry foo␞bar alone fails the same way, and there lookup("f") gives 1.00 because no unigram was counted); the probes' unrelated lookups kept working (lookup("hello "), also with separator '_' after [foo_bar, hello world, hello there]). With separator '_' and [foo bar, foo_bar], lookup("f") returns foo_bar/1.00 ahead of foo/0.50. If foo also occurs on its own in the build text ([foo_bar, foo qux]), lookup("foo ") works.

• Reach. The default U+001E reaches it with KeywordAnalyzer, UnicodeWhitespaceAnalyzer (U+001E is not Unicode White_Space) or a keyword- or pattern-style tokenizer. A custom separator (any 7-bit byte; in Solr a FreeTextLookupFactory setting next to any analyzer field type) reaches it whenever the tokenizer keeps it inside words. StandardAnalyzer, WhitespaceAnalyzer and SimpleAnalyzer split the token and are unaffected with the default separator: WhitespaceTokenizer because Character.isWhitespace(0x1E) is true, SimpleAnalyzer's LetterTokenizer because U+001E is not a letter, StandardTokenizer by its UAX #29 word-break rules.
• What goes wrong. The documented IllegalArgumentException is not thrown, so a model the API means to reject is built. With a query analyzer that splits the key into words, lookups whose last complete word is the token's part before the separator (foo for foo␞bar: "foo ", "foo b", "x foo "), when that word never occurs on its own in the build text, then throw on every such request until the model is rebuilt (also after store()/load()). Lookups on the part after it ("bar ", "x bar ") return nothing, and with KeywordAnalyzer on both sides these lookups do not throw; the probes' unrelated lookups kept working. The token can also come back from the no-context (1-gram) search with an inflated score.
• What it takes. With bigrams, an entry that is that single token (for example one-word query-log lines); with grams >= 3, few enough neighbours. The lookup failure also needs the token's part before the separator to be absent as a standalone word, and a query analyzer that splits the key into words (with KeywordAnalyzer on both sides, foo␞bar yields only the stray completion, because the query foo stays one token).
• Cost. Deterministic, no scale. The application's index is never touched: build writes only to its own temporary index, which it always deletes (after rolling it back when the build succeeds). No hang (a failing lookup takes a few microseconds).
• Workaround. Strip or replace the separator byte in the build text, or pick a separator the analyzer always splits on.

Copy the test into lucene/suggest/src/test/org/apache/lucene/search/suggest/analyzing/ of a Lucene checkout and run it with gradle (command below). With the per-token check (Suggested fix) it passes unchanged, so it doubles as the regression test.

Executed: this gradle command on a clean 645d4fe14a20 (assertions off for this seed) and again with -Ptests.asserts=true in a fresh scratch worktree (fails, same message); the standalone runner on the stock main jars and on the 10.6.0 release jars, each with -ea and -da (fails, same message).

With -Ptests.seed=DEADBEEF, Lucene's gradle runs with assertions off (it picks tests.asserts at random per seed); add -Ptests.asserts=true for an assertions-on run, which fails the same way.

Left out on purpose: the lookup failure. It needs no second entry, but a query analyzer that splits the key into words (for example UnicodeWhitespaceAnalyzer; with this test's KeywordAnalyzer, lookup("foo ") returns nothing). It is described under "The finding in 30 seconds" with its executed output.

cp $REPRO/TestFreeTextSuggesterSeparatorInToken.java $LUCENE/lucene/suggest/src/test/org/apache/lucene/search/suggest/analyzing/
cd $LUCENE && ./gradlew :lucene:suggest:test --tests org.apache.lucene.search.suggest.analyzing.TestFreeTextSuggesterSeparatorInToken -Ptests.seed=DEADBEEF

# gradle, main 645d4fe14a20, -Ptests.seed=DEADBEEF
TestFreeTextSuggesterSeparatorInToken > testTokenWithDefaultSeparatorRejected FAILED
    junit.framework.AssertionFailedError: Expected exception IllegalArgumentException but no exception was thrown
        at org.apache.lucene.tests.util.LuceneTestCaseParent.expectThrows(LuceneTestCaseParent.java:1432)
        at org.apache.lucene.tests.util.LuceneTestCaseParent.expectThrows(LuceneTestCaseParent.java:1418)
        at org.apache.lucene.search.suggest.analyzing.TestFreeTextSuggesterSeparatorInToken.testTokenWithDefaultSeparatorRejected(TestFreeTextSuggesterSeparatorInToken.java:30)

# standalone runner, seed DEADBEEF
main 645d4fe14a20  -ea / -da   tests_run=1 failures=1   same message
10.6.0             -ea / -da   tests_run=1 failures=1   same message

• Contract. DEFAULT_SEPARATOR: "The input tokens produced by the analyzer must not contain this character" (:147-150). The 4-arg constructor promises the IAE (:179-180), and the build's error message names exactly this condition: "tokens must not contain separator byte" (:308).
• Test. testIllegalByteDuringBuild (TestFreeTextSuggester.java:121-134), with the comment "no input token is allowed to contain it", asserts the IAE at build time for foo\u001ebar baz. It passes only because of the neighbour baz.
• Code. Where the position length is available (lookup), the check is exact (:459-468), and lookup asserts the model invariant that every context exists (:576). Sibling suggest code checks reserved characters one by one and throws an IAE: SuggestField and ContextSuggestField document it in their javadoc (SuggestField.java:75-76, ContextSuggestField.java:62-63); AnalyzingSuggester throws it, with a message but no javadoc, for a lookup key and, with payloads, for a surface form at build.
• History. The promise, the check, countGrams, the lookup assert and the test came in one commit (LUCENE-5214, 2013). Its issue (#6278, 12 comments) never calls the build check partial or best-effort, and no comment, CHANGES or MIGRATE entry does.

The strongest case for "works as designed" (the cross-model lenses and the intended-behaviour checker): separator-free tokens are a caller precondition ("should", "must not"), so behaviour after violating it is unsupported; the class is experimental; checking each unique indexed term may have been chosen as a cheap safeguard rather than complete validation; and SOLR-11139 even asks for a more lenient build. Also conceded: the javadoc does not say when the IAE is thrown (at build or at lookup).

Why it fails: the same javadoc sentence promises the IAE for exactly this violation and the upstream test asserts it at build; here it is thrown neither at build nor at lookup; nothing records the check as best-effort; a per-token check is one byte scan per token, the same work lookup already does for every token; and under any reading the outcome is wrong, with a word-splitting query analyzer, a later NullPointerException on a lookup key that itself contains no separator, instead of the documented IAE. SOLR-11139 is about the fix direction (reject or skip), not evidence that silent acceptance is wanted. Verdict: a defect.

Three options. The per-token check was validated with gradle in a fresh scratch worktree at 645d4fe14a20 (assertions on, whole suggest suite); the other two were not executed. Whatever the choice, the token has to be treated the same with or without neighbours.

In FreeTextSuggester.addShingles, wrap the wrapped analyzer's token stream in a private SeparatorCheckFilter and feed that to ShingleFilter. The filter scans each token's bytes and throws IllegalArgumentException("tokens must not contain separator byte; got token=..."). It applies to the index and the query analyzer; with grams == 1 there is no shingling and the existing build check is already exact.

TokenStream checked = new SeparatorCheckFilter(components.getTokenStream(), separator);
ShingleFilter shingles = new ShingleFilter(checked, 2, grams); // was: (components.getTokenStream(), 2, grams)

private static final class SeparatorCheckFilter extends TokenFilter {
  ...
  public boolean incrementToken() throws IOException {
    if (input.incrementToken() == false) return false;
    BytesRef term = termAtt.getBytesRef();
    for (int i = 0; i < term.length; i++)
      if (term.bytes[term.offset + i] == separator)
        throw new IllegalArgumentException("tokens must not contain separator byte; got token=" + term);
    return true;
  }
}

Trade-offs

• Builds that succeed today with such a token now fail as a whole, as the javadoc says. That is stricter, and the opposite of SOLR-11139's request.
• Cost: one byte scan per analyzed token at build and lookup (lookup already counts separators per token); not benchmarked.
• A lookup key whose token contains the separator still gets an IllegalArgumentException (now from the filter), so upstream testIllegalByteDuringQuery keeps passing.
• The existing n-gram check stays. It still fires in one case the new filter cannot see: across a position gap ShingleFilter inserts its filler token _, so with separator _ a legal entry such as the foo (stop word the) is rejected, on main today and with the fix alike.
• Constructor order is safe: addShingles() runs before this.separator is assigned, but the wrapper builds its token stream only when the analyzer is first used, after construction, and the new line reads the suggester's final field separator, as the existing setTokenSeparator line does.

What the checks validated

• Gradle, fresh scratch worktree at 645d4fe14a20, JDK 25, -Ptests.seed=DEADBEEF -Ptests.asserts=true: stock, the minimal test fails; with the fix, :lucene:suggest:checkGoogleJavaFormat passes and the whole :lucene:suggest:test suite passes, 237 tests in 32 suites, 7 skipped, 0 failures, including the minimal test, TestFreeTextSuggester (testIllegalByteDuringBuild and testIllegalByteDuringQuery pass) and TestAnalyzingSuggester.
• An earlier run of the same suite with assertions at the seed's default (off): 237 tests, 0 failures; an independent fact check repeated it.
• The test file used in the gradle runs has its one 107-character comment line wrapped onto two lines, which the format check requires; nothing else differs from the repro.
• Standalone runner with the patched class shadowed: the minimal test passes; the steps probe gets the IllegalArgumentException for foo␞bar alone and for foo␞bar baz, still builds foo bar, and rejects the two-entry build at build time, so no lookup can fail; the lookup audit probe passes 4 of 4.
• Earlier shadow runs of the hunt's equivalent diff: the finder's 4 tests, an independent reproducer's 6 and the intended-behaviour checker's 7 pass.

Drop such tokens (or the entries that contain them) before shingling instead of failing the build, and change the javadoc to say so. Not executed.

Trade-offs

Lenient, and it resolves SOLR-11139 too, but it contradicts the current javadoc and test, so it is an API behaviour change; silently dropping input can hide a misconfigured separator. The check must still be per token, or the hole stays.

Say in the javadoc that only tokens whose separators push an n-gram past grams are rejected. Not executed (documentation only).

Trade-offs

No behaviour change, but the lookup NullPointerException and the stray completions stay; not recommended.

Method: bounded-exhaustive translation validation (the hunt's semi-formal tier), in a hunt unit on in-band separators and reserved symbols, run on the stock main jars at ea57360912ed (FreeTextSuggester.java is unchanged since).

• Property P1: build() throws IllegalArgumentException if and only if an analyzer token contains the separator byte. Property P2: after a successful build, lookup() throws nothing but IllegalArgumentException.
• Inputs: separators U+001E and _; keyword, space-splitting and standard analyzers; grams 1 to 3; every 1-entry dictionary of strings of length <= 3 and every 2-entry dictionary of strings of length <= 2 over {a, b, space, separator}; every lookup key of length <= 3 over {a, b, space}, with 1 and 5 results.
• Result: 2,536 accepted-illegal builds, 0 legal inputs rejected; lookup failures only after accepted-illegal builds (3,564 AssertionError with -ea, 2,820 NullPointerException with -da), none after legal builds.
• The hypothesis came from reading the build check while listing the reserved in-band constants; the harness then witnessed and bounded it, and the finding was reduced to a public-API JUnit test, later to the single test shown here.