mawiesne commented on code in PR #1151:
URL: https://github.com/apache/opennlp/pull/1151#discussion_r3593681082
##########
opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/normalizer/SpellCheckingCharSequenceNormalizer.java:
##########
@@ -344,7 +435,10 @@ private Builder(SpellChecker spellChecker) {
* @return this builder
*/
public Builder mode(Mode value) {
- this.mode = Objects.requireNonNull(value, "mode must not be null");
+ if (value == null) {
+ throw new IllegalArgumentException("mode must not be null");
Review Comment:
This needs to be mentioned in Javadoc! Please add `@throws` for
IllegalArgumentException.
##########
opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/util/normalizer/UrlCharSequenceNormalizerCharacterizationTest.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package opennlp.tools.util.normalizer;
+
+import java.util.Random;
+import java.util.regex.Pattern;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+/**
+ * Characterization tests for {@link UrlCharSequenceNormalizer}.
+ *
+ * <p>The fixed expectations below were probed against the regex implementation
+ * ({@code "https?://[-_.?&~;+=/#0-9A-Za-z]+"} to a space, then
+ * {@code "(?<![-+_.0-9A-Za-z])[-+_.0-9A-Za-z]+@[-0-9A-Za-z]+[-.0-9A-Za-z]+"}
to a space) and pin
+ * the accept/reject boundary byte for byte. Notable sharp edges kept
visible:</p>
+ * <ul>
+ * <li>the scheme is lowercase-only, and a URL body needs at least one
character out of the
+ * body class, so {@code "http:// x"} and {@code "http://"} pass through
untouched;</li>
+ * <li>the body class has no colon, percent sign, at sign, or non-ASCII, so
ports, escapes,
+ * userinfo, and IDN labels cut the match short;</li>
+ * <li>a mail match needs a local part run bounded on the left by the
lookbehind, and a domain
+ * of at least two characters starting with no dot: {@code "a@b c"} does
not match, while
+ * {@code "a@b."} swallows the trailing dot;</li>
+ * <li>after a failed candidate the scan resumes one char further, so {@code
"a@[email protected]"}
+ * still finds the inner address.</li>
+ * </ul>
+ */
+public class UrlCharSequenceNormalizerCharacterizationTest {
+
+ private static final UrlCharSequenceNormalizer NORMALIZER =
+ UrlCharSequenceNormalizer.getInstance();
+
+ private static void check(String input, String expected) {
+ assertEquals(expected, NORMALIZER.normalize(input).toString());
+ }
+
+ @Test
+ void urlsBecomeASingleSpace() {
+ check("", "");
+ check("asdf http://asdf.com/dfa/cxs 2nnfdf", "asdf 2nnfdf");
+ check("asdf http://asdf.com/dfa/cxs 2nnfdf http://asdf.com/dfa/cxs";, "asdf
2nnfdf ");
+ check("http://example.com";, " ");
+ check("visit http://example.com now", "visit now");
+ check("https://a.b/c?d=e&f=g#h+i~j;k=l_m-n";, " ");
+ check("https://x";, " ");
+ check("http://EXAMPLE.com";, " ");
+ check("xhttp://y";, "x ");
+ check("ahttp://x";, "a ");
+ }
+
+ @Test
+ void urlRecognitionRejectsAtTheExactRegexBoundary() {
+ check("http:// x", "http:// x");
+ check("http://";, "http://";);
+ check("HTTP://X.COM", "HTTP://X.COM");
+ check("httpss://x", "httpss://x");
+ check("ftp://x";, "ftp://x";);
+ check("www.example.com", "www.example.com");
+ // The body class has no colon, percent sign, or at sign, and no non-ASCII.
+ check("http://x:8080/y";, " :8080/y");
+ check("http://a%20b";, " %20b");
+ check("http://http://x";, " ://x");
+ check("http://\u043F\u0440\u0438\u043C\u0435\u0440";,
"http://\u043F\u0440\u0438\u043C\u0435\u0440";);
+ check("http://x.\u043F\u0440\u0438\u043C\u0435\u0440";, "
\u043F\u0440\u0438\u043C\u0435\u0440");
+ check("http://[email protected]";, " @example.com");
+ }
+
+ @Test
+ void mailAddressesBecomeASingleSpace() {
+ check("asdf [email protected] 2nnfdf", "asdf 2nnfdf");
+ check("asdf [email protected] 2nnfdf [email protected]", "asdf
2nnfdf ");
+ check("asdf [email protected] 2nnfdf", "asdf 2nnfdf");
+ check("asdf [email protected]_br 2nnfdf", "asdf _br 2nnfdf");
+ check("[email protected]", " ");
+ check("[email protected] end", " end");
+ check("[email protected]", " ");
+ check("[email protected]", " ");
+ check("a@-b", " ");
+ check("[email protected]", " ");
+ check("x@y-.", " ");
+ check("mailto:[email protected]";, "mailto: ");
+ check("[email protected],[email protected]", " , ");
+ check("a [email protected] f", "a f");
+ check("see http://x.y, then [email protected]!", "see , then !");
+ }
+
+ @Test
+ void mailRecognitionRejectsAtTheExactRegexBoundary() {
+ // A one-char domain with nothing to lend to the second domain class fails.
+ check("a@b c", "a@b c");
+ check("a@bc d", " d");
+ // The trailing dot is part of the second domain class and gets swallowed.
+ check("a@b.", " ");
+ check("[email protected].", " ");
+ check("Contact [email protected].", "Contact ");
+ // The domain must not start with a dot.
+ check("[email protected]", "[email protected]");
+ check("[email protected]", "[email protected]");
+ // The local part must be a maximal run: no match can start inside one.
+ check("user@@example.com", "user@@example.com");
+ check("@example.com", "@example.com");
+ // After the failed candidate at "a", the scan resumes and finds
"[email protected]".
+ check("a@[email protected]", "a@ ");
+ }
+
+ @Test
+ void lookbehindOnlySeesTheSingleCharBeforeTheLocalPart() {
+ // Non-ASCII (including a low surrogate) is outside the lookbehind class,
so the address
+ // right after it still matches.
+ check("\uD83D\[email protected]", "\uD83D\uDE00 ");
+ check("\[email protected]", "\u00E9 ");
+ }
+
+ @Test
+ void nullTextIsRejected() {
+ // Null input is rejected with IllegalArgumentException.
+ assertThrows(IllegalArgumentException.class, () ->
NORMALIZER.normalize(null));
+ }
+
+ @Test
+ void matchesTheFormerRegexesOnRandomizedInputs() {
+ final Pattern urlRegex =
Pattern.compile("https?://[-_.?&~;+=/#0-9A-Za-z]+");
+ final Pattern mailRegex =
+
Pattern.compile("(?<![-+_.0-9A-Za-z])[-+_.0-9A-Za-z]+@[-0-9A-Za-z]+[-.0-9A-Za-z]+");
+ final String[] pool = {"a", "b", "Z", "1", " ", ".", ",", "-", "+", "_",
"@", ":", "/", "%",
+ "~", ";", "=", "&", "?", "#", "http", "https", "ttp", "://",
"http://";, "s", "x.com",
+ "co", ".com", "a@b", "@b.co", "\u00E9", "\uD83D\uDE00", "\uD83D",
"\uDE00"};
+ final Random random = new Random(42);
+ for (int i = 0; i < 5000; i++) {
+ final String input = CharacterizationInputs.randomInput(random, pool);
+ final String expected = mailRegex
+ .matcher(urlRegex.matcher(input).replaceAll(" "))
+ .replaceAll(" ");
+ assertEquals(expected, NORMALIZER.normalize(input).toString(),
+ () -> "Input: " + CharacterizationInputs.escape(input));
+ }
+ }
+ @Test
+ void noMatchInputIsReturnedUncopied() {
+ final String plain = "no links in this sentence at all";
+ Assertions.assertSame(plain, NORMALIZER.normalize(plain));
+ }
+
+ @Test
+ void weirdUrlsMatchTheFormerRegexExactly() {
+ // Adversarial shapes in the spirit of the web-platform-tests URL suite:
userinfo, ports,
+ // percent escapes, IPv6 brackets, backslashes, IDN and punycode hosts,
stray schemes and
+ // separators. Each input runs differentially through the two former
patterns, so the
+ // accept/reject boundary is pinned by construction rather than by hand.
+ final Pattern urls = Pattern.compile("https?://[-_.?&~;+=/#0-9A-Za-z]+");
+ final Pattern mails =
+
Pattern.compile("(?<![-+_.0-9A-Za-z])[-+_.0-9A-Za-z]+@[-0-9A-Za-z]+[-.0-9A-Za-z]+");
+ final String[] inputs = {
Review Comment:
Please change this test to be a `@ParameterizedTest` with this array of hard
coded examples as input parameter. Benefit is: We can then better identify
which of the examples urls breaks it, if in the future the underlying code is
change or adapted. It's easier to spot the problem right away for devs and or
robots.
##########
opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/normalizer/SpellCheckingCharSequenceNormalizer.java:
##########
@@ -152,15 +136,21 @@ private SpellCheckingCharSequenceNormalizer(Builder b) {
* @return a new {@link Builder} seeded with sensible defaults
*/
public static Builder builder(SpellChecker spellChecker) {
- return new Builder(Objects.requireNonNull(spellChecker, "spellChecker must
not be null"));
+ if (spellChecker == null) {
+ throw new IllegalArgumentException("spellChecker must not be null");
Review Comment:
This needs to be mentioned in Javadoc! Please add `@throws` for
IllegalArgumentException.
##########
opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/normalizer/SpellCheckingCharSequenceNormalizer.java:
##########
@@ -152,15 +136,21 @@ private SpellCheckingCharSequenceNormalizer(Builder b) {
* @return a new {@link Builder} seeded with sensible defaults
*/
public static Builder builder(SpellChecker spellChecker) {
- return new Builder(Objects.requireNonNull(spellChecker, "spellChecker must
not be null"));
+ if (spellChecker == null) {
+ throw new IllegalArgumentException("spellChecker must not be null");
+ }
+ return new Builder(spellChecker);
}
/**
* @param model the loaded model whose engine to wrap; must not be {@code
null}
* @return a new {@link Builder} seeded with sensible defaults
*/
public static Builder builder(SymSpellModel model) {
- return new Builder(Objects.requireNonNull(model, "model must not be
null").getSymSpell());
+ if (model == null) {
+ throw new IllegalArgumentException("model must not be null");
Review Comment:
This needs to be mentioned in Javadoc! Please add `@throws` for
IllegalArgumentException.
##########
opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/util/normalizer/UrlCharSequenceNormalizerCharacterizationTest.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package opennlp.tools.util.normalizer;
+
+import java.util.Random;
+import java.util.regex.Pattern;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+/**
+ * Characterization tests for {@link UrlCharSequenceNormalizer}.
+ *
+ * <p>The fixed expectations below were probed against the regex implementation
+ * ({@code "https?://[-_.?&~;+=/#0-9A-Za-z]+"} to a space, then
+ * {@code "(?<![-+_.0-9A-Za-z])[-+_.0-9A-Za-z]+@[-0-9A-Za-z]+[-.0-9A-Za-z]+"}
to a space) and pin
+ * the accept/reject boundary byte for byte. Notable sharp edges kept
visible:</p>
+ * <ul>
+ * <li>the scheme is lowercase-only, and a URL body needs at least one
character out of the
+ * body class, so {@code "http:// x"} and {@code "http://"} pass through
untouched;</li>
+ * <li>the body class has no colon, percent sign, at sign, or non-ASCII, so
ports, escapes,
+ * userinfo, and IDN labels cut the match short;</li>
+ * <li>a mail match needs a local part run bounded on the left by the
lookbehind, and a domain
+ * of at least two characters starting with no dot: {@code "a@b c"} does
not match, while
+ * {@code "a@b."} swallows the trailing dot;</li>
+ * <li>after a failed candidate the scan resumes one char further, so {@code
"a@[email protected]"}
+ * still finds the inner address.</li>
+ * </ul>
+ */
+public class UrlCharSequenceNormalizerCharacterizationTest {
+
+ private static final UrlCharSequenceNormalizer NORMALIZER =
+ UrlCharSequenceNormalizer.getInstance();
+
+ private static void check(String input, String expected) {
+ assertEquals(expected, NORMALIZER.normalize(input).toString());
+ }
+
+ @Test
+ void urlsBecomeASingleSpace() {
+ check("", "");
+ check("asdf http://asdf.com/dfa/cxs 2nnfdf", "asdf 2nnfdf");
+ check("asdf http://asdf.com/dfa/cxs 2nnfdf http://asdf.com/dfa/cxs";, "asdf
2nnfdf ");
+ check("http://example.com";, " ");
+ check("visit http://example.com now", "visit now");
+ check("https://a.b/c?d=e&f=g#h+i~j;k=l_m-n";, " ");
+ check("https://x";, " ");
+ check("http://EXAMPLE.com";, " ");
+ check("xhttp://y";, "x ");
+ check("ahttp://x";, "a ");
+ }
+
+ @Test
+ void urlRecognitionRejectsAtTheExactRegexBoundary() {
+ check("http:// x", "http:// x");
+ check("http://";, "http://";);
+ check("HTTP://X.COM", "HTTP://X.COM");
+ check("httpss://x", "httpss://x");
+ check("ftp://x";, "ftp://x";);
+ check("www.example.com", "www.example.com");
+ // The body class has no colon, percent sign, or at sign, and no non-ASCII.
+ check("http://x:8080/y";, " :8080/y");
+ check("http://a%20b";, " %20b");
+ check("http://http://x";, " ://x");
+ check("http://\u043F\u0440\u0438\u043C\u0435\u0440";,
"http://\u043F\u0440\u0438\u043C\u0435\u0440";);
+ check("http://x.\u043F\u0440\u0438\u043C\u0435\u0440";, "
\u043F\u0440\u0438\u043C\u0435\u0440");
+ check("http://[email protected]";, " @example.com");
+ }
+
+ @Test
+ void mailAddressesBecomeASingleSpace() {
+ check("asdf [email protected] 2nnfdf", "asdf 2nnfdf");
+ check("asdf [email protected] 2nnfdf [email protected]", "asdf
2nnfdf ");
+ check("asdf [email protected] 2nnfdf", "asdf 2nnfdf");
+ check("asdf [email protected]_br 2nnfdf", "asdf _br 2nnfdf");
+ check("[email protected]", " ");
+ check("[email protected] end", " end");
+ check("[email protected]", " ");
+ check("[email protected]", " ");
+ check("a@-b", " ");
+ check("[email protected]", " ");
+ check("x@y-.", " ");
+ check("mailto:[email protected]";, "mailto: ");
+ check("[email protected],[email protected]", " , ");
+ check("a [email protected] f", "a f");
+ check("see http://x.y, then [email protected]!", "see , then !");
+ }
+
+ @Test
+ void mailRecognitionRejectsAtTheExactRegexBoundary() {
+ // A one-char domain with nothing to lend to the second domain class fails.
+ check("a@b c", "a@b c");
+ check("a@bc d", " d");
+ // The trailing dot is part of the second domain class and gets swallowed.
+ check("a@b.", " ");
+ check("[email protected].", " ");
+ check("Contact [email protected].", "Contact ");
+ // The domain must not start with a dot.
+ check("[email protected]", "[email protected]");
+ check("[email protected]", "[email protected]");
+ // The local part must be a maximal run: no match can start inside one.
+ check("user@@example.com", "user@@example.com");
+ check("@example.com", "@example.com");
+ // After the failed candidate at "a", the scan resumes and finds
"[email protected]".
+ check("a@[email protected]", "a@ ");
+ }
+
+ @Test
+ void lookbehindOnlySeesTheSingleCharBeforeTheLocalPart() {
+ // Non-ASCII (including a low surrogate) is outside the lookbehind class,
so the address
+ // right after it still matches.
+ check("\uD83D\[email protected]", "\uD83D\uDE00 ");
+ check("\[email protected]", "\u00E9 ");
+ }
+
+ @Test
+ void nullTextIsRejected() {
+ // Null input is rejected with IllegalArgumentException.
+ assertThrows(IllegalArgumentException.class, () ->
NORMALIZER.normalize(null));
+ }
+
+ @Test
+ void matchesTheFormerRegexesOnRandomizedInputs() {
+ final Pattern urlRegex =
Pattern.compile("https?://[-_.?&~;+=/#0-9A-Za-z]+");
+ final Pattern mailRegex =
+
Pattern.compile("(?<![-+_.0-9A-Za-z])[-+_.0-9A-Za-z]+@[-0-9A-Za-z]+[-.0-9A-Za-z]+");
+ final String[] pool = {"a", "b", "Z", "1", " ", ".", ",", "-", "+", "_",
"@", ":", "/", "%",
+ "~", ";", "=", "&", "?", "#", "http", "https", "ttp", "://",
"http://";, "s", "x.com",
+ "co", ".com", "a@b", "@b.co", "\u00E9", "\uD83D\uDE00", "\uD83D",
"\uDE00"};
+ final Random random = new Random(42);
+ for (int i = 0; i < 5000; i++) {
+ final String input = CharacterizationInputs.randomInput(random, pool);
+ final String expected = mailRegex
+ .matcher(urlRegex.matcher(input).replaceAll(" "))
+ .replaceAll(" ");
+ assertEquals(expected, NORMALIZER.normalize(input).toString(),
+ () -> "Input: " + CharacterizationInputs.escape(input));
+ }
+ }
+ @Test
+ void noMatchInputIsReturnedUncopied() {
+ final String plain = "no links in this sentence at all";
+ Assertions.assertSame(plain, NORMALIZER.normalize(plain));
+ }
+
+ @Test
+ void weirdUrlsMatchTheFormerRegexExactly() {
+ // Adversarial shapes in the spirit of the web-platform-tests URL suite:
userinfo, ports,
+ // percent escapes, IPv6 brackets, backslashes, IDN and punycode hosts,
stray schemes and
+ // separators. Each input runs differentially through the two former
patterns, so the
+ // accept/reject boundary is pinned by construction rather than by hand.
+ final Pattern urls = Pattern.compile("https?://[-_.?&~;+=/#0-9A-Za-z]+");
+ final Pattern mails =
Review Comment:
This pattern is repeated here, see line 142. Can we please extract this
pattern on others in this test cases into constant values? This avoid
duplication and bloat of the actual test case / scenario.
Please re-iterate over all other new or changed tests for similar potential,
that is extract all `Pattern` instances from local method to (test) class
constants.
##########
opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/normalizer/SpellCheckingCharSequenceNormalizer.java:
##########
@@ -181,13 +171,17 @@ public SpellCheckingCharSequenceNormalizer
withSpellChecker(SpellChecker checker
.build();
}
+ /** {@inheritDoc} */
@Override
public CharSequence normalize(CharSequence text) {
if (spellChecker == null) {
throw new IllegalStateException("no SpellChecker attached; this instance
was likely "
+ "restored by Java deserialization. Re-attach one via
withSpellChecker(...).");
}
- if (text == null || text.isEmpty()) {
+ if (text == null) {
+ throw new IllegalArgumentException("The text must not be null.");
Review Comment:
This needs to be mentioned in Javadoc! Please add `@throws` for
IllegalArgumentException.
##########
opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/stream/SpellCorrectingTokenStream.java:
##########
@@ -33,11 +31,9 @@
* (whitespace by default). Every token is spell-corrected independently and
the tokens
* are re-joined with the same delimiter.
*
- * <p>This is the shape produced by OpenNLP tokenizers / token-sample formats
and is
- * what the trainable components consume: a fixed sequence of tokens per
element. Unlike
- * {@link SpellCorrectingObjectStream} in compound mode, this stream is
- * <em>token-count preserving</em> – it never splits or merges tokens,
so the
- * corrected element stays aligned with any parallel annotation (tags,
spans).</p>
+ * <p>This is the shape produced by OpenNLP tokenizers and token-sample
formats, a fixed
Review Comment:
The Javadoc of this class was shortened. Why? Is there a specific reason? If
not: Please re-establish the previous, long(er) version of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
