OPENNLP-1898: Address the OPENNLP-1876 review follow-ups - #1196
Merged
Conversation
These notes were raised in the review of OPENNLP-1876 and addressed before PR #1151 merged, but the commit was never pushed, so the work is absent from main. - Document IllegalArgumentException and IllegalStateException on the public spellcheck normalizer, which throws both but declared neither - Restore the SpellCorrectingTokenStream class javadoc, including the contrast with SpellCorrectingObjectStream in compound mode that explains why the stream is token count preserving - Hoist the former-regex Pattern constants out of the four characterization test bodies, where they were recompiled on every one of the 5000 randomized inputs per case, and name what each pattern represents - Turn the adversarial URL pins into a parameterized test so a regression names the shape that broke instead of only the test
rzo1
approved these changes
Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the review of OPENNLP-1876 (#1151). These notes were raised in that review and
addressed on 2026-07-16, but the commit was never pushed and #1151 merged later the same
day without it, so the work is missing from
main.No behaviour changes. This is review follow-up work: 6 files, 68 insertions, 41 deletions.
What this adds
Undocumented exceptions on public spellcheck API.
SpellCheckingCharSequenceNormalizer.normalizethrowsIllegalArgumentExceptionon anull input and
IllegalStateExceptionwhen noSpellCheckeris attached, and two settersreject out of range values. None of that was declared. The
@throwstags are added so thecontract is visible to callers, matching the rest of the normalizer family.
Lost explanation on
SpellCorrectingTokenStream.The class javadoc no longer explained that the stream is token count preserving in
contrast to
SpellCorrectingObjectStreamin compound mode, which is the property callersrely on to keep tags and spans aligned with corrected tokens. That contrast and the cross
reference are restored.
Reference patterns recompiled on a hot path.
The
Number,Shrink,SocialMediaandUrlcharacterization tests each calledPattern.compileinside the test body for the former regexes they differentially testagainst. Those are the randomized tests that run 5000 generated inputs per case, so the
patterns were recompiled per invocation. They are hoisted into named
static finalconstants (
FORMER_DIGIT_REGEX,FORMER_SPACE_REGEX,FORMER_REPEATED_CHAR_REGEX,FORMER_HASH_USER_REGEX,FORMER_RT_REGEX,FORMER_FACE_REGEX,FORMER_URL_REGEX,FORMER_MAIL_REGEX), which also names what each pattern is instead of leaving a bareliteral inside the assertion.
The adversarial URL test hid which input failed.
weirdUrlsMatchTheFormerRegexExactlydrove a hard codedString[]of adversarial URLshapes (userinfo, ports, percent escapes, IPv6 brackets, backslashes, IDN and punycode
hosts) through one
@Test, so a regression reported only that the test failed. It is nowa
@ParameterizedTestwith@MethodSource, so the failing shape is named in the report.Verification
Branch is based on current
main(a8642301f) and applies cleanly../mvnw -pl opennlp-extensions/opennlp-spellcheck,opennlp-core/opennlp-runtime -am test-compilesucceedsSocialMediaCharSequenceNormalizerCharacterizationTest(9) andUrlCharSequenceNormalizerCharacterizationTest(32)